[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <591ba004-c28a-4956-ae13-c02cc8d235d3@vivo.com>
Date: Thu, 11 Jul 2024 17:47:36 +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 v6] sbitmap: fix io hung due to race on
sbitmap_word::cleared
On 2024/7/11 0:41, Bart Van Assche wrote:
> On 7/9/24 11:56 PM, Yang Yang wrote:
>> + spin_lock_irqsave(&map->swap_lock, flags);
>
> Please use guard(spinlock_irqsave) in new code instead of spin_lock_irqsave() + goto out_unlock
> + spin_unlock_irqrestore().
> That will make this function significantly easier to read and to
> maintain.
Got it.
>
>> +
>> + 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 dead loop, we need to take
>
> What is a "dead loop"? Did you perhaps want to write "infinite loop"?
Yeah. I suppose so.
>
>> + * wrap & alloc_hint into account. Without this, a soft
>> + * lockup was detected in our test environment.
>
> Source code comments should not refer to "our test environment". Code
> that is intended for upstream inclusion should work for all setups.
Got it.
>
>> + */
>> + if (!wrap && alloc_hint)
>> + word_mask &= ~((1UL << alloc_hint) - 1);
>
> Above I see an open-coded __clear_bit() operation. Has it been
> considered to use __clear_bit() instead of open-coding it?
It is meant to reset multiple bits to zero, but __clear_bit() is only
capable of resetting one bit to zero.
Thanks.
>
> Thanks,
>
> Bart.
Powered by blists - more mailing lists