2009-06-23 20:13:26

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH 2/5] VFS: Add VFS helper functions for setting up private namespaces

Quoting Trond Myklebust ([email protected]):
> The purpose of this patch is to improve the remote mount path lookup
> support for distributed filesystems such as the NFSv4 client.
>
> When given a mount command of the form "mount server:/foo/bar /mnt", the
> NFSv4 client is required to look up the filehandle for "server:/", and
> then look up each component of the remote mount path "foo/bar" in order
> to find the directory that is actually going to be mounted on /mnt.
> Following that remote mount path may involve following symlinks,
> crossing server-side mount points and even following referrals to
> filesystem volumes on other servers.
>
> Since the standard VFS path lookup code already supports walking paths
> that contain all these features (using in-kernel automounts for
> following referrals) we would like to be able to reuse that rather than
> duplicate the full path traversal functionality in the NFSv4 client code.
>
> This patch therefore defines a VFS helper function create_mnt_ns(), that
> sets up a temporary filesystem namespace and attaches a root filesystem to
> it. It exports the create_mnt_ns() and put_mnt_ns() function for use by
> filesystem modules.
>
> Signed-off-by: Trond Myklebust <[email protected]>

This looks good, thanks. Though I see no reason not to also switch over
init_mount_tree() to the new helper.

(Seems plausible that c/r code would use this as well)

Reviewed-by: Serge Hallyn <[email protected]>

thanks,
-serge

> ---
>
> fs/namespace.c | 45 ++++++++++++++++++++++++++++++++++-------
> include/linux/mnt_namespace.h | 1 +
> 2 files changed, 38 insertions(+), 8 deletions(-)
>
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 2465c05..9b766b0 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1937,6 +1937,21 @@ dput_out:
> return retval;
> }
>
> +static struct mnt_namespace *alloc_mnt_ns(void)
> +{
> + struct mnt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
> + if (!new_ns)
> + return ERR_PTR(-ENOMEM);
> + atomic_set(&new_ns->count, 1);
> + new_ns->root = NULL;
> + INIT_LIST_HEAD(&new_ns->list);
> + init_waitqueue_head(&new_ns->poll);
> + new_ns->event = 0;
> + return new_ns;
> +}
> +
> /*
> * Allocate a new namespace structure and populate it with contents
> * copied from the namespace of the passed in task structure.
> @@ -1948,14 +1963,9 @@ static struct mnt_namespace *dup_mnt_ns(struct mnt_namespace *mnt_ns,
> struct vfsmount *rootmnt = NULL, *pwdmnt = NULL;
> struct vfsmount *p, *q;
>
> - new_ns = kmalloc(sizeof(struct mnt_namespace), GFP_KERNEL);
> - if (!new_ns)
> - return ERR_PTR(-ENOMEM);
> -
> - atomic_set(&new_ns->count, 1);
> - INIT_LIST_HEAD(&new_ns->list);
> - init_waitqueue_head(&new_ns->poll);
> - new_ns->event = 0;
> + new_ns = alloc_mnt_ns();
> + if (IS_ERR(new_ns))
> + return new_ns;
>
> down_write(&namespace_sem);
> /* First pass: copy the tree topology */
> @@ -2019,6 +2029,24 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
> return new_ns;
> }
>
> +/**
> + * create_mnt_ns - creates a private namespace and adds a root filesystem
> + * @mnt: pointer to the new root filesystem mountpoint
> + */
> +struct mnt_namespace *create_mnt_ns(struct vfsmount *mnt)
> +{
> + struct mnt_namespace *new_ns;
> +
> + new_ns = alloc_mnt_ns();
> + if (!IS_ERR(new_ns)) {
> + mnt->mnt_ns = new_ns;
> + new_ns->root = mnt;
> + list_add(&new_ns->list, &new_ns->root->mnt_list);
> + }
> + return new_ns;
> +}
> +EXPORT_SYMBOL(create_mnt_ns);
> +
> SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
> char __user *, type, unsigned long, flags, void __user *, data)
> {
> @@ -2264,3 +2292,4 @@ void put_mnt_ns(struct mnt_namespace *ns)
> release_mounts(&umount_list);
> kfree(ns);
> }
> +EXPORT_SYMBOL(put_mnt_ns);
> diff --git a/include/linux/mnt_namespace.h b/include/linux/mnt_namespace.h
> index 299d11a..3beb259 100644
> --- a/include/linux/mnt_namespace.h
> +++ b/include/linux/mnt_namespace.h
> @@ -24,6 +24,7 @@ struct proc_mounts {
>
> struct fs_struct;
>
> +extern struct mnt_namespace *create_mnt_ns(struct vfsmount *mnt);
> extern struct mnt_namespace *copy_mnt_ns(unsigned long, struct mnt_namespace *,
> struct fs_struct *);
> extern void put_mnt_ns(struct mnt_namespace *ns);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html


2009-06-23 21:30:11

by Myklebust, Trond

[permalink] [raw]
Subject: Re: [PATCH 2/5] VFS: Add VFS helper functions for setting up private namespaces

On Tue, 2009-06-23 at 15:13 -0500, Serge E. Hallyn wrote:
> Quoting Trond Myklebust ([email protected]):
> > The purpose of this patch is to improve the remote mount path lookup
> > support for distributed filesystems such as the NFSv4 client.
> >
> > When given a mount command of the form "mount server:/foo/bar /mnt", the
> > NFSv4 client is required to look up the filehandle for "server:/", and
> > then look up each component of the remote mount path "foo/bar" in order
> > to find the directory that is actually going to be mounted on /mnt.
> > Following that remote mount path may involve following symlinks,
> > crossing server-side mount points and even following referrals to
> > filesystem volumes on other servers.
> >
> > Since the standard VFS path lookup code already supports walking paths
> > that contain all these features (using in-kernel automounts for
> > following referrals) we would like to be able to reuse that rather than
> > duplicate the full path traversal functionality in the NFSv4 client code.
> >
> > This patch therefore defines a VFS helper function create_mnt_ns(), that
> > sets up a temporary filesystem namespace and attaches a root filesystem to
> > it. It exports the create_mnt_ns() and put_mnt_ns() function for use by
> > filesystem modules.
> >
> > Signed-off-by: Trond Myklebust <[email protected]>
>
> This looks good, thanks. Though I see no reason not to also switch over
> init_mount_tree() to the new helper.
>
> (Seems plausible that c/r code would use this as well)
>
> Reviewed-by: Serge Hallyn <[email protected]>
>
> thanks,
> -serge

Thanks for the review! I missed the code duplication in
init_mount_tree(). Something like the following?

Cheers
Trond
--------------------------------------------------------------------
From: Trond Myklebust <[email protected]>
VFS: Switch init_mount_tree() to use the new create_mnt_ns() helper

Eliminates some duplicated code...

Signed-off-by: Trond Myklebust <[email protected]>
---

fs/namespace.c | 11 ++---------
1 files changed, 2 insertions(+), 9 deletions(-)


diff --git a/fs/namespace.c b/fs/namespace.c
index a7bea8c..4a86b85 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -2222,16 +2222,9 @@ static void __init init_mount_tree(void)
mnt = do_kern_mount("rootfs", 0, "rootfs", NULL);
if (IS_ERR(mnt))
panic("Can't create rootfs");
- ns = kmalloc(sizeof(*ns), GFP_KERNEL);
- if (!ns)
+ ns = create_mnt_ns(mnt);
+ if (IS_ERR(ns))
panic("Can't allocate initial namespace");
- atomic_set(&ns->count, 1);
- INIT_LIST_HEAD(&ns->list);
- init_waitqueue_head(&ns->poll);
- ns->event = 0;
- list_add(&mnt->mnt_list, &ns->list);
- ns->root = mnt;
- mnt->mnt_ns = ns;

init_task.nsproxy->mnt_ns = ns;
get_mnt_ns(ns);


--
Trond Myklebust
Linux NFS client maintainer

NetApp
[email protected]
http://www.netapp.com

2009-06-23 22:32:30

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH 2/5] VFS: Add VFS helper functions for setting up private namespaces

Quoting Trond Myklebust ([email protected]):
> On Tue, 2009-06-23 at 15:13 -0500, Serge E. Hallyn wrote:
> > Quoting Trond Myklebust ([email protected]):
> > > The purpose of this patch is to improve the remote mount path lookup
> > > support for distributed filesystems such as the NFSv4 client.
> > >
> > > When given a mount command of the form "mount server:/foo/bar /mnt", the
> > > NFSv4 client is required to look up the filehandle for "server:/", and
> > > then look up each component of the remote mount path "foo/bar" in order
> > > to find the directory that is actually going to be mounted on /mnt.
> > > Following that remote mount path may involve following symlinks,
> > > crossing server-side mount points and even following referrals to
> > > filesystem volumes on other servers.
> > >
> > > Since the standard VFS path lookup code already supports walking paths
> > > that contain all these features (using in-kernel automounts for
> > > following referrals) we would like to be able to reuse that rather than
> > > duplicate the full path traversal functionality in the NFSv4 client code.
> > >
> > > This patch therefore defines a VFS helper function create_mnt_ns(), that
> > > sets up a temporary filesystem namespace and attaches a root filesystem to
> > > it. It exports the create_mnt_ns() and put_mnt_ns() function for use by
> > > filesystem modules.
> > >
> > > Signed-off-by: Trond Myklebust <[email protected]>
> >
> > This looks good, thanks. Though I see no reason not to also switch over
> > init_mount_tree() to the new helper.
> >
> > (Seems plausible that c/r code would use this as well)
> >
> > Reviewed-by: Serge Hallyn <[email protected]>
> >
> > thanks,
> > -serge
>
> Thanks for the review! I missed the code duplication in
> init_mount_tree(). Something like the following?

Yup.

(it keeps bugging me that the order of args to list_add() gets reversed
as a result, but clearly with both starting out empty it doesn't
matter..)

thanks,
-serge

> Cheers
> Trond
> --------------------------------------------------------------------
> From: Trond Myklebust <[email protected]>
> VFS: Switch init_mount_tree() to use the new create_mnt_ns() helper
>
> Eliminates some duplicated code...
>
> Signed-off-by: Trond Myklebust <[email protected]>
> ---
>
> fs/namespace.c | 11 ++---------
> 1 files changed, 2 insertions(+), 9 deletions(-)
>
>
> diff --git a/fs/namespace.c b/fs/namespace.c
> index a7bea8c..4a86b85 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2222,16 +2222,9 @@ static void __init init_mount_tree(void)
> mnt = do_kern_mount("rootfs", 0, "rootfs", NULL);
> if (IS_ERR(mnt))
> panic("Can't create rootfs");
> - ns = kmalloc(sizeof(*ns), GFP_KERNEL);
> - if (!ns)
> + ns = create_mnt_ns(mnt);
> + if (IS_ERR(ns))
> panic("Can't allocate initial namespace");
> - atomic_set(&ns->count, 1);
> - INIT_LIST_HEAD(&ns->list);
> - init_waitqueue_head(&ns->poll);
> - ns->event = 0;
> - list_add(&mnt->mnt_list, &ns->list);
> - ns->root = mnt;
> - mnt->mnt_ns = ns;
>
> init_task.nsproxy->mnt_ns = ns;
> get_mnt_ns(ns);
>
>
> --
> Trond Myklebust
> Linux NFS client maintainer
>
> NetApp
> [email protected]
> http://www.netapp.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2009-06-24 02:52:11

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 2/5] VFS: Add VFS helper functions for setting up private namespaces



On Tue, 23 Jun 2009, Serge E. Hallyn wrote:
>
> (it keeps bugging me that the order of args to list_add() gets reversed
> as a result, but clearly with both starting out empty it doesn't
> matter..)

.. and the new one is more logical. "list_add()" really adds the first
entry to the list pointed to by the second one. It _works_ the other way
too in this case, as you point out, but now that you mention it, it really
looks like the pre-patch code is "wrong".

Linus