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

[Brian Foster <bfoster@redhat.com>]

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
>