Messages in this thread | | | Date | Fri, 24 Apr 2020 08:15:29 +0200 | From | Christoph Hellwig <> | Subject | Re: [PATCH v2] block: Limit number of items taken from the I/O scheduler in one go |
| |
> index 74cedea56034..b69b780351c1 100644 > --- a/block/blk-mq-sched.c > +++ b/block/blk-mq-sched.c > @@ -84,12 +84,17 @@ void blk_mq_sched_restart(struct blk_mq_hw_ctx *hctx) > * Only SCSI implements .get_budget and .put_budget, and SCSI restarts > * its queue by itself in its completion handler, so we don't need to > * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE. > + * > + * Returns true if hctx->dispatch was found non-empty and > + * run_work has to be run again. This is necessary to avoid > + * starving flushes. > */
Please use up all 80 chars for your comments (just like the existing part of the comment).
> * Only SCSI implements .get_budget and .put_budget, and SCSI restarts > * its queue by itself in its completion handler, so we don't need to > * restart queue if .get_budget() returns BLK_STS_NO_RESOURCE. > + * > + * Returns true if hctx->dispatch was found non-empty and > + * run_work has to be run again. This is necessary to avoid > + * starving flushes.
Same here.
> +again: > + run_again = false; > + > /* > * If we have previous entries on our dispatch list, grab them first for > * more fair dispatch. > @@ -208,19 +236,28 @@ void blk_mq_sched_dispatch_requests(struct blk_mq_hw_ctx *hctx) > blk_mq_sched_mark_restart_hctx(hctx); > if (blk_mq_dispatch_rq_list(q, &rq_list, false)) { > if (has_sched_dispatch) > - blk_mq_do_dispatch_sched(hctx); > + run_again = blk_mq_do_dispatch_sched(hctx); > else > - blk_mq_do_dispatch_ctx(hctx); > + run_again = blk_mq_do_dispatch_ctx(hctx); > } > } else if (has_sched_dispatch) { > - blk_mq_do_dispatch_sched(hctx); > + run_again = blk_mq_do_dispatch_sched(hctx); > } else if (hctx->dispatch_busy) { > /* dequeue request one by one from sw queue if queue is busy */ > - blk_mq_do_dispatch_ctx(hctx); > + run_again = blk_mq_do_dispatch_ctx(hctx); > } else { > blk_mq_flush_busy_ctxs(hctx, &rq_list); > blk_mq_dispatch_rq_list(q, &rq_list, false); > } > + > + if (run_again) { > + if (!restarted) { > + restarted = true; > + goto again; > + } > + > + blk_mq_run_hw_queue(hctx, true); > + }
This is a weird loop. I'd split the code betweem the again label and the run_again check here into a __blk_mq_sched_dispatch_requests helper, and then you can do:
if (__blk_mq_sched_dispatch_requests()) { if (__blk_mq_sched_dispatch_requests()) blk_mq_run_hw_queue(hctx, true); }
here. Preferably with ha good comment explaining the logic.
| |