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 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