Re: [PATCH 1/4] nfsd: remove artificial limits on the session-based DRC

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-11-13 at 16:38 +1100, NeilBrown wrote:
> 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.
> 
> Also *always* allow NFSD_MAX_SLOTS_PER_SESSION slot pointers per
> session.  This allows the session to be allocated before we know how
> many slots we will successfully allocate, and will be useful when we
> starting dynamically increasing the number of allocated slots.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/nfs4state.c | 91 +++++++--------------------------------------
>  fs/nfsd/nfsd.h      |  3 --
>  fs/nfsd/nfssvc.c    | 32 ----------------
>  fs/nfsd/state.h     |  2 +-
>  4 files changed, 14 insertions(+), 114 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 551d2958ec29..2dcba0c83c10 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1935,59 +1935,6 @@ 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)
>  {
> @@ -1996,26 +1943,27 @@ 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);
> +	BUILD_BUG_ON(sizeof(new) > PAGE_SIZE);
>  
> -	new = kzalloc(struct_size(new, se_slots, numslots), GFP_KERNEL);
> +	new = kzalloc(sizeof(*new), GFP_KERNEL);
>  	if (!new)
>  		return NULL;
>  	/* 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);
> +	new->se_slots[0] = kzalloc(slotsize, GFP_KERNEL);
> +	if (!new->se_slots[0])
> +		goto out_free;
> +
> +	for (i = 1; i < numslots; i++) {
> +		new->se_slots[i] = kzalloc(slotsize, GFP_KERNEL | __GFP_NORETRY);
>  		if (!new->se_slots[i])
> -			goto out_free;
> +			break;
>  	}
> -
> +	fattrs->maxreqs = i;
>  	memcpy(&new->se_fchannel, fattrs, sizeof(struct nfsd4_channel_attrs));
>  	memcpy(&new->se_bchannel, battrs, sizeof(struct nfsd4_channel_attrs));
>  
>  	return new;
>  out_free:
> -	while (i--)
> -		kfree(new->se_slots[i]);
>  	kfree(new);
>  	return NULL;
>  }
> @@ -2128,7 +2076,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);
>  }
>  
> @@ -3748,17 +3695,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;
>  }
> @@ -3836,11 +3772,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;
> @@ -3950,8 +3886,7 @@ nfsd4_create_session(struct svc_rqst *rqstp,
>  		expire_client(old);
>  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);
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 35b3564c065f..c052e9eea81b 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -310,7 +310,7 @@ struct nfsd4_session {
>  	struct list_head	se_conns;
>  	u32			se_cb_prog;
>  	u32			se_cb_seq_nr;
> -	struct nfsd4_slot	*se_slots[];	/* forward channel slots */
> +	struct nfsd4_slot	*se_slots[NFSD_MAX_SLOTS_PER_SESSION];	/* forward channel slots */
>  };
>  
>  /* formatted contents of nfs4_sessionid */

I like this patch overall. One thing we could consider is allocating
slots aggressively with GFP_KERNEL up to a particular threshold, and
then switch to GFP_NOWAIT, but this seems like a reasonable place to
start.
 
-- 
Jeff Layton <jlayton@kernel.org>