On Mon, 14 Jul 2014 22:35:13 +1000
NeilBrown <neilb@xxxxxxx> wrote:
> On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton <jeff.layton@xxxxxxxxxxxxxxx>
> wrote:
>
> > On Mon, 14 Jul 2014 15:14:05 +1000
> > NeilBrown <neilb@xxxxxxx> wrote:
> >
> > >
> > > If an 'open' of a file in an NFSv4 filesystem finds that the dentry is
> > > in cache, but the inode is stale (on the server), the dentry will not
> > > be re-validated immediately and may cause ESTALE to be returned to
> > > user-space.
> > >
> > > For a non-create 'open', do_last() calls lookup_fast() and on success
> > > will eventually call may_open() which calls into nfs_permission().
> > > If nfs_permission() makes the ACCESS call to the server it will get
> > > NFS4ERR_STALE, resulting in ESTALE from may_open() and thence from
> > > do_last().
> > > The retry-on-ESTALE in filename_lookup() will repeat exactly the same
> > > process because nothing in this path will invalidate the dentry due to
> > > the inode being stale, so the ESTALE will be returned.
> > >
> > > lookup_fast() calls ->d_revalidate(), but for an OPEN on an NFSv4
> > > filesystem, that will succeed for regular files:
> > > /* Let f_op->open() actually open (and revalidate) the file */
> > >
> > > Unfortunately in the case of a STALE inode, f_op->open() never gets
> > > called. If we teach nfs4_lookup_revalidate() to report a failure on
> > > NFS_STALE() inodes, then the dentry will be invalidated and a full
> > > lookup will be attempted. The ESTALE errors go away.
> > >
> > >
> > > While I think this fix is correct, I'm not convinced that it is
> > > sufficient, particularly if lookupcache=none.
> > > The current code will fail an "open" is nfs_permission() fails,
> > > without having performed a LOOKUP. i.e. it will use the cache.
> > > nfs_lookup_revalidate will force a lookup before the permission check
> > > if NFS_MOUNT_LOOKUP_CACHE_NONE, but nfs4_lookup_revalidate will not.
> > >
> >
> > This patch should make the code fall through to nfs_lookup_revalidate,
> > which would then force the lookup, right?
>
> Yes ... though maybe that's not what I really want to do. I really wanted to
> just return '0', though I would need to check that is right in all cases.
>
> >
> > Also, I'm a little unclear...
> >
> > Why would may_open fail with ESTALE after the v4 OPEN succeeds? The
> > OPEN should be returning a filehandle and attributes for the inode
> > actually opened. It seems like we ought to be doing any permission
> > checks vs. that inode, not anything we had in cache. Presumably the
> > server is then holding it open so it shouldn't be stale.
>
> may_open is called *before* and v4 OPEN.
>
> In do_last, if the inode is already in cache, then
> lookup_fast is called, which calls d_revalidate
> then may_open (calls ->permission)
> then finish_open which calls f_op->open
>
> Yes, we should be doing permission checking against whatever 'open' finds.
> But the VFS is structured to the the permission check after d_revalidate and
> before ->open. So maybe d_revalidate needs to do the NFS open??
>
Ok, I see. Ugh, having the revalidate do the open sounds...messy.
A simpler fix might be to fix it so that an -ESTALE return from
may_open triggers a retry. Something like this maybe (probably
whitespace damaged, so just for discussion purposes):
diff --git a/fs/namei.c b/fs/namei.c
index 985c6f368485..c1657deea52c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3045,8 +3045,13 @@ finish_open:
}
finish_open_created:
error = may_open(&nd->path, acc_mode, open_flag);
- if (error)
+ if (error) {
+ if (error == -ESTALE)
+ goto stale_open;
goto out;
+ }
file->f_path.mnt = nd->path.mnt;
error = finish_open(file, nd->path.dentry, NULL, opened);
if (error) {
...though might need to convert the ESTALE to EOPENSTALE there too, not
sure...
>
> >
> > Are we not properly updating the dcache (and attrcache) after the
> > OPEN reply?
>
> I think so, yes. But in the problem case, we don't even send an OPEN
> request.
>
>
> >
> > >
> > > Signed-off-by: NeilBrown <neilb@xxxxxxx>
> > >
> > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> > > index 4a3d4ef76127..4f7414afca27 100644
> > > --- a/fs/nfs/dir.c
> > > +++ b/fs/nfs/dir.c
> > > @@ -1563,6 +1563,8 @@ static int nfs4_lookup_revalidate(struct
> > > dentry *dentry, unsigned int flags) /* We cannot do exclusive
> > > creation on a positive dentry */ if (flags & LOOKUP_EXCL)
> > > goto no_open_dput;
> > > + if (NFS_STALE(inode))
> > > + goto no_open_dput;
> > >
> > > /* Let f_op->open() actually open (and revalidate) the
> > > file */ ret = 1;
> >
> > Looks legit to me too, but it seems like the inode could go stale
> > w/o us knowing after this point.
> >
> > Acked-by: Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
>
> Thanks,
> NeilBrown
--
Jeff Layton <jlayton@xxxxxxxxxxxxxxx>
Attachment:
signature.asc
Description: PGP signature
