Re: [PATCH 2/2] vfs: force reval on dentry of bind mounted files on FS_REVAL_DOT filesystems

[ebiederm@xmission.com (Eric W. Biederman)]

Jeff Layton <jlayton@redhat.com> writes:

> On Thu, 03 Dec 2009 11:58:43 +0100
> Miklos Szeredi <miklos@szeredi.hu> wrote:
>
>> On Wed,  2 Dec 2009, Jeff Layton wrote:
>> > In the case of a bind mounted file, the path walking code will assume
>> > that the cached dentry that was bind mounted is valid. This is a problem
>> > problem for NFSv4 in a way that's similar to LAST_BIND symlinks.
>> > 
>> > Fix this by revalidating the dentry if FS_FOLLOW_DOT is set and
>> > __follow_mount returns true.
>> > 
>> > Note that in the non-open codepath, we cannot return an error to the
>> > lookup if the revalidation fails. Doing so will leave a bind mount in
>> > a state such that we can't unmount it. In that case we'll just have to
>> > settle for d_invalidating it (which should mostly turn out to be a
>> > d_drop in this case) and returning success.
>> 
>> The only worry I have is that this adds an extra branch in a very hot
>> codepath (do_lookup).  An error can't be returned, as you note, and
>> for bind mounted directories d_invalidate() will not succeed: the
>> directory is busy, it's referenced by the mount.  So basically the
>> only thing this does is working around the NFSv4 issue.  But Trond has
>> a proper solution to that, and a temporary solution could be added to
>> do_filp_open() rather than burdening do_lookup() with it, no?
>> 
>
> (re-adding Trond. I forgot to cc him on this latest set)
>
> Self-NAK on this patch...
>
> That's my main worry too, and sadly it doesn't seem to be unfounded.
> This patch adds a lot of extra d_revalidate calls here. I think it's
> going to be too expensive to do this.

How so?  We should only see extra calls if we follow a mount point.
Currently we call d_revalidate on every path component.

> The only problem I've identified that this fixes is with file bind
> mounts and I don't get the impression they're that common. Maybe the
> best thing is to just fix the LAST_BIND symlink case for now and wait
> for Trond or Al's overhaul of this code.

Well right now following mount points breaks the VFS contract that we
will revalidate all dentries before we use them.  That breaking of the
contract breaks NFS.

I don't know what else d_revalidate is good for.  On the sysfs side
I only use it to unhash the dentry.  Something we don't care about
from the do_lookup side of things if we have a bind mount.

I'm not clear what kind of changes revalidating a deleted but open
file will give you on NFS.

Eric