On 2024/6/7 20:59, Pavel Begunkov wrote:
On 6/4/24 08:03, YangYang wrote:
On 2024/6/4 14:12, Yu Kuai wrote:
Hi,
在 2024/06/04 11:25, Ming Lei 写道:
On Tue, Jun 4, 2024 at 11:12 AM Yang Yang <yang.yang@xxxxxxxx> wrote:
Configuration for sbq:
depth=64, wake_batch=6, shift=6, map_nr=1
1. There are 64 requests in progress:
map->word = 0xFFFFFFFFFFFFFFFF
2. After all the 64 requests complete, and no more requests come:
map->word = 0xFFFFFFFFFFFFFFFF, map->cleared = 0xFFFFFFFFFFFFFFFF
3. Now two tasks try to allocate requests:
T1: T2:
__blk_mq_get_tag .
__sbitmap_queue_get .
sbitmap_get .
sbitmap_find_bit .
sbitmap_find_bit_in_word .
__sbitmap_get_word -> nr=-1 __blk_mq_get_tag
sbitmap_deferred_clear __sbitmap_queue_get
/* map->cleared=0xFFFFFFFFFFFFFFFF */ sbitmap_find_bit
if (!READ_ONCE(map->cleared)) sbitmap_find_bit_in_word
return false; __sbitmap_get_word -> nr=-1
mask = xchg(&map->cleared, 0) sbitmap_deferred_clear
atomic_long_andnot() /* map->cleared=0 */
if (!(map->cleared))
return false;
/*
* map->cleared is cleared by T1
* T2 fail to acquire the tag
*/
4. T2 is the sole tag waiter. When T1 puts the tag, T2 cannot be woken
up due to the wake_batch being set at 6. If no more requests come, T1
will wait here indefinitely.
To fix this issue, simply revert commit 661d4f55a794 ("sbitmap:
remove swap_lock"), which causes this issue.
I'd suggest to add the following words in commit log:
Check on ->cleared and update on both ->cleared and ->word need to be
done atomically, and using spinlock could be the simplest solution.
Otherwise, the patch looks fine for me.
Maybe I'm noob, but I'm confused how can this fix the problem, looks
like the race condition doesn't change.
In sbitmap_find_bit_in_word:
1) __sbitmap_get_word read word;
2) sbitmap_deferred_clear clear cleared;
3) sbitmap_deferred_clear update word;
2) and 3) are done atomically while 1) can still concurrent with 3):
t1:
sbitmap_find_bit_in_word
__sbitmap_get_word
-> read old word, return -1 > t2:
sbitmap_find_bit_in_word
__sbitmap_get_word
-> read old word, return -1
sbitmap_deferred_clear
-> clear cleared and update word
sbitmap_deferred_clear
-> cleared is cleared, fail
Yes, you are right, this patch cannot fix this issue.
One other alternative is to kill ->cleared. It's not
immediately clear how important it is. Do we have any
numbers?
Sorry, I can't get it. Are you suggesting to remove ->cleared from
struct sbitmap_word entirely?
Thanks.
BYW, I still think it's fine to fix this problem by trying the
__sbitmap_get_word() at least one more time if __sbitmap_get_word()
failed.
Err, after trying one more time __sbitmap_get_word() may still fail.
