| Message ID | 20140714151405.2fa06dd7@notabene.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 14 Jul 2014 15:14:05 +1000 NeilBrown <neilb@suse.de> 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? 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. Are we not properly updating the dcache (and attrcache) after the OPEN reply? > > Signed-off-by: NeilBrown <neilb@suse.de> > > 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@primarydata.com>
On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton <jeff.layton@primarydata.com> wrote: > On Mon, 14 Jul 2014 15:14:05 +1000 > NeilBrown <neilb@suse.de> 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?? > > 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@suse.de> > > > > 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@primarydata.com> Thanks, NeilBrown
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;
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. Signed-off-by: NeilBrown <neilb@suse.de>