[PATCH 0/4] Allow NFS to use ordinary path lookup when mounting NFSv4

[PATCH 0/4] Allow NFS to use ordinary path lookup when mounting NFSv4

[Trond Myklebust]

The following series of patches contain a VFS change, and hence I'm sending
them via mail instead of as a git pull. The first two patches have been
posted earlier (see http://thread.gmane.org/gmane.linux.file-systems/29663).

To recap:
The first patch adds VFS support for walking the remote path, using a
temporary mount namespace to represent the server's namespace, so that
symlinks and referrals can be followed across remote filesystem and
server boundaries.
The second patch then uses this VFS helper in the NFSv4 mount code.

The last two patches fix issues with referrals that turned up during testing
at Connectathon.

Please apply after pulling the NFS client git tree as requested earlier today,
or you can pull the full set of changes from
   git pull git://git.linux-nfs.org/projects/trondmy/nfs-2.6.git vfs-changes

Cheers
  Trond


Trond Myklebust (4):
  VFS: Add a VFS helper function vfs_remote_path_lookup()
  NFSv4: Use vfs_path_lookup() instead of nfs4_path_walk()
  NFS: Fix nfs_path() to always return a '/' at the beginning of the
    path
  NFS: Correct the NFS mount path when following a referral

 fs/namei.c                    |   75 ++++++++++++++++-
 fs/namespace.c                |   56 ++++++++++--
 fs/nfs/namespace.c            |    5 +
 fs/nfs/super.c                |  192 ++++++++++++++++++++++++++++++++++++-----
 include/linux/mnt_namespace.h |    2 +
 include/linux/namei.h         |    2 +
 include/linux/nsproxy.h       |    1 +
 kernel/nsproxy.c              |   11 +++
 8 files changed, 314 insertions(+), 30 deletions(-)

[PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

The purpose of this patch is to improve the mount path lookup support for
filesystems such as NFSv4, which require you to look up a mount path
string in a remote server's namespace.

Traversing such a path is pretty much identical to walking a local path,
in that it may involve following symlinks and even following referrals to
volumes that reside on other servers. Since the standard VFS path lookup
code already supports all these features (using in-kernel automounts for
following referrals) it would be nice to be able to reuse that code rather
than special case the mount path lookup in the NFS client.

This patch therefore defines a VFS helper function that sets up a temporary
mount namespace to represent the server namespace, and has the current
task pivot into that prior to doing the path lookup. Upon completion, it
pivots back into the original namespace, and destroys the private one.

Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
---
 fs/namei.c                    |   75 ++++++++++++++++++++++++++++++++++++++++-
 fs/namespace.c                |   56 ++++++++++++++++++++++++++----
 include/linux/mnt_namespace.h |    2 +
 include/linux/namei.h         |    2 +
 include/linux/nsproxy.h       |    1 +
 kernel/nsproxy.c              |   11 ++++++
 6 files changed, 138 insertions(+), 9 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index d040ce1..d167f40 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -26,7 +26,7 @@
 #include <linux/security.h>
 #include <linux/ima.h>
 #include <linux/syscalls.h>
-#include <linux/mount.h>
+#include <linux/mnt_namespace.h>
 #include <linux/audit.h>
 #include <linux/capability.h>
 #include <linux/file.h>
@@ -1122,6 +1122,79 @@ int vfs_path_lookup(struct dentry *dentry, struct vfsmount *mnt,
 }
 
 /**
+ * vfs_remote_path_lookup - look up a path in a remote server namespace
+ * @dentry:  pointer to dentry of the base directory
+ * @mnt: pointer to vfs mount of the base directory
+ * @name: pointer to file name
+ * @flags: lookup flags
+ * @nd: pointer to nameidata
+ *
+ * This function creates a private mount namespace and sets 'mnt' as the
+ * root volume before looking up the path 'name'.
+ * It is intended for use by filesystems like NFSv4, which has a mount
+ * path that is relative to a remote server's namespace, and where walking
+ * that path may involve following referrals/links to volumes that reside
+ * on yet other servers. The resulting in-kernel automount can now be
+ * done safely as it affects the private namespace only.
+ */
+int vfs_remote_path_lookup(struct dentry *dentry,
+		struct vfsmount *mnt, const char *name,
+		unsigned int flags, struct nameidata *nd)
+{
+	struct nsproxy *new_nsproxy, *orig_nsproxy;
+	struct mnt_namespace *new_mnt_ns;
+	struct fs_struct *new_fs, *orig_fs;
+	int error = -ENOMEM;
+
+	new_fs = copy_fs_struct(current->fs);
+	if (new_fs == NULL)
+		goto out_err;
+
+	new_mnt_ns = create_private_mnt_ns(mnt, new_fs);
+	if (new_mnt_ns == NULL)
+		goto out_put_fs_struct;
+
+	/* Create a private copy of current->nsproxy */
+	new_nsproxy = unshare_current_nsproxy();
+	error = PTR_ERR(new_nsproxy);
+	if (IS_ERR(new_nsproxy))
+		goto out_put_mnt_ns;
+
+	/* ...and substitute the private mount namespace */
+	put_mnt_ns(new_nsproxy->mnt_ns);
+	new_nsproxy->mnt_ns = new_mnt_ns;
+	get_mnt_ns(new_mnt_ns);
+
+	/* Save the old nsproxy */
+	orig_nsproxy = current->nsproxy;
+	get_nsproxy(orig_nsproxy);
+
+	/* Pivot into the new mount namespace */
+	switch_task_namespaces(current, new_nsproxy);
+	task_lock(current);
+	orig_fs = current->fs;
+	current->fs = new_fs;
+	task_unlock(current);
+
+	error = vfs_path_lookup(dentry, mnt, name, flags, nd);
+
+	/* Pivot back into the original namespace */
+	task_lock(current);
+	current->fs = orig_fs;
+	task_unlock(current);
+	switch_task_namespaces(current, orig_nsproxy);
+
+out_put_mnt_ns:
+	put_mnt_ns(new_mnt_ns);
+
+out_put_fs_struct:
+	put_fs_struct(new_fs);
+out_err:
+	return error;
+}
+EXPORT_SYMBOL_GPL(vfs_remote_path_lookup);
+
+/**
  * path_lookup_open - lookup a file path with open intent
  * @dfd: the directory to use as base, or AT_FDCWD
  * @name: pointer to file name
diff --git a/fs/namespace.c b/fs/namespace.c
index 0a42e0e..1762e3d 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1969,6 +1969,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.
@@ -1980,14 +1995,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 */
@@ -2051,6 +2061,36 @@ struct mnt_namespace *copy_mnt_ns(unsigned long flags, struct mnt_namespace *ns,
 	return new_ns;
 }
 
+struct mnt_namespace *create_private_mnt_ns(struct vfsmount *mnt_root,
+		struct fs_struct *fs)
+{
+	struct mnt_namespace *new_ns;
+
+	new_ns = alloc_mnt_ns();
+	if (IS_ERR(new_ns))
+		return new_ns;
+
+	/* We're starting a completely fresh namespace, so we shouldn't need
+	 * to lock
+	 */
+	mnt_root->mnt_ns = new_ns;
+	new_ns->root = mnt_root;
+	list_add(&new_ns->list, &new_ns->root->mnt_list);
+
+	/* Also assume that the fs_struct is private, hence no locks... */
+	if (fs) {
+		dput(fs->pwd.dentry);
+		mntput(fs->pwd.mnt);
+		dput(fs->root.dentry);
+		mntput(fs->root.mnt);
+		fs->root.mnt = mntget(new_ns->root);
+		fs->root.dentry = dget(new_ns->root->mnt_root);
+		fs->pwd.mnt = mntget(new_ns->root);
+		fs->pwd.dentry = dget(new_ns->root->mnt_root);
+	}
+	return new_ns;
+}
+
 SYSCALL_DEFINE5(mount, char __user *, dev_name, char __user *, dir_name,
 		char __user *, type, unsigned long, flags, void __user *, data)
 {
diff --git a/include/linux/mnt_namespace.h b/include/linux/mnt_namespace.h
index 830bbcd..e81c076 100644
--- a/include/linux/mnt_namespace.h
+++ b/include/linux/mnt_namespace.h
@@ -22,6 +22,8 @@ struct proc_mounts {
 	int event;
 };
 
+extern struct mnt_namespace *create_private_mnt_ns(struct vfsmount *,
+		struct fs_struct *);
 extern struct mnt_namespace *copy_mnt_ns(unsigned long, struct mnt_namespace *,
 		struct fs_struct *);
 extern void __put_mnt_ns(struct mnt_namespace *ns);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index fc2e035..b9c0d1e 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -68,6 +68,8 @@ extern int kern_path(const char *, unsigned, struct path *);
 extern int path_lookup(const char *, unsigned, struct nameidata *);
 extern int vfs_path_lookup(struct dentry *, struct vfsmount *,
 			   const char *, unsigned int, struct nameidata *);
+extern int vfs_remote_path_lookup(struct dentry *, struct vfsmount *,
+		const char *, unsigned int , struct nameidata *);
 
 extern int path_lookup_open(int dfd, const char *name, unsigned lookup_flags, struct nameidata *, int open_flags);
 extern struct file *lookup_instantiate_filp(struct nameidata *nd, struct dentry *dentry,
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index afad7de..3c12b79 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -67,6 +67,7 @@ void switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
 void free_nsproxy(struct nsproxy *ns);
 int unshare_nsproxy_namespaces(unsigned long, struct nsproxy **,
 	struct fs_struct *);
+struct nsproxy *unshare_current_nsproxy(void);
 
 static inline void put_nsproxy(struct nsproxy *ns)
 {
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index 63598dc..05ea102 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -169,6 +169,17 @@ void free_nsproxy(struct nsproxy *ns)
 }
 
 /*
+ * Unshare just the current->nsproxy itself.
+ */
+struct nsproxy *unshare_current_nsproxy(void)
+{
+	if (!capable(CAP_SYS_ADMIN))
+		return ERR_PTR(-EPERM);
+
+	return create_new_namespaces(0, current, current->fs);
+}
+
+/*
  * Called from unshare. Unshare all the namespaces part of nsproxy.
  * On success, returns the new nsproxy.
  */
-- 
1.6.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Linus Torvalds]

On Wed, 11 Mar 2009, Trond Myklebust wrote:
>
> This patch therefore defines a VFS helper function that sets up a temporary
> mount namespace to represent the server namespace, and has the current
> task pivot into that prior to doing the path lookup. Upon completion, it
> pivots back into the original namespace, and destroys the private one.

This is disgusting.

Why don't you just create the namespace once?

Also, why do you need that disgusting pivot thing, when we could instead 
trivially just add a 

	struct filesystem *fs;

into the nameidata, and then we can initialize it to

	nd->fs = current->fs

and make all the path walkers use that.

Or we could even try to clean up that horrid AT_FDCWD mess, and drop the 
whole "dfd" passing to "do_path_lookup()", and instead do

	rwlock_t *lock;
	struct path *root, *pwd;

and do

	nd->lock = &current->fs->lock;
	nd->root = &current->fs->root;
	nd->pwd =  &current->fs->pwd;

to initialize things. Then drop dfd as an argument entirely from 
path_lookup_open() and do_path_lookup(), and just have the caller 
initialize the nameidata (the only caller that doesn't use fs->pwd 
currently is do_filp_open(), which takes that 'dfd' and could just 
initialize nd->pwd to the right thing.

I dunno. This is very Al Viroish country, but I really hate how your patch 
looks. 99% of it is just working around the fact that you want some very 
_slight_ differences to how that special '/' thing is handled.

It's not worth doing these kinds of hacks for that.

And I think it's positively _wrong_ to have a function that creates and 
destroys the whole "struct fs_struct" and a namespace for just one call. 
Even if you don't think it's at all performance-critical, the interface is 
too damn ugly. Have separate "create/destroy context" functions, so that 
you _can_ do it just once, and have multiple calls in between.

Even if you personally don't happen to care.

			Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Tue, 2009-03-31 at 14:40 -0700, Linus Torvalds wrote:
> 
> On Wed, 11 Mar 2009, Trond Myklebust wrote:
> >
> > This patch therefore defines a VFS helper function that sets up a temporary
> > mount namespace to represent the server namespace, and has the current
> > task pivot into that prior to doing the path lookup. Upon completion, it
> > pivots back into the original namespace, and destroys the private one.
> 
> This is disgusting.
> 
> Why don't you just create the namespace once?
> 
> Also, why do you need that disgusting pivot thing, when we could instead 
> trivially just add a 
> 
> 	struct filesystem *fs;
> 
> into the nameidata, and then we can initialize it to
> 
> 	nd->fs = current->fs
> 
> and make all the path walkers use that.
> 
> Or we could even try to clean up that horrid AT_FDCWD mess, and drop the 
> whole "dfd" passing to "do_path_lookup()", and instead do
> 
> 	rwlock_t *lock;
> 	struct path *root, *pwd;
> 
> and do
> 
> 	nd->lock = &current->fs->lock;
> 	nd->root = &current->fs->root;
> 	nd->pwd =  &current->fs->pwd;
> 
> to initialize things. Then drop dfd as an argument entirely from 
> path_lookup_open() and do_path_lookup(), and just have the caller 
> initialize the nameidata (the only caller that doesn't use fs->pwd 
> currently is do_filp_open(), which takes that 'dfd' and could just 
> initialize nd->pwd to the right thing.

I'd be fine with that.

> I dunno. This is very Al Viroish country, but I really hate how your patch 
> looks. 99% of it is just working around the fact that you want some very 
> _slight_ differences to how that special '/' thing is handled.

No. The main purpose is to able to look up and walk down an NFSv4 mount
path, which is a path on the _server_'s file/directory namespace.
In order to be able to follow any valid mount path that the user
supplies, we need the ability to follow symlinks, cross mount points and
even cross NFSv4 referrals (i.e. afs-like junctions that point to paths
on a different server).

Under normal path walking we use the 'follow_link()' method of
autogenerating mountpoints (see
Documentation/filesystems/automount-support.txt) when we come across
mount points or referrals on the server. If you are walking a path that
is not supposed to be visible to user processes, you therefore need a
private namespace.

> It's not worth doing these kinds of hacks for that.
> 
> And I think it's positively _wrong_ to have a function that creates and 
> destroys the whole "struct fs_struct" and a namespace for just one call. 
> Even if you don't think it's at all performance-critical, the interface is 
> too damn ugly. Have separate "create/destroy context" functions, so that 
> you _can_ do it just once, and have multiple calls in between.

That can probably be done, but the main reason for having the namespace
was to be able, once the sys_mount() is complete, to garbage collect and
get rid of those autogenerated mount points that are not user visible.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Linus Torvalds]

On Tue, 31 Mar 2009, Trond Myklebust wrote:
> 
> No. The main purpose is to able to look up and walk down an NFSv4 mount
> path, which is a path on the _server_'s file/directory namespace.

Sure, but that's not the "swizzle" part.

If it was just abotu the namespace part, then you'd just do one namespace 
per server, and be done with it.

So if you split that function up into a sepate
 - allocate namespace for nfsd
 - free nfsd namespace
 - do the lookup

then really _all_ the extraneous code comes just from that "playing games 
with it" swizzling part.

> > And I think it's positively _wrong_ to have a function that creates and 
> > destroys the whole "struct fs_struct" and a namespace for just one call. 
> > Even if you don't think it's at all performance-critical, the interface is 
> > too damn ugly. Have separate "create/destroy context" functions, so that 
> > you _can_ do it just once, and have multiple calls in between.
> 
> That can probably be done, but the main reason for having the namespace
> was to be able, once the sys_mount() is complete, to garbage collect and
> get rid of those autogenerated mount points that are not user visible.

So there is never any reason to do that nfsd-specific pathwalk at 
run-time? There are no "server pwd" requests that clients can do?

		Linus

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Tue, 2009-03-31 at 15:43 -0700, Linus Torvalds wrote:
> 
> On Tue, 31 Mar 2009, Trond Myklebust wrote:
> > 
> > No. The main purpose is to able to look up and walk down an NFSv4 mount
> > path, which is a path on the _server_'s file/directory namespace.
> 
> Sure, but that's not the "swizzle" part.
> 
> If it was just abotu the namespace part, then you'd just do one namespace 
> per server, and be done with it.
> 
> So if you split that function up into a sepate
>  - allocate namespace for nfsd
>  - free nfsd namespace
>  - do the lookup
> 
> then really _all_ the extraneous code comes just from that "playing games 
> with it" swizzling part.

Fair enough.

My main technical problem with the namespace per server is knowing when
it is safe to release it. AFAICS, you want to cache the per-server
namespace if there are mount points on "public" namespaces that
reference that server, but release it if the only remaining mount points
are the ones on the per-server namespace itself.

> > > And I think it's positively _wrong_ to have a function that creates and 
> > > destroys the whole "struct fs_struct" and a namespace for just one call. 
> > > Even if you don't think it's at all performance-critical, the interface is 
> > > too damn ugly. Have separate "create/destroy context" functions, so that 
> > > you _can_ do it just once, and have multiple calls in between.
> > 
> > That can probably be done, but the main reason for having the namespace
> > was to be able, once the sys_mount() is complete, to garbage collect and
> > get rid of those autogenerated mount points that are not user visible.
> 
> So there is never any reason to do that nfsd-specific pathwalk at 
> run-time? There are no "server pwd" requests that clients can do?

NFSv4 does theoretically support the idea of volatile filehandles. If
so, and the server were to declare that your mount point's filehandle
has expired, then your only way of recovering would be to look up that
mount path again. (We might perhaps implement client support for that
some day if somebody discovers a good enough reason).

However, if your mount point filehandle has expired, then chances are
that you'll probably have to look up all the filehandles in the mount
path again too, so why should you cache all that information?

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Linus Torvalds]

On Tue, 31 Mar 2009, Trond Myklebust wrote:
> 
> However, if your mount point filehandle has expired, then chances are
> that you'll probably have to look up all the filehandles in the mount
> path again too, so why should you cache all that information?

No, that' not the caching I was thinking about.

I meant literally just the "namespace" thing. Keeping that around as long 
as you have a export active. How many of those do you have on your average 
nfs server? Why not just create those nfsd namespaces once for each 
export at startup, and destroy them at exit.

			Linus

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Tue, 2009-03-31 at 17:16 -0700, Linus Torvalds wrote:
> 
> On Tue, 31 Mar 2009, Trond Myklebust wrote:
> > 
> > However, if your mount point filehandle has expired, then chances are
> > that you'll probably have to look up all the filehandles in the mount
> > path again too, so why should you cache all that information?
> 
> No, that' not the caching I was thinking about.
> 
> I meant literally just the "namespace" thing. Keeping that around as long 
> as you have a export active. How many of those do you have on your average 
> nfs server? Why not just create those nfsd namespaces once for each 
> export at startup, and destroy them at exit.

Wait, I'm confused now.

None of this is running on the server. The private namespace is being
created on the NFSv4 client, which only needs to resolve a mount path as
part of a sys_mount() call.

Unlike NFSv2/v3, there is no MOUNT protocol to translate an NFSv4 mount
path into a filehandle. Instead the client has an operation to fetch the
'root filehandle' (i.e. the filehandle representing 'mount -t nfs4
foo:/') and then does a series of LOOKUP rpc calls to resolve the rest
of the path.

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Linus Torvalds]

On Tue, 31 Mar 2009, Trond Myklebust wrote:
> 
> None of this is running on the server. The private namespace is being
> created on the NFSv4 client, which only needs to resolve a mount path as
> part of a sys_mount() call.

Ahh. A light goes on. So it realy does end up being a once-only thing. I 
didn't look at the users, and assumed that "server side" implied nfsd.

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Tue, 2009-03-31 at 18:38 -0400, Trond Myklebust wrote:
> On Tue, 2009-03-31 at 14:40 -0700, Linus Torvalds wrote:
> > 
> > On Wed, 11 Mar 2009, Trond Myklebust wrote:
> > >
> > > This patch therefore defines a VFS helper function that sets up a temporary
> > > mount namespace to represent the server namespace, and has the current
> > > task pivot into that prior to doing the path lookup. Upon completion, it
> > > pivots back into the original namespace, and destroys the private one.
> > 
> > This is disgusting.
> > 
> > Why don't you just create the namespace once?
> > 
> > Also, why do you need that disgusting pivot thing, when we could instead 
> > trivially just add a 
> > 
> > 	struct filesystem *fs;
> > 
> > into the nameidata, and then we can initialize it to
> > 
> > 	nd->fs = current->fs
> > 
> > and make all the path walkers use that.
> > 
> > Or we could even try to clean up that horrid AT_FDCWD mess, and drop the 
> > whole "dfd" passing to "do_path_lookup()", and instead do
> > 
> > 	rwlock_t *lock;
> > 	struct path *root, *pwd;
> > 
> > and do
> > 
> > 	nd->lock = &current->fs->lock;
> > 	nd->root = &current->fs->root;
> > 	nd->pwd =  &current->fs->pwd;
> > 
> > to initialize things. Then drop dfd as an argument entirely from 
> > path_lookup_open() and do_path_lookup(), and just have the caller 
> > initialize the nameidata (the only caller that doesn't use fs->pwd 
> > currently is do_filp_open(), which takes that 'dfd' and could just 
> > initialize nd->pwd to the right thing.
> 
> I'd be fine with that.
> 
> > I dunno. This is very Al Viroish country, but I really hate how your patch 
> > looks. 99% of it is just working around the fact that you want some very 
> > _slight_ differences to how that special '/' thing is handled.
> 
> No. The main purpose is to able to look up and walk down an NFSv4 mount
> path, which is a path on the _server_'s file/directory namespace.
> In order to be able to follow any valid mount path that the user
> supplies, we need the ability to follow symlinks, cross mount points and
> even cross NFSv4 referrals (i.e. afs-like junctions that point to paths
> on a different server).
> 
> Under normal path walking we use the 'follow_link()' method of
> autogenerating mountpoints (see
> Documentation/filesystems/automount-support.txt) when we come across
> mount points or referrals on the server. If you are walking a path that
> is not supposed to be visible to user processes, you therefore need a
> private namespace.
> 
> > It's not worth doing these kinds of hacks for that.
> > 
> > And I think it's positively _wrong_ to have a function that creates and 
> > destroys the whole "struct fs_struct" and a namespace for just one call. 
> > Even if you don't think it's at all performance-critical, the interface is 
> > too damn ugly. Have separate "create/destroy context" functions, so that 
> > you _can_ do it just once, and have multiple calls in between.
> 
> That can probably be done, but the main reason for having the namespace
> was to be able, once the sys_mount() is complete, to garbage collect and
> get rid of those autogenerated mount points that are not user visible.

I meant to say "...the main reason for destroying the namespace"...

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Wed, Mar 11, 2009 at 03:50:33PM -0400, Trond Myklebust wrote:
> The purpose of this patch is to improve the mount path lookup support for
> filesystems such as NFSv4, which require you to look up a mount path
> string in a remote server's namespace.
> 
> Traversing such a path is pretty much identical to walking a local path,
> in that it may involve following symlinks and even following referrals to
> volumes that reside on other servers. Since the standard VFS path lookup
> code already supports all these features (using in-kernel automounts for
> following referrals) it would be nice to be able to reuse that code rather
> than special case the mount path lookup in the NFS client.
> 
> This patch therefore defines a VFS helper function that sets up a temporary
> mount namespace to represent the server namespace, and has the current
> task pivot into that prior to doing the path lookup. Upon completion, it
> pivots back into the original namespace, and destroys the private one.

NAK.  You are relying on too many things about caller (e.g. just what happens
if you have shared fs_struct?  Does caller guarantee it won't happen?), so
that's a bloody bad interface.  Open-code that sucker, then let's see what
to do with surrounding code.  As it stands - no.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Wed, 2009-04-01 at 03:15 +0100, Al Viro wrote:
> On Wed, Mar 11, 2009 at 03:50:33PM -0400, Trond Myklebust wrote:
> > The purpose of this patch is to improve the mount path lookup support for
> > filesystems such as NFSv4, which require you to look up a mount path
> > string in a remote server's namespace.
> > 
> > Traversing such a path is pretty much identical to walking a local path,
> > in that it may involve following symlinks and even following referrals to
> > volumes that reside on other servers. Since the standard VFS path lookup
> > code already supports all these features (using in-kernel automounts for
> > following referrals) it would be nice to be able to reuse that code rather
> > than special case the mount path lookup in the NFS client.
> > 
> > This patch therefore defines a VFS helper function that sets up a temporary
> > mount namespace to represent the server namespace, and has the current
> > task pivot into that prior to doing the path lookup. Upon completion, it
> > pivots back into the original namespace, and destroys the private one.
> 
> NAK.  You are relying on too many things about caller (e.g. just what happens
> if you have shared fs_struct?  Does caller guarantee it won't happen?), so
> that's a bloody bad interface.  Open-code that sucker, then let's see what
> to do with surrounding code.  As it stands - no.

Since vfs_remote_path_lookup() creates a private copy of the fs_struct,
are you referring to the assumptions that are made in
create_private_mnt_ns()? Sure; that's not intended as an interface for
general use. I'll rewrite that...

Cheers
  Trond

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 03:34:34PM -0400, Trond Myklebust wrote:
> > *OW*
> > 
> > I've missed that completely.  OK, may I see the users, please?
> 
> This is why I'm reluctant to call it unshare_mnt_namespace() and was why
> I was trying to limit the usage of the interface.

Ho-hum...  You know what, it *still* fits into unshare() model.  The thing
is, if you look at the origin of clone()/unshare() (i.e. Plan 9 rfork()),
there's the following group of flags:

   RFNAMEG    If set, the new process inherits a copy of the parent's
   name space; otherwise the new process shares the parent's name space.
   Is mutually exclusive with RFCNAMEG.
   RFCNAMEG   If set, the new process starts with a clean name space. A
   new name space must be built from a mount of an open file descriptor.
   Is mutually exclusive with RFNAMEG.
   RFNOMNT    If set, subsequent mounts into the new name space and
   dereferencing of pathnames starting with # are disallowed.

And analog of that sucker (RFCNAMEG) is just about what you are doing
(well, that + attaching your vfsmount as separate action).  So how about
about CLONE_CLEANNS and passing it in flags?  Linus, do you have any
objections against such a flag?  It'd give you a new instance of
rootfs (empty, obviously) as root and flip root/pwd to it.

The thing is, I really want it to make sense other than just a very
specialized wrapper around path lookup.  We *already* have a bunch of
places doing subsets of unshare()-and-save; would be nice to get it
right once and for all...

BTW, if we look into the original, there's also RFCFDG - "give it an empty
descriptor table".  Might be also interesting, but that's a separate story.

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 08:52:12PM +0100, Al Viro wrote:

> And analog of that sucker (RFCNAMEG) is just about what you are doing
> (well, that + attaching your vfsmount as separate action).  So how about
> about CLONE_CLEANNS and passing it in flags?  Linus, do you have any
> objections against such a flag?  It'd give you a new instance of
> rootfs (empty, obviously) as root and flip root/pwd to it.

*growl*

Right, we are out of clone() flags, thanks to namespace-of-the-week
insanity by openvz bunch ;-/

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Linus Torvalds]

On Thu, 2 Apr 2009, Al Viro wrote:
>
> *growl*
> 
> Right, we are out of clone() flags, thanks to namespace-of-the-week
> insanity by openvz bunch ;-/

I really think "clone()" is the wrong kind of granularity, and much too 
big a hammer.

You never answered my suggestion about passing in namespace details to the 
open routines in nameidata. I don't think it actually needs the whole 
thing, the only thing (I _think_) it cares about ends up being just the 
root/cwd.

And as we've with openat/fstatat/etc, NFS really isn't the only thing 
wanting to do per-operation versions of those. Of course, openat() and 
friends just want to use a temporary local cwd. NFS wants to do the same 
thing for /. But I think the problems are related, and the solution could 
be related too - just allow per-operation 'fs' namespaces.

And the nameidata really _is_ the logical place for that all. Not the 
clone flags.

			Linus

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 01:17:47PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 2 Apr 2009, Al Viro wrote:
> >
> > *growl*
> > 
> > Right, we are out of clone() flags, thanks to namespace-of-the-week
> > insanity by openvz bunch ;-/
> 
> I really think "clone()" is the wrong kind of granularity, and much too 
> big a hammer.
> 
> You never answered my suggestion about passing in namespace details to the 
> open routines in nameidata. I don't think it actually needs the whole 
> thing, the only thing (I _think_) it cares about ends up being just the 
> root/cwd.

The thing is, it wants more - it wants to mount stuff there.  Which means
that we really want current switched to it, or the normal check_mnt()
logics will screw you.

I don't have too serious objections against having a reference to namespace
in nameidata, but I really doubt that it'll be enough here.

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Thu, 2009-04-02 at 21:28 +0100, Al Viro wrote:
> On Thu, Apr 02, 2009 at 01:17:47PM -0700, Linus Torvalds wrote:
> > 
> > 
> > On Thu, 2 Apr 2009, Al Viro wrote:
> > >
> > > *growl*
> > > 
> > > Right, we are out of clone() flags, thanks to namespace-of-the-week
> > > insanity by openvz bunch ;-/
> > 
> > I really think "clone()" is the wrong kind of granularity, and much too 
> > big a hammer.
> > 
> > You never answered my suggestion about passing in namespace details to the 
> > open routines in nameidata. I don't think it actually needs the whole 
> > thing, the only thing (I _think_) it cares about ends up being just the 
> > root/cwd.
> 
> The thing is, it wants more - it wants to mount stuff there.  Which means
> that we really want current switched to it, or the normal check_mnt()
> logics will screw you.
> 
> I don't have too serious objections against having a reference to namespace
> in nameidata, but I really doubt that it'll be enough here.

The NFS client is constructing the namespace as it walks the path using
the ->follow_link() trick to automount new filesystems. As long as we
can add a mount namespace argument to do_add_mount(), then we should be
able to make use of a namespace in struct nameidata.

The other thing we'd need is a 'root' path argument to ensure that
__vfs_follow_link() does the right thing when presented with an absolute
symlink.

Given those two, plus a method for creating an empty namespace, I think
we should have all we need.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 04:45:30PM -0400, Trond Myklebust wrote:

> The NFS client is constructing the namespace as it walks the path using
> the ->follow_link() trick to automount new filesystems. As long as we
> can add a mount namespace argument to do_add_mount(), then we should be
> able to make use of a namespace in struct nameidata.

*wince*
Theoretically doable, practically can get ugly.

> The other thing we'd need is a 'root' path argument to ensure that
> __vfs_follow_link() does the right thing when presented with an absolute
> symlink.

The only problem I see with this is that we'll get root vfsmount/dentry
refcount jerked on *all* lookups now, instead of just the absolute
ones...

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 09:54:18PM +0100, Al Viro wrote:
> On Thu, Apr 02, 2009 at 04:45:30PM -0400, Trond Myklebust wrote:
> 
> > The NFS client is constructing the namespace as it walks the path using
> > the ->follow_link() trick to automount new filesystems. As long as we
> > can add a mount namespace argument to do_add_mount(), then we should be
> > able to make use of a namespace in struct nameidata.
> 
> *wince*
> Theoretically doable, practically can get ugly.

BTW, it's enough to tell it "take the namespace mountpoint belongs to",
if we really go that way.

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Thu, 2009-04-02 at 21:56 +0100, Al Viro wrote:
> On Thu, Apr 02, 2009 at 09:54:18PM +0100, Al Viro wrote:
> > On Thu, Apr 02, 2009 at 04:45:30PM -0400, Trond Myklebust wrote:
> > 
> > > The NFS client is constructing the namespace as it walks the path using
> > > the ->follow_link() trick to automount new filesystems. As long as we
> > > can add a mount namespace argument to do_add_mount(), then we should be
> > > able to make use of a namespace in struct nameidata.
> > 
> > *wince*
> > Theoretically doable, practically can get ugly.
> 
> BTW, it's enough to tell it "take the namespace mountpoint belongs to",
> if we really go that way.

True. The only place (apart from check_mnt()) that doesn't use the
mountpoint namespace appears to be attach_recursive_mnt() which, for
reasons which are not clear, appears to do a touch_mnt_namespace() on
current->nsproxy->mnt_ns instead.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Thu, 2009-04-02 at 21:54 +0100, Al Viro wrote:
> > The other thing we'd need is a 'root' path argument to ensure that
> > __vfs_follow_link() does the right thing when presented with an absolute
> > symlink.
> 
> The only problem I see with this is that we'll get root vfsmount/dentry
> refcount jerked on *all* lookups now, instead of just the absolute
> ones...

How about using a LOOKUP_ROOT_SET nd->flag to switch between use of
current->fs->root or nd->root? That allows you to keep the default
behaviour of not changing the root refcount.

-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Thu, Apr 02, 2009 at 06:16:25PM -0400, Trond Myklebust wrote:
> On Thu, 2009-04-02 at 21:54 +0100, Al Viro wrote:
> > > The other thing we'd need is a 'root' path argument to ensure that
> > > __vfs_follow_link() does the right thing when presented with an absolute
> > > symlink.
> > 
> > The only problem I see with this is that we'll get root vfsmount/dentry
> > refcount jerked on *all* lookups now, instead of just the absolute
> > ones...
> 
> How about using a LOOKUP_ROOT_SET nd->flag to switch between use of
> current->fs->root or nd->root? That allows you to keep the default
> behaviour of not changing the root refcount.

Eww...  Let's not.  First of all, the really useful part of nd->root for
normal case is that it allows to get ...->fs->root once.  So it's better
to *cache* ->fs->root in there.  At which point the flag disappears,
since it becomes simply nd->root.mnt != NULL.

The interesting part is keeping refcounting happy.  I'll see how to do
that - it's clearly useful on its own.  Hopefully will post later tonight...

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Fri, Apr 03, 2009 at 12:18:59AM +0100, Al Viro wrote:

> Eww...  Let's not.  First of all, the really useful part of nd->root for
> normal case is that it allows to get ...->fs->root once.  So it's better
> to *cache* ->fs->root in there.  At which point the flag disappears,
> since it becomes simply nd->root.mnt != NULL.
> 
> The interesting part is keeping refcounting happy.  I'll see how to do
> that - it's clearly useful on its own.  Hopefully will post later tonight...

... and the ugly part is the check in follow_dotdot() - one about crossing
out of chroot.  Joy...

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Fri, Apr 03, 2009 at 02:09:54AM +0100, Al Viro wrote:
> On Fri, Apr 03, 2009 at 12:18:59AM +0100, Al Viro wrote:
> 
> > Eww...  Let's not.  First of all, the really useful part of nd->root for
> > normal case is that it allows to get ...->fs->root once.  So it's better
> > to *cache* ->fs->root in there.  At which point the flag disappears,
> > since it becomes simply nd->root.mnt != NULL.
> > 
> > The interesting part is keeping refcounting happy.  I'll see how to do
> > that - it's clearly useful on its own.  Hopefully will post later tonight...
> 
> ... and the ugly part is the check in follow_dotdot() - one about crossing
> out of chroot.  Joy...

OK, that's actually doable.
See git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ cache-root
for current variant; it's for comments *only* - not for merge.  After that
one we should be able to do a modified variant of e.g. vfs_path_lookup()
that would do
	get_path(root);
	nd->root = root;
instead of
	nd->root.mnt = NULL;
and have lookup with root set to given place without touching current->fs
at all.  Warning: this is completely untested.  Not even "does it boot?",
just a code dump.

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Fri, Apr 03, 2009 at 02:52:20AM +0100, Al Viro wrote:
> OK, that's actually doable.
> See git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ cache-root
> for current variant; it's for comments *only* - not for merge.  After that
> one we should be able to do a modified variant of e.g. vfs_path_lookup()
> that would do
> 	get_path(root);
	path_get(root), even
> 	nd->root = root;
> instead of
> 	nd->root.mnt = NULL;
> and have lookup with root set to given place without touching current->fs
> at all.  Warning: this is completely untested.  Not even "does it boot?",
> just a code dump.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Trond Myklebust]

On Fri, 2009-04-03 at 02:52 +0100, Al Viro wrote:
> On Fri, Apr 03, 2009 at 02:09:54AM +0100, Al Viro wrote:
> > On Fri, Apr 03, 2009 at 12:18:59AM +0100, Al Viro wrote:
> > 
> > > Eww...  Let's not.  First of all, the really useful part of nd->root for
> > > normal case is that it allows to get ...->fs->root once.  So it's better
> > > to *cache* ->fs->root in there.  At which point the flag disappears,
> > > since it becomes simply nd->root.mnt != NULL.
> > > 
> > > The interesting part is keeping refcounting happy.  I'll see how to do
> > > that - it's clearly useful on its own.  Hopefully will post later tonight...
> > 
> > ... and the ugly part is the check in follow_dotdot() - one about crossing
> > out of chroot.  Joy...
> 
> OK, that's actually doable.
> See git.kernel.org/pub/scm/linux/kernel/git/viro/vfs-2.6.git/ cache-root
> for current variant; it's for comments *only* - not for merge.  After that
> one we should be able to do a modified variant of e.g. vfs_path_lookup()
> that would do
> 	get_path(root);
> 	nd->root = root;
> instead of
> 	nd->root.mnt = NULL;
> and have lookup with root set to given place without touching current->fs
> at all.  Warning: this is completely untested.  Not even "does it boot?",
> just a code dump.

You have a potential problem in the case of do_filp_open(). It calls
do_path_lookup(), but then expects to be able to reuse the same
nameidata structure in subsequent calls to __do_follow_link().

Cheers
  Trond
-- 
Trond Myklebust
Linux NFS client maintainer

NetApp
Trond.Myklebust@netapp.com
www.netapp.com

Re: [PATCH 1/4] VFS: Add a VFS helper function vfs_remote_path_lookup()

[Al Viro]

On Fri, Apr 03, 2009 at 03:13:55PM -0400, Trond Myklebust wrote:

> You have a potential problem in the case of do_filp_open(). It calls
> do_path_lookup(), but then expects to be able to reuse the same
> nameidata structure in subsequent calls to __do_follow_link().

Nothing potential about it.  I'll push fixes for that and a couple of
other nasties in a bit; sorry about disappearing, zeniv had been down
last night ;-/

[PATCH 2/4] NFSv4: Use vfs_path_lookup() instead of nfs4_path_walk()

[Trond Myklebust]

The NFSv4 mount code currently has several limitations which we really
don't want to keep. For one thing, it will not follow symlinks, and neither
will it follow referrals.
The correct approach here should be to reuse the existing VFS and NFS path
lookup code instead of duplicating it inside nfs4_path_walk().

Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
---
 fs/nfs/super.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 147 insertions(+), 21 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 82eaadb..1da36bf 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -42,6 +42,7 @@
 #include <linux/smp_lock.h>
 #include <linux/seq_file.h>
 #include <linux/mount.h>
+#include <linux/namei.h>
 #include <linux/nfs_idmap.h>
 #include <linux/vfs.h>
 #include <linux/inet.h>
@@ -270,10 +271,14 @@ static const struct super_operations nfs_sops = {
 #ifdef CONFIG_NFS_V4
 static int nfs4_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
+static int nfs4_remote_get_sb(struct file_system_type *fs_type,
+	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
 static int nfs4_xdev_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
 static int nfs4_referral_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
+static int nfs4_remote_referral_get_sb(struct file_system_type *fs_type,
+	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt);
 static void nfs4_kill_super(struct super_block *sb);
 
 static struct file_system_type nfs4_fs_type = {
@@ -284,6 +289,14 @@ static struct file_system_type nfs4_fs_type = {
 	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
 };
 
+static struct file_system_type nfs4_remote_fs_type = {
+	.owner		= THIS_MODULE,
+	.name		= "nfs4",
+	.get_sb		= nfs4_remote_get_sb,
+	.kill_sb	= nfs4_kill_super,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+};
+
 struct file_system_type nfs4_xdev_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "nfs4",
@@ -292,6 +305,14 @@ struct file_system_type nfs4_xdev_fs_type = {
 	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
 };
 
+static struct file_system_type nfs4_remote_referral_fs_type = {
+	.owner		= THIS_MODULE,
+	.name		= "nfs4",
+	.get_sb		= nfs4_remote_referral_get_sb,
+	.kill_sb	= nfs4_kill_super,
+	.fs_flags	= FS_RENAME_DOES_D_MOVE|FS_REVAL_DOT|FS_BINARY_MOUNTDATA,
+};
+
 struct file_system_type nfs4_referral_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "nfs4",
@@ -2370,12 +2391,12 @@ out_no_client_address:
 }
 
 /*
- * Get the superblock for an NFS4 mountpoint
+ * Get the superblock for the NFS4 root partition
  */
-static int nfs4_get_sb(struct file_system_type *fs_type,
+static int nfs4_remote_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt)
 {
-	struct nfs_parsed_mount_data *data;
+	struct nfs_parsed_mount_data *data = raw_data;
 	struct super_block *s;
 	struct nfs_server *server;
 	struct nfs_fh *mntfh;
@@ -2386,18 +2407,12 @@ static int nfs4_get_sb(struct file_system_type *fs_type,
 	};
 	int error = -ENOMEM;
 
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	mntfh = kzalloc(sizeof(*mntfh), GFP_KERNEL);
 	if (data == NULL || mntfh == NULL)
 		goto out_free_fh;
 
 	security_init_mnt_opts(&data->lsm_opts);
 
-	/* Validate the mount data */
-	error = nfs4_validate_mount_data(raw_data, data, dev_name);
-	if (error < 0)
-		goto out;
-
 	/* Get a volume representation */
 	server = nfs4_create_server(data, mntfh);
 	if (IS_ERR(server)) {
@@ -2410,7 +2425,7 @@ static int nfs4_get_sb(struct file_system_type *fs_type,
 		compare_super = NULL;
 
 	/* Get a superblock - note that we may end up sharing one that already exists */
-	s = sget(fs_type, compare_super, nfs_set_super, &sb_mntdata);
+	s = sget(&nfs4_fs_type, compare_super, nfs_set_super, &sb_mntdata);
 	if (IS_ERR(s)) {
 		error = PTR_ERR(s);
 		goto out_free;
@@ -2447,14 +2462,9 @@ static int nfs4_get_sb(struct file_system_type *fs_type,
 	error = 0;
 
 out:
-	kfree(data->client_address);
-	kfree(data->nfs_server.export_path);
-	kfree(data->nfs_server.hostname);
-	kfree(data->fscache_uniq);
 	security_free_mnt_opts(&data->lsm_opts);
 out_free_fh:
 	kfree(mntfh);
-	kfree(data);
 	return error;
 
 out_free:
@@ -2469,6 +2479,93 @@ error_splat_super:
 	goto out;
 }
 
+static struct vfsmount *nfs_do_root_mount(struct file_system_type *fs_type,
+		int flags, void *data, const char *hostname)
+{
+	struct vfsmount *root_mnt;
+	char *root_devname;
+	size_t len;
+
+	len = strlen(hostname) + 3;
+	root_devname = kmalloc(len, GFP_KERNEL);
+	if (root_devname == NULL)
+		return ERR_PTR(-ENOMEM);
+	snprintf(root_devname, len, "%s:/", hostname);
+	root_mnt = vfs_kern_mount(fs_type, flags, root_devname, data);
+	kfree(root_devname);
+	return root_mnt;
+}
+
+static int nfs_follow_remote_path(struct vfsmount *root_mnt,
+		const char *export_path, struct vfsmount *mnt_target)
+{
+	struct nameidata nd;
+	struct super_block *s;
+	int ret;
+
+	ret = vfs_remote_path_lookup(root_mnt->mnt_root, root_mnt,
+			export_path, LOOKUP_FOLLOW, &nd);
+	if (ret != 0)
+		goto out_mntput;
+
+	s = nd.path.mnt->mnt_sb;
+	atomic_inc(&s->s_active);
+	mnt_target->mnt_sb = s;
+	mnt_target->mnt_root = dget(nd.path.dentry);
+
+	path_put(&nd.path);
+	mntput(root_mnt);
+	down_write(&s->s_umount);
+	return 0;
+out_mntput:
+	mntput(root_mnt);
+	return ret;
+}
+
+/*
+ * Get the superblock for an NFS4 mountpoint
+ */
+static int nfs4_get_sb(struct file_system_type *fs_type,
+	int flags, const char *dev_name, void *raw_data, struct vfsmount *mnt)
+{
+	struct nfs_parsed_mount_data *data;
+	char *export_path;
+	struct vfsmount *root_mnt;
+	int error = -ENOMEM;
+
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (data == NULL)
+		goto out_free_data;
+
+	/* Validate the mount data */
+	error = nfs4_validate_mount_data(raw_data, data, dev_name);
+	if (error < 0)
+		goto out;
+
+	export_path = data->nfs_server.export_path;
+	data->nfs_server.export_path = "/";
+	root_mnt = nfs_do_root_mount(&nfs4_remote_fs_type, flags, data,
+			data->nfs_server.hostname);
+	data->nfs_server.export_path = export_path;
+
+	error = PTR_ERR(root_mnt);
+	if (IS_ERR(root_mnt))
+		goto out;
+
+	error = nfs_follow_remote_path(root_mnt, export_path, mnt);
+
+out:
+	kfree(data->client_address);
+	kfree(data->nfs_server.export_path);
+	kfree(data->nfs_server.hostname);
+	kfree(data->fscache_uniq);
+out_free_data:
+	kfree(data);
+	dprintk("<-- nfs4_referral_get_sb() = %d%s\n", error,
+			error != 0 ? " [error]" : "");
+	return error;
+}
+
 static void nfs4_kill_super(struct super_block *sb)
 {
 	struct nfs_server *server = NFS_SB(sb);
@@ -2565,12 +2662,9 @@ error_splat_super:
 	return error;
 }
 
-/*
- * Create an NFS4 server record on referral traversal
- */
-static int nfs4_referral_get_sb(struct file_system_type *fs_type, int flags,
-				const char *dev_name, void *raw_data,
-				struct vfsmount *mnt)
+static int nfs4_remote_referral_get_sb(struct file_system_type *fs_type,
+		int flags, const char *dev_name, void *raw_data,
+		struct vfsmount *mnt)
 {
 	struct nfs_clone_mount *data = raw_data;
 	struct super_block *s;
@@ -2650,4 +2744,36 @@ error_splat_super:
 	return error;
 }
 
+/*
+ * Create an NFS4 server record on referral traversal
+ */
+static int nfs4_referral_get_sb(struct file_system_type *fs_type,
+		int flags, const char *dev_name, void *raw_data,
+		struct vfsmount *mnt)
+{
+	struct nfs_clone_mount *data = raw_data;
+	char *export_path;
+	struct vfsmount *root_mnt;
+	int error;
+
+	dprintk("--> nfs4_referral_get_sb()\n");
+
+	export_path = data->mnt_path;
+	data->mnt_path = "/";
+
+	root_mnt = nfs_do_root_mount(&nfs4_remote_referral_fs_type,
+			flags, data, data->hostname);
+	data->mnt_path = export_path;
+
+	error = PTR_ERR(root_mnt);
+	if (IS_ERR(root_mnt))
+		goto out;
+
+	error = nfs_follow_remote_path(root_mnt, export_path, mnt);
+out:
+	dprintk("<-- nfs4_referral_get_sb() = %d%s\n", error,
+			error != 0 ? " [error]" : "");
+	return error;
+}
+
 #endif /* CONFIG_NFS_V4 */
-- 
1.6.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] NFS: Correct the NFS mount path when following a referral

[Trond Myklebust]

Signed-off-by: Trond Myklebust <Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>
---
 fs/nfs/super.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/super.c b/fs/nfs/super.c
index 1da36bf..f0f88a5 100644
--- a/fs/nfs/super.c
+++ b/fs/nfs/super.c
@@ -2496,6 +2496,27 @@ static struct vfsmount *nfs_do_root_mount(struct file_system_type *fs_type,
 	return root_mnt;
 }
 
+static void nfs_fix_devname(const struct path *path, struct vfsmount *mnt)
+{
+	char *page = (char *) __get_free_page(GFP_KERNEL);
+	char *devname, *tmp;
+
+	if (page == NULL)
+		return;
+	devname = nfs_path(path->mnt->mnt_devname,
+			path->mnt->mnt_root, path->dentry,
+			page, PAGE_SIZE);
+	if (devname == NULL)
+		goto out_freepage;
+	tmp = kstrdup(devname, GFP_KERNEL);
+	if (tmp == NULL)
+		goto out_freepage;
+	kfree(mnt->mnt_devname);
+	mnt->mnt_devname = tmp;
+out_freepage:
+	free_page((unsigned long)page);
+}
+
 static int nfs_follow_remote_path(struct vfsmount *root_mnt,
 		const char *export_path, struct vfsmount *mnt_target)
 {
@@ -2513,6 +2534,9 @@ static int nfs_follow_remote_path(struct vfsmount *root_mnt,
 	mnt_target->mnt_sb = s;
 	mnt_target->mnt_root = dget(nd.path.dentry);
 
+	/* Correct the device pathname */
+	nfs_fix_devname(&nd.path, mnt_target);
+
 	path_put(&nd.path);
 	mntput(root_mnt);
 	down_write(&s->s_umount);
-- 
1.6.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] NFS: Fix nfs_path() to always return a '/' at the beginning of the path

[Trond Myklebust]

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---
 fs/nfs/namespace.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c
index 64a288e..63b2b19 100644
--- a/fs/nfs/namespace.c
+++ b/fs/nfs/namespace.c
@@ -65,6 +65,11 @@ char *nfs_path(const char *base,
 		dentry = dentry->d_parent;
 	}
 	spin_unlock(&dcache_lock);
+	if (*end != '/') {
+		if (--buflen < 0)
+			goto Elong;
+		*--end = '/';
+	}
 	namelen = strlen(base);
 	/* Strip off excess slashes in base string */
 	while (namelen > 0 && base[namelen - 1] == '/')
-- 
1.6.0.4