[RFC][PATCH 0/5] Simplify devpts code

[RFC][PATCH 0/5] Simplify devpts code

[Sukadev Bhattiprolu]

This patchset tries to address Christoph Hellwig's review comments on the
support for multiple-instances in devpts.  http://lkml.org/lkml/2009/1/3/84
It breaks up the patch into smaller pieces for (hopefully)  easier review.

IIUC, the key observation was that most of do_remount_sb() (MS_RDONLY flag
and shrink_dcache() does not apply to devpts and only the parsing of
options does. Moving the parsing code into devpts enables us to parse
the options just once and vastly simplifies the code.

This patchset does not change any functionality/behavior. But it does
depend on following two related patches.

	http://lkml.org/lkml/2009/1/29/10
	http://lkml.org/lkml/2009/1/29/11

Patches in this set:

	[PATCH 1/5] Unroll essentials of do_remount_sb() into devpts
	[PATCH 2/5] Parse mount options just once and copy them to super block
	[PATCH 3/5] Move common mknod_ptmx() calls into caller
	[PATCH 4/5] Remove get_init_pts_sb()
	[PATCH 5/5] Merge code for single and multiple-instance mounts

Touch tested. Bisect safe. Needs more testing.

TODO: (possibly as a separate patch ?)
	- Merge CONFIG_DEVPTS_MULTIPLE_INSTANCES token with other container-
	  related tokens ? 

[RFC][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 27 Jan 2009 22:58:18 -0800
Subject: [RFC][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

On remount, devpts fs only needs to parse the mount options. Users cannot
directly create/dirty files in /dev/pts so the MS_RDONLY flag and
shrinking the dcache does not really apply to devpts.

So effectively on remount, devpts only parses the mount options and updates
these options in its super block. As such, we could replace do_remount_sb()
call with a direct parse_mount_options().

Doing so enables subsequent patches to avoid parsing the mount options twice
and simplify the code.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index ad186b4..de15e73 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -442,6 +442,8 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		void *data, struct vfsmount *mnt)
 {
 	struct super_block *s;
+	struct pts_mount_opts *opts;
+	struct pts_fs_info *fsi;
 	int error;
 
 	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
@@ -458,7 +460,10 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
+	fsi = DEVPTS_SB(s);
+	opts = &fsi->mount_opts;
+	parse_mount_options(data, PARSE_REMOUNT, opts);
+
 	simple_set_mnt(mnt, s);
 	return 0;
 }
-- 
1.5.2.5

Re: [RFC][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

[Christoph Hellwig]

On Wed, Jan 28, 2009 at 10:45:47PM -0800, Sukadev Bhattiprolu wrote:
> +	fsi = DEVPTS_SB(s);
> +	opts = &fsi->mount_opts;
> +	parse_mount_options(data, PARSE_REMOUNT, opts);

You could probably get rid of at least the fsi variable without making the
code less readable.  Independent of these cosmetic bits the patch looks
good to me.

[RFC][PATCH 2/5] Parse mount options just once and copy them to super block

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Tue, 27 Jan 2009 23:15:47 -0800
Subject: [RFC][PATCH 2/5] Parse mount options just once and copy them to super block

Since all the mount option parsing is done in devpts, we could do it
just once and pass it around in devpts functions and eventually store
it in the super block.
---
 fs/devpts/inode.c |   99 +++++++++++-----------------------------------------
 1 files changed, 21 insertions(+), 78 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index de15e73..8e00779 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -325,49 +325,14 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
 }
 
 /*
- * Safely parse the mount options in @data and update @opts.
- *
- * devpts ends up parsing options two times during mount, due to the
- * two modes of operation it supports. The first parse occurs in
- * devpts_get_sb() when determining the mode (single-instance or
- * multi-instance mode). The second parse happens in devpts_remount()
- * or new_pts_mount() depending on the mode.
- *
- * Parsing of options modifies the @data making subsequent parsing
- * incorrect. So make a local copy of @data and parse it.
- *
- * Return: 0 On success, -errno on error
- */
-static int safe_parse_mount_options(void *data, struct pts_mount_opts *opts)
-{
-	int rc;
-	void *datacp;
-
-	if (!data)
-		return 0;
-
-	/* Use kstrdup() ?  */
-	datacp = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!datacp)
-		return -ENOMEM;
-
-	memcpy(datacp, data, PAGE_SIZE);
-	rc = parse_mount_options((char *)datacp, PARSE_MOUNT, opts);
-	kfree(datacp);
-
-	return rc;
-}
-
-/*
  * Mount a new (private) instance of devpts.  PTYs created in this
  * instance are independent of the PTYs in other devpts instances.
  */
 static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 	struct pts_fs_info *fsi;
-	struct pts_mount_opts *opts;
 
 	printk(KERN_NOTICE "devpts: newinstance mount\n");
 
@@ -376,11 +341,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 		return err;
 
 	fsi = DEVPTS_SB(mnt->mnt_sb);
-	opts = &fsi->mount_opts;
-
-	err = parse_mount_options(data, PARSE_MOUNT, opts);
-	if (err)
-		goto fail;
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
 
 	err = mknod_ptmx(mnt->mnt_sb);
 	if (err)
@@ -396,28 +357,6 @@ fail:
 }
 
 /*
- * Check if 'newinstance' mount option was specified in @data.
- *
- * Return: -errno  	on error (eg: invalid mount options specified)
- * 	 : 1 		if 'newinstance' mount option was specified
- * 	 : 0 		if 'newinstance' mount option was NOT specified
- */
-static int is_new_instance_mount(void *data)
-{
-	int rc;
-	struct pts_mount_opts opts;
-
-	if (!data)
-		return 0;
-
-	rc = safe_parse_mount_options(data, &opts);
-	if (!rc)
-		rc = opts.newinstance;
-
-	return rc;
-}
-
-/*
  * get_init_pts_sb()
  *
  *     This interface is needed to support multiple namespace semantics in
@@ -439,10 +378,9 @@ static int is_new_instance_mount(void *data)
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
 static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
-	struct pts_mount_opts *opts;
 	struct pts_fs_info *fsi;
 	int error;
 
@@ -460,11 +398,12 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	fsi = DEVPTS_SB(s);
-	opts = &fsi->mount_opts;
-	parse_mount_options(data, PARSE_REMOUNT, opts);
 
 	simple_set_mnt(mnt, s);
+
+	fsi = DEVPTS_SB(mnt->mnt_sb);
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
+
 	return 0;
 }
 
@@ -474,11 +413,11 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
  * kernel still allows multiple-instances.
  */
 static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 
-	err = get_init_pts_sb(fs_type, flags, data, mnt);
+	err = get_init_pts_sb(fs_type, flags, data, opts, mnt);
 	if (err)
 		return err;
 
@@ -495,17 +434,21 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-	int new;
-
-	new = is_new_instance_mount(data);
-	if (new < 0)
-		return new;
+	int error;
+	struct pts_mount_opts opts = { 0, }; // check
 
-	if (new)
-		return new_pts_mount(fs_type, flags, data, mnt);
+	if (data) {
+		error = parse_mount_options(data, PARSE_MOUNT, &opts);
+		if (error)
+			return error;
+	}
 
-	return init_pts_mount(fs_type, flags, data, mnt);
+	if (opts.newinstance)
+		return new_pts_mount(fs_type, flags, data, &opts, mnt);
+	else
+		return init_pts_mount(fs_type, flags, data, &opts, mnt);
 }
+
 #else
 /*
  * This supports only the legacy single-instance semantics (no
-- 
1.5.2.5

Re: [RFC][PATCH 2/5] Parse mount options just once and copy them to super block

[Christoph Hellwig]

Looks good (once the // check comment is cleaned up)

[RFC][PATCH 3/5] Move common mknod_ptmx() calls into caller

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 28 Jan 2009 18:54:25 -0800
Subject: [RFC][PATCH 3/5] Move common mknod_ptmx() calls into caller

We create 'ptmx' node in both single-instance and multiple-instance
mounts. So devpts_get_sb() can call mknod_ptmx() once rather than
have both modes calling mknod_ptmx() separately.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 8e00779..97b8e9f 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -343,17 +343,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 	fsi = DEVPTS_SB(mnt->mnt_sb);
 	memcpy(&fsi->mount_opts, opts, sizeof(opts));
 
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err)
-		goto fail;
-
 	return 0;
-
-fail:
-	dput(mnt->mnt_sb->s_root);
-	up_write(&mnt->mnt_sb->s_umount);
-	deactivate_super(mnt->mnt_sb);
-	return err;
 }
 
 /*
@@ -421,13 +411,6 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 	if (err)
 		return err;
 
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err) {
-		dput(mnt->mnt_sb->s_root);
-		up_write(&mnt->mnt_sb->s_umount);
-		deactivate_super(mnt->mnt_sb);
-	}
-
 	return err;
 }
 
@@ -444,9 +427,24 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 	}
 
 	if (opts.newinstance)
-		return new_pts_mount(fs_type, flags, data, &opts, mnt);
+		error = new_pts_mount(fs_type, flags, data, &opts, mnt);
 	else
-		return init_pts_mount(fs_type, flags, data, &opts, mnt);
+		error = init_pts_mount(fs_type, flags, data, &opts, mnt);
+
+	if (error)
+		return error;
+
+	error = mknod_ptmx(mnt->mnt_sb);
+	if (error)
+		goto out_dput;
+
+	return 0;
+
+out_dput:
+	dput(mnt->mnt_sb->s_root);
+	up_write(&mnt->mnt_sb->s_umount);
+	deactivate_super(mnt->mnt_sb);
+	return error;
 }
 
 #else
-- 
1.5.2.5

Re: [RFC][PATCH 3/5] Move common mknod_ptmx() calls into caller

[Christoph Hellwig]

On Wed, Jan 28, 2009 at 10:46:43PM -0800, Sukadev Bhattiprolu wrote:
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 28 Jan 2009 18:54:25 -0800
> Subject: [RFC][PATCH 3/5] Move common mknod_ptmx() calls into caller
> 
> We create 'ptmx' node in both single-instance and multiple-instance
> mounts. So devpts_get_sb() can call mknod_ptmx() once rather than
> have both modes calling mknod_ptmx() separately.

Looks good.

[RFC][PATCH 4/5] Remove get_init_pts_sb()

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 28 Jan 2009 18:59:28 -0800
Subject: [RFC][PATCH 4/5] Remove get_init_pts_sb()

With mknod_ptmx() moved to devpts_get_sb(), init_pts_mount() becomes
a wrapper around get_init_pts_sb(). Remove get_init_pts_sb() and
fold code into init_pts_mount().

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 97b8e9f..733e8e0 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -347,7 +347,11 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 }
 
 /*
- * get_init_pts_sb()
+ * init_pts_mount()
+ *
+ *     Mount or remount the initial kernel mount of devpts. This type of
+ *     mount maintains the legacy, single-instance semantics, while the
+ *     kernel still allows multiple-instances.
  *
  *     This interface is needed to support multiple namespace semantics in
  *     devpts while preserving backward compatibility of the current 'single-
@@ -367,7 +371,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
  *     consistently selects the 'single-namespace' superblock even in the
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
-static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
+static int init_pts_mount(struct file_system_type *fs_type, int flags,
 		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
@@ -397,23 +401,6 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 	return 0;
 }
 
-/*
- * Mount or remount the initial kernel mount of devpts. This type of
- * mount maintains the legacy, single-instance semantics, while the
- * kernel still allows multiple-instances.
- */
-static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
-{
-	int err;
-
-	err = get_init_pts_sb(fs_type, flags, data, opts, mnt);
-	if (err)
-		return err;
-
-	return err;
-}
-
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-- 
1.5.2.5

Re: [RFC][PATCH 4/5] Remove get_init_pts_sb()

[Christoph Hellwig]

Looks good.

[RFC][PATCH 5/5] Merge code for single and multiple-instance mounts

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Wed, 28 Jan 2009 19:11:15 -0800
Subject: [RFC][PATCH 5/5] Merge code for single and multiple-instance mounts

new_pts_mount() (including the get_sb_nodev()), shares a lot of code
with init_pts_mount(). The only difference between them is the 'test-super'
function passed into sget().

Move the shared code into a new, common function, do_pts_mount() and
remove new_pts_mount() and init_pts_mount() functions,

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |   34 ++++++++++------------------------
 1 files changed, 10 insertions(+), 24 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 733e8e0..8d0b675 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -328,23 +328,6 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
  * Mount a new (private) instance of devpts.  PTYs created in this
  * instance are independent of the PTYs in other devpts instances.
  */
-static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
-{
-	int err;
-	struct pts_fs_info *fsi;
-
-	printk(KERN_NOTICE "devpts: newinstance mount\n");
-
-	err = get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
-	if (err)
-		return err;
-
-	fsi = DEVPTS_SB(mnt->mnt_sb);
-	memcpy(&fsi->mount_opts, opts, sizeof(opts));
-
-	return 0;
-}
 
 /*
  * init_pts_mount()
@@ -371,14 +354,21 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
  *     consistently selects the 'single-namespace' superblock even in the
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
-static int init_pts_mount(struct file_system_type *fs_type, int flags,
+static int do_pts_mount(struct file_system_type *fs_type, int flags,
 		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
 	struct pts_fs_info *fsi;
 	int error;
 
-	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
+	if (opts->newinstance) {
+		printk(KERN_NOTICE "devpts: newinstance mount\n");
+		s = sget(fs_type, NULL, set_anon_super, NULL);
+	} else {
+		printk(KERN_NOTICE "devpts: single-instance mount\n");
+		s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
+	}
+
 	if (IS_ERR(s))
 		return PTR_ERR(s);
 
@@ -413,11 +403,7 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 			return error;
 	}
 
-	if (opts.newinstance)
-		error = new_pts_mount(fs_type, flags, data, &opts, mnt);
-	else
-		error = init_pts_mount(fs_type, flags, data, &opts, mnt);
-
+	error = do_pts_mount(fs_type, flags, data, &opts, mnt);
 	if (error)
 		return error;
 
-- 
1.5.2.5

Re: [RFC][PATCH 5/5] Merge code for single and multiple-instance mounts

[Christoph Hellwig]

On Wed, Jan 28, 2009 at 10:47:53PM -0800, Sukadev Bhattiprolu wrote:
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 28 Jan 2009 19:11:15 -0800
> Subject: [RFC][PATCH 5/5] Merge code for single and multiple-instance mounts
> 
> new_pts_mount() (including the get_sb_nodev()), shares a lot of code
> with init_pts_mount(). The only difference between them is the 'test-super'
> function passed into sget().
> 
> Move the shared code into a new, common function, do_pts_mount() and
> remove new_pts_mount() and init_pts_mount() functions,

I would even merge init_pts_mount into devpts_get_sb, otherwise looks
good to me.