Re: [PATCH v2] blk-mq-sched: separate mark hctx and queue restart operations

[Omar Sandoval <osandov@osandov.com>]

On Mon, Feb 06, 2017 at 01:07:41PM -0700, Jens Axboe wrote:
> On 02/06/2017 12:53 PM, Omar Sandoval wrote:
> > On Mon, Feb 06, 2017 at 12:39:57PM -0700, Jens Axboe wrote:
> >> On 02/06/2017 12:24 PM, Omar Sandoval wrote:
> >>> From: Omar Sandoval <osandov@fb.com>
> >>>
> >>> In blk_mq_sched_dispatch_requests(), we call blk_mq_sched_mark_restart()
> >>> after we dispatch requests left over on our hardware queue dispatch
> >>> list. This is so we'll go back and dispatch requests from the scheduler.
> >>> In this case, it's only necessary to restart the hardware queue that we
> >>> are running; there's no reason to run other hardware queues just because
> >>> we are using shared tags.
> >>>
> >>> So, split out blk_mq_sched_mark_restart() into two operations, one for
> >>> just the hardware queue and one for the whole request queue. The core
> >>> code uses both, and I/O schedulers may also want to use them.
> >>>
> >>> Signed-off-by: Omar Sandoval <osandov@fb.com>
> >>> ---
> >>> Patch based on block/for-next.
> >>>
> >>>  block/blk-mq-sched.c |  2 +-
> >>>  block/blk-mq-sched.h | 25 ++++++++++++++++++-------
> >>>  block/blk-mq.c       |  5 ++++-
> >>>  3 files changed, 23 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
> >>> index ee455e7cf9d8..7538565359ea 100644
> >>> --- a/block/blk-mq-sched.c
> >>> +++ b/block/blk-mq-sched.c
> >>> @@ -201,7 +201,7 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx)
> >>>  	 * needing a restart in that case.
> >>>  	 */
> >>>  	if (!list_empty(&rq_list)) {
> >>> -		blk_mq_sched_mark_restart(hctx);
> >>> +		blk_mq_sched_mark_restart_hctx(hctx);
> >>>  		blk_mq_dispatch_rq_list(hctx, &rq_list);
> >>
> >> What if we dispatched nothing on this hardware queue, and it currently
> >> doesn't have any IO pending?
> > 
> > Hm, so there are two ways that could happen. If it's because
> > ->queue_rq() returned BLK_MQ_RQ_QUEUE_BUSY, then the driver is supposed
> > to kick I/O off again, right?
> > 
> > If it's because we failed to get a driver tag, then we'll call
> > blk_mq_sched_mark_restart_queue() in the shared case. I just realized
> > that there's a bug there, though. Since we already set the hctx restart
> > bit, we won't set the queue restart bit. The below should work, and
> > makes more sense in general.
> > 
> > Or were you thinking of something else?
> 
> No, I think that covers it, I had not read far enough either to see that
> you handle the shared tag case for tag starvation in the caller.

Yup, I considered making that its own helper but I figured we could do
that when we need the same logic elsewhere.