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,
