| Message ID | 1501859062-11120-6-git-send-email-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2017-08-04 at 09:04 -0600, Jens Axboe wrote: > @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, > return; > > /* > - * Count as inflight if it either matches the partition we asked > - * for, or if it's the root > + * Count as inflight if it matches the partition, count separately > + * (but all) if we got asked for the root > */ > - if (rq->part == mi->part || mi->part->partno) > + if (rq->part == mi->part) > mi->inflight[0]++; > + if (mi->part->partno) > + mi->inflight[1]++; > } Hello Jens, Same question here: should "if (mi->part->partno)" perhaps be changed into "if (mi->part->partno == 0)"? Thanks, Bart.
On 08/04/2017 01:44 PM, Bart Van Assche wrote: > On Fri, 2017-08-04 at 09:04 -0600, Jens Axboe wrote: >> @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, >> return; >> >> /* >> - * Count as inflight if it either matches the partition we asked >> - * for, or if it's the root >> + * Count as inflight if it matches the partition, count separately >> + * (but all) if we got asked for the root >> */ >> - if (rq->part == mi->part || mi->part->partno) >> + if (rq->part == mi->part) >> mi->inflight[0]++; >> + if (mi->part->partno) >> + mi->inflight[1]++; >> } > > Hello Jens, > > Same question here: should "if (mi->part->partno)" perhaps be changed into > "if (mi->part->partno == 0)"? It should be correct as-is.
On Fri, 2017-08-04 at 13:56 -0600, Jens Axboe wrote: > On 08/04/2017 01:44 PM, Bart Van Assche wrote: > > On Fri, 2017-08-04 at 09:04 -0600, Jens Axboe wrote: > > > @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, > > > return; > > > > > > /* > > > - * Count as inflight if it either matches the partition we asked > > > - * for, or if it's the root > > > + * Count as inflight if it matches the partition, count separately > > > + * (but all) if we got asked for the root > > > */ > > > - if (rq->part == mi->part || mi->part->partno) > > > + if (rq->part == mi->part) > > > mi->inflight[0]++; > > > + if (mi->part->partno) > > > + mi->inflight[1]++; > > > } > > > > Hello Jens, > > > > Same question here: should "if (mi->part->partno)" perhaps be changed into > > "if (mi->part->partno == 0)"? > > It should be correct as-is. Agreed, I got misled by the comment above the code. Bart.
On Fri, Aug 04, 2017 at 09:04:21AM -0600, Jens Axboe wrote: > Modify blk_mq_in_flight() to count both a partition and root at > the same time. Then we only have to call it once, instead of > potentially looping the tags twice. Reviewed-by: Omar Sandoval <osandov@fb.com> One comment below. > Signed-off-by: Jens Axboe <axboe@kernel.dk> [snip] > diff --git a/block/blk-mq.c b/block/blk-mq.c > index fe1aa1f5f069..410ed246bc9b 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, > return; > > /* > - * Count as inflight if it either matches the partition we asked > - * for, or if it's the root > + * Count as inflight if it matches the partition, count separately > + * (but all) if we got asked for the root > */ > - if (rq->part == mi->part || mi->part->partno) > + if (rq->part == mi->part) > mi->inflight[0]++; Similar concern as with patch 3, why special case the part0 case below? > + if (mi->part->partno) > + mi->inflight[1]++; > }
On 08/08/2017 04:48 PM, Omar Sandoval wrote: > On Fri, Aug 04, 2017 at 09:04:21AM -0600, Jens Axboe wrote: >> Modify blk_mq_in_flight() to count both a partition and root at >> the same time. Then we only have to call it once, instead of >> potentially looping the tags twice. > > Reviewed-by: Omar Sandoval <osandov@fb.com> > > One comment below. > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > [snip] > >> diff --git a/block/blk-mq.c b/block/blk-mq.c >> index fe1aa1f5f069..410ed246bc9b 100644 >> --- a/block/blk-mq.c >> +++ b/block/blk-mq.c >> @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, >> return; >> >> /* >> - * Count as inflight if it either matches the partition we asked >> - * for, or if it's the root >> + * Count as inflight if it matches the partition, count separately >> + * (but all) if we got asked for the root >> */ >> - if (rq->part == mi->part || mi->part->partno) >> + if (rq->part == mi->part) >> mi->inflight[0]++; > > Similar concern as with patch 3, why special case the part0 case below? Not sure I follow, both are initialized for this case. Or do you mean the increment? The comment isn't great, I should update that. Basically we want to increment [1] if this is a partition, and increment [0] if it matches the one that was asked for.
On Tue, Aug 08, 2017 at 05:47:15PM -0600, Jens Axboe wrote: > On 08/08/2017 04:48 PM, Omar Sandoval wrote: > > On Fri, Aug 04, 2017 at 09:04:21AM -0600, Jens Axboe wrote: > >> Modify blk_mq_in_flight() to count both a partition and root at > >> the same time. Then we only have to call it once, instead of > >> potentially looping the tags twice. > > > > Reviewed-by: Omar Sandoval <osandov@fb.com> > > > > One comment below. > > > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > > > > [snip] > > > >> diff --git a/block/blk-mq.c b/block/blk-mq.c > >> index fe1aa1f5f069..410ed246bc9b 100644 > >> --- a/block/blk-mq.c > >> +++ b/block/blk-mq.c > >> @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, > >> return; > >> > >> /* > >> - * Count as inflight if it either matches the partition we asked > >> - * for, or if it's the root > >> + * Count as inflight if it matches the partition, count separately > >> + * (but all) if we got asked for the root > >> */ > >> - if (rq->part == mi->part || mi->part->partno) > >> + if (rq->part == mi->part) > >> mi->inflight[0]++; > > > > Similar concern as with patch 3, why special case the part0 case below? > > Not sure I follow, both are initialized for this case. Or do you mean > the increment? Yeah I mean the increment. If I'm reading this right, for the !part0 case, inflight[0] is the count of in-flight requests for the partition and inflight[1] is the count of in-flight requests for the whole device. For the part0 case, inflight[0] is the count of in-flight requests for the root device and inflight[1] is always 0. Can we make inflight[1] the same in the part0 and !part0 cases? > The comment isn't great, I should update that. Basically > we want to increment [1] if this is a partition, and increment [0] if it > matches the one that was asked for. > > -- > Jens Axboe >
On 08/09/2017 01:14 AM, Omar Sandoval wrote: > On Tue, Aug 08, 2017 at 05:47:15PM -0600, Jens Axboe wrote: >> On 08/08/2017 04:48 PM, Omar Sandoval wrote: >>> On Fri, Aug 04, 2017 at 09:04:21AM -0600, Jens Axboe wrote: >>>> Modify blk_mq_in_flight() to count both a partition and root at >>>> the same time. Then we only have to call it once, instead of >>>> potentially looping the tags twice. >>> >>> Reviewed-by: Omar Sandoval <osandov@fb.com> >>> >>> One comment below. >>> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >>> >>> [snip] >>> >>>> diff --git a/block/blk-mq.c b/block/blk-mq.c >>>> index fe1aa1f5f069..410ed246bc9b 100644 >>>> --- a/block/blk-mq.c >>>> +++ b/block/blk-mq.c >>>> @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, >>>> return; >>>> >>>> /* >>>> - * Count as inflight if it either matches the partition we asked >>>> - * for, or if it's the root >>>> + * Count as inflight if it matches the partition, count separately >>>> + * (but all) if we got asked for the root >>>> */ >>>> - if (rq->part == mi->part || mi->part->partno) >>>> + if (rq->part == mi->part) >>>> mi->inflight[0]++; >>> >>> Similar concern as with patch 3, why special case the part0 case below? >> >> Not sure I follow, both are initialized for this case. Or do you mean >> the increment? > > Yeah I mean the increment. If I'm reading this right, for the !part0 > case, inflight[0] is the count of in-flight requests for the partition > and inflight[1] is the count of in-flight requests for the whole device. > For the part0 case, inflight[0] is the count of in-flight requests for > the root device and inflight[1] is always 0. Can we make inflight[1] the > same in the part0 and !part0 cases? Not sure that would change much. [0] is indeed the exact partition. That's always the case. [1] is always the root, except if [0] is. We just don't want to count it twice. So I'd argue they are the same :-)
diff --git a/block/blk-core.c b/block/blk-core.c index 6ad2b8602c1d..d836c84ad3da 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1470,17 +1470,12 @@ static void add_acct_request(struct request_queue *q, struct request *rq, } static void part_round_stats_single(struct request_queue *q, int cpu, - struct hd_struct *part, unsigned long now) + struct hd_struct *part, unsigned long now, + unsigned int inflight) { - int inflight[2]; - - if (now == part->stamp) - return; - - part_in_flight(q, part, inflight); - if (inflight[0]) { + if (inflight) { __part_stat_add(cpu, part, time_in_queue, - inflight[0] * (now - part->stamp)); + inflight * (now - part->stamp)); __part_stat_add(cpu, part, io_ticks, (now - part->stamp)); } part->stamp = now; @@ -1505,12 +1500,29 @@ static void part_round_stats_single(struct request_queue *q, int cpu, */ void part_round_stats(struct request_queue *q, int cpu, struct hd_struct *part) { + struct hd_struct *part2 = NULL; unsigned long now = jiffies; + unsigned int inflight[2]; + int stats = 0; + + if (part->stamp != now) + stats |= 1; + + if (part->partno) { + part2 = &part_to_disk(part)->part0; + if (part2->stamp != now) + stats |= 2; + } + + if (!stats) + return; + + part_in_flight(q, part, inflight); - if (part->partno) - part_round_stats_single(q, cpu, &part_to_disk(part)->part0, - now); - part_round_stats_single(q, cpu, part, now); + if (stats & 2) + part_round_stats_single(q, cpu, part2, now, inflight[1]); + if (stats & 1) + part_round_stats_single(q, cpu, part, now, inflight[0]); } EXPORT_SYMBOL_GPL(part_round_stats); diff --git a/block/blk-mq.c b/block/blk-mq.c index fe1aa1f5f069..410ed246bc9b 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -98,11 +98,13 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx, return; /* - * Count as inflight if it either matches the partition we asked - * for, or if it's the root + * Count as inflight if it matches the partition, count separately + * (but all) if we got asked for the root */ - if (rq->part == mi->part || mi->part->partno) + if (rq->part == mi->part) mi->inflight[0]++; + if (mi->part->partno) + mi->inflight[1]++; } void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, @@ -110,7 +112,7 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part, { struct mq_inflight mi = { .part = part, .inflight = inflight, }; - inflight[0] = 0; + inflight[0] = inflight[1] = 0; blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi); }
Modify blk_mq_in_flight() to count both a partition and root at the same time. Then we only have to call it once, instead of potentially looping the tags twice. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- block/blk-core.c | 38 +++++++++++++++++++++++++------------- block/blk-mq.c | 10 ++++++---- 2 files changed, 31 insertions(+), 17 deletions(-)