Re: [PATCH] bcachefs: Change bucket_lock() to use bit_spin_lock()

[Brian Foster <bfoster@redhat.com>]

On Thu, Sep 14, 2023 at 03:47:06PM -0400, Kent Overstreet wrote:
> On Thu, Sep 14, 2023 at 08:56:47AM -0400, Brian Foster wrote:
> > > +/*
> > > + * Ugly hack alert:
> > > + *
> > > + * We need to cram a spinlock in a single byte, because that's what we have left
> > > + * in struct bucket, and we care about the size of these - during fsck, we need
> > > + * in memory state for every single bucket on every device.
> > > + *
> > > + * We used to do
> > > + *   while (xchg(&b->lock, 1) cpu_relax();
> > > + * but, it turns out not all architectures support xchg on a single byte.
> > > + *
> > > + * So now we use bit_spin_lock(), with fun games since we can't burn a whole
> > > + * ulong for this.
> > > + */
> > > +
> > 
> > Oof. :P Well I think I understand what this is doing, but it would be
> > helpful if this last sentence were a little more direct. For example:
> > 
> > "So now we use bit_spin_lock(). We can't burn a whole ulong for this, so
> > cast and define the lock bit such that it always lands in the b->lock
> > byte."
> > 
> > ... but feel free to reword that, of course.
> 
> yeah, that's good
> 
> > 
> > > +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > > +#define BUCKET_LOCK_BITNR	0
> > > +#else
> > > +#define BUCKET_LOCK_BITNR	(BITS_PER_LONG - 1)
> > > +#endif
> > > +
> > >  static inline void bucket_unlock(struct bucket *b)
> > >  {
> > > -	smp_store_release(&b->lock, 0);
> > > +	bit_unspin_lock(BUCKET_LOCK_BITNR, (void *) &b->lock);
> > 
> > This doesn't compile.. bit_spin_unlock() I assume.
> 
> that's what I get for writing code right before my flight. Now I'm going
> to have to try to think of a legit usage for unspin_lock() :)
> 

Heh. :)

> > Also, is there any good way to add a simple debug mode check here just
> > to confirm the external code does what we expect on whatever
> > obscure/otherwise untested arch somebody might try to use? I.e.
> > EBUG_ON(b->lock != 1) or some such after acquiring the lock..?
> 
> How about:
> 
> union ulong_byte_assert {                                                                                                                                                                                                                                                                
>         ulong   ulong;                                                                                                                                                                                                                                                                   
>         u8      byte;                                                                                                                                                                                                                                                                    
> };
> 
> BUILD_BUG_ON(!((union ulong_byte_assert) { .ulong = 1UL << BUCKET_LOCK_BITNR }).byte);                                                                                                                                                                                           
> 

Nice idea. I like the build time check much better. Thanks!

Brian