| Message ID | dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike >> its blk_mq_alloc_request() counterpart. It also crashes because it >> doesn't update the tags->rqs map. >> >> Fix it by making it allocate a scheduler request. >> >> Reported-by: Sagi Grimberg <sagi@grimberg.me> >> Signed-off-by: Omar Sandoval <osandov@fb.com> >> --- >> I think this should do it. Verified that normal operation still works >> for me, but I'm not sure how to test the actual _hctx() alloc path. >> Sagi, could you please test this series out? > > Hm, Sagi, your mail server seems to have rejected patches 2 and 3 > because git-send-email duplicated the in-reply-to header for some > reason. I've attached the patches instead. Strange, I got the them.
On 02/27/2017 10:47 AM, Omar Sandoval wrote: > From: Omar Sandoval <osandov@fb.com> > > blk_mq_alloc_request_hctx() allocates a driver request directly, unlike > its blk_mq_alloc_request() counterpart. It also crashes because it > doesn't update the tags->rqs map. > > Fix it by making it allocate a scheduler request. All three look good to me. Would you mind respinning #1, it doesn't apply on top of the reserved tag patch.
On Mon, Feb 27, 2017 at 11:07:27AM -0700, Jens Axboe wrote: > On 02/27/2017 10:47 AM, Omar Sandoval wrote: > > From: Omar Sandoval <osandov@fb.com> > > > > blk_mq_alloc_request_hctx() allocates a driver request directly, unlike > > its blk_mq_alloc_request() counterpart. It also crashes because it > > doesn't update the tags->rqs map. > > > > Fix it by making it allocate a scheduler request. > > All three look good to me. Would you mind respinning #1, it doesn't > apply on top of the reserved tag patch. Yup, I had those based on Sagi's original patches for some reason. I fat-fingered send-email, sent as a reply to the original patch 1 instead of this email.
On 02/27/2017 11:30 AM, Omar Sandoval wrote: > On Mon, Feb 27, 2017 at 11:07:27AM -0700, Jens Axboe wrote: >> On 02/27/2017 10:47 AM, Omar Sandoval wrote: >>> From: Omar Sandoval <osandov@fb.com> >>> >>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike >>> its blk_mq_alloc_request() counterpart. It also crashes because it >>> doesn't update the tags->rqs map. >>> >>> Fix it by making it allocate a scheduler request. >> >> All three look good to me. Would you mind respinning #1, it doesn't >> apply on top of the reserved tag patch. > > Yup, I had those based on Sagi's original patches for some reason. I > fat-fingered send-email, sent as a reply to the original patch 1 instead > of this email. I got it, applied all 3, thanks Omar!
>>>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike >>>> its blk_mq_alloc_request() counterpart. It also crashes because it >>>> doesn't update the tags->rqs map. >>>> >>>> Fix it by making it allocate a scheduler request. >>> >>> All three look good to me. Would you mind respinning #1, it doesn't >>> apply on top of the reserved tag patch. >> >> Yup, I had those based on Sagi's original patches for some reason. I >> fat-fingered send-email, sent as a reply to the original patch 1 instead >> of this email. > > I got it, applied all 3, thanks Omar! FWIW, you can add my: Tested-by: Sagi Grimberg <sagi@grmberg.me> Although I didn't try to trigger reserved vs. normal tag competition which will never ever happen for nvmf btw, but I assume it can happen in drivers who use reserved for error handling.
On 02/27/2017 11:41 AM, Sagi Grimberg wrote: > >>>>> blk_mq_alloc_request_hctx() allocates a driver request directly, unlike >>>>> its blk_mq_alloc_request() counterpart. It also crashes because it >>>>> doesn't update the tags->rqs map. >>>>> >>>>> Fix it by making it allocate a scheduler request. >>>> >>>> All three look good to me. Would you mind respinning #1, it doesn't >>>> apply on top of the reserved tag patch. >>> >>> Yup, I had those based on Sagi's original patches for some reason. I >>> fat-fingered send-email, sent as a reply to the original patch 1 instead >>> of this email. >> >> I got it, applied all 3, thanks Omar! > > FWIW, you can add my: > > Tested-by: Sagi Grimberg <sagi@grmberg.me> Done, I fixed up your typo, though :-) > Although I didn't try to trigger reserved vs. normal tag > competition which will never ever happen for nvmf btw, but > I assume it can happen in drivers who use reserved for error > handling. As long as we verify they work independently, there should not be a need to check that edge case separately. We should have identical behavior there now to previously.
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index 46ca965fff5c..5697b23412a1 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -110,15 +110,14 @@ struct request *blk_mq_sched_get_request(struct request_queue *q, struct blk_mq_alloc_data *data) { struct elevator_queue *e = q->elevator; - struct blk_mq_hw_ctx *hctx; - struct blk_mq_ctx *ctx; struct request *rq; blk_queue_enter_live(q); - ctx = blk_mq_get_ctx(q); - hctx = blk_mq_map_queue(q, ctx->cpu); - - blk_mq_set_alloc_data(data, q, data->flags, ctx, hctx); + data->q = q; + if (likely(!data->ctx)) + data->ctx = blk_mq_get_ctx(q); + if (likely(!data->hctx)) + data->hctx = blk_mq_map_queue(q, data->ctx->cpu); if (e) { data->flags |= BLK_MQ_REQ_INTERNAL; diff --git a/block/blk-mq.c b/block/blk-mq.c index 9611cd9920e9..cc9713f574a5 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -273,10 +273,9 @@ EXPORT_SYMBOL(blk_mq_alloc_request); struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw, unsigned int flags, unsigned int hctx_idx) { - struct blk_mq_hw_ctx *hctx; - struct blk_mq_ctx *ctx; + struct blk_mq_alloc_data alloc_data = { .flags = flags }; struct request *rq; - struct blk_mq_alloc_data alloc_data; + unsigned int cpu; int ret; /* @@ -299,26 +298,23 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, int rw, * Check if the hardware context is actually mapped to anything. * If not tell the caller that it should skip this queue. */ - hctx = q->queue_hw_ctx[hctx_idx]; - if (!blk_mq_hw_queue_mapped(hctx)) { - ret = -EXDEV; - goto out_queue_exit; + alloc_data.hctx = q->queue_hw_ctx[hctx_idx]; + if (!blk_mq_hw_queue_mapped(alloc_data.hctx)) { + blk_queue_exit(q); + return ERR_PTR(-EXDEV); } - ctx = __blk_mq_get_ctx(q, cpumask_first(hctx->cpumask)); + cpu = cpumask_first(alloc_data.hctx->cpumask); + alloc_data.ctx = __blk_mq_get_ctx(q, cpu); - blk_mq_set_alloc_data(&alloc_data, q, flags, ctx, hctx); - rq = __blk_mq_alloc_request(&alloc_data, rw); - if (!rq) { - ret = -EWOULDBLOCK; - goto out_queue_exit; - } - alloc_data.hctx->tags->rqs[rq->tag] = rq; - - return rq; + rq = blk_mq_sched_get_request(q, NULL, rw, &alloc_data); -out_queue_exit: + blk_mq_put_ctx(alloc_data.ctx); blk_queue_exit(q); - return ERR_PTR(ret); + + if (!rq) + return ERR_PTR(-EWOULDBLOCK); + + return rq; } EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);