| Message ID | 20220726122224.1790882-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] blk-mq: fix io hung due to missing commit_rqs | expand |
On Tue, Jul 26, 2022 at 08:22:24PM +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. > > Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since > it means that request is not queued successfully. > > Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE' > can't be treated as 'errors' here, fix the problem by calling > commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'. > > Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > Changes in v3: > - as poinited out by Ming, v2 will break return value by treating > BLK_STS_*RESOURCE as errors. > Changes in v2: > - suggested by Ming, handle blk_mq_dispatch_rq_list() as well. > - change title and modify commit message. > > block/blk-mq.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 70177ee74295..7d26b222cbc9 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -1931,7 +1931,8 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, > /* If we didn't flush the entire list, we could have told the driver > * there was more coming, but that turned out to be a lie. > */ > - if ((!list_empty(list) || errors) && q->mq_ops->commit_rqs && queued) > + if ((!list_empty(list) || errors || needs_resource || > + ret == BLK_STS_DEV_RESOURCE) && q->mq_ops->commit_rqs && queued) 'ret == BLK_STS_DEV_RESOURCE' may happen before, but 'ret' could be overridden later. Thanks, Ming
Hi, Ming 在 2022/07/27 10:11, Ming Lei 写道: > On Tue, Jul 26, 2022 at 08:22:24PM +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. >> >> Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since >> it means that request is not queued successfully. >> >> Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE' >> can't be treated as 'errors' here, fix the problem by calling >> commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'. >> >> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> Changes in v3: >> - as poinited out by Ming, v2 will break return value by treating >> BLK_STS_*RESOURCE as errors. >> Changes in v2: >> - suggested by Ming, handle blk_mq_dispatch_rq_list() as well. >> - change title and modify commit message. >> >> block/blk-mq.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index 70177ee74295..7d26b222cbc9 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1931,7 +1931,8 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, >> /* If we didn't flush the entire list, we could have told the driver >> * there was more coming, but that turned out to be a lie. >> */ >> - if ((!list_empty(list) || errors) && q->mq_ops->commit_rqs && queued) >> + if ((!list_empty(list) || errors || needs_resource || >> + ret == BLK_STS_DEV_RESOURCE) && q->mq_ops->commit_rqs && queued) > > 'ret == BLK_STS_DEV_RESOURCE' may happen before, but 'ret' could be > overridden later. From what I see, if queue_rq return 'BLK_STS_RESOURCE' or 'BLK_STS_DEV_RESOURCE', the loop will break, so that 'ret' could not be overridden. 'BLK_STS_ZONE_RESOURCE' can be overridden, however, needs_resource can cover that case. Thanks, Kuai > > Thanks, > Ming > > . >
On Tue, Jul 26, 2022 at 08:22:24PM +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. > > Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since > it means that request is not queued successfully. > > Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE' > can't be treated as 'errors' here, fix the problem by calling > commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'. > > Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> Reviewed-by: Ming Lei <ming.lei@redhat.com> Thanks, Ming
Hi, Jens Can you please consider this patch for v5.20? Thanks, Kuai 在 2022/07/27 11:08, Ming Lei 写道: > On Tue, Jul 26, 2022 at 08:22:24PM +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. >> >> Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since >> it means that request is not queued successfully. >> >> Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE' >> can't be treated as 'errors' here, fix the problem by calling >> commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'. >> >> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > > Reviewed-by: Ming Lei <ming.lei@redhat.com> > > Thanks, > Ming > > . >
在 2022/07/29 14:35, Yu Kuai 写道: > Hi, Jens > > Can you please consider this patch for v5.20? > friendly ping ... > Thanks, > Kuai > > 在 2022/07/27 11:08, Ming Lei 写道: >> On Tue, Jul 26, 2022 at 08:22:24PM +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. >>> >>> Fix the problem by also treat 'BLK_STS_*RESOURCE' as 'errors' since >>> it means that request is not queued successfully. >>> >>> Same problem exists in blk_mq_dispatch_rq_list(), 'BLK_STS_*RESOURCE' >>> can't be treated as 'errors' here, fix the problem by calling >>> commit_rqs if queue_rq return 'BLK_STS_*RESOURCE'. >>> >>> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()") >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> >> Reviewed-by: Ming Lei <ming.lei@redhat.com> >> >> Thanks, >> Ming >> >> . >> > > . >
On Tue, 26 Jul 2022 20:22:24 +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. > > [...] Applied, thanks! [1/1] blk-mq: fix io hung due to missing commit_rqs (no commit info) Best regards,
diff --git a/block/blk-mq.c b/block/blk-mq.c index 70177ee74295..7d26b222cbc9 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1931,7 +1931,8 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list, /* If we didn't flush the entire list, we could have told the driver * there was more coming, but that turned out to be a lie. */ - if ((!list_empty(list) || errors) && q->mq_ops->commit_rqs && queued) + if ((!list_empty(list) || errors || needs_resource || + ret == BLK_STS_DEV_RESOURCE) && q->mq_ops->commit_rqs && queued) q->mq_ops->commit_rqs(hctx); /* * Any items that need requeuing? Stuff them into hctx->dispatch, @@ -2680,6 +2681,7 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, list_del_init(&rq->queuelist); ret = blk_mq_request_issue_directly(rq, list_empty(list)); if (ret != BLK_STS_OK) { + errors++; if (ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) { blk_mq_request_bypass_insert(rq, false, @@ -2687,7 +2689,6 @@ void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, break; } blk_mq_end_request(rq, ret); - errors++; } else queued++; }