> On Aug 30, 2022, at 5:48 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
> Add the courtesy client shrinker to react to low memory condition
> triggered by the memory shrinker.
>
> On the shrinker's count callback, we increment a callback counter
> and return the number of outstanding courtesy clients. When the
> laundromat runs, it checks if this counter is not zero and starts
> reaping old courtesy clients. The maximum number of clients to be
> reaped is limited to NFSD_CIENT_MAX_TRIM_PER_RUN (128). This limit
> is to prevent the laundromat from spending too much time reaping
> the clients and not processing other tasks in a timely manner.
>
> The laundromat is rescheduled to run sooner if it detects low
> low memory condition and there are more clients to reap.
>
> On the shrinker's scan callback, we return the number of clients
> That were reaped since the last scan callback. We can not reap
> the clients on the scan callback context since destroying the
> client might require call into the underlying filesystem or other
> subsystems which might allocate memory which can cause deadlock.
>
> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> ---
> fs/nfsd/netns.h | 3 +++
> fs/nfsd/nfs4state.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++----
> fs/nfsd/nfsctl.c | 6 ++++--
> fs/nfsd/nfsd.h | 9 +++++++--
> 4 files changed, 62 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 2695dff1378a..2a604951623f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -194,6 +194,9 @@ struct nfsd_net {
> int nfs4_max_clients;
>
> atomic_t nfsd_courtesy_client_count;
> + atomic_t nfsd_client_shrinker_cb_count;
> + atomic_t nfsd_client_shrinker_reapcount;
> + struct shrinker nfsd_client_shrinker;
> };
>
> /* Simple check to find out if a given net was properly initialized */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index eaf7b4dcea33..9aed9eed1892 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4343,7 +4343,40 @@ nfsd4_init_slabs(void)
> return -ENOMEM;
> }
>
> -void nfsd4_init_leases_net(struct nfsd_net *nn)
> +static unsigned long
> +nfsd_courtesy_client_count(struct shrinker *shrink, struct shrink_control *sc)
I find it confusing to have a function and a variable with exactly
the same name, especially if they are related. Maybe the variable
name can be nfsd_courtesy_client_num ?
> +{l
> + struct nfsd_net *nn = container_of(shrink,
> + struct nfsd_net, nfsd_client_shrinker);
> +
> + atomic_inc(&nn->nfsd_client_shrinker_cb_count);
> + mod_delayed_work(laundry_wq, &nn->laundromat_work, 0);
> + return (unsigned long)atomic_read(&nn->nfsd_courtesy_client_count);
> +}
> +
> +static unsigned long
> +nfsd_courtesy_client_scan(struct shrinker *shrink, struct shrink_control *sc)
> +{
> + struct nfsd_net *nn = container_of(shrink,
> + struct nfsd_net, nfsd_client_shrinker);
> + unsigned long cnt;
> +
> + cnt = atomic_read(&nn->nfsd_client_shrinker_reapcount);
> + atomic_set(&nn->nfsd_client_shrinker_reapcount, 0);
> + return cnt;
> +}
> +
> +static int
> +nfsd_register_client_shrinker(struct nfsd_net *nn)
> +{
> + nn->nfsd_client_shrinker.scan_objects = nfsd_courtesy_client_scan;
> + nn->nfsd_client_shrinker.count_objects = nfsd_courtesy_client_count;
> + nn->nfsd_client_shrinker.seeks = DEFAULT_SEEKS;
> + return register_shrinker(&nn->nfsd_client_shrinker, "nfsd-client");
> +}
Nit: Move this into nfsd4_init_leases_net(). I don't see added
value for having a one-time use helper for this code.
> +
> +int
> +nfsd4_init_leases_net(struct nfsd_net *nn)
> {
> struct sysinfo si;
> u64 max_clients;
> @@ -4364,6 +4397,8 @@ void nfsd4_init_leases_net(struct nfsd_net *nn)
> nn->nfs4_max_clients = max_t(int, max_clients, NFS4_CLIENTS_PER_GB);
>
> atomic_set(&nn->nfsd_courtesy_client_count, 0);
> + atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
> + return nfsd_register_client_shrinker(nn);
> }
>
> static void init_nfs4_replay(struct nfs4_replay *rp)
> @@ -5872,12 +5907,17 @@ static void
> nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head *reaplist,
> struct laundry_time *lt)
> {
> - unsigned int maxreap, reapcnt = 0;
> + unsigned int maxreap = 0, reapcnt = 0;
> + int cb_cnt;
Nit: Reverse christmas tree, please. cb_cnt goes at the end
of the variable definitions.
> struct list_head *pos, *next;
> struct nfs4_client *clp;
>
> - maxreap = (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients) ?
> - NFSD_CLIENT_MAX_TRIM_PER_RUN : 0;
> + cb_cnt = atomic_read(&nn->nfsd_client_shrinker_cb_count);
> + if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients ||
> + cb_cnt) {
> + maxreap = NFSD_CLIENT_MAX_TRIM_PER_RUN;
> + atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
> + }
I'm not terribly happy with this, but I don't have a better suggestion
at the moment. Let me think about it.
> INIT_LIST_HEAD(reaplist);
> spin_lock(&nn->client_lock);
> list_for_each_safe(pos, next, &nn->client_lru) {
> @@ -5903,6 +5943,8 @@ nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head *reaplist,
> }
> }
> spin_unlock(&nn->client_lock);
> + if (cb_cnt)
> + atomic_add(reapcnt, &nn->nfsd_client_shrinker_reapcount);
> }
>
> static time64_t
> @@ -5943,6 +5985,8 @@ nfs4_laundromat(struct nfsd_net *nn)
> list_del_init(&clp->cl_lru);
> expire_client(clp);
> }
> + if (atomic_read(&nn->nfsd_client_shrinker_cb_count) > 0)
> + lt.new_timeo = NFSD_LAUNDROMAT_MINTIMEOUT;
> spin_lock(&state_lock);
> list_for_each_safe(pos, next, &nn->del_recall_lru) {
> dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
> diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
> index 917fa1892fd2..597a26ad4183 100644
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -1481,11 +1481,12 @@ static __net_init int nfsd_init_net(struct net *net)
> goto out_idmap_error;
> nn->nfsd_versions = NULL;
> nn->nfsd4_minorversions = NULL;
> + retval = nfsd4_init_leases_net(nn);
> + if (retval)
> + goto out_drc_error;
> retval = nfsd_reply_cache_init(nn);
> if (retval)
> goto out_drc_error;
> - nfsd4_init_leases_net(nn);
> -
> get_random_bytes(&nn->siphash_key, sizeof(nn->siphash_key));
> seqlock_init(&nn->writeverf_lock);
>
> @@ -1507,6 +1508,7 @@ static __net_exit void nfsd_exit_net(struct net *net)
> nfsd_idmap_shutdown(net);
> nfsd_export_shutdown(net);
> nfsd_netns_free_versions(net_generic(net, nfsd_net_id));
> + nfsd4_leases_net_shutdown(nn);
> }
>
> static struct pernet_operations nfsd_net_ops = {
> diff --git a/fs/nfsd/nfsd.h b/fs/nfsd/nfsd.h
> index 57a468ed85c3..7e05ab7a3532 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -498,7 +498,11 @@ extern void unregister_cld_notifier(void);
> extern void nfsd4_ssc_init_umount_work(struct nfsd_net *nn);
> #endif
>
> -extern void nfsd4_init_leases_net(struct nfsd_net *nn);
> +extern int nfsd4_init_leases_net(struct nfsd_net *nn);
> +static inline void nfsd4_leases_net_shutdown(struct nfsd_net *nn)
> +{
> + unregister_shrinker(&nn->nfsd_client_shrinker);
> +};
Nit: please move this into nfs4state.c next to nfsd4_init_leases_net().
static inline is used typically for performance-sensitive helpers, and
this adds a dependency on unregister_shrinker in every file that includes
nfsd.h.
> #else /* CONFIG_NFSD_V4 */
> static inline int nfsd4_is_junction(struct dentry *dentry)
> @@ -506,7 +510,8 @@ static inline int nfsd4_is_junction(struct dentry *dentry)
> return 0;
> }
>
> -static inline void nfsd4_init_leases_net(struct nfsd_net *nn) {};
> +static inline int nfsd4_init_leases_net(struct nfsd_net *nn) { return 0; };
> +static inline void nfsd4_leases_net_shutdown(struct nfsd_net *nn) { };
>
> #define register_cld_notifier() 0
> #define unregister_cld_notifier() do { } while(0)
> --
> 2.9.5
>
--
Chuck Lever
