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

[Chuck Lever III <chuck.lever@oracle.com>]

> On Feb 16, 2024, at 1:18 PM, Trond Myklebust <trondmy@hammerspace.com> wrote:
> 
> 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.

I get that. Why does that matter?


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


--
Chuck Lever