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

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2025-01-27 at 12:20 +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.
> 
> Rather than taking the lock in the timer callback, which would require
> using _bh locking, simply test a flag and wake an nfsd thread.  That
> thread checks the flag and ages the lists when needed.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/nfsd/filecache.c | 43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 7264faa57280..eb95a53f806f 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -67,10 +67,12 @@ 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 */

Dang, adding flags would push this into 
> -	struct delayed_work filecache_laundrette;
> +	struct timer_list timer;
>  	struct shrinker *file_shrinker;
>  	struct nfsd_net *nn;
> +	unsigned long flags;
>  };
> +#define NFSD_FCACHE_DO_AGE	BIT(0)	/* time to age the lists */
>  
>  static struct kmem_cache		*nfsd_file_slab;
>  static struct kmem_cache		*nfsd_file_mark_slab;
> @@ -115,8 +117,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
> @@ -521,6 +523,19 @@ void nfsd_file_net_dispose(struct nfsd_net *nn)
>  {
>  	struct nfsd_fcache_disposal *l = nn->fcache_disposal;
>  
> +	if (test_and_clear_bit(NFSD_FCACHE_DO_AGE, &l->flags)) {
> +		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.  This is only used
> +		 * for the shrinker which doesn't need complete precision.
> +		 */
> +		l->num_gc /= 2;
> +		if (!list_empty(&l->older) || !list_empty(&l->recent))
> +			mod_timer(&l->timer, jiffies + NFSD_LAUNDRETTE_DELAY);
> +		spin_unlock(&l->lock);
> +	}
>  	if (!list_empty(&l->freeme)) {
>  		LIST_HEAD(dispose);
>  		int i;
> @@ -557,23 +572,13 @@ 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);
> -	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);
> +	set_bit(NFSD_FCACHE_DO_AGE, &l->flags);
> +	svc_wake_up(l->nn->nfsd_serv);

Disregard my earlier comment about the cacheline. It still wouldn't
hurt to do, but since you're not actually taking the lock inside the
timer callback in this version, it's not as big a deal.

This seems better.

>  }
>  
>  static unsigned long
> @@ -868,7 +873,7 @@ 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->freeme);
> @@ -891,7 +896,7 @@ nfsd_alloc_fcache_disposal(void)
>  static void
>  nfsd_free_fcache_disposal(struct nfsd_fcache_disposal *l)
>  {
> -	cancel_delayed_work_sync(&l->filecache_laundrette);
> +	del_timer_sync(&l->timer);
>  	shrinker_free(l->file_shrinker);
>  	nfsd_file_release_list(&l->recent);
>  	WARN_ON_ONCE(!list_empty(&l->recent));

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