> On Feb 16, 2024, at 1:19 PM, Chuck Lever III <chuck.lever@xxxxxxxxxx> wrote:
>
>
>
>> On Feb 16, 2024, at 1:18 PM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>>
>> On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
>>> On Thu, Feb 15, 2024 at 08:24:50PM -0500, trondmy@xxxxxxxxxx wrote:
>>>> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>>>
>>>> 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?
Or, to be a little less terse... clients rely on the pre/post
op attributes around a SETATTR, I guess, but I don't see why.
I'm missing some context.
>>>> Fixes: bb4d53d66e4b ("NFSD: use (un)lock_inode instead of
>>>> fh_(un)lock for file operations")
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxxxxxxx
>
>
> --
> Chuck Lever
--
Chuck Lever
