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

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

[Kent Overstreet]

bucket_lock() previously open coded a spinlock, because we need to cram
a spinlock into a single byte.

But it turns out not all archs support xchg() on a single byte; since we
need struct bucket to be small, this means we have to play fun games
with casts and ifdefs for endianness.

This fixes building on 32 bit arm, and likely other architectures.

Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
Cc: linux-bcachefs@vger.kernel.org
---
 fs/bcachefs/buckets.h | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
index f192809f50cf..e055c1076e63 100644
--- a/fs/bcachefs/buckets.h
+++ b/fs/bcachefs/buckets.h
@@ -40,15 +40,35 @@ static inline size_t sector_to_bucket_and_offset(const struct bch_dev *ca, secto
 	for (_b = (_buckets)->b + (_buckets)->first_bucket;	\
 	     _b < (_buckets)->b + (_buckets)->nbuckets; _b++)
 
+/*
+ * 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.
+ */
+
+#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);
 }
 
 static inline void bucket_lock(struct bucket *b)
 {
-	while (xchg(&b->lock, 1))
-		cpu_relax();
+	bit_spin_lock(BUCKET_LOCK_BITNR, (void *) &b->lock);
 }
 
 static inline struct bucket_array *gc_bucket_array(struct bch_dev *ca)
-- 
2.40.1

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

[Brian Foster]

Hmm.. seems like something is wrong either with a mailer (yours or mine)
or the list. I initially thought I replied to this and accidentally
dropped the list cc, but now that I try again, mutt drops the list on
reply-to-all. It shows the following for the original mail, so somehow
this ends up garbled in the cc list for whatever reason.

  To: unlisted-recipients: no To-header on input <;

Anyways, here's a resend of my previous reply that unintentionally
dropped the list.

On Wed, Sep 13, 2023 at 08:37:46PM -0400, Kent Overstreet wrote:
> bucket_lock() previously open coded a spinlock, because we need to cram
> a spinlock into a single byte.
> 
> But it turns out not all archs support xchg() on a single byte; since we
> need struct bucket to be small, this means we have to play fun games
> with casts and ifdefs for endianness.
> 
> This fixes building on 32 bit arm, and likely other architectures.
> 
> Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>
> Cc: linux-bcachefs@vger.kernel.org
> ---
>  fs/bcachefs/buckets.h | 26 +++++++++++++++++++++++---
>  1 file changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/bcachefs/buckets.h b/fs/bcachefs/buckets.h
> index f192809f50cf..e055c1076e63 100644
> --- a/fs/bcachefs/buckets.h
> +++ b/fs/bcachefs/buckets.h
> @@ -40,15 +40,35 @@ static inline size_t sector_to_bucket_and_offset(const struct bch_dev *ca, secto
>  	for (_b = (_buckets)->b + (_buckets)->first_bucket;	\
>  	     _b < (_buckets)->b + (_buckets)->nbuckets; _b++)
>  
> +/*
> + * 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.

> +#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.

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

Brian

>  }
>  
>  static inline void bucket_lock(struct bucket *b)
>  {
> -	while (xchg(&b->lock, 1))
> -		cpu_relax();
> +	bit_spin_lock(BUCKET_LOCK_BITNR, (void *) &b->lock);
>  }
>  
>  static inline struct bucket_array *gc_bucket_array(struct bch_dev *ca)
> -- 
> 2.40.1
> 

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

[Kent Overstreet]

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() :)

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

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

[Brian Foster]

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

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

[Brian Foster]

On Fri, Sep 15, 2023 at 06:51:02AM -0400, Brian Foster wrote:
> 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.
> > > > + */
> > > > +
...

Hi Kent,

So FYI reviewing this prompted me to do some "less common" arch testing
on s390x. bcachefs format currently fails in this environment, which
prompted the couple of related byte ordering patches I've posted (also
susceptible to the problem fixed by this bucket lock patch). The most
recent [1] seems rather closely related to this one as it pertains to
how bit_spin_lock() and friends work in userspace. Still no unspin
locking use case I'm afraid ;P, but curious on your thoughts on that
one..

Brian

[1] https://lore.kernel.org/linux-bcachefs/20230919142611.36445-1-bfoster@redhat.com/