Re: [PATCH v5 09/10] nfsd: handle delegated timestamps in SETATTR

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

On 12/14/24 12:02 PM, Chuck Lever wrote:
> On 12/14/24 11:34 AM, Chuck Lever wrote:
>> On 12/14/24 9:55 AM, Jeff Layton wrote:
>>> On Fri, 2024-12-13 at 09:18 -0500, Chuck Lever wrote:
>>>> On 12/13/24 9:14 AM, Jeff Layton wrote:
>>>>> On Thu, 2024-12-12 at 16:06 -0500, Chuck Lever wrote:
>>>>>> On 12/9/24 4:14 PM, Jeff Layton wrote:
>>>>>>> Allow SETATTR to handle delegated timestamps. This patch assumes 
>>>>>>> that
>>>>>>> only the delegation holder has the ability to set the timestamps 
>>>>>>> in this
>>>>>>> way, so we allow this only if the SETATTR stateid refers to a
>>>>>>> *_ATTRS_DELEG delegation.
>>>>>>>
>>>>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>>>>> ---
>>>>>>>     fs/nfsd/nfs4proc.c  | 31 ++++++++++++++++++++++++++++---
>>>>>>>     fs/nfsd/nfs4state.c |  2 +-
>>>>>>>     fs/nfsd/nfs4xdr.c   | 20 ++++++++++++++++++++
>>>>>>>     fs/nfsd/nfsd.h      |  5 ++++-
>>>>>>>     4 files changed, 53 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>>>>>>> index 
>>>>>>> f8a10f90bc7a4b288c20d2733c85f331cc0a8dba..fea171ffed623818c61886b786339b0b73f1053d 100644
>>>>>>> --- a/fs/nfsd/nfs4proc.c
>>>>>>> +++ b/fs/nfsd/nfs4proc.c
>>>>>>> @@ -1135,18 +1135,43 @@ nfsd4_setattr(struct svc_rqst *rqstp, 
>>>>>>> struct nfsd4_compound_state *cstate,
>>>>>>>             .na_iattr    = &setattr->sa_iattr,
>>>>>>>             .na_seclabel    = &setattr->sa_label,
>>>>>>>         };
>>>>>>> +    bool save_no_wcc, deleg_attrs;
>>>>>>> +    struct nfs4_stid *st = NULL;
>>>>>>>         struct inode *inode;
>>>>>>>         __be32 status = nfs_ok;
>>>>>>> -    bool save_no_wcc;
>>>>>>>         int err;
>>>>>>> -    if (setattr->sa_iattr.ia_valid & ATTR_SIZE) {
>>>>>>> +    deleg_attrs = setattr->sa_bmval[2] & 
>>>>>>> (FATTR4_WORD2_TIME_DELEG_ACCESS |
>>>>>>> +                          FATTR4_WORD2_TIME_DELEG_MODIFY);
>>>>>>> +
>>>>>>> +    if (deleg_attrs || (setattr->sa_iattr.ia_valid & ATTR_SIZE)) {
>>>>>>> +        int flags = WR_STATE;
>>>>>>> +
>>>>>>> +        if (setattr->sa_bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS)
>>>>>>> +            flags |= RD_STATE;
>>>>>>> +
>>>>>>>             status = nfs4_preprocess_stateid_op(rqstp, cstate,
>>>>>>>                     &cstate->current_fh, &setattr->sa_stateid,
>>>>>>> -                WR_STATE, NULL, NULL);
>>>>>>> +                flags, NULL, &st);
>>>>>>>             if (status)
>>>>>>>                 return status;
>>>>>>>         }
>>>>>>> +
>>>>>>> +    if (deleg_attrs) {
>>>>>>> +        status = nfserr_bad_stateid;
>>>>>>> +        if (st->sc_type & SC_TYPE_DELEG) {
>>>>>>> +            struct nfs4_delegation *dp = delegstateid(st);
>>>>>>> +
>>>>>>> +            /* Only for *_ATTRS_DELEG flavors */
>>>>>>> +            if (deleg_attrs_deleg(dp->dl_type))
>>>>>>> +                status = nfs_ok;
>>>>>>> +        }
>>>>>>> +    }
>>>>>>> +    if (st)
>>>>>>> +        nfs4_put_stid(st);
>>>>>>> +    if (status)
>>>>>>> +        return status;
>>>>>>> +
>>>>>>>         err = fh_want_write(&cstate->current_fh);
>>>>>>>         if (err)
>>>>>>>             return nfserrno(err);
>>>>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>>>>> index 
>>>>>>> c882eeba7830b0249ccd74654f81e63b12a30f14..a76e35f86021c5657e31e4fddf08cb5781f01e32 100644
>>>>>>> --- a/fs/nfsd/nfs4state.c
>>>>>>> +++ b/fs/nfsd/nfs4state.c
>>>>>>> @@ -5486,7 +5486,7 @@ nfsd4_process_open1(struct 
>>>>>>> nfsd4_compound_state *cstate,
>>>>>>>     static inline __be32
>>>>>>>     nfs4_check_delegmode(struct nfs4_delegation *dp, int flags)
>>>>>>>     {
>>>>>>> -    if ((flags & WR_STATE) && deleg_is_read(dp->dl_type))
>>>>>>> +    if (!(flags & RD_STATE) && deleg_is_read(dp->dl_type))
>>>>>>>             return nfserr_openmode;
>>>>>>>         else
>>>>>>>             return nfs_ok;
>>>>>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>>>>>> index 
>>>>>>> 0561c99b5def2eccf679bf3ea0e5b1a57d5d8374..ce93a31ac5cec75b0f944d288e796e7a73641572 100644
>>>>>>> --- a/fs/nfsd/nfs4xdr.c
>>>>>>> +++ b/fs/nfsd/nfs4xdr.c
>>>>>>> @@ -521,6 +521,26 @@ nfsd4_decode_fattr4(struct 
>>>>>>> nfsd4_compoundargs *argp, u32 *bmval, u32 bmlen,
>>>>>>>             *umask = mask & S_IRWXUGO;
>>>>>>>             iattr->ia_valid |= ATTR_MODE;
>>>>>>>         }
>>>>>>> +    if (bmval[2] & FATTR4_WORD2_TIME_DELEG_ACCESS) {
>>>>>>> +        fattr4_time_deleg_access access;
>>>>>>> +
>>>>>>> +        if (!xdrgen_decode_fattr4_time_deleg_access(argp->xdr, 
>>>>>>> &access))
>>>>>>> +            return nfserr_bad_xdr;
>>>>>>> +        iattr->ia_atime.tv_sec = access.seconds;
>>>>>>> +        iattr->ia_atime.tv_nsec = access.nseconds;
>>>>>>> +        iattr->ia_valid |= ATTR_ATIME | ATTR_ATIME_SET | 
>>>>>>> ATTR_DELEG;
>>>>>>> +    }
>>>>>>> +    if (bmval[2] & FATTR4_WORD2_TIME_DELEG_MODIFY) {
>>>>>>> +        fattr4_time_deleg_modify modify;
>>>>>>> +
>>>>>>> +        if (!xdrgen_decode_fattr4_time_deleg_modify(argp->xdr, 
>>>>>>> &modify))
>>>>>>> +            return nfserr_bad_xdr;
>>>>>>> +        iattr->ia_mtime.tv_sec = modify.seconds;
>>>>>>> +        iattr->ia_mtime.tv_nsec = modify.nseconds;
>>>>>>> +        iattr->ia_ctime.tv_sec = modify.seconds;
>>>>>>> +        iattr->ia_ctime.tv_nsec = modify.seconds;
>>>>>>> +        iattr->ia_valid |= ATTR_CTIME | ATTR_MTIME | 
>>>>>>> ATTR_MTIME_SET | ATTR_DELEG;
>>>>>>> +    }
>>>>>>>         /* request sanity: did attrlist4 contain the expected 
>>>>>>> number of words? */
>>>>>>>         if (attrlist4_count != xdr_stream_pos(argp->xdr) - 
>>>>>>> starting_pos)
>>>>>>> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
>>>>>>> index 
>>>>>>> 004415651295891b3440f52a4c986e3a668a48cb..f007699aa397fe39042d80ccd568db4654d19dd5 100644
>>>>>>> --- a/fs/nfsd/nfsd.h
>>>>>>> +++ b/fs/nfsd/nfsd.h
>>>>>>> @@ -531,7 +531,10 @@ static inline bool nfsd_attrs_supported(u32 
>>>>>>> minorversion, const u32 *bmval)
>>>>>>>     #endif
>>>>>>>     #define NFSD_WRITEABLE_ATTRS_WORD2 \
>>>>>>>         (FATTR4_WORD2_MODE_UMASK \
>>>>>>> -    | MAYBE_FATTR4_WORD2_SECURITY_LABEL)
>>>>>>> +    | MAYBE_FATTR4_WORD2_SECURITY_LABEL \
>>>>>>> +    | FATTR4_WORD2_TIME_DELEG_ACCESS \
>>>>>>> +    | FATTR4_WORD2_TIME_DELEG_MODIFY \
>>>>>>> +    )
>>>>>>>     #define NFSD_SUPPATTR_EXCLCREAT_WORD0 \
>>>>>>>         NFSD_WRITEABLE_ATTRS_WORD0
>>>>>>>
>>>>>>
>>>>>> Hi Jeff-
>>>>>>
>>>>>> After this patch is applied, I see failures of the git regression 
>>>>>> suite
>>>>>> on NFSv4.2 mounts.
>>>>>>
>>>>>> Test Summary Report
>>>>>> -------------------
>>>>>> ./t3412-rebase-root.sh                             (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 25 Failed: 5)
>>>>>>      Failed tests:  6, 19, 21-22, 24
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3400-rebase.sh                                  (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 38 Failed: 1)
>>>>>>      Failed test:  31
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3406-rebase-message.sh                          (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 32 Failed: 2)
>>>>>>      Failed tests:  15, 20
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3428-rebase-signoff.sh                          (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 7 Failed: 2)
>>>>>>      Failed tests:  6-7
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3418-rebase-continue.sh                         (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 29 Failed: 1)
>>>>>>      Failed test:  7
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3415-rebase-autosquash.sh                       (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 27 Failed: 2)
>>>>>>      Failed tests:  3-4
>>>>>>      Non-zero exit status: 1
>>>>>> ./t3404-rebase-interactive.sh                      (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 131 Failed: 15)
>>>>>>      Failed tests:  32, 34-43, 45, 121-123
>>>>>>      Non-zero exit status: 1
>>>>>> ./t1013-read-tree-submodule.sh                     (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 68 Failed: 1)
>>>>>>      Failed test:  34
>>>>>>      Non-zero exit status: 1
>>>>>> ./t2013-checkout-submodule.sh                      (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 74 Failed: 4)
>>>>>>      Failed tests:  26-27, 30-31
>>>>>>      Non-zero exit status: 1
>>>>>> ./t5500-fetch-pack.sh                              (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 375 Failed: 1)
>>>>>>      Failed test:  28
>>>>>>      Non-zero exit status: 1
>>>>>> ./t5572-pull-submodule.sh                          (Wstat: 256 
>>>>>> (exited
>>>>>> 1) Tests: 67 Failed: 2)
>>>>>>      Failed tests:  5, 7
>>>>>>      Non-zero exit status: 1
>>>>>> Files=1007, Tests=30810, 1417 wallclock secs (11.18 usr 10.17 sys +
>>>>>> 1037.05 cusr 6529.12 csys = 7587.52 CPU)
>>>>>> Result: FAIL
>>>>>>
>>>>>> The NFS client and NFS server under test are running the same 
>>>>>> v6.13-rc2
>>>>>> kernel from my git.kernel.org nfsd-testing branch.
>>>>>>
>>>>>>
>>>>>
>>>>> I'm not seeing these failures. I ran the gitr suite under kdevops with
>>>>> your nfsd-testing branch (6.13.0-rc2-ge9a809c5714e):
>>>>>
>>>>> All tests successful.
>>>>> Files=1007, Tests=30695, 10767 wallclock secs (13.87 usr 16.86 sys 
>>>>> + 1160.76 cusr 17870.80 csys = 19062.29 CPU)
>>>>> Result: PASS
>>>>>
>>>>> ...and looking at the results of those specific tests, they did run 
>>>>> and
>>>>> they did pass.
>>>>>
>>>>> I'm rerunning the tests now. It's possible the underlying fs matters.
>>>>> Mine is exporting xfs. Yours?
>>>>
>>>> Mine is btrfs, and the NFS version is v4.2 on TCP.
>>>>
>>>>
>>>
>>> Nope, I still can't reproduce this with btrfs either. I'm also using
>>> v4.2 on TCP. I assume you're running this under kdevops, so we should
>>> have a relatively similar environment.
>>
>> I'm running the "stress" setting, which starts twice as many threads
>> as there are CPUs (so, 16, I think?). 32 nfsd threads.
> 
> Also, I'm testing 2.47.0 of git. The default version that kdevops uses
> might be older.
> 
> 
>>> Are you also testing the same commit?
>>
>> The first failing test run is on 6.13.0-rc2-00016-gb45eda1daa7d
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/commit/? 
>> h=nfsd-testing&id=b45eda1daa7d79a2bf0426d27d4b359b8bb71d33
>>
>> I'll take a closer look.

My bad, I was testing with 2.46.0. When I changed to 2.47.0, the
failures in 9/10 went away.

Now with 2.47.0, I see one failure when testing in "stress" mode
on 10/10:

Test Summary Report
-------------------
./t4255-am-submodule.sh                            (Wstat: 256 (exited 
1) Tests: 33 Failed: 1)
   Failed test:  19
   Non-zero exit status: 1
Files=1007, Tests=30810, 1330 wallclock secs (11.27 usr  9.81 sys + 
1098.89 cusr 6274.49 csys = 7394.46 CPU)
Result: FAIL

It doesn't seem to reproduce in "fast" mode.


-- 
Chuck Lever