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
