Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx

[Omar Sandoval <osandov@xxxxxxxxxxx>]

On Mon, Feb 27, 2017 at 05:36:21PM +0200, Sagi Grimberg wrote:
> Otherwise we won't be able to retrieve the request from
> the tag.
> 
> Signed-off-by: Sagi Grimberg <sagi@xxxxxxxxxxx>
> ---
>  block/blk-mq.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index d84c66fb37b7..9611cd9920e9 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -312,6 +312,7 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw,
>  		ret = -EWOULDBLOCK;
>  		goto out_queue_exit;
>  	}
> +	alloc_data.hctx->tags->rqs[rq->tag] = rq;
>  
>  	return rq;
>  
> -- 
> 2.7.4

This one I think is a little bit cleaner if we just push that assignment
into __blk_mq_alloc_request() like this (again, compile tested only):

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 98c7b061781e..7267c9c23529 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -135,8 +135,6 @@ struct request *blk_mq_sched_get_request(struct request_queue *q,
 			rq = __blk_mq_alloc_request(data, op);
 	} else {
 		rq = __blk_mq_alloc_request(data, op);
-		if (rq)
-			data->hctx->tags->rqs[rq->tag] = rq;
 	}
 
 	if (rq) {
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9e6b064e5339..b4cf9dfa926b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -234,6 +234,7 @@ struct request *__blk_mq_alloc_request(struct blk_mq_alloc_data *data,
 			}
 			rq->tag = tag;
 			rq->internal_tag = -1;
+			data->hctx->tags->rqs[rq->tag] = rq;
 		}
 
 		blk_mq_rq_ctx_init(data->q, data->ctx, rq, op);

Looking a little closer at the caller, though, this is kind of weird:

struct request *nvme_alloc_request(struct request_queue *q,
		struct nvme_command *cmd, unsigned int flags, int qid)
{
	unsigned op = nvme_is_write(cmd) ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
	struct request *req;

	if (qid == NVME_QID_ANY) {
		req = blk_mq_alloc_request(q, op, flags);
	} else {
		req = blk_mq_alloc_request_hctx(q, op, flags,
				qid ? qid - 1 : 0);
	}
	if (IS_ERR(req))
		return req;

	req->cmd_flags |= REQ_FAILFAST_DRIVER;
	nvme_req(req)->cmd = cmd;

	return req;
}

In the "any" case, we allocate a request with a scheduler tag and go
through the scheduler as usual. In the hctx case, we're getting a
request with a driver tag, meaning we go through the
blk_mq_sched_bypass_insert() path when we run the request.

There's nothing really wrong about that, it just seems weird. Not sure
if it's weird enough to act on :)