| Message ID | 1544067160-20564-4-git-send-email-jianchao.w.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blk-mq: refactor code of issue directly | expand |
On 12/5/18 8:32 PM, Jianchao Wang wrote: > It is not necessary to issue request directly with bypass 'true' > in blk_mq_sched_insert_requests and handle the non-issued requests > itself. Just set bypass to 'false' and let blk_mq_try_issue_directly > handle them totally. Remove the blk_rq_can_direct_dispatch check, > because blk_mq_try_issue_directly can handle it well. > > With respect to commit_rqs hook, we only need to care about the last > request's result. If it is inserted, invoke commit_rqs. I don't think there's anything wrong, functionally, with this patch, but I question the logic of continuing to attempt direct dispatch if we fail one. If we get busy on one, for instance, we should just insert that one to the dispatch list, and insert the rest of the list normally.
On 12/6/18 11:19 PM, Jens Axboe wrote: > On 12/5/18 8:32 PM, Jianchao Wang wrote: >> It is not necessary to issue request directly with bypass 'true' >> in blk_mq_sched_insert_requests and handle the non-issued requests >> itself. Just set bypass to 'false' and let blk_mq_try_issue_directly >> handle them totally. Remove the blk_rq_can_direct_dispatch check, >> because blk_mq_try_issue_directly can handle it well. >> >> With respect to commit_rqs hook, we only need to care about the last >> request's result. If it is inserted, invoke commit_rqs. > > I don't think there's anything wrong, functionally, with this patch, > but I question the logic of continuing to attempt direct dispatch > if we fail one. If we get busy on one, for instance, we should just > insert that one to the dispatch list, and insert the rest of the list > normally. > > It is OK for me to stop to attempt direct dispatch and insert all of the rest when meet the non-ok case. Thanks Jianchao
On 12/6/18 6:16 PM, jianchao.wang wrote: > > > On 12/6/18 11:19 PM, Jens Axboe wrote: >> On 12/5/18 8:32 PM, Jianchao Wang wrote: >>> It is not necessary to issue request directly with bypass 'true' >>> in blk_mq_sched_insert_requests and handle the non-issued requests >>> itself. Just set bypass to 'false' and let blk_mq_try_issue_directly >>> handle them totally. Remove the blk_rq_can_direct_dispatch check, >>> because blk_mq_try_issue_directly can handle it well. >>> >>> With respect to commit_rqs hook, we only need to care about the last >>> request's result. If it is inserted, invoke commit_rqs. >> >> I don't think there's anything wrong, functionally, with this patch, >> but I question the logic of continuing to attempt direct dispatch >> if we fail one. If we get busy on one, for instance, we should just >> insert that one to the dispatch list, and insert the rest of the list >> normally. >> >> > It is OK for me to stop to attempt direct dispatch and insert all of the > rest when meet the non-ok case. Great, let's do that then, I think that makes more sense. The usual case of not being able to dispatch is resource limited, and for that case we'd just be wasting our time continuing to attempt direct dispatch.
diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c index f096d898..5b4d52d 100644 --- a/block/blk-mq-sched.c +++ b/block/blk-mq-sched.c @@ -417,12 +417,10 @@ void blk_mq_sched_insert_requests(struct blk_mq_hw_ctx *hctx, * busy in case of 'none' scheduler, and this way may save * us one extra enqueue & dequeue to sw queue. */ - if (!hctx->dispatch_busy && !e && !run_queue_async) { + if (!hctx->dispatch_busy && !e && !run_queue_async) blk_mq_try_issue_list_directly(hctx, list); - if (list_empty(list)) - return; - } - blk_mq_insert_requests(hctx, ctx, list); + else + blk_mq_insert_requests(hctx, ctx, list); } blk_mq_run_hw_queue(hctx, run_queue_async); diff --git a/block/blk-mq.c b/block/blk-mq.c index a1cccdd..dd07fe1 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1896,32 +1896,24 @@ blk_status_t blk_mq_request_issue_directly(struct request *rq, bool last) void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx, struct list_head *list) { + blk_qc_t unused; + blk_status_t ret = BLK_STS_OK; + while (!list_empty(list)) { - blk_status_t ret; struct request *rq = list_first_entry(list, struct request, queuelist); - if (!blk_rq_can_direct_dispatch(rq)) - break; - list_del_init(&rq->queuelist); - ret = blk_mq_request_issue_directly(rq, list_empty(list)); - if (ret != BLK_STS_OK) { - if (ret == BLK_STS_RESOURCE || - ret == BLK_STS_DEV_RESOURCE) { - list_add(&rq->queuelist, list); - break; - } - blk_mq_end_request(rq, ret); - } + ret = blk_mq_try_issue_directly(hctx, rq, &unused, false, + list_empty(list)); } /* - * If we didn't flush the entire list, we could have told - * the driver there was more coming, but that turned out to - * be a lie. + * We only need to care about the last request's result, + * if it is inserted, kick the hardware with commit_rqs hook. */ - if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs) + if ((ret == BLK_STS_RESOURCE || ret == BLK_STS_DEV_RESOURCE) && + hctx->queue->mq_ops->commit_rqs) hctx->queue->mq_ops->commit_rqs(hctx); }
It is not necessary to issue request directly with bypass 'true' in blk_mq_sched_insert_requests and handle the non-issued requests itself. Just set bypass to 'false' and let blk_mq_try_issue_directly handle them totally. Remove the blk_rq_can_direct_dispatch check, because blk_mq_try_issue_directly can handle it well. With respect to commit_rqs hook, we only need to care about the last request's result. If it is inserted, invoke commit_rqs. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- block/blk-mq-sched.c | 8 +++----- block/blk-mq.c | 26 +++++++++----------------- 2 files changed, 12 insertions(+), 22 deletions(-)