On 02/24/2015 10:12 AM, Mike Snitzer wrote:
On Tue, Feb 24 2015 at 12:52pm -0500,
Jens Axboe <axboe@xxxxxxxxx> wrote:
On 02/24/2015 09:22 AM, Mike Snitzer wrote:
On Tue, Feb 24 2015 at 11:51P -0500,
Jens Axboe <axboe@xxxxxxxxx> wrote:
On 02/24/2015 08:44 AM, Mike Snitzer wrote:
On really fast storage it can be beneficial to delay running the
request_queue to allow the elevator more opportunity to merge requests.
Otherwise, it has been observed that requests are being sent to
q->request_fn much quicker than is ideal on IOPS-bound backends.
Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
---
drivers/md/dm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fc92899..92091e0 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1034,7 +1034,7 @@ static void rq_completed(struct mapped_device *md, int rw, bool run_queue)
* queue lock again.
*/
if (run_queue)
- blk_run_queue_async(md->queue);
+ blk_delay_queue(md->queue, HZ / 10);
This looks dangerous... How will this impact sync IO? Heuristics like this
will always come back and bite you in the ass.
A slightly more friendly heuristic might be to delay running the queue, if
you still have pending IO. That would give you a more sawtooth like queue
depth management, so it would potentially slow down a bit, but the upside
would be more efficient merging since it would allow some requests so sit a
little bit before being dispatched.
OK, thanks for the suggestion, sending RFC patches FTW:
From: Mike Snitzer <snitzer@xxxxxxxxxx>
Subject: [PATCH] dm: delay running the queue slightly during request
completion
On really fast storage it can be beneficial to delay running the
request_queue to allow the elevator more opportunity to merge requests.
Otherwise, it has been observed that requests are being sent to
q->request_fn much quicker than is ideal on IOPS-bound backends.
To avoid impacting sync IO, the delay when running the queue is only
used if there is pending IO. As Jens put it when suggesting this
heuristic:
"That would give you a more sawtooth like queue depth management, so it
would potentially slow down a bit, but the upside would be more
efficient merging since it would allow some requests to sit a little
bit before being dispatched."
Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx>
---
drivers/md/dm.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index fc92899..85b8919 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -1033,8 +1033,12 @@ static void rq_completed(struct mapped_device *md, int rw, bool run_queue)
* back into ->request_fn() could deadlock attempting to grab the
* queue lock again.
*/
- if (run_queue)
- blk_run_queue_async(md->queue);
+ if (run_queue) {
+ if (md->queue->nr_pending)
+ blk_delay_queue(md->queue, HZ / 10);
+ else
+ blk_run_queue_async(md->queue);
+ }
So all of this needs to be tested and performance vetted. But my
original suggestion was something like:
if (run_queue && !md->queue->nr_pending)
blk_run_queue_async(md->queue);
which might be a bit extreme, but if we hit 0, that's the only case
where you truly do need to run the queue. So that kind of logic
would give you the highest chance of merge success, potentially at
the cost of reduced performance for other cases.
Yeah, I was wondering about not running the queue at all when discussing
with Jeff earlier today. Seemed extreme, and Jeff thought it could
cause performance to really take a hit.
Whether you have to or not depends on how you break out of queueing
loops. But you definitely don't have to run it on every single request
completion...
That aside, where did you pull this ->nr_pending from? I think you
need to look at that again...
Um, as in q->nr_pending doesn't reflect the number of pending requests?
I looked at blk-core.c, saw nr_pending and ran with it...
But looking closer it is only used if CONFIG_PM. SO what is the right
way to check for pending requests on the queue?
You probably want to track it yourself. You could look at the request
counters, but that would introduce a dependency on the old request path.
--
Jens Axboe
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
