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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
