On Thu, Jan 26, 2017 at 01:59:23PM -0700, Jens Axboe wrote:

On 01/26/2017 01:54 PM, Omar Sandoval wrote:

quoted

On Thu, Jan 26, 2017 at 12:48:18PM -0700, Jens Axboe wrote:

quoted

When we invoke dispatch_requests(), the scheduler empties everything
into the passed in list. This isn't always a good thing, since it
means that we remove items that we could have potentially merged
with.

Change the function to dispatch single requests at the time. If
we do that, we can backoff exactly at the point where the device
can't consume more IO, and leave the rest with the scheduler for
better merging and future dispatch decision making.

Hmm, I think I previously changed this from ->dispatch_request() to
->dispatch_requests() to support schedulers using software queues. My
current mq-token stuff doesn't have a ->dispatch_requests() hook anymore
after the sched_tags rework, but I think I'm going to need it again
soon. Having the scheduler do blk_mq_flush_busy_ctxs() into its own
private list and then handing the requests out one-by-one kinda sucks.
(Plus, deferred issue wouldn't work with this, but it's not implemented,
anyways :)

One idea: what if we have the scheduler get the driver tags inside of
its ->dispatch_requests()? For example, __dd_dispatch_request() could
first check whether it has a request to dispatch and then try to grab a
driver tag. If it succeeds, it dispatches the request, and if it
doesn't, it marks itself as needing restart.

With that, the scheduler will only return requests ready for
->queue_rq(), meaning we could get rid of the list reshuffling in
blk_mq_dispatch_rq_list().

That'd work for the driver tags, but it would not work for the case
where the driver returns BUSY. So we'd only be covering some of the
cases. That may or may not matter... Hard to say.

Right, I didn't think of that case.

I appreciate having
the hook that dispatches them all for efficiency reasons, but from a
scheduler point of view, sending off one is generally simpler and it'll
be better for rotational storage since we depend so heavily on merging
to get good performance there.

I'm definitely open to alternatives. We can keep the dispatch_requests()
and pass in a dispatch count, ramping up the dispatch count or something
like that. Seems a bit fragile though, and hard to get right.

Yeah, beyond having a way to shove requests back into the scheduler, I
can't think of a good way to reconcile it. I guess we can go with this,
and I'll figure something out for the software queue case.

Begrudgingly-reviewed-by: Omar Sandoval [off-list ref]