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

[Jeff Layton <jlayton@kernel.org>]

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>