d_revalidate not being called enough on mountpoints?

d_revalidate not being called enough on mountpoints?

[David Howells]

Hi Al,

I think there might be a problem in the VFS with d_revalidate() not being
called enough on mountpoints.  As far as I can tell from the printks in my AFS
stuff, it's only called on the mounted-on dentry, and not the vfsmount-root
dentry.  However, with NFS at least (not so much AFS), can you trust that the
mount-root dentry still maps to the same inode and event if it does that that
inode is still up to date?

I discovered it because I was relying on d_revalidate() to spot that the
server had broken the callback on a directory that had been changed.  However,
the root directory of each volume isn't being d_revalidated.

In the kernel output, I see:

(1) Permission check on the root dir of /afs: volume ID 20000001, vnode ID 1:

[0ls    ] ==> afs_permission({20000001:1},1,)

(2) Revalidation of the mountpoint dir in /afs (vnode ID 6):

[0ls    ] ==> afs_d_revalidate({v={20000001:6} n=.cambridge.redhat.com fl=20},)
[0ls    ]     not promised

(3) At this point, the stats of the mounted-on dir are rechecked.

[0ls    ]     new promise [fl=20]

(4) Permission check on the root dir of /afs/.cambridge.redhat.com (volume ID
    20000003, vnode ID 1):

[0ls    ] ==> afs_permission({20000003:1},1,)

(5) Revalidation of the mountpoint dir in /afs/.cambridge.redhat.com (vnode ID
    2):

[0ls    ] ==> afs_d_revalidate({v={20000003:2} n=afsdoc fl=20},)
[0ls    ]     not promised

(6) Again, the stats of the mounted-on dir are rechecked.

[0ls    ]     new promise [fl=20]

I was expecting to see the root dentry of each vfsmount be revalidated, but
that doesn't occur.

David

Re: d_revalidate not being called enough on mountpoints?

[Trond Myklebust]

On Mon, 2007-04-16 at 13:50 +0100, David Howells wrote:
> Hi Al,
> 
> I think there might be a problem in the VFS with d_revalidate() not being
> called enough on mountpoints.  As far as I can tell from the printks in my AFS
> stuff, it's only called on the mounted-on dentry, and not the vfsmount-root
> dentry.  However, with NFS at least (not so much AFS), can you trust that the
> mount-root dentry still maps to the same inode and event if it does that that
> inode is still up to date?

The NFS mountpoint is _not_ defined in terms of the remote path. Once
you have mounted a given directory, the expectation is that that
particular directory stays mounted. You cannot allow it to morph into
another object just because someone messes with the path on the server. 

This is very much analogous to the expectation that a bind mount should
not morph into a different object just because someone has renamed
something in the directory you were binding from.

> I discovered it because I was relying on d_revalidate() to spot that the
> server had broken the callback on a directory that had been changed.  However,
> the root directory of each volume isn't being d_revalidated.

That sounds like an abuse. You are not revalidating the path itself
(which is the purpose of d_revalidate). Instead you are revalidating the
directory metadata, which is a very different thing.

Cheers
  Trond

Re: d_revalidate not being called enough on mountpoints?

[David Howells]

Trond Myklebust <trond.myklebust@fys.uio.no> wrote:

> > I discovered it because I was relying on d_revalidate() to spot that the
> > server had broken the callback on a directory that had been changed.
> > However, the root directory of each volume isn't being d_revalidated.
> 
> That sounds like an abuse. You are not revalidating the path itself
> (which is the purpose of d_revalidate). Instead you are revalidating the
> directory metadata, which is a very different thing.

More to the point I'm attempting to revalidate the contents of the directory,
so that I can honour the next d_revalidate() call down the line.  There is no
lookup op in AFS as there is in NFS: the way things are done is that the
client's lookup parses the directory blob to map a name to a FID.

Maybe then what I need to do is revalidate the parent directory's contents in
each d_revalidate().  That has fun with rename, but if I do it under get ref
and lock, it should be okay.  I'll just have to be careful to avoid deadlock.

Alternatively, is the permission() call made prior to each d_revalidate()?  If
so, maybe I can use that:

[0ls    ] ==> afs_permission({{20000001:1},0},1,)
[0ls    ] ==> afs_d_revalidate({v={20000001:6} n=.cambridge.redhat.com fl=20},)
[0ls    ] ==> afs_permission({{20000003:1},0},1,)
[0ls    ] ==> afs_d_revalidate({v={20000003:2} n=afsdoc fl=20},)
[0ls    ] ==> afs_permission({{20000006:1},0},1,)
[0ls    ] ==> afs_d_revalidate({v={20000006:4} n=NEWS fl=0},)

Except that that doesn't get called on the last item in the chain (if I do,
say, just an ls of it).  That probably means I need to make the getattr() op
do content revalidation too.

David