Received-SPF: pass (google.com: domain of linux-kernel+bounces-209882-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <c28e9a07-c538-4f85-9eec-dc9d00679499@vivo.com>
Date: Tue, 11 Jun 2024 20:53:50 +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: Pavel Begunkov <asml.silence@gmail.com>
Cc: Jens Axboe <axboe@kernel.dk>, Andrew Morton <akpm@linux-foundation.org>,
 linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
 "yukuai (C)" <yukuai3@huawei.com>, Yu Kuai <yukuai1@huaweicloud.com>,
 Ming Lei <ming.lei@redhat.com>
References: <20240604031124.2261-1-yang.yang@vivo.com>
 <CAFj5m9KV7OJ4_KjbSkpdtfrKamoLzV6EH-mJP3=y+VvoYOzC3w@mail.gmail.com>
 <aa7246f9-f7df-3054-077e-eb21c7f423ac@huaweicloud.com>
 <e1cdf579-007b-415f-9e4d-3fadd6f97b36@vivo.com>
 <cf8bb1db-b601-4f54-bafc-d6c58f6ce946@gmail.com>
From: YangYang <yang.yang@vivo.com>
In-Reply-To: <cf8bb1db-b601-4f54-bafc-d6c58f6ce946@gmail.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?WHZmbWtPeHI2dkVRUlBMWkJhc0J4WWU4UlQzUURWTzE5SEd1MXJwd1ZVMHVW?=
 =?utf-8?B?dFUrcDd5SlVRUEJjUVJmNm5zckVZNkxhVk1vTUtWN2RCOEFibENkZnpTWWNt?=
 =?utf-8?B?VzBmendnVVEyYUpER2dQdEVqcnlWSnFzQWwxdXdoV015NG43eTd2UU1JV0xD?=
 =?utf-8?B?QmF6end0VGtTY0NlVzlmRnpYU3d2Vm93aS9yK25ZZU91OUVzM3ozemw4NlZK?=
 =?utf-8?B?WVNXeEJyZEU0dC9IVTkwWUNmZmFudzhKakdyaU9JV0lxSkJEOUJ0Uk1XWjFl?=
 =?utf-8?B?NEpvelpRRkhsRjVKMDljS3hJSHA3VnhoSll2M3lZall3dTMrWDJqV1R2Rlp6?=
 =?utf-8?B?UnM3QXRZcm9JYjVJM3FNVjY5NGRscnhqaWx4V1EwMFVkTTlPenpFTUgvV2dj?=
 =?utf-8?B?cmE1dlAyVE5FUHZycm5lUzEyY3RuWjZHc2FWc1BCc3NYdnF4amp6N3l3MlY3?=
 =?utf-8?B?blJESVh2S3U3QnE2UjgrSDRIMWlTanhpR1pLbkhOY1prbitqNzZmVzY0QlE3?=
 =?utf-8?B?bnJiZGlQeVdlTmU2TlMyU1dlTnl1cndWekIySHkzdFlySE9XT0RIeVJEOUJ3?=
 =?utf-8?B?TGM2WXg5eWZXQ1pLQ05wNS9rOXkzNnZKY1cwMXhxTFlEM3Q1VUJZcU5iSDJX?=
 =?utf-8?B?aEcvK3hLays3WHpyeHdybjA1WnQ0aDVDdUxBSnFtQ1kwUlhadmV2d0Q4ZHVB?=
 =?utf-8?B?alZidFRSM05sU2xnOS9aelRBZHowK0E5TUE5Rks5UVV3eGZVSmZTRXZQczND?=
 =?utf-8?B?MzNKc1VoU0pLd3hUcldETjZta01pd3Zpd0dVZ0tkb0Q4SWZja3dHeVR0Y3Uv?=
 =?utf-8?B?a0t4S0FZeFpvSUc0dmRpVnFrbnRCck9aYmhhekVUVXJYV2Q3R2haVG5HS1Bm?=
 =?utf-8?B?WGRMc2VQd0tzZEFBRjJOWmRQVnNPTmhtRk9mejd4M0dmRnFTWDhNN2YwQW9J?=
 =?utf-8?B?L3VENVdxdlk3YURDVWpRSkRocjhkNFYzYWsvYmx6SHRpalAwSVRlM3I4ZnY3?=
 =?utf-8?B?K2dzeVRxTVV1T0plbnUwQktIT0Z6YzVKMzg1cGxaVlY2ZndqRTJtRGlQdjlv?=
 =?utf-8?B?NFVrSm5KZEdFeTRyNVVuZjV1Zlo1aS9zTVNkdnZTRFpUbVZ2NUlhZE15WEt6?=
 =?utf-8?B?VkRGSlJLaHNyb2pHa0VKaHFabFVibTVJKzZjaXV2V0UyYzdRcEdCWlo1RlV0?=
 =?utf-8?B?ZVJMOTE2ZU1kdGhQYnlOc0NyQ1ZwZGRzQ2dSN2xnMmpZaDE0b3VVQk9mRHBW?=
 =?utf-8?B?cUZ0NWVINmQxMXF1VHhRZStOZkNFTXQvUTBWemlNVWttM1NxZE9nWW9yelBi?=
 =?utf-8?B?eGtzUjJLeFFFRzNZNDR1aUMxYkh3emJGVWtuYlQreGthS1FqSnhvcHc3aVUw?=
 =?utf-8?B?RW1BMmR2dmZzTjNYTW8zblRvcnZGNFNuaUZaZEErSk1GeG1yYzFQZ0RPZGtv?=
 =?utf-8?B?QW5aQS9xalpHTEY1SmU5aHFXc0hEaEtVbVF5Q2hOWGpkMDZhMVZybXhUMWJF?=
 =?utf-8?B?SWk0OTJZbzVOVHF1V1ZtV05oL244N2s0TTZ1VXVpQnFIdXhRandnK2h6U0dj?=
 =?utf-8?B?ckpsdkFCVzRkNm1DNkhsMTRSYTJKWUdPYWQwY1AxVXNybUJKMlcrdGlsWWJP?=
 =?utf-8?B?cHpOeDBreHg5WlNYVW8rKzMyaEdOQ0F6SjZ6UEpianpnQ094ckZYUmpVL3Jw?=
 =?utf-8?B?SlNkQjFKaENveUxQZHBPcWxLTzlGQmhYUUM1dmJNMUNESHl1TUJ5OGlSVFVx?=
 =?utf-8?B?cmlYK0g1YnVaeUZxTmMxSzZjUVpOZzBrY2xJMHdjeHhOaWhxelRkWVVlenh0?=
 =?utf-8?B?NGpaNitWYnZFZjlDOWFHMy9jVXdFQzNtbGdYamJuVEI1T2JLb2VrZy93MVlT?=
 =?utf-8?B?bHZqWkhLalE0SXYyVDliMXBjenNrRktYVnhLa1JMbFNmclRFTVBJdHI0Ui9n?=
 =?utf-8?B?ZEdJRDZRTDMvc2NkTzIyNlhYN1Boa3FKRkNBWWsrL3VFS0NFM0I4dGR5b25L?=
 =?utf-8?B?YVVHbFdTT2syWEx1L3ZiczE5QWxCWmtnZURUZlB4OFFzZFZxR1pFSFE4VW01?=
 =?utf-8?B?dS8vTmJ1Y0Z4ZGo0Tzk2OGZHOW1CSit3QnROWEFYTkttOTF4MmNpbGp0cWEz?=
 =?utf-8?Q?VpbZZAOLmjLii7URph2EoQRVn?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2ea955ab-6560-4f28-ed0f-08dc8a1589ec
X-MS-Exchange-CrossTenant-AuthSource: KL1PR06MB7401.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jun 2024 12:53:49.3698
 (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: jm8xgtdKRO0EqEkW7kQng/nsbbKiRxVaNVmBexHP49PrpQu2suEyf2mTieiUVcJctiL0ek5kduZ8bS/J5HY2JA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEZPR06MB6816

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