Re: [RFC PATCH 4/4 v2] dm: delay running the queue slightly during request completion

[Mike Snitzer <snitzer@redhat.com>]

On Tue, Feb 24 2015 at 12:52pm -0500,
Jens Axboe <axboe@kernel.dk> wrote:

> On 02/24/2015 09:22 AM, Mike Snitzer wrote:
> >On Tue, Feb 24 2015 at 11:51P -0500,
> >Jens Axboe <axboe@kernel.dk> 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@redhat.com>
> >>>---
> >>>  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@redhat.com>
> >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@redhat.com>
> >---
> >  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.
 
> 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?