On Thu, 2010-05-06 at 14:13 +1000, Neil Brown wrote:
> On Wed, 21 Apr 2010 17:03:21 +1000
> Neil Brown <neilb@xxxxxxx> wrote:
>
> > On Tue, 20 Apr 2010 09:02:01 -0400
> > Trond Myklebust <trond.myklebust@xxxxxxxxxx> wrote:
> >
> > > On Tue, 2010-04-20 at 17:22 +1000, Neil Brown wrote:
> > > > Hi Trond et al,
> > > >
> > > > It has come to my attention that NFS directories don't behave consistently
> > > > in terms of cache consistency.
> > > >
> > > > If, on the client, you have a loop like:
> > > >
> > > > while true; do sleep 1; ls -l $dirname ; done
> > > >
> > > > and then on the server you make changes to the named directory, there are
> > > > some cases where you will see changes promptly and some where you wont.
> > > >
> > > > In particular, if $dirname is '.' or the name of an NFS mountpoint, then
> > > > changes can be delayed by up to acdirmax. If it is any other path, i.e. with
> > > > a non-trivial path component that is in the NFS filesystem, then changes
> > > > are seen promptly.
> > > >
> > > > This seems to me to relate to "close to open" consistency. Of course with
> > > > directories the 'close' side isn't relevant, but I still think it should be
> > > > that when you open a directory it validates the 'change' attribute on that
> > > > directory over the wire.
> > > >
> > > > However the Linux VFS never tells NFS when a directory is opened. The
> > > > current correct behaviour for most directories is achieved through
> > > > d_revalidate == nfs_lookup_revalidate.
> > > >
> > > > For '.' and mountpoints we need a different approach. Possibly the VFS could
> > > > be changed to tell the filesystem when such a directory is opened. However I
> > > > don't feel up to that at the moment.
> > >
> > > I agree that mountpoints are problematic in this case, however why isn't
> > > '.' working correctly? Is the FS_REVAL_DOT mechanism broken?
> >
> > Yes, the FS_REVAL_DOT mechanism is broken.
> > Specifically, when you open ".", ->d_revalidate is called by link_path_walk,
> > but LOOKUP_PARENT is set, and LOOKUP_OPEN is not set, so
> > nfs_lookup_verify_inode doesn't force a revalidate.
> >
> > Then in do_last(), LOOKUP_PARENT is no longer set, and LOOKUP_OPEN is, but
> > do_last doesn't bother calling ->d_revalidate for LAST_DOT.
> >
> > I verified this understanding with the following patch which causes
> > "ls ." to reliably get current (rather than cached) contents of the directory.
>
>
> No replies ... Maybe Al is busy.
>
> I looked at this again, created a patch that I thought looked good and tested
> it to ensure it addressed both sides of the problem.
>
> Does it look OK to you Trond?
> Thanks.
>
> NFS - ensure directory at end of path is always revalidated.
>
> The FS_REVAL_DOT fs_type flag is meant to ensure that the final component of
> a path is always revalidated, even if it isn't a normal (LAST_NORM) path
> component (which is always revalidated).
> There are two cases where this doesn't happen for NFS
> One is where the last component is '.' as the revalidation happens while
> LOOKUP_PARENT is set, so NFS ignores it (see nfs_lookup_check_intent).
> The other is where the directory is a mountpoint, so it is LAST_NORM,
> but that directory is different from the mounted directory.
>
> This patches fixes these two issues by
> 1/ extending do_last() to revalidate DOT as well as DOTDOT and
> 2/ extending do_lookup() to revalidate after a successful __follow_mount
> if FS_REVAL_DOT is set.
>
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>
>
> diff --git a/fs/namei.c b/fs/namei.c
> index a7dce91..256ae13 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -719,7 +719,11 @@ static int do_lookup(struct nameidata *nd, struct qstr *name,
> done:
> path->mnt = mnt;
> path->dentry = dentry;
> - __follow_mount(path);
> + if (__follow_mount(path) &&
> + (path->mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT)) {
> + if (!path->dentry->d_op->d_revalidate(path->dentry, nd))
> + return -ESTALE;
Won't this prevent you from ever being able to unmount the stale
filesystem?
> + }
> return 0;
>
> need_lookup:
> @@ -1619,6 +1623,7 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
> switch (nd->last_type) {
> case LAST_DOTDOT:
> follow_dotdot(nd);
> + case LAST_DOT:
> dir = nd->path.dentry;
> if (nd->path.mnt->mnt_sb->s_type->fs_flags & FS_REVAL_DOT) {
> if (!dir->d_op->d_revalidate(dir, nd)) {
> @@ -1627,7 +1632,6 @@ static struct file *do_last(struct nameidata *nd, struct path *path,
> }
> }
> /* fallthrough */
> - case LAST_DOT:
> case LAST_ROOT:
> if (open_flag & O_CREAT)
> goto exit;
>
--
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
