On Thu, 2011-09-22 at 14:44 -0400, Jeff Layton wrote:
> On Thu, 22 Sep 2011 13:35:29 -0400
> Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> > On Fri, 23 Sep 2011 00:45:35 +0800
> > Ian Kent <raven@xxxxxxxxxx> wrote:
> >
> > > On Thu, 2011-09-22 at 09:30 -0700, Linus Torvalds wrote:
> > > > On Thu, Sep 22, 2011 at 9:04 AM, Ian Kent <raven@xxxxxxxxxx> wrote:
> > > > >
> > > > > I haven't checked what the side effects would be but using the
> > > > > LOOKUP_DIRECTORY flag in walks that get caught by the removal of the
> > > > > LOOKUP_FOLLOW test in follow_automount() and retaining Miklos's original
> > > > > patch is probably a more natural solution IMO.
> > > >
> > > > Ok, I do have to agree with that.
> > > >
> > > > So instead of adding a new flag, let's just document a few *logical*
> > > > rules for what causes auto-mounting:
> > > >
> > > > - opening the mount-point itself with LOOKUP_DIRECTORY does so
> > > >
> > > > Logic: when you use LOOKUP_DIRECTORY, you expect to see the
> > > > *contents* of the mount-point.
> > > >
> > > > - looking something up *under* the mount-point does so.
> > > >
> > > > This may be obvious, but it actually has a non-obvious special
> > > > case: what about the pathname "mountpoint" vs "mountpoint/"
> > > >
> > > > And I think the "LOOKUP_DIRECTORY" rule ends up automatically also
> > > > resolving that special case: when we have a slash at the end of the
> > > > last component, it not only implies that we care about the contents,
> > > > it will also automatically set LOOKUP_DIRECTORY.
> > > >
> > > > So I'm getting more and more convinced that LOOKUP_DIRECTORY is
> > > > actually the right thing to trigger on. It automatically means that
> > > > "opendir()" on the mountpoint will do the right thing (because
> > > > O_DIRECTORY results in LOOKUP_DIRECTORY), and it automatically - and
> > > > very naturally - gives user processes the ability to choose whether
> > > > they want to see auto-monting or not ("path" doesn't get auto-mounted,
> > > > but "path/" does).
> > > >
> > > > Yet at the same time it keeps the "stupid default behavior for
> > > > processes that only look at each directory entry and don't even think
> > > > about automounting" be the "don't auto-mount when not necessary"
> > > > behavior.
> > > >
> > > > So: no new flag. Just make nfs4 use LOOKUP_DIRECTORY, and let's add
> > > > big documentation notes about this.
> > >
> > > Yes, that's what I think is best.
> > >
> > > It's late here so I'll survey the code and see if I can find any other
> > > instances of this and post a patch, tomorrow.
> > >
> > > Jeff, could you test adding LOOKUP_DIRECTORY to the walk in
> > > nfs_follow_remote_path() please, just in case I've got this all wrong.
> > >
> >
> > Yep, adding LOOKUP_DIRECTORY also fixes the problem. I'll go ahead and
> > send a patch to Trond...
> >
>
> To be clear... I tested this with LOOKUP_FOLLOW|LOOKUP_DIRECTORY.
>
> Also, Ian requested that I hold off on pushing a patch to Trond until
> he can audit the rest of the code for similar problems.
We need to audit not only NFS, but a bunch of stuff in the VFS.
For instance, what is the correct behaviour if I do a bind mount with
the source being an automount directory? Currently, that doesn't set
LOOKUP_DIRECTORY (they cant, since file may also be a mountpoint) and so
the current behaviour means that they will fail to automount.
I'm not sure if we care about umount() following a terminal automount,
but that too is affected and suffers from the 'doesn't have to be a
directory' syndrome.
Quotactl seems to be affected too. Dunno if quotactl is allowed to apply
to a file, but if it is, then we can't set LOOKUP_DIRECTORY.
Cheers
Trond
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust@xxxxxxxxxx
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
