On Wed, 20 Nov 2024, Chuck Lever wrote:
> On Tue, Nov 19, 2024 at 11:41:32AM +1100, NeilBrown wrote:
> > Reducing the number of slots in the session slot table requires
> > confirmation from the client. This patch adds reduce_session_slots()
> > which starts the process of getting confirmation, but never calls it.
> > That will come in a later patch.
> >
> > Before we can free a slot we need to confirm that the client won't try
> > to use it again. This involves returning a lower cr_maxrequests in a
> > SEQUENCE reply and then seeing a ca_maxrequests on the same slot which
> > is not larger than we limit we are trying to impose. So for each slot
> > we need to remember that we have sent a reduced cr_maxrequests.
> >
> > To achieve this we introduce a concept of request "generations". Each
> > time we decide to reduce cr_maxrequests we increment the generation
> > number, and record this when we return the lower cr_maxrequests to the
> > client. When a slot with the current generation reports a low
> > ca_maxrequests, we commit to that level and free extra slots.
> >
> > We use an 8 bit generation number (64 seems wasteful) and if it cycles
> > we iterate all slots and reset the generation number to avoid false matches.
> >
> > 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 can be read as
> > suggesting that the slot number could restart from one after a slot is
> > retired and reactivated, but also suggests that retiring slots is not
> > required. So when we reactive a slot we accept with the next seqid in
> > sequence, or 1.
> >
> > When 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@xxxxxxx>
> > ---
> > fs/nfsd/nfs4state.c | 81 ++++++++++++++++++++++++++++++++++++++-------
> > fs/nfsd/nfs4xdr.c | 5 +--
> > fs/nfsd/state.h | 4 +++
> > fs/nfsd/xdr4.h | 2 --
> > 4 files changed, 76 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index fb522165b376..0625b0aec6b8 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1910,17 +1910,55 @@ 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++) {
> > struct nfsd4_slot *slot = xa_load(&ses->se_slots, i);
> >
> > - xa_erase(&ses->se_slots, i);
> > + /*
> > + * Save the seqid in case we reactivate this slot.
> > + * This will never require a memory allocation so GFP
> > + * flag is irrelevant
> > + */
> > + xa_store(&ses->se_slots, i, xa_mk_value(slot->sl_seqid),
> > + GFP_ATOMIC);
>
> Again... ATOMIC is probably not what we want here, even if it is
> only documentary.
Why not? It might be called under a spinlock so GFP_KERNEL might trigger
a warning.
>
> And, I thought we determined that an unretired slot had a sequence
> number that is reset. Why save the slot's seqid? If I'm missing
> something, the comment here should be bolstered to explain it.
It isn't clear to me that we determined that - only the some people
asserted it. Until the spec is clarified I think it is safest to be
cautious.
Thanks,
NeilBrown
