[PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-mq-sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 98c7b061781e..46ca965fff5c 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c

@@ -454,7 +454,8 @@ int blk_mq_sched_setup(struct request_queue *q)
 	 */
 	ret = 0;
 	queue_for_each_hw_ctx(q, hctx, i) {
-		hctx->sched_tags = blk_mq_alloc_rq_map(set, i, q->nr_requests, 0);
+		hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
+				q->nr_requests, set->reserved_tags);
 		if (!hctx->sched_tags) {
 			ret = -ENOMEM;
 			break;

-- 
2.7.4

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

On 02/27/2017 08:36 AM, Sagi Grimberg wrote:

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

Thanks for finding these, Sagi. Applied 1-2 for this series.

-- 
Jens Axboe

On Mon, Feb 27, 2017 at 05:36:20PM +0200, Sagi Grimberg wrote:

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

On 02/27/2017 08:49 AM, Omar Sandoval wrote:

On Mon, Feb 27, 2017 at 05:36:20PM +0200, Sagi Grimberg wrote:

quoted

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
---
 block/blk-mq-sched.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 98c7b061781e..46ca965fff5c 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c

@@ -454,7 +454,8 @@ int blk_mq_sched_setup(struct request_queue *q)
 	 */
 	ret = 0;
 	queue_for_each_hw_ctx(q, hctx, i) {
-		hctx->sched_tags = blk_mq_alloc_rq_map(set, i, q->nr_requests, 0);
+		hctx->sched_tags = blk_mq_alloc_rq_map(set, i,
+				q->nr_requests, set->reserved_tags);
 		if (!hctx->sched_tags) {
 			ret = -ENOMEM;
 			break;

-- 
2.7.4

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

-- 
Jens Axboe

quoted

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

On Mon, Feb 27, 2017 at 06:10:01PM +0200, Sagi Grimberg wrote:

quoted

quoted

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

Option 1 looks simple enough that I don't think it warrants a new
request flag (compile tested only):

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9e6b064e5339..87590f7d4f80 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -852,6 +852,9 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
 		return true;
 	}
 
+	if (rq->internal_tag < data.hctx->sched_tags->nr_reserved_tags)
+		data.flags |= BLK_MQ_REQ_RESERVED;
+
 	rq->tag = blk_mq_get_tag(&data);
 	if (rq->tag >= 0) {
 		if (blk_mq_tag_busy(data.hctx)) {

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

That sounds nice, since I'd be worried about the scheduler also needing
to be aware of the reserved status lest it also get the reserved request
stuck behind some normal requests. But, we special case flush in this
way, and it has been a huge pain.

On 02/27/2017 09:10 AM, Sagi Grimberg wrote:

quoted

quoted

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

Right, that would be possible. But I'd rather not treat any requests
differently, it's a huge pain in the ass that flush request currently
insert with a driver tag already allocated. So it's not because
scheduling will add anything at all, it's more that I'd like to move
flush requests to use regular inserts as well and not deal with some
request being "special" in any way.

The below should hopefully work. Totally untested...

diff --git a/block/blk-mq-tag.c b/block/blk-mq-tag.c
index 54c84363c1b2..e48bc2c72615 100644
--- a/block/blk-mq-tag.c
+++ b/block/blk-mq-tag.c

@@ -181,7 +181,7 @@ unsigned int blk_mq_get_tag(struct blk_mq_alloc_data *data)
 void blk_mq_put_tag(struct blk_mq_hw_ctx *hctx, struct blk_mq_tags *tags,
 		    struct blk_mq_ctx *ctx, unsigned int tag)
 {
-	if (tag >= tags->nr_reserved_tags) {
+	if (!blk_mq_tag_is_reserved(tags, tag)) {
 		const int real_tag = tag - tags->nr_reserved_tags;
 
 		BUG_ON(real_tag >= tags->nr_tags);

diff --git a/block/blk-mq-tag.h b/block/blk-mq-tag.h
index 63497423c5cd..5cb51e53cc03 100644
--- a/block/blk-mq-tag.h
+++ b/block/blk-mq-tag.h

@@ -85,4 +85,10 @@ static inline void blk_mq_tag_set_rq(struct blk_mq_hw_ctx *hctx,
 	hctx->tags->rqs[tag] = rq;
 }
 
+static inline bool blk_mq_tag_is_reserved(struct blk_mq_tags *tags,
+					  unsigned int tag)
+{
+	return tag < tags->nr_reserved_tags;
+}
+
 #endif

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 9611cd9920e9..293e79c1ee95 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -853,6 +853,9 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
 		return true;
 	}
 
+	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 >= 0) {
 		if (blk_mq_tag_busy(data.hctx)) {

-- 
Jens Axboe

On 02/27/2017 09:14 AM, Omar Sandoval wrote:

Agree, that's identical to what I just sent out as well, functionally.

quoted

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

That sounds nice, since I'd be worried about the scheduler also needing
to be aware of the reserved status lest it also get the reserved request
stuck behind some normal requests. But, we special case flush in this
way, and it has been a huge pain.

The caller better be using head insertion for this, in case we already
have requests in the queue. But that's no different than the current
logic.

So I think it should work fine.

-- 
Jens Axboe

Both patches look they'd work, I'll test. Thanks.

On Mon, Feb 27, 2017 at 09:15:27AM -0700, Jens Axboe wrote:

On 02/27/2017 09:10 AM, Sagi Grimberg wrote:

quoted

quoted

quoted

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

Right, that would be possible. But I'd rather not treat any requests
differently, it's a huge pain in the ass that flush request currently
insert with a driver tag already allocated. So it's not because
scheduling will add anything at all, it's more that I'd like to move
flush requests to use regular inserts as well and not deal with some
request being "special" in any way.

The below should hopefully work. Totally untested...

I like your variant if it works for Sagi. My only complaint (which was
already there) is that the BUG_ON(tag >= tags->nr_reserved_tags) in
blk_mq_put_tag() looks kind of silly since we just checked that exact
same condition.

On 02/27/2017 09:25 AM, Omar Sandoval wrote:

On Mon, Feb 27, 2017 at 09:15:27AM -0700, Jens Axboe wrote:

quoted

On 02/27/2017 09:10 AM, Sagi Grimberg wrote:

quoted

quoted

quoted

Hm, this may fix the crash, but I'm not sure it'll work as intended.
When we allocate the request, we'll get a reserved scheduler tag, but
then when we go to dispatch the request and call
blk_mq_get_driver_tag(), we'll be competing with all of the normal
requests for a regular driver tag. So maybe on top of this we should add
the BLK_MQ_REQ_RESERVED flag to the allocation attempt in
blk_mq_get_driver_tag() if the scheduler tag is reserved? I'm hazy on
what we expect from reserved tags, so feel free to call me crazy.

Yeah good point, we need to carry it through. Reserved tags exist
because drivers often need a request/tag for error handling. If all
tags currently are used up for regular IO that is stuck, you need
a reserved tag for error handling to guarantee progress.

So Sagi's patch does take it half the way there, but get_driver_tag
really needs to know about this as well, or we will just get stuck
there as well. Two solutions, I can think of:

1) Check the tag value in get_driver_tag, add BLK_MQ_REQ_RESERVED
   when allocating a driver tag if above X.
2) Add an RQF_SOMETHING_RESERVED. Add BLK_MQ_REQ_RESERVED in
   get_driver_tag if that is set.

Comments?

Can't we just not go through the scheduler for reserved tags? Obviously
there is no point in scheduling them...

Right, that would be possible. But I'd rather not treat any requests
differently, it's a huge pain in the ass that flush request currently
insert with a driver tag already allocated. So it's not because
scheduling will add anything at all, it's more that I'd like to move
flush requests to use regular inserts as well and not deal with some
request being "special" in any way.

The below should hopefully work. Totally untested...

I like your variant if it works for Sagi. My only complaint (which was
already there) is that the BUG_ON(tag >= tags->nr_reserved_tags) in
blk_mq_put_tag() looks kind of silly since we just checked that exact
same condition.

Yeah, that check is nonsensical. Let's kill it.

-- 
Jens Axboe

On Mon, Feb 27, 2017 at 05:36:21PM +0200, Sagi Grimberg wrote:

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 :)

On 02/27/2017 09:59 AM, Omar Sandoval wrote:

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.

-- 
Jens Axboe

On Mon, Feb 27, 2017 at 10:03:29AM -0700, Jens Axboe wrote:

On 02/27/2017 09:59 AM, Omar Sandoval wrote:

quoted

On Mon, Feb 27, 2017 at 05:36:21PM +0200, Sagi Grimberg wrote:

quoted

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.

quoted

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.

On 02/27/2017 10:04 AM, Omar Sandoval wrote:

On Mon, Feb 27, 2017 at 10:03:29AM -0700, Jens Axboe wrote:

quoted

On 02/27/2017 09:59 AM, Omar Sandoval wrote:

quoted

On Mon, Feb 27, 2017 at 05:36:21PM +0200, Sagi Grimberg wrote:

quoted

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.

quoted

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.

Thanks. Sagi, I updated your first patch as follows:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=d06f713e5d200959cdb445a0104e71d9e6070c51

and this is now head of for-linus.

-- 
Jens Axboe

Thanks. Sagi, I updated your first patch as follows:

http://git.kernel.dk/cgit/linux-block/commit/?h=for-linus&id=d06f713e5d200959cdb445a0104e71d9e6070c51

and this is now head of for-linus.

thanks.