linux-kernel - Re: [PATCH] blk-mq: fix io hung due to missing commit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Jul 2022 10:48:10 +0800
From:   Ming Lei <ming.lei@...hat.com>
To:     Yu Kuai <yukuai1@...weicloud.com>
Cc:     axboe@...nel.dk, osandov@...com, linux-block@...r.kernel.org,
        linux-kernel@...r.kernel.org, yukuai3@...wei.com,
        yi.zhang@...wei.com, ming.lei@...hat.com
Subject: Re: [PATCH] blk-mq: fix io hung due to missing commit_rqs while
 scheduler is none

On Tue, Jul 26, 2022 at 10:38:52AM +0800, Yu Kuai wrote:
> From: Yu Kuai <yukuai3@...wei.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' and
> 'BLK_STS_DEV_RESOURCE' as 'errors' in blk_mq_try_issue_list_directly(),
> so that 'commit_rqs' will be called when dispatch of last rq failed.
> 
> Fixes: d666ba98f849 ("blk-mq: add mq_ops->commit_rqs()")
> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> ---
>  block/blk-mq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 70177ee74295..752b0fe4c128 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -2680,6 +2680,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++;

OK, it is because that list becomes empty, but .queue_rq(last_rq_in_list)
returns BLK_STS_*RESOURCE, but scsi can't call ->queuecommand() for this
real last rq. Then blk_mq_try_issue_list_directly() doesn't call
->commit_rqs() too.

Here errors means that request not queued successfully, so this patch
looks fine.

Also I think blk_mq_dispatch_rq_list() needs similar handling too,
right?


thanks,
Ming