> On Nov 21, 2024, at 4:47 PM, NeilBrown <neilb@xxxxxxx> wrote:
>
> On Wed, 20 Nov 2024, Chuck Lever wrote:
>> On Wed, Nov 20, 2024 at 09:35:00AM +1100, NeilBrown wrote:
>>> 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.
>>
>> I find using GFP_ATOMIC here to be confusing -- it requests
>> allocation from special memory reserves and is to be used in
>> situations where allocation might result in system failure. That is
>> clearly not the case here, and the resulting memory allocation might
>> be long-lived.
>
> Would you be comfortable with GFP_NOWAIT which leaves out __GFP_HIGH ??
I will be comfortable when I hear back from Matthew and Liam.
:-)
>> I see the comment that says memory won't actually be allocated. I'm
>> not sure that's the way xa_store() works, however.
>
> xarray.rst says:
>
> The xa_store(), xa_cmpxchg(), xa_alloc(),
> xa_reserve() and xa_insert() functions take a gfp_t
> parameter in case the XArray needs to allocate memory to store this entry.
> If the entry is being deleted, no memory allocation needs to be performed,
> and the GFP flags specified will be ignored.`
>
> The particular context is that a normal pointer is currently stored a
> the given index, and we are replacing that with a number. The above
> doesn't explicitly say that won't require a memory allocation, but I
> think it is reasonable to say it won't need "to allocate memory to store
> this entry" - as an entry is already stored - so allocation should not
> be needed.
xa_mk_value() converts a number to a pointer, and xa_store
stores that pointer.
I suspect that xa_store() is allowed to rebalance the
xarray's internal data structures, and that could result
in memory release or allocation. That's why a GFP flag is
one of the arguments.
--
Chuck Lever
