| Message ID | 1543995842-20704-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 12:44 AM, Jianchao Wang wrote: > diff --git a/block/blk-mq.c b/block/blk-mq.c > index fe92e52..0dfa269 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -1899,32 +1899,23 @@ 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 cookie = BLK_QC_T_INVALID; > + I'm fine with adding this, but I think we need some sort of check for that not being a valid cookie. That isn't new, we really should have already. > 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); > - } > + blk_mq_try_issue_directly(hctx, rq, &cookie, false, > + list_empty(list)); Indent the list_empty() one more tab, should be after the ( if possible. > - * 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. > + * cookie is set to a valid value only when reqeust is issued successfully. > + * We only need to care about the last request's result, if it is inserted, > + * kick the hardware with commit_rqs hook. reqeust -> request Also lines are too long, limit to 80 chars please. And why aren't we just using the list_empty() check like before, and not having to add the inval cookie value? > - if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs) > + if ((cookie == BLK_QC_T_INVALID) && hctx->queue->mq_ops->commit_rqs) > hctx->queue->mq_ops->commit_rqs(hctx); Redundant parens around the cookie check.
Hi Jens On 12/6/18 12:30 AM, Jens Axboe wrote: > On 12/5/18 12:44 AM, Jianchao Wang wrote: >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index fe92e52..0dfa269 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -1899,32 +1899,23 @@ 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 cookie = BLK_QC_T_INVALID; >> + > > I'm fine with adding this, but I think we need some sort of check for > that not being a valid cookie. That isn't new, we really should have > already. > >> 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); >> - } >> + blk_mq_try_issue_directly(hctx, rq, &cookie, false, >> + list_empty(list)); > > Indent the list_empty() one more tab, should be after the ( if possible. Yes, I will do it > >> - * 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. >> + * cookie is set to a valid value only when reqeust is issued successfully. >> + * We only need to care about the last request's result, if it is inserted, >> + * kick the hardware with commit_rqs hook. > > reqeust -> request > > Also lines are too long, limit to 80 chars please. Yes, I will do it. > > And why aren't we just using the list_empty() check like before, and not > having to add the inval cookie value? Because we use 'bypass == false' here, so blk_mq_try_issue_directly will take over the request totally, so the request will always be removed from the list and finally, the list must be empty. There is another way to identify the result of blk_mq_try_issue_directly. Currently, for the 'bypass == true' case, it always return BLK_STS_OK, for the 'bypass == false' case, it return the actual result, except for 'force == true' case where the request has to be inserted into hctx dispatch list and return a BLK_STS_OK. We could let the 'bypass == true' case also return the actual result to show what has been done in the blk_mq_try_issue_directly and thus we could get the actual result of the last request. Would you mind we handle it like this ? >> - if (!list_empty(list) && hctx->queue->mq_ops->commit_rqs) >> + if ((cookie == BLK_QC_T_INVALID) && hctx->queue->mq_ops->commit_rqs) >> hctx->queue->mq_ops->commit_rqs(hctx); > > Redundant parens around the cookie check. > Yes. Thanks Jianchao
On 12/5/18 6:11 PM, jianchao.wang wrote: >> And why aren't we just using the list_empty() check like before, and not >> having to add the inval cookie value? > > Because we use 'bypass == false' here, so blk_mq_try_issue_directly > will take over the request totally, so the request will always be > removed from the list and finally, the list must be empty. > > There is another way to identify the result of blk_mq_try_issue_directly. > Currently, > for the 'bypass == true' case, > it always return BLK_STS_OK, > for the 'bypass == false' case, > it return the actual result, except for 'force == true' case > where the request has to be inserted into hctx dispatch list > and return a BLK_STS_OK. > > We could let the 'bypass == true' case also return the actual result to > show what has been done in the blk_mq_try_issue_directly and thus we could > get the actual result of the last request. > > Would you mind we handle it like this ? I like that, sounds better than adding a new qc type.
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 fe92e52..0dfa269 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -1899,32 +1899,23 @@ 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 cookie = BLK_QC_T_INVALID; + 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); - } + blk_mq_try_issue_directly(hctx, rq, &cookie, 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. + * cookie is set to a valid value only when reqeust is issued successfully. + * 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 ((cookie == BLK_QC_T_INVALID) && hctx->queue->mq_ops->commit_rqs) hctx->queue->mq_ops->commit_rqs(hctx); } diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index c0ba1a0..a0a467a41 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h @@ -414,6 +414,7 @@ static inline int op_stat_group(unsigned int op) } typedef unsigned int blk_qc_t; +#define BLK_QC_T_INVALID -2U #define BLK_QC_T_NONE -1U #define BLK_QC_T_SHIFT 16 #define BLK_QC_T_INTERNAL (1U << 31)
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. We identify the actual result of blk_mq_try_issue_directly with outputed cookie. Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com> --- block/blk-mq-sched.c | 8 +++----- block/blk-mq.c | 25 ++++++++----------------- include/linux/blk_types.h | 1 + 3 files changed, 12 insertions(+), 22 deletions(-)