Re: [PATCH v2] blk-mq: fix io hung due to missing commit_rqs

[Yu Kuai]

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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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
>