Re: [PATCH v7] sbitmap: fix io hung due to race on sbitmap_word::cleared

[YangYang]

On 2024/7/16 1:26, Bart Van Assche wrote:
> On 7/15/24 2:11 AM, Yang Yang wrote:
> > +
> > +    /**
> > +     * @swap_lock: serializes simultaneous updates of ->word and ->cleared
> > +     */
> > +    spinlock_t swap_lock;
> >   } ____cacheline_aligned_in_smp;
>
> Thank you for having updated this comment.
>
> > -static inline bool sbitmap_deferred_clear(struct sbitmap_word *map)
> > +static inline bool sbitmap_deferred_clear(struct sbitmap_word *map,
> > +        unsigned int depth, unsigned int alloc_hint, bool wrap)
> >   {
> > -    unsigned long mask;
> > +    unsigned long mask, word_mask;
> > +    bool ret = false;
> > -    if (!READ_ONCE(map->cleared))
> > -        return false;
> > +    guard(spinlock_irqsave)(&map->swap_lock);
> > +
> > +    if (!map->cleared) {
> > +        if (depth > 0) {
> > +            word_mask = (~0UL) >> (BITS_PER_LONG - depth);
> > +            /*
> > +             * The current behavior is to always retry after moving
> > +             * ->cleared to word, and we change it to retry in case
> > +             * of any free bits. To avoid an infinite loop, we need
> > +             * to take wrap & alloc_hint into account, otherwise a
> > +             * soft lockup may occur.
> > +             */
> > +            if (!wrap && alloc_hint)
> > +                word_mask &= ~((1UL << alloc_hint) - 1);
> > +
> > +            if ((READ_ONCE(map->word) & word_mask) == word_mask)
> > +                ret = false;
> > +            else
> > +                ret = true;
> > +        }
> > +
> > +        return ret;
> > +    }
>
> Now that guard()() is being used, the local variable 'ret' can be eliminated. The if 
> (READ_ONCE() ...) statement can be changed into the
> following: return (READ_ONCE(map->word) & word_mask) != word_mask;
> and "return ret;" can be changed into "return false;". Additionally,
> the indentation depth can be reduced by changing "if (depth > 0) {" ...
> into "if (depth == 0) return false;".

This looks much better, I will make these changes in the next version.

Thanks.

> Otherwise this patch looks good to me.
>
> Thanks,
>
> Bart.