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. The text from the RFC
quoted here is for copy's callback stateids. 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.
> +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
>
>
