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

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

On 10/13/25 8:04 PM, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  When replayed, the
> SEQUENCE op itself is replay.


s/replay./replayed./


> So removing the code and comments which seem to imply that we do, and


s/removing/remove/


> detect the case of a SOLO sequence to avoid looking a the cache.


s/looking a the cache./looking at the slot replay cache./


> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/nfsd/nfs4state.c |  7 ++-----
>  fs/nfsd/xdr4.h      | 20 ++++++--------------
>  2 files changed, 8 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 1c01836e8507..b51a37ad70aa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3530,11 +3530,8 @@ nfsd4_enc_sequence_replay(struct nfsd4_compoundargs *args,


The function's comments (including its documenting comment) seem
repetitive. Let's remove the function's documenting comment, and
then disperse those remarks inside the function like so:

-       /* Encode the replayed sequence operation */
+	/* Encode the replayed SEQUENCE response from current slot values. */


>  		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:
> +		 * We will simply replay the SEQUENCE.
>  		 */


The now empty set of braces here with just a comment is perhaps
confusing.


> -		op->status = nfserr_seq_false_retry;
>  	} else {


From above, add:

+		/*
+		 * Encode the uncached_rep error on the next
+		 * operation, and set resp->p and increment
+		 * resp->opcnt for nfs4svc_encode_compoundres.
+		 */


>  		op = &args->ops[resp->opcnt++];
>  		op->status = nfserr_retry_uncached_rep;
> @@ -3559,7 +3556,7 @@ nfsd4_replay_cache_entry(struct nfsd4_compoundres *resp,
>  	dprintk("--> %s slot %p\n", __func__, slot);
>  
>  	status = nfsd4_enc_sequence_replay(resp->rqstp->rq_argp, resp);
> -	if (status)
> +	if (status || !slot->sl_datalen)


This seems subtle. Maybe a comment could explain what this check
is trying to detect. I assume it is either the solo sequence case,
or the case where there is no replay cache?


>  		return status;
>  
>  	p = xdr_reserve_space(xdr, slot->sl_datalen);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index ee0570cbdd9e..390bfb0ba13f 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -923,25 +923,17 @@ 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).
> + * actually asked us to.
> + * This doesn't apply to SEQUENCE.  We must always record in the slot
> + * that we have received a SEQUENCE, and must always respond
> + * successfully to a replayed successful SEQUENCE - that is handled
> + * separately from the sl_data cache.

Once is_solo_sequence is gone, I'm not convinced this is the right spot
for your new explanation. (ie, the comment seems sensible, but maybe it
belongs somewhere else).

The only thing this helper is doing is looking at how the client
set sa_cachethis in the original request, and that's already
obvious from the code -- no comment is needed. And maybe even this
helper could be removed, and the now smaller "if" statement can be
relocated to its only call site.


>   */
>  static inline bool nfsd4_cache_this(struct nfsd4_compoundres *resp)
>  {
> -	return (resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS)
> -		|| nfsd4_is_solo_sequence(resp);
> +	return resp->cstate.slot->sl_flags & NFSD4_SLOT_CACHETHIS;
>  }
>  
>  static inline bool nfsd4_last_compound_op(struct svc_rqst *rqstp)


-- 
Chuck Lever