Messages in this thread | | | Subject | Re: [PATCH v2] blk-mq: fix io hung due to missing commit_rqs | From | Yu Kuai <> | Date | Tue, 26 Jul 2022 14:30:07 +0800 |
| |
Hi, Ming!
在 2022/07/26 12:21, Ming Lei 写道: > On Tue, Jul 26, 2022 at 11:35:19AM +0800, Yu Kuai wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> Currently, in virtio_scsi, if 'bd->last' is not set to true while >> dispatching request, such io will stay in driver's queue, and driver >> will wait for block layer to dispatch more rqs. However, if block >> layer failed to dispatch more rq, it should trigger commit_rqs to >> inform driver. >> >> There is a problem in blk_mq_try_issue_list_directly() that commit_rqs >> won't be called: >> >> // assume that queue_depth is set to 1, list contains two rq >> blk_mq_try_issue_list_directly >> blk_mq_request_issue_directly >> // dispatch first rq >> // last is false >> __blk_mq_try_issue_directly >> blk_mq_get_dispatch_budget >> // succeed to get first budget >> __blk_mq_issue_directly >> scsi_queue_rq >> cmd->flags |= SCMD_LAST >> virtscsi_queuecommand >> kick = (sc->flags & SCMD_LAST) != 0 >> // kick is false, first rq won't issue to disk >> queued++ >> >> blk_mq_request_issue_directly >> // dispatch second rq >> __blk_mq_try_issue_directly >> blk_mq_get_dispatch_budget >> // failed to get second budget >> ret == BLK_STS_RESOURCE >> blk_mq_request_bypass_insert >> // errors is still 0 >> >> if (!list_empty(list) || errors && ...) >> // won't pass, commit_rqs won't be called >> >> In this situation, first rq relied on second rq to dispatch, while >> second rq relied on first rq to complete, thus they will both hung. >> And same problem exists in blk_mq_dispatch_rq_list() >> >> Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since >> it means that request is not queued successfully. >> >> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> Changes in v2: >> - suggested by Ming, handle blk_mq_dispatch_rq_list() as well. >> - change title and modify commit message. >> >> block/blk-mq.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 70177ee74295..ee1e065fe63f 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1909,6 +1909,7 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, >> fallthrough; >> case BLK_STS_DEV_RESOURCE: >> blk_mq_handle_dev_resource(rq, list); >> + errors++; >> goto out; >> case BLK_STS_ZONE_RESOURCE: >> /* >> @@ -1918,6 +1919,7 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, >> */ >> blk_mq_handle_zone_resource(rq, &zone_list); >> needs_resource = true; >> + errors++; > > But accounting error here may break return value of > blk_mq_dispatch_rq_list(), see: > > /* Returns true if we did some work AND can potentially do more. */
Yes, you're right. I'll try to fix that in v3.
Thanks, Kuai >
| |