Re: [PATCH v3 2/2] nfsd: stop pretending that we cache the SEQUENCE reply.

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-10-16 at 12:31 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  As the comment above
> nfsd4_replay_cache_entry() says:
> 
>  * The sequence operation is not cached because we can use the slot and
>  * session values.
> 
> The comment above nfsd4_cache_this() suggests otherwise.
> 
>  * The session reply cache only needs to cache replies that the client
>  * actually asked us to.  But it's almost free for us to cache compounds
>  * consisting of only a SEQUENCE op, so we may as well cache those too.
>  * Also, the protocol doesn't give us a convenient response in the case
>  * of a replay of a solo SEQUENCE op that wasn't cached
> 
> The code in nfsd4_store_cache_entry() makes it clear that only responses
> beyond 'cstate.data_offset' are actually cached, and data_offset is set
> at the end of nfsd4_encode_sequence() *after* the sequence response has
> been encoded.
> 
> This patch simplifies code and removes the confusing comments.
> 
> - nfsd4_is_solo_sequence() is discarded as not-useful.
> - nfsd4_cache_this() is now trivial so it too is discarded with the
>   code placed in-line at the one call-site in nfsd4_store_cache_entry().
> - nfsd4_enc_sequence_replay() is open-coded in to
>   nfsd4_replay_cache_entry(), and then simplified to (hopefully) make
>   the process of replaying a reply clearer.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c | 57 ++++++++++++++-------------------------------
>  fs/nfsd/xdr4.h      | 21 -----------------
>  2 files changed, 17 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 60451fd98bdf..7bd20c9efd29 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3494,7 +3494,7 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  	free_svc_cred(&slot->sl_cred);
>  	copy_cred(&slot->sl_cred, &resp->rqstp->rq_cred);
>  
> -	if (!nfsd4_cache_this(resp)) {
> +	if (!(resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)) {
>  		slot->sl_flags &= ~NFSD4_SLOT_CACHED;
>  		return;
>  	}
> @@ -3508,41 +3508,6 @@ nfsd4_store_cache_entry(struct nfsd4_compoundres *resp)
>  	return;
>  }
>  
> -/*
> - * Encode the replay sequence operation from the slot values.
> - * If cachethis is FALSE encode the uncached rep error on the next
> - * operation which sets resp->p and increments resp->opcnt for
> - * nfs4svc_encode_compoundres.
> - *
> - */
> -static __be32
> -nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,
> -			  struct nfsd4_compoundres *resp)
> -{
> -	struct nfsd4_op *op;
> -	struct nfsd4_slot *slot = resp->cstate.slot;
> -
> -	/* Encode the replayed sequence operation */
> -	op = &args->ops[resp->opcnt - 1];
> -	nfsd4_encode_operation(resp, op);
> -
> -	if (slot->sl_flags & NFSD4_SLOT_CACHED)
> -		return op->status;
> -	if (args->opcnt == 1) {
> -		/*
> -		 * The original operation wasn't a solo sequence--we
> -		 * always cache those--so this retry must not match the
> -		 * original:
> -		 */
> -		op->status = nfserr_seq_false_retry;
> -	} else {
> -		op = &args->ops[resp->opcnt++];
> -		op->status = nfserr_retry_uncached_rep;
> -		nfsd4_encode_operation(resp, op);
> -	}
> -	return op->status;
> -}
> -
>  /*
>   * The sequence operation is not cached because we can use the slot and
>   * session values.
> @@ -3551,17 +3516,29 @@ static __be32
>  nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
>  			 struct nfsd4_sequence *seq)
>  {
> +	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
>  	struct nfsd4_slot *slot = resp->cstate.slot;
>  	struct xdr_stream *xdr = resp->xdr;
>  	__be32 *p;
> -	__be32 status;
>  
>  	dprintk("--> %s slot %p\n", __func__, slot);
>  
> -	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
> -	if (status)
> -		return status;
> +	/* Always encode the SEQUENCE response. */
> +	nfsd4_encode_operation(resp, &args->ops[0]);
> +	if (args->opcnt == 1)
> +		/* A solo SEQUENCE - nothing was cached */
> +		return args->ops[0].status;
> +
> +	if (!(slot->sl_flags & NFSD4_SLOT_CACHED)) {
> +		/* We weren't asked to cache this. */
> +		struct nfsd4_op *op;
> +		op = &args->ops[resp->opcnt++];
> +		op->status = nfserr_retry_uncached_rep;
> +		nfsd4_encode_operation(resp, op);
> +		return op->status;
> +	}
>  
> +	/* return reply from cache */
>  	p = xdr_reserve_space(xdr, slot->sl_datalen);
>  	if (!p) {
>  		WARN_ON_ONCE(1);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 1ce8e12ae335..ae75846b3cd7 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -924,27 +924,6 @@ struct nfsd4_compoundres {
>  	struct nfsd4_compound_state	cstate;
>  };
>  
> -static inline bool nfsd4_is_solo_sequence(struct nfsd4_compoundres *resp)
> -{
> -	struct nfsd4_compoundargs *args = resp->rqstp->rq_argp;
> -	return resp->opcnt == 1 && args->ops[0].opnum == OP_SEQUENCE;
> -}
> -
> -/*
> - * The session reply cache only needs to cache replies that the client
> - * actually asked us to.  But it's almost free for us to cache compounds
> - * consisting of only a SEQUENCE op, so we may as well cache those too.
> - * Also, the protocol doesn't give us a convenient response in the case
> - * of a replay of a solo SEQUENCE op that wasn't cached
> - * (RETRY_UNCACHED_REP can only be returned in the second op of a
> - * compound).
> - */
> -static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
> -{
> -	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> -		|| nfsd4_is_solo_sequence(resp);
> -}
> -
>  static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
>  {
>  	struct nfsd4_compoundres *resp = rqstp->rq_resp;

Seems sane, and I like the simplification.

Reviewed-by: Jeff Layton <jlayton@kernel.org>