Re: [PATCH 02/12] xfs: consolidate buffer locking in xfs_buf_get_map

[Brian Foster <bfoster@redhat.com>]

On Fri, Jul 17, 2026 at 10:59:50AM +0200, Christoph Hellwig wrote:
> On Thu, Jul 16, 2026 at 09:12:50AM -0400, Brian Foster wrote:
> > > -	/*
> > > -	 * A new buffer is held and locked by the owner.  This ensures that the
> > > -	 * buffer is owned by the caller and racing RCU lookups right after
> > > -	 * inserting into the hash table are safe (and will have to wait for
> > > -	 * the unlock to do anything non-trivial).
> > > -	 */
> > >  	lockref_init(&bp->b_lockref);
> > > -	sema_init(&bp->b_sema, 0); /* held, no waiters */
> > > +	sema_init(&bp->b_sema, 1); /* unlocked */
> > 
> > Maybe I'm missing something, but I thought the point of the locked init
> > was to ensure that the allocator owns the buffer and can't end up
> > blocking on the lock with a racing lookup. I.e., the buffer is locked
> > before it becomes available for external lookup. Do we plan to do that
> > another way, or are we just dropping that assumption?
> 
> The comment has been stale for a while.  b_sema is not involved in
> lookup synchronization in any way.  Only the lockref is needed to actually
> get a stable buffer reference.  Once we have a stable buffer reference,
> b_sema is used to protect again multiple readers trying to actually
> read the data (for xfs_buf_read*), but we don't care if that is done
> by the thread that allocated the buffer or not.
> 

Hm, Ok. I read the comment as wanting to guarantee that the allocator
gets first crack at the buffer lock, but looking at it again it says it
ensures "racing RCU lookups right after inserting into the hash table
are safe," and indeed that seems to be a little different.

I see that you're intentionally separating out the lookup from the buf
data locking, but it does still look like we're making a potential
functional tradeoff here by changing the insertion side locking. Note
that I don't object or know that it matters in any way, I'd just suggest
to add a note about this in the commit log in case somebody has to refer
back to this in the future.

Otherwise I read through the rest of the series and modulo a nit on
patch 10, it all looks reasonable to me. For the series:

Reviewed-by: Brian Foster <bfoster@redhat.com>