Received-SPF: pass (google.com: domain of linux-kernel+bounces-184876-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <de0c70fc-4965-42e3-b621-b5ffeb3bbc84@vivo.com>
Date: Tue, 21 May 2024 19:25:29 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] blk-mq: fix potential I/O hang caused by batch wakeup
Content-Language: en-US
To: Bart Van Assche <bvanassche@acm.org>, linux-block@vger.kernel.org
References: <20240520033847.13533-1-yang.yang@vivo.com>
 <24d4d60f-05f3-472b-8dfc-4edcb5f7883c@acm.org>
Cc: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org
From: YangYang <yang.yang@vivo.com>
In-Reply-To: <24d4d60f-05f3-472b-8dfc-4edcb5f7883c@acm.org>
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?UmxxNzRka0xjSEFLK3MrRlVuN1VJd2N3WmNHWWFBZkt5ZTQwdDdKNVNJWlFk?=
 =?utf-8?B?eFhjdnJJNVZFL2pSZGMxN281bUtQOWxrRnBvWFZQdnV1Y3NTTUxvNUJzTFJD?=
 =?utf-8?B?SmxnT3dyNWtraGxPK0J2S1MwOWtVVHVhSVhjR241TzB3MHFURHpMVzg3aVlO?=
 =?utf-8?B?SUlZaFVlUTNMVlpyajgzMmQ5ejBDVzd2WEF2aEpUSGNqODRaS3J6VVVYUW50?=
 =?utf-8?B?cFR6NDZvNUY1azZIYjdiNmhTZXpGcTB2ZzBHeUpnU1h0MEg4dGZyQzhKUHFY?=
 =?utf-8?B?cEJwUHJSdVJuYTlJOS9oaVFFN1RRK3JNUTBOMUZVZFZHSXVBRDVoeUVUMVlC?=
 =?utf-8?B?UFQrL0hHZzdBejVuUzNUYUl3ZU5sdGlkU0orUXhLWVNFMEI4eHZvY1prNzI1?=
 =?utf-8?B?K3o3T1NLLzZoR1FYVFltKzR5YmZEUlVtSHRLQmdQbWtzWjY2cnNIaThUSWZG?=
 =?utf-8?B?cnIzbkdTTDUxdHROcWM0Uk5Lb1hCM0lnZmplNlJWMWhtc1dlemg1TXBOckla?=
 =?utf-8?B?YXd2RWRjdmYzM0xXYnEwdVFmeE11aEo0SjZwNjJUM2lKcWNFTjNEOHJNSGgy?=
 =?utf-8?B?SU9CcS9haGFDQ1VUaDJCQ3pKYUZXQmpzZy9UOGtiUWswdng5T1NFbGIrUnpj?=
 =?utf-8?B?TDM1NjdOVENnWmRqL3JCcU5ZL3BWRXlreHNxY29WdG1FWER2bmtRTUN3K0VN?=
 =?utf-8?B?WnRrQk5LeHRuSnNHMy94SENVbXExb1VWOU1LanpkKzdpa2NuTUl5ckZJbTlB?=
 =?utf-8?B?UW13bTcvK0R3MDdsUVBVZGtCdnhtWGtMZnBqcmRVN1ZaMFNHaE1jamJuUGF3?=
 =?utf-8?B?K1pGSkJwZ1ZGUjhCMFprM0h1Wi8yRGlXRkxlditZcjZKTU9TNzBqVmI1M0ta?=
 =?utf-8?B?QWVrZnI5d010eGlWUzdleWVxQTNUMjNqUEhsdHZjR2I3RjNhU3dzT2lZWDNx?=
 =?utf-8?B?Y3JJMWR4akx3ZE1xOGFqOFJmMEpIbjBJd2k2NzhOdFJpVVh0ZDNEM0o0TWMv?=
 =?utf-8?B?SUMwZkF6RmtvUmRGb2FYWGdobU1Rczh2OFcvVkc5VlRJVWhTSk1CTGdsQmps?=
 =?utf-8?B?VDVibFB6KzczbXQ3Z3RRRDFFTzZKQWp2SkhIRnVZeGtpQTQ1cG54WVNoeVFC?=
 =?utf-8?B?VGJUSWRXK2JOK3krbEhyV0YvWG5HYnFaMnRQS00vN0dhbDhVTFRyeXk3V2R4?=
 =?utf-8?B?SEFFbUhPcGxJd0dWeTdPclNxQnZwMEtVa2NPdkEwTzF3V2tDTlJMdklnSVNY?=
 =?utf-8?B?eStnVlVwcGFBS29yVEdtVWdtNE1OM2I0M3hBSDNUZzQ5NEdIdi9VOTlENkNu?=
 =?utf-8?B?RDBQRmdvRmw3bDlIRDZiTVBoV1drT2ZWS3htYTJUeXhhNVFNV0RmaXlFcktu?=
 =?utf-8?B?aXUyblZUQlROWkFVb0lsLzZmNFZQUGxJS01TbDNJK1NYMDlwcU1uUTJOU2VG?=
 =?utf-8?B?RCszZ29ONEFOUTJCNlNzbnNkU0hqdVE5SmFwYVRWYkZIRWk1YTVPUDVqWnlN?=
 =?utf-8?B?Sjc5RXBQNUsveEs2N1RoYU93SHptTStkWGJ0blhDVzB6MnF4cFdJMDhFcmNY?=
 =?utf-8?B?eVNYcXlyYmJyVGtZMFZ3Z0Y2cUNweUkvdUlUYlV2SC9HcHZxSVA5bnJMOTFi?=
 =?utf-8?B?OElaZGVPaStXN2VJWk02eVZpVEJVS25xbzczWGt1MUZ0L1draUZTelVrTHd0?=
 =?utf-8?B?MmNjNHRkTjZ6UHFEYkQwQThJQU1SSDkxOTRrVEh6cUo5MzhveEpZUmRLRGV0?=
 =?utf-8?B?RWRrWnp0S25Pd28yRUpRaHVlMjVKdi9ZVWUrS05haTUwOG43cStva2E3RmtV?=
 =?utf-8?B?ZkN2NndJaGoyN3owK1JMQWVtTHlZYXVDOFpjYjJqSGNEWFBWdnh5ZlJtME9P?=
 =?utf-8?B?cERNaVlUN3RVU1p2NUZ2OUUyM05GVElvRU5rQVpqeGFnWDNHNnBsUTdhcVFp?=
 =?utf-8?B?UUdRNktlTmwyczZKNlBkYlNUYWhWaXpxVXRpRzlOODEzWTkxMmhKWTVxbWNR?=
 =?utf-8?B?NzhaTWdQS1ZCUVBMOG9zVEdneVhhaU0wZVFoRllOQUJXSnZ6cmFRYUpzL1pm?=
 =?utf-8?B?elFYVE1XMU5JMjZQYlhPc28rWllTY2x2cmFNZE0zZ2dkVytoMDVvd1VBb0Er?=
 =?utf-8?Q?yulHQNwzdsDfD5LDjwNtUnqYX?=
X-OriginatorOrg: vivo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7d058ada-e9c5-4bb2-0646-08dc7988babe
X-MS-Exchange-CrossTenant-AuthSource: KL1PR06MB7401.apcprd06.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2024 11:25:33.7499
 (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: +drSKikW231MztHej+sWMvQLAjfsSwwrtc2GE6NOkYbtGyJVBrSdfaUscfSPd/8aM5XFW+118XVAaPZMDi4ELg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SEYPR06MB6999

On 2024/5/21 2:11, Bart Van Assche wrote:
> On 5/19/24 20:38, Yang Yang wrote:
>> The depth is 62, and the wake_batch is 8. In the following situation,
>> the task would hang forever.
>>
>>    t1:                 t2:                          t3:
>>    blk_mq_get_tag      .                            .
>>    io_schedule         .                            .
>>                        elevator_switch              .
>>                        blk_mq_freeze_queue          .
>>                        blk_freeze_queue_start       .
>>                        blk_mq_freeze_queue_wait     .
>>                                                     blk_mq_submit_bio
>>                                                     __bio_queue_enter
>>
>> Fix this issue by waking up all the waiters sleeping on tags after
>> freezing the queue.
> 
> Shouldn't blk_mq_alloc_request() be mentioned in t1 since that is the function
> that calls blk_queue_enter()?

  t1:                      t2:                          t3:
  blk_mq_submit_bio        .                            .
  __blk_mq_alloc_requests  .                            .
  blk_mq_get_tag           .                            .
  io_schedule              .                            .
                           elevator_switch              .
                           blk_mq_freeze_queue          .
                           blk_freeze_queue_start       .
                           q->mq_freeze_depth=1         .
                           blk_mq_freeze_queue_wait     .
                                                        blk_mq_submit_bio
                                                        __bio_queue_enter
                                                        wait_event(!q->mq_freeze_depth)

> 
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index a16b5abdbbf5..e1eacfad6e5b 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -298,8 +298,6 @@ void blk_queue_start_drain(struct request_queue *q)
>>        * prevent I/O from crossing blk_queue_enter().
>>        */
>>       blk_freeze_queue_start(q);
>> -    if (queue_is_mq(q))
>> -        blk_mq_wake_waiters(q);
>>       /* Make blk_queue_enter() reexamine the DYING flag. */
>>       wake_up_all(&q->mq_freeze_wq);
>>   }
> 
> Why has blk_queue_start_drain() been modified? I don't see any reference
> in the patch description to blk_queue_start_drain(). Am I perhaps missing
> something?

blk_mq_wake_waiters() has already been called in blk_freeze_queue_start(),
so I thought it can be removed from blk_queue_start_drain().

> 
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 4ecb9db62337..9eb3139e713a 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -125,8 +125,10 @@ void blk_freeze_queue_start(struct request_queue *q)
>>       if (++q->mq_freeze_depth == 1) {
>>           percpu_ref_kill(&q->q_usage_counter);
>>           mutex_unlock(&q->mq_freeze_lock);
>> -        if (queue_is_mq(q))
>> +        if (queue_is_mq(q)) {
>> +            blk_mq_wake_waiters(q);
>>               blk_mq_run_hw_queues(q, false);
>> +        }
>>       } else {
>>           mutex_unlock(&q->mq_freeze_lock);
>>       }
> 
> Why would the above change be necessary? If the blk_queue_enter() call
> by blk_mq_alloc_request() succeeds and blk_mq_get_tag() calls
> io_schedule(), io_schedule() will be woken up indirectly by the
> blk_mq_run_hw_queues() call because that call will free one of the tags
> that the io_schedule() call is waiting for.

This patch is a workaround solution. I think the hang is caused by
a lost wakeup, so after blk_mq_run_hw_queues(), t1 is still waiting
for the tag.

     bt = 0xFFFFFF802F9C6790 -> (
     sb = (
       depth = 62,
       shift = 6,
       map_nr = 1,
       round_robin = FALSE,
       map = 0xFFFFFF803BF97000,
       alloc_hint = 0x00000049119B3F4C),
     wake_batch = 6,
     wake_index = (counter = 0),
     ws = 0xFFFFFF803BEBCA00,
     ws_active = (counter = 1),
     min_shallow_depth = 48,
     completion_cnt = (counter = 1),
     wakeup_cnt = (counter = 0))

Upon analyzing the coredump, it was noticed that sbq->completion_cnt=1,
and I can't figure out why.
blk_mq_put_tag->sbitmap_queue_clear->sbitmap_queue_wake_up(sbq, 1) should
be called multiple times, considering that sbq->ws_active=1,
sbq->completion_cnt should be greater than 1.
Looking forward to some advice from block layer experts.

Thanks.

> 
> Thanks,
> 
> Bart.