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

[Jens Axboe <axboe@kernel.dk>]

On 02/24/2015 10:12 AM, Mike Snitzer wrote:
> 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.

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