Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()

[Trond Myklebust <trondmy@hammerspace.com>]

On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > Commit bb4d53d66e4b broke the NFSv3 pre/post op attributes
> > behaviour
> > when doing a SETATTR rpc call by stripping out the calls to
> > fh_fill_pre_attrs() and fh_fill_post_attrs().
> 
> Can you give more detail about what broke?

Without the calls to fh_fill_pre_attrs() and fh_fill_post_attrs(), we
don't store any pre/post op attributes and we can't return any such
attributes to the NFSv3 client.

> 
> 
> > Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
> > fh_(un)lock for file operations")
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 4 ++++
> >  fs/nfsd/vfs.c      | 9 +++++++--
> >  2 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 14712fa08f76..e6d8624efc83 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1143,6 +1143,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  	};
> >  	struct inode *inode;
> >  	__be32 status = nfs_ok;
> > +	bool save_no_wcc;
> >  	int err;
> >  
> >  	if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
> > @@ -1168,8 +1169,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >  
> >  	if (status)
> >  		goto out;
> > +	save_no_wcc = cstate->current_fh.fh_no_wcc;
> > +	cstate->current_fh.fh_no_wcc = true;
> >  	status = nfsd_setattr(rqstp, &cstate->current_fh, &attrs,
> >  				0, (time64_t)0);
> > +	cstate->current_fh.fh_no_wcc = save_no_wcc;
> >  	if (!status)
> >  		status = nfserrno(attrs.na_labelerr);
> >  	if (!status)
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 6e7e37192461..58fab461bc00 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -497,7 +497,7 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  	int		accmode = NFSD_MAY_SATTR;
> >  	umode_t		ftype = 0;
> >  	__be32		err;
> > -	int		host_err;
> > +	int		host_err = 0;
> >  	bool		get_write_count;
> >  	bool		size_change = (iap->ia_valid & ATTR_SIZE);
> >  	int		retries;
> > @@ -555,6 +555,9 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  	}
> >  
> >  	inode_lock(inode);
> > +	err = fh_fill_pre_attrs(fhp);
> > +	if (err)
> > +		goto out_unlock;
> >  	for (retries = 1;;) {
> >  		struct iattr attrs;
> >  
> > @@ -582,13 +585,15 @@ nfsd_setattr(struct svc_rqst *rqstp, struct
> > svc_fh *fhp,
> >  		attr->na_aclerr = set_posix_acl(&nop_mnt_idmap,
> >  						dentry,
> > ACL_TYPE_DEFAULT,
> >  						attr->na_dpacl);
> > +	fh_fill_post_attrs(fhp);
> > +out_unlock:
> >  	inode_unlock(inode);
> >  	if (size_change)
> >  		put_write_access(inode);
> >  out:
> >  	if (!host_err)
> >  		host_err = commit_metadata(fhp);
> > -	return nfserrno(host_err);
> > +	return err != 0 ? err : nfserrno(host_err);
> >  }
> >  
> >  #if defined(CONFIG_NFSD_V4)
> > -- 
> > 2.43.1
> > 
> > 
> 

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com