Re: [PATCH v2 1/2] nfsd: handle failure to collect pre/post-op attrs more sanely

[Jeff Layton]

On Thu, 2023-07-20 at 19:09 -0400, Chuck Lever wrote:
> On Fri, Jul 21, 2023 at 07:46:20AM +1000, NeilBrown wrote:
> > On Fri, 21 Jul 2023, Jeff Layton wrote:
> > > Collecting pre_op_attrs can fail, in which case it's probably best to
> > > fail the whole operation.
> > > 
> > > Change fh_fill_{pre,post,both}_attrs to return __be32. For the pre and
> > > both functions, have the callers check the return code and abort the
> > > operation if it failed.
> > > 
> > > If fh_fill_post_attrs fails, then it's too late to do anything about it,
> > > so most of those callers ignore the return value. If this happens, then
> > > fh_post_saved will be false, which should cue the later stages to deal
> > > with it.
> > > 
> > > Suggested-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > >  fs/nfsd/nfs3proc.c |  4 +++-
> > >  fs/nfsd/nfs4proc.c | 14 ++++++------
> > >  fs/nfsd/nfsfh.c    | 26 ++++++++++++++---------
> > >  fs/nfsd/nfsfh.h    |  6 +++---
> > >  fs/nfsd/vfs.c      | 62 ++++++++++++++++++++++++++++++++++--------------------
> > >  5 files changed, 69 insertions(+), 43 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
> > > index fc8d5b7db9f8..268ef57751c4 100644
> > > --- a/fs/nfsd/nfs3proc.c
> > > +++ b/fs/nfsd/nfs3proc.c
> > > @@ -307,7 +307,9 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >  	if (!IS_POSIXACL(inode))
> > >  		iap->ia_mode &= ~current_umask();
> > >  
> > > -	fh_fill_pre_attrs(fhp);
> > > +	status = fh_fill_pre_attrs(fhp);
> > > +	if (status != nfs_ok)
> > > +		goto out;
> > >  	host_err = vfs_create(&nop_mnt_idmap, inode, child, iap->ia_mode, true);
> > >  	if (host_err < 0) {
> > >  		status = nfserrno(host_err);
> > > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > > index d8e7a533f9d2..9285e1eab4d5 100644
> > > --- a/fs/nfsd/nfs4proc.c
> > > +++ b/fs/nfsd/nfs4proc.c
> > > @@ -297,12 +297,12 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >  	}
> > >  
> > >  	if (d_really_is_positive(child)) {
> > > -		status = nfs_ok;
> > > -
> > >  		/* NFSv4 protocol requires change attributes even though
> > >  		 * no change happened.
> > >  		 */
> > > -		fh_fill_both_attrs(fhp);
> > > +		status = fh_fill_both_attrs(fhp);
> > > +		if (status != nfs_ok)
> > > +			goto out;
> > >  
> > >  		switch (open->op_createmode) {
> > >  		case NFS4_CREATE_UNCHECKED:
> > > @@ -345,7 +345,9 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >  	if (!IS_POSIXACL(inode))
> > >  		iap->ia_mode &= ~current_umask();
> > >  
> > > -	fh_fill_pre_attrs(fhp);
> > > +	status = fh_fill_pre_attrs(fhp);
> > > +	if (status != nfs_ok)
> > > +		goto out;
> > >  	status = nfsd4_vfs_create(fhp, child, open);
> > >  	if (status != nfs_ok)
> > >  		goto out;
> > > @@ -424,11 +426,11 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
> > >  	} else {
> > >  		status = nfsd_lookup(rqstp, current_fh,
> > >  				     open->op_fname, open->op_fnamelen, *resfh);
> > > -		if (!status)
> > > +		if (status == nfs_ok)
> > >  			/* NFSv4 protocol requires change attributes even though
> > >  			 * no change happened.
> > >  			 */
> > > -			fh_fill_both_attrs(current_fh);
> > > +			status = fh_fill_both_attrs(current_fh);
> > >  	}
> > >  	if (status)
> > >  		goto out;
> > > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c
> > > index c291389a1d71..f7e68a91e826 100644
> > > --- a/fs/nfsd/nfsfh.c
> > > +++ b/fs/nfsd/nfsfh.c
> > > @@ -614,7 +614,7 @@ fh_update(struct svc_fh *fhp)
> > >   * @fhp: file handle to be updated
> > >   *
> > >   */
> > > -void fh_fill_pre_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > >  	struct inode *inode;
> > > @@ -622,12 +622,12 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc || fhp->fh_pre_saved)
> > > -		return;
> > > +		return nfs_ok;
> > >  
> > >  	inode = d_inode(fhp->fh_dentry);
> > >  	err = fh_getattr(fhp, &stat);
> > >  	if (err)
> > > -		return;
> > > +		return err;
> > >  
> > >  	if (v4)
> > >  		fhp->fh_pre_change = nfsd4_change_attribute(&stat, inode);
> > > @@ -636,6 +636,7 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >  	fhp->fh_pre_ctime = stat.ctime;
> > >  	fhp->fh_pre_size  = stat.size;
> > >  	fhp->fh_pre_saved = true;
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /**
> > > @@ -643,26 +644,27 @@ void fh_fill_pre_attrs(struct svc_fh *fhp)
> > >   * @fhp: file handle to be updated
> > >   *
> > >   */
> > > -void fh_fill_post_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp)
> > >  {
> > >  	bool v4 = (fhp->fh_maxsize == NFS4_FHSIZE);
> > >  	struct inode *inode = d_inode(fhp->fh_dentry);
> > >  	__be32 err;
> > >  
> > >  	if (fhp->fh_no_wcc)
> > > -		return;
> > > +		return nfs_ok;
> > >  
> > >  	if (fhp->fh_post_saved)
> > >  		printk("nfsd: inode locked twice during operation.\n");
> > >  
> > >  	err = fh_getattr(fhp, &fhp->fh_post_attr);
> > >  	if (err)
> > > -		return;
> > > +		return err;
> > >  
> > >  	fhp->fh_post_saved = true;
> > >  	if (v4)
> > >  		fhp->fh_post_change =
> > >  			nfsd4_change_attribute(&fhp->fh_post_attr, inode);
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /**
> > > @@ -672,16 +674,20 @@ void fh_fill_post_attrs(struct svc_fh *fhp)
> > >   * This is used when the directory wasn't changed, but wcc attributes
> > >   * are needed anyway.
> > >   */
> > > -void fh_fill_both_attrs(struct svc_fh *fhp)
> > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp)
> > >  {
> > > -	fh_fill_post_attrs(fhp);
> > > -	if (!fhp->fh_post_saved)
> > > -		return;
> > > +	__be32 err;
> > > +
> > > +	err = fh_fill_post_attrs(fhp);
> > > +	if (err)
> > > +		return err;
> > > +
> > >  	fhp->fh_pre_change = fhp->fh_post_change;
> > >  	fhp->fh_pre_mtime = fhp->fh_post_attr.mtime;
> > >  	fhp->fh_pre_ctime = fhp->fh_post_attr.ctime;
> > >  	fhp->fh_pre_size = fhp->fh_post_attr.size;
> > >  	fhp->fh_pre_saved = true;
> > > +	return nfs_ok;
> > >  }
> > >  
> > >  /*
> > > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h
> > > index 4e0ecf0ae2cf..486803694acc 100644
> > > --- a/fs/nfsd/nfsfh.h
> > > +++ b/fs/nfsd/nfsfh.h
> > > @@ -294,7 +294,7 @@ static inline void fh_clear_pre_post_attrs(struct svc_fh *fhp)
> > >  }
> > >  
> > >  u64 nfsd4_change_attribute(struct kstat *stat, struct inode *inode);
> > > -extern void fh_fill_pre_attrs(struct svc_fh *fhp);
> > > -extern void fh_fill_post_attrs(struct svc_fh *fhp);
> > > -extern void fh_fill_both_attrs(struct svc_fh *fhp);
> > > +__be32 fh_fill_pre_attrs(struct svc_fh *fhp);
> > > +__be32 fh_fill_post_attrs(struct svc_fh *fhp);
> > > +__be32 fh_fill_both_attrs(struct svc_fh *fhp);
> > >  #endif /* _LINUX_NFSD_NFSFH_H */
> > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > > index 8a2321d19194..f200afd33630 100644
> > > --- a/fs/nfsd/vfs.c
> > > +++ b/fs/nfsd/vfs.c
> > > @@ -1537,9 +1537,11 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
> > >  	dput(dchild);
> > >  	if (err)
> > >  		goto out_unlock;
> > > -	fh_fill_pre_attrs(fhp);
> > > -	err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
> > > -	fh_fill_post_attrs(fhp);
> > > +	err = fh_fill_pre_attrs(fhp);
> > > +	if (err == nfs_ok) {
> > > +		err = nfsd_create_locked(rqstp, fhp, attrs, type, rdev, resfhp);
> > > +		fh_fill_post_attrs(fhp);
> > 
> > Most error handling in nfsd is
> >  
> >    if (err)
> >        goto ....
> > 
> > Here (and one other place I think) you have
> >    if (not err)
> >        do stuff;
> > 
> > Do we want to be more consistent?
> 
> Yes, unless being consistent makes this code unreadable. There
> doesn't seem to be a reason to drop that convention here.
> 

My usual test for this is to use gotos if unwinding errors is complex
enough to warrant it, and to just use the second form if the code is
fairly simple.

But...if you want gotos everywhere, then so be it. I'll respin this.

> 
> > I'm in two minds about this and I
> > don't dislike your patch.  But I noticed the inconsistency and thought I
> > should mention it.
> > 
> > Also, should we put a __must_check annotation on fh_fill_pre_attrs() and
> > ..post..?  Then I wouldn't have to manually check that you found and
> > fixed all callers (which I haven't).

Maybe for the "pre" and "both" ones. We would _not_ want to add
__must_check for the post one, since most of the callers (correctly)
ignore that return value.

I'll plan to roll that in.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>