Message ID | 77be993185fa7f114f6856f74f2f7affb5bd411d.1568904510.git.bcodding@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v2] NFS: Don't skip lookup when holding a delegation | expand |
I'd like to bump this one again while we're talking about lookup/revalidation. We have folks that hit this problem in the field: - client caches a dentry - file gets moved - server gives out a delegation - client never notices the change because we always skip revalidation Is this the wrong place to fix this? Any other feedback? Ben On 19 Sep 2019, at 10:49, Benjamin Coddington wrote: > If we skip lookup revalidation while holding a delegation, we might > miss > that the file has changed directories on the server. The directory's > change attribute should still be checked against the dentry's d_time > to > perform a complete revalidation. > > V2 - Add some commentary as suggested-by J. Bruce Fields. > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > --- > fs/nfs/dir.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > index 0adfd8840110..8723e82f5c9d 100644 > --- a/fs/nfs/dir.c > +++ b/fs/nfs/dir.c > @@ -1197,12 +1197,20 @@ nfs_do_lookup_revalidate(struct inode *dir, > struct dentry *dentry, > goto out_bad; > } > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > - > /* Force a full look up iff the parent directory has changed */ > if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && > nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { > + > + /* > + * Note that the file can't move while we hold a > + * delegation. But this dentry could have been cached > + * before we got a delegation. So it's only safe to > + * skip revalidation when the parent directory is > + * unchanged: > + */ > + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > + return nfs_lookup_revalidate_delegated(dir, dentry, inode); > + > error = nfs_lookup_verify_inode(inode, flags); > if (error) { > if (error == -ESTALE) > @@ -1635,9 +1643,6 @@ nfs4_do_lookup_revalidate(struct inode *dir, > struct dentry *dentry, > if (inode == NULL) > goto full_reval; > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > - return nfs_lookup_revalidate_delegated(dir, dentry, inode); > - > /* NFS only supports OPEN on regular files */ > if (!S_ISREG(inode->i_mode)) > goto full_reval; > -- > 2.20.1
On Thu, 2020-01-16 at 10:19 -0500, Benjamin Coddington wrote: > I'd like to bump this one again while we're talking about > lookup/revalidation. > > We have folks that hit this problem in the field: > - client caches a dentry > - file gets moved > - server gives out a delegation > - client never notices the change because we always skip > revalidation > > Is this the wrong place to fix this? Any other feedback? > > Ben > > On 19 Sep 2019, at 10:49, Benjamin Coddington wrote: > > > If we skip lookup revalidation while holding a delegation, we > > might > > miss > > that the file has changed directories on the server. The > > directory's > > change attribute should still be checked against the dentry's > > d_time > > to > > perform a complete revalidation. > > > > V2 - Add some commentary as suggested-by J. Bruce Fields. > > > > Signed-off-by: Benjamin Coddington <bcodding@redhat.com> > > --- > > fs/nfs/dir.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > index 0adfd8840110..8723e82f5c9d 100644 > > --- a/fs/nfs/dir.c > > +++ b/fs/nfs/dir.c > > @@ -1197,12 +1197,20 @@ nfs_do_lookup_revalidate(struct inode > > *dir, > > struct dentry *dentry, > > goto out_bad; > > } > > > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > > - return nfs_lookup_revalidate_delegated(dir, dentry, > > inode); > > - > > /* Force a full look up iff the parent directory has changed */ > > if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && > > nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { > > + > > + /* > > + * Note that the file can't move while we hold a > > + * delegation. But this dentry could have been cached > > + * before we got a delegation. So it's only safe to > > + * skip revalidation when the parent directory is > > + * unchanged: > > + */ > > + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > > + return nfs_lookup_revalidate_delegated(dir, > > dentry, inode); > > + > > error = nfs_lookup_verify_inode(inode, flags); > > if (error) { > > if (error == -ESTALE) > > @@ -1635,9 +1643,6 @@ nfs4_do_lookup_revalidate(struct inode *dir, > > struct dentry *dentry, > > if (inode == NULL) > > goto full_reval; > > > > - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) > > - return nfs_lookup_revalidate_delegated(dir, dentry, > > inode); > > - > > /* NFS only supports OPEN on regular files */ > > if (!S_ISREG(inode->i_mode)) > > goto full_reval; > > -- > > 2.20.1 We should need to perform this revalidation once, and only once for that directory, and only if we opened the file using a CLAIM_FH open, or if we opened it through a different hard linked name (and did not create this hard link after we got the delegation). Perhaps we could define a magic value for dentry->d_time that causes us to skip revalidation if and only if we hold a delegation?
On 16 Jan 2020, at 11:02, Trond Myklebust wrote: > We should need to perform this revalidation once, and only once for > that directory, and only if we opened the file using a CLAIM_FH open, > or if we opened it through a different hard linked name (and did not > create this hard link after we got the delegation). > > Perhaps we could define a magic value for dentry->d_time that causes us > to skip revalidation if and only if we hold a delegation? Can we put the delegation's change_attr in d_time for dentries that have been revalided while holding a delegation? Ben
On Thu, 2020-01-16 at 11:32 -0500, Benjamin Coddington wrote: > On 16 Jan 2020, at 11:02, Trond Myklebust wrote: > > We should need to perform this revalidation once, and only once for > > that directory, and only if we opened the file using a CLAIM_FH > > open, > > or if we opened it through a different hard linked name (and did > > not > > create this hard link after we got the delegation). > > > > Perhaps we could define a magic value for dentry->d_time that > > causes us > > to skip revalidation if and only if we hold a delegation? > > Can we put the delegation's change_attr in d_time for dentries that > have > been revalided while holding a delegation? We could, but that seems more complicated because it means you need to decide whether you are checking the parent inode or your own inode. It seems easier to just have nfs_force_lookup_revalidate() skip the magic value so that we know that there are no collisions. Once the revalidation is done on the dentry holding the delegation, then we can set dentry->d_time to the magic value, and we're done...
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 0adfd8840110..8723e82f5c9d 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1197,12 +1197,20 @@ nfs_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, goto out_bad; } - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) - return nfs_lookup_revalidate_delegated(dir, dentry, inode); - /* Force a full look up iff the parent directory has changed */ if (!(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) && nfs_check_verifier(dir, dentry, flags & LOOKUP_RCU)) { + + /* + * Note that the file can't move while we hold a + * delegation. But this dentry could have been cached + * before we got a delegation. So it's only safe to + * skip revalidation when the parent directory is + * unchanged: + */ + if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) + return nfs_lookup_revalidate_delegated(dir, dentry, inode); + error = nfs_lookup_verify_inode(inode, flags); if (error) { if (error == -ESTALE) @@ -1635,9 +1643,6 @@ nfs4_do_lookup_revalidate(struct inode *dir, struct dentry *dentry, if (inode == NULL) goto full_reval; - if (NFS_PROTO(dir)->have_delegation(inode, FMODE_READ)) - return nfs_lookup_revalidate_delegated(dir, dentry, inode); - /* NFS only supports OPEN on regular files */ if (!S_ISREG(inode->i_mode)) goto full_reval;
If we skip lookup revalidation while holding a delegation, we might miss that the file has changed directories on the server. The directory's change attribute should still be checked against the dentry's d_time to perform a complete revalidation. V2 - Add some commentary as suggested-by J. Bruce Fields. Signed-off-by: Benjamin Coddington <bcodding@redhat.com> --- fs/nfs/dir.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)