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

[PATCH 0/5][v2] 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

Changelog[v2]:
	- (Christoph Hellwig) Patch 2/5 remove '//check' comment. Initialized
	  object using memset().
	- (Christoph Hellwig) - Patch 5/5; Removed the do_pts_mount() function
	   and moved that code into devpts_get_sb()

TODO: (Independent patch set)

	- Merge CONFIG_DEVPTS_MULTIPLE_INSTANCES token with other container-
	  related tokens ? 

[v2][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: [v2][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

[v2][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: [v2][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 |  100 ++++++++++++-----------------------------------------
 1 files changed, 22 insertions(+), 78 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index de15e73..9b24de8 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,22 @@ 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;
 
-	if (new)
-		return new_pts_mount(fs_type, flags, data, mnt);
+	memset(&opts, 0, sizeof(opts)); 
+	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

[v2][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: [v2][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 9b24de8..f7e887c 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;
 }
 
@@ -445,9 +428,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

[v2][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: [v2][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 f7e887c..c821c9a 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

[v2][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: [v2][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 all common code into devpts_get_sb() and remove the new_pts_mount() and
init_pts_mount() functions,

Changelog[v2]:
	(Christoph Hellwig): Merge code in 'do_pts_mount()' into devpts_get_sb()

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

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c821c9a..6438782 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -325,87 +325,38 @@ 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()
+ * devpts_get_sb()
+ *
+ *     If the '-o newinstance' mount option was specified, mount a new
+ *     (private) instance of devpts.  PTYs created in this instance are
+ *     independent of the PTYs in other devpts instances.
+ *
+ *     If the '-o newinstance' option was not specified, mount/remount the
+ *     initial kernel mount of devpts.  This type of mount gives the
+ *     legacy, single-instance semantics.
  *
- *     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.
+ *     The 'newinstance' option is needed to support multiple namespace
+ *     semantics in devpts while preserving backward compatibility of the
+ *     current 'single-namespace' semantics. i.e all mounts of devpts
+ *     without the 'newinstance' mount option should bind to the initial
+ *     kernel mount, like get_sb_single().
  *
- *     This interface is needed to support multiple namespace semantics in
- *     devpts while preserving backward compatibility of the current 'single-
- *     namespace' semantics. i.e all mounts of devpts without the 'newinstance'
- *     mount option should bind to the initial kernel mount, like
- *     get_sb_single().
+ *     Mounts with 'newinstance' option create a new, private namespace.
  *
- *     Mounts with 'newinstance' option create a new private namespace.
+ *     NOTE:
  *
- *     But for single-mount semantics, devpts cannot use get_sb_single(),
+ *     For single-mount semantics, devpts cannot use get_sb_single(),
  *     because get_sb_single()/sget() find and use the super-block from
  *     the most recent mount of devpts. But that recent mount may be a
  *     'newinstance' mount and get_sb_single() would pick the newinstance
  *     super-block instead of the initial super-block.
- *
- *     This interface is identical to get_sb_single() except that it
- *     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,
-		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 (IS_ERR(s))
-		return PTR_ERR(s);
-
-	if (!s->s_root) {
-		s->s_flags = flags;
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error) {
-			up_write(&s->s_umount);
-			deactivate_super(s);
-			return error;
-		}
-		s->s_flags |= MS_ACTIVE;
-	}
-
-	simple_set_mnt(mnt, s);
-
-	fsi = DEVPTS_SB(mnt->mnt_sb);
-	memcpy(&fsi->mount_opts, opts, sizeof(opts));
-
-	return 0;
-}
-
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
 	int error;
 	struct pts_mount_opts opts;
+	struct super_block *s;
 
 	memset(&opts, 0, sizeof(opts)); 
 	if (data) {
@@ -414,24 +365,41 @@ 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);
+	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 (error)
-		return error;
+	if (IS_ERR(s))
+		return PTR_ERR(s);
 
-	error = mknod_ptmx(mnt->mnt_sb);
+	if (!s->s_root) {
+		s->s_flags = flags;
+		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
+		if (error)
+			goto out_undo_sget;
+		s->s_flags |= MS_ACTIVE;
+	}
+
+	simple_set_mnt(mnt, s);
+
+	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
+
+	error = mknod_ptmx(s);
 	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);
+	dput(s->s_root);
+
+out_undo_sget:
+	up_write(&s->s_umount);
+	deactivate_super(s);
 	return error;
 }
 
-- 
1.5.2.5

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

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Tue, 27 Jan 2009 22:58:18 -0800
> Subject: [v2][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.

You've dropped the update_ptmx_mode() which you used to do
inside devpts_remount().  Is that also on purpose?

> 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: [v2][PATCH 2/5] Parse mount options just once and copy them to super block

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Tue, 27 Jan 2009 23:15:47 -0800
> Subject: [v2][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 |  100 ++++++++++++-----------------------------------------
>  1 files changed, 22 insertions(+), 78 deletions(-)

Nice ratio of - to +....

> @@ -495,17 +434,22 @@ 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;
> 
> -	if (new)
> -		return new_pts_mount(fs_type, flags, data, mnt);
> +	memset(&opts, 0, sizeof(opts)); 
> +	if (data) {
> +		error = parse_mount_options(data, PARSE_MOUNT, &opts);

Is there any reason to keep the PARSE_MOUNT argument to
parse_mount_options?

> +		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: [v2][PATCH 3/5] Move common mknod_ptmx() calls into caller

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 28 Jan 2009 18:54:25 -0800
> Subject: [v2][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>

Acked-by: Serge Hallyn <serue@us.ibm.com>

And now I see, you could get rid of update_ptmx() because
mknod_ptmx() will be called again anyway.  So if you
haven't replied yet pls ignore my previous email where I
asked about that :)

thanks,
-serge

> ---
>  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 9b24de8..f7e887c 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;
>  }
> 
> @@ -445,9 +428,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: [v2][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue@us.ibm.com):
> Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> > 
> > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Date: Tue, 27 Jan 2009 22:58:18 -0800
> > Subject: [v2][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.
> 
> You've dropped the update_ptmx_mode() which you used to do
> inside devpts_remount().  Is that also on purpose?

Since it appears that mknod_ptmx() will be called anyway and
will do the same thing,

> > Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serue@us.ibm.com>

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

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 28 Jan 2009 18:59:28 -0800
> Subject: [v2][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>

Acked-by: Serge Hallyn <serue@us.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 f7e887c..c821c9a 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: [v2][PATCH 5/5] Merge code for single and multiple-instance mounts

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> 
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Wed, 28 Jan 2009 19:11:15 -0800
> Subject: [v2][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 all common code into devpts_get_sb() and remove the new_pts_mount() and
> init_pts_mount() functions,
> 
> Changelog[v2]:
> 	(Christoph Hellwig): Merge code in 'do_pts_mount()' into devpts_get_sb()
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serue@us.ibm.com>
Tested-by: Serge Hallyn <serue@us.ibm.com>

except that:

> @@ -414,24 +365,41 @@ 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);
> +	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");

These printks probably shouldn't stay in...

> +		s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
> +	}

thanks,
-serge

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

[Sukadev Bhattiprolu]

Sorry, I had not responded to this:

Serge E. Hallyn [serue@us.ibm.com] wrote:
| > -	if (new)
| > -		return new_pts_mount(fs_type, flags, data, mnt);
| > +	memset(&opts, 0, sizeof(opts)); 
| > +	if (data) {
| > +		error = parse_mount_options(data, PARSE_MOUNT, &opts);
| 
| Is there any reason to keep the PARSE_MOUNT argument to
| parse_mount_options?

Yes parse_mount_options() needs to know whether it is a MOUNT or 
REMOUNT operation - MOUNT operation should clear 'newinstance' to
default before parsing, but REMOUNT should not.