Re: [PATCH 3/4] nfsd: free unused session-DRC slots

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-11-13 at 16:38 +1100, NeilBrown wrote:
> When the client confirms that some currently allocated DRC slots are not
> used, free some.  We currently keep 6 (NFSD_MAX_UNUSED_SLOTS) unused
> slots to support bursts of client activity.  This could be tuned with a
> shrinker.
> 
> When we free a slot we store the seqid in the slot pointer so that it can
> be restored when we reactivate the slot.  The RFC requires the seqid for
> each slot to increment on each request and does not permit it ever to be
> reset.
> 

I think we need to get some clarification on the above. 

IMO, once you free a slot, it's gone and its seqid should also be
forgotten. A reactivated slot, IOW, is effectively "new" and its seqid
should start again at 1. I don't think the spec intended to require
that both ends to remember seqids for defunct slots.

> We decoding sa_highest_slotid into maxslots we need to add 1 - this
> matches how it is encoded for the reply.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 25 +++++++++++++++++++------
>  fs/nfsd/nfs4xdr.c   |  3 ++-
>  2 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 683bb908039b..15de62416243 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1910,14 +1910,21 @@ gen_sessionid(struct nfsd4_session *ses)
>  #define NFSD_MIN_HDR_SEQ_SZ  (24 + 12 + 44)
>  
>  static void
> -free_session_slots(struct nfsd4_session *ses)
> +free_session_slots(struct nfsd4_session *ses, int from)
>  {
>  	int i;
>  
> -	for (i = 0; i < ses->se_fchannel.maxreqs; i++) {
> +	if (from >= ses->se_fchannel.maxreqs)
> +		return;
> +
> +	for (i = from; i < ses->se_fchannel.maxreqs; i++) {
> +		uintptr_t seqid = ses->se_slots[i]->sl_seqid;
>  		free_svc_cred(&ses->se_slots[i]->sl_cred);
>  		kfree(ses->se_slots[i]);
> +		/* Save the seqid in case we reactivate this slot */
> +		ses->se_slots[i] = (void *)seqid;
>  	}
> +	ses->se_fchannel.maxreqs = from;
>  }
>  
>  /*
> @@ -2069,7 +2076,7 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
>  
>  static void __free_session(struct nfsd4_session *ses)
>  {
> -	free_session_slots(ses);
> +	free_session_slots(ses, 0);
>  	kfree(ses);
>  }
>  
> @@ -4214,6 +4221,9 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (status)
>  		goto out_put_session;
>  
> +	/* If there are lots of unused slots, free some */
> +	free_session_slots(session, seq->maxslots + NFSD_MAX_UNUSED_SLOTS);
> +
>  	buflen = (seq->cachethis) ?
>  			session->se_fchannel.maxresp_cached :
>  			session->se_fchannel.maxresp_sz;
> @@ -4244,10 +4254,13 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
>  		int s = session->se_fchannel.maxreqs;
>  
> -		session->se_slots[s] = kzalloc(slot_bytes(&session->se_fchannel),
> -					       GFP_NOWAIT);
> -		if (session->se_slots[s])
> +		struct nfsd4_slot *slot = kzalloc(slot_bytes(&session->se_fchannel),
> +						  GFP_NOWAIT);
> +		if (slot) {
> +			slot->sl_seqid = (uintptr_t)session->se_slots[s];
> +			session->se_slots[s] = slot;
>  			session->se_fchannel.maxreqs += 1;
> +		}
>  	}
>  	seq->maxslots = session->se_fchannel.maxreqs;
>  
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index f118921250c3..846ed52fdaf5 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1884,7 +1884,8 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
>  		return nfserr_bad_xdr;
>  	seq->seqid = be32_to_cpup(p++);
>  	seq->slotid = be32_to_cpup(p++);
> -	seq->maxslots = be32_to_cpup(p++);
> +	/* sa_highest_slotid counts from 0 but maxslots  counts from 1 ... */
> +	seq->maxslots = be32_to_cpup(p++) + 1;
>  	seq->cachethis = be32_to_cpup(p);
>  
>  	seq->status_flags = 0;

-- 
Jeff Layton <jlayton@kernel.org>