Received-SPF: pass (google.com: domain of linux-kernel+bounces-200129-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <e1cdf579-007b-415f-9e4d-3fadd6f97b36@vivo.com>
Date: Tue, 4 Jun 2024 15:03:36 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] sbitmap: fix io hung due to race on
 sbitmap_word::cleared
Content-Language: en-US
To: Yu Kuai <yukuai1@huaweicloud.com>, Ming Lei <ming.lei@redhat.com>
Cc: Jens Axboe <axboe@kernel.dk>, Andrew Morton <akpm@linux-foundation.org>,
 Pavel Begunkov <asml.silence@gmail.com>, linux-kernel@vger.kernel.org,
 linux-block@vger.kernel.org, "yukuai (C)" <yukuai3@huawei.com>
References: <20240604031124.2261-1-yang.yang@vivo.com>
 <CAFj5m9KV7OJ4_KjbSkpdtfrKamoLzV6EH-mJP3=y+VvoYOzC3w@mail.gmail.com>
 <aa7246f9-f7df-3054-077e-eb21c7f423ac@huaweicloud.com>
From: YangYang <yang.yang@vivo.com>
In-Reply-To: <aa7246f9-f7df-3054-077e-eb21c7f423ac@huaweicloud.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?K25xTjN0alJPaDF3ckdYYWV3NHlXYjRad0tGdDJHYVQ4RnIyejA2WGxmSHBB?=
 =?utf-8?B?akR0L0g0dHhSZk9rTC9RcG1TYUg3bWNObzJheW0wTjRhM2lzRWRvTUliUGlv?=
 =?utf-8?B?Q2s0ajFXSTdXK0xRZTJpbEZ5TjlxdERqVytyajlCa285aVBsei9PdjNtR0pm?=
 =?utf-8?B?RHhXeEw2T0k5ZkRsOVovUkVKcnlJZzR5OWZQWWFLc1ZYcUhCZWdsS0dzdENy?=
 =?utf-8?B?VmF1UkNlME1LN0FNVnB6bEU0U1BudE5OTE9HWWwzazIxeWsydzQ3dmRDYnA5?=
 =?utf-8?B?WFhFWjZKSTBpanpQZjVoSHc3blZCeURVYTU2OWl3eDNRT1lFOHQrTk5uRi9N?=
 =?utf-8?B?aGpueWoxUjRQclR5eDh5UnVRRGJjVTNqbU5jaWZVMnZXVVBCN2VSNWFVMWZT?=
 =?utf-8?B?LzVwaSs1RDNURmFpTVlmR0d0RjZBcW1qSXh2MTVKaENDYVA1VHFlajNEeHZL?=
 =?utf-8?B?bkN6U2hwZklQK1V5VWp1elk0Y1VKaXJRMGtoY0kyZjdNSS9mSzZIV2kxQ0dH?=
 =?utf-8?B?eFF5NFZldXFxd1V3YkkxMFRSa2s0V2xuZkFGNGRkQSs1UTVsVFVxaGZjNUl2?=
 =?utf-8?B?cy9oSGcvRldNT2tTOHltL1J6Z0JyY2VtdkVscjNwZzVONnNxWE1ZU2pxdXhS?=
 =?utf-8?B?d0F3R0pmNHk3cmlwMzVJNXdnSlZZaGsxSGFtSGI5MkwzVXVoSzZFUmxVU2xl?=
 =?utf-8?B?Vnh2QWpJcW5IT3Z1aytad1lJSEdKaUFqUzdUMHNFdUx5QWEwZW0rRCt3cWRC?=
 =?utf-8?B?dzFHeGZzVHRWbHBnUXhMSWJtUXg1TGpBQzM4QUhpY2plZHYzc1duQTJlSWNE?=
 =?utf-8?B?TThkOTkySHRubllPUU5hWENzNTFNREtsUkZpOVRybDhadzhFZnhsQ2FxanUy?=
 =?utf-8?B?T2ttUGxFRStNbFc3dytCK25GZCtxSmZwQk5Rb1FQYk9VaTg4dlVCWGx3TW1G?=
 =?utf-8?B?Y1h2UGlHMWQ5ZTljb0hhZkhwdHoyOTdoenJSQm40OXJLUm1Eb2xQVUxXZ2hP?=
 =?utf-8?B?MWpkM1R2TmQ2amVXdWNTb3FPTm9md2hGeXVaQmQxY1lEd3hQT3lGUDFRaXdE?=
 =?utf-8?B?eU9IM1NOclQyZTBXYng2ZExvR1VtNXlhOWM5aytFWnhqdGZ3SC85bzdOUUlk?=
 =?utf-8?B?SmtwQUZTZHo4QkpqTVJDMUZjL1N3K09HeVA0SHNUUjIvZ1JYY1l4VlN3emJ5?=
 =?utf-8?B?anBWSlg5WGI0bm1qbjR2YUlpMUNUanQ2U3BDMkxMZ2ZEdWYvbUtXd0M4blJL?=
 =?utf-8?B?akcwUlAxZmhlMEtEU1VFZ3RWWmcrblhpWkkvQ0FKQkVaRUVvaFl5c1NXbXN0?=
 =?utf-8?B?QjV4TFN6ais5LzVObExSakdocXkvQm84SnQyVnBZRTl2MXZ1S3JESFRQd0ds?=
 =?utf-8?B?b3QvWllVZHhMaDJQdXZiOUp2M1IwS3o4SzY5QXNSQXkwU1h0TVB4TmVHaFQy?=
 =?utf-8?B?eXlyT3dvYkFQVk8vbE5FdmlHYitDVnVKK3NCRzNCRWhpVERqWFdOWG4rbW9K?=
 =?utf-8?B?SHowUXd1cEUwcnVzNzR5U1RYWXkxSUt6M3ZJOWF6czlNdmp2TmhiWmFNcDFM?=
 =?utf-8?B?RnZnM2pJNWN0dUpzeHVTU1R1Z0hkVitqbGJWYW9Bb3NmTTZzK0JpQ1VVcEMv?=
 =?utf-8?B?SU1Ha2E2WVRpUGZWZEw0NlF1SWpHcTRDdHduRXdjZ2I5YzNFZ3RydzhLcWVO?=
 =?utf-8?B?Z0dub3J1TC9yL0t4SktXcFM4VHYvZnJQeW9pRkRFNkhaM2NrLzhVYWhWT1pi?=
 =?utf-8?B?NnVUVDEvVDlVWGlmVm9McXVXUzEzMktQUnZJSFdFQ0djZW5pNkhMSm5SbDRx?=
 =?utf-8?B?Y0RldDRVZHFwVHBWUlJMV21yWDVSN21LV1Q2aUdSOHBDRWZ0ZjBmdGdHdE1q?=
 =?utf-8?B?dk9WOXN0ejF6cHZGREdGVnUweHFUNnh0dW9BTXZ4QitYZHNCZ3IrU3N0ZGo3?=
 =?utf-8?B?ZXpPMnZ1cllsUXpMaitLWGJMcS9ya3ZOdHJpNXp6bHAvNFV1cDl6bmlhM3Z0?=
 =?utf-8?B?TXZWMEQrTzNZYW5YS0xhQUpZemVCOFJTU0hSSlBDK3BSZzREK3NWajBoUTFM?=
 =?utf-8?B?MkRES1hwSHdaaFdVZTRIc2p3VHQvQjhNc0twbUFiWVladm5iS25DMFpCOTdU?=
 =?utf-8?Q?O/5dK0e1jolrHKO/7y3Fy66rG?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 00cb5f90-c5a4-4ea5-e972-08dc846475d3
X-MS-Exchange-CrossTenant-AuthSource: KL1PR06MB7401.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 07:03:38.9013
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 923e42dc-48d5-4cbe-b582-1a797a6412ed
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qP3RD7jOqB3yMLZuBqjZp840b0+lLA5GpWrvSiXMMhH8ORaIUT2h2pPncUMcf1zo8Q2gTh+jXiMYTymNI0/LYw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: TY0PR06MB5548

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@vivo.com> 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.

> 
> 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.

> 
> Thanks,
> Kuai
> 
>>
>> Thanks,
>>
>>
>> .
>>
>