[PATCH 0/4] nfsd: filecache: change garbage collect lists

[PATCH 0/4] nfsd: filecache: change garbage collect lists

[NeilBrown]

The nfsd filecache currently uses  list_lru for tracking files recently
used in NFSv3 requests which need to be "garbage collected" when they
have becoming idle - unused for 2-4 seconds.

I do not believe list_lru is a good tool for this.  It does no allow the
timeout which filecache requires so we have to add a timeout mechanism
which holds the list_lru for while the whole list is scanned looking for
entries that haven't been recently accessed.  When the list is largish
(even a few hundred) this can block new requests which need the lock to
remove a file to access it.

This patch removes the list_lru and instead uses 2 simple linked lists.
When a file is accessed it is removed from whichever list it is one,
then added to the tail of the first list.  Every 2 seconds the second
list is moved to the "freeme" list and the first list is moved to the
second list.  This avoids any need to walk a list to find old entries.

These lists are per-netns rather than global as the freeme list is
per-netns as the actual freeing is done in nfsd threads which are
per-netns.

This should not be applied until we resolve how to handle the
race-detection code in nfsd_file_put().  However I'm posting it now to
get any feedback so that a final version can be posted as soon as that
issue is resolved.

Thanks,
NeilBrown


 [PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in
 [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and
 [PATCH 3/4] nfsd: filecache: change garbage collection list
 [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in nfsd_file_close_inode_sync()

[NeilBrown]

nfsd_file_close_inode_sync() contains an exactly copy of
nfsd_file_dispose_list().

This patch removes it and calls nfsd_file_dispose_list() instead.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index fcd751cb7c76..d8f98e847dc0 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -682,17 +682,12 @@ nfsd_file_close_inode(struct inode *inode)
 void
 nfsd_file_close_inode_sync(struct inode *inode)
 {
-	struct nfsd_file *nf;
 	LIST_HEAD(dispose);
 
 	trace_nfsd_file_close(inode);
 
 	nfsd_file_queue_for_close(inode, &dispose);
-	while (!list_empty(&dispose)) {
-		nf = list_first_entry(&dispose, struct nfsd_file, nf_gc);
-		list_del_init(&nf->nf_gc);
-		nfsd_file_free(nf);
-	}
+	nfsd_file_dispose_list(&dispose);
 }
 
 static int
-- 
2.47.1

Re: [PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in nfsd_file_close_inode_sync()

[Jeff Layton]

On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> nfsd_file_close_inode_sync() contains an exactly copy of
> nfsd_file_dispose_list().
> 
> This patch removes it and calls nfsd_file_dispose_list() instead.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index fcd751cb7c76..d8f98e847dc0 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -682,17 +682,12 @@ nfsd_file_close_inode(struct inode *inode)
>  void
>  nfsd_file_close_inode_sync(struct inode *inode)
>  {
> -	struct nfsd_file *nf;
>  	LIST_HEAD(dispose);
>  
>  	trace_nfsd_file_close(inode);
>  
>  	nfsd_file_queue_for_close(inode, &dispose);
> -	while (!list_empty(&dispose)) {
> -		nf = list_first_entry(&dispose, struct nfsd_file, nf_gc);
> -		list_del_init(&nf->nf_gc);
> -		nfsd_file_free(nf);
> -	}
> +	nfsd_file_dispose_list(&dispose);
>  }
>  
>  static int

Reviewed-by: Jeff Layton <jlayton@kernel.org>

[PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[NeilBrown]

The final freeing of nfsd files is done by per-net nfsd threads (which
call nfsd_file_net_dispose()) so it makes some sense to make more of the
freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.

This is a step towards replacing the list_lru with simple lists which
each share the per-net lock in nfsd_fcache_disposal and will require
less list walking.

As the net is always shutdown before there is any chance that the rest
of the filecache is shut down we can removed the tests on
NFSD_FILE_CACHE_UP.

For the filecache stats file, which assumes a global lru, we keep a
separate counter which includes all files in all netns lrus.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c | 125 ++++++++++++++++++++++++--------------------
 1 file changed, 68 insertions(+), 57 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index d8f98e847dc0..4f39f6632b35 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -63,17 +63,19 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
 
 struct nfsd_fcache_disposal {
 	spinlock_t lock;
+	struct list_lru file_lru;
 	struct list_head freeme;
+	struct delayed_work filecache_laundrette;
+	struct shrinker *file_shrinker;
 };
 
 static struct kmem_cache		*nfsd_file_slab;
 static struct kmem_cache		*nfsd_file_mark_slab;
-static struct list_lru			nfsd_file_lru;
 static unsigned long			nfsd_file_flags;
 static struct fsnotify_group		*nfsd_file_fsnotify_group;
-static struct delayed_work		nfsd_filecache_laundrette;
 static struct rhltable			nfsd_file_rhltable
 						____cacheline_aligned_in_smp;
+static atomic_long_t			nfsd_lru_total = ATOMIC_LONG_INIT(0);
 
 static bool
 nfsd_match_cred(const struct cred *c1, const struct cred *c2)
@@ -109,11 +111,18 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
 };
 
 static void
-nfsd_file_schedule_laundrette(void)
+nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
 {
-	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
-		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
-				   NFSD_LAUNDRETTE_DELAY);
+	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
+			   NFSD_LAUNDRETTE_DELAY);
+}
+
+static void
+nfsd_file_schedule_laundrette_net(struct net *net)
+{
+	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+
+	nfsd_file_schedule_laundrette(nn->fcache_disposal);
 }
 
 static void
@@ -318,11 +327,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
 		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
 }
 
-
 static bool nfsd_file_lru_add(struct nfsd_file *nf)
 {
+	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
+	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
+
 	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
-	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
+	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
+		atomic_long_inc(&nfsd_lru_total);
 		trace_nfsd_file_lru_add(nf);
 		return true;
 	}
@@ -331,7 +343,11 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
 
 static bool nfsd_file_lru_remove(struct nfsd_file *nf)
 {
-	if (list_lru_del_obj(&nfsd_file_lru, &nf->nf_lru)) {
+	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
+	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
+
+	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
+		atomic_long_dec(&nfsd_lru_total);
 		trace_nfsd_file_lru_del(nf);
 		return true;
 	}
@@ -373,7 +389,7 @@ nfsd_file_put(struct nfsd_file *nf)
 		if (nfsd_file_lru_add(nf)) {
 			/* If it's still hashed, we're done */
 			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
-				nfsd_file_schedule_laundrette();
+				nfsd_file_schedule_laundrette_net(nf->nf_net);
 				return;
 			}
 
@@ -539,18 +555,18 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
 }
 
 static void
-nfsd_file_gc(void)
+nfsd_file_gc(struct nfsd_fcache_disposal *l)
 {
-	unsigned long remaining = list_lru_count(&nfsd_file_lru);
+	unsigned long remaining = list_lru_count(&l->file_lru);
 	LIST_HEAD(dispose);
 	unsigned long ret;
 
 	while (remaining > 0) {
 		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
 
-		ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
+		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
 				    &dispose, num_to_scan);
-		trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
+		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
 		nfsd_file_dispose_list_delayed(&dispose);
 		remaining -= num_to_scan;
 	}
@@ -559,32 +575,36 @@ nfsd_file_gc(void)
 static void
 nfsd_file_gc_worker(struct work_struct *work)
 {
-	nfsd_file_gc();
-	if (list_lru_count(&nfsd_file_lru))
-		nfsd_file_schedule_laundrette();
+	struct nfsd_fcache_disposal *l = container_of(
+		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
+	nfsd_file_gc(l);
+	if (list_lru_count(&l->file_lru))
+		nfsd_file_schedule_laundrette(l);
 }
 
 static unsigned long
 nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
 {
-	return list_lru_count(&nfsd_file_lru);
+	struct nfsd_fcache_disposal *l = s->private_data;
+
+	return list_lru_count(&l->file_lru);
 }
 
 static unsigned long
 nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
 {
+	struct nfsd_fcache_disposal *l = s->private_data;
+
 	LIST_HEAD(dispose);
 	unsigned long ret;
 
-	ret = list_lru_shrink_walk(&nfsd_file_lru, sc,
+	ret = list_lru_shrink_walk(&l->file_lru, sc,
 				   nfsd_file_lru_cb, &dispose);
-	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&nfsd_file_lru));
+	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
 	nfsd_file_dispose_list_delayed(&dispose);
 	return ret;
 }
 
-static struct shrinker *nfsd_file_shrinker;
-
 /**
  * nfsd_file_cond_queue - conditionally unhash and queue a nfsd_file
  * @nf: nfsd_file to attempt to queue
@@ -764,29 +784,10 @@ nfsd_file_cache_init(void)
 		goto out_err;
 	}
 
-	ret = list_lru_init(&nfsd_file_lru);
-	if (ret) {
-		pr_err("nfsd: failed to init nfsd_file_lru: %d\n", ret);
-		goto out_err;
-	}
-
-	nfsd_file_shrinker = shrinker_alloc(0, "nfsd-filecache");
-	if (!nfsd_file_shrinker) {
-		ret = -ENOMEM;
-		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
-		goto out_lru;
-	}
-
-	nfsd_file_shrinker->count_objects = nfsd_file_lru_count;
-	nfsd_file_shrinker->scan_objects = nfsd_file_lru_scan;
-	nfsd_file_shrinker->seeks = 1;
-
-	shrinker_register(nfsd_file_shrinker);
-
 	ret = lease_register_notifier(&nfsd_file_lease_notifier);
 	if (ret) {
 		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
-		goto out_shrinker;
+		goto out_err;
 	}
 
 	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
@@ -799,17 +800,12 @@ nfsd_file_cache_init(void)
 		goto out_notifier;
 	}
 
-	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
 out:
 	if (ret)
 		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
 	return ret;
 out_notifier:
 	lease_unregister_notifier(&nfsd_file_lease_notifier);
-out_shrinker:
-	shrinker_free(nfsd_file_shrinker);
-out_lru:
-	list_lru_destroy(&nfsd_file_lru);
 out_err:
 	kmem_cache_destroy(nfsd_file_slab);
 	nfsd_file_slab = NULL;
@@ -861,13 +857,36 @@ nfsd_alloc_fcache_disposal(void)
 	if (!l)
 		return NULL;
 	spin_lock_init(&l->lock);
+	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
 	INIT_LIST_HEAD(&l->freeme);
+	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
+	if (!l->file_shrinker) {
+		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
+		kfree(l);
+		return NULL;
+	}
+	l->file_shrinker->count_objects = nfsd_file_lru_count;
+	l->file_shrinker->scan_objects = nfsd_file_lru_scan;
+	l->file_shrinker->seeks = 1;
+	l->file_shrinker->private_data = l;
+
+	if (list_lru_init(&l->file_lru)) {
+		pr_err("nfsd: failed to init nfsd_file_lru\n");
+		shrinker_free(l->file_shrinker);
+		kfree(l);
+		return NULL;
+	}
+
+	shrinker_register(l->file_shrinker);
 	return l;
 }
 
 static void
 nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
 {
+	cancel_delayed_work_sync(&l->filecache_laundrette);
+	shrinker_free(l->file_shrinker);
+	list_lru_destroy(&l->file_lru);
 	nfsd_file_dispose_list(&l->freeme);
 	kfree(l);
 }
@@ -899,8 +918,7 @@ void
 nfsd_file_cache_purge(struct net *net)
 {
 	lockdep_assert_held(&nfsd_mutex);
-	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
-		__nfsd_file_cache_purge(net);
+	__nfsd_file_cache_purge(net);
 }
 
 void
@@ -920,14 +938,7 @@ nfsd_file_cache_shutdown(void)
 		return;
 
 	lease_unregister_notifier(&nfsd_file_lease_notifier);
-	shrinker_free(nfsd_file_shrinker);
-	/*
-	 * make sure all callers of nfsd_file_lru_cb are done before
-	 * calling nfsd_file_cache_purge
-	 */
-	cancel_delayed_work_sync(&nfsd_filecache_laundrette);
 	__nfsd_file_cache_purge(NULL);
-	list_lru_destroy(&nfsd_file_lru);
 	rcu_barrier();
 	fsnotify_put_group(nfsd_file_fsnotify_group);
 	nfsd_file_fsnotify_group = NULL;
@@ -1298,7 +1309,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
 		struct bucket_table *tbl;
 		struct rhashtable *ht;
 
-		lru = list_lru_count(&nfsd_file_lru);
+		lru = atomic_long_read(&nfsd_lru_total);
 
 		rcu_read_lock();
 		ht = &nfsd_file_rhltable.ht;
-- 
2.47.1

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[Jeff Layton]

On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> The final freeing of nfsd files is done by per-net nfsd threads (which
> call nfsd_file_net_dispose()) so it makes some sense to make more of the
> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> 
> This is a step towards replacing the list_lru with simple lists which
> each share the per-net lock in nfsd_fcache_disposal and will require
> less list walking.
> 
> As the net is always shutdown before there is any chance that the rest
> of the filecache is shut down we can removed the tests on
> NFSD_FILE_CACHE_UP.
> 
> For the filecache stats file, which assumes a global lru, we keep a
> separate counter which includes all files in all netns lrus.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 125 ++++++++++++++++++++++++--------------------
>  1 file changed, 68 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index d8f98e847dc0..4f39f6632b35 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -63,17 +63,19 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
>  
>  struct nfsd_fcache_disposal {
>  	spinlock_t lock;
> +	struct list_lru file_lru;
>  	struct list_head freeme;
> +	struct delayed_work filecache_laundrette;
> +	struct shrinker *file_shrinker;
>  };
>  
>  static struct kmem_cache		*nfsd_file_slab;
>  static struct kmem_cache		*nfsd_file_mark_slab;
> -static struct list_lru			nfsd_file_lru;
>  static unsigned long			nfsd_file_flags;
>  static struct fsnotify_group		*nfsd_file_fsnotify_group;
> -static struct delayed_work		nfsd_filecache_laundrette;
>  static struct rhltable			nfsd_file_rhltable
>  						____cacheline_aligned_in_smp;
> +static atomic_long_t			nfsd_lru_total = ATOMIC_LONG_INIT(0);
>  
>  static bool
>  nfsd_match_cred(const struct cred *c1, const struct cred *c2)
> @@ -109,11 +111,18 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
>  };
>  
>  static void
> -nfsd_file_schedule_laundrette(void)
> +nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
>  {
> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
> -		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
> -				   NFSD_LAUNDRETTE_DELAY);
> +	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
> +			   NFSD_LAUNDRETTE_DELAY);
> +}
> +
> +static void
> +nfsd_file_schedule_laundrette_net(struct net *net)
> +{
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	nfsd_file_schedule_laundrette(nn->fcache_disposal);
>  }
>  
>  static void
> @@ -318,11 +327,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
>  		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
>  }
>  
> -
>  static bool nfsd_file_lru_add(struct nfsd_file *nf)
>  {
> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
>  	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> -	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
> +	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
> +		atomic_long_inc(&nfsd_lru_total);
>  		trace_nfsd_file_lru_add(nf);
>  		return true;
>  	}
> @@ -331,7 +343,11 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
>  
>  static bool nfsd_file_lru_remove(struct nfsd_file *nf)
>  {
> -	if (list_lru_del_obj(&nfsd_file_lru, &nf->nf_lru)) {
> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
> +	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
> +		atomic_long_dec(&nfsd_lru_total);
>  		trace_nfsd_file_lru_del(nf);
>  		return true;
>  	}
> @@ -373,7 +389,7 @@ nfsd_file_put(struct nfsd_file *nf)
>  		if (nfsd_file_lru_add(nf)) {
>  			/* If it's still hashed, we're done */
>  			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> -				nfsd_file_schedule_laundrette();
> +				nfsd_file_schedule_laundrette_net(nf->nf_net);
>  				return;
>  			}
>  
> @@ -539,18 +555,18 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
>  }
>  
>  static void
> -nfsd_file_gc(void)
> +nfsd_file_gc(struct nfsd_fcache_disposal *l)
>  {
> -	unsigned long remaining = list_lru_count(&nfsd_file_lru);
> +	unsigned long remaining = list_lru_count(&l->file_lru);
>  	LIST_HEAD(dispose);
>  	unsigned long ret;
>  
>  	while (remaining > 0) {
>  		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
>  
> -		ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
> +		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
>  				    &dispose, num_to_scan);
> -		trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
> +		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
>  		nfsd_file_dispose_list_delayed(&dispose);
>  		remaining -= num_to_scan;
>  	}
> @@ -559,32 +575,36 @@ nfsd_file_gc(void)
>  static void
>  nfsd_file_gc_worker(struct work_struct *work)
>  {
> -	nfsd_file_gc();
> -	if (list_lru_count(&nfsd_file_lru))
> -		nfsd_file_schedule_laundrette();
> +	struct nfsd_fcache_disposal *l = container_of(
> +		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> +	nfsd_file_gc(l);
> +	if (list_lru_count(&l->file_lru))
> +		nfsd_file_schedule_laundrette(l);
>  }
>  
>  static unsigned long
>  nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
>  {
> -	return list_lru_count(&nfsd_file_lru);
> +	struct nfsd_fcache_disposal *l = s->private_data;
> +
> +	return list_lru_count(&l->file_lru);
>  }
>  
>  static unsigned long
>  nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>  {
> +	struct nfsd_fcache_disposal *l = s->private_data;
> +
>  	LIST_HEAD(dispose);
>  	unsigned long ret;
>  
> -	ret = list_lru_shrink_walk(&nfsd_file_lru, sc,
> +	ret = list_lru_shrink_walk(&l->file_lru, sc,
>  				   nfsd_file_lru_cb, &dispose);
> -	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&nfsd_file_lru));
> +	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
>  	nfsd_file_dispose_list_delayed(&dispose);
>  	return ret;
>  }
>  
> -static struct shrinker *nfsd_file_shrinker;
> -
>  /**
>   * nfsd_file_cond_queue - conditionally unhash and queue a nfsd_file
>   * @nf: nfsd_file to attempt to queue
> @@ -764,29 +784,10 @@ nfsd_file_cache_init(void)
>  		goto out_err;
>  	}
>  
> -	ret = list_lru_init(&nfsd_file_lru);
> -	if (ret) {
> -		pr_err("nfsd: failed to init nfsd_file_lru: %d\n", ret);
> -		goto out_err;
> -	}
> -
> -	nfsd_file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> -	if (!nfsd_file_shrinker) {
> -		ret = -ENOMEM;
> -		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> -		goto out_lru;
> -	}
> -
> -	nfsd_file_shrinker->count_objects = nfsd_file_lru_count;
> -	nfsd_file_shrinker->scan_objects = nfsd_file_lru_scan;
> -	nfsd_file_shrinker->seeks = 1;
> -
> -	shrinker_register(nfsd_file_shrinker);
> -
>  	ret = lease_register_notifier(&nfsd_file_lease_notifier);
>  	if (ret) {
>  		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
> -		goto out_shrinker;
> +		goto out_err;
>  	}
>  
>  	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
> @@ -799,17 +800,12 @@ nfsd_file_cache_init(void)
>  		goto out_notifier;
>  	}
>  
> -	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
>  out:
>  	if (ret)
>  		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
>  	return ret;
>  out_notifier:
>  	lease_unregister_notifier(&nfsd_file_lease_notifier);
> -out_shrinker:
> -	shrinker_free(nfsd_file_shrinker);
> -out_lru:
> -	list_lru_destroy(&nfsd_file_lru);
>  out_err:
>  	kmem_cache_destroy(nfsd_file_slab);
>  	nfsd_file_slab = NULL;
> @@ -861,13 +857,36 @@ nfsd_alloc_fcache_disposal(void)
>  	if (!l)
>  		return NULL;
>  	spin_lock_init(&l->lock);
> +	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
>  	INIT_LIST_HEAD(&l->freeme);
> +	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> +	if (!l->file_shrinker) {
> +		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> +		kfree(l);
> +		return NULL;
> +	}
> +	l->file_shrinker->count_objects = nfsd_file_lru_count;
> +	l->file_shrinker->scan_objects = nfsd_file_lru_scan;
> +	l->file_shrinker->seeks = 1;
> +	l->file_shrinker->private_data = l;
> +
> +	if (list_lru_init(&l->file_lru)) {
> +		pr_err("nfsd: failed to init nfsd_file_lru\n");
> +		shrinker_free(l->file_shrinker);
> +		kfree(l);
> +		return NULL;
> +	}
> +
> +	shrinker_register(l->file_shrinker);
>  	return l;
>  }
>  
>  static void
>  nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>  {
> +	cancel_delayed_work_sync(&l->filecache_laundrette);
> +	shrinker_free(l->file_shrinker);
> +	list_lru_destroy(&l->file_lru);
>  	nfsd_file_dispose_list(&l->freeme);
>  	kfree(l);
>  }
> @@ -899,8 +918,7 @@ void
>  nfsd_file_cache_purge(struct net *net)
>  {
>  	lockdep_assert_held(&nfsd_mutex);
> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
> -		__nfsd_file_cache_purge(net);
> +	__nfsd_file_cache_purge(net);
>  }
>  
>  void
> @@ -920,14 +938,7 @@ nfsd_file_cache_shutdown(void)
>  		return;
>  
>  	lease_unregister_notifier(&nfsd_file_lease_notifier);
> -	shrinker_free(nfsd_file_shrinker);
> -	/*
> -	 * make sure all callers of nfsd_file_lru_cb are done before
> -	 * calling nfsd_file_cache_purge
> -	 */
> -	cancel_delayed_work_sync(&nfsd_filecache_laundrette);
>  	__nfsd_file_cache_purge(NULL);
> -	list_lru_destroy(&nfsd_file_lru);
>  	rcu_barrier();
>  	fsnotify_put_group(nfsd_file_fsnotify_group);
>  	nfsd_file_fsnotify_group = NULL;
> @@ -1298,7 +1309,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
>  		struct bucket_table *tbl;
>  		struct rhashtable *ht;
>  
> -		lru = list_lru_count(&nfsd_file_lru);
> +		lru = atomic_long_read(&nfsd_lru_total);
>  
>  		rcu_read_lock();
>  		ht = &nfsd_file_rhltable.ht;

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[Chuck Lever]

On 1/21/25 10:54 PM, NeilBrown wrote:
> The final freeing of nfsd files is done by per-net nfsd threads (which
> call nfsd_file_net_dispose()) so it makes some sense to make more of the
> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> 
> This is a step towards replacing the list_lru with simple lists which
> each share the per-net lock in nfsd_fcache_disposal and will require
> less list walking.
> 
> As the net is always shutdown before there is any chance that the rest
> of the filecache is shut down we can removed the tests on
> NFSD_FILE_CACHE_UP.
> 
> For the filecache stats file, which assumes a global lru, we keep a
> separate counter which includes all files in all netns lrus.

Hey Neil -

One of the issues with the current code is that the contention for
the LRU lock has been effectively buried. It would be nice to have a
way to expose that contention in the new code.

Can this patch or this series add some lock class infrastructure to
help account for the time spent contending for these dynamically
allocated spin locks?


> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>   fs/nfsd/filecache.c | 125 ++++++++++++++++++++++++--------------------
>   1 file changed, 68 insertions(+), 57 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index d8f98e847dc0..4f39f6632b35 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -63,17 +63,19 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
>   
>   struct nfsd_fcache_disposal {
>   	spinlock_t lock;
> +	struct list_lru file_lru;
>   	struct list_head freeme;
> +	struct delayed_work filecache_laundrette;
> +	struct shrinker *file_shrinker;
>   };
>   
>   static struct kmem_cache		*nfsd_file_slab;
>   static struct kmem_cache		*nfsd_file_mark_slab;
> -static struct list_lru			nfsd_file_lru;
>   static unsigned long			nfsd_file_flags;
>   static struct fsnotify_group		*nfsd_file_fsnotify_group;
> -static struct delayed_work		nfsd_filecache_laundrette;
>   static struct rhltable			nfsd_file_rhltable
>   						____cacheline_aligned_in_smp;
> +static atomic_long_t			nfsd_lru_total = ATOMIC_LONG_INIT(0);
>   
>   static bool
>   nfsd_match_cred(const struct cred *c1, const struct cred *c2)
> @@ -109,11 +111,18 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
>   };
>   
>   static void
> -nfsd_file_schedule_laundrette(void)
> +nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
>   {
> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
> -		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
> -				   NFSD_LAUNDRETTE_DELAY);
> +	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
> +			   NFSD_LAUNDRETTE_DELAY);
> +}
> +
> +static void
> +nfsd_file_schedule_laundrette_net(struct net *net)
> +{
> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +
> +	nfsd_file_schedule_laundrette(nn->fcache_disposal);
>   }
>   
>   static void
> @@ -318,11 +327,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
>   		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
>   }
>   
> -
>   static bool nfsd_file_lru_add(struct nfsd_file *nf)
>   {
> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
>   	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> -	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
> +	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
> +		atomic_long_inc(&nfsd_lru_total);
>   		trace_nfsd_file_lru_add(nf);
>   		return true;
>   	}
> @@ -331,7 +343,11 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
>   
>   static bool nfsd_file_lru_remove(struct nfsd_file *nf)
>   {
> -	if (list_lru_del_obj(&nfsd_file_lru, &nf->nf_lru)) {
> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> +
> +	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
> +		atomic_long_dec(&nfsd_lru_total);
>   		trace_nfsd_file_lru_del(nf);
>   		return true;
>   	}
> @@ -373,7 +389,7 @@ nfsd_file_put(struct nfsd_file *nf)
>   		if (nfsd_file_lru_add(nf)) {
>   			/* If it's still hashed, we're done */
>   			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> -				nfsd_file_schedule_laundrette();
> +				nfsd_file_schedule_laundrette_net(nf->nf_net);
>   				return;
>   			}
>   
> @@ -539,18 +555,18 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
>   }
>   
>   static void
> -nfsd_file_gc(void)
> +nfsd_file_gc(struct nfsd_fcache_disposal *l)
>   {
> -	unsigned long remaining = list_lru_count(&nfsd_file_lru);
> +	unsigned long remaining = list_lru_count(&l->file_lru);
>   	LIST_HEAD(dispose);
>   	unsigned long ret;
>   
>   	while (remaining > 0) {
>   		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
>   
> -		ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
> +		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
>   				    &dispose, num_to_scan);
> -		trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
> +		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
>   		nfsd_file_dispose_list_delayed(&dispose);
>   		remaining -= num_to_scan;
>   	}
> @@ -559,32 +575,36 @@ nfsd_file_gc(void)
>   static void
>   nfsd_file_gc_worker(struct work_struct *work)
>   {
> -	nfsd_file_gc();
> -	if (list_lru_count(&nfsd_file_lru))
> -		nfsd_file_schedule_laundrette();
> +	struct nfsd_fcache_disposal *l = container_of(
> +		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> +	nfsd_file_gc(l);
> +	if (list_lru_count(&l->file_lru))
> +		nfsd_file_schedule_laundrette(l);
>   }
>   
>   static unsigned long
>   nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
>   {
> -	return list_lru_count(&nfsd_file_lru);
> +	struct nfsd_fcache_disposal *l = s->private_data;
> +
> +	return list_lru_count(&l->file_lru);
>   }
>   
>   static unsigned long
>   nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>   {
> +	struct nfsd_fcache_disposal *l = s->private_data;
> +
>   	LIST_HEAD(dispose);
>   	unsigned long ret;
>   
> -	ret = list_lru_shrink_walk(&nfsd_file_lru, sc,
> +	ret = list_lru_shrink_walk(&l->file_lru, sc,
>   				   nfsd_file_lru_cb, &dispose);
> -	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&nfsd_file_lru));
> +	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
>   	nfsd_file_dispose_list_delayed(&dispose);
>   	return ret;
>   }
>   
> -static struct shrinker *nfsd_file_shrinker;
> -
>   /**
>    * nfsd_file_cond_queue - conditionally unhash and queue a nfsd_file
>    * @nf: nfsd_file to attempt to queue
> @@ -764,29 +784,10 @@ nfsd_file_cache_init(void)
>   		goto out_err;
>   	}
>   
> -	ret = list_lru_init(&nfsd_file_lru);
> -	if (ret) {
> -		pr_err("nfsd: failed to init nfsd_file_lru: %d\n", ret);
> -		goto out_err;
> -	}
> -
> -	nfsd_file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> -	if (!nfsd_file_shrinker) {
> -		ret = -ENOMEM;
> -		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> -		goto out_lru;
> -	}
> -
> -	nfsd_file_shrinker->count_objects = nfsd_file_lru_count;
> -	nfsd_file_shrinker->scan_objects = nfsd_file_lru_scan;
> -	nfsd_file_shrinker->seeks = 1;
> -
> -	shrinker_register(nfsd_file_shrinker);
> -
>   	ret = lease_register_notifier(&nfsd_file_lease_notifier);
>   	if (ret) {
>   		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
> -		goto out_shrinker;
> +		goto out_err;
>   	}
>   
>   	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
> @@ -799,17 +800,12 @@ nfsd_file_cache_init(void)
>   		goto out_notifier;
>   	}
>   
> -	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
>   out:
>   	if (ret)
>   		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
>   	return ret;
>   out_notifier:
>   	lease_unregister_notifier(&nfsd_file_lease_notifier);
> -out_shrinker:
> -	shrinker_free(nfsd_file_shrinker);
> -out_lru:
> -	list_lru_destroy(&nfsd_file_lru);
>   out_err:
>   	kmem_cache_destroy(nfsd_file_slab);
>   	nfsd_file_slab = NULL;
> @@ -861,13 +857,36 @@ nfsd_alloc_fcache_disposal(void)
>   	if (!l)
>   		return NULL;
>   	spin_lock_init(&l->lock);
> +	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
>   	INIT_LIST_HEAD(&l->freeme);
> +	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> +	if (!l->file_shrinker) {
> +		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> +		kfree(l);
> +		return NULL;
> +	}
> +	l->file_shrinker->count_objects = nfsd_file_lru_count;
> +	l->file_shrinker->scan_objects = nfsd_file_lru_scan;
> +	l->file_shrinker->seeks = 1;
> +	l->file_shrinker->private_data = l;
> +
> +	if (list_lru_init(&l->file_lru)) {
> +		pr_err("nfsd: failed to init nfsd_file_lru\n");
> +		shrinker_free(l->file_shrinker);
> +		kfree(l);
> +		return NULL;
> +	}
> +
> +	shrinker_register(l->file_shrinker);
>   	return l;
>   }
>   
>   static void
>   nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>   {
> +	cancel_delayed_work_sync(&l->filecache_laundrette);
> +	shrinker_free(l->file_shrinker);
> +	list_lru_destroy(&l->file_lru);
>   	nfsd_file_dispose_list(&l->freeme);
>   	kfree(l);
>   }
> @@ -899,8 +918,7 @@ void
>   nfsd_file_cache_purge(struct net *net)
>   {
>   	lockdep_assert_held(&nfsd_mutex);
> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
> -		__nfsd_file_cache_purge(net);
> +	__nfsd_file_cache_purge(net);
>   }
>   
>   void
> @@ -920,14 +938,7 @@ nfsd_file_cache_shutdown(void)
>   		return;
>   
>   	lease_unregister_notifier(&nfsd_file_lease_notifier);
> -	shrinker_free(nfsd_file_shrinker);
> -	/*
> -	 * make sure all callers of nfsd_file_lru_cb are done before
> -	 * calling nfsd_file_cache_purge
> -	 */
> -	cancel_delayed_work_sync(&nfsd_filecache_laundrette);
>   	__nfsd_file_cache_purge(NULL);
> -	list_lru_destroy(&nfsd_file_lru);
>   	rcu_barrier();
>   	fsnotify_put_group(nfsd_file_fsnotify_group);
>   	nfsd_file_fsnotify_group = NULL;
> @@ -1298,7 +1309,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
>   		struct bucket_table *tbl;
>   		struct rhashtable *ht;
>   
> -		lru = list_lru_count(&nfsd_file_lru);
> +		lru = atomic_long_read(&nfsd_lru_total);
>   
>   		rcu_read_lock();
>   		ht = &nfsd_file_rhltable.ht;


-- 
Chuck Lever

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[NeilBrown]

On Thu, 23 Jan 2025, Chuck Lever wrote:
> On 1/21/25 10:54 PM, NeilBrown wrote:
> > The final freeing of nfsd files is done by per-net nfsd threads (which
> > call nfsd_file_net_dispose()) so it makes some sense to make more of the
> > freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> > 
> > This is a step towards replacing the list_lru with simple lists which
> > each share the per-net lock in nfsd_fcache_disposal and will require
> > less list walking.
> > 
> > As the net is always shutdown before there is any chance that the rest
> > of the filecache is shut down we can removed the tests on
> > NFSD_FILE_CACHE_UP.
> > 
> > For the filecache stats file, which assumes a global lru, we keep a
> > separate counter which includes all files in all netns lrus.
> 
> Hey Neil -
> 
> One of the issues with the current code is that the contention for
> the LRU lock has been effectively buried. It would be nice to have a
> way to expose that contention in the new code.
> 
> Can this patch or this series add some lock class infrastructure to
> help account for the time spent contending for these dynamically
> allocated spin locks?

Unfortunately I don't know anything about accounting for lock contention
time.

I know about lock classes for the purpose of lockdep checking but not
how they relate to contention tracking.
Could you give me some pointers?

Thanks,
NeilBrown


> 
> 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> >   fs/nfsd/filecache.c | 125 ++++++++++++++++++++++++--------------------
> >   1 file changed, 68 insertions(+), 57 deletions(-)
> > 
> > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > index d8f98e847dc0..4f39f6632b35 100644
> > --- a/fs/nfsd/filecache.c
> > +++ b/fs/nfsd/filecache.c
> > @@ -63,17 +63,19 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
> >   
> >   struct nfsd_fcache_disposal {
> >   	spinlock_t lock;
> > +	struct list_lru file_lru;
> >   	struct list_head freeme;
> > +	struct delayed_work filecache_laundrette;
> > +	struct shrinker *file_shrinker;
> >   };
> >   
> >   static struct kmem_cache		*nfsd_file_slab;
> >   static struct kmem_cache		*nfsd_file_mark_slab;
> > -static struct list_lru			nfsd_file_lru;
> >   static unsigned long			nfsd_file_flags;
> >   static struct fsnotify_group		*nfsd_file_fsnotify_group;
> > -static struct delayed_work		nfsd_filecache_laundrette;
> >   static struct rhltable			nfsd_file_rhltable
> >   						____cacheline_aligned_in_smp;
> > +static atomic_long_t			nfsd_lru_total = ATOMIC_LONG_INIT(0);
> >   
> >   static bool
> >   nfsd_match_cred(const struct cred *c1, const struct cred *c2)
> > @@ -109,11 +111,18 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
> >   };
> >   
> >   static void
> > -nfsd_file_schedule_laundrette(void)
> > +nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
> >   {
> > -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
> > -		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
> > -				   NFSD_LAUNDRETTE_DELAY);
> > +	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
> > +			   NFSD_LAUNDRETTE_DELAY);
> > +}
> > +
> > +static void
> > +nfsd_file_schedule_laundrette_net(struct net *net)
> > +{
> > +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> > +
> > +	nfsd_file_schedule_laundrette(nn->fcache_disposal);
> >   }
> >   
> >   static void
> > @@ -318,11 +327,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
> >   		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
> >   }
> >   
> > -
> >   static bool nfsd_file_lru_add(struct nfsd_file *nf)
> >   {
> > +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> > +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > +
> >   	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> > -	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
> > +	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
> > +		atomic_long_inc(&nfsd_lru_total);
> >   		trace_nfsd_file_lru_add(nf);
> >   		return true;
> >   	}
> > @@ -331,7 +343,11 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
> >   
> >   static bool nfsd_file_lru_remove(struct nfsd_file *nf)
> >   {
> > -	if (list_lru_del_obj(&nfsd_file_lru, &nf->nf_lru)) {
> > +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
> > +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
> > +
> > +	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
> > +		atomic_long_dec(&nfsd_lru_total);
> >   		trace_nfsd_file_lru_del(nf);
> >   		return true;
> >   	}
> > @@ -373,7 +389,7 @@ nfsd_file_put(struct nfsd_file *nf)
> >   		if (nfsd_file_lru_add(nf)) {
> >   			/* If it's still hashed, we're done */
> >   			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
> > -				nfsd_file_schedule_laundrette();
> > +				nfsd_file_schedule_laundrette_net(nf->nf_net);
> >   				return;
> >   			}
> >   
> > @@ -539,18 +555,18 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
> >   }
> >   
> >   static void
> > -nfsd_file_gc(void)
> > +nfsd_file_gc(struct nfsd_fcache_disposal *l)
> >   {
> > -	unsigned long remaining = list_lru_count(&nfsd_file_lru);
> > +	unsigned long remaining = list_lru_count(&l->file_lru);
> >   	LIST_HEAD(dispose);
> >   	unsigned long ret;
> >   
> >   	while (remaining > 0) {
> >   		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
> >   
> > -		ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
> > +		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
> >   				    &dispose, num_to_scan);
> > -		trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
> > +		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
> >   		nfsd_file_dispose_list_delayed(&dispose);
> >   		remaining -= num_to_scan;
> >   	}
> > @@ -559,32 +575,36 @@ nfsd_file_gc(void)
> >   static void
> >   nfsd_file_gc_worker(struct work_struct *work)
> >   {
> > -	nfsd_file_gc();
> > -	if (list_lru_count(&nfsd_file_lru))
> > -		nfsd_file_schedule_laundrette();
> > +	struct nfsd_fcache_disposal *l = container_of(
> > +		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> > +	nfsd_file_gc(l);
> > +	if (list_lru_count(&l->file_lru))
> > +		nfsd_file_schedule_laundrette(l);
> >   }
> >   
> >   static unsigned long
> >   nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
> >   {
> > -	return list_lru_count(&nfsd_file_lru);
> > +	struct nfsd_fcache_disposal *l = s->private_data;
> > +
> > +	return list_lru_count(&l->file_lru);
> >   }
> >   
> >   static unsigned long
> >   nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
> >   {
> > +	struct nfsd_fcache_disposal *l = s->private_data;
> > +
> >   	LIST_HEAD(dispose);
> >   	unsigned long ret;
> >   
> > -	ret = list_lru_shrink_walk(&nfsd_file_lru, sc,
> > +	ret = list_lru_shrink_walk(&l->file_lru, sc,
> >   				   nfsd_file_lru_cb, &dispose);
> > -	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&nfsd_file_lru));
> > +	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
> >   	nfsd_file_dispose_list_delayed(&dispose);
> >   	return ret;
> >   }
> >   
> > -static struct shrinker *nfsd_file_shrinker;
> > -
> >   /**
> >    * nfsd_file_cond_queue - conditionally unhash and queue a nfsd_file
> >    * @nf: nfsd_file to attempt to queue
> > @@ -764,29 +784,10 @@ nfsd_file_cache_init(void)
> >   		goto out_err;
> >   	}
> >   
> > -	ret = list_lru_init(&nfsd_file_lru);
> > -	if (ret) {
> > -		pr_err("nfsd: failed to init nfsd_file_lru: %d\n", ret);
> > -		goto out_err;
> > -	}
> > -
> > -	nfsd_file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> > -	if (!nfsd_file_shrinker) {
> > -		ret = -ENOMEM;
> > -		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> > -		goto out_lru;
> > -	}
> > -
> > -	nfsd_file_shrinker->count_objects = nfsd_file_lru_count;
> > -	nfsd_file_shrinker->scan_objects = nfsd_file_lru_scan;
> > -	nfsd_file_shrinker->seeks = 1;
> > -
> > -	shrinker_register(nfsd_file_shrinker);
> > -
> >   	ret = lease_register_notifier(&nfsd_file_lease_notifier);
> >   	if (ret) {
> >   		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
> > -		goto out_shrinker;
> > +		goto out_err;
> >   	}
> >   
> >   	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
> > @@ -799,17 +800,12 @@ nfsd_file_cache_init(void)
> >   		goto out_notifier;
> >   	}
> >   
> > -	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
> >   out:
> >   	if (ret)
> >   		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
> >   	return ret;
> >   out_notifier:
> >   	lease_unregister_notifier(&nfsd_file_lease_notifier);
> > -out_shrinker:
> > -	shrinker_free(nfsd_file_shrinker);
> > -out_lru:
> > -	list_lru_destroy(&nfsd_file_lru);
> >   out_err:
> >   	kmem_cache_destroy(nfsd_file_slab);
> >   	nfsd_file_slab = NULL;
> > @@ -861,13 +857,36 @@ nfsd_alloc_fcache_disposal(void)
> >   	if (!l)
> >   		return NULL;
> >   	spin_lock_init(&l->lock);
> > +	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> >   	INIT_LIST_HEAD(&l->freeme);
> > +	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
> > +	if (!l->file_shrinker) {
> > +		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> > +		kfree(l);
> > +		return NULL;
> > +	}
> > +	l->file_shrinker->count_objects = nfsd_file_lru_count;
> > +	l->file_shrinker->scan_objects = nfsd_file_lru_scan;
> > +	l->file_shrinker->seeks = 1;
> > +	l->file_shrinker->private_data = l;
> > +
> > +	if (list_lru_init(&l->file_lru)) {
> > +		pr_err("nfsd: failed to init nfsd_file_lru\n");
> > +		shrinker_free(l->file_shrinker);
> > +		kfree(l);
> > +		return NULL;
> > +	}
> > +
> > +	shrinker_register(l->file_shrinker);
> >   	return l;
> >   }
> >   
> >   static void
> >   nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
> >   {
> > +	cancel_delayed_work_sync(&l->filecache_laundrette);
> > +	shrinker_free(l->file_shrinker);
> > +	list_lru_destroy(&l->file_lru);
> >   	nfsd_file_dispose_list(&l->freeme);
> >   	kfree(l);
> >   }
> > @@ -899,8 +918,7 @@ void
> >   nfsd_file_cache_purge(struct net *net)
> >   {
> >   	lockdep_assert_held(&nfsd_mutex);
> > -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
> > -		__nfsd_file_cache_purge(net);
> > +	__nfsd_file_cache_purge(net);
> >   }
> >   
> >   void
> > @@ -920,14 +938,7 @@ nfsd_file_cache_shutdown(void)
> >   		return;
> >   
> >   	lease_unregister_notifier(&nfsd_file_lease_notifier);
> > -	shrinker_free(nfsd_file_shrinker);
> > -	/*
> > -	 * make sure all callers of nfsd_file_lru_cb are done before
> > -	 * calling nfsd_file_cache_purge
> > -	 */
> > -	cancel_delayed_work_sync(&nfsd_filecache_laundrette);
> >   	__nfsd_file_cache_purge(NULL);
> > -	list_lru_destroy(&nfsd_file_lru);
> >   	rcu_barrier();
> >   	fsnotify_put_group(nfsd_file_fsnotify_group);
> >   	nfsd_file_fsnotify_group = NULL;
> > @@ -1298,7 +1309,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
> >   		struct bucket_table *tbl;
> >   		struct rhashtable *ht;
> >   
> > -		lru = list_lru_count(&nfsd_file_lru);
> > +		lru = atomic_long_read(&nfsd_lru_total);
> >   
> >   		rcu_read_lock();
> >   		ht = &nfsd_file_rhltable.ht;
> 
> 
> -- 
> Chuck Lever
> 

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[Chuck Lever]

On 1/22/25 5:10 PM, NeilBrown wrote:
> On Thu, 23 Jan 2025, Chuck Lever wrote:
>> On 1/21/25 10:54 PM, NeilBrown wrote:
>>> The final freeing of nfsd files is done by per-net nfsd threads (which
>>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
>>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
>>>
>>> This is a step towards replacing the list_lru with simple lists which
>>> each share the per-net lock in nfsd_fcache_disposal and will require
>>> less list walking.
>>>
>>> As the net is always shutdown before there is any chance that the rest
>>> of the filecache is shut down we can removed the tests on
>>> NFSD_FILE_CACHE_UP.
>>>
>>> For the filecache stats file, which assumes a global lru, we keep a
>>> separate counter which includes all files in all netns lrus.
>>
>> Hey Neil -
>>
>> One of the issues with the current code is that the contention for
>> the LRU lock has been effectively buried. It would be nice to have a
>> way to expose that contention in the new code.
>>
>> Can this patch or this series add some lock class infrastructure to
>> help account for the time spent contending for these dynamically
>> allocated spin locks?
> 
> Unfortunately I don't know anything about accounting for lock contention
> time.
> 
> I know about lock classes for the purpose of lockdep checking but not
> how they relate to contention tracking.
> Could you give me some pointers?

Sticking these locks into a class is all you need to do. When lockstat
is enabled, it automatically accumulates the statistics for all locks
in a class, and treats that as a single lock in /proc/lock_stat.


> Thanks,
> NeilBrown
> 
> 
>>
>>
>>> Signed-off-by: NeilBrown <neilb@suse.de>
>>> ---
>>>    fs/nfsd/filecache.c | 125 ++++++++++++++++++++++++--------------------
>>>    1 file changed, 68 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>>> index d8f98e847dc0..4f39f6632b35 100644
>>> --- a/fs/nfsd/filecache.c
>>> +++ b/fs/nfsd/filecache.c
>>> @@ -63,17 +63,19 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
>>>    
>>>    struct nfsd_fcache_disposal {
>>>    	spinlock_t lock;
>>> +	struct list_lru file_lru;
>>>    	struct list_head freeme;
>>> +	struct delayed_work filecache_laundrette;
>>> +	struct shrinker *file_shrinker;
>>>    };
>>>    
>>>    static struct kmem_cache		*nfsd_file_slab;
>>>    static struct kmem_cache		*nfsd_file_mark_slab;
>>> -static struct list_lru			nfsd_file_lru;
>>>    static unsigned long			nfsd_file_flags;
>>>    static struct fsnotify_group		*nfsd_file_fsnotify_group;
>>> -static struct delayed_work		nfsd_filecache_laundrette;
>>>    static struct rhltable			nfsd_file_rhltable
>>>    						____cacheline_aligned_in_smp;
>>> +static atomic_long_t			nfsd_lru_total = ATOMIC_LONG_INIT(0);
>>>    
>>>    static bool
>>>    nfsd_match_cred(const struct cred *c1, const struct cred *c2)
>>> @@ -109,11 +111,18 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
>>>    };
>>>    
>>>    static void
>>> -nfsd_file_schedule_laundrette(void)
>>> +nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
>>>    {
>>> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags))
>>> -		queue_delayed_work(system_unbound_wq, &nfsd_filecache_laundrette,
>>> -				   NFSD_LAUNDRETTE_DELAY);
>>> +	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
>>> +			   NFSD_LAUNDRETTE_DELAY);
>>> +}
>>> +
>>> +static void
>>> +nfsd_file_schedule_laundrette_net(struct net *net)
>>> +{
>>> +	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>>> +
>>> +	nfsd_file_schedule_laundrette(nn->fcache_disposal);
>>>    }
>>>    
>>>    static void
>>> @@ -318,11 +327,14 @@ nfsd_file_check_writeback(struct nfsd_file *nf)
>>>    		mapping_tagged(mapping, PAGECACHE_TAG_WRITEBACK);
>>>    }
>>>    
>>> -
>>>    static bool nfsd_file_lru_add(struct nfsd_file *nf)
>>>    {
>>> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>>> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>>> +
>>>    	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
>>> -	if (list_lru_add_obj(&nfsd_file_lru, &nf->nf_lru)) {
>>> +	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
>>> +		atomic_long_inc(&nfsd_lru_total);
>>>    		trace_nfsd_file_lru_add(nf);
>>>    		return true;
>>>    	}
>>> @@ -331,7 +343,11 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
>>>    
>>>    static bool nfsd_file_lru_remove(struct nfsd_file *nf)
>>>    {
>>> -	if (list_lru_del_obj(&nfsd_file_lru, &nf->nf_lru)) {
>>> +	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>>> +	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>>> +
>>> +	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
>>> +		atomic_long_dec(&nfsd_lru_total);
>>>    		trace_nfsd_file_lru_del(nf);
>>>    		return true;
>>>    	}
>>> @@ -373,7 +389,7 @@ nfsd_file_put(struct nfsd_file *nf)
>>>    		if (nfsd_file_lru_add(nf)) {
>>>    			/* If it's still hashed, we're done */
>>>    			if (test_bit(NFSD_FILE_HASHED, &nf->nf_flags)) {
>>> -				nfsd_file_schedule_laundrette();
>>> +				nfsd_file_schedule_laundrette_net(nf->nf_net);
>>>    				return;
>>>    			}
>>>    
>>> @@ -539,18 +555,18 @@ nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
>>>    }
>>>    
>>>    static void
>>> -nfsd_file_gc(void)
>>> +nfsd_file_gc(struct nfsd_fcache_disposal *l)
>>>    {
>>> -	unsigned long remaining = list_lru_count(&nfsd_file_lru);
>>> +	unsigned long remaining = list_lru_count(&l->file_lru);
>>>    	LIST_HEAD(dispose);
>>>    	unsigned long ret;
>>>    
>>>    	while (remaining > 0) {
>>>    		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
>>>    
>>> -		ret = list_lru_walk(&nfsd_file_lru, nfsd_file_lru_cb,
>>> +		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
>>>    				    &dispose, num_to_scan);
>>> -		trace_nfsd_file_gc_removed(ret, list_lru_count(&nfsd_file_lru));
>>> +		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
>>>    		nfsd_file_dispose_list_delayed(&dispose);
>>>    		remaining -= num_to_scan;
>>>    	}
>>> @@ -559,32 +575,36 @@ nfsd_file_gc(void)
>>>    static void
>>>    nfsd_file_gc_worker(struct work_struct *work)
>>>    {
>>> -	nfsd_file_gc();
>>> -	if (list_lru_count(&nfsd_file_lru))
>>> -		nfsd_file_schedule_laundrette();
>>> +	struct nfsd_fcache_disposal *l = container_of(
>>> +		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
>>> +	nfsd_file_gc(l);
>>> +	if (list_lru_count(&l->file_lru))
>>> +		nfsd_file_schedule_laundrette(l);
>>>    }
>>>    
>>>    static unsigned long
>>>    nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
>>>    {
>>> -	return list_lru_count(&nfsd_file_lru);
>>> +	struct nfsd_fcache_disposal *l = s->private_data;
>>> +
>>> +	return list_lru_count(&l->file_lru);
>>>    }
>>>    
>>>    static unsigned long
>>>    nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>>>    {
>>> +	struct nfsd_fcache_disposal *l = s->private_data;
>>> +
>>>    	LIST_HEAD(dispose);
>>>    	unsigned long ret;
>>>    
>>> -	ret = list_lru_shrink_walk(&nfsd_file_lru, sc,
>>> +	ret = list_lru_shrink_walk(&l->file_lru, sc,
>>>    				   nfsd_file_lru_cb, &dispose);
>>> -	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&nfsd_file_lru));
>>> +	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
>>>    	nfsd_file_dispose_list_delayed(&dispose);
>>>    	return ret;
>>>    }
>>>    
>>> -static struct shrinker *nfsd_file_shrinker;
>>> -
>>>    /**
>>>     * nfsd_file_cond_queue - conditionally unhash and queue a nfsd_file
>>>     * @nf: nfsd_file to attempt to queue
>>> @@ -764,29 +784,10 @@ nfsd_file_cache_init(void)
>>>    		goto out_err;
>>>    	}
>>>    
>>> -	ret = list_lru_init(&nfsd_file_lru);
>>> -	if (ret) {
>>> -		pr_err("nfsd: failed to init nfsd_file_lru: %d\n", ret);
>>> -		goto out_err;
>>> -	}
>>> -
>>> -	nfsd_file_shrinker = shrinker_alloc(0, "nfsd-filecache");
>>> -	if (!nfsd_file_shrinker) {
>>> -		ret = -ENOMEM;
>>> -		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
>>> -		goto out_lru;
>>> -	}
>>> -
>>> -	nfsd_file_shrinker->count_objects = nfsd_file_lru_count;
>>> -	nfsd_file_shrinker->scan_objects = nfsd_file_lru_scan;
>>> -	nfsd_file_shrinker->seeks = 1;
>>> -
>>> -	shrinker_register(nfsd_file_shrinker);
>>> -
>>>    	ret = lease_register_notifier(&nfsd_file_lease_notifier);
>>>    	if (ret) {
>>>    		pr_err("nfsd: unable to register lease notifier: %d\n", ret);
>>> -		goto out_shrinker;
>>> +		goto out_err;
>>>    	}
>>>    
>>>    	nfsd_file_fsnotify_group = fsnotify_alloc_group(&nfsd_file_fsnotify_ops,
>>> @@ -799,17 +800,12 @@ nfsd_file_cache_init(void)
>>>    		goto out_notifier;
>>>    	}
>>>    
>>> -	INIT_DELAYED_WORK(&nfsd_filecache_laundrette, nfsd_file_gc_worker);
>>>    out:
>>>    	if (ret)
>>>    		clear_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags);
>>>    	return ret;
>>>    out_notifier:
>>>    	lease_unregister_notifier(&nfsd_file_lease_notifier);
>>> -out_shrinker:
>>> -	shrinker_free(nfsd_file_shrinker);
>>> -out_lru:
>>> -	list_lru_destroy(&nfsd_file_lru);
>>>    out_err:
>>>    	kmem_cache_destroy(nfsd_file_slab);
>>>    	nfsd_file_slab = NULL;
>>> @@ -861,13 +857,36 @@ nfsd_alloc_fcache_disposal(void)
>>>    	if (!l)
>>>    		return NULL;
>>>    	spin_lock_init(&l->lock);
>>> +	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
>>>    	INIT_LIST_HEAD(&l->freeme);
>>> +	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
>>> +	if (!l->file_shrinker) {
>>> +		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
>>> +		kfree(l);
>>> +		return NULL;
>>> +	}
>>> +	l->file_shrinker->count_objects = nfsd_file_lru_count;
>>> +	l->file_shrinker->scan_objects = nfsd_file_lru_scan;
>>> +	l->file_shrinker->seeks = 1;
>>> +	l->file_shrinker->private_data = l;
>>> +
>>> +	if (list_lru_init(&l->file_lru)) {
>>> +		pr_err("nfsd: failed to init nfsd_file_lru\n");
>>> +		shrinker_free(l->file_shrinker);
>>> +		kfree(l);
>>> +		return NULL;
>>> +	}
>>> +
>>> +	shrinker_register(l->file_shrinker);
>>>    	return l;
>>>    }
>>>    
>>>    static void
>>>    nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>>>    {
>>> +	cancel_delayed_work_sync(&l->filecache_laundrette);
>>> +	shrinker_free(l->file_shrinker);
>>> +	list_lru_destroy(&l->file_lru);
>>>    	nfsd_file_dispose_list(&l->freeme);
>>>    	kfree(l);
>>>    }
>>> @@ -899,8 +918,7 @@ void
>>>    nfsd_file_cache_purge(struct net *net)
>>>    {
>>>    	lockdep_assert_held(&nfsd_mutex);
>>> -	if (test_bit(NFSD_FILE_CACHE_UP, &nfsd_file_flags) == 1)
>>> -		__nfsd_file_cache_purge(net);
>>> +	__nfsd_file_cache_purge(net);
>>>    }
>>>    
>>>    void
>>> @@ -920,14 +938,7 @@ nfsd_file_cache_shutdown(void)
>>>    		return;
>>>    
>>>    	lease_unregister_notifier(&nfsd_file_lease_notifier);
>>> -	shrinker_free(nfsd_file_shrinker);
>>> -	/*
>>> -	 * make sure all callers of nfsd_file_lru_cb are done before
>>> -	 * calling nfsd_file_cache_purge
>>> -	 */
>>> -	cancel_delayed_work_sync(&nfsd_filecache_laundrette);
>>>    	__nfsd_file_cache_purge(NULL);
>>> -	list_lru_destroy(&nfsd_file_lru);
>>>    	rcu_barrier();
>>>    	fsnotify_put_group(nfsd_file_fsnotify_group);
>>>    	nfsd_file_fsnotify_group = NULL;
>>> @@ -1298,7 +1309,7 @@ int nfsd_file_cache_stats_show(struct seq_file *m, void *v)
>>>    		struct bucket_table *tbl;
>>>    		struct rhashtable *ht;
>>>    
>>> -		lru = list_lru_count(&nfsd_file_lru);
>>> +		lru = atomic_long_read(&nfsd_lru_total);
>>>    
>>>    		rcu_read_lock();
>>>    		ht = &nfsd_file_rhltable.ht;
>>
>>
>> -- 
>> Chuck Lever
>>
> 


-- 
Chuck Lever

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[NeilBrown]

On Fri, 24 Jan 2025, Chuck Lever wrote:
> On 1/22/25 5:10 PM, NeilBrown wrote:
> > On Thu, 23 Jan 2025, Chuck Lever wrote:
> >> On 1/21/25 10:54 PM, NeilBrown wrote:
> >>> The final freeing of nfsd files is done by per-net nfsd threads (which
> >>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
> >>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> >>>
> >>> This is a step towards replacing the list_lru with simple lists which
> >>> each share the per-net lock in nfsd_fcache_disposal and will require
> >>> less list walking.
> >>>
> >>> As the net is always shutdown before there is any chance that the rest
> >>> of the filecache is shut down we can removed the tests on
> >>> NFSD_FILE_CACHE_UP.
> >>>
> >>> For the filecache stats file, which assumes a global lru, we keep a
> >>> separate counter which includes all files in all netns lrus.
> >>
> >> Hey Neil -
> >>
> >> One of the issues with the current code is that the contention for
> >> the LRU lock has been effectively buried. It would be nice to have a
> >> way to expose that contention in the new code.
> >>
> >> Can this patch or this series add some lock class infrastructure to
> >> help account for the time spent contending for these dynamically
> >> allocated spin locks?
> > 
> > Unfortunately I don't know anything about accounting for lock contention
> > time.
> > 
> > I know about lock classes for the purpose of lockdep checking but not
> > how they relate to contention tracking.
> > Could you give me some pointers?
> 
> Sticking these locks into a class is all you need to do. When lockstat
> is enabled, it automatically accumulates the statistics for all locks
> in a class, and treats that as a single lock in /proc/lock_stat.
> 

Ahh thanks.  So I don't need to do anything as this lock has it's own
spin_lock_init() so it already has it's own class.
However.... the init call is :

	spin_lock_init(&l->lock);

so that appear in /proc/lock_stat as

      &l->lock:
or maybe
      &l->lock/1:
or even
      &l->lock/2:

Maybe I should change the "l" to something more unique. "nfd" ??
Or I could actually define a lock_class_key and call __spin_lock_init:

   __skin_lock_init(&l->lock, "nfsd_fcache_disposal->lock", &key,
     false);

There is no precedent for that though.

NeilBrown

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[Chuck Lever]

On 1/26/25 5:33 PM, NeilBrown wrote:
> On Fri, 24 Jan 2025, Chuck Lever wrote:
>> On 1/22/25 5:10 PM, NeilBrown wrote:
>>> On Thu, 23 Jan 2025, Chuck Lever wrote:
>>>> On 1/21/25 10:54 PM, NeilBrown wrote:
>>>>> The final freeing of nfsd files is done by per-net nfsd threads (which
>>>>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
>>>>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
>>>>>
>>>>> This is a step towards replacing the list_lru with simple lists which
>>>>> each share the per-net lock in nfsd_fcache_disposal and will require
>>>>> less list walking.
>>>>>
>>>>> As the net is always shutdown before there is any chance that the rest
>>>>> of the filecache is shut down we can removed the tests on
>>>>> NFSD_FILE_CACHE_UP.
>>>>>
>>>>> For the filecache stats file, which assumes a global lru, we keep a
>>>>> separate counter which includes all files in all netns lrus.
>>>>
>>>> Hey Neil -
>>>>
>>>> One of the issues with the current code is that the contention for
>>>> the LRU lock has been effectively buried. It would be nice to have a
>>>> way to expose that contention in the new code.
>>>>
>>>> Can this patch or this series add some lock class infrastructure to
>>>> help account for the time spent contending for these dynamically
>>>> allocated spin locks?
>>>
>>> Unfortunately I don't know anything about accounting for lock contention
>>> time.
>>>
>>> I know about lock classes for the purpose of lockdep checking but not
>>> how they relate to contention tracking.
>>> Could you give me some pointers?
>>
>> Sticking these locks into a class is all you need to do. When lockstat
>> is enabled, it automatically accumulates the statistics for all locks
>> in a class, and treats that as a single lock in /proc/lock_stat.
>>
> 
> Ahh thanks.  So I don't need to do anything as this lock has it's own
> spin_lock_init() so it already has it's own class.
> However.... the init call is :
> 
> 	spin_lock_init(&l->lock);
> 
> so that appear in /proc/lock_stat as
> 
>        &l->lock:
> or maybe
>        &l->lock/1:
> or even
>        &l->lock/2:

Well, that's the problem, as I see it. They might all appear separately,
but we want lock_stat to group them together so we can see the aggregate
wait time and contention metrics.

Have a look at svc_reclassify_socket().


> Maybe I should change the "l" to something more unique. "nfd" ??
> Or I could actually define a lock_class_key and call __spin_lock_init:
> 
>     __skin_lock_init(&l->lock, "nfsd_fcache_disposal->lock", &key,
>       false);
> 
> There is no precedent for that though.
> 
> NeilBrown
> 


-- 
Chuck Lever

Re: [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and nfsd_file_shrinker to be per-net

[NeilBrown]

On Tue, 28 Jan 2025, Chuck Lever wrote:
> On 1/26/25 5:33 PM, NeilBrown wrote:
> > On Fri, 24 Jan 2025, Chuck Lever wrote:
> >> On 1/22/25 5:10 PM, NeilBrown wrote:
> >>> On Thu, 23 Jan 2025, Chuck Lever wrote:
> >>>> On 1/21/25 10:54 PM, NeilBrown wrote:
> >>>>> The final freeing of nfsd files is done by per-net nfsd threads (which
> >>>>> call nfsd_file_net_dispose()) so it makes some sense to make more of the
> >>>>> freeing infrastructure to be per-net - in struct nfsd_fcache_disposal.
> >>>>>
> >>>>> This is a step towards replacing the list_lru with simple lists which
> >>>>> each share the per-net lock in nfsd_fcache_disposal and will require
> >>>>> less list walking.
> >>>>>
> >>>>> As the net is always shutdown before there is any chance that the rest
> >>>>> of the filecache is shut down we can removed the tests on
> >>>>> NFSD_FILE_CACHE_UP.
> >>>>>
> >>>>> For the filecache stats file, which assumes a global lru, we keep a
> >>>>> separate counter which includes all files in all netns lrus.
> >>>>
> >>>> Hey Neil -
> >>>>
> >>>> One of the issues with the current code is that the contention for
> >>>> the LRU lock has been effectively buried. It would be nice to have a
> >>>> way to expose that contention in the new code.
> >>>>
> >>>> Can this patch or this series add some lock class infrastructure to
> >>>> help account for the time spent contending for these dynamically
> >>>> allocated spin locks?
> >>>
> >>> Unfortunately I don't know anything about accounting for lock contention
> >>> time.
> >>>
> >>> I know about lock classes for the purpose of lockdep checking but not
> >>> how they relate to contention tracking.
> >>> Could you give me some pointers?
> >>
> >> Sticking these locks into a class is all you need to do. When lockstat
> >> is enabled, it automatically accumulates the statistics for all locks
> >> in a class, and treats that as a single lock in /proc/lock_stat.
> >>
> > 
> > Ahh thanks.  So I don't need to do anything as this lock has it's own
> > spin_lock_init() so it already has it's own class.
> > However.... the init call is :
> > 
> > 	spin_lock_init(&l->lock);
> > 
> > so that appear in /proc/lock_stat as
> > 
> >        &l->lock:
> > or maybe
> >        &l->lock/1:
> > or even
> >        &l->lock/2:
> 
> Well, that's the problem, as I see it. They might all appear separately,
> but we want lock_stat to group them together so we can see the aggregate
> wait time and contention metrics.

That isn't what is happening here.

$ git grep 'spin_lock_init(&l->lock)'
fs/bcachefs/nocow_locking.c:            spin_lock_init(&l->lock);
fs/nfsd/filecache.c:    spin_lock_init(&l->lock);
kernel/bpf/bpf_lru_list.c:      raw_spin_lock_init(&l->lock);
mm/list_lru.c:  spin_lock_init(&l->lock);

so there are 4 completely different locks that can appear in
/proc/locks_stat as &l->lock.  So subsequent ones are given numeric
suffixes. 

> 
> Have a look at svc_reclassify_socket().

Ahh - nice.
I was thinking it would be good to have
    spin_lock_init_name()
for cases like this.  Maybe I should post a patch..

Thanks,
NeilBrown

> 
> 
> > Maybe I should change the "l" to something more unique. "nfd" ??
> > Or I could actually define a lock_class_key and call __spin_lock_init:
> > 
> >     __skin_lock_init(&l->lock, "nfsd_fcache_disposal->lock", &key,
> >       false);
> > 
> > There is no precedent for that though.
> > 
> > NeilBrown
> > 
> 
> 
> -- 
> Chuck Lever
> 

[PATCH 3/4] nfsd: filecache: change garbage collection list management.

[NeilBrown]

The nfsd filecache currently uses  list_lru for tracking files recently
used in NFSv3 requests which need to be "garbage collected" when they
have becoming idle - unused for 2-4 seconds.

I do not believe list_lru is a good tool for this.  It does not allow the
timeout which filecache requires so we have to add a timeout mechanism
which holds the list_lru lock while the whole list is scanned looking for
entries that haven't been recently accessed.  When the list is largish
(even a few hundred) this can block new requests which need the lock to
remove a file to access it.

This patch removes the list_lru and instead uses 2 simple linked lists.
When a file is accessed it is removed from whichever list it is on,
then added to the tail of the first list.  Every 2 seconds the second
list is moved to the "freeme" list and the first list is moved to the
second list.  This avoids any need to walk a list to find old entries.

Previously a file would be unhashed before being moved to the freeme
list.  We don't do that any more.  The freeme list is much like the
other two lists (recent and older) in that they all hold a reference to
the file and the file is still hashed.  When the nfsd thread processes
the freeme list it now uses the new nfsd_file_release_list() which uses
nfsd_file_cond_queue() to unhash and drop the refcount.

We no longer have a precise count of the size of the lru (recent +
older) as we don't know how big "older" is when it is moved to "freeme".
However the shrinker can cope with an approximation.  So we keep a
count of number in the lru and when "older" is moved to "freeme" we
divide that count by 2.  When we remove anything from the lru we
decrement that counter but ensure it never goes negative.  Naturally
when we add to the lru we increase the counter.

For the filecache stats file, which assumes a global lru, we keep a
separate counter which includes all files in all netns in recent or
older or freeme.

We discard the nf_gc linkage in an nfsd_file and only use nf_lru.
We discard NFSD_FILE_REFERENCED.

This patch drops the nfsd_file_gc_removed() trace point.  I couldn't
think of useful information to provide.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c | 215 ++++++++++++++++++++++----------------------
 fs/nfsd/filecache.h |   4 +-
 fs/nfsd/trace.h     |   4 -
 3 files changed, 108 insertions(+), 115 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 4f39f6632b35..552feba94f09 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -34,7 +34,6 @@
 #include <linux/file.h>
 #include <linux/pagemap.h>
 #include <linux/sched.h>
-#include <linux/list_lru.h>
 #include <linux/fsnotify_backend.h>
 #include <linux/fsnotify.h>
 #include <linux/seq_file.h>
@@ -63,10 +62,13 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
 
 struct nfsd_fcache_disposal {
 	spinlock_t lock;
-	struct list_lru file_lru;
-	struct list_head freeme;
+	struct list_head recent; /* have been used in last 0-2 seconds */
+	struct list_head older;	/* haven't been used in last 0-2 seconds */
+	struct list_head freeme; /* ready to be discarded */
+	unsigned long num_gc; /* Approximate size of recent plus older */
 	struct delayed_work filecache_laundrette;
 	struct shrinker *file_shrinker;
+	struct nfsd_net *nn;
 };
 
 static struct kmem_cache		*nfsd_file_slab;
@@ -227,7 +229,6 @@ nfsd_file_alloc(struct net *net, struct inode *inode, unsigned char need,
 
 	this_cpu_inc(nfsd_file_allocations);
 	INIT_LIST_HEAD(&nf->nf_lru);
-	INIT_LIST_HEAD(&nf->nf_gc);
 	nf->nf_birthtime = ktime_get();
 	nf->nf_file = NULL;
 	nf->nf_cred = get_current_cred();
@@ -332,12 +333,16 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
 	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
-	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
-	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
+	spin_lock(&l->lock);
+	if (list_empty(&nf->nf_lru)) {
+		list_add_tail(&nf->nf_lru, &l->recent);
+		l->num_gc += 1;
 		atomic_long_inc(&nfsd_lru_total);
 		trace_nfsd_file_lru_add(nf);
+		spin_unlock(&l->lock);
 		return true;
 	}
+	spin_unlock(&l->lock);
 	return false;
 }
 
@@ -346,11 +351,17 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
 	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
-	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
+	spin_lock(&l->lock);
+	if (!list_empty(&nf->nf_lru)) {
+		list_del_init(&nf->nf_lru);
 		atomic_long_dec(&nfsd_lru_total);
+		if (l->num_gc > 0)
+			l->num_gc -= 1;
 		trace_nfsd_file_lru_del(nf);
+		spin_unlock(&l->lock);
 		return true;
 	}
+	spin_unlock(&l->lock);
 	return false;
 }
 
@@ -440,12 +451,26 @@ nfsd_file_dispose_list(struct list_head *dispose)
 	struct nfsd_file *nf;
 
 	while (!list_empty(dispose)) {
-		nf = list_first_entry(dispose, struct nfsd_file, nf_gc);
-		list_del_init(&nf->nf_gc);
+		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
+		list_del_init(&nf->nf_lru);
 		nfsd_file_free(nf);
 	}
 }
 
+static void
+nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose);
+
+static void
+nfsd_file_release_list(struct list_head *dispose)
+{
+	LIST_HEAD(dispose2);
+	struct nfsd_file *nf, *nf2;
+
+	list_for_each_entry_safe(nf, nf2, dispose, nf_lru)
+		nfsd_file_cond_queue(nf, &dispose2);
+	nfsd_file_dispose_list(&dispose2);
+}
+
 /**
  * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list
  * @dispose: list of nfsd_files to be disposed
@@ -458,12 +483,12 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
 {
 	while(!list_empty(dispose)) {
 		struct nfsd_file *nf = list_first_entry(dispose,
-						struct nfsd_file, nf_gc);
+						struct nfsd_file, nf_lru);
 		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
 		spin_lock(&l->lock);
-		list_move_tail(&nf->nf_gc, &l->freeme);
+		list_move_tail(&nf->nf_lru, &l->freeme);
 		spin_unlock(&l->lock);
 		svc_wake_up(nn->nfsd_serv);
 	}
@@ -487,88 +512,32 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
 		int i;
 
 		spin_lock(&l->lock);
-		for (i = 0; i < 8 && !list_empty(&l->freeme); i++)
-			list_move(l->freeme.next, &dispose);
+		for (i = 0; i < 8 && !list_empty(&l->freeme); i++) {
+			struct nfsd_file *nf = list_first_entry(
+				&l->freeme, struct nfsd_file, nf_lru);
+
+			/*
+			 * Don't throw out files that are still
+			 * undergoing I/O or that have uncleared errors
+			 * pending.
+			 */
+			if (nfsd_file_check_writeback(nf)) {
+				trace_nfsd_file_gc_writeback(nf);
+				list_move(&nf->nf_lru, &l->recent);
+				l->num_gc += 1;
+			} else {
+				trace_nfsd_file_gc_disposed(nf);
+				list_move(&nf->nf_lru, &dispose);
+				this_cpu_inc(nfsd_file_evictions);
+			}
+		}
 		spin_unlock(&l->lock);
 		if (!list_empty(&l->freeme))
 			/* Wake up another thread to share the work
 			 * *before* doing any actual disposing.
 			 */
 			svc_wake_up(nn->nfsd_serv);
-		nfsd_file_dispose_list(&dispose);
-	}
-}
-
-/**
- * nfsd_file_lru_cb - Examine an entry on the LRU list
- * @item: LRU entry to examine
- * @lru: controlling LRU
- * @arg: dispose list
- *
- * Return values:
- *   %LRU_REMOVED: @item was removed from the LRU
- *   %LRU_ROTATE: @item is to be moved to the LRU tail
- *   %LRU_SKIP: @item cannot be evicted
- */
-static enum lru_status
-nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
-		 void *arg)
-{
-	struct list_head *head = arg;
-	struct nfsd_file *nf = list_entry(item, struct nfsd_file, nf_lru);
-
-	/* We should only be dealing with GC entries here */
-	WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));
-
-	/*
-	 * Don't throw out files that are still undergoing I/O or
-	 * that have uncleared errors pending.
-	 */
-	if (nfsd_file_check_writeback(nf)) {
-		trace_nfsd_file_gc_writeback(nf);
-		return LRU_SKIP;
-	}
-
-	/* If it was recently added to the list, skip it */
-	if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf->nf_flags)) {
-		trace_nfsd_file_gc_referenced(nf);
-		return LRU_ROTATE;
-	}
-
-	/*
-	 * Put the reference held on behalf of the LRU. If it wasn't the last
-	 * one, then just remove it from the LRU and ignore it.
-	 */
-	if (!refcount_dec_and_test(&nf->nf_ref)) {
-		trace_nfsd_file_gc_in_use(nf);
-		list_lru_isolate(lru, &nf->nf_lru);
-		return LRU_REMOVED;
-	}
-
-	/* Refcount went to zero. Unhash it and queue it to the dispose list */
-	nfsd_file_unhash(nf);
-	list_lru_isolate(lru, &nf->nf_lru);
-	list_add(&nf->nf_gc, head);
-	this_cpu_inc(nfsd_file_evictions);
-	trace_nfsd_file_gc_disposed(nf);
-	return LRU_REMOVED;
-}
-
-static void
-nfsd_file_gc(struct nfsd_fcache_disposal *l)
-{
-	unsigned long remaining = list_lru_count(&l->file_lru);
-	LIST_HEAD(dispose);
-	unsigned long ret;
-
-	while (remaining > 0) {
-		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
-
-		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
-				    &dispose, num_to_scan);
-		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
-		nfsd_file_dispose_list_delayed(&dispose);
-		remaining -= num_to_scan;
+		nfsd_file_release_list(&dispose);
 	}
 }
 
@@ -577,9 +546,20 @@ nfsd_file_gc_worker(struct work_struct *work)
 {
 	struct nfsd_fcache_disposal *l = container_of(
 		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
-	nfsd_file_gc(l);
-	if (list_lru_count(&l->file_lru))
+
+	spin_lock(&l->lock);
+	list_splice_init(&l->older, &l->freeme);
+	list_splice_init(&l->recent, &l->older);
+	/* We don't know how many were moved to 'freeme' and don't want
+	 * to waste time counting - guess a half.
+	 */
+	l->num_gc /= 2;
+	if (!list_empty(&l->freeme))
+		svc_wake_up(l->nn->nfsd_serv);
+	if (!list_empty(&l->older) || !list_empty(&l->recent))
 		nfsd_file_schedule_laundrette(l);
+	spin_unlock(&l->lock);
+
 }
 
 static unsigned long
@@ -587,22 +567,40 @@ nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
 {
 	struct nfsd_fcache_disposal *l = s->private_data;
 
-	return list_lru_count(&l->file_lru);
+	return l->num_gc;
 }
 
 static unsigned long
 nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
 {
 	struct nfsd_fcache_disposal *l = s->private_data;
+	struct nfsd_file *nf;
+	int scanned = 0;
+
+	spin_lock(&l->lock);
+	while (scanned < sc->nr_to_scan &&
+	       (nf = list_first_entry_or_null(&l->older,
+					      struct nfsd_file, nf_lru)) != NULL) {
+		list_del_init(&nf->nf_lru);
+		list_add_tail(&nf->nf_lru, &l->freeme);
+		if (l->num_gc > 0)
+			l->num_gc -= 1;
+		scanned += 1;
+	}
+	if (scanned > 0)
+		svc_wake_up(l->nn->nfsd_serv);
 
-	LIST_HEAD(dispose);
-	unsigned long ret;
+	trace_nfsd_file_shrinker_removed(scanned, l->num_gc);
 
-	ret = list_lru_shrink_walk(&l->file_lru, sc,
-				   nfsd_file_lru_cb, &dispose);
-	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
-	nfsd_file_dispose_list_delayed(&dispose);
-	return ret;
+	while (scanned < sc->nr_to_scan &&
+	       (nf = list_first_entry_or_null(&l->recent,
+					      struct nfsd_file, nf_lru)) != NULL) {
+		list_del_init(&nf->nf_lru);
+		list_add_tail(&nf->nf_lru, &l->older);
+		scanned += 1;
+	}
+	spin_unlock(&l->lock);
+	return scanned;
 }
 
 /**
@@ -615,7 +613,6 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
  */
 static void
 nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose)
-	__must_hold(RCU)
 {
 	int decrement = 1;
 
@@ -633,7 +630,7 @@ nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose)
 
 	/* If refcount goes to 0, then put on the dispose list */
 	if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
-		list_add(&nf->nf_gc, dispose);
+		list_add(&nf->nf_lru, dispose);
 		trace_nfsd_file_closing(nf);
 	}
 }
@@ -858,7 +855,10 @@ nfsd_alloc_fcache_disposal(void)
 		return NULL;
 	spin_lock_init(&l->lock);
 	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
+	INIT_LIST_HEAD(&l->recent);
+	INIT_LIST_HEAD(&l->older);
 	INIT_LIST_HEAD(&l->freeme);
+	l->num_gc = 0;
 	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
 	if (!l->file_shrinker) {
 		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
@@ -870,13 +870,6 @@ nfsd_alloc_fcache_disposal(void)
 	l->file_shrinker->seeks = 1;
 	l->file_shrinker->private_data = l;
 
-	if (list_lru_init(&l->file_lru)) {
-		pr_err("nfsd: failed to init nfsd_file_lru\n");
-		shrinker_free(l->file_shrinker);
-		kfree(l);
-		return NULL;
-	}
-
 	shrinker_register(l->file_shrinker);
 	return l;
 }
@@ -886,8 +879,12 @@ nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
 {
 	cancel_delayed_work_sync(&l->filecache_laundrette);
 	shrinker_free(l->file_shrinker);
-	list_lru_destroy(&l->file_lru);
-	nfsd_file_dispose_list(&l->freeme);
+	nfsd_file_release_list(&l->recent);
+	WARN_ON_ONCE(!list_empty(&l->recent));
+	nfsd_file_release_list(&l->older);
+	WARN_ON_ONCE(!list_empty(&l->older));
+	nfsd_file_release_list(&l->freeme);
+	WARN_ON_ONCE(!list_empty(&l->freeme));
 	kfree(l);
 }
 
@@ -906,6 +903,8 @@ nfsd_file_cache_start_net(struct net *net)
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
 
 	nn->fcache_disposal = nfsd_alloc_fcache_disposal();
+	if (nn->fcache_disposal)
+		nn->fcache_disposal->nn = nn;
 	return nn->fcache_disposal ? 0 : -ENOMEM;
 }
 
diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
index a09a851d510e..02059b6c5e9a 100644
--- a/fs/nfsd/filecache.h
+++ b/fs/nfsd/filecache.h
@@ -42,15 +42,13 @@ struct nfsd_file {
 	struct net		*nf_net;
 #define NFSD_FILE_HASHED	(0)
 #define NFSD_FILE_PENDING	(1)
-#define NFSD_FILE_REFERENCED	(2)
-#define NFSD_FILE_GC		(3)
+#define NFSD_FILE_GC		(2)
 	unsigned long		nf_flags;
 	refcount_t		nf_ref;
 	unsigned char		nf_may;
 
 	struct nfsd_file_mark	*nf_mark;
 	struct list_head	nf_lru;
-	struct list_head	nf_gc;
 	struct rcu_head		nf_rcu;
 	ktime_t			nf_birthtime;
 };
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index ad2c0c432d08..efa683541ed5 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -1038,7 +1038,6 @@ DEFINE_CLID_EVENT(confirmed_r);
 	__print_flags(val, "|",						\
 		{ 1 << NFSD_FILE_HASHED,	"HASHED" },		\
 		{ 1 << NFSD_FILE_PENDING,	"PENDING" },		\
-		{ 1 << NFSD_FILE_REFERENCED,	"REFERENCED" },		\
 		{ 1 << NFSD_FILE_GC,		"GC" })
 
 DECLARE_EVENT_CLASS(nfsd_file_class,
@@ -1314,9 +1313,7 @@ DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add);
 DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add_disposed);
 DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del);
 DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del_disposed);
-DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_in_use);
 DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_writeback);
-DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_referenced);
 DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_disposed);
 
 DECLARE_EVENT_CLASS(nfsd_file_lruwalk_class,
@@ -1345,7 +1342,6 @@ DEFINE_EVENT(nfsd_file_lruwalk_class, name,				\
 	),								\
 	TP_ARGS(removed, remaining))
 
-DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
 DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
 
 TRACE_EVENT(nfsd_file_close,
-- 
2.47.1

Re: [PATCH 3/4] nfsd: filecache: change garbage collection list management.

[Jeff Layton]

On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> The nfsd filecache currently uses  list_lru for tracking files recently
> used in NFSv3 requests which need to be "garbage collected" when they
> have becoming idle - unused for 2-4 seconds.
> 
> I do not believe list_lru is a good tool for this.  It does not allow the
> timeout which filecache requires so we have to add a timeout mechanism
> which holds the list_lru lock while the whole list is scanned looking for
> entries that haven't been recently accessed.  When the list is largish
> (even a few hundred) this can block new requests which need the lock to
> remove a file to access it.
> 
> This patch removes the list_lru and instead uses 2 simple linked lists.
> When a file is accessed it is removed from whichever list it is on,
> then added to the tail of the first list.  Every 2 seconds the second
> list is moved to the "freeme" list and the first list is moved to the
> second list.  This avoids any need to walk a list to find old entries.
> 
> Previously a file would be unhashed before being moved to the freeme
> list.  We don't do that any more.  The freeme list is much like the
> other two lists (recent and older) in that they all hold a reference to
> the file and the file is still hashed.  When the nfsd thread processes
> the freeme list it now uses the new nfsd_file_release_list() which uses
> nfsd_file_cond_queue() to unhash and drop the refcount.
> 
> We no longer have a precise count of the size of the lru (recent +
> older) as we don't know how big "older" is when it is moved to "freeme".
> However the shrinker can cope with an approximation.  So we keep a
> count of number in the lru and when "older" is moved to "freeme" we
> divide that count by 2.  When we remove anything from the lru we
> decrement that counter but ensure it never goes negative.  Naturally
> when we add to the lru we increase the counter.
> 
> For the filecache stats file, which assumes a global lru, we keep a
> separate counter which includes all files in all netns in recent or
> older or freeme.
> 
> We discard the nf_gc linkage in an nfsd_file and only use nf_lru.
> We discard NFSD_FILE_REFERENCED.
> 
> This patch drops the nfsd_file_gc_removed() trace point.  I couldn't
> think of useful information to provide.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 215 ++++++++++++++++++++++----------------------
>  fs/nfsd/filecache.h |   4 +-
>  fs/nfsd/trace.h     |   4 -
>  3 files changed, 108 insertions(+), 115 deletions(-)
> 

I like the basic approach! This is a better fit than list_lru.

> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 4f39f6632b35..552feba94f09 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -34,7 +34,6 @@
>  #include <linux/file.h>
>  #include <linux/pagemap.h>
>  #include <linux/sched.h>
> -#include <linux/list_lru.h>
>  #include <linux/fsnotify_backend.h>
>  #include <linux/fsnotify.h>
>  #include <linux/seq_file.h>
> @@ -63,10 +62,13 @@ static DEFINE_PER_CPU(unsigned long, nfsd_file_evictions);
>  
>  struct nfsd_fcache_disposal {
>  	spinlock_t lock;
> -	struct list_lru file_lru;
> -	struct list_head freeme;
> +	struct list_head recent; /* have been used in last 0-2 seconds */
> +	struct list_head older;	/* haven't been used in last 0-2 seconds */
> +	struct list_head freeme; /* ready to be discarded */
> +	unsigned long num_gc; /* Approximate size of recent plus older */
>  	struct delayed_work filecache_laundrette;
>  	struct shrinker *file_shrinker;
> +	struct nfsd_net *nn;
>  };
>  
>  static struct kmem_cache		*nfsd_file_slab;
> @@ -227,7 +229,6 @@ nfsd_file_alloc(struct net *net, struct inode *inode, unsigned char need,
>  
>  	this_cpu_inc(nfsd_file_allocations);
>  	INIT_LIST_HEAD(&nf->nf_lru);
> -	INIT_LIST_HEAD(&nf->nf_gc);
>  	nf->nf_birthtime = ktime_get();
>  	nf->nf_file = NULL;
>  	nf->nf_cred = get_current_cred();
> @@ -332,12 +333,16 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
>  	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> -	set_bit(NFSD_FILE_REFERENCED, &nf->nf_flags);
> -	if (list_lru_add_obj(&l->file_lru, &nf->nf_lru)) {
> +	spin_lock(&l->lock);
> +	if (list_empty(&nf->nf_lru)) {
> +		list_add_tail(&nf->nf_lru, &l->recent);
> +		l->num_gc += 1;
>  		atomic_long_inc(&nfsd_lru_total);
>  		trace_nfsd_file_lru_add(nf);
> +		spin_unlock(&l->lock);
>  		return true;
>  	}
> +	spin_unlock(&l->lock);
>  	return false;
>  }
>  
> @@ -346,11 +351,17 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
>  	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> -	if (list_lru_del_obj(&l->file_lru, &nf->nf_lru)) {
> +	spin_lock(&l->lock);
> +	if (!list_empty(&nf->nf_lru)) {
> +		list_del_init(&nf->nf_lru);
>  		atomic_long_dec(&nfsd_lru_total);
> +		if (l->num_gc > 0)
> +			l->num_gc -= 1;
>  		trace_nfsd_file_lru_del(nf);
> +		spin_unlock(&l->lock);
>  		return true;
>  	}
> +	spin_unlock(&l->lock);
>  	return false;
>  }
>  
> @@ -440,12 +451,26 @@ nfsd_file_dispose_list(struct list_head *dispose)
>  	struct nfsd_file *nf;
>  
>  	while (!list_empty(dispose)) {
> -		nf = list_first_entry(dispose, struct nfsd_file, nf_gc);
> -		list_del_init(&nf->nf_gc);
> +		nf = list_first_entry(dispose, struct nfsd_file, nf_lru);
> +		list_del_init(&nf->nf_lru);
>  		nfsd_file_free(nf);
>  	}
>  }
>  
> +static void
> +nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose);
> +
> +static void
> +nfsd_file_release_list(struct list_head *dispose)
> +{
> +	LIST_HEAD(dispose2);
> +	struct nfsd_file *nf, *nf2;
> +
> +	list_for_each_entry_safe(nf, nf2, dispose, nf_lru)
> +		nfsd_file_cond_queue(nf, &dispose2);
> +	nfsd_file_dispose_list(&dispose2);
> +}
> +
>  /**
>   * nfsd_file_dispose_list_delayed - move list of dead files to net's freeme list
>   * @dispose: list of nfsd_files to be disposed
> @@ -458,12 +483,12 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
>  {
>  	while(!list_empty(dispose)) {
>  		struct nfsd_file *nf = list_first_entry(dispose,
> -						struct nfsd_file, nf_gc);
> +						struct nfsd_file, nf_lru);
>  		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
>  		spin_lock(&l->lock);
> -		list_move_tail(&nf->nf_gc, &l->freeme);
> +		list_move_tail(&nf->nf_lru, &l->freeme);
>  		spin_unlock(&l->lock);
>  		svc_wake_up(nn->nfsd_serv);
>  	}
> @@ -487,88 +512,32 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
>  		int i;
>  
>  		spin_lock(&l->lock);
> -		for (i = 0; i < 8 && !list_empty(&l->freeme); i++)
> -			list_move(l->freeme.next, &dispose);

While you're in here, could you document why we only take 8 at a time?
Maybe even consider turning it into a named constant?

> +		for (i = 0; i < 8 && !list_empty(&l->freeme); i++) {
> +			struct nfsd_file *nf = list_first_entry(
> +				&l->freeme, struct nfsd_file, nf_lru);
> +
> +			/*
> +			 * Don't throw out files that are still
> +			 * undergoing I/O or that have uncleared errors
> +			 * pending.
> +			 */
> +			if (nfsd_file_check_writeback(nf)) {
> +				trace_nfsd_file_gc_writeback(nf);
> +				list_move(&nf->nf_lru, &l->recent);
> +				l->num_gc += 1;
> +			} else {
> +				trace_nfsd_file_gc_disposed(nf);
> +				list_move(&nf->nf_lru, &dispose);
> +				this_cpu_inc(nfsd_file_evictions);
> +			}
> +		}
>  		spin_unlock(&l->lock);
>  		if (!list_empty(&l->freeme))
>  			/* Wake up another thread to share the work
>  			 * *before* doing any actual disposing.
>  			 */
>  			svc_wake_up(nn->nfsd_serv);
> -		nfsd_file_dispose_list(&dispose);
> -	}
> -}
> -
> -/**
> - * nfsd_file_lru_cb - Examine an entry on the LRU list
> - * @item: LRU entry to examine
> - * @lru: controlling LRU
> - * @arg: dispose list
> - *
> - * Return values:
> - *   %LRU_REMOVED: @item was removed from the LRU
> - *   %LRU_ROTATE: @item is to be moved to the LRU tail
> - *   %LRU_SKIP: @item cannot be evicted
> - */
> -static enum lru_status
> -nfsd_file_lru_cb(struct list_head *item, struct list_lru_one *lru,
> -		 void *arg)
> -{
> -	struct list_head *head = arg;
> -	struct nfsd_file *nf = list_entry(item, struct nfsd_file, nf_lru);
> -
> -	/* We should only be dealing with GC entries here */
> -	WARN_ON_ONCE(!test_bit(NFSD_FILE_GC, &nf->nf_flags));
> -
> -	/*
> -	 * Don't throw out files that are still undergoing I/O or
> -	 * that have uncleared errors pending.
> -	 */
> -	if (nfsd_file_check_writeback(nf)) {
> -		trace_nfsd_file_gc_writeback(nf);
> -		return LRU_SKIP;
> -	}
> -
> -	/* If it was recently added to the list, skip it */
> -	if (test_and_clear_bit(NFSD_FILE_REFERENCED, &nf->nf_flags)) {
> -		trace_nfsd_file_gc_referenced(nf);
> -		return LRU_ROTATE;
> -	}
> -
> -	/*
> -	 * Put the reference held on behalf of the LRU. If it wasn't the last
> -	 * one, then just remove it from the LRU and ignore it.
> -	 */
> -	if (!refcount_dec_and_test(&nf->nf_ref)) {
> -		trace_nfsd_file_gc_in_use(nf);
> -		list_lru_isolate(lru, &nf->nf_lru);
> -		return LRU_REMOVED;
> -	}
> -
> -	/* Refcount went to zero. Unhash it and queue it to the dispose list */
> -	nfsd_file_unhash(nf);
> -	list_lru_isolate(lru, &nf->nf_lru);
> -	list_add(&nf->nf_gc, head);
> -	this_cpu_inc(nfsd_file_evictions);
> -	trace_nfsd_file_gc_disposed(nf);
> -	return LRU_REMOVED;
> -}
> -
> -static void
> -nfsd_file_gc(struct nfsd_fcache_disposal *l)
> -{
> -	unsigned long remaining = list_lru_count(&l->file_lru);
> -	LIST_HEAD(dispose);
> -	unsigned long ret;
> -
> -	while (remaining > 0) {
> -		unsigned long num_to_scan = min(remaining, NFSD_FILE_GC_BATCH);
> -
> -		ret = list_lru_walk(&l->file_lru, nfsd_file_lru_cb,
> -				    &dispose, num_to_scan);
> -		trace_nfsd_file_gc_removed(ret, list_lru_count(&l->file_lru));
> -		nfsd_file_dispose_list_delayed(&dispose);
> -		remaining -= num_to_scan;
> +		nfsd_file_release_list(&dispose);
>  	}
>  }
>  
> @@ -577,9 +546,20 @@ nfsd_file_gc_worker(struct work_struct *work)
>  {
>  	struct nfsd_fcache_disposal *l = container_of(
>  		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> -	nfsd_file_gc(l);
> -	if (list_lru_count(&l->file_lru))
> +
> +	spin_lock(&l->lock);
> +	list_splice_init(&l->older, &l->freeme);
> +	list_splice_init(&l->recent, &l->older);
> +	/* We don't know how many were moved to 'freeme' and don't want
> +	 * to waste time counting - guess a half.
> +	 */
> +	l->num_gc /= 2;

Given that you have to manipulate the lists under a spinlock, it
wouldn't be difficult or expensive to keep accurate counts. nfsd
workloads can be "spiky", so it seems like this method may be wildly
inaccurate at times.

> +	if (!list_empty(&l->freeme))
> +		svc_wake_up(l->nn->nfsd_serv);
> +	if (!list_empty(&l->older) || !list_empty(&l->recent))
>  		nfsd_file_schedule_laundrette(l);
> +	spin_unlock(&l->lock);
> +

nit: remove the extra newline above

>  }
>  
>  static unsigned long
> @@ -587,22 +567,40 @@ nfsd_file_lru_count(struct shrinker *s, struct shrink_control *sc)
>  {
>  	struct nfsd_fcache_disposal *l = s->private_data;
>  
> -	return list_lru_count(&l->file_lru);
> +	return l->num_gc;
>  }
>  
>  static unsigned long
>  nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>  {
>  	struct nfsd_fcache_disposal *l = s->private_data;
> +	struct nfsd_file *nf;
> +	int scanned = 0;
> +
> +	spin_lock(&l->lock);
> +	while (scanned < sc->nr_to_scan &&
> +	       (nf = list_first_entry_or_null(&l->older,
> +					      struct nfsd_file, nf_lru)) != NULL) {
> +		list_del_init(&nf->nf_lru);
> +		list_add_tail(&nf->nf_lru, &l->freeme);
> +		if (l->num_gc > 0)
> +			l->num_gc -= 1;
> +		scanned += 1;
> +	}
> +	if (scanned > 0)
> +		svc_wake_up(l->nn->nfsd_serv);
>  
> -	LIST_HEAD(dispose);
> -	unsigned long ret;
> +	trace_nfsd_file_shrinker_removed(scanned, l->num_gc);
>  
> -	ret = list_lru_shrink_walk(&l->file_lru, sc,
> -				   nfsd_file_lru_cb, &dispose);
> -	trace_nfsd_file_shrinker_removed(ret, list_lru_count(&l->file_lru));
> -	nfsd_file_dispose_list_delayed(&dispose);
> -	return ret;
> +	while (scanned < sc->nr_to_scan &&
> +	       (nf = list_first_entry_or_null(&l->recent,
> +					      struct nfsd_file, nf_lru)) != NULL) {
> +		list_del_init(&nf->nf_lru);
> +		list_add_tail(&nf->nf_lru, &l->older);
> +		scanned += 1;
> +	}
> +	spin_unlock(&l->lock);
> +	return scanned;
>  }
>  
>  /**
> @@ -615,7 +613,6 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>   */
>  static void
>  nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose)
> -	__must_hold(RCU)
>  {
>  	int decrement = 1;
>  
> @@ -633,7 +630,7 @@ nfsd_file_cond_queue(struct nfsd_file *nf, struct list_head *dispose)
>  
>  	/* If refcount goes to 0, then put on the dispose list */
>  	if (refcount_sub_and_test(decrement, &nf->nf_ref)) {
> -		list_add(&nf->nf_gc, dispose);
> +		list_add(&nf->nf_lru, dispose);
>  		trace_nfsd_file_closing(nf);
>  	}
>  }
> @@ -858,7 +855,10 @@ nfsd_alloc_fcache_disposal(void)
>  		return NULL;
>  	spin_lock_init(&l->lock);
>  	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> +	INIT_LIST_HEAD(&l->recent);
> +	INIT_LIST_HEAD(&l->older);
>  	INIT_LIST_HEAD(&l->freeme);
> +	l->num_gc = 0;
>  	l->file_shrinker = shrinker_alloc(0, "nfsd-filecache");
>  	if (!l->file_shrinker) {
>  		pr_err("nfsd: failed to allocate nfsd_file_shrinker\n");
> @@ -870,13 +870,6 @@ nfsd_alloc_fcache_disposal(void)
>  	l->file_shrinker->seeks = 1;
>  	l->file_shrinker->private_data = l;
>  
> -	if (list_lru_init(&l->file_lru)) {
> -		pr_err("nfsd: failed to init nfsd_file_lru\n");
> -		shrinker_free(l->file_shrinker);
> -		kfree(l);
> -		return NULL;
> -	}
> -
>  	shrinker_register(l->file_shrinker);
>  	return l;
>  }
> @@ -886,8 +879,12 @@ nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>  {
>  	cancel_delayed_work_sync(&l->filecache_laundrette);
>  	shrinker_free(l->file_shrinker);
> -	list_lru_destroy(&l->file_lru);
> -	nfsd_file_dispose_list(&l->freeme);
> +	nfsd_file_release_list(&l->recent);
> +	WARN_ON_ONCE(!list_empty(&l->recent));
> +	nfsd_file_release_list(&l->older);
> +	WARN_ON_ONCE(!list_empty(&l->older));
> +	nfsd_file_release_list(&l->freeme);
> +	WARN_ON_ONCE(!list_empty(&l->freeme));
>  	kfree(l);
>  }
>  
> @@ -906,6 +903,8 @@ nfsd_file_cache_start_net(struct net *net)
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
>  
>  	nn->fcache_disposal = nfsd_alloc_fcache_disposal();
> +	if (nn->fcache_disposal)
> +		nn->fcache_disposal->nn = nn;
>  	return nn->fcache_disposal ? 0 : -ENOMEM;
>  }
>  
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index a09a851d510e..02059b6c5e9a 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -42,15 +42,13 @@ struct nfsd_file {
>  	struct net		*nf_net;
>  #define NFSD_FILE_HASHED	(0)
>  #define NFSD_FILE_PENDING	(1)
> -#define NFSD_FILE_REFERENCED	(2)
> -#define NFSD_FILE_GC		(3)
> +#define NFSD_FILE_GC		(2)
>  	unsigned long		nf_flags;
>  	refcount_t		nf_ref;
>  	unsigned char		nf_may;
>  
>  	struct nfsd_file_mark	*nf_mark;
>  	struct list_head	nf_lru;
> -	struct list_head	nf_gc;
>  	struct rcu_head		nf_rcu;
>  	ktime_t			nf_birthtime;
>  };
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index ad2c0c432d08..efa683541ed5 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1038,7 +1038,6 @@ DEFINE_CLID_EVENT(confirmed_r);
>  	__print_flags(val, "|",						\
>  		{ 1 << NFSD_FILE_HASHED,	"HASHED" },		\
>  		{ 1 << NFSD_FILE_PENDING,	"PENDING" },		\
> -		{ 1 << NFSD_FILE_REFERENCED,	"REFERENCED" },		\
>  		{ 1 << NFSD_FILE_GC,		"GC" })
>  
>  DECLARE_EVENT_CLASS(nfsd_file_class,
> @@ -1314,9 +1313,7 @@ DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add);
>  DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_add_disposed);
>  DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del);
>  DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_lru_del_disposed);
> -DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_in_use);
>  DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_writeback);
> -DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_referenced);
>  DEFINE_NFSD_FILE_GC_EVENT(nfsd_file_gc_disposed);
>  
>  DECLARE_EVENT_CLASS(nfsd_file_lruwalk_class,
> @@ -1345,7 +1342,6 @@ DEFINE_EVENT(nfsd_file_lruwalk_class, name,				\
>  	),								\
>  	TP_ARGS(removed, remaining))
>  
> -DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
>  DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
>  
>  TRACE_EVENT(nfsd_file_close,



-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 3/4] nfsd: filecache: change garbage collection list management.

[NeilBrown]

On Thu, 23 Jan 2025, Jeff Layton wrote:
> On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> > @@ -487,88 +512,32 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
> >  		int i;
> >  
> >  		spin_lock(&l->lock);
> > -		for (i = 0; i < 8 && !list_empty(&l->freeme); i++)
> > -			list_move(l->freeme.next, &dispose);
> 
> While you're in here, could you document why we only take 8 at a time?
> Maybe even consider turning it into a named constant?

I've added a patch to do that.

> > @@ -577,9 +546,20 @@ nfsd_file_gc_worker(struct work_struct *work)
> >  {
> >  	struct nfsd_fcache_disposal *l = container_of(
> >  		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> > -	nfsd_file_gc(l);
> > -	if (list_lru_count(&l->file_lru))
> > +
> > +	spin_lock(&l->lock);
> > +	list_splice_init(&l->older, &l->freeme);
> > +	list_splice_init(&l->recent, &l->older);
> > +	/* We don't know how many were moved to 'freeme' and don't want
> > +	 * to waste time counting - guess a half.
> > +	 */
> > +	l->num_gc /= 2;
> 
> Given that you have to manipulate the lists under a spinlock, it
> wouldn't be difficult or expensive to keep accurate counts. nfsd
> workloads can be "spiky", so it seems like this method may be wildly
> inaccurate at times.

The only way I can think of to get an accurate count is to iterate the
list under the spin lock, and the cost of iterating long lists under a
spinlock is what started this whole exercise.

I could keep an accurate count of recent+older+freeme but giving that to
the shrinker could cause it to shrink too much as the objects on
"freeme" cannot be freed any faster.

Maybe when freeme is not empty I could give zero to the shrinker...

Any ideas?

NeilBrown

Re: [PATCH 3/4] nfsd: filecache: change garbage collection list management.

[Jeff Layton]

On Thu, 2025-01-23 at 08:14 +1100, NeilBrown wrote:
> On Thu, 23 Jan 2025, Jeff Layton wrote:
> > On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> > > @@ -487,88 +512,32 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
> > >  		int i;
> > >  
> > >  		spin_lock(&l->lock);
> > > -		for (i = 0; i < 8 && !list_empty(&l->freeme); i++)
> > > -			list_move(l->freeme.next, &dispose);
> > 
> > While you're in here, could you document why we only take 8 at a time?
> > Maybe even consider turning it into a named constant?
> 
> I've added a patch to do that.
> 
> > > @@ -577,9 +546,20 @@ nfsd_file_gc_worker(struct work_struct *work)
> > >  {
> > >  	struct nfsd_fcache_disposal *l = container_of(
> > >  		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> > > -	nfsd_file_gc(l);
> > > -	if (list_lru_count(&l->file_lru))
> > > +
> > > +	spin_lock(&l->lock);
> > > +	list_splice_init(&l->older, &l->freeme);
> > > +	list_splice_init(&l->recent, &l->older);
> > > +	/* We don't know how many were moved to 'freeme' and don't want
> > > +	 * to waste time counting - guess a half.
> > > +	 */
> > > +	l->num_gc /= 2;
> > 
> > Given that you have to manipulate the lists under a spinlock, it
> > wouldn't be difficult or expensive to keep accurate counts. nfsd
> > workloads can be "spiky", so it seems like this method may be wildly
> > inaccurate at times.
> 
> The only way I can think of to get an accurate count is to iterate the
> list under the spin lock, and the cost of iterating long lists under a
> spinlock is what started this whole exercise.
> 
> I could keep an accurate count of recent+older+freeme but giving that to
> the shrinker could cause it to shrink too much as the objects on
> "freeme" cannot be freed any faster.
> 
> Maybe when freeme is not empty I could give zero to the shrinker...
> 
> Any ideas?


I was thinking we could just increment a counter when adding it to the
list and transplant those counts as needed, but removals are a problem
since you don't necessarily know what list each entry resides on at any
given time, and you can't decrement the right one. It's probably
doable, but you're right that it's complex.

I'm OK with your estimate for now. If it turns out to be a problem
later, we can deal with it then.
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[NeilBrown]

garbage collection never sleeps and no longer walks a list so it runs
quickly only requiring a spinlock.

This means we don't need to use a workqueue, we can use a simple timer
instead.  This means it can run in "bh" context so we need to use "_bh"
locking.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/nfsd/filecache.c | 51 +++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 23 deletions(-)

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index 552feba94f09..ebde4e86cdee 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -66,7 +66,7 @@ struct nfsd_fcache_disposal {
 	struct list_head older;	/* haven't been used in last 0-2 seconds */
 	struct list_head freeme; /* ready to be discarded */
 	unsigned long num_gc; /* Approximate size of recent plus older */
-	struct delayed_work filecache_laundrette;
+	struct timer_list timer;
 	struct shrinker *file_shrinker;
 	struct nfsd_net *nn;
 };
@@ -115,8 +115,8 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
 static void
 nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
 {
-	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
-			   NFSD_LAUNDRETTE_DELAY);
+	if (!timer_pending(&l->timer))
+		mod_timer(&l->timer, jiffies + NFSD_LAUNDRETTE_DELAY);
 }
 
 static void
@@ -333,16 +333,16 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
 	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
-	spin_lock(&l->lock);
+	spin_lock_bh(&l->lock);
 	if (list_empty(&nf->nf_lru)) {
 		list_add_tail(&nf->nf_lru, &l->recent);
 		l->num_gc += 1;
 		atomic_long_inc(&nfsd_lru_total);
 		trace_nfsd_file_lru_add(nf);
-		spin_unlock(&l->lock);
+		spin_unlock_bh(&l->lock);
 		return true;
 	}
-	spin_unlock(&l->lock);
+	spin_unlock_bh(&l->lock);
 	return false;
 }
 
@@ -351,17 +351,17 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
 	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
-	spin_lock(&l->lock);
+	spin_lock_bh(&l->lock);
 	if (!list_empty(&nf->nf_lru)) {
 		list_del_init(&nf->nf_lru);
 		atomic_long_dec(&nfsd_lru_total);
 		if (l->num_gc > 0)
 			l->num_gc -= 1;
 		trace_nfsd_file_lru_del(nf);
-		spin_unlock(&l->lock);
+		spin_unlock_bh(&l->lock);
 		return true;
 	}
-	spin_unlock(&l->lock);
+	spin_unlock_bh(&l->lock);
 	return false;
 }
 
@@ -487,9 +487,9 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
 		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
 		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
 
-		spin_lock(&l->lock);
+		spin_lock_bh(&l->lock);
 		list_move_tail(&nf->nf_lru, &l->freeme);
-		spin_unlock(&l->lock);
+		spin_unlock_bh(&l->lock);
 		svc_wake_up(nn->nfsd_serv);
 	}
 }
@@ -511,7 +511,7 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
 		LIST_HEAD(dispose);
 		int i;
 
-		spin_lock(&l->lock);
+		spin_lock_bh(&l->lock);
 		for (i = 0; i < 8 && !list_empty(&l->freeme); i++) {
 			struct nfsd_file *nf = list_first_entry(
 				&l->freeme, struct nfsd_file, nf_lru);
@@ -531,7 +531,7 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
 				this_cpu_inc(nfsd_file_evictions);
 			}
 		}
-		spin_unlock(&l->lock);
+		spin_unlock_bh(&l->lock);
 		if (!list_empty(&l->freeme))
 			/* Wake up another thread to share the work
 			 * *before* doing any actual disposing.
@@ -542,12 +542,12 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
 }
 
 static void
-nfsd_file_gc_worker(struct work_struct *work)
+nfsd_file_gc_worker(struct timer_list *t)
 {
 	struct nfsd_fcache_disposal *l = container_of(
-		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
+		t, struct nfsd_fcache_disposal, timer);
 
-	spin_lock(&l->lock);
+	spin_lock_bh(&l->lock);
 	list_splice_init(&l->older, &l->freeme);
 	list_splice_init(&l->recent, &l->older);
 	/* We don't know how many were moved to 'freeme' and don't want
@@ -557,9 +557,8 @@ nfsd_file_gc_worker(struct work_struct *work)
 	if (!list_empty(&l->freeme))
 		svc_wake_up(l->nn->nfsd_serv);
 	if (!list_empty(&l->older) || !list_empty(&l->recent))
-		nfsd_file_schedule_laundrette(l);
-	spin_unlock(&l->lock);
-
+		mod_timer(t, jiffies + NFSD_LAUNDRETTE_DELAY);
+	spin_unlock_bh(&l->lock);
 }
 
 static unsigned long
@@ -577,7 +576,7 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
 	struct nfsd_file *nf;
 	int scanned = 0;
 
-	spin_lock(&l->lock);
+	spin_lock_bh(&l->lock);
 	while (scanned < sc->nr_to_scan &&
 	       (nf = list_first_entry_or_null(&l->older,
 					      struct nfsd_file, nf_lru)) != NULL) {
@@ -599,7 +598,9 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
 		list_add_tail(&nf->nf_lru, &l->older);
 		scanned += 1;
 	}
-	spin_unlock(&l->lock);
+	spin_unlock_bh(&l->lock);
+
+	trace_nfsd_file_shrinker_removed(scanned, l->num_gc);
 	return scanned;
 }
 
@@ -854,7 +855,9 @@ nfsd_alloc_fcache_disposal(void)
 	if (!l)
 		return NULL;
 	spin_lock_init(&l->lock);
-	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
+	timer_setup(&l->timer, nfsd_file_gc_worker, 0);
+	INIT_LIST_HEAD(&l->recent);
+	INIT_LIST_HEAD(&l->older);
 	INIT_LIST_HEAD(&l->recent);
 	INIT_LIST_HEAD(&l->older);
 	INIT_LIST_HEAD(&l->freeme);
@@ -871,13 +874,15 @@ nfsd_alloc_fcache_disposal(void)
 	l->file_shrinker->private_data = l;
 
 	shrinker_register(l->file_shrinker);
+
 	return l;
 }
 
 static void
 nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
 {
-	cancel_delayed_work_sync(&l->filecache_laundrette);
+	shrinker_free(l->file_shrinker);
+	del_timer_sync(&l->timer);
 	shrinker_free(l->file_shrinker);
 	nfsd_file_release_list(&l->recent);
 	WARN_ON_ONCE(!list_empty(&l->recent));
-- 
2.47.1

Re: [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[Jeff Layton]

On Wed, 2025-01-22 at 14:54 +1100, NeilBrown wrote:
> garbage collection never sleeps and no longer walks a list so it runs
> quickly only requiring a spinlock.
> 
> This means we don't need to use a workqueue, we can use a simple timer
> instead.  This means it can run in "bh" context so we need to use "_bh"
> locking.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 51 +++++++++++++++++++++++++--------------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 552feba94f09..ebde4e86cdee 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -66,7 +66,7 @@ struct nfsd_fcache_disposal {
>  	struct list_head older;	/* haven't been used in last 0-2 seconds */
>  	struct list_head freeme; /* ready to be discarded */
>  	unsigned long num_gc; /* Approximate size of recent plus older */
> -	struct delayed_work filecache_laundrette;
> +	struct timer_list timer;
>  	struct shrinker *file_shrinker;
>  	struct nfsd_net *nn;
>  };
> @@ -115,8 +115,8 @@ static const struct rhashtable_params nfsd_file_rhash_params = {
>  static void
>  nfsd_file_schedule_laundrette(struct nfsd_fcache_disposal *l)
>  {
> -	queue_delayed_work(system_unbound_wq, &l->filecache_laundrette,
> -			   NFSD_LAUNDRETTE_DELAY);
> +	if (!timer_pending(&l->timer))
> +		mod_timer(&l->timer, jiffies + NFSD_LAUNDRETTE_DELAY);
>  }
>  
>  static void
> @@ -333,16 +333,16 @@ static bool nfsd_file_lru_add(struct nfsd_file *nf)
>  	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> -	spin_lock(&l->lock);
> +	spin_lock_bh(&l->lock);
>  	if (list_empty(&nf->nf_lru)) {
>  		list_add_tail(&nf->nf_lru, &l->recent);
>  		l->num_gc += 1;
>  		atomic_long_inc(&nfsd_lru_total);
>  		trace_nfsd_file_lru_add(nf);
> -		spin_unlock(&l->lock);
> +		spin_unlock_bh(&l->lock);
>  		return true;
>  	}
> -	spin_unlock(&l->lock);
> +	spin_unlock_bh(&l->lock);
>  	return false;
>  }
>  
> @@ -351,17 +351,17 @@ static bool nfsd_file_lru_remove(struct nfsd_file *nf)
>  	struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> -	spin_lock(&l->lock);
> +	spin_lock_bh(&l->lock);
>  	if (!list_empty(&nf->nf_lru)) {
>  		list_del_init(&nf->nf_lru);
>  		atomic_long_dec(&nfsd_lru_total);
>  		if (l->num_gc > 0)
>  			l->num_gc -= 1;
>  		trace_nfsd_file_lru_del(nf);
> -		spin_unlock(&l->lock);
> +		spin_unlock_bh(&l->lock);
>  		return true;
>  	}
> -	spin_unlock(&l->lock);
> +	spin_unlock_bh(&l->lock);
>  	return false;
>  }
>  
> @@ -487,9 +487,9 @@ nfsd_file_dispose_list_delayed(struct list_head *dispose)
>  		struct nfsd_net *nn = net_generic(nf->nf_net, nfsd_net_id);
>  		struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> -		spin_lock(&l->lock);
> +		spin_lock_bh(&l->lock);
>  		list_move_tail(&nf->nf_lru, &l->freeme);
> -		spin_unlock(&l->lock);
> +		spin_unlock_bh(&l->lock);
>  		svc_wake_up(nn->nfsd_serv);
>  	}
>  }
> @@ -511,7 +511,7 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
>  		LIST_HEAD(dispose);
>  		int i;
>  
> -		spin_lock(&l->lock);
> +		spin_lock_bh(&l->lock);
>  		for (i = 0; i < 8 && !list_empty(&l->freeme); i++) {
>  			struct nfsd_file *nf = list_first_entry(
>  				&l->freeme, struct nfsd_file, nf_lru);
> @@ -531,7 +531,7 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
>  				this_cpu_inc(nfsd_file_evictions);
>  			}
>  		}
> -		spin_unlock(&l->lock);
> +		spin_unlock_bh(&l->lock);
>  		if (!list_empty(&l->freeme))
>  			/* Wake up another thread to share the work
>  			 * *before* doing any actual disposing.
> @@ -542,12 +542,12 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
>  }
>  
>  static void
> -nfsd_file_gc_worker(struct work_struct *work)
> +nfsd_file_gc_worker(struct timer_list *t)
>  {
>  	struct nfsd_fcache_disposal *l = container_of(
> -		work, struct nfsd_fcache_disposal, filecache_laundrette.work);
> +		t, struct nfsd_fcache_disposal, timer);
>  
> -	spin_lock(&l->lock);
> +	spin_lock_bh(&l->lock);
>  	list_splice_init(&l->older, &l->freeme);
>  	list_splice_init(&l->recent, &l->older);
>  	/* We don't know how many were moved to 'freeme' and don't want
> @@ -557,9 +557,8 @@ nfsd_file_gc_worker(struct work_struct *work)
>  	if (!list_empty(&l->freeme))
>  		svc_wake_up(l->nn->nfsd_serv);
>  	if (!list_empty(&l->older) || !list_empty(&l->recent))
> -		nfsd_file_schedule_laundrette(l);
> -	spin_unlock(&l->lock);
> -
> +		mod_timer(t, jiffies + NFSD_LAUNDRETTE_DELAY);
> +	spin_unlock_bh(&l->lock);
>  }
>  
>  static unsigned long
> @@ -577,7 +576,7 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>  	struct nfsd_file *nf;
>  	int scanned = 0;
>  
> -	spin_lock(&l->lock);
> +	spin_lock_bh(&l->lock);
>  	while (scanned < sc->nr_to_scan &&
>  	       (nf = list_first_entry_or_null(&l->older,
>  					      struct nfsd_file, nf_lru)) != NULL) {
> @@ -599,7 +598,9 @@ nfsd_file_lru_scan(struct shrinker *s, struct shrink_control *sc)
>  		list_add_tail(&nf->nf_lru, &l->older);
>  		scanned += 1;
>  	}
> -	spin_unlock(&l->lock);
> +	spin_unlock_bh(&l->lock);
> +
> +	trace_nfsd_file_shrinker_removed(scanned, l->num_gc);
>  	return scanned;
>  }
>  
> @@ -854,7 +855,9 @@ nfsd_alloc_fcache_disposal(void)
>  	if (!l)
>  		return NULL;
>  	spin_lock_init(&l->lock);
> -	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> +	timer_setup(&l->timer, nfsd_file_gc_worker, 0);
> +	INIT_LIST_HEAD(&l->recent);
> +	INIT_LIST_HEAD(&l->older);
>  	INIT_LIST_HEAD(&l->recent);
>  	INIT_LIST_HEAD(&l->older);

No need to do the list initializations twice. ^^^

>  	INIT_LIST_HEAD(&l->freeme);
> @@ -871,13 +874,15 @@ nfsd_alloc_fcache_disposal(void)
>  	l->file_shrinker->private_data = l;
>  
>  	shrinker_register(l->file_shrinker);
> +
>  	return l;
>  }
>  
>  static void
>  nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>  {
> -	cancel_delayed_work_sync(&l->filecache_laundrette);
> +	shrinker_free(l->file_shrinker);
> +	del_timer_sync(&l->timer);
>  	shrinker_free(l->file_shrinker);
>  	nfsd_file_release_list(&l->recent);
>  	WARN_ON_ONCE(!list_empty(&l->recent));

It does seem like this is lightweight enough now that we can do the GC
in interrupt context. I'm not certain that's best for latency, but it's
worth experimenting.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[NeilBrown]

On Thu, 23 Jan 2025, Jeff Layton wrote:

> > @@ -854,7 +855,9 @@ nfsd_alloc_fcache_disposal(void)
> >  	if (!l)
> >  		return NULL;
> >  	spin_lock_init(&l->lock);
> > -	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> > +	timer_setup(&l->timer, nfsd_file_gc_worker, 0);
> > +	INIT_LIST_HEAD(&l->recent);
> > +	INIT_LIST_HEAD(&l->older);
> >  	INIT_LIST_HEAD(&l->recent);
> >  	INIT_LIST_HEAD(&l->older);
> 
> No need to do the list initializations twice. ^^^

Thanks.  I fixed up a few other merge-errors too.

> 
> It does seem like this is lightweight enough now that we can do the GC
> in interrupt context. I'm not certain that's best for latency, but it's
> worth experimenting.

What sort of latency are you thinking of?  By avoiding a scheduler
switch into the workqueue task we should be reducing overhead.
In the old code a timer would wake a thread which would need to be
scheduled to do the work.  In the new thread and identical time will do
the work directly.

Thanks,
NeilBrown


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

Re: [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[Jeff Layton]

On Thu, 2025-01-23 at 07:39 +1100, NeilBrown wrote:
> On Thu, 23 Jan 2025, Jeff Layton wrote:
> 
> > > @@ -854,7 +855,9 @@ nfsd_alloc_fcache_disposal(void)
> > >  	if (!l)
> > >  		return NULL;
> > >  	spin_lock_init(&l->lock);
> > > -	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> > > +	timer_setup(&l->timer, nfsd_file_gc_worker, 0);
> > > +	INIT_LIST_HEAD(&l->recent);
> > > +	INIT_LIST_HEAD(&l->older);
> > >  	INIT_LIST_HEAD(&l->recent);
> > >  	INIT_LIST_HEAD(&l->older);
> > 
> > No need to do the list initializations twice. ^^^
> 
> Thanks.  I fixed up a few other merge-errors too.
> 
> > 
> > It does seem like this is lightweight enough now that we can do the GC
> > in interrupt context. I'm not certain that's best for latency, but it's
> > worth experimenting.
> 
> What sort of latency are you thinking of?  By avoiding a scheduler
> switch into the workqueue task we should be reducing overhead.
> In the old code a timer would wake a thread which would need to be
> scheduled to do the work.  In the new thread and identical time will do
> the work directly.
> 
> 

Anytime we have to take *_bh locks, we block interrupts. In this case,
I think you're right that that's probably the lesser evil, but we will
be taking these locks somewhat frequently. It's certainly worth
starting here though, and only offloading to a workqueue if that proves
to be a problem.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

[NeilBrown]

On Thu, 23 Jan 2025, Jeff Layton wrote:
> On Thu, 2025-01-23 at 07:39 +1100, NeilBrown wrote:
> > On Thu, 23 Jan 2025, Jeff Layton wrote:
> > 
> > > > @@ -854,7 +855,9 @@ nfsd_alloc_fcache_disposal(void)
> > > >  	if (!l)
> > > >  		return NULL;
> > > >  	spin_lock_init(&l->lock);
> > > > -	INIT_DELAYED_WORK(&l->filecache_laundrette, nfsd_file_gc_worker);
> > > > +	timer_setup(&l->timer, nfsd_file_gc_worker, 0);
> > > > +	INIT_LIST_HEAD(&l->recent);
> > > > +	INIT_LIST_HEAD(&l->older);
> > > >  	INIT_LIST_HEAD(&l->recent);
> > > >  	INIT_LIST_HEAD(&l->older);
> > > 
> > > No need to do the list initializations twice. ^^^
> > 
> > Thanks.  I fixed up a few other merge-errors too.
> > 
> > > 
> > > It does seem like this is lightweight enough now that we can do the GC
> > > in interrupt context. I'm not certain that's best for latency, but it's
> > > worth experimenting.
> > 
> > What sort of latency are you thinking of?  By avoiding a scheduler
> > switch into the workqueue task we should be reducing overhead.
> > In the old code a timer would wake a thread which would need to be
> > scheduled to do the work.  In the new thread and identical time will do
> > the work directly.
> > 
> > 
> 
> Anytime we have to take *_bh locks, we block interrupts. In this case,
> I think you're right that that's probably the lesser evil, but we will
> be taking these locks somewhat frequently. It's certainly worth
> starting here though, and only offloading to a workqueue if that proves
> to be a problem.

We only block soft-interrupts, not hard interrupts.  So yes it could
cause some latency for softirqs.
If we really cared we could spin_try_lock in the timer and if that
fails, don't bother but reschedule the timer for a shorter timeout.

But I agree that we don't need to fix until we see a problem.

Thanks,
NeilBrown

Re: [PATCH 0/4] nfsd: filecache: change garbage collect lists

[Chuck Lever]

On 1/21/25 10:54 PM, NeilBrown wrote:
> 
> The nfsd filecache currently uses  list_lru for tracking files recently
> used in NFSv3 requests which need to be "garbage collected" when they
> have becoming idle - unused for 2-4 seconds.
> 
> I do not believe list_lru is a good tool for this.  It does no allow the
> timeout which filecache requires so we have to add a timeout mechanism
> which holds the list_lru for while the whole list is scanned looking for
> entries that haven't been recently accessed.  When the list is largish
> (even a few hundred) this can block new requests which need the lock to
> remove a file to access it.
> 
> This patch removes the list_lru and instead uses 2 simple linked lists.
> When a file is accessed it is removed from whichever list it is one,
> then added to the tail of the first list.  Every 2 seconds the second
> list is moved to the "freeme" list and the first list is moved to the
> second list.  This avoids any need to walk a list to find old entries.
> 
> These lists are per-netns rather than global as the freeme list is
> per-netns as the actual freeing is done in nfsd threads which are
> per-netns.
> 
> This should not be applied until we resolve how to handle the
> race-detection code in nfsd_file_put().  However I'm posting it now to
> get any feedback so that a final version can be posted as soon as that
> issue is resolved.
> 
> Thanks,
> NeilBrown
> 
> 
>   [PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in
>   [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and
>   [PATCH 3/4] nfsd: filecache: change garbage collection list
>   [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.

Hi Neil -

I would like Dave Chinner to chime in on this approach. When you
resend, please Cc: him. Thanks!


-- 
Chuck Lever

Re: [PATCH 0/4] nfsd: filecache: change garbage collect lists

[NeilBrown]

On Thu, 23 Jan 2025, Chuck Lever wrote:
> On 1/21/25 10:54 PM, NeilBrown wrote:
> > 
> > The nfsd filecache currently uses  list_lru for tracking files recently
> > used in NFSv3 requests which need to be "garbage collected" when they
> > have becoming idle - unused for 2-4 seconds.
> > 
> > I do not believe list_lru is a good tool for this.  It does no allow the
> > timeout which filecache requires so we have to add a timeout mechanism
> > which holds the list_lru for while the whole list is scanned looking for
> > entries that haven't been recently accessed.  When the list is largish
> > (even a few hundred) this can block new requests which need the lock to
> > remove a file to access it.
> > 
> > This patch removes the list_lru and instead uses 2 simple linked lists.
> > When a file is accessed it is removed from whichever list it is one,
> > then added to the tail of the first list.  Every 2 seconds the second
> > list is moved to the "freeme" list and the first list is moved to the
> > second list.  This avoids any need to walk a list to find old entries.
> > 
> > These lists are per-netns rather than global as the freeme list is
> > per-netns as the actual freeing is done in nfsd threads which are
> > per-netns.
> > 
> > This should not be applied until we resolve how to handle the
> > race-detection code in nfsd_file_put().  However I'm posting it now to
> > get any feedback so that a final version can be posted as soon as that
> > issue is resolved.
> > 
> > Thanks,
> > NeilBrown
> > 
> > 
> >   [PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in
> >   [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and
> >   [PATCH 3/4] nfsd: filecache: change garbage collection list
> >   [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.
> 
> Hi Neil -
> 
> I would like Dave Chinner to chime in on this approach. When you
> resend, please Cc: him. Thanks!

Sure, I can do that.  But why Dave in particular?
I would like to add a comment to the cover letter explaining to Dave
what he was added to see and I don't know what to say.

Thanks,
NeilBrown

Re: [PATCH 0/4] nfsd: filecache: change garbage collect lists

[Chuck Lever]

On 1/22/25 5:16 PM, NeilBrown wrote:
> On Thu, 23 Jan 2025, Chuck Lever wrote:
>> On 1/21/25 10:54 PM, NeilBrown wrote:
>>>
>>> The nfsd filecache currently uses  list_lru for tracking files recently
>>> used in NFSv3 requests which need to be "garbage collected" when they
>>> have becoming idle - unused for 2-4 seconds.
>>>
>>> I do not believe list_lru is a good tool for this.  It does no allow the
>>> timeout which filecache requires so we have to add a timeout mechanism
>>> which holds the list_lru for while the whole list is scanned looking for
>>> entries that haven't been recently accessed.  When the list is largish
>>> (even a few hundred) this can block new requests which need the lock to
>>> remove a file to access it.
>>>
>>> This patch removes the list_lru and instead uses 2 simple linked lists.
>>> When a file is accessed it is removed from whichever list it is one,
>>> then added to the tail of the first list.  Every 2 seconds the second
>>> list is moved to the "freeme" list and the first list is moved to the
>>> second list.  This avoids any need to walk a list to find old entries.
>>>
>>> These lists are per-netns rather than global as the freeme list is
>>> per-netns as the actual freeing is done in nfsd threads which are
>>> per-netns.
>>>
>>> This should not be applied until we resolve how to handle the
>>> race-detection code in nfsd_file_put().  However I'm posting it now to
>>> get any feedback so that a final version can be posted as soon as that
>>> issue is resolved.
>>>
>>> Thanks,
>>> NeilBrown
>>>
>>>
>>>    [PATCH 1/4] nfsd: filecache: use nfsd_file_dispose_list() in
>>>    [PATCH 2/4] nfsd: filecache: move globals nfsd_file_lru and
>>>    [PATCH 3/4] nfsd: filecache: change garbage collection list
>>>    [PATCH 4/4] nfsd: filecache: change garbage collection to a timer.
>>
>> Hi Neil -
>>
>> I would like Dave Chinner to chime in on this approach. When you
>> resend, please Cc: him. Thanks!
> 
> Sure, I can do that.  But why Dave in particular?
> I would like to add a comment to the cover letter explaining to Dave
> what he was added to see and I don't know what to say.

Dave helped me with edead3a55804 ("NFSD: Fix the filecache LRU
shrinker") and a few other related fixes, and he is the original
author of the list_lru facility (see a38e40824844 ("list: add a new LRU
list type"). He might have additional insight on whether the filecache's
use of list_lru is salvageable or how a replacement of that mechanism
should work.


-- 
Chuck Lever