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

[Dave Chinner <dgc@kernel.org>]

On Tue, Mar 17, 2026 at 02:40:53PM +0100, Christoph Hellwig wrote:
> The lockref structure allows incrementing/decrementing counters like
> an atomic_t for the fast path, while still allowing complex slow path
> operations as if the counter was protected by a lock.  The only slow
> path operations that actually need to take the lock are the final
> put, LRU evictions and marking a buffer stale.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

Seems straight forward enough, even if it is more verbose...

> @@ -853,16 +837,14 @@ xfs_buf_hold(
>  {
>  	trace_xfs_buf_hold(bp, _RET_IP_);
>  
> -	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...

> @@ -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?

> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index e7324d58bd96..3a1d066e1c13 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -14,6 +14,7 @@
>  #include <linux/dax.h>
>  #include <linux/uio.h>
>  #include <linux/list_lru.h>
> +#include <linux/lockref.h>
>  
>  extern struct kmem_cache *xfs_buf_cache;
>  
> @@ -154,7 +155,7 @@ struct xfs_buf {
>  
>  	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...

>  	xfs_buf_flags_t		b_flags;	/* status flags */

And we end up with another 4 byte hole here because...

>  	struct semaphore	b_sema;		/* semaphore for lockables */

the struct semaphore also ends up being u64 aligned...

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.)

Otherwise, the code change looks ok.

-Dave.
-- 
Dave Chinner
dgc@kernel.org