Re: [PATCH 5/5] blk-mq-sched: change ->dispatch_requests() to ->dispatch_request()

[Omar Sandoval <osandov@osandov.com>]

On Thu, Jan 26, 2017 at 01:59:23PM -0700, Jens Axboe wrote:
> On 01/26/2017 01:54 PM, Omar Sandoval wrote:
> > On Thu, Jan 26, 2017 at 12:48:18PM -0700, Jens Axboe wrote:
> >> 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 <osandov@fb.com>