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

[Trond Myklebust <Trond.Myklebust@netapp.com>]

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@netapp.com
www.netapp.com