Re: [PATCH V7 2/9] blk-mq: assign rq->tag in blk_mq_get_driver_tag

[Christoph Hellwig <hch@lst.de>]

On Sat, Apr 18, 2020 at 11:09:18AM +0800, Ming Lei wrote:
> Especially for none elevator, rq->tag is assigned after the request is
> allocated, so there isn't any way to figure out if one request is in
> being dispatched. Also the code path wrt. driver tag becomes a bit
> difference between none and io scheduler.
> 
> When one hctx becomes inactive, we have to prevent any request from
> being dispatched to LLD. And get driver tag provides one perfect chance
> to do that. Meantime we can drain any such requests by checking if
> rq->tag is assigned.
> 
> So only assign rq->tag until blk_mq_get_driver_tag() is called.
> 
> This way also simplifies code of dealing with driver tag a lot.

I really like this, but I find blk_mq_get_driver_tag really convoluted
after this patch, mostly due to the excessive use of gotos, but also
because it mixed tag >= 0 and tag == -1 checks, which should mean the
same as -1 is the only negative value assigned (which btw really should
grow a BLK_MQ_NO_TAG name in another patch).  I think something like
this folded into your patch would be a nice improvement and also fit
in with the changes later in the series:

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 05fe2eb615a9..fb7b35941e6c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1021,40 +1021,41 @@ static inline unsigned int queued_to_index(unsigned int queued)
 	return min(BLK_MQ_MAX_DISPATCH_ORDER - 1, ilog2(queued) + 1);
 }
 
-static bool blk_mq_get_driver_tag(struct request *rq)
+static bool __blk_mq_get_driver_tag(struct request *rq)
 {
 	struct blk_mq_alloc_data data = {
-		.q = rq->q,
-		.hctx = rq->mq_hctx,
-		.flags = BLK_MQ_REQ_NOWAIT,
-		.cmd_flags = rq->cmd_flags,
+		.q		= rq->q,
+		.hctx		= rq->mq_hctx,
+		.flags		= BLK_MQ_REQ_NOWAIT,
+		.cmd_flags	= rq->cmd_flags,
 	};
-	bool shared;
-
-	if (rq->tag >= 0)
-		goto allocated;
 
-	if (!data.hctx->sched_tags) {
+	if (data.hctx->sched_tags) {
+		if (blk_mq_tag_is_reserved(data.hctx->sched_tags,
+				rq->internal_tag))
+			data.flags |= BLK_MQ_REQ_RESERVED;
+		rq->tag = blk_mq_get_tag(&data);
+	} else {
 		rq->tag = rq->internal_tag;
-		goto set_rq;
 	}
 
-	if (blk_mq_tag_is_reserved(data.hctx->sched_tags, rq->internal_tag))
-		data.flags |= BLK_MQ_REQ_RESERVED;
-
-	rq->tag = blk_mq_get_tag(&data);
+	if (rq->tag == -1)
+		return false;
 
-set_rq:
-	shared = blk_mq_tag_busy(data.hctx);
-	if (rq->tag >= 0) {
-		if (shared) {
-			rq->rq_flags |= RQF_MQ_INFLIGHT;
-			atomic_inc(&data.hctx->nr_active);
-		}
-		data.hctx->tags->rqs[rq->tag] = rq;
+	if (blk_mq_tag_busy(data.hctx)) {
+		rq->rq_flags |= RQF_MQ_INFLIGHT;
+		atomic_inc(&data.hctx->nr_active);
 	}
-allocated:
-	return rq->tag != -1;
+	data.hctx->tags->rqs[rq->tag] = rq;
+	return true;
+}
+
+
+static bool blk_mq_get_driver_tag(struct request *rq)
+{
+	if (rq->tag == -1)
+		return __blk_mq_get_driver_tag(rq);
+	return true;
 }
 
 static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode,