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

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2025-10-14 at 11:04 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> nfsd does not cache the reply to a SEQUENCE.  When replayed, the
> SEQUENCE op itself is replay.
> 
> So removing the code and comments which seem to imply that we do, and
> detect the case of a SOLO sequence to avoid looking a the 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,
>  		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.
>  		 */
> -		op->status = nfserr_seq_false_retry;
>  	} else {
>  		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)
>  		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.
>   */
>  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)

Again, this makes more sense to me.

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