| Message ID | 1501859062-11120-4-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: > Instead of returning the count that matches the partition, pass > in an array of two ints. Index 0 will be filled with the inflight > count for the partition in question, and index 1 will filled > with the root infligh count, if the partition passed in is not the > root. Hello Jens, It looks like a letter 't' is missing from the patch description ("infligh")? Anyway: Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>
On 08/04/2017 01:38 PM, Bart Van Assche wrote: > On Fri, 2017-08-04 at 09:04 -0600, Jens Axboe wrote: >> Instead of returning the count that matches the partition, pass >> in an array of two ints. Index 0 will be filled with the inflight >> count for the partition in question, and index 1 will filled >> with the root infligh count, if the partition passed in is not the >> root. > > Hello Jens, > > It looks like a letter 't' is missing from the patch description ("infligh")? Oops thanks, fixed up.
On Fri, Aug 04, 2017 at 09:04:19AM -0600, Jens Axboe wrote: > Instead of returning the count that matches the partition, pass > in an array of two ints. Index 0 will be filled with the inflight > count for the partition in question, and index 1 will filled > with the root infligh count, if the partition passed in is not the > root. > > This is in preparation for being able to calculate both in one > go. One tiny comment below, besides that Reviewed-by: Omar Sandoval <osandov@fb.com> > Signed-off-by: Jens Axboe <axboe@kernel.dk> > diff --git a/include/linux/genhd.h b/include/linux/genhd.h > index 7f7427e00f9c..a9c8ea632fdc 100644 > --- a/include/linux/genhd.h > +++ b/include/linux/genhd.h > @@ -378,11 +378,17 @@ static inline void part_dec_in_flight(struct request_queue *q, > atomic_dec(&part_to_disk(part)->part0.in_flight[rw]); > } > > -static inline int part_in_flight(struct request_queue *q, > - struct hd_struct *part) > +static inline void part_in_flight(struct request_queue *q, > + struct hd_struct *part, > + unsigned int inflight[2]) > { > - return atomic_read(&part->in_flight[0]) + > + inflight[0] = atomic_read(&part->in_flight[0]) + > atomic_read(&part->in_flight[1]); It makes me a little nervous here that we only initialize inflight[1] if part is not part0, that seems a little subtle and easy to miss. Can we change the line above to this? inflight[0] = inflight[1] = (atomic_read(&part->in_flight[0]) + atomic_read(&part->in_flight[1])); > + if (part->partno) { > + part = &part_to_disk(part)->part0; > + inflight[1] = atomic_read(&part->in_flight[0]) + > + atomic_read(&part->in_flight[1]); > + } > } > > static inline struct partition_meta_info *alloc_part_info(struct gendisk *disk) > -- > 2.7.4 >
On 08/08/2017 04:42 PM, Omar Sandoval wrote: > On Fri, Aug 04, 2017 at 09:04:19AM -0600, Jens Axboe wrote: >> Instead of returning the count that matches the partition, pass >> in an array of two ints. Index 0 will be filled with the inflight >> count for the partition in question, and index 1 will filled >> with the root infligh count, if the partition passed in is not the >> root. >> >> This is in preparation for being able to calculate both in one >> go. > > One tiny comment below, besides that > > Reviewed-by: Omar Sandoval <osandov@fb.com> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> diff --git a/include/linux/genhd.h b/include/linux/genhd.h >> index 7f7427e00f9c..a9c8ea632fdc 100644 >> --- a/include/linux/genhd.h >> +++ b/include/linux/genhd.h >> @@ -378,11 +378,17 @@ static inline void part_dec_in_flight(struct request_queue *q, >> atomic_dec(&part_to_disk(part)->part0.in_flight[rw]); >> } >> >> -static inline int part_in_flight(struct request_queue *q, >> - struct hd_struct *part) >> +static inline void part_in_flight(struct request_queue *q, >> + struct hd_struct *part, >> + unsigned int inflight[2]) >> { >> - return atomic_read(&part->in_flight[0]) + >> + inflight[0] = atomic_read(&part->in_flight[0]) + >> atomic_read(&part->in_flight[1]); > > It makes me a little nervous here that we only initialize inflight[1] if > part is not part0, that seems a little subtle and easy to miss. Can we > change the line above to this? > > inflight[0] = inflight[1] = (atomic_read(&part->in_flight[0]) + > atomic_read(&part->in_flight[1])); Yes good point, I'll make that change.
diff --git a/block/blk-core.c b/block/blk-core.c index 8ee954c12e9d..6ad2b8602c1d 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1472,15 +1472,15 @@ 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) { - int inflight; + int inflight[2]; if (now == part->stamp) return; - inflight = part_in_flight(q, part); - if (inflight) { + part_in_flight(q, part, inflight); + if (inflight[0]) { __part_stat_add(cpu, part, time_in_queue, - inflight * (now - part->stamp)); + inflight[0] * (now - part->stamp)); __part_stat_add(cpu, part, io_ticks, (now - part->stamp)); } part->stamp = now; diff --git a/block/genhd.c b/block/genhd.c index f735af67a0c9..822f65f95e2a 100644 --- a/block/genhd.c +++ b/block/genhd.c @@ -1204,6 +1204,7 @@ static int diskstats_show(struct seq_file *seqf, void *v) struct disk_part_iter piter; struct hd_struct *hd; char buf[BDEVNAME_SIZE]; + unsigned int inflight[2]; int cpu; /* @@ -1219,6 +1220,7 @@ static int diskstats_show(struct seq_file *seqf, void *v) cpu = part_stat_lock(); part_round_stats(gp->queue, cpu, hd); part_stat_unlock(); + part_in_flight(gp->queue, hd, inflight); seq_printf(seqf, "%4d %7d %s %lu %lu %lu " "%u %lu %lu %lu %u %u %u %u\n", MAJOR(part_devt(hd)), MINOR(part_devt(hd)), @@ -1231,7 +1233,7 @@ static int diskstats_show(struct seq_file *seqf, void *v) part_stat_read(hd, merges[WRITE]), part_stat_read(hd, sectors[WRITE]), jiffies_to_msecs(part_stat_read(hd, ticks[WRITE])), - part_in_flight(gp->queue, hd), + inflight[0], jiffies_to_msecs(part_stat_read(hd, io_ticks)), jiffies_to_msecs(part_stat_read(hd, time_in_queue)) ); diff --git a/block/partition-generic.c b/block/partition-generic.c index d1bdd61e1d71..fa5049a4d99b 100644 --- a/block/partition-generic.c +++ b/block/partition-generic.c @@ -113,11 +113,13 @@ ssize_t part_stat_show(struct device *dev, { struct hd_struct *p = dev_to_part(dev); struct request_queue *q = dev_to_disk(dev)->queue; + unsigned int inflight[2]; int cpu; cpu = part_stat_lock(); part_round_stats(q, cpu, p); part_stat_unlock(); + part_in_flight(q, p, inflight); return sprintf(buf, "%8lu %8lu %8llu %8u " "%8lu %8lu %8llu %8u " @@ -131,7 +133,7 @@ ssize_t part_stat_show(struct device *dev, part_stat_read(p, merges[WRITE]), (unsigned long long)part_stat_read(p, sectors[WRITE]), jiffies_to_msecs(part_stat_read(p, ticks[WRITE])), - part_in_flight(q, p), + inflight[0], jiffies_to_msecs(part_stat_read(p, io_ticks)), jiffies_to_msecs(part_stat_read(p, time_in_queue))); } diff --git a/include/linux/genhd.h b/include/linux/genhd.h index 7f7427e00f9c..a9c8ea632fdc 100644 --- a/include/linux/genhd.h +++ b/include/linux/genhd.h @@ -378,11 +378,17 @@ static inline void part_dec_in_flight(struct request_queue *q, atomic_dec(&part_to_disk(part)->part0.in_flight[rw]); } -static inline int part_in_flight(struct request_queue *q, - struct hd_struct *part) +static inline void part_in_flight(struct request_queue *q, + struct hd_struct *part, + unsigned int inflight[2]) { - return atomic_read(&part->in_flight[0]) + + inflight[0] = atomic_read(&part->in_flight[0]) + atomic_read(&part->in_flight[1]); + if (part->partno) { + part = &part_to_disk(part)->part0; + inflight[1] = atomic_read(&part->in_flight[0]) + + atomic_read(&part->in_flight[1]); + } } static inline struct partition_meta_info *alloc_part_info(struct gendisk *disk)
Instead of returning the count that matches the partition, pass in an array of two ints. Index 0 will be filled with the inflight count for the partition in question, and index 1 will filled with the root infligh count, if the partition passed in is not the root. This is in preparation for being able to calculate both in one go. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- block/blk-core.c | 8 ++++---- block/genhd.c | 4 +++- block/partition-generic.c | 4 +++- include/linux/genhd.h | 12 +++++++++--- 4 files changed, 19 insertions(+), 9 deletions(-)