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@osandov.com>]

On Mon, Feb 27, 2017 at 10:03:29AM -0700, Jens Axboe wrote:
> On 02/27/2017 09:59 AM, Omar Sandoval wrote:
> > 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@grimberg.me>
> >> ---
> >>  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);
> 
> Agree, let's keep that in one place, if we can.
> 
> > 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 :)
> 
> That's just broken, we need to fix that up. _hctx() request alloc
> should return scheduler request as well.
> 
> Omar, care to rework patch #1 and incorporate a fix for the hctx
> alloc? Then I'll fix up patch #2, adding the carry-over of the
> reserved flag. We'll just rebase for-linus, it's not a stable
> branch.

Will do, I'll make sure to add Sagi's reported-by.