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@xxxxxx>
> 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@xxxxxx>
> ---
> 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@xxxxxxxxxx>
Thanks,
Ming
