Re: [PATCH 1/3] xfs: don't keep a reference for buffers on the LRU

[Brian Foster <bfoster@redhat.com>]

On Tue, Jan 27, 2026 at 06:20:50AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 26, 2026 at 02:18:42PM -0500, Brian Foster wrote:
> > > +	/*
> > > +	 * If the buffer is in use, remove it from the LRU for now as we can't
> > > +	 * free it.  It will be added to the LRU again when the reference count
> > > +	 * hits zero.
> > > +	 */
> > > +	if (bp->b_hold > 0) {
> > > +		list_lru_isolate(lru, &bp->b_lru);
> > > +		spin_unlock(&bp->b_lock);
> > > +		return LRU_REMOVED;
> > > +	}
> > > +
> > 
> > Sorry I missed this on my first look at this, but I don't think I quite
> > realized why this was here. This looks like a subtle change in behavior
> > where a buffer that makes it onto the LRU and then is subsequently held
> > can no longer be cycled off the LRU by background shrinker activity.
> > Instead, we drop the buffer off the LRU entirely where it will no longer
> > be visible from ongoing shrinker activity.
> 
> Yes.
> 
> > AFAICT the reason for this is we no longer support the ability for the
> > shrinker to drop the LRU b_hold ref to indicate a buffer is fully cycled
> > out and can go direct to freeing when the current b_hold lifecycle ends.
> > Am I following that correctly?
> 
> Yes.
> 
> > If so, that doesn't necessarily seem like a showstopper as I'm not sure
> > realistically a significant amount of memory would be caught up like
> > this in practice, even under significant pressure. But regardless, why
> > not do this preventative LRU removal only after the lru ref count is
> > fully depleted? Wouldn't that more accurately preserve existing
> > behavior, or am I missing something?
> 
> It would more closely resemble the current behavior, which seems wrong
> and an artifact of the current reference counting.  A buffer that is
> in use really should not be counted down on the LRU, which really is
> for unused buffers.  So starting the countdown only once the buffer
> is unused is the right thing to do.  I should have explained this
> much better, though.
> 

Ok, but right or wrong it's been that way for quite some time (forever?)
and this sort of change probably shouldn't be buried implicitly in this
patch. For one, the LRU refcounting behavior doesn't depend on this work
because we could have always done something like skip the decrement and
rotate held buffers, but nobody has ever proposed such a change to my
knowledge. Also, ISTM this patch could fairly easily preserve existing
behavior by decrementing lru ref and deferring list removal to when the
lru ref drops to zero but the buffer is still held.

IOW, I'm not arguing for or against a change in buffer lifetime behavior
here, just that it should probably be done separately with some more
careful analysis. The secondary advantage is that if this behavior does
somehow uncover something problematic, we can bisect/revert back to
historical lifetime behavior without having to walk back these
functional changes.

Brian