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

[Jeff Layton <jlayton@kernel.org>]

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>