Re: [PATCH] xfs: remove xfs_buf_cache.bc_lock

[Dave Chinner <david@fromorbit.com>]

On Tue, Jan 28, 2025 at 06:06:14AM +0100, Christoph Hellwig wrote:
> On Tue, Jan 28, 2025 at 07:19:11AM +1100, Dave Chinner wrote:
> > Ok, so now we can get racing inserts, which means this can find
> > the buffer that has just been inserted by another thread in this
> > same function. Or, indeed, and xfs_buf_lookup() call.
> 
> Yes.
> 
> > What prevents
> > those racing tasks from using this buffer before the task that
> > inserted it can use it?
> > 
> > I think that the the buffer lock being initialised to "held" and
> > b_hold being initialised to 1 make this all work correctly,
> 
> Exactly, the buffer is inserted with the b_sema held and b_hold
> initializes 1, aka locked and held.
> 
> > but
> > comments that explicitly spell out why RCU inserts are safe
> > (both in xfs_buf_alloc() for the init values and here) would be
> > appreciated.
> 
> Sure.

Thanks.

> > >  struct xfs_buf_cache {
> > > -	spinlock_t		bc_lock;
> > >  	struct rhashtable	bc_hash;
> > >  };
> > 
> > At this point, the struct xfs_buf_cache structure can go away,
> > right?  (separate patch and all that...)
> 
> Yes.  And in fact I think the per-pag hash should also go away, as with
> the per-bucket locking there is no point in it.  I've had this patch in
> my testing runs for a while, which I think is where we should be
> going:
> 
> http://git.infradead.org/?p=users/hch/xfs.git;a=commitdiff;h=890cd2cd255710ee5d3408bc60792b9cdad3adfb

*nod*

Code seems reasonable, but it'll need some benchmarking and
scalability analysis before merging...

-Dave.
-- 
Dave Chinner
david@fromorbit.com