[v2] NFS: Don't skip lookup when holding a delegation

Commit Message

Benjamin Coddington Sept. 19, 2019, 2:49 p.m. UTC

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 <[e-mail]>
---
 fs/nfs/dir.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Comments

Benjamin Coddington Jan. 16, 2020, 3:19 p.m. UTC | #1

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 <[e-mail]>
> ---
>  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

Trond Myklebust Jan. 16, 2020, 4:02 p.m. UTC | #2

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 <[e-mail]>
> > ---
> >  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?

Benjamin Coddington Jan. 16, 2020, 4:32 p.m. UTC | #3

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

Trond Myklebust Jan. 16, 2020, 4:44 p.m. UTC | #4

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...

Patch

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;