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
