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