This patch series implements the memory shrinker for NFSv4 clients
to react to system low memory condition.
The first patch adds a counter to keep track of the number of courtesy
clients in the system.
The second patch implememts the courtesy client shrinker by creating
a new trigger, in addition to the max client limit, for the laundromat
to reap courtesy clients.
By destroying the courtesy clients, all states associated with these
clients are also released.
v2:
. fix kernel test robot errors in nfsd.h when CONFIG_NFSD_V4 not defined.
v3:
. add mod_delayed_work in nfsd_courtesy_client_scan to kick start
the laundromat.
v4:
. replace the use of xchg() with vanilla '=' in patch 1.
v5:
. rename nfsd_courtesy_client_count to nfsd_courtesy_clients
. add helper nfsd4_update_courtesy_client_count
. move nfsd_register_client_shrinker into nfsd4_init_leases_net
. move nfsd4_leases_net_shutdown from nfsd.h to nfs4state.c
. do away with shrinker 'scan' callback, just return SHRINK_STOP
. remove unused nfsd_client_shrinker_reapcount
---
Dai Ngo (2):
NFSD: keep track of the number of courtesy clients in the system
NFSD: add shrinker to reap courtesy clients on low memory condition
fs/nfsd/netns.h | 4 ++++
fs/nfsd/nfs4state.c | 60 ++++++++++++++++++++++++++++++++++++++++++++----
fs/nfsd/nfsctl.c | 6 +++--
fs/nfsd/nfsd.h | 6 +++--
4 files changed, 67 insertions(+), 9 deletions(-)
Add counter nfs4_courtesy_client_count to nfsd_net to keep track
of the number of courtesy clients in the system.
Signed-off-by: Dai Ngo <[email protected]>
---
fs/nfsd/netns.h | 2 ++
fs/nfsd/nfs4state.c | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index ffe17743cc74..55c7006d6109 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -192,6 +192,8 @@ struct nfsd_net {
atomic_t nfs4_client_count;
int nfs4_max_clients;
+
+ atomic_t nfsd_courtesy_clients;
};
/* 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 c5d199d7e6b4..3af4fc5241b2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -160,6 +160,13 @@ static bool is_client_expired(struct nfs4_client *clp)
return clp->cl_time == 0;
}
+static inline void nfsd4_decr_courtesy_client_count(struct nfsd_net *nn,
+ struct nfs4_client *clp)
+{
+ if (clp->cl_state != NFSD4_ACTIVE)
+ atomic_add_unless(&nn->nfsd_courtesy_clients, -1, 0);
+}
+
static __be32 get_client_locked(struct nfs4_client *clp)
{
struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
@@ -169,6 +176,7 @@ static __be32 get_client_locked(struct nfs4_client *clp)
if (is_client_expired(clp))
return nfserr_expired;
atomic_inc(&clp->cl_rpc_users);
+ nfsd4_decr_courtesy_client_count(nn, clp);
clp->cl_state = NFSD4_ACTIVE;
return nfs_ok;
}
@@ -190,6 +198,7 @@ renew_client_locked(struct nfs4_client *clp)
list_move_tail(&clp->cl_lru, &nn->client_lru);
clp->cl_time = ktime_get_boottime_seconds();
+ nfsd4_decr_courtesy_client_count(nn, clp);
clp->cl_state = NFSD4_ACTIVE;
}
@@ -2233,6 +2242,7 @@ __destroy_client(struct nfs4_client *clp)
if (clp->cl_cb_conn.cb_xprt)
svc_xprt_put(clp->cl_cb_conn.cb_xprt);
atomic_add_unless(&nn->nfs4_client_count, -1, 0);
+ nfsd4_decr_courtesy_client_count(nn, clp);
free_client(clp);
wake_up_all(&expiry_wq);
}
@@ -4356,6 +4366,8 @@ void nfsd4_init_leases_net(struct nfsd_net *nn)
max_clients = (u64)si.totalram * si.mem_unit / (1024 * 1024 * 1024);
max_clients *= NFS4_CLIENTS_PER_GB;
nn->nfs4_max_clients = max_t(int, max_clients, NFS4_CLIENTS_PER_GB);
+
+ atomic_set(&nn->nfsd_courtesy_clients, 0);
}
static void init_nfs4_replay(struct nfs4_replay *rp)
@@ -5878,8 +5890,11 @@ nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head *reaplist,
goto exp_client;
if (!state_expired(lt, clp->cl_time))
break;
- if (!atomic_read(&clp->cl_rpc_users))
+ if (!atomic_read(&clp->cl_rpc_users)) {
+ if (clp->cl_state == NFSD4_ACTIVE)
+ atomic_inc(&nn->nfsd_courtesy_clients);
clp->cl_state = NFSD4_COURTESY;
+ }
if (!client_has_state(clp))
goto exp_client;
if (!nfs4_anylock_blockers(clp))
--
2.9.5
Add the courtesy client shrinker to react to low memory condition
triggered by the memory shrinker.
Currently the laundromat starts reaping courtesy clients only when
the maximum number of clients is reached. This patch adds another
trigger condition for the laundromat to reap courtesy clients.
The low memory trigger is created and the laundromat is kick started
by the shrinker's count callback. The shrinker's scan callback is
not used for expiring the courtesy clients due to potential deadlocks.
The laundromat reschedules itself to run sooner if it detects low
memory condition persists and there are more coutersy clients to reap.
Signed-off-by: Dai Ngo <[email protected]>
---
fs/nfsd/netns.h | 2 ++
fs/nfsd/nfs4state.c | 43 +++++++++++++++++++++++++++++++++++++++----
fs/nfsd/nfsctl.c | 6 ++++--
fs/nfsd/nfsd.h | 6 ++++--
4 files changed, 49 insertions(+), 8 deletions(-)
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 55c7006d6109..85e351f08e57 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -194,6 +194,8 @@ struct nfsd_net {
int nfs4_max_clients;
atomic_t nfsd_courtesy_clients;
+ atomic_t nfsd_client_shrinker_cb_count;
+ 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 3af4fc5241b2..a27553b42e72 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -4347,7 +4347,25 @@ 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)
+{
+ 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_clients);
+}
+
+static unsigned long
+nfsd_courtesy_client_scan(struct shrinker *shrink, struct shrink_control *sc)
+{
+ return SHRINK_STOP;
+}
+
+int
+nfsd4_init_leases_net(struct nfsd_net *nn)
{
struct sysinfo si;
u64 max_clients;
@@ -4368,6 +4386,17 @@ 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_clients, 0);
+ atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
+ 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");
+}
+
+void
+nfsd4_leases_net_shutdown(struct nfsd_net *nn)
+{
+ unregister_shrinker(&nn->nfsd_client_shrinker);
}
static void init_nfs4_replay(struct nfs4_replay *rp)
@@ -5876,12 +5905,15 @@ 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;
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;
+ if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients ||
+ atomic_read(&nn->nfsd_client_shrinker_cb_count) > 0) {
+ maxreap = NFSD_CLIENT_MAX_TRIM_PER_RUN;
+ atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
+ }
INIT_LIST_HEAD(reaplist);
spin_lock(&nn->client_lock);
list_for_each_safe(pos, next, &nn->client_lru) {
@@ -5947,6 +5979,9 @@ 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 &&
+ atomic_read(&nn->nfsd_courtesy_clients) > 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..d87a465aa950 100644
--- a/fs/nfsd/nfsd.h
+++ b/fs/nfsd/nfsd.h
@@ -498,7 +498,8 @@ 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);
+extern void nfsd4_leases_net_shutdown(struct nfsd_net *nn);
#else /* CONFIG_NFSD_V4 */
static inline int nfsd4_is_junction(struct dentry *dentry)
@@ -506,7 +507,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
> On Sep 4, 2022, at 1:27 PM, Dai Ngo <[email protected]> wrote:
>
> Add the courtesy client shrinker to react to low memory condition
> triggered by the memory shrinker.
>
> Currently the laundromat starts reaping courtesy clients only when
> the maximum number of clients is reached. This patch adds another
> trigger condition for the laundromat to reap courtesy clients.
>
> The low memory trigger is created and the laundromat is kick started
> by the shrinker's count callback. The shrinker's scan callback is
> not used for expiring the courtesy clients due to potential deadlocks.
>
> The laundromat reschedules itself to run sooner if it detects low
> memory condition persists and there are more coutersy clients to reap.
>
> Signed-off-by: Dai Ngo <[email protected]>
> ---
> fs/nfsd/netns.h | 2 ++
> fs/nfsd/nfs4state.c | 43 +++++++++++++++++++++++++++++++++++++++----
> fs/nfsd/nfsctl.c | 6 ++++--
> fs/nfsd/nfsd.h | 6 ++++--
> 4 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 55c7006d6109..85e351f08e57 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -194,6 +194,8 @@ struct nfsd_net {
> int nfs4_max_clients;
>
> atomic_t nfsd_courtesy_clients;
> + atomic_t nfsd_client_shrinker_cb_count;
> + 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 3af4fc5241b2..a27553b42e72 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4347,7 +4347,25 @@ 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)
> +{
> + 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_clients);
> +}
> +
> +static unsigned long
> +nfsd_courtesy_client_scan(struct shrinker *shrink, struct shrink_control *sc)
> +{
> + return SHRINK_STOP;
> +}
I notice that backend_memory_shrinker does not have a .scan_objects
method -- it does it's garbage collection in its .count_objects
method like we're doing here. But it seems to be the only shrinker
that does not define a .scan_objects method. What you've done here
might be extra lines of code, but is probably safer in the long run.
So, good to go.
> +
> +int
> +nfsd4_init_leases_net(struct nfsd_net *nn)
> {
> struct sysinfo si;
> u64 max_clients;
> @@ -4368,6 +4386,17 @@ 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_clients, 0);
> + atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
> + 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");
> +}
> +
> +void
> +nfsd4_leases_net_shutdown(struct nfsd_net *nn)
> +{
> + unregister_shrinker(&nn->nfsd_client_shrinker);
> }
>
> static void init_nfs4_replay(struct nfs4_replay *rp)
> @@ -5876,12 +5905,15 @@ 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;
> 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;
> + if (atomic_read(&nn->nfs4_client_count) >= nn->nfs4_max_clients ||
> + atomic_read(&nn->nfsd_client_shrinker_cb_count) > 0) {
> + maxreap = NFSD_CLIENT_MAX_TRIM_PER_RUN;
> + atomic_set(&nn->nfsd_client_shrinker_cb_count, 0);
> + }
The rest of this work looks great.
But I really don't like the idea of making the current
nfsd4_get_client_reaplist() do more and more by adding
exception cases inside of it. It's already gnarly! I had the
same difficulty with Neil's original lock simplification
patches.
If there are separate distinct functional needs, let's break
them into distinct functions that then call common pieces (to
maintain good code sharing). That way the operation of both
cases is clearly documented by the code instead of having
one big function with lots of exception processing.
AFAIU, the shrinker needs to look just at courtesy clients. The
laundromat looks at both courtesy clients /and/ active clients.
That suggests refactoring the courtesy client code reaper into
its own function that can be invoked by either the shrinker or
the laundromat.
The laundromat can call this new code and ask for it to trim
only when nfs4_client_count exceeds nfs4_max_clients. The
shrinker can call this new code and ask for it to trim exactly
N clients. N might be a small fixed number like 1 or 5 if we
prefer to start simple.
Obviously you'll have to wrap the refactored code in a work
queue for the shrinker to kick, just like the laundromat.
That adds a little more code, but it makes it crystal clear
what is going on.
> INIT_LIST_HEAD(reaplist);
> spin_lock(&nn->client_lock);
> list_for_each_safe(pos, next, &nn->client_lru) {
> @@ -5947,6 +5979,9 @@ 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 &&
> + atomic_read(&nn->nfsd_courtesy_clients) > 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..d87a465aa950 100644
> --- a/fs/nfsd/nfsd.h
> +++ b/fs/nfsd/nfsd.h
> @@ -498,7 +498,8 @@ 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);
> +extern void nfsd4_leases_net_shutdown(struct nfsd_net *nn);
>
> #else /* CONFIG_NFSD_V4 */
> static inline int nfsd4_is_junction(struct dentry *dentry)
> @@ -506,7 +507,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