[12/19] blk-mq: Initialize a request before assigning a tag
diff mbox

Message ID 20170525184327.23570-13-bart.vanassche@sandisk.com
State New
Headers show

Commit Message

Bart Van Assche May 25, 2017, 6:43 p.m. UTC
Initialization of blk-mq requests is a bit weird: blk_mq_rq_ctx_init()
is called after a tag has been assigned and .rq_flags is initialized
in __blk_mq_finish_request(). Call blk_mq_rq_ctx_init() before
modifying any struct request members. Initialize .rq_flags in
blk_mq_rq_ctx_init() instead of in __blk_mq_finish_request(). This
patch does not change the behavior of the block layer.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Omar Sandoval <osandov@fb.com>
Cc: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Christoph Hellwig May 28, 2017, 8:42 a.m. UTC | #1
On Thu, May 25, 2017 at 11:43:20AM -0700, Bart Van Assche wrote:
> Initialization of blk-mq requests is a bit weird: blk_mq_rq_ctx_init()
> is called after a tag has been assigned and .rq_flags is initialized
> in __blk_mq_finish_request().
>
> Call blk_mq_rq_ctx_init() before
> modifying any struct request members. Initialize .rq_flags in
> blk_mq_rq_ctx_init() instead of in __blk_mq_finish_request(). This
> patch does not change the behavior of the block layer.

One things this patch does is to initialize the tag value actually
set twice.  In general this looks ok, but I can't really see the real
value.

So a very reluctant:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Bart Van Assche May 28, 2017, 4:17 p.m. UTC | #2
On Sun, 2017-05-28 at 10:42 +0200, Christoph Hellwig wrote:
> On Thu, May 25, 2017 at 11:43:20AM -0700, Bart Van Assche wrote:

> > Initialization of blk-mq requests is a bit weird: blk_mq_rq_ctx_init()

> > is called after a tag has been assigned and .rq_flags is initialized

> > in __blk_mq_finish_request().

> > 

> > Call blk_mq_rq_ctx_init() before

> > modifying any struct request members. Initialize .rq_flags in

> > blk_mq_rq_ctx_init() instead of in __blk_mq_finish_request(). This

> > patch does not change the behavior of the block layer.

> 

> One things this patch does is to initialize the tag value actually

> set twice.  In general this looks ok, but I can't really see the real

> value.


Hello Christoph,

If you want I can leave out the changes to the .tag / .internal_tag
assignments from this patch.

Bart.

Patch
diff mbox

diff --git a/block/blk-mq.c b/block/blk-mq.c
index b8092786d42f..1fcc20df1a4e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -212,6 +212,7 @@  void blk_mq_rq_ctx_init(struct request_queue *q, struct blk_mq_ctx *ctx,
 	rq->q = q;
 	rq->mq_ctx = ctx;
 	rq->cmd_flags = op;
+	rq->rq_flags = 0;
 	if (blk_queue_io_stat(q))
 		rq->rq_flags |= RQF_IO_STAT;
 	/* do not touch atomic flags, it needs atomic ops against the timer */
@@ -231,7 +232,8 @@  void blk_mq_rq_ctx_init(struct request_queue *q, struct blk_mq_ctx *ctx,
 	rq->nr_integrity_segments = 0;
 #endif
 	rq->special = NULL;
-	/* tag was already set */
+	rq->tag = -1;
+	rq->internal_tag = -1;
 	rq->extra_len = 0;
 
 	INIT_LIST_HEAD(&rq->timeout_list);
@@ -260,20 +262,19 @@  struct request *__blk_mq_alloc_request(struct blk_mq_alloc_data *data,
 
 		rq = tags->static_rqs[tag];
 
+		blk_mq_rq_ctx_init(data->q, data->ctx, rq, op);
+
 		if (data->flags & BLK_MQ_REQ_INTERNAL) {
-			rq->tag = -1;
 			rq->internal_tag = tag;
 		} else {
 			if (blk_mq_tag_busy(data->hctx)) {
-				rq->rq_flags = RQF_MQ_INFLIGHT;
+				rq->rq_flags |= RQF_MQ_INFLIGHT;
 				atomic_inc(&data->hctx->nr_active);
 			}
 			rq->tag = tag;
-			rq->internal_tag = -1;
-			data->hctx->tags->rqs[rq->tag] = rq;
+			data->hctx->tags->rqs[tag] = rq;
 		}
 
-		blk_mq_rq_ctx_init(data->q, data->ctx, rq, op);
 		return rq;
 	}
 
@@ -364,7 +365,6 @@  void __blk_mq_finish_request(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx,
 		atomic_dec(&hctx->nr_active);
 
 	wbt_done(q->rq_wb, &rq->issue_stat);
-	rq->rq_flags = 0;
 
 	clear_bit(REQ_ATOM_STARTED, &rq->atomic_flags);
 	clear_bit(REQ_ATOM_POLL_SLEPT, &rq->atomic_flags);