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@xxxxxxx>
> ---
> 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@xxxxxxxxxx>
