Re: [PATCH 1/2] nfsd: hold reference to delegation when updating it for cb_getattr

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2024-08-24 at 11:03 -0400, Chuck Lever wrote:
> On Fri, Aug 23, 2024 at 06:27:38PM -0400, Jeff Layton wrote:
> > Once we've dropped the flc_lock, there is nothing that ensures that the
> > delegation that was found will still be around later. Take a reference
> > to it while holding the lock and then drop it when we've finished with
> > the delegation.
> > 
> > Fixes: c5967721e106 ("NFSD: handle GETATTR conflict with write delegation")
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/nfsd/nfs4state.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index dafff707e23a..19d39872be32 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -8837,7 +8837,6 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> >  	struct file_lock_context *ctx;
> >  	struct file_lease *fl;
> > -	struct nfs4_delegation *dp;
> >  	struct iattr attrs;
> >  	struct nfs4_cb_fattr *ncf;
> >  
> > @@ -8862,7 +8861,8 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  			goto break_lease;
> >  		}
> >  		if (type == F_WRLCK) {
> > -			dp = fl->c.flc_owner;
> > +			struct nfs4_delegation *dp = fl->c.flc_owner;
> 
> Setting @dp here seems redundant; just below, after the break_lease
> label it is set again to the same value. May I change this line to:
> 
> 			struct nfs4_delegation *dp;
> 

I don't think you can just remove that one since it's dereferenced just
after that. The problem is the goto break_lease case needs to have that
assigned too so you also can't just remove the later one.

The way the code flows here is weird, unfortunately, but I don't see an
easy way to improve it right offhand. Maybe assign "dp" just before the
"goto break_lease" ?


> > +
> >  			if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> >  				spin_unlock(&ctx->flc_lock);
> >  				return 0;
> > @@ -8870,6 +8870,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  break_lease:
> >  			nfsd_stats_wdeleg_getattr_inc(nn);
> >  			dp = fl->c.flc_owner;
> > +			refcount_inc(&dp->dl_stid.sc_count);
> >  			ncf = &dp->dl_cb_fattr;
> >  			nfs4_cb_getattr(&dp->dl_cb_fattr);
> >  			spin_unlock(&ctx->flc_lock);
> > @@ -8879,8 +8880,10 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  				/* Recall delegation only if client didn't respond */
> >  				status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> >  				if (status != nfserr_jukebox ||
> > -						!nfsd_wait_for_delegreturn(rqstp, inode))
> > +						!nfsd_wait_for_delegreturn(rqstp, inode)) {
> > +					nfs4_put_stid(&dp->dl_stid);
> >  					return status;
> > +				}
> >  			}
> >  			if (!ncf->ncf_file_modified &&
> >  					(ncf->ncf_initial_cinfo != ncf->ncf_cb_change ||
> > @@ -8900,6 +8903,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode,
> >  				*size = ncf->ncf_cur_fsize;
> >  				*modified = true;
> >  			}
> > +			nfs4_put_stid(&dp->dl_stid);
> >  			return 0;
> >  		}
> >  		break;
> > 
> > -- 
> > 2.46.0
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>