Re: [PATCH 4/6] nfsd: allocate new session-based DRC slots on demand.

NeilBrown
2024-11-21T21:20:19Z

On Wed, 20 Nov 2024, Chuck Lever wrote:
> On Wed, Nov 20, 2024 at 09:27:51AM +1100, NeilBrown wrote:
> > On Wed, 20 Nov 2024, Chuck Lever wrote:
> > > On Tue, Nov 19, 2024 at 11:41:31AM +1100, NeilBrown wrote:
> > > > If a client ever uses the highest available slot for a given session,
> > > > attempt to allocate another slot so there is room for the client to use
> > > > more slots if wanted. GFP_NOWAIT is used so if there is not plenty of
> > > > free memory, failure is expected - which is what we want. It also
> > > > allows the allocation while holding a spinlock.
> > > >
> > > > We would expect to stablise with one more slot available than the client
> > > > actually uses.
> > >
> > > Which begs the question "why have a 2048 slot maximum session slot
> > > table size?" 1025 might work too. But is there a need for any
> > > maximum at all, or is this just a sanity check?
> >
> > Linux NFS presumably isn't the only client, and it might change in the
> > future. Maybe there is no need for a maximum. It was mostly as a
> > sanity check.
> >
> > It wouldn't take much to convince me to remove the limit.
>
> What's the worse that might happen if there is no cap? Can this be
> used as a DoS vector?

It depends on how much you trust the clients that you have decided to
trust. Probably we want the option of a "public" NFS server (read only
probably) so we cannot assume much trust in the implementation of the
client.

Certainly a client could only ever use the highest slot number available
- though the RFC prefers lowest - and that could push allocating through
the roof. We could defend against that in more subtle ways, but a hard
upper limit is easy.

>
> If a maximum should be necessary, its value should be clearly
> labeled as "not an architectural limit -- for sanity checking only".

That is certainly sensible.

>
>
> > > > Now that we grow the slot table on demand we can start with a smaller
> > > > allocation. Define NFSD_MAX_INITIAL_SLOTS and allocate at most that
> > > > many when session is created.
> > >
> > > Maybe NFSD_DEFAULT_INITIAL_SLOTS is more descriptive?
> >
> > I don't think "DEFAULT" is the right word. The client requests a number
> > of slots. That is the "Default". The server can impose a limit - a
> > maximum.
> > Maybe we don't need a limit here either?
>
> I see. Well I don't think there needs to be a "maximum" number of
> initial slots. NFSD can try to allocate the number the client
> requested as best it can, until it hits our sane maximum above.

Given that we have a shrinker to discard them if they ever become a
problem, that makes sense.

>
> I think sessions should have a minimum number of slots to guarantee
> forward progress (or IOW prevent a deadlock). I would say that
> number should be larger than 1 -- perhaps 2 or even 4.

I think one is enough to ensure forward progress. Otherwise the RFC
would have something to say about this.

>
> The problem with a small initial slot count is that means the
> session has a slow start heuristic. That might or might not be
> desirable here.

The question of how quickly to increase slot count can be relevant at
any time, not just at session creation time. If there is a bust of
activity after a quite time during which the shrinker discarded a lot of
slots - how quickly should we rebuild?
My current approach is effectively one new slot per requests round-trip.
So there might be 1 request in flight. Then 2. Then 3. etc.

We could aim for exponential rather than linear growth. Maybe when the
highest slot is used, add 20% of the current number of slots - rounded
up.
So 1,2,3,4,5,6,8,10,12,15,18,22,26,31,37,44,52,62,74,88,105,126,

??

Thanks,
NeilBrown