[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <37053ba7-68bb-4afc-88cd-8ceac30e2f85@vivo.com>
Date: Tue, 16 Jul 2024 11:48:01 +0800
From: YangYang <yang.yang@...o.com>
To: Bart Van Assche <bvanassche@....org>
Cc: Andrew Morton <akpm@...ux-foundation.org>, Jens Axboe <axboe@...nel.dk>,
Ming Lei <ming.lei@...hat.com>, Omar Sandoval <osandov@...com>,
linux-kernel@...r.kernel.org, linux-block@...r.kernel.org
Subject: Re: [PATCH v7] sbitmap: fix io hung due to race on
sbitmap_word::cleared
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.
Powered by blists - more mailing lists