| Message ID | 20241119004928.3245873-5-neilb@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | nfsd: allocate/free session-based DRC slots on demand | expand |
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? > 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? > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++------ > fs/nfsd/state.h | 2 ++ > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 31ff9f92a895..fb522165b376 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1956,7 +1956,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs, > if (!slot || xa_is_err(xa_store(&new->se_slots, 0, slot, GFP_KERNEL))) > goto out_free; > > - for (i = 1; i < numslots; i++) { > + for (i = 1; i < numslots && i < NFSD_MAX_INITIAL_SLOTS; i++) { > slot = kzalloc(slotsize, GFP_KERNEL | __GFP_NORETRY); > if (!slot) > break; > @@ -4248,11 +4248,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > slot = xa_load(&session->se_slots, seq->slotid); > dprintk("%s: slotid %d\n", __func__, seq->slotid); > > - /* We do not negotiate the number of slots yet, so set the > - * maxslots to the session maxreqs which is used to encode > - * sr_highest_slotid and the sr_target_slot id to maxslots */ > - seq->maxslots = session->se_fchannel.maxreqs; > - > trace_nfsd_slot_seqid_sequence(clp, seq, slot); > status = check_slot_seqid(seq->seqid, slot->sl_seqid, > slot->sl_flags & NFSD4_SLOT_INUSE); > @@ -4302,6 +4297,31 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > cstate->session = session; > cstate->clp = clp; > > + /* > + * If the client ever uses the highest available slot, > + * gently try to allocate another one. > + */ > + if (seq->slotid == session->se_fchannel.maxreqs - 1 && > + session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) { > + int s = session->se_fchannel.maxreqs; > + > + /* > + * GFP_NOWAIT is a low-priority non-blocking allocation > + * which can be used under client_lock and only succeeds > + * if there is plenty of memory. > + * Use GFP_ATOMIC which is higher priority for xa_store() > + * so we are less likely to waste the effort of the first > + * allocation. IIUC, GFP_ATOMIC allocations come from a special pool. I don't think we want that here. I'd rather stick with NORETRY or KERNEL. > + */ > + slot = kzalloc(slot_bytes(&session->se_fchannel), GFP_NOWAIT); > + if (slot && !xa_is_err(xa_store(&session->se_slots, s, slot, > + GFP_ATOMIC))) > + session->se_fchannel.maxreqs += 1; > + else > + kfree(slot); > + } > + seq->maxslots = session->se_fchannel.maxreqs; > + > out: > switch (clp->cl_cb_state) { > case NFSD4_CB_DOWN: > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index e97626916a68..a14a823670e9 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -249,6 +249,8 @@ static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s) > * get good throughput on high-latency servers. > */ > #define NFSD_MAX_SLOTS_PER_SESSION 2048 > +/* Maximum number of slots per session to allocate for CREATE_SESSION */ > +#define NFSD_MAX_INITIAL_SLOTS 32 The first couple of patches did so nicely at ruthlessly discarding a lot of arbitrary logic. I'm not convinced by the patch description that the INITIAL_SLOTS complexity is needed... > /* Maximum session per slot cache size */ > #define NFSD_SLOT_CACHE_SIZE 2048 > /* Maximum number of NFSD_SLOT_CACHE_SIZE slots per session */ > -- > 2.47.0 >
On Tue, 2024-11-19 at 11:41 +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. > > 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. > > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++------ > fs/nfsd/state.h | 2 ++ > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 31ff9f92a895..fb522165b376 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1956,7 +1956,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs, > if (!slot || xa_is_err(xa_store(&new->se_slots, 0, slot, GFP_KERNEL))) > goto out_free; > > - for (i = 1; i < numslots; i++) { > + for (i = 1; i < numslots && i < NFSD_MAX_INITIAL_SLOTS; i++) { nit: maybe just clamp numslots at NFSD_MAX_INITIAL_SLOTS? > slot = kzalloc(slotsize, GFP_KERNEL | __GFP_NORETRY); > if (!slot) > break; > @@ -4248,11 +4248,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > slot = xa_load(&session->se_slots, seq->slotid); > dprintk("%s: slotid %d\n", __func__, seq->slotid); > > - /* We do not negotiate the number of slots yet, so set the > - * maxslots to the session maxreqs which is used to encode > - * sr_highest_slotid and the sr_target_slot id to maxslots */ > - seq->maxslots = session->se_fchannel.maxreqs; > - > trace_nfsd_slot_seqid_sequence(clp, seq, slot); > status = check_slot_seqid(seq->seqid, slot->sl_seqid, > slot->sl_flags & NFSD4_SLOT_INUSE); > @@ -4302,6 +4297,31 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > cstate->session = session; > cstate->clp = clp; > > + /* > + * If the client ever uses the highest available slot, > + * gently try to allocate another one. > + */ > + if (seq->slotid == session->se_fchannel.maxreqs - 1 && > + session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) { > + int s = session->se_fchannel.maxreqs; > + > + /* > + * GFP_NOWAIT is a low-priority non-blocking allocation > + * which can be used under client_lock and only succeeds > + * if there is plenty of memory. > + * Use GFP_ATOMIC which is higher priority for xa_store() > + * so we are less likely to waste the effort of the first > + * allocation. > + */ > + slot = kzalloc(slot_bytes(&session->se_fchannel), GFP_NOWAIT); > + if (slot && !xa_is_err(xa_store(&session->se_slots, s, slot, > + GFP_ATOMIC))) > + session->se_fchannel.maxreqs += 1; > + else > + kfree(slot); > + } > + seq->maxslots = session->se_fchannel.maxreqs; > + > out: > switch (clp->cl_cb_state) { > case NFSD4_CB_DOWN: > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index e97626916a68..a14a823670e9 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -249,6 +249,8 @@ static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s) > * get good throughput on high-latency servers. > */ > #define NFSD_MAX_SLOTS_PER_SESSION 2048 > +/* Maximum number of slots per session to allocate for CREATE_SESSION */ > +#define NFSD_MAX_INITIAL_SLOTS 32 > /* Maximum session per slot cache size */ > #define NFSD_SLOT_CACHE_SIZE 2048 > /* Maximum number of NFSD_SLOT_CACHE_SIZE slots per session */
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. > > > > 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? Thanks, NeilBrown > > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++------ > > fs/nfsd/state.h | 2 ++ > > 2 files changed, 28 insertions(+), 6 deletions(-) > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 31ff9f92a895..fb522165b376 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -1956,7 +1956,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs, > > if (!slot || xa_is_err(xa_store(&new->se_slots, 0, slot, GFP_KERNEL))) > > goto out_free; > > > > - for (i = 1; i < numslots; i++) { > > + for (i = 1; i < numslots && i < NFSD_MAX_INITIAL_SLOTS; i++) { > > slot = kzalloc(slotsize, GFP_KERNEL | __GFP_NORETRY); > > if (!slot) > > break; > > @@ -4248,11 +4248,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > slot = xa_load(&session->se_slots, seq->slotid); > > dprintk("%s: slotid %d\n", __func__, seq->slotid); > > > > - /* We do not negotiate the number of slots yet, so set the > > - * maxslots to the session maxreqs which is used to encode > > - * sr_highest_slotid and the sr_target_slot id to maxslots */ > > - seq->maxslots = session->se_fchannel.maxreqs; > > - > > trace_nfsd_slot_seqid_sequence(clp, seq, slot); > > status = check_slot_seqid(seq->seqid, slot->sl_seqid, > > slot->sl_flags & NFSD4_SLOT_INUSE); > > @@ -4302,6 +4297,31 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > cstate->session = session; > > cstate->clp = clp; > > > > + /* > > + * If the client ever uses the highest available slot, > > + * gently try to allocate another one. > > + */ > > + if (seq->slotid == session->se_fchannel.maxreqs - 1 && > > + session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) { > > + int s = session->se_fchannel.maxreqs; > > + > > + /* > > + * GFP_NOWAIT is a low-priority non-blocking allocation > > + * which can be used under client_lock and only succeeds > > + * if there is plenty of memory. > > + * Use GFP_ATOMIC which is higher priority for xa_store() > > + * so we are less likely to waste the effort of the first > > + * allocation. > > IIUC, GFP_ATOMIC allocations come from a special pool. I don't think > we want that here. I'd rather stick with NORETRY or KERNEL. > > > > + */ > > + slot = kzalloc(slot_bytes(&session->se_fchannel), GFP_NOWAIT); > > + if (slot && !xa_is_err(xa_store(&session->se_slots, s, slot, > > + GFP_ATOMIC))) > > + session->se_fchannel.maxreqs += 1; > > + else > > + kfree(slot); > > + } > > + seq->maxslots = session->se_fchannel.maxreqs; > > + > > out: > > switch (clp->cl_cb_state) { > > case NFSD4_CB_DOWN: > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index e97626916a68..a14a823670e9 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -249,6 +249,8 @@ static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s) > > * get good throughput on high-latency servers. > > */ > > #define NFSD_MAX_SLOTS_PER_SESSION 2048 > > +/* Maximum number of slots per session to allocate for CREATE_SESSION */ > > +#define NFSD_MAX_INITIAL_SLOTS 32 > > The first couple of patches did so nicely at ruthlessly discarding a > lot of arbitrary logic. I'm not convinced by the patch description > that the INITIAL_SLOTS complexity is needed... > > > > /* Maximum session per slot cache size */ > > #define NFSD_SLOT_CACHE_SIZE 2048 > > /* Maximum number of NFSD_SLOT_CACHE_SIZE slots per session */ > > -- > > 2.47.0 > > > > -- > Chuck Lever >
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? If a maximum should be necessary, its value should be clearly labeled as "not an architectural limit -- for sanity checking only". > > > 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. 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. 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.
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
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 31ff9f92a895..fb522165b376 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1956,7 +1956,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs, if (!slot || xa_is_err(xa_store(&new->se_slots, 0, slot, GFP_KERNEL))) goto out_free; - for (i = 1; i < numslots; i++) { + for (i = 1; i < numslots && i < NFSD_MAX_INITIAL_SLOTS; i++) { slot = kzalloc(slotsize, GFP_KERNEL | __GFP_NORETRY); if (!slot) break; @@ -4248,11 +4248,6 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, slot = xa_load(&session->se_slots, seq->slotid); dprintk("%s: slotid %d\n", __func__, seq->slotid); - /* We do not negotiate the number of slots yet, so set the - * maxslots to the session maxreqs which is used to encode - * sr_highest_slotid and the sr_target_slot id to maxslots */ - seq->maxslots = session->se_fchannel.maxreqs; - trace_nfsd_slot_seqid_sequence(clp, seq, slot); status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags & NFSD4_SLOT_INUSE); @@ -4302,6 +4297,31 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, cstate->session = session; cstate->clp = clp; + /* + * If the client ever uses the highest available slot, + * gently try to allocate another one. + */ + if (seq->slotid == session->se_fchannel.maxreqs - 1 && + session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) { + int s = session->se_fchannel.maxreqs; + + /* + * GFP_NOWAIT is a low-priority non-blocking allocation + * which can be used under client_lock and only succeeds + * if there is plenty of memory. + * Use GFP_ATOMIC which is higher priority for xa_store() + * so we are less likely to waste the effort of the first + * allocation. + */ + slot = kzalloc(slot_bytes(&session->se_fchannel), GFP_NOWAIT); + if (slot && !xa_is_err(xa_store(&session->se_slots, s, slot, + GFP_ATOMIC))) + session->se_fchannel.maxreqs += 1; + else + kfree(slot); + } + seq->maxslots = session->se_fchannel.maxreqs; + out: switch (clp->cl_cb_state) { case NFSD4_CB_DOWN: diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index e97626916a68..a14a823670e9 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -249,6 +249,8 @@ static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s) * get good throughput on high-latency servers. */ #define NFSD_MAX_SLOTS_PER_SESSION 2048 +/* Maximum number of slots per session to allocate for CREATE_SESSION */ +#define NFSD_MAX_INITIAL_SLOTS 32 /* Maximum session per slot cache size */ #define NFSD_SLOT_CACHE_SIZE 2048 /* Maximum number of NFSD_SLOT_CACHE_SIZE slots per session */
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. 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. Signed-off-by: NeilBrown <neilb@suse.de> --- fs/nfsd/nfs4state.c | 32 ++++++++++++++++++++++++++------ fs/nfsd/state.h | 2 ++ 2 files changed, 28 insertions(+), 6 deletions(-)