> On Aug 28, 2024, at 6:49 PM, Olga Kornievskaia <aglo@xxxxxxxxx> wrote:
>
> On Wed, Aug 28, 2024 at 1:40 PM <cel@xxxxxxxxxx> wrote:
>>
>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>>
>> NFSD removes COPY callback stateids after once lease period. This
>> practice keeps the list of callback stateids limited to prevent a
>> DoS possibility, but doesn't comply with the spirit of RFC 7862
>> Section 4.8, which says:
>>
>>> A copy offload stateid will be valid until either (A) the client or
>>> server restarts or (B) the client returns the resource by issuing an
>>> OFFLOAD_CANCEL operation or the client replies to a CB_OFFLOAD
>>> operation.
>>
>> Note there are no BCP 14 compliance keywords in this text, so NFSD
>> is free to ignore this stateid lifetime guideline without becoming
>> non-compliant.
>>
>> Nevertheless, this behavior variance should be explicitly documented
>> in the code.
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs4state.c | 36 +++++++++++++++++++++++++-----------
>> 1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index aaebc60cc77c..437b94beb115 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -6324,6 +6324,29 @@ static void nfsd4_ssc_expire_umount(struct nfsd_net *nn)
>> }
>> #endif
>>
>> +/*
>> + * RFC 7862 Section 4.8 says that, if the client hasn't replied to a
>> + * CB_OFFLOAD, that COPY callback stateid will live until the client or
>> + * server restarts. To prevent a DoS resulting from a pile of these
>> + * stateids accruing over time, NFSD purges them after one lease period.
>> + */
>
> I don't believe this is correct documentation for this piece of code.
> There are two kinds of stateids in the copy offload world: one is
> issued by the source server "cnr_stateid" in the response of the
> COPY_NOTIFY and given to be client (to be given to the destination
> server to use for the read) and those are the ones kept in the
> knfsd's s2s_cp_stateids list and then cleaned up by the laundry thread
> when copy isn't cleaned up on the source server.
Got it.
That detail was not included in what you mentioned to me
before ;-) Either I can drop this patch, or please suggest
a corrected text and I will replace the comment.
> The text from the RFC
> quoted here is for copy's callback stateids.
I'll look for something more relevant, if only for my own
edification.
> In the current
> implementation, we don't keep callback stateids around past when the
> async copy is done. I agree that needs to be changed for
> OFFLOAD_STATUS op and then we can add the wording of how long we are
> keeping those.
Yep, as we discussed yesterday.
>> +static void nfs4_launder_cpntf_statelist(struct nfsd_net *nn,
>> + struct laundry_time *lt)
>> +{
>> + struct nfs4_cpntf_state *cps;
>> + copy_stateid_t *cps_t;
>> + int i;
>> +
>> + spin_lock(&nn->s2s_cp_lock);
>> + idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
>> + cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
>> + if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID &&
>> + state_expired(lt, cps->cpntf_time))
>> + _free_cpntf_state_locked(nn, cps);
>> + }
>> + spin_unlock(&nn->s2s_cp_lock);
>> +}
>> +
>> /* Check if any lock belonging to this lockowner has any blockers */
>> static bool
>> nfs4_lockowner_has_blockers(struct nfs4_lockowner *lo)
>> @@ -6495,9 +6518,6 @@ nfs4_laundromat(struct nfsd_net *nn)
>> .cutoff = ktime_get_boottime_seconds() - nn->nfsd4_lease,
>> .new_timeo = nn->nfsd4_lease
>> };
>> - struct nfs4_cpntf_state *cps;
>> - copy_stateid_t *cps_t;
>> - int i;
>>
>> if (clients_still_reclaiming(nn)) {
>> lt.new_timeo = 0;
>> @@ -6505,14 +6525,8 @@ nfs4_laundromat(struct nfsd_net *nn)
>> }
>> nfsd4_end_grace(nn);
>>
>> - spin_lock(&nn->s2s_cp_lock);
>> - idr_for_each_entry(&nn->s2s_cp_stateids, cps_t, i) {
>> - cps = container_of(cps_t, struct nfs4_cpntf_state, cp_stateid);
>> - if (cps->cp_stateid.cs_type == NFS4_COPYNOTIFY_STID &&
>> - state_expired(<, cps->cpntf_time))
>> - _free_cpntf_state_locked(nn, cps);
>> - }
>> - spin_unlock(&nn->s2s_cp_lock);
>> + nfs4_launder_cpntf_statelist(nn, <);
>> +
>> nfs4_get_client_reaplist(nn, &reaplist, <);
>> nfs4_process_client_reaplist(&reaplist);
>>
>> --
>> 2.46.0
--
Chuck Lever
