Re: [PATCH] blk-mq: don't fail driver tag allocation because of inactive hctx

[Ming Lei <ming.lei@redhat.com>]

On Wed, Jun 03, 2020 at 01:53:47PM +0200, Christoph Hellwig wrote:
> On Wed, Jun 03, 2020 at 06:51:27PM +0800, Ming Lei wrote:
> > Commit bf0beec0607d ("blk-mq: drain I/O when all CPUs in a hctx are offline")
> > prevents new request from being allocated on hctx which is going to be inactive,
> > meantime drains all in-queue requests.
> > 
> > We needn't to prevent driver tag from being allocated during cpu hotplug, more
> > importantly we have to provide driver tag for requests, so that the cpu hotplug
> > handling can move on. blk_mq_get_tag() is shared for allocating both internal
> > tag and drive tag, so driver tag allocation may fail because the hctx is
> > marked as inactive.
> > 
> > Fix the issue by moving BLK_MQ_S_INACTIVE check to __blk_mq_alloc_request().
> 
> This looks correct, but a little ugly.  How about we resurrect my
> patch to split off blk_mq_get_driver_tag entirely?  Quick untested rebase

OK, I am fine.

> below, still needs a better changelog and fixes tg:
> 
> ---
> From e432011e2eb5ac7bd1046bbf936645e9f7b74e64 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Sat, 16 May 2020 08:03:48 +0200
> Subject: blk-mq: split out a __blk_mq_get_driver_tag helper
> 
> Allocation of the driver tag in the case of using a scheduler shares very
> little code with the "normal" tag allocation.  Split out a new helper to
> streamline this path, and untangle it from the complex normal tag
> allocation.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  block/blk-mq-tag.c | 27 +++++++++++++++++++++++++++
>  block/blk-mq-tag.h |  8 ++++++++
>  block/blk-mq.c     | 29 -----------------------------
>  block/blk-mq.h     |  1 -
>  4 files changed, 35 insertions(+), 30 deletions(-)
> 
> diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
> index 96a39d0724a29..cded7fdcad8ef 100644
> --- a/block/blk-mq-tag.c
> +++ b/block/blk-mq-tag.c
> @@ -191,6 +191,33 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
>  	return tag + tag_offset;
>  }
>  
> +bool __blk_mq_get_driver_tag(struct request *rq)
> +{
> +	struct sbitmap_queue *bt = &rq->mq_hctx->tags->bitmap_tags;
> +	unsigned int tag_offset = rq->mq_hctx->tags->nr_reserved_tags;
> +	bool shared = blk_mq_tag_busy(rq->mq_hctx);

Not necessary to add 'shared' which is just used once.

> +	int tag;
> +
> +	if (blk_mq_tag_is_reserved(rq->mq_hctx->sched_tags, rq->internal_tag)) {
> +		bt = &rq->mq_hctx->tags->breserved_tags;

Too many rq->mq_hctx->tags, you can add one local variable to store it.

Otherwise, looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>

Thanks, 
Ming