From: Yu Kuai <[email protected]>
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 <[email protected]>
---
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)
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++;
}
--
2.31.1
On Tue, Jul 26, 2022 at 08:22:24PM +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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 <[email protected]>
Reviewed-by: Ming Lei <[email protected]>
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 <[email protected]>
>>
>> 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 <[email protected]>
>
> Reviewed-by: Ming Lei <[email protected]>
>
> 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>
>> Reviewed-by: Ming Lei <[email protected]>
>>
>> Thanks,
>> Ming
>>
>> .
>>
>
> .
>
On Tue, 26 Jul 2022 20:22:24 +0800, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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,
--
Jens Axboe