Re: [PATCH 2/4] xfs: use a lockref for the buffer reference count

[Christoph Hellwig <hch@lst.de>]

On Wed, Mar 18, 2026 at 08:53:13AM +1100, Dave Chinner wrote:
> > -	spin_lock(&bp->b_lock);
> > -	bp->b_hold++;
> > -	spin_unlock(&bp->b_lock);
> > +	lockref_get(&bp->b_lockref);
> >  }
> 
> Can we make xfs_buf_hold a static inline in xfs_buf.h now? It is
> called quite frequently and it's now just a one line wrapper
> around the lockref code...

Well, that plus the trace point.  For which we'd need to pull in
xfs_trace.h into xfs_buf.h, which I've been trying to avoid.

> 
> > @@ -1564,10 +1547,10 @@ xfs_buftarg_isolate(
> >  	struct list_head	*dispose = arg;
> >  
> >  	/*
> > -	 * We are inverting the lru lock vs bp->b_lock order here, so use a
> > -	 * trylock. If we fail to get the lock, just skip the buffer.
> > +	 * We are inverting the lru lock vs bp->b_lockref.lock order here, so
> > +	 * use a trylock. If we fail to get the lock, just skip the buffer.
> >  	 */
> > -	if (!spin_trylock(&bp->b_lock))
> > +	if (!spin_trylock(&bp->b_lockref.lock))
> >  		return LRU_SKIP;
> >  
> >  	/*
> 
> You modify this comment and whitespace in the previous patch without
> any code change, and then do it again in this patch with a code
> change. Can you collapse that just into a single change in this
> patch with the code change?

Sure, done.

> >  	xfs_daddr_t		b_rhash_key;	/* buffer cache index */
> >  	int			b_length;	/* size of buffer in BBs */
> > -	int			b_hold;		/* reference count */
> > +	struct lockref		b_lockref;	/* refcount + lock */
> >  	atomic_t		b_lru_ref;	/* lru reclaim ref count */
> 
> IIUC, this adds a 4 byte hole to the structure - b_lockref is an aligned
> u64...

Yes.

> The first cacheline of the xfs_buf is explicitly packed
> packed with the fields that a hash lookup needs to
> minimise cacheline misses during lookup, so we really don't want to
> screw that up by wasting space in the first cacheline.
> 
> Indeed, seeing as we've simplified the structure of the cache index
> over time and are moving back to a global cache index, we probably
> need to take another look at exactly where the cacheline boundary
> sits and exactly what the lockless lookup fast path accesses. i.e.
> make sure that lookups only access the first cacheline, and there is
> nothing on that first cacheline that any other concurrent buffer
> access might contend on (e.g. lru list scanning, wait queues, pure
> read-only accesses, etc.)

With this change, the first 64-byte cache line ends after b_flags.
So even as-is it avoid having b_sema spread cache lines (all according
to b_sema).  So for now I'd like to avoid reordering things in this
series, even if I agree with the sentiment that a good analysis
here might be in order.  Maybe wait until willy's series to shrink
the semaphore lands, which will changes thing a bit again.