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

Eric Paris
2009-05-07T20:36:26Z

On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
<sukadev@linux.vnet.ibm.com> wrote:
>
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

I just tried to load the linux-next kernel on F11 and ran into a
problem. X started, I could log in, I could start programs like
firefox and evolution, but not gnome-terminal. It would just flash
and disappear. Running xterm resulted in a window, that I could type
in, but it wasn't a shell. It didn't do anything.

I switched to vt2 set the display to my X session and tried to run
xterm. It said something about a permission being denied, so I
decided to strace it. I saw EACCESS returning from calls dealing with
/dev/pts/0. This lead me to git bisect start fs/devpts from the
latest in linux-next as bad and 2.6.29 as good. Couple interations
later and I find that this commit (1bd7903560f1f7) breaks
gnome-terminal xterm!

So I have no idea why and haven't tried to figure it out (I'll try to
poke it more tonight), but this is the commit that git bisect blames!

> 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[v3]:
>        [Serge Hallyn]: Remove unnecessary printk()s
> 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>
> ---
>  fs/devpts/inode.c |  115 ++++++++++++++++++----------------------------------
>  1 files changed, 40 insertions(+), 75 deletions(-)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index c821c9a..c25fb34 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.
>  *
> - *     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.
> + *     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.
>  *
> - *     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().
> + *     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().
>  *
> - *     Mounts with 'newinstance' option create a new private namespace.
> + *     Mounts with 'newinstance' option create a new, private namespace.
>  *
> - *     But for single-mount semantics, devpts cannot use get_sb_single(),
> + *     NOTE:
> + *
> + *     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) {
> @@ -415,23 +366,37 @@ static int devpts_get_sb(struct file_system_type *fs_type,
>        }
>
>        if (opts.newinstance)
> -               error = new_pts_mount(fs_type, flags, data, &opts, mnt);
> +               s = sget(fs_type, NULL, set_anon_super, NULL);
>        else
> -               error = init_pts_mount(fs_type, flags, data, &opts, mnt);
> +               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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>