[PATCH 0/6 v3] nfsd: allocate/free session-based DRC slots on demand

[PATCH 0/6 v3] nfsd: allocate/free session-based DRC slots on demand

[NeilBrown]

Changes from v2:
 - number of slots is increased more quickly.  Every time the highest-number
   slot is used, we increase by at least 20%.
 - when xa_store() is used in a context where we no that no allocation can
   happen, pass '0' as the GFP flag.
 - report target slots as well as total slots in /proc/fs/nfsd/clients/*/info

I've stay with reporting session information in the client info file
rather than creating a directory or using netlink.  I think the client
info file is simple and adequate.

I still haven't added support for CB_RECALL_SLOT.  While it is helpful,
it isn't essential.  I'll probably try to add it after the current
series lands.

Thanks,
NeilBrown

[PATCH 1/6] nfsd: use an xarray to store v4.1 session slots

[NeilBrown]

Using an xarray to store session slots will make it easier to change the
number of active slots based on demand, and removes an unnecessary
limit.

To achieve good throughput with a high-latency server it can be helpful
to have hundreds of concurrent writes, which means hundreds of slots.
So increase the limit to 2048 (twice what the Linux client will
currently use).  This limit is only a sanity check, not a hard limit.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 28 ++++++++++++++++++----------
 fs/nfsd/state.h     |  9 ++++++---
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 741b9449f727..aa4f1293d4d3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1915,8 +1915,11 @@ free_session_slots(struct nfsd4_session *ses)
 	int i;
 
 	for (i = 0; i < ses->se_fchannel.maxreqs; i++) {
-		free_svc_cred(&ses->se_slots[i]->sl_cred);
-		kfree(ses->se_slots[i]);
+		struct nfsd4_slot *slot = xa_load(&ses->se_slots, i);
+
+		xa_erase(&ses->se_slots, i);
+		free_svc_cred(&slot->sl_cred);
+		kfree(slot);
 	}
 }
 
@@ -1996,17 +1999,20 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 	struct nfsd4_session *new;
 	int i;
 
-	BUILD_BUG_ON(struct_size(new, se_slots, NFSD_MAX_SLOTS_PER_SESSION)
-		     > PAGE_SIZE);
-
-	new = kzalloc(struct_size(new, se_slots, numslots), GFP_KERNEL);
+	new = kzalloc(sizeof(*new), GFP_KERNEL);
 	if (!new)
 		return NULL;
+	xa_init(&new->se_slots);
 	/* allocate each struct nfsd4_slot and data cache in one piece */
 	for (i = 0; i < numslots; i++) {
-		new->se_slots[i] = kzalloc(slotsize, GFP_KERNEL);
-		if (!new->se_slots[i])
+		struct nfsd4_slot *slot;
+		slot = kzalloc(slotsize, GFP_KERNEL);
+		if (!slot)
 			goto out_free;
+		if (xa_is_err(xa_store(&new->se_slots, i, slot, GFP_KERNEL))) {
+			kfree(slot);
+			goto out_free;
+		}
 	}
 
 	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
@@ -2017,7 +2023,8 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 	return new;
 out_free:
 	while (i--)
-		kfree(new->se_slots[i]);
+		kfree(xa_load(&new->se_slots, i));
+	xa_destroy(&new->se_slots);
 	kfree(new);
 	return NULL;
 }
@@ -2124,6 +2131,7 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
 static void __free_session(struct nfsd4_session *ses)
 {
 	free_session_slots(ses);
+	xa_destroy(&ses->se_slots);
 	kfree(ses);
 }
 
@@ -4278,7 +4286,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (seq->slotid >= session->se_fchannel.maxreqs)
 		goto out_put_session;
 
-	slot = session->se_slots[seq->slotid];
+	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
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index e16bb3717fb9..aad547d3ad8b 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -227,8 +227,11 @@ static inline struct nfs4_delegation *delegstateid(struct nfs4_stid *s)
 	return container_of(s, struct nfs4_delegation, dl_stid);
 }
 
-/* Maximum number of slots per session. 160 is useful for long haul TCP */
-#define NFSD_MAX_SLOTS_PER_SESSION     160
+/* Maximum number of slots per session.  This is for sanity-check only.
+ * It could be increased if we had a mechanism to shutdown misbehaving clients.
+ * A large number can be needed to get good throughput on high-latency servers.
+ */
+#define NFSD_MAX_SLOTS_PER_SESSION	2048
 /* Maximum  session per slot cache size */
 #define NFSD_SLOT_CACHE_SIZE		2048
 /* Maximum number of NFSD_SLOT_CACHE_SIZE slots per session */
@@ -327,7 +330,7 @@ struct nfsd4_session {
 	struct nfsd4_cb_sec	se_cb_sec;
 	struct list_head	se_conns;
 	u32			se_cb_seq_nr[NFSD_BC_SLOT_TABLE_SIZE];
-	struct nfsd4_slot	*se_slots[];	/* forward channel slots */
+	struct xarray		se_slots;	/* forward channel slots */
 };
 
 /* formatted contents of nfs4_sessionid */
-- 
2.47.0

[PATCH 2/6] nfsd: remove artificial limits on the session-based DRC

[NeilBrown]

Rather than guessing how much space it might be safe to use for the DRC,
simply try allocating slots and be prepared to accept failure.

The first slot for each session is allocated with GFP_KERNEL which is
unlikely to fail.  Subsequent slots are allocated with the addition of
__GFP_NORETRY which is expected to fail if there isn't much free memory.

This is probably too aggressive but clears the way for adding a
shrinker interface to free extra slots when memory is tight.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 94 ++++++++-------------------------------------
 fs/nfsd/nfsd.h      |  3 --
 fs/nfsd/nfssvc.c    | 32 ---------------
 3 files changed, 16 insertions(+), 113 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index aa4f1293d4d3..808cb0d897d5 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1938,65 +1938,13 @@ static inline u32 slot_bytes(struct nfsd4_channel_attrs *ca)
 	return size + sizeof(struct nfsd4_slot);
 }
 
-/*
- * XXX: If we run out of reserved DRC memory we could (up to a point)
- * re-negotiate active sessions and reduce their slot usage to make
- * room for new connections. For now we just fail the create session.
- */
-static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca, struct nfsd_net *nn)
-{
-	u32 slotsize = slot_bytes(ca);
-	u32 num = ca->maxreqs;
-	unsigned long avail, total_avail;
-	unsigned int scale_factor;
-
-	spin_lock(&nfsd_drc_lock);
-	if (nfsd_drc_max_mem > nfsd_drc_mem_used)
-		total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used;
-	else
-		/* We have handed out more space than we chose in
-		 * set_max_drc() to allow.  That isn't really a
-		 * problem as long as that doesn't make us think we
-		 * have lots more due to integer overflow.
-		 */
-		total_avail = 0;
-	avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail);
-	/*
-	 * Never use more than a fraction of the remaining memory,
-	 * unless it's the only way to give this client a slot.
-	 * The chosen fraction is either 1/8 or 1/number of threads,
-	 * whichever is smaller.  This ensures there are adequate
-	 * slots to support multiple clients per thread.
-	 * Give the client one slot even if that would require
-	 * over-allocation--it is better than failure.
-	 */
-	scale_factor = max_t(unsigned int, 8, nn->nfsd_serv->sv_nrthreads);
-
-	avail = clamp_t(unsigned long, avail, slotsize,
-			total_avail/scale_factor);
-	num = min_t(int, num, avail / slotsize);
-	num = max_t(int, num, 1);
-	nfsd_drc_mem_used += num * slotsize;
-	spin_unlock(&nfsd_drc_lock);
-
-	return num;
-}
-
-static void nfsd4_put_drc_mem(struct nfsd4_channel_attrs *ca)
-{
-	int slotsize = slot_bytes(ca);
-
-	spin_lock(&nfsd_drc_lock);
-	nfsd_drc_mem_used -= slotsize * ca->maxreqs;
-	spin_unlock(&nfsd_drc_lock);
-}
-
 static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 					   struct nfsd4_channel_attrs *battrs)
 {
 	int numslots = fattrs->maxreqs;
 	int slotsize = slot_bytes(fattrs);
 	struct nfsd4_session *new;
+	struct nfsd4_slot *slot;
 	int i;
 
 	new = kzalloc(sizeof(*new), GFP_KERNEL);
@@ -2004,17 +1952,21 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 		return NULL;
 	xa_init(&new->se_slots);
 	/* allocate each struct nfsd4_slot and data cache in one piece */
-	for (i = 0; i < numslots; i++) {
-		struct nfsd4_slot *slot;
-		slot = kzalloc(slotsize, GFP_KERNEL);
+	slot = kzalloc(slotsize, GFP_KERNEL);
+	if (!slot || xa_is_err(xa_store(&new->se_slots, 0, slot, GFP_KERNEL)))
+		goto out_free;
+
+	for (i = 1; i < numslots; i++) {
+		const gfp_t gfp = GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN;
+		slot = kzalloc(slotsize, gfp);
 		if (!slot)
-			goto out_free;
-		if (xa_is_err(xa_store(&new->se_slots, i, slot, GFP_KERNEL))) {
+			break;
+		if (xa_is_err(xa_store(&new->se_slots, i, slot, gfp))) {
 			kfree(slot);
-			goto out_free;
+			break;
 		}
 	}
-
+	fattrs->maxreqs = i;
 	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
 	new->se_cb_slot_avail = ~0U;
 	new->se_cb_highest_slot = min(battrs->maxreqs - 1,
@@ -2022,8 +1974,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 	spin_lock_init(&new->se_lock);
 	return new;
 out_free:
-	while (i--)
-		kfree(xa_load(&new->se_slots, i));
+	kfree(slot);
 	xa_destroy(&new->se_slots);
 	kfree(new);
 	return NULL;
@@ -2138,7 +2089,6 @@ static void __free_session(struct nfsd4_session *ses)
 static void free_session(struct nfsd4_session *ses)
 {
 	nfsd4_del_conns(ses);
-	nfsd4_put_drc_mem(&ses->se_fchannel);
 	__free_session(ses);
 }
 
@@ -3786,17 +3736,6 @@ static __be32 check_forechannel_attrs(struct nfsd4_channel_attrs *ca, struct nfs
 	ca->maxresp_cached = min_t(u32, ca->maxresp_cached,
 			NFSD_SLOT_CACHE_SIZE + NFSD_MIN_HDR_SEQ_SZ);
 	ca->maxreqs = min_t(u32, ca->maxreqs, NFSD_MAX_SLOTS_PER_SESSION);
-	/*
-	 * Note decreasing slot size below client's request may make it
-	 * difficult for client to function correctly, whereas
-	 * decreasing the number of slots will (just?) affect
-	 * performance.  When short on memory we therefore prefer to
-	 * decrease number of slots instead of their size.  Clients that
-	 * request larger slots than they need will get poor results:
-	 * Note that we always allow at least one slot, because our
-	 * accounting is soft and provides no guarantees either way.
-	 */
-	ca->maxreqs = nfsd4_get_drc_mem(ca, nn);
 
 	return nfs_ok;
 }
@@ -3874,11 +3813,11 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 		return status;
 	status = check_backchannel_attrs(&cr_ses->back_channel);
 	if (status)
-		goto out_release_drc_mem;
+		goto out_err;
 	status = nfserr_jukebox;
 	new = alloc_session(&cr_ses->fore_channel, &cr_ses->back_channel);
 	if (!new)
-		goto out_release_drc_mem;
+		goto out_err;
 	conn = alloc_conn_from_crses(rqstp, cr_ses);
 	if (!conn)
 		goto out_free_session;
@@ -3987,8 +3926,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
 	free_conn(conn);
 out_free_session:
 	__free_session(new);
-out_release_drc_mem:
-	nfsd4_put_drc_mem(&cr_ses->fore_channel);
+out_err:
 	return status;
 }
 
diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
index 4b56ba1e8e48..3eb21e63b921 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -88,9 +88,6 @@ struct nfsd_genl_rqstp {
 extern struct svc_program	nfsd_programs[];
 extern const struct svc_version	nfsd_version2, nfsd_version3, nfsd_version4;
 extern struct mutex		nfsd_mutex;
-extern spinlock_t		nfsd_drc_lock;
-extern unsigned long		nfsd_drc_max_mem;
-extern unsigned long		nfsd_drc_mem_used;
 extern atomic_t			nfsd_th_cnt;		/* number of available threads */
 
 extern const struct seq_operations nfs_exports_op;
diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
index 49e2f32102ab..3dbaefc96608 100644
--- a/fs/nfsd/nfssvc.c
+++ b/fs/nfsd/nfssvc.c
@@ -70,16 +70,6 @@ static __be32			nfsd_init_request(struct svc_rqst *,
  */
 DEFINE_MUTEX(nfsd_mutex);
 
-/*
- * nfsd_drc_lock protects nfsd_drc_max_pages and nfsd_drc_pages_used.
- * nfsd_drc_max_pages limits the total amount of memory available for
- * version 4.1 DRC caches.
- * nfsd_drc_pages_used tracks the current version 4.1 DRC memory usage.
- */
-DEFINE_SPINLOCK(nfsd_drc_lock);
-unsigned long	nfsd_drc_max_mem;
-unsigned long	nfsd_drc_mem_used;
-
 #if IS_ENABLED(CONFIG_NFS_LOCALIO)
 static const struct svc_version *localio_versions[] = {
 	[1] = &localio_version1,
@@ -575,27 +565,6 @@ void nfsd_reset_versions(struct nfsd_net *nn)
 		}
 }
 
-/*
- * Each session guarantees a negotiated per slot memory cache for replies
- * which in turn consumes memory beyond the v2/v3/v4.0 server. A dedicated
- * NFSv4.1 server might want to use more memory for a DRC than a machine
- * with mutiple services.
- *
- * Impose a hard limit on the number of pages for the DRC which varies
- * according to the machines free pages. This is of course only a default.
- *
- * For now this is a #defined shift which could be under admin control
- * in the future.
- */
-static void set_max_drc(void)
-{
-	#define NFSD_DRC_SIZE_SHIFT	7
-	nfsd_drc_max_mem = (nr_free_buffer_pages()
-					>> NFSD_DRC_SIZE_SHIFT) * PAGE_SIZE;
-	nfsd_drc_mem_used = 0;
-	dprintk("%s nfsd_drc_max_mem %lu \n", __func__, nfsd_drc_max_mem);
-}
-
 static int nfsd_get_default_max_blksize(void)
 {
 	struct sysinfo i;
@@ -678,7 +647,6 @@ int nfsd_create_serv(struct net *net)
 	nn->nfsd_serv = serv;
 	spin_unlock(&nfsd_notifier_lock);
 
-	set_max_drc();
 	/* check if the notifier is already set */
 	if (atomic_inc_return(&nfsd_notifier_refcount) == 1) {
 		register_inetaddr_notifier(&nfsd_inetaddr_notifier);
-- 
2.47.0

[PATCH 3/6] nfsd: add session slot count to /proc/fs/nfsd/clients/*/info

[NeilBrown]

Each client now reports the number of slots allocated in each session.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 808cb0d897d5..67dfc699e411 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -2643,6 +2643,7 @@ static const char *cb_state2str(int state)
 static int client_info_show(struct seq_file *m, void *v)
 {
 	struct inode *inode = file_inode(m->file);
+	struct nfsd4_session *ses;
 	struct nfs4_client *clp;
 	u64 clid;
 
@@ -2679,6 +2680,13 @@ static int client_info_show(struct seq_file *m, void *v)
 	seq_printf(m, "callback address: \"%pISpc\"\n", &clp->cl_cb_conn.cb_addr);
 	seq_printf(m, "admin-revoked states: %d\n",
 		   atomic_read(&clp->cl_admin_revoked));
+	spin_lock(&clp->cl_lock);
+	seq_printf(m, "session slots:");
+	list_for_each_entry(ses, &clp->cl_sessions, se_perclnt)
+		seq_printf(m, " %u", ses->se_fchannel.maxreqs);
+	spin_unlock(&clp->cl_lock);
+	seq_puts(m, "\n");
+
 	drop_client(clp);
 
 	return 0;
-- 
2.47.0

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

[NeilBrown]

If a client ever uses the highest available slot for a given session,
attempt to allocate more slots so there is room for the client to use
them 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.

Each time we increase the number of slots by 20% (rounded up).  This
allows fairly quick growth while avoiding excessive over-shoot.

We would expect to stablise with around 10% more slots available than
the client actually uses.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 35 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 67dfc699e411..ec4468ebbd40 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4235,11 +4235,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);
@@ -4289,6 +4284,41 @@ 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 20%.  This allows
+	 * fairly quick growth without grossly over-shooting what
+	 * the client might use.
+	 */
+	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
+	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
+		int s = session->se_fchannel.maxreqs;
+		int cnt = DIV_ROUND_UP(s, 5);
+
+		do {
+			/*
+			 * 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 | __GFP_NOWARN))) {
+				s += 1;
+				session->se_fchannel.maxreqs = s;
+			} else {
+				kfree(slot);
+			}
+		} while (slot && --cnt > 0);
+	}
+	seq->maxslots = session->se_fchannel.maxreqs;
+
 out:
 	switch (clp->cl_cb_state) {
 	case NFSD4_CB_DOWN:
-- 
2.47.0

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

[Jeff Layton]

On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:
> If a client ever uses the highest available slot for a given session,
> attempt to allocate more slots so there is room for the client to use
> them 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.
> 
> Each time we increase the number of slots by 20% (rounded up).  This
> allows fairly quick growth while avoiding excessive over-shoot.
> 
> We would expect to stablise with around 10% more slots available than
> the client actually uses.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 67dfc699e411..ec4468ebbd40 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4235,11 +4235,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);
> @@ -4289,6 +4284,41 @@ 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 20%.  This allows
> +	 * fairly quick growth without grossly over-shooting what
> +	 * the client might use.
> +	 */

20% seems like a reasonable place to start, but I do wonder if this
might need to be tunable under some workloads. Oh well, we can cross
that bridge if/when someone complains.
 
> +	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> +	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
> +		int s = session->se_fchannel.maxreqs;
> +		int cnt = DIV_ROUND_UP(s, 5);
> +
> +		do {
> +			/*
> +			 * 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.
> +			 */

I don't know here. Why not just use GFP_NOWAIT for the xa_store too? If
we're so memory constrained that that fails, we're probably better off
releasing the slot.

> +			slot = kzalloc(slot_bytes(&session->se_fchannel),
> +				       GFP_NOWAIT);
> +			if (slot &&
> +			    !xa_is_err(xa_store(&session->se_slots, s, slot,
> +						GFP_ATOMIC | __GFP_NOWARN))) {
> +				s += 1;
> +				session->se_fchannel.maxreqs = s;
> +			} else {
> +				kfree(slot);
> +			}
> +		} while (slot && --cnt > 0);
> +	}
> +	seq->maxslots = session->se_fchannel.maxreqs;
> +
>  out:
>  	switch (clp->cl_cb_state) {
>  	case NFSD4_CB_DOWN:

-- 
Jeff Layton <jlayton@kernel.org>

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

[NeilBrown]

On Fri, 06 Dec 2024, Jeff Layton wrote:
> On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:
> > If a client ever uses the highest available slot for a given session,
> > attempt to allocate more slots so there is room for the client to use
> > them 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.
> > 
> > Each time we increase the number of slots by 20% (rounded up).  This
> > allows fairly quick growth while avoiding excessive over-shoot.
> > 
> > We would expect to stablise with around 10% more slots available than
> > the client actually uses.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 67dfc699e411..ec4468ebbd40 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4235,11 +4235,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);
> > @@ -4289,6 +4284,41 @@ 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 20%.  This allows
> > +	 * fairly quick growth without grossly over-shooting what
> > +	 * the client might use.
> > +	 */
> 
> 20% seems like a reasonable place to start, but I do wonder if this
> might need to be tunable under some workloads. Oh well, we can cross
> that bridge if/when someone complains.

I think that if we need a tunable, then it is a failure of design.
If?when someone complains we may well need to redesign.  I hope we could
avoid a tunable in that design!

>  
> > +	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> > +	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
> > +		int s = session->se_fchannel.maxreqs;
> > +		int cnt = DIV_ROUND_UP(s, 5);
> > +
> > +		do {
> > +			/*
> > +			 * 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.
> > +			 */
> 
> I don't know here. Why not just use GFP_NOWAIT for the xa_store too? If
> we're so memory constrained that that fails, we're probably better off
> releasing the slot.

Maybe.  I'm open simple using GFP_NOWAIT both places.
Most often xa_store won't need to allocate anything - it adds slots to
the array in batches (at least I assume it does - anything else would be
inefficient).  So it mostly won't matter.
So if seems at all inelegant - let's drop it.

Thanks,
NeilBrown


> 
> > +			slot = kzalloc(slot_bytes(&session->se_fchannel),
> > +				       GFP_NOWAIT);
> > +			if (slot &&
> > +			    !xa_is_err(xa_store(&session->se_slots, s, slot,
> > +						GFP_ATOMIC | __GFP_NOWARN))) {
> > +				s += 1;
> > +				session->se_fchannel.maxreqs = s;
> > +			} else {
> > +				kfree(slot);
> > +			}
> > +		} while (slot && --cnt > 0);
> > +	}
> > +	seq->maxslots = session->se_fchannel.maxreqs;
> > +
> >  out:
> >  	switch (clp->cl_cb_state) {
> >  	case NFSD4_CB_DOWN:
> 
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
> 

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

[Jeff Layton]

On Fri, 2024-12-06 at 12:43 +1100, NeilBrown wrote:
> On Fri, 06 Dec 2024, Jeff Layton wrote:
> > On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:
> > > If a client ever uses the highest available slot for a given session,
> > > attempt to allocate more slots so there is room for the client to use
> > > them 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.
> > > 
> > > Each time we increase the number of slots by 20% (rounded up).  This
> > > allows fairly quick growth while avoiding excessive over-shoot.
> > > 
> > > We would expect to stablise with around 10% more slots available than
> > > the client actually uses.
> > > 
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > ---
> > >  fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
> > >  1 file changed, 35 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index 67dfc699e411..ec4468ebbd40 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -4235,11 +4235,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);
> > > @@ -4289,6 +4284,41 @@ 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 20%.  This allows
> > > +	 * fairly quick growth without grossly over-shooting what
> > > +	 * the client might use.
> > > +	 */
> > 
> > 20% seems like a reasonable place to start, but I do wonder if this
> > might need to be tunable under some workloads. Oh well, we can cross
> > that bridge if/when someone complains.
> 
> I think that if we need a tunable, then it is a failure of design.
> If?when someone complains we may well need to redesign.  I hope we could
> avoid a tunable in that design!
> 

I hope so too.

> >  
> > > +	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> > > +	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
> > > +		int s = session->se_fchannel.maxreqs;
> > > +		int cnt = DIV_ROUND_UP(s, 5);
> > > +
> > > +		do {
> > > +			/*
> > > +			 * 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.
> > > +			 */
> > 
> > I don't know here. Why not just use GFP_NOWAIT for the xa_store too? If
> > we're so memory constrained that that fails, we're probably better off
> > releasing the slot.
> 
> Maybe.  I'm open simple using GFP_NOWAIT both places.
> Most often xa_store won't need to allocate anything - it adds slots to
> the array in batches (at least I assume it does - anything else would be
> inefficient).  So it mostly won't matter.
> So if seems at all inelegant - let's drop it.
> 
> 

I'd prefer we drop that part. It probably won't matter much in the long
run anyway.

> 
> > 
> > > +			slot = kzalloc(slot_bytes(&session->se_fchannel),
> > > +				       GFP_NOWAIT);
> > > +			if (slot &&
> > > +			    !xa_is_err(xa_store(&session->se_slots, s, slot,
> > > +						GFP_ATOMIC | __GFP_NOWARN))) {
> > > +				s += 1;
> > > +				session->se_fchannel.maxreqs = s;
> > > +			} else {
> > > +				kfree(slot);
> > > +			}
> > > +		} while (slot && --cnt > 0);
> > > +	}
> > > +	seq->maxslots = session->se_fchannel.maxreqs;
> > > +
> > >  out:
> > >  	switch (clp->cl_cb_state) {
> > >  	case NFSD4_CB_DOWN:
> > 
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Chuck Lever]

On 12/5/24 7:43 PM, NeilBrown wrote:
> If a client ever uses the highest available slot for a given session,
> attempt to allocate more slots so there is room for the client to use
> them 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.
> 
> Each time we increase the number of slots by 20% (rounded up).  This
> allows fairly quick growth while avoiding excessive over-shoot.
> 
> We would expect to stablise with around 10% more slots available than
> the client actually uses.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
>   1 file changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 67dfc699e411..ec4468ebbd40 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4235,11 +4235,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);
> @@ -4289,6 +4284,41 @@ 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 20%.  This allows
> +	 * fairly quick growth without grossly over-shooting what
> +	 * the client might use.
> +	 */
> +	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> +	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
> +		int s = session->se_fchannel.maxreqs;
> +		int cnt = DIV_ROUND_UP(s, 5);
> +
> +		do {
> +			/*
> +			 * 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 | __GFP_NOWARN))) {
> +				s += 1;
> +				session->se_fchannel.maxreqs = s;
> +			} else {
> +				kfree(slot);

Don't you want to break out of this loop if slot allocation or the
xa_store() fails? Does the session logic work if there is a gap
of unallocated slots in the slot table? Seems like we want to wait
a bit anyway after an allocation failure before asking again.

Otherwise, LGTM. I assume a v4 is forthcoming to address review
comments.


> +			}
> +		} while (slot && --cnt > 0);
> +	}
> +	seq->maxslots = session->se_fchannel.maxreqs;
> +
>   out:
>   	switch (clp->cl_cb_state) {
>   	case NFSD4_CB_DOWN:


-- 
Chuck Lever

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

[NeilBrown]

On Sat, 07 Dec 2024, Chuck Lever wrote:
> On 12/5/24 7:43 PM, NeilBrown wrote:
> > If a client ever uses the highest available slot for a given session,
> > attempt to allocate more slots so there is room for the client to use
> > them 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.
> > 
> > Each time we increase the number of slots by 20% (rounded up).  This
> > allows fairly quick growth while avoiding excessive over-shoot.
> > 
> > We would expect to stablise with around 10% more slots available than
> > the client actually uses.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   fs/nfsd/nfs4state.c | 40 +++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 35 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 67dfc699e411..ec4468ebbd40 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -4235,11 +4235,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);
> > @@ -4289,6 +4284,41 @@ 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 20%.  This allows
> > +	 * fairly quick growth without grossly over-shooting what
> > +	 * the client might use.
> > +	 */
> > +	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> > +	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
> > +		int s = session->se_fchannel.maxreqs;
> > +		int cnt = DIV_ROUND_UP(s, 5);
> > +
> > +		do {
> > +			/*
> > +			 * 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 | __GFP_NOWARN))) {
> > +				s += 1;
> > +				session->se_fchannel.maxreqs = s;
> > +			} else {
> > +				kfree(slot);
> 
> Don't you want to break out of this loop if slot allocation or the
> xa_store() fails? Does the session logic work if there is a gap
> of unallocated slots in the slot table? Seems like we want to wait
> a bit anyway after an allocation failure before asking again.

Indeed!  The "slot = NULL" which the next patch adds should be in this
patch.  That makes the loop abort.

> 
> Otherwise, LGTM. I assume a v4 is forthcoming to address review
> comments.

I'll send that out Monday morning.

Thanks,
NeilBrown

> 
> 
> > +			}
> > +		} while (slot && --cnt > 0);
> > +	}
> > +	seq->maxslots = session->se_fchannel.maxreqs;
> > +
> >   out:
> >   	switch (clp->cl_cb_state) {
> >   	case NFSD4_CB_DOWN:
> 
> 
> -- 
> Chuck Lever
> 

[PATCH 5/6] nfsd: add support for freeing unused session-DRC slots

[NeilBrown]

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@suse.de>
---
 fs/nfsd/nfs4state.c | 80 +++++++++++++++++++++++++++++++++++++++------
 fs/nfsd/nfs4xdr.c   |  5 +--
 fs/nfsd/state.h     |  4 +++
 fs/nfsd/xdr4.h      |  2 --
 4 files changed, 77 insertions(+), 14 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index ec4468ebbd40..e73668462739 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1910,17 +1910,54 @@ 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), 0);
 		free_svc_cred(&slot->sl_cred);
 		kfree(slot);
 	}
+	ses->se_fchannel.maxreqs = from;
+	if (ses->se_target_maxslots > from)
+		ses->se_target_maxslots = from;
+}
+
+static int __maybe_unused
+reduce_session_slots(struct nfsd4_session *ses, int dec)
+{
+	struct nfsd_net *nn = net_generic(ses->se_client->net,
+					  nfsd_net_id);
+	int ret = 0;
+
+	if (ses->se_target_maxslots <= 1)
+		return ret;
+	if (!spin_trylock(&nn->client_lock))
+		return ret;
+	ret = min(dec, ses->se_target_maxslots-1);
+	ses->se_target_maxslots -= ret;
+	ses->se_slot_gen += 1;
+	if (ses->se_slot_gen == 0) {
+		int i;
+		ses->se_slot_gen = 1;
+		for (i = 0; i < ses->se_fchannel.maxreqs; i++) {
+			struct nfsd4_slot *slot = xa_load(&ses->se_slots, i);
+			slot->sl_generation = 0;
+		}
+	}
+	spin_unlock(&nn->client_lock);
+	return ret;
 }
 
 /*
@@ -1968,6 +2005,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 	}
 	fattrs->maxreqs = i;
 	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
+	new->se_target_maxslots = i;
 	new->se_cb_slot_avail = ~0U;
 	new->se_cb_highest_slot = min(battrs->maxreqs - 1,
 				      NFSD_BC_SLOT_TABLE_SIZE - 1);
@@ -2081,7 +2119,7 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
 
 static void __free_session(struct nfsd4_session *ses)
 {
-	free_session_slots(ses);
+	free_session_slots(ses, 0);
 	xa_destroy(&ses->se_slots);
 	kfree(ses);
 }
@@ -2684,6 +2722,9 @@ static int client_info_show(struct seq_file *m, void *v)
 	seq_printf(m, "session slots:");
 	list_for_each_entry(ses, &clp->cl_sessions, se_perclnt)
 		seq_printf(m, " %u", ses->se_fchannel.maxreqs);
+	seq_printf(m, "\nsession target slots:");
+	list_for_each_entry(ses, &clp->cl_sessions, se_perclnt)
+		seq_printf(m, " %u", ses->se_target_maxslots);
 	spin_unlock(&clp->cl_lock);
 	seq_puts(m, "\n");
 
@@ -3674,10 +3715,10 @@ nfsd4_exchange_id_release(union nfsd4_op_u *u)
 	kfree(exid->server_impl_name);
 }
 
-static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, bool slot_inuse)
+static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, u8 flags)
 {
 	/* The slot is in use, and no response has been sent. */
-	if (slot_inuse) {
+	if (flags & NFSD4_SLOT_INUSE) {
 		if (seqid == slot_seqid)
 			return nfserr_jukebox;
 		else
@@ -3686,6 +3727,8 @@ static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, bool slot_inuse)
 	/* Note unsigned 32-bit arithmetic handles wraparound: */
 	if (likely(seqid == slot_seqid + 1))
 		return nfs_ok;
+	if ((flags & NFSD4_SLOT_REUSED) && seqid == 1)
+		return nfs_ok;
 	if (seqid == slot_seqid)
 		return nfserr_replay_cache;
 	return nfserr_seq_misordered;
@@ -4236,8 +4279,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	dprintk("%s: slotid %d\n", __func__, seq->slotid);
 
 	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
-	status = check_slot_seqid(seq->seqid, slot->sl_seqid,
-					slot->sl_flags & NFSD4_SLOT_INUSE);
+	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
 	if (status == nfserr_replay_cache) {
 		status = nfserr_seq_misordered;
 		if (!(slot->sl_flags & NFSD4_SLOT_INITIALIZED))
@@ -4262,6 +4304,12 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	if (status)
 		goto out_put_session;
 
+	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
+	    slot->sl_generation == session->se_slot_gen &&
+	    seq->maxslots <= session->se_target_maxslots)
+		/* Client acknowledged our reduce maxreqs */
+		free_session_slots(session, session->se_target_maxslots);
+
 	buflen = (seq->cachethis) ?
 			session->se_fchannel.maxresp_cached :
 			session->se_fchannel.maxresp_sz;
@@ -4272,9 +4320,11 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	svc_reserve(rqstp, buflen);
 
 	status = nfs_ok;
-	/* Success! bump slot seqid */
+	/* Success! accept new slot seqid */
 	slot->sl_seqid = seq->seqid;
+	slot->sl_flags &= ~NFSD4_SLOT_REUSED;
 	slot->sl_flags |= NFSD4_SLOT_INUSE;
+	slot->sl_generation = session->se_slot_gen;
 	if (seq->cachethis)
 		slot->sl_flags |= NFSD4_SLOT_CACHETHIS;
 	else
@@ -4291,9 +4341,11 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * the client might use.
 	 */
 	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
+	    session->se_target_maxslots >= session->se_fchannel.maxreqs &&
 	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
 		int s = session->se_fchannel.maxreqs;
 		int cnt = DIV_ROUND_UP(s, 5);
+		void *prev_slot;
 
 		do {
 			/*
@@ -4307,17 +4359,25 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			 */
 			slot = kzalloc(slot_bytes(&session->se_fchannel),
 				       GFP_NOWAIT);
+			prev_slot = xa_load(&session->se_slots, s);
+			if (xa_is_value(prev_slot) && slot) {
+				slot->sl_seqid = xa_to_value(prev_slot);
+				slot->sl_flags |= NFSD4_SLOT_REUSED;
+			}
 			if (slot &&
 			    !xa_is_err(xa_store(&session->se_slots, s, slot,
 						GFP_ATOMIC | __GFP_NOWARN))) {
 				s += 1;
 				session->se_fchannel.maxreqs = s;
+				session->se_target_maxslots = s;
 			} else {
 				kfree(slot);
+				slot = NULL;
 			}
 		} while (slot && --cnt > 0);
 	}
-	seq->maxslots = session->se_fchannel.maxreqs;
+	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
+	seq->target_maxslots = session->se_target_maxslots;
 
 out:
 	switch (clp->cl_cb_state) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 53fac037611c..4dcb03cd9292 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1884,7 +1884,8 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
 		return nfserr_bad_xdr;
 	seq->seqid = be32_to_cpup(p++);
 	seq->slotid = be32_to_cpup(p++);
-	seq->maxslots = be32_to_cpup(p++);
+	/* sa_highest_slotid counts from 0 but maxslots  counts from 1 ... */
+	seq->maxslots = be32_to_cpup(p++) + 1;
 	seq->cachethis = be32_to_cpup(p);
 
 	seq->status_flags = 0;
@@ -4968,7 +4969,7 @@ nfsd4_encode_sequence(struct nfsd4_compoundres *resp, __be32 nfserr,
 	if (nfserr != nfs_ok)
 		return nfserr;
 	/* sr_target_highest_slotid */
-	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots - 1);
+	nfserr = nfsd4_encode_slotid4(xdr, seq->target_maxslots - 1);
 	if (nfserr != nfs_ok)
 		return nfserr;
 	/* sr_status_flags */
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index aad547d3ad8b..74f2ab3c95aa 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -249,7 +249,9 @@ struct nfsd4_slot {
 #define NFSD4_SLOT_CACHETHIS	(1 << 1)
 #define NFSD4_SLOT_INITIALIZED	(1 << 2)
 #define NFSD4_SLOT_CACHED	(1 << 3)
+#define NFSD4_SLOT_REUSED	(1 << 4)
 	u8	sl_flags;
+	u8	sl_generation;
 	char	sl_data[];
 };
 
@@ -331,6 +333,8 @@ struct nfsd4_session {
 	struct list_head	se_conns;
 	u32			se_cb_seq_nr[NFSD_BC_SLOT_TABLE_SIZE];
 	struct xarray		se_slots;	/* forward channel slots */
+	u8			se_slot_gen;
+	u32			se_target_maxslots;
 };
 
 /* formatted contents of nfs4_sessionid */
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 382cc1389396..c26ba86dbdfd 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -576,9 +576,7 @@ struct nfsd4_sequence {
 	u32			slotid;			/* request/response */
 	u32			maxslots;		/* request/response */
 	u32			cachethis;		/* request */
-#if 0
 	u32			target_maxslots;	/* response */
-#endif /* not yet */
 	u32			status_flags;		/* response */
 };
 
-- 
2.47.0

Re: [PATCH 5/6] nfsd: add support for freeing unused session-DRC slots

[Jeff Layton]

On Fri, 2024-12-06 at 11:43 +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@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 80 +++++++++++++++++++++++++++++++++++++++------
>  fs/nfsd/nfs4xdr.c   |  5 +--
>  fs/nfsd/state.h     |  4 +++
>  fs/nfsd/xdr4.h      |  2 --
>  4 files changed, 77 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index ec4468ebbd40..e73668462739 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1910,17 +1910,54 @@ 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), 0);
>  		free_svc_cred(&slot->sl_cred);
>  		kfree(slot);
>  	}
> +	ses->se_fchannel.maxreqs = from;
> +	if (ses->se_target_maxslots > from)
> +		ses->se_target_maxslots = from;
> +}
> +
> +static int __maybe_unused
> +reduce_session_slots(struct nfsd4_session *ses, int dec)
> +{
> +	struct nfsd_net *nn = net_generic(ses->se_client->net,
> +					  nfsd_net_id);
> +	int ret = 0;
> +
> +	if (ses->se_target_maxslots <= 1)
> +		return ret;
> +	if (!spin_trylock(&nn->client_lock))
> +		return ret;
> +	ret = min(dec, ses->se_target_maxslots-1);
> +	ses->se_target_maxslots -= ret;
> +	ses->se_slot_gen += 1;
> +	if (ses->se_slot_gen == 0) {
> +		int i;
> +		ses->se_slot_gen = 1;
> +		for (i = 0; i < ses->se_fchannel.maxreqs; i++) {
> +			struct nfsd4_slot *slot = xa_load(&ses->se_slots, i);
> +			slot->sl_generation = 0;
> +		}
> +	}
> +	spin_unlock(&nn->client_lock);
> +	return ret;
>  }
>  
>  /*
> @@ -1968,6 +2005,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
>  	}
>  	fattrs->maxreqs = i;
>  	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
> +	new->se_target_maxslots = i;
>  	new->se_cb_slot_avail = ~0U;
>  	new->se_cb_highest_slot = min(battrs->maxreqs - 1,
>  				      NFSD_BC_SLOT_TABLE_SIZE - 1);
> @@ -2081,7 +2119,7 @@ static void nfsd4_del_conns(struct nfsd4_session *s)
>  
>  static void __free_session(struct nfsd4_session *ses)
>  {
> -	free_session_slots(ses);
> +	free_session_slots(ses, 0);
>  	xa_destroy(&ses->se_slots);
>  	kfree(ses);
>  }
> @@ -2684,6 +2722,9 @@ static int client_info_show(struct seq_file *m, void *v)
>  	seq_printf(m, "session slots:");
>  	list_for_each_entry(ses, &clp->cl_sessions, se_perclnt)
>  		seq_printf(m, " %u", ses->se_fchannel.maxreqs);
> +	seq_printf(m, "\nsession target slots:");
> +	list_for_each_entry(ses, &clp->cl_sessions, se_perclnt)
> +		seq_printf(m, " %u", ses->se_target_maxslots);
>  	spin_unlock(&clp->cl_lock);
>  	seq_puts(m, "\n");
>  
> @@ -3674,10 +3715,10 @@ nfsd4_exchange_id_release(union nfsd4_op_u *u)
>  	kfree(exid->server_impl_name);
>  }
>  
> -static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, bool slot_inuse)
> +static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, u8 flags)
>  {
>  	/* The slot is in use, and no response has been sent. */
> -	if (slot_inuse) {
> +	if (flags & NFSD4_SLOT_INUSE) {
>  		if (seqid == slot_seqid)
>  			return nfserr_jukebox;
>  		else
> @@ -3686,6 +3727,8 @@ static __be32 check_slot_seqid(u32 seqid, u32 slot_seqid, bool slot_inuse)
>  	/* Note unsigned 32-bit arithmetic handles wraparound: */
>  	if (likely(seqid == slot_seqid + 1))
>  		return nfs_ok;
> +	if ((flags & NFSD4_SLOT_REUSED) && seqid == 1)
> +		return nfs_ok;
>  	if (seqid == slot_seqid)
>  		return nfserr_replay_cache;
>  	return nfserr_seq_misordered;
> @@ -4236,8 +4279,7 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	dprintk("%s: slotid %d\n", __func__, seq->slotid);
>  
>  	trace_nfsd_slot_seqid_sequence(clp, seq, slot);
> -	status = check_slot_seqid(seq->seqid, slot->sl_seqid,
> -					slot->sl_flags & NFSD4_SLOT_INUSE);
> +	status = check_slot_seqid(seq->seqid, slot->sl_seqid, slot->sl_flags);
>  	if (status == nfserr_replay_cache) {
>  		status = nfserr_seq_misordered;
>  		if (!(slot->sl_flags & NFSD4_SLOT_INITIALIZED))
> @@ -4262,6 +4304,12 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	if (status)
>  		goto out_put_session;
>  
> +	if (session->se_target_maxslots < session->se_fchannel.maxreqs &&
> +	    slot->sl_generation == session->se_slot_gen &&
> +	    seq->maxslots <= session->se_target_maxslots)
> +		/* Client acknowledged our reduce maxreqs */
> +		free_session_slots(session, session->se_target_maxslots);
> +
>  	buflen = (seq->cachethis) ?
>  			session->se_fchannel.maxresp_cached :
>  			session->se_fchannel.maxresp_sz;
> @@ -4272,9 +4320,11 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	svc_reserve(rqstp, buflen);
>  
>  	status = nfs_ok;
> -	/* Success! bump slot seqid */
> +	/* Success! accept new slot seqid */
>  	slot->sl_seqid = seq->seqid;
> +	slot->sl_flags &= ~NFSD4_SLOT_REUSED;
>  	slot->sl_flags |= NFSD4_SLOT_INUSE;
> +	slot->sl_generation = session->se_slot_gen;
>  	if (seq->cachethis)
>  		slot->sl_flags |= NFSD4_SLOT_CACHETHIS;
>  	else
> @@ -4291,9 +4341,11 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	 * the client might use.
>  	 */
>  	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
> +	    session->se_target_maxslots >= session->se_fchannel.maxreqs &&
>  	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
>  		int s = session->se_fchannel.maxreqs;
>  		int cnt = DIV_ROUND_UP(s, 5);
> +		void *prev_slot;
>  
>  		do {
>  			/*
> @@ -4307,17 +4359,25 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			 */
>  			slot = kzalloc(slot_bytes(&session->se_fchannel),
>  				       GFP_NOWAIT);
> +			prev_slot = xa_load(&session->se_slots, s);
> +			if (xa_is_value(prev_slot) && slot) {
> +				slot->sl_seqid = xa_to_value(prev_slot);
> +				slot->sl_flags |= NFSD4_SLOT_REUSED;
> +			}
>  			if (slot &&
>  			    !xa_is_err(xa_store(&session->se_slots, s, slot,
>  						GFP_ATOMIC | __GFP_NOWARN))) {
>  				s += 1;
>  				session->se_fchannel.maxreqs = s;
> +				session->se_target_maxslots = s;
>  			} else {
>  				kfree(slot);
> +				slot = NULL;
>  			}
>  		} while (slot && --cnt > 0);
>  	}
> -	seq->maxslots = session->se_fchannel.maxreqs;
> +	seq->maxslots = max(session->se_target_maxslots, seq->maxslots);
> +	seq->target_maxslots = session->se_target_maxslots;
>  
>  out:
>  	switch (clp->cl_cb_state) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 53fac037611c..4dcb03cd9292 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1884,7 +1884,8 @@ nfsd4_decode_sequence(struct nfsd4_compoundargs *argp,
>  		return nfserr_bad_xdr;
>  	seq->seqid = be32_to_cpup(p++);
>  	seq->slotid = be32_to_cpup(p++);
> -	seq->maxslots = be32_to_cpup(p++);
> +	/* sa_highest_slotid counts from 0 but maxslots  counts from 1 ... */
> +	seq->maxslots = be32_to_cpup(p++) + 1;
>  	seq->cachethis = be32_to_cpup(p);
>  
>  	seq->status_flags = 0;
> @@ -4968,7 +4969,7 @@ nfsd4_encode_sequence(struct nfsd4_compoundres *resp, __be32 nfserr,
>  	if (nfserr != nfs_ok)
>  		return nfserr;
>  	/* sr_target_highest_slotid */
> -	nfserr = nfsd4_encode_slotid4(xdr, seq->maxslots - 1);
> +	nfserr = nfsd4_encode_slotid4(xdr, seq->target_maxslots - 1);
>  	if (nfserr != nfs_ok)
>  		return nfserr;
>  	/* sr_status_flags */
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index aad547d3ad8b..74f2ab3c95aa 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -249,7 +249,9 @@ struct nfsd4_slot {
>  #define NFSD4_SLOT_CACHETHIS	(1 << 1)
>  #define NFSD4_SLOT_INITIALIZED	(1 << 2)
>  #define NFSD4_SLOT_CACHED	(1 << 3)
> +#define NFSD4_SLOT_REUSED	(1 << 4)
>  	u8	sl_flags;
> +	u8	sl_generation;
>  	char	sl_data[];
>  };
>  
> @@ -331,6 +333,8 @@ struct nfsd4_session {
>  	struct list_head	se_conns;
>  	u32			se_cb_seq_nr[NFSD_BC_SLOT_TABLE_SIZE];
>  	struct xarray		se_slots;	/* forward channel slots */
> +	u8			se_slot_gen;
> +	u32			se_target_maxslots;
>  };
>  
>  /* formatted contents of nfs4_sessionid */
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 382cc1389396..c26ba86dbdfd 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -576,9 +576,7 @@ struct nfsd4_sequence {
>  	u32			slotid;			/* request/response */
>  	u32			maxslots;		/* request/response */
>  	u32			cachethis;		/* request */
> -#if 0
>  	u32			target_maxslots;	/* response */
> -#endif /* not yet */
>  	u32			status_flags;		/* response */
>  };
>  


I don't see where the above "#if 0" gets removed in patch 6. Shouldn't
it be?

While it makes for a larger patch, I think we'd be better served by
squashing 5 and 6 together. It doesn't make sense to add this core
infrastructure without something to call it.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 5/6] nfsd: add support for freeing unused session-DRC slots

[NeilBrown]

On Fri, 06 Dec 2024, Jeff Layton wrote:
> On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:

> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 382cc1389396..c26ba86dbdfd 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -576,9 +576,7 @@ struct nfsd4_sequence {
> >  	u32			slotid;			/* request/response */
> >  	u32			maxslots;		/* request/response */
> >  	u32			cachethis;		/* request */
> > -#if 0
> >  	u32			target_maxslots;	/* response */
> > -#endif /* not yet */
> >  	u32			status_flags;		/* response */
> >  };
> >  
> 
> 
> I don't see where the above "#if 0" gets removed in patch 6. Shouldn't
> it be?

You are misreading.  It is being removed here in patch 5. 
It was added in 2.6.38 in 
Commit b85d4c01b76f ("nfsd41: sequence operation")


> 
> While it makes for a larger patch, I think we'd be better served by
> squashing 5 and 6 together. It doesn't make sense to add this core
> infrastructure without something to call it.

I find it easier to review if the distinct elements of functionality are
kept separate.  But if both you and Chuck want just one patch here, I
can do that.

Thanks,
NeilBrown


> -- 
> Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH 5/6] nfsd: add support for freeing unused session-DRC slots

[Jeff Layton]

On Fri, 2024-12-06 at 17:05 +1100, NeilBrown wrote:
> On Fri, 06 Dec 2024, Jeff Layton wrote:
> > On Fri, 2024-12-06 at 11:43 +1100, NeilBrown wrote:
> 
> > > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > > index 382cc1389396..c26ba86dbdfd 100644
> > > --- a/fs/nfsd/xdr4.h
> > > +++ b/fs/nfsd/xdr4.h
> > > @@ -576,9 +576,7 @@ struct nfsd4_sequence {
> > >  	u32			slotid;			/* request/response */
> > >  	u32			maxslots;		/* request/response */
> > >  	u32			cachethis;		/* request */
> > > -#if 0
> > >  	u32			target_maxslots;	/* response */
> > > -#endif /* not yet */
> > >  	u32			status_flags;		/* response */
> > >  };
> > >  
> > 
> > 
> > I don't see where the above "#if 0" gets removed in patch 6. Shouldn't
> > it be?
> 
> You are misreading.  It is being removed here in patch 5. 
> It was added in 2.6.38 in 
> Commit b85d4c01b76f ("nfsd41: sequence operation")
> 

Oh, sorry -- my mistake. That's what I get for reviewing patches just
before boarding a redeye flight!

> 
> > 
> > While it makes for a larger patch, I think we'd be better served by
> > squashing 5 and 6 together. It doesn't make sense to add this core
> > infrastructure without something to call it.
> 
> I find it easier to review if the distinct elements of functionality are
> kept separate.  But if both you and Chuck want just one patch here, I
> can do that.
> 

The proposed code is bisectable, so I don't feel too strongly about it.
Adding in unused functions is "Not The Way We (Usually) Do Things"
though.

I think in this case it was harder for me to review, since I had to
skip ahead to patch #6 to see how reduce_session_slots() would actually
be used. The spin_trylock(), in particular was confusing until I
realized it was being called from a shrinker that iterated over all of
the clients and spinning there is probably not good.

Either way, a kerneldoc header over reduce_session_slots() that
explains this subtlety would be nice.
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 6/6] nfsd: add shrinker to reduce number of slots allocated per session

[NeilBrown]

Add a shrinker which frees unused slots and may ask the clients to use
fewer slots on each session.

We keep a global count of the number of freeable slots, which is the sum
of one less than the current "target" slots in all sessions in all
clients in all net-namespaces. This number is reported by the shrinker.

When the shrinker is asked to free some, we call xxx on each session in
a round-robin asking each to reduce the slot count by 1.  This will
reduce the "target" so the number reported by the shrinker will reduce
immediately.  The memory will only be freed later when the client
confirmed that it is no longer needed.

We use a global list of sessions and move the "head" to after the last
session that we asked to reduce, so the next callback from the shrinker
will move on to the next session.  This pressure should be applied
"evenly" across all sessions over time.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 71 ++++++++++++++++++++++++++++++++++++++++++---
 fs/nfsd/state.h     |  1 +
 2 files changed, 68 insertions(+), 4 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index e73668462739..d7bccc237027 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1909,6 +1909,16 @@ gen_sessionid(struct nfsd4_session *ses)
  */
 #define NFSD_MIN_HDR_SEQ_SZ  (24 + 12 + 44)
 
+static struct shrinker *nfsd_slot_shrinker;
+static DEFINE_SPINLOCK(nfsd_session_list_lock);
+static LIST_HEAD(nfsd_session_list);
+/* The sum of "target_slots-1" on every session.  The shrinker can push this
+ * down, though it can take a little while for the memory to actually
+ * be freed.  The "-1" is because we can never free slot 0 while the
+ * session is active.
+ */
+static atomic_t nfsd_total_target_slots = ATOMIC_INIT(0);
+
 static void
 free_session_slots(struct nfsd4_session *ses, int from)
 {
@@ -1930,11 +1940,14 @@ free_session_slots(struct nfsd4_session *ses, int from)
 		kfree(slot);
 	}
 	ses->se_fchannel.maxreqs = from;
-	if (ses->se_target_maxslots > from)
-		ses->se_target_maxslots = from;
+	if (ses->se_target_maxslots > from) {
+		int new_target = from ?: 1;
+		atomic_sub(ses->se_target_maxslots - new_target, &nfsd_total_target_slots);
+		ses->se_target_maxslots = new_target;
+	}
 }
 
-static int __maybe_unused
+static int
 reduce_session_slots(struct nfsd4_session *ses, int dec)
 {
 	struct nfsd_net *nn = net_generic(ses->se_client->net,
@@ -1947,6 +1960,7 @@ reduce_session_slots(struct nfsd4_session *ses, int dec)
 		return ret;
 	ret = min(dec, ses->se_target_maxslots-1);
 	ses->se_target_maxslots -= ret;
+	atomic_sub(ret, &nfsd_total_target_slots);
 	ses->se_slot_gen += 1;
 	if (ses->se_slot_gen == 0) {
 		int i;
@@ -2006,6 +2020,7 @@ static struct nfsd4_session *alloc_session(struct nfsd4_channel_attrs *fattrs,
 	fattrs->maxreqs = i;
 	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
 	new->se_target_maxslots = i;
+	atomic_add(i - 1, &nfsd_total_target_slots);
 	new->se_cb_slot_avail = ~0U;
 	new->se_cb_highest_slot = min(battrs->maxreqs - 1,
 				      NFSD_BC_SLOT_TABLE_SIZE - 1);
@@ -2130,6 +2145,36 @@ static void free_session(struct nfsd4_session *ses)
 	__free_session(ses);
 }
 
+static unsigned long
+nfsd_slot_count(struct shrinker *s, struct shrink_control *sc)
+{
+	unsigned long cnt = atomic_read(&nfsd_total_target_slots);
+
+	return cnt ? cnt : SHRINK_EMPTY;
+}
+
+static unsigned long
+nfsd_slot_scan(struct shrinker *s, struct shrink_control *sc)
+{
+	struct nfsd4_session *ses;
+	unsigned long scanned = 0;
+	unsigned long freed = 0;
+
+	spin_lock(&nfsd_session_list_lock);
+	list_for_each_entry(ses, &nfsd_session_list, se_all_sessions) {
+		freed += reduce_session_slots(ses, 1);
+		scanned += 1;
+		if (scanned >= sc->nr_to_scan) {
+			/* Move starting point for next scan */
+			list_move(&nfsd_session_list, &ses->se_all_sessions);
+			break;
+		}
+	}
+	spin_unlock(&nfsd_session_list_lock);
+	sc->nr_scanned = scanned;
+	return freed;
+}
+
 static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, struct nfs4_client *clp, struct nfsd4_create_session *cses)
 {
 	int idx;
@@ -2154,6 +2199,10 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
 	list_add(&new->se_perclnt, &clp->cl_sessions);
 	spin_unlock(&clp->cl_lock);
 
+	spin_lock(&nfsd_session_list_lock);
+	list_add_tail(&new->se_all_sessions, &nfsd_session_list);
+	spin_unlock(&nfsd_session_list_lock);
+
 	{
 		struct sockaddr *sa = svc_addr(rqstp);
 		/*
@@ -2223,6 +2272,9 @@ unhash_session(struct nfsd4_session *ses)
 	spin_lock(&ses->se_client->cl_lock);
 	list_del(&ses->se_perclnt);
 	spin_unlock(&ses->se_client->cl_lock);
+	spin_lock(&nfsd_session_list_lock);
+	list_del(&ses->se_all_sessions);
+	spin_unlock(&nfsd_session_list_lock);
 }
 
 /* SETCLIENTID and SETCLIENTID_CONFIRM Helper functions */
@@ -4369,6 +4421,8 @@ nfsd4_sequence(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 						GFP_ATOMIC | __GFP_NOWARN))) {
 				s += 1;
 				session->se_fchannel.maxreqs = s;
+				atomic_add(s - session->se_target_maxslots,
+					   &nfsd_total_target_slots);
 				session->se_target_maxslots = s;
 			} else {
 				kfree(slot);
@@ -8765,7 +8819,6 @@ nfs4_state_start_net(struct net *net)
 }
 
 /* initialization to perform when the nfsd service is started: */
-
 int
 nfs4_state_start(void)
 {
@@ -8775,6 +8828,15 @@ nfs4_state_start(void)
 	if (ret)
 		return ret;
 
+	nfsd_slot_shrinker = shrinker_alloc(0, "nfsd-DRC-slot");
+	if (!nfsd_slot_shrinker) {
+		rhltable_destroy(&nfs4_file_rhltable);
+		return -ENOMEM;
+	}
+	nfsd_slot_shrinker->count_objects = nfsd_slot_count;
+	nfsd_slot_shrinker->scan_objects = nfsd_slot_scan;
+	shrinker_register(nfsd_slot_shrinker);
+
 	set_max_delegations();
 	return 0;
 }
@@ -8816,6 +8878,7 @@ void
 nfs4_state_shutdown(void)
 {
 	rhltable_destroy(&nfs4_file_rhltable);
+	shrinker_free(nfsd_slot_shrinker);
 }
 
 static void
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 74f2ab3c95aa..a4ce2cf3d6a3 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -326,6 +326,7 @@ struct nfsd4_session {
 	bool			se_dead;
 	struct list_head	se_hash;	/* hash by sessionid */
 	struct list_head	se_perclnt;
+	struct list_head	se_all_sessions;/* global list of sessions */
 	struct nfs4_client	*se_client;
 	struct nfs4_sessionid	se_sessionid;
 	struct nfsd4_channel_attrs se_fchannel;
-- 
2.47.0

[PATCH 0/6 v5] nfsd: allocate/free session-based DRC slots on demand

[NeilBrown]

Differences from v4:
 - added Jeff's Reviewed-by - thanks.
 - fixed problem in final patch.  There are two places where a session can be
   detached from lists and both need to detach from the new global list.

Thanks,
NeilBrown

 [PATCH 1/6] nfsd: use an xarray to store v4.1 session slots
 [PATCH 2/6] nfsd: remove artificial limits on the session-based DRC
 [PATCH 3/6] nfsd: add session slot count to
 [PATCH 4/6] nfsd: allocate new session-based DRC slots on demand.
 [PATCH 5/6] nfsd: add support for freeing unused session-DRC slots
 [PATCH 6/6] nfsd: add shrinker to reduce number of slots allocated

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

[NeilBrown]

If a client ever uses the highest available slot for a given session,
attempt to allocate more slots so there is room for the client to use
them 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.

Each time we increase the number of slots by 20% (rounded up).  This
allows fairly quick growth while avoiding excessive over-shoot.

We would expect to stablise with around 10% more slots available than
the client actually uses.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 67dfc699e411..fd9473d487f3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4235,11 +4235,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);
@@ -4289,6 +4284,38 @@ 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 20%.  This allows
+	 * fairly quick growth without grossly over-shooting what
+	 * the client might use.
+	 */
+	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
+	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
+		int s = session->se_fchannel.maxreqs;
+		int cnt = DIV_ROUND_UP(s, 5);
+
+		do {
+			/*
+			 * GFP_NOWAIT both allows allocation under a
+			 * spinlock, and only succeeds if there is
+			 * plenty of memory.
+			 */
+			slot = kzalloc(slot_bytes(&session->se_fchannel),
+				       GFP_NOWAIT);
+			if (slot &&
+			    !xa_is_err(xa_store(&session->se_slots, s, slot,
+						GFP_NOWAIT))) {
+				s += 1;
+				session->se_fchannel.maxreqs = s;
+			} else {
+				kfree(slot);
+				slot = NULL;
+			}
+		} while (slot && --cnt > 0);
+	}
+	seq->maxslots = session->se_fchannel.maxreqs;
+
 out:
 	switch (clp->cl_cb_state) {
 	case NFSD4_CB_DOWN:
-- 
2.47.0

[PATCH 0/6 v4] nfsd: allocate/free session-based DRC slots on demand

[NeilBrown]

Changes from v3 include:
 - use GFP_NOWAIT more consistently - don't use GFP_ATOMIC
 - document reduce_session_slots()
 - change sl_generation to u16.  As we reduce the number of slots one at
   a time and update se_slot_gen each time, we could cycle a u8 generation
   counter quickly.

Thanks,
NeilBrown

 [PATCH 1/6] nfsd: use an xarray to store v4.1 session slots
 [PATCH 2/6] nfsd: remove artificial limits on the session-based DRC
 [PATCH 3/6] nfsd: add session slot count to
 [PATCH 4/6] nfsd: allocate new session-based DRC slots on demand.
 [PATCH 5/6] nfsd: add support for freeing unused session-DRC slots
 [PATCH 6/6] nfsd: add shrinker to reduce number of slots allocated

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

[NeilBrown]

If a client ever uses the highest available slot for a given session,
attempt to allocate more slots so there is room for the client to use
them 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.

Each time we increase the number of slots by 20% (rounded up).  This
allows fairly quick growth while avoiding excessive over-shoot.

We would expect to stablise with around 10% more slots available than
the client actually uses.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/nfs4state.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 67dfc699e411..fd9473d487f3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4235,11 +4235,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);
@@ -4289,6 +4284,38 @@ 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 20%.  This allows
+	 * fairly quick growth without grossly over-shooting what
+	 * the client might use.
+	 */
+	if (seq->slotid == session->se_fchannel.maxreqs - 1 &&
+	    session->se_fchannel.maxreqs < NFSD_MAX_SLOTS_PER_SESSION) {
+		int s = session->se_fchannel.maxreqs;
+		int cnt = DIV_ROUND_UP(s, 5);
+
+		do {
+			/*
+			 * GFP_NOWAIT both allows allocation under a
+			 * spinlock, and only succeeds if there is
+			 * plenty of memory.
+			 */
+			slot = kzalloc(slot_bytes(&session->se_fchannel),
+				       GFP_NOWAIT);
+			if (slot &&
+			    !xa_is_err(xa_store(&session->se_slots, s, slot,
+						GFP_NOWAIT))) {
+				s += 1;
+				session->se_fchannel.maxreqs = s;
+			} else {
+				kfree(slot);
+				slot = NULL;
+			}
+		} while (slot && --cnt > 0);
+	}
+	seq->maxslots = session->se_fchannel.maxreqs;
+
 out:
 	switch (clp->cl_cb_state) {
 	case NFSD4_CB_DOWN:
-- 
2.47.0

[PATCH 0/6 RFC v2] nfsd: allocate/free session-based DRC slots on demand

[NeilBrown]

Here is v2 of my series for on-demand allocation and freeing of session DRC slots.

- Now uses an xarray to store slots, and the limit is raised to 2048
- delays retiring a slot until the client has confirmed that it isn't
  using it as described in RFC:

      The replier SHOULD retain the slots it wants to retire until the
      requester sends a request with a highest_slotid less than or equal
      to the replier's new enforced highest_slotid.

- When a retired slot is used, allow the seqid to be the next in sequence
  as required by the RFC:

         Each time a slot is reused, the request MUST specify a sequence
         ID that is one greater than that of the previous request on the
         slot.

  or "1" as (arguably) allowed by the RFC:

         The first time a slot is used, the requester MUST specify a
         sequence ID of one

- current slot allocation is now reported in /proc/fs/nfsd/clients/*/info

This has been tested with highly aggressive shrinker settings:
	nfsd_slot_shrinker->seeks = 0;
	nfsd_slot_shrinker->batch = 2;

and with periodic "echo 3 > drop_caches".  The slot count drops as
expected and then increases again.

NeilBrown

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

[NeilBrown]

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++) {
 		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 */
-- 
2.47.0

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

[Chuck Lever]

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
> 

-- 
Chuck Lever

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

[NeilBrown]

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
> 

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

[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.


-- 
Chuck Lever

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

[NeilBrown]

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

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

[Jeff Layton]

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 */

-- 
Jeff Layton <jlayton@kernel.org>