* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Omar Sandoval @ 2017-02-27 18:30 UTC (permalink / raw)
To: Jens Axboe; +Cc: Sagi Grimberg, linux-block, kernel-team
In-Reply-To: <6137d2c6-816e-83b2-103c-cdf0840f49fd@fb.com>
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.
^ permalink raw reply
* Re: [PATCH 1/2] lightnvm: add generic ocssd detection
From: Sagi Grimberg @ 2017-02-27 18:35 UTC (permalink / raw)
To: Christoph Hellwig, Matias Bjørling
Cc: linux-block, linux-kernel, linux-nvme
In-Reply-To: <20170226064701.GA7557@infradead.org>
> [adding linux-nvme to Cc as the patch changes the nvme driver, despite
> the subject line]
>
> On Sat, Feb 25, 2017 at 08:16:04PM +0100, Matias Bj=F8rling wrote:
>> On 02/25/2017 07:21 PM, Christoph Hellwig wrote:
>>> On Fri, Feb 24, 2017 at 06:16:48PM +0100, Matias Bj=F8rling wrote:
>>>> More implementations of OCSSDs are becoming available. Adding each usi=
ng
>>>> pci ids are becoming a hassle. Instead, use a 16 byte string in the
>>>> vendor-specific area of the identification command to identify an
>>>> Open-Channel SSD.
>>>>
>>>> The large string should make the collision probability with other
>>>> vendor-specific strings to be near nil.
>>>
>>> No way in hell. vs is vendor specific and we absolutely can't overload
>>> it with any sort of meaning. Get OCSSD support properly standardized a=
nd
>>> add a class code for it. Until then it's individual PCI IDs.
>>>
>>
>> You are right, that is the right way to go, and we are working on it. In=
the
>> meantime, there are a couple of reasons I want to do a pragmatic solutio=
n:
>
> Reasonable reaosons, but that's just not how standard interfaces work.
> Either you standardize the behaviour and have a standardized trigger
> for it, or it is vendor specific and needs to be keyed off a specific
> vendor/device identification.
I agree, I don't see how we're allowed to use vs for that.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Omar Sandoval @ 2017-02-27 17:53 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg, linux-block; +Cc: kernel-team
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
[-- Attachment #1: Type: text/plain, Size: 809 bytes --]
On Mon, Feb 27, 2017 at 09:47:53AM -0800, 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.
>
> 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.
[-- Attachment #2: 0001-blk-mq-make-blk_mq_alloc_request_hctx-allocate-a-sch.patch --]
[-- Type: text/x-diff, Size: 3435 bytes --]
>From dcfe49f597dc1eec3b326024c86a6ad3afb82fa8 Mon Sep 17 00:00:00 2001
Message-Id: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
Date: Mon, 27 Feb 2017 09:34:20 -0800
Subject: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a
scheduler request
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?
block/blk-mq-sched.c | 11 +++++------
block/blk-mq.c | 34 +++++++++++++++-------------------
2 files changed, 20 insertions(+), 25 deletions(-)
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);
--
2.12.0
[-- Attachment #3: 0002-blk-mq-kill-blk_mq_set_alloc_data.patch --]
[-- Type: text/x-diff, Size: 1227 bytes --]
>From 87aa35aa12f2edd4a9c56c382bf85ee580edf156 Mon Sep 17 00:00:00 2001
Message-Id: <87aa35aa12f2edd4a9c56c382bf85ee580edf156.1488217516.git.osandov@fb.com>
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
References: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
Date: Mon, 27 Feb 2017 09:38:58 -0800
Subject: [PATCH 2/3] blk-mq: kill blk_mq_set_alloc_data()
Nothing is using it anymore.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 24b2256186f3..088ced003c13 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -146,16 +146,6 @@ struct blk_mq_alloc_data {
struct blk_mq_hw_ctx *hctx;
};
-static inline void blk_mq_set_alloc_data(struct blk_mq_alloc_data *data,
- struct request_queue *q, unsigned int flags,
- struct blk_mq_ctx *ctx, struct blk_mq_hw_ctx *hctx)
-{
- data->q = q;
- data->flags = flags;
- data->ctx = ctx;
- data->hctx = hctx;
-}
-
static inline struct blk_mq_tags *blk_mq_tags_from_data(struct blk_mq_alloc_data *data)
{
if (data->flags & BLK_MQ_REQ_INTERNAL)
--
2.12.0
[-- Attachment #4: 0003-blk-mq-move-update-of-tags-rqs-to-__blk_mq_alloc_req.patch --]
[-- Type: text/x-diff, Size: 1611 bytes --]
>From 4715cce107df8b579734a074093a34001efd01d0 Mon Sep 17 00:00:00 2001
Message-Id: <4715cce107df8b579734a074093a34001efd01d0.1488217516.git.osandov@fb.com>
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
References: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
Date: Mon, 27 Feb 2017 09:40:34 -0800
Subject: [PATCH 3/3] blk-mq: move update of tags->rqs to
__blk_mq_alloc_request()
No functional difference, it just makes a little more sense to update
the tag map where we actually allocate the tag.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Optional cleanup, since there's only one place that does this.
block/blk-mq-sched.c | 2 --
block/blk-mq.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 5697b23412a1..09af8ff18719 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -134,8 +134,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 cc9713f574a5..452c1caf978f 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);
--
2.12.0
^ permalink raw reply related
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Sagi Grimberg @ 2017-02-27 18:02 UTC (permalink / raw)
To: Omar Sandoval, Jens Axboe, linux-block; +Cc: kernel-team
In-Reply-To: <20170227175351.GF10715@vader>
>> 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.
^ permalink raw reply
* Re: [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Jens Axboe @ 2017-02-27 18:07 UTC (permalink / raw)
To: Omar Sandoval, Sagi Grimberg, linux-block; +Cc: kernel-team
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
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.
--
Jens Axboe
^ permalink raw reply
* [PATCH 2/3] blk-mq: kill blk_mq_set_alloc_data()
From: Omar Sandoval @ 2017-02-27 17:47 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg, linux-block; +Cc: kernel-team
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
Nothing is using it anymore.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
block/blk-mq.h | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 24b2256186f3..088ced003c13 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -146,16 +146,6 @@ struct blk_mq_alloc_data {
struct blk_mq_hw_ctx *hctx;
};
-static inline void blk_mq_set_alloc_data(struct blk_mq_alloc_data *data,
- struct request_queue *q, unsigned int flags,
- struct blk_mq_ctx *ctx, struct blk_mq_hw_ctx *hctx)
-{
- data->q = q;
- data->flags = flags;
- data->ctx = ctx;
- data->hctx = hctx;
-}
-
static inline struct blk_mq_tags *blk_mq_tags_from_data(struct blk_mq_alloc_data *data)
{
if (data->flags & BLK_MQ_REQ_INTERNAL)
--
2.12.0
^ permalink raw reply related
* [PATCH 3/3] blk-mq: move update of tags->rqs to __blk_mq_alloc_request()
From: Omar Sandoval @ 2017-02-27 17:47 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg, linux-block; +Cc: kernel-team
In-Reply-To: <dcfe49f597dc1eec3b326024c86a6ad3afb82fa8.1488217516.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
No functional difference, it just makes a little more sense to update
the tag map where we actually allocate the tag.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
Optional cleanup, since there's only one place that does this.
block/blk-mq-sched.c | 2 --
block/blk-mq.c | 1 +
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 5697b23412a1..09af8ff18719 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -134,8 +134,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 cc9713f574a5..452c1caf978f 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);
--
2.12.0
^ permalink raw reply related
* [PATCH 1/3] blk-mq: make blk_mq_alloc_request_hctx() allocate a scheduler request
From: Omar Sandoval @ 2017-02-27 17:47 UTC (permalink / raw)
To: Jens Axboe, Sagi Grimberg, linux-block; +Cc: kernel-team
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.
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?
block/blk-mq-sched.c | 11 +++++------
block/blk-mq.c | 34 +++++++++++++++-------------------
2 files changed, 20 insertions(+), 25 deletions(-)
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);
--
2.12.0
^ permalink raw reply related
* Re: [PATCHv2 2/2] nvme: Complete all stuck requests
From: Sagi Grimberg @ 2017-02-27 17:27 UTC (permalink / raw)
To: Keith Busch
Cc: linux-block, linux-nvme, Jens Axboe, Christoph Hellwig,
Marc MERLIN
In-Reply-To: <20170227150107.GA5789@localhost.localdomain>
>>> If the block layer has entered requests and gets a CPU hot plug event
>>> prior to the resume event, it will wait for those requests to exit. If
>>> the nvme driver is shutting down, it will not start the queues back up,
>>> preventing forward progress.
>>>
>>> To fix that, this patch freezes the request queues when the driver intends
>>> to shut down the controller so that no new requests may enter. After the
>>> controller has been disabled, the queues will be restarted to force all
>>> entered requests to end in failure so that blk-mq's hot cpu notifier may
>>> progress. To ensure the queue usage count is 0 on a shutdown, the driver
>>> waits for freeze to complete before completing the controller shutdown.
>>
>> Keith, can you explain (again) for me why is the freeze_wait must happen
>> after the controller has been disabled, instead of starting the queues
>> and waiting right after freeze start?
>
> Yeah, the driver needs to make forward progress even if the controller
> isn't functioning. If we do the freeze wait before disabling the
> controller, there's no way to reclaim missing completions. If the
> controller is working perfectly, it'd be okay, but the driver would be
> stuck if there's a problem.
OK, I think we can get it for fabrics too, need to figure out how to
handle it there too.
Do you have a reproducer?
^ permalink raw reply
* Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Sagi Grimberg @ 2017-02-27 17:26 UTC (permalink / raw)
To: Jens Axboe, Omar Sandoval; +Cc: linux-block, linux-nvme
In-Reply-To: <7c499dec-b3b3-be6c-c650-c358fb69b391@kernel.dk>
> 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.
^ permalink raw reply
* Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Jens Axboe @ 2017-02-27 17:06 UTC (permalink / raw)
To: Omar Sandoval; +Cc: Sagi Grimberg, linux-block, linux-nvme
In-Reply-To: <20170227170434.GE10715@vader>
On 02/27/2017 10:04 AM, Omar Sandoval wrote:
> 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.
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
^ permalink raw reply
* Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Omar Sandoval @ 2017-02-27 17:04 UTC (permalink / raw)
To: Jens Axboe; +Cc: Sagi Grimberg, linux-block, linux-nvme
In-Reply-To: <ad451231-eca0-3f25-fd82-1db44acc1cd9@kernel.dk>
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.
^ permalink raw reply
* Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Jens Axboe @ 2017-02-27 17:03 UTC (permalink / raw)
To: Omar Sandoval, Sagi Grimberg; +Cc: linux-block, linux-nvme
In-Reply-To: <20170227165959.GD10715@vader>
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.
--
Jens Axboe
^ permalink raw reply
* Re: [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Omar Sandoval @ 2017-02-27 16:59 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <1488209781-1084-2-git-send-email-sagi@grimberg.me>
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);
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 :)
^ permalink raw reply related
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Jens Axboe @ 2017-02-27 16:27 UTC (permalink / raw)
To: Omar Sandoval; +Cc: Sagi Grimberg, linux-block, linux-nvme
In-Reply-To: <20170227162520.GC10715@vader>
On 02/27/2017 09:25 AM, Omar Sandoval wrote:
> On Mon, Feb 27, 2017 at 09:15:27AM -0700, Jens Axboe wrote:
>> On 02/27/2017 09:10 AM, 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.
>>>>
>>>> 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
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Omar Sandoval @ 2017-02-27 16:25 UTC (permalink / raw)
To: Jens Axboe; +Cc: Sagi Grimberg, linux-block, linux-nvme
In-Reply-To: <353bb44b-7180-7a78-47f5-2ebf03b714bc@kernel.dk>
On Mon, Feb 27, 2017 at 09:15:27AM -0700, Jens Axboe wrote:
> On 02/27/2017 09:10 AM, 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.
> >>
> >> 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.
> 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
>
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Sagi Grimberg @ 2017-02-27 16:23 UTC (permalink / raw)
To: Jens Axboe, Omar Sandoval; +Cc: linux-block, linux-nvme
In-Reply-To: <353bb44b-7180-7a78-47f5-2ebf03b714bc@kernel.dk>
>> 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)) {
>
>
Both patches look they'd work, I'll test. Thanks.
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Jens Axboe @ 2017-02-27 16:17 UTC (permalink / raw)
To: Omar Sandoval, Sagi Grimberg; +Cc: linux-block, linux-nvme
In-Reply-To: <20170227161452.GB10715@vader>
On 02/27/2017 09:14 AM, Omar Sandoval wrote:
> On Mon, Feb 27, 2017 at 06:10:01PM +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.
>>>
>>> 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)) {
Agree, that's identical to what I just sent out as well, functionally.
>> 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
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Jens Axboe @ 2017-02-27 16:15 UTC (permalink / raw)
To: Sagi Grimberg, Omar Sandoval; +Cc: linux-block, linux-nvme
In-Reply-To: <eb51d324-62df-629e-8a7b-fea50ef7543e@grimberg.me>
On 02/27/2017 09:10 AM, 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.
>>
>> 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
^ permalink raw reply related
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Omar Sandoval @ 2017-02-27 16:14 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <eb51d324-62df-629e-8a7b-fea50ef7543e@grimberg.me>
On Mon, Feb 27, 2017 at 06:10:01PM +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.
> >
> > 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.
^ permalink raw reply related
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Sagi Grimberg @ 2017-02-27 16:10 UTC (permalink / raw)
To: Jens Axboe, Omar Sandoval; +Cc: linux-block, linux-nvme
In-Reply-To: <4829b70c-53fd-f622-310e-0a30e4ede6fa@kernel.dk>
>> 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...
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Jens Axboe @ 2017-02-27 15:53 UTC (permalink / raw)
To: Omar Sandoval, Sagi Grimberg; +Cc: linux-block, linux-nvme
In-Reply-To: <20170227154939.GA10715@vader>
On 02/27/2017 08:49 AM, Omar Sandoval wrote:
> On Mon, Feb 27, 2017 at 05:36:20PM +0200, Sagi Grimberg wrote:
>> 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
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Omar Sandoval @ 2017-02-27 15:49 UTC (permalink / raw)
To: Sagi Grimberg; +Cc: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <1488209781-1084-1-git-send-email-sagi@grimberg.me>
On Mon, Feb 27, 2017 at 05:36:20PM +0200, Sagi Grimberg wrote:
> 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.
^ permalink raw reply
* Re: [PATCH 1/2] blk-mq-sched: Allocate sched reserved tags as specified in the original queue tagset
From: Jens Axboe @ 2017-02-27 15:38 UTC (permalink / raw)
To: Sagi Grimberg, linux-block, linux-nvme
In-Reply-To: <1488209781-1084-1-git-send-email-sagi@grimberg.me>
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
^ permalink raw reply
* [PATCH 2/2] blk-mq: make sure to back-assign the request to rq_map in blk_mq_alloc_request_hctx
From: Sagi Grimberg @ 2017-02-27 15:36 UTC (permalink / raw)
To: Jens Axboe, linux-block, linux-nvme
In-Reply-To: <1488209781-1084-1-git-send-email-sagi@grimberg.me>
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
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox