| Message ID | 20230111130159.3741753-8-shikemeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | A few bugfix and cleanup patches for blk-mq | expand |
On Wed, Jan 11, 2023 at 09:01:52PM +0800, Kemeng Shi wrote: > 1. rename orignal blk_mq_commit_rqs to blk_mq_plug_commit_rqs as > trace_block_unplug is only needed when we dispatch request from plug list. Why? I think always having the trace even for the commit case seems very useful for making the traces useful. > +/* blk_mq_commit_rqs and blk_mq_plug_commit_rqs notify driver using > + * bd->last that there is no more requests. (See comment in struct This is not the normal kernel comment style. > +static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int queued) > +{ > + if (hctx->queue->mq_ops->commit_rqs && queued) { > + hctx->queue->mq_ops->commit_rqs(hctx); > + } No need for the braces.
Hi Christoph, thank you for taking time to review, alos sorry for the bother of code style problem. I will fix them in next version. on 1/11/2023 1:45 PM, Christoph Hellwig wrote: > On Wed, Jan 11, 2023 at 09:01:52PM +0800, Kemeng Shi wrote: >> 1. rename orignal blk_mq_commit_rqs to blk_mq_plug_commit_rqs as >> trace_block_unplug is only needed when we dispatch request from plug list. > > Why? I think always having the trace even for the commit case seems > very useful for making the traces useful. I think unplug event more likely means that request going to be sent to driver was plugged and in plug list. And the current code do only trace unplug event when dispatching requests from plug list. If so, would it be better to add a new event to trace commit? >> +/* blk_mq_commit_rqs and blk_mq_plug_commit_rqs notify driver using >> + * bd->last that there is no more requests. (See comment in struct > > This is not the normal kernel comment style. > >> +static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int queued) >> +{ >> + if (hctx->queue->mq_ops->commit_rqs && queued) { >> + hctx->queue->mq_ops->commit_rqs(hctx); >> + } > > No need for the braces. >
on 1/11/2023 2:30 PM, Kemeng Shi wrote: > > Hi Christoph, thank you for taking time to review, alos sorry for the bother > of code style problem. I will fix them in next version. > on 1/11/2023 1:45 PM, Christoph Hellwig wrote: >> On Wed, Jan 11, 2023 at 09:01:52PM +0800, Kemeng Shi wrote: >>> 1. rename orignal blk_mq_commit_rqs to blk_mq_plug_commit_rqs as >>> trace_block_unplug is only needed when we dispatch request from plug list. >> >> Why? I think always having the trace even for the commit case seems >> very useful for making the traces useful. > I think unplug event more likely means that request going to be sent to driver > was plugged and in plug list. And the current code do only trace unplug event > when dispatching requests from plug list. If so, would it be better to add > a new event to trace commit? Hi Christoph, which way do you prefer now? Keep unplug event consistent to trace commit of requests from plug list only or trace all commits with unplug event. Please let me know and I will consider it in next version. Thanks. >>> +/* blk_mq_commit_rqs and blk_mq_plug_commit_rqs notify driver using >>> + * bd->last that there is no more requests. (See comment in struct >> >> This is not the normal kernel comment style. >> >>> +static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int queued) >>> +{ >>> + if (hctx->queue->mq_ops->commit_rqs && queued) { >>> + hctx->queue->mq_ops->commit_rqs(hctx); >>> + } >> >> No need for the braces. >> >
On Mon, Jan 16, 2023 at 09:07:00AM +0800, Kemeng Shi wrote: > >> Why? I think always having the trace even for the commit case seems > >> very useful for making the traces useful. > > I think unplug event more likely means that request going to be sent to driver > > was plugged and in plug list. And the current code do only trace unplug event > > when dispatching requests from plug list. If so, would it be better to add > > a new event to trace commit? > Hi Christoph, which way do you prefer now? Keep unplug event consistent to > trace commit of requests from plug list only or trace all commits with > unplug event. Please let me know and I will consider it in next version. > Thanks. To me always having the trace feels more useful, but let's see if Jens has an opinion on it.
On 1/16/23 9:09 AM, Christoph Hellwig wrote: > On Mon, Jan 16, 2023 at 09:07:00AM +0800, Kemeng Shi wrote: >>>> Why? I think always having the trace even for the commit case seems >>>> very useful for making the traces useful. >>> I think unplug event more likely means that request going to be sent to driver >>> was plugged and in plug list. And the current code do only trace unplug event >>> when dispatching requests from plug list. If so, would it be better to add >>> a new event to trace commit? >> Hi Christoph, which way do you prefer now? Keep unplug event consistent to >> trace commit of requests from plug list only or trace all commits with >> unplug event. Please let me know and I will consider it in next version. >> Thanks. > > To me always having the trace feels more useful, but let's see if Jens > has an opinion on it. Agree, that is probably the saner option.
on 1/17/2023 12:13 AM, Jens Axboe wrote: > On 1/16/23 9:09 AM, Christoph Hellwig wrote: >> On Mon, Jan 16, 2023 at 09:07:00AM +0800, Kemeng Shi wrote: >>>>> Why? I think always having the trace even for the commit case seems >>>>> very useful for making the traces useful. >>>> I think unplug event more likely means that request going to be sent to driver >>>> was plugged and in plug list. And the current code do only trace unplug event >>>> when dispatching requests from plug list. If so, would it be better to add >>>> a new event to trace commit? >>> Hi Christoph, which way do you prefer now? Keep unplug event consistent to >>> trace commit of requests from plug list only or trace all commits with >>> unplug event. Please let me know and I will consider it in next version. >>> Thanks. >> >> To me always having the trace feels more useful, but let's see if Jens >> has an opinion on it. > > Agree, that is probably the saner option. > Thanks for replies, I will trace all commits with unplug event in next version.
diff --git a/block/blk-mq.c b/block/blk-mq.c index c6cc3feb3b84..98f6003474f2 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -2007,6 +2007,29 @@ static void blk_mq_release_budgets(struct request_queue *q, } } +/* blk_mq_commit_rqs and blk_mq_plug_commit_rqs notify driver using + * bd->last that there is no more requests. (See comment in struct + * blk_mq_ops for commit_rqs for details) + * Attention, we should explicitly call this in unusual cases: + * 1) did not queue everything initially scheduled to queue + * 2) the last attempt to queue a request failed + */ +static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int queued) +{ + if (hctx->queue->mq_ops->commit_rqs && queued) { + hctx->queue->mq_ops->commit_rqs(hctx); + } +} + +static void blk_mq_plug_commit_rqs(struct blk_mq_hw_ctx *hctx, int *queued) +{ + if (hctx->queue->mq_ops->commit_rqs && *queued) { + trace_block_unplug(hctx->queue, *queued, true); + hctx->queue->mq_ops->commit_rqs(hctx); + } + *queued = 0; +} + /* * Returns true if we did some work AND can potentially do more. */ @@ -2555,15 +2578,6 @@ void blk_mq_insert_requests(struct blk_mq_hw_ctx *hctx, struct blk_mq_ctx *ctx, spin_unlock(&ctx->lock); } -static void blk_mq_commit_rqs(struct blk_mq_hw_ctx *hctx, int *queued) -{ - if (hctx->queue->mq_ops->commit_rqs) { - trace_block_unplug(hctx->queue, *queued, true); - hctx->queue->mq_ops->commit_rqs(hctx); - } - *queued = 0; -} - static void blk_mq_bio_to_request(struct request *rq, struct bio *bio, unsigned int nr_segs) { @@ -2700,7 +2714,7 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug) if (hctx != rq->mq_hctx) { if (hctx) - blk_mq_commit_rqs(hctx, &queued); + blk_mq_plug_commit_rqs(hctx, &queued); hctx = rq->mq_hctx; } @@ -2712,7 +2726,7 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug) case BLK_STS_RESOURCE: case BLK_STS_DEV_RESOURCE: blk_mq_request_bypass_insert(rq, false, true); - blk_mq_commit_rqs(hctx, &queued); + blk_mq_plug_commit_rqs(hctx, &queued); return; default: blk_mq_end_request(rq, ret); @@ -2726,7 +2740,7 @@ static void blk_mq_plug_issue_direct(struct blk_plug *plug) * there was more coming, but that turned out to be a lie. */ if (errors) - blk_mq_commit_rqs(hctx, &queued); + blk_mq_plug_commit_rqs(hctx, &queued); } static void __blk_mq_flush_plug_list(struct request_queue *q,
1. rename orignal blk_mq_commit_rqs to blk_mq_plug_commit_rqs as trace_block_unplug is only needed when we dispatch request from plug list. We need a commit_rqs wrapper for this case. Besides, this patch adds queued check and only commits request if any request was queued to keep commit behavior consistent and remove unnecessary commit. 2. add new blk_mq_commit_rqs for general commits. Current blk_mq_commit_rqs will not clear queued as queued clearing is not wanted generally. 3. document rule for unusual cases which need explicit commit_rqs. Suggested-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Kemeng Shi <shikemeng@huaweicloud.com> --- block/blk-mq.c | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-)