per-cpu in_flight counters for bio-based drivers
Subsystems (3)
v4 cover per-cpu in_flight counters for bio-based drivers
Hey,
This v4 addresses the compile issues on various archs when CONFIG_SMP isn't set (by introducing appropriate wrappers in genhd.h)
Testing with this v4 I was unable to reproduce the issue you reported where iostat always reports 0 for avgqu-sz -- but please let me know if you still see problems like that.
Thanks,
Mike
Mike Snitzer (2):
dm rq: leverage blk_mq_queue_busy() to check for outstanding IO
block: stop passing 'cpu' to all percpu stats methods
Mikulas Patocka (5):
dm: dont rewrite dm_disk(md)->part0.in_flight
block: delete part_round_stats and switch to less precise counting
block: switch to per-cpu in-flight counters
block: return just one value from part_in_flight
dm: remove the pending IO accounting
block/bio.c | 36 ++++++++++++++++----- block/blk-core.c | 80 +++++++---------------------------------------- block/blk-merge.c | 4 +-- block/blk-mq.c | 12 +++---- block/blk-mq.h | 3 +- block/genhd.c | 55 ++++++++++++++++++-------------- block/partition-generic.c | 10 ++---- drivers/md/dm-core.h | 2 -- drivers/md/dm-rq.c | 9 +++--- drivers/md/dm.c | 36 +++++++++------------ drivers/md/md.c | 7 ++--- include/linux/genhd.h | 55 +++++++++++++++++++------------- 12 files changed, 137 insertions(+), 172 deletions(-)
Signature · 1 line
2.15.0
PATCH v4 1/7 dm: dont rewrite dm_disk(md)->part0.in_flight
From: Mikulas Patocka <[email protected]>
generic_start_io_acct and generic_end_io_acct already update the variable in_flight using atomic operations, so we don't have to overwrite them again.
--- drivers/md/dm.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/md/dm.c +1 -3
@@ -663,8 +663,7 @@ static void start_io_acct(struct dm_io *io)generic_start_io_acct (md->queue,bio_op (bio),bio_sectors (bio), &dm_disk (md)->part0); -atomic_set (&dm_disk (md)->part0.in_flight[rw], -atomic_inc_return (&md->pending[rw])); +atomic_inc (&md->pending[rw]);if (unlikely (dm_stats_used (&md->stats))) dm_stats_account_io(&md->stats, bio_data_dir(bio),
@@ -693,7 +692,6 @@ static void end_io_acct(struct dm_io *io)* a flush. */ pending =atomic_dec_return (&md->pending[rw]); -atomic_set (&dm_disk (md)->part0.in_flight[rw], pending); pending +=atomic_read (&md->pending[rw^0x1 ]);/* nudge anyone waiting on suspend queue */
Signature · 1 line
2.15.0
PATCH v4 2/7 dm rq: leverage blk_mq_queue_busy() to check for outstanding IO
In reply to Mike SnitzerNow that request-based dm-multipath only supports blk-mq, make use of the newly introduced blk_mq_queue_busy() to check for outstanding IO -- rather than (ab)using the block core's in_flight counters.
--- drivers/md/dm-rq.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
drivers/md/dm-rq.c +4 -5
@@ -130,11 +130,11 @@ static void rq_end_stats(struct mapped_device *md, struct request *orig)*/ static void rq_completed (struct mapped_device *md,int rw,bool run_queue) { -atomic_dec (&md->pending[rw]); -/* nudge anyone waiting on suspend queue */ -if (!md_in_flight (md)) -wake_up (&md->wait); +if (unlikely (waitqueue_active (&md->wait))) { +if (!blk_mq_queue_busy (md->queue)) +wake_up (&md->wait); + }/* * dm_put() must be at the end of this function. See the comment above
@@ -436,7 +436,6 @@ ssize_t dm_attr_rq_based_seq_io_merge_deadline_store(struct mapped_device *md,static void dm_start_request (struct mapped_device *md,struct request *orig) {blk_mq_start_request (orig); -atomic_inc (&md->pending[rq_data_dir (orig)]);if (unlikely (dm_stats_used (&md->stats))) {struct dm_rq_target_io *tio =tio_from_request (orig);
Signature · 1 line
2.15.0
PATCH v4 3/7 block: stop passing 'cpu' to all percpu stats methods
In reply to Mike SnitzerAll of part_stat_* and related methods are used with preempt disabled, so there is no need to pass cpu around to allow of them. Just call smp_processor_id() as needed.
--- block/bio.c | 16 +++++++++------- block/blk-core.c | 34 +++++++++++++++------------------- block/blk-merge.c | 5 ++--- block/genhd.c | 5 ++--- block/partition-generic.c | 5 ++--- drivers/md/md.c | 7 +++---- include/linux/genhd.h | 26 +++++++++++++------------- 7 files changed, 46 insertions(+), 52 deletions(-)
block/bio.c +9 -7
@@ -1667,11 +1667,12 @@ void generic_start_io_acct(struct request_queue *q, int op, unsigned long sectors, struct hd_struct *part) { const int sgrp = op_stat_group(op); -int cpu =part_stat_lock (); -part_round_stats (q, cpu, part); -part_stat_inc (cpu, part, ios[sgrp]); -part_stat_add (cpu, part, sectors[sgrp], sectors); +part_stat_lock (); + +part_round_stats (q, part); +part_stat_inc (part, ios[sgrp]); +part_stat_add (part, sectors[sgrp], sectors);part_inc_in_flight (q, part,op_is_write (op));part_stat_unlock ();
@@ -1683,10 +1684,11 @@ void generic_end_io_acct(struct request_queue *q, int req_op, {unsigned long duration = jiffies - start_time;const int sgrp =op_stat_group (req_op); -int cpu =part_stat_lock (); -part_stat_add (cpu, part, nsecs[sgrp],jiffies_to_nsecs (duration)); -part_round_stats (q, cpu, part); +part_stat_lock (); + +part_stat_add (part, nsecs[sgrp],jiffies_to_nsecs (duration)); +part_round_stats (q, part);part_dec_in_flight (q, part,op_is_write (req_op));part_stat_unlock ();
block/blk-core.c +15 -19
@@ -584,14 +584,14 @@ struct request *blk_get_request(struct request_queue *q, unsigned int op, }EXPORT_SYMBOL (blk_get_request); -static void part_round_stats_single (struct request_queue *q,int cpu, +static void part_round_stats_single (struct request_queue *q,struct hd_struct *part,unsigned long now,unsigned int inflight) {if (inflight) { -__part_stat_add (cpu, part, time_in_queue, +__part_stat_add (part, time_in_queue, inflight * (now - part->stamp)); -__part_stat_add (cpu, part, io_ticks, (now - part->stamp)); +__part_stat_add (part, io_ticks, (now - part->stamp)); } part->stamp = now; }
@@ -599,7 +599,6 @@ static void part_round_stats_single(struct request_queue *q, int cpu,/** * part_round_stats() - Round off the performance stats on a struct disk_stats. * @q: target block queue -* @cpu: cpu number for stats access * @part: target partition * * The average IO queue length and utilisation statistics are maintained
@@ -613,7 +612,7 @@ static void part_round_stats_single(struct request_queue *q, int cpu,* /proc/diskstats. This accounts immediately for all queue usage up to * the current jiffies and restarts the counters again. */ -void part_round_stats (struct request_queue *q,int cpu,struct hd_struct *part) +void part_round_stats (struct request_queue *q,struct hd_struct *part) {struct hd_struct *part2 =NULL ;unsigned long now = jiffies;
@@ -635,9 +634,9 @@ void part_round_stats(struct request_queue *q, int cpu, struct hd_struct *part)part_in_flight (q, part, inflight);if (stats &2 ) -part_round_stats_single (q, cpu, part2, now, inflight[1 ]); +part_round_stats_single (q, part2, now, inflight[1 ]);if (stats &1 ) -part_round_stats_single (q, cpu, part, now, inflight[0 ]); +part_round_stats_single (q, part, now, inflight[0 ]); }EXPORT_SYMBOL_GPL (part_round_stats);
@@ -1362,11 +1361,10 @@ void blk_account_io_completion(struct request *req, unsigned int bytes)if (blk_do_io_stat (req)) {const int sgrp =op_stat_group (req_op (req));struct hd_struct *part; -int cpu; - cpu =part_stat_lock (); +part_stat_lock (); part = req->part; -part_stat_add (cpu, part, sectors[sgrp], bytes >>9 ); +part_stat_add (part, sectors[sgrp], bytes >>9 );part_stat_unlock (); } }
@@ -1381,14 +1379,13 @@ void blk_account_io_done(struct request *req, u64 now)if (blk_do_io_stat (req) && !(req->rq_flags &RQF_FLUSH_SEQ )) {const int sgrp =op_stat_group (req_op (req));struct hd_struct *part; -int cpu; - cpu =part_stat_lock (); +part_stat_lock (); part = req->part; -part_stat_inc (cpu, part, ios[sgrp]); -part_stat_add (cpu, part, nsecs[sgrp], now - req->start_time_ns); -part_round_stats (req->q, cpu, part); +part_stat_inc (part, ios[sgrp]); +part_stat_add (part, nsecs[sgrp], now - req->start_time_ns); +part_round_stats (req->q, part);part_dec_in_flight (req->q, part,rq_data_dir (req));hd_struct_put (part);
@@ -1400,16 +1397,15 @@ void blk_account_io_start(struct request *rq, bool new_io) {struct hd_struct *part;int rw =rq_data_dir (rq); -int cpu;if (!blk_do_io_stat (rq))return ; - cpu =part_stat_lock (); +part_stat_lock ();if (!new_io) { part = rq->part; -part_stat_inc (cpu, part, merges[rw]); +part_stat_inc (part, merges[rw]); }else { part =disk_map_sector_rcu (rq->rq_disk,blk_rq_pos (rq));if (!hd_struct_try_get (part)) {
@@ -1424,7 +1420,7 @@ void blk_account_io_start(struct request *rq, bool new_io) part = &rq->rq_disk->part0;hd_struct_get (part); } -part_round_stats (rq->q, cpu, part); +part_round_stats (rq->q, part);part_inc_in_flight (rq->q, part, rw); rq->part = part; }
block/blk-merge.c +2 -3
@@ -685,12 +685,11 @@ static void blk_account_io_merge(struct request *req) {if (blk_do_io_stat (req)) {struct hd_struct *part; -int cpu; - cpu =part_stat_lock (); +part_stat_lock (); part = req->part; -part_round_stats (req->q, cpu, part); +part_round_stats (req->q, part);part_dec_in_flight (req->q, part,rq_data_dir (req));hd_struct_put (part);
block/genhd.c +2 -3
@@ -1326,7 +1326,6 @@ static int diskstats_show(struct seq_file *seqf, void *v)struct hd_struct *hd;char buf[BDEVNAME_SIZE ];unsigned int inflight[2 ]; -int cpu;/* if (&disk_to_dev(gp)->kobj.entry == block_class.devices.next)
@@ -1338,8 +1337,8 @@ static int diskstats_show(struct seq_file *seqf, void *v)disk_part_iter_init (&piter, gp,DISK_PITER_INCL_EMPTY_PART0 );while ((hd =disk_part_iter_next (&piter))) { - cpu =part_stat_lock (); -part_round_stats (gp->queue, cpu, hd); +part_stat_lock (); +part_round_stats (gp->queue, hd);part_stat_unlock ();part_in_flight (gp->queue, hd, inflight); seq_printf(seqf, "%4d %7d %s "
block/partition-generic.c +2 -3
@@ -121,10 +121,9 @@ ssize_t part_stat_show(struct device *dev,struct hd_struct *p =dev_to_part (dev);struct request_queue *q =part_to_disk (p)->queue;unsigned int inflight[2 ]; -int cpu; - cpu =part_stat_lock (); -part_round_stats (q, cpu, p); +part_stat_lock (); +part_round_stats (q, p);part_stat_unlock ();part_in_flight (q, p, inflight); return sprintf(buf,
drivers/md/md.c +3 -4
@@ -334,7 +334,6 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)const int sgrp =op_stat_group (bio_op (bio));struct mddev *mddev = q->queuedata;unsigned int sectors; -int cpu;blk_queue_split (q, &bio);
@@ -359,9 +358,9 @@ static blk_qc_t md_make_request(struct request_queue *q, struct bio *bio)md_handle_request (mddev, bio); - cpu =part_stat_lock (); -part_stat_inc (cpu, &mddev->gendisk->part0, ios[sgrp]); -part_stat_add (cpu, &mddev->gendisk->part0, sectors[sgrp], sectors); +part_stat_lock (); +part_stat_inc (&mddev->gendisk->part0, ios[sgrp]); +part_stat_add (&mddev->gendisk->part0, sectors[sgrp], sectors);part_stat_unlock ();return BLK_QC_T_NONE ;
include/linux/genhd.h +13 -13
@@ -295,8 +295,8 @@ extern struct hd_struct *disk_map_sector_rcu(struct gendisk *disk,#define part_stat_lock () ({ rcu_read_lock(); get_cpu(); })#define part_stat_unlock () do { put_cpu(); rcu_read_unlock(); } while (0) -#define __part_stat_add (cpu, part, field, addnd) \ - (per_cpu_ptr((part)->dkstats, (cpu))->field += (addnd)) +#define __part_stat_add (part, field, addnd) \ + (per_cpu_ptr((part)->dkstats, smp_processor_id())->field += (addnd))#define part_stat_read (part, field) \ ({ \
@@ -333,7 +333,7 @@ static inline void free_part_stats(struct hd_struct *part)#define part_stat_lock () ({ rcu_read_lock(); 0; })#define part_stat_unlock () rcu_read_unlock() -#define __part_stat_add (cpu, part, field, addnd) \ +#define __part_stat_add (part, field, addnd) \ ((part)->dkstats.field += addnd)#define part_stat_read (part, field) ((part)->dkstats.field)
@@ -362,19 +362,19 @@ static inline void free_part_stats(struct hd_struct *part) part_stat_read(part, field[STAT_WRITE]) + \ part_stat_read(part, field[STAT_DISCARD])) -#define part_stat_add (cpu, part, field, addnd) do { \ - __part_stat_add((cpu), (part), field, addnd); \ +#define part_stat_add (part, field, addnd) do { \ + __part_stat_add((part), field, addnd); \ if ((part)->partno) \ - __part_stat_add((cpu), &part_to_disk((part))->part0, \ + __part_stat_add(&part_to_disk((part))->part0, \ field, addnd); \ } while (0) -#define part_stat_dec (cpu, gendiskp, field) \ - part_stat_add(cpu, gendiskp, field, -1) -#define part_stat_inc (cpu, gendiskp, field) \ - part_stat_add(cpu, gendiskp, field, 1) -#define part_stat_sub (cpu, gendiskp, field, subnd) \ - part_stat_add(cpu, gendiskp, field, -subnd) +#define part_stat_dec (gendiskp, field) \ + part_stat_add(gendiskp, field, -1) +#define part_stat_inc (gendiskp, field) \ + part_stat_add(gendiskp, field, 1) +#define part_stat_sub (gendiskp, field, subnd) \ + part_stat_add(gendiskp, field, -subnd)void part_in_flight (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]);
@@ -399,7 +399,7 @@ static inline void free_part_info(struct hd_struct *part) }/* block/blk-core.c */ -extern void part_round_stats (struct request_queue *q,int cpu,struct hd_struct *part); +extern void part_round_stats (struct request_queue *q,struct hd_struct *part);/* block/genhd.c */ extern void device_add_disk(struct device *parent, struct gendisk *disk,
Signature · 1 line
2.15.0
PATCH v4 4/7 block: delete part_round_stats and switch to less precise counting
In reply to Mike SnitzerFrom: Mikulas Patocka <[email protected]>
We want to convert to per-cpu in_flight counters.
The function part_round_stats needs the in_flight counter every jiffy, it would be too costly to sum all the percpu variables every jiffy, so it must be deleted. part_round_stats is used to calculate two counters - time_in_queue and io_ticks.
time_in_queue can be calculated without part_round_stats, by adding the duration of the I/O when the I/O ends (the value is almost as exact as the previously calculated value, except that time for in-progress I/Os is not counted).
io_ticks can be approximated by increasing the value when I/O is started or ended and the jiffies value has changed. If the I/Os take less than a jiffy, the value is as exact as the previously calculated value. If the I/Os take more than a jiffy, io_ticks can drift behind the previously calculated value.
--- block/bio.c | 24 +++++++++++++++--- block/blk-core.c | 62 +++-------------------------------------------- block/blk-merge.c | 1 - block/genhd.c | 3 --- block/partition-generic.c | 3 --- include/linux/genhd.h | 3 +-- 6 files changed, 26 insertions(+), 70 deletions(-)
block/bio.c +21 -3
@@ -1663,6 +1663,22 @@ void bio_check_pages_dirty(struct bio *bio) }EXPORT_SYMBOL_GPL (bio_check_pages_dirty); +void update_io_ticks (struct hd_struct *part,unsigned long now) +{ +unsigned long stamp; +again: + stamp =READ_ONCE (part->stamp); +if (unlikely (stamp != now)) { +if (likely (cmpxchg (&part->stamp, stamp, now) == stamp)) { +__part_stat_add (part, io_ticks,1 ); + } + } +if (part->partno) { + part = &part_to_disk (part)->part0; + goto again; + } +} +void generic_start_io_acct (struct request_queue *q,int op,unsigned long sectors,struct hd_struct *part) {
@@ -1670,7 +1686,7 @@ void generic_start_io_acct(struct request_queue *q, int op,part_stat_lock (); -part_round_stats (q, part); +update_io_ticks (part, jiffies);part_stat_inc (part, ios[sgrp]);part_stat_add (part, sectors[sgrp], sectors);part_inc_in_flight (q, part,op_is_write (op));
@@ -1682,13 +1698,15 @@ EXPORT_SYMBOL(generic_start_io_acct);void generic_end_io_acct (struct request_queue *q,int req_op,struct hd_struct *part,unsigned long start_time) { -unsigned long duration = jiffies - start_time; +unsigned long now = jiffies; +unsigned long duration = now - start_time;const int sgrp =op_stat_group (req_op);part_stat_lock (); +update_io_ticks (part, now);part_stat_add (part, nsecs[sgrp],jiffies_to_nsecs (duration)); -part_round_stats (q, part); +part_stat_add (part, time_in_queue, duration);part_dec_in_flight (q, part,op_is_write (req_op));part_stat_unlock ();
block/blk-core.c +4 -58
86 lines not on this page · raw message
block/blk-merge.c +0 -1
@@ -689,7 +689,6 @@ static void blk_account_io_merge(struct request *req)part_stat_lock (); part = req->part; -part_round_stats (req->q, part);part_dec_in_flight (req->q, part,rq_data_dir (req));hd_struct_put (part);
block/genhd.c +0 -3
@@ -1337,9 +1337,6 @@ static int diskstats_show(struct seq_file *seqf, void *v)disk_part_iter_init (&piter, gp,DISK_PITER_INCL_EMPTY_PART0 );while ((hd =disk_part_iter_next (&piter))) { -part_stat_lock (); -part_round_stats (gp->queue, hd); -part_stat_unlock ();part_in_flight (gp->queue, hd, inflight);seq_printf (seqf,"%4d %7d %s " "%lu %lu %lu %u "
block/partition-generic.c +0 -3
@@ -122,9 +122,6 @@ ssize_t part_stat_show(struct device *dev,struct request_queue *q =part_to_disk (p)->queue;unsigned int inflight[2 ]; -part_stat_lock (); -part_round_stats (q, p); -part_stat_unlock ();part_in_flight (q, p, inflight);return sprintf (buf, "%8lu %8lu %8llu %8u "
include/linux/genhd.h +1 -2
@@ -398,8 +398,7 @@ static inline void free_part_info(struct hd_struct *part)kfree (part->info); } -/* block/blk-core.c */ -extern void part_round_stats (struct request_queue *q,struct hd_struct *part); +void update_io_ticks (struct hd_struct *part,unsigned long now);/* block/genhd.c */ extern void device_add_disk(struct device *parent, struct gendisk *disk,
Signature · 1 line
2.15.0
PATCH v4 5/7 block: switch to per-cpu in-flight counters
In reply to Mike SnitzerFrom: Mikulas Patocka <[email protected]>
Now when part_round_stats is gone, we can switch to per-cpu in-flight counters.
We use the local-atomic type local_t, so that if part_inc_in_flight or part_dec_in_flight is reentrantly called from an interrupt, the value will be correct.
The other counters could be corrupted due to reentrant interrupt, but the corruption only results in slight counter skew - the in_flight counter must be exact, so it needs local_t.
--- block/genhd.c | 43 +++++++++++++++++++++++++++++++++---------- include/linux/genhd.h | 29 ++++++++++++++++++++++------- 2 files changed, 55 insertions(+), 17 deletions(-)
block/genhd.c +33 -10
@@ -50,9 +50,9 @@ void part_inc_in_flight(struct request_queue *q, struct hd_struct *part, int rw)if (queue_is_mq (q))return ; -atomic_inc (&part->in_flight[rw]); +part_stat_local_inc (part, in_flight[rw]);if (part->partno) -atomic_inc (&part_to_disk (part)->part0.in_flight[rw]); +part_stat_local_inc (&part_to_disk (part)->part0, in_flight[rw]); }void part_dec_in_flight (struct request_queue *q,struct hd_struct *part,int rw)
@@ -60,38 +60,61 @@ void part_dec_in_flight(struct request_queue *q, struct hd_struct *part, int rw)if (queue_is_mq (q))return ; -atomic_dec (&part->in_flight[rw]); +part_stat_local_dec (part, in_flight[rw]);if (part->partno) -atomic_dec (&part_to_disk (part)->part0.in_flight[rw]); +part_stat_local_dec (&part_to_disk (part)->part0, in_flight[rw]); }void part_in_flight (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]) { +int cpu; +if (queue_is_mq (q)) {blk_mq_in_flight (q, part, inflight);return ; } - inflight[0 ] =atomic_read (&part->in_flight[0 ]) + -atomic_read (&part->in_flight[1 ]); + inflight[0 ] =0 ; +for_each_possible_cpu (cpu) { + inflight[0 ] +=part_stat_local_read_cpu (part, in_flight[0 ], cpu) + +part_stat_local_read_cpu (part, in_flight[1 ], cpu); + } +if ((int )inflight[0 ] <0 ) + inflight[0 ] =0 ; +if (part->partno) { part = &part_to_disk (part)->part0; - inflight[1 ] =atomic_read (&part->in_flight[0 ]) + -atomic_read (&part->in_flight[1 ]); + inflight[1 ] =0 ; +for_each_possible_cpu (cpu) { + inflight[1 ] +=part_stat_local_read_cpu (part, in_flight[0 ], cpu) + +part_stat_local_read_cpu (part, in_flight[1 ], cpu); + } +if ((int )inflight[1 ] <0 ) + inflight[1 ] =0 ; } }void part_in_flight_rw (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]) { +int cpu; +if (queue_is_mq (q)) {blk_mq_in_flight_rw (q, part, inflight);return ; } - inflight[0 ] =atomic_read (&part->in_flight[0 ]); - inflight[1 ] =atomic_read (&part->in_flight[1 ]); + inflight[0 ] =0 ; + inflight[1 ] =0 ; +for_each_possible_cpu (cpu) { + inflight[0 ] +=part_stat_local_read_cpu (part, in_flight[0 ], cpu); + inflight[1 ] +=part_stat_local_read_cpu (part, in_flight[1 ], cpu); + } +if ((int )inflight[0 ] <0 ) + inflight[0 ] =0 ; +if ((int )inflight[1 ] <0 ) + inflight[1 ] =0 ; }struct hd_struct *__disk_get_part (struct gendisk *disk,int partno)
include/linux/genhd.h +22 -7
@@ -17,6 +17,7 @@#include <linux/percpu-refcount.h> #include <linux/uuid.h> #include <linux/blk_types.h> +#include <asm/local.h> #ifdef CONFIG_BLOCK
@@ -89,6 +90,7 @@ struct disk_stats {unsigned long merges[NR_STAT_GROUPS ];unsigned long io_ticks;unsigned long time_in_queue; +local_t in_flight[2 ]; };#define PARTITION_META_INFO_VOLNAMELTH 64
@@ -122,7 +124,6 @@ struct hd_struct {int make_it_fail;#endif unsigned long stamp; -atomic_t in_flight[2 ];#ifdef CONFIG_SMP struct disk_stats __percpu *dkstats;#else
@@ -295,8 +296,11 @@ extern struct hd_struct *disk_map_sector_rcu(struct gendisk *disk,#define part_stat_lock () ({ rcu_read_lock(); get_cpu(); })#define part_stat_unlock () do { put_cpu(); rcu_read_unlock(); } while (0) -#define __part_stat_add (part, field, addnd) \ - (per_cpu_ptr((part)->dkstats, smp_processor_id())->field += (addnd)) +#define part_stat_get_cpu (part, field, cpu) \ + (per_cpu_ptr((part)->dkstats, (cpu))->field) + +#define part_stat_get (part, field) \ + part_stat_get_cpu(part, field, smp_processor_id())#define part_stat_read (part, field) \ ({ \
@@ -333,10 +337,9 @@ static inline void free_part_stats(struct hd_struct *part)#define part_stat_lock () ({ rcu_read_lock(); 0; })#define part_stat_unlock () rcu_read_unlock() -#define __part_stat_add (part, field, addnd) \ - ((part)->dkstats.field += addnd) - -#define part_stat_read (part, field) ((part)->dkstats.field) +#define part_stat_get (part, field) ((part)->dkstats.field) +#define part_stat_get_cpu (part, field, cpu) part_stat_get(part, field) +#define part_stat_read (part, field) part_stat_get(part, field)static inline void part_stat_set_all (struct hd_struct *part,int value) {
@@ -362,6 +365,9 @@ static inline void free_part_stats(struct hd_struct *part) part_stat_read(part, field[STAT_WRITE]) + \ part_stat_read(part, field[STAT_DISCARD])) +#define __part_stat_add (part, field, addnd) \ + (part_stat_get(part, field) += (addnd)) +#define part_stat_add (part, field, addnd) do { \ __part_stat_add((part), field, addnd); \ if ((part)->partno) \
@@ -376,6 +382,15 @@ static inline void free_part_stats(struct hd_struct *part)#define part_stat_sub (gendiskp, field, subnd) \ part_stat_add(gendiskp, field, -subnd) +#define part_stat_local_dec (gendiskp, field) \ + local_dec(&(part_stat_get(gendiskp, field))) +#define part_stat_local_inc (gendiskp, field) \ + local_inc(&(part_stat_get(gendiskp, field))) +#define part_stat_local_read (gendiskp, field) \ + local_read(&(part_stat_get(gendiskp, field))) +#define part_stat_local_read_cpu (gendiskp, field, cpu) \ + local_read(&(part_stat_get_cpu(gendiskp, field, cpu))) +void part_in_flight (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]); void part_in_flight_rw(struct request_queue *q, struct hd_struct *part,
Signature · 1 line
2.15.0
PATCH v4 6/7 block: return just one value from part_in_flight
In reply to Mike SnitzerFrom: Mikulas Patocka <[email protected]>
The previous patches deleted all the code that needed the second value returned from part_in_flight - now the kernel only uses the first value.
Consequently, part_in_flight (and blk_mq_in_flight) may be changed so that it only returns one value.
This patch just refactors the code, there's no functional change.
--- block/blk-mq.c | 12 +++++------- block/blk-mq.h | 3 +-- block/genhd.c | 34 ++++++++++++---------------------- block/partition-generic.c | 6 +++--- include/linux/genhd.h | 3 +-- 5 files changed, 22 insertions(+), 36 deletions(-)
block/blk-mq.c +5 -7
@@ -100,25 +100,23 @@ static bool blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,struct mq_inflight *mi = priv;/* -* index[0] counts the specific partition that was asked for. index[1] -* counts the ones that are active on the whole device, so increment -* that if mi->part is indeed a partition, and not a whole device. +* index[0] counts the specific partition that was asked for. */ if (rq->part == mi->part) mi->inflight[0 ]++; -if (mi->part->partno) - mi->inflight[1 ]++;return true; } -void blk_mq_in_flight (struct request_queue *q,struct hd_struct *part, -unsigned int inflight[2 ]) +unsigned int blk_mq_in_flight (struct request_queue *q,struct hd_struct *part) { +unsigned inflight[2 ];struct mq_inflight mi = { .part = part, .inflight = inflight, }; inflight[0 ] = inflight[1 ] =0 ;blk_mq_queue_tag_busy_iter (q, blk_mq_check_inflight, &mi); + +return inflight[0 ]; } static bool blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
block/blk-mq.h +1 -2
@@ -187,8 +187,7 @@ static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx)return hctx->nr_ctx && hctx->tags; } -void blk_mq_in_flight (struct request_queue *q,struct hd_struct *part, -unsigned int inflight[2 ]); +unsigned int blk_mq_in_flight (struct request_queue *q,struct hd_struct *part);void blk_mq_in_flight_rw (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]);
block/genhd.c +12 -22
@@ -65,34 +65,24 @@ void part_dec_in_flight(struct request_queue *q, struct hd_struct *part, int rw)part_stat_local_dec (&part_to_disk (part)->part0, in_flight[rw]); } -void part_in_flight (struct request_queue *q,struct hd_struct *part, -unsigned int inflight[2 ]) +unsigned int part_in_flight (struct request_queue *q,struct hd_struct *part) {int cpu; +unsigned int inflight;if (queue_is_mq (q)) { -blk_mq_in_flight (q, part, inflight); -return ; +return blk_mq_in_flight (q, part); } - inflight[0 ] =0 ; + inflight =0 ;for_each_possible_cpu (cpu) { - inflight[0 ] +=part_stat_local_read_cpu (part, in_flight[0 ], cpu) + -part_stat_local_read_cpu (part, in_flight[1 ], cpu); + inflight +=part_stat_local_read_cpu (part, in_flight[0 ], cpu) + +part_stat_local_read_cpu (part, in_flight[1 ], cpu); } -if ((int )inflight[0 ] <0 ) - inflight[0 ] =0 ; +if ((int )inflight <0 ) + inflight =0 ; -if (part->partno) { - part = &part_to_disk (part)->part0; - inflight[1 ] =0 ; -for_each_possible_cpu (cpu) { - inflight[1 ] +=part_stat_local_read_cpu (part, in_flight[0 ], cpu) + -part_stat_local_read_cpu (part, in_flight[1 ], cpu); - } -if ((int )inflight[1 ] <0 ) - inflight[1 ] =0 ; - } +return inflight; } void part_in_flight_rw(struct request_queue *q, struct hd_struct *part,
@@ -1348,7 +1338,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 ]; +unsigned int inflight;/* if (&disk_to_dev(gp)->kobj.entry == block_class.devices.next)
@@ -1360,7 +1350,7 @@ static int diskstats_show(struct seq_file *seqf, void *v)disk_part_iter_init (&piter, gp,DISK_PITER_INCL_EMPTY_PART0 );while ((hd =disk_part_iter_next (&piter))) { -part_in_flight (gp->queue, hd, inflight); + inflight =part_in_flight (gp->queue, hd);seq_printf (seqf,"%4d %7d %s " "%lu %lu %lu %u " "%lu %lu %lu %u "
@@ -1376,7 +1366,7 @@ static int diskstats_show(struct seq_file *seqf, void *v)part_stat_read (hd, merges[STAT_WRITE ]),part_stat_read (hd, sectors[STAT_WRITE ]), (unsigned int )part_stat_read_msecs (hd,STAT_WRITE ), - inflight[0 ], + inflight,jiffies_to_msecs (part_stat_read (hd, io_ticks)),jiffies_to_msecs (part_stat_read (hd, time_in_queue)), part_stat_read(hd, ios[STAT_DISCARD]),
block/partition-generic.c +3 -3
@@ -120,9 +120,9 @@ ssize_t part_stat_show(struct device *dev, {struct hd_struct *p =dev_to_part (dev);struct request_queue *q =part_to_disk (p)->queue; -unsigned int inflight[2 ]; +unsigned int inflight; -part_in_flight (q, p, inflight); + inflight =part_in_flight (q, p);return sprintf (buf,"%8lu %8lu %8llu %8u " "%8lu %8lu %8llu %8u "
@@ -137,7 +137,7 @@ ssize_t part_stat_show(struct device *dev,part_stat_read (p, merges[STAT_WRITE ]), (unsigned long long )part_stat_read (p, sectors[STAT_WRITE ]), (unsigned int )part_stat_read_msecs (p,STAT_WRITE ), - inflight[0 ], + inflight,jiffies_to_msecs (part_stat_read (p, io_ticks)),jiffies_to_msecs (part_stat_read (p, time_in_queue)), part_stat_read(p, ios[STAT_DISCARD]),
include/linux/genhd.h +1 -2
@@ -391,8 +391,7 @@ static inline void free_part_stats(struct hd_struct *part)#define part_stat_local_read_cpu (gendiskp, field, cpu) \ local_read(&(part_stat_get_cpu(gendiskp, field, cpu))) -void part_in_flight (struct request_queue *q,struct hd_struct *part, -unsigned int inflight[2 ]); +unsigned int part_in_flight (struct request_queue *q,struct hd_struct *part);void part_in_flight_rw (struct request_queue *q,struct hd_struct *part,unsigned int inflight[2 ]); void part_dec_in_flight(struct request_queue *q, struct hd_struct *part,
Signature · 1 line
2.15.0
PATCH v4 7/7 dm: remove the pending IO accounting
In reply to Mike SnitzerFrom: Mikulas Patocka <[email protected]>
Remove the "pending" atomic counters, that duplicate block-core's in_flight counters, and update md_in_flight() to look at percpu in_flight counters.
--- drivers/md/dm-core.h | 2 -- drivers/md/dm.c | 34 +++++++++++++++------------------- 2 files changed, 15 insertions(+), 21 deletions(-)
drivers/md/dm-core.h +0 -2
@@ -65,7 +65,6 @@ struct mapped_device {*/ struct work_struct work;wait_queue_head_t wait; -atomic_t pending[2 ];spinlock_t deferred_lock;struct bio_list deferred;
@@ -119,7 +118,6 @@ struct mapped_device {struct srcu_struct io_barrier; }; -int md_in_flight (struct mapped_device *md);void disable_write_same (struct mapped_device *md);void disable_write_zeroes (struct mapped_device *md);
drivers/md/dm.c +15 -19
@@ -646,25 +646,30 @@ static void free_tio(struct dm_target_io *tio)bio_put (&tio->clone); } -int md_in_flight (struct mapped_device *md) +static bool md_in_flight (struct mapped_device *md) { -return atomic_read (&md->pending[READ ]) + -atomic_read (&md->pending[WRITE ]); +int cpu; +struct hd_struct *part = &dm_disk (md)->part0; + +for_each_possible_cpu (cpu) { +if (part_stat_local_read_cpu (part, in_flight[0 ], cpu) || +part_stat_local_read_cpu (part, in_flight[1 ], cpu)) +return true; + } + +return false; }static void start_io_acct (struct dm_io *io) {struct mapped_device *md = io->md;struct bio *bio = io->orig_bio; -int rw =bio_data_dir (bio); io->start_time = jiffies;generic_start_io_acct (md->queue,bio_op (bio),bio_sectors (bio), &dm_disk (md)->part0); -atomic_inc (&md->pending[rw]); -if (unlikely (dm_stats_used (&md->stats))) dm_stats_account_io(&md->stats, bio_data_dir(bio), bio->bi_iter.bi_sector, bio_sectors(bio),
@@ -676,8 +681,6 @@ static void end_io_acct(struct dm_io *io)struct mapped_device *md = io->md;struct bio *bio = io->orig_bio;unsigned long duration = jiffies - io->start_time; -int pending; -int rw =bio_data_dir (bio);generic_end_io_acct (md->queue,bio_op (bio), &dm_disk (md)->part0, io->start_time);
@@ -687,16 +690,11 @@ static void end_io_acct(struct dm_io *io) bio->bi_iter.bi_sector,bio_sectors (bio), true, duration, &io->stats_aux); -/* -* After this is decremented the bio must not be touched if it is -* a flush. -*/ - pending =atomic_dec_return (&md->pending[rw]); - pending +=atomic_read (&md->pending[rw^0x1 ]); -/* nudge anyone waiting on suspend queue */ -if (!pending) -wake_up (&md->wait); +if (unlikely (waitqueue_active (&md->wait))) { +if (!md_in_flight (md)) +wake_up (&md->wait); + } }/*
@@ -1904,8 +1902,6 @@ static struct mapped_device *alloc_dev(int minor)if (!md->disk) goto bad; -atomic_set (&md->pending[0 ],0 ); -atomic_set (&md->pending[1 ],0 );init_waitqueue_head (&md->wait);INIT_WORK (&md->work, dm_wq_work);init_waitqueue_head (&md->eventq);
Signature · 1 line
2.15.0
Quoted 8 lines · Mike Snitzer
Hey,
This v4 addresses the compile issues on various archs when
CONFIG_SMPisn't set (by introducing appropriate wrappers ingenhd.h)Testing with this v4 I was unable to reproduce the issue you reported where iostat always reports 0 for avgqu-sz -- but please let me know if you still see problems like that.
I ran blktests and both block/017 and block/018 fail due to this patchset's changes, I'm still trying to grok what the test result is _saying_ but I'll hopefully get there soon:
block/017 (do I/O and check the inflight counter) [failed]
runtime ... 77.675s
--- tests/block/017.out 2018-12-06 12:31:21.457936217 -0500
+++ /home/git/blktests/results/nodev/block/017.out.bad 2018-12-06 12:36:15.735053586 -0500
@@ -1,14 +1,14 @@
Running block/017
-sysfs inflight reads 1
+sysfs inflight reads 2
sysfs inflight writes 0
-sysfs stat 1
-diskstats 1
-sysfs inflight reads 1
...
(Run 'diff -u tests/block/017.out /home/git/blktests/results/nodev/block/017.out.bad' to see the entire diff)
# diff -u tests/block/017.out /home/git/blktests/results/nodev/block/017.out.bad --- tests/block/017.out 2018-12-06 12:31:21.457936217 -0500 +++ /home/git/blktests/results/nodev/block/017.out.bad 2018-12-06 12:51:36.695542332 -0500 @@ -1,14 +1,14 @@ Running block/017 -sysfs inflight reads 1 +sysfs inflight reads 2 sysfs inflight writes 0 -sysfs stat 1 -diskstats 1 -sysfs inflight reads 1 -sysfs inflight writes 1 sysfs stat 2 diskstats 2 -sysfs inflight reads 0 +sysfs inflight reads 2 +sysfs inflight writes 1 +sysfs stat 3 +diskstats 3 +sysfs inflight reads 1 sysfs inflight writes 0 -sysfs stat 0 -diskstats 0 +sysfs stat 1 +diskstats 1 Test complete
block/018 (do I/O and check iostats times) [failed]
runtime ... 156.073s
--- tests/block/018.out 2018-12-06 12:31:21.458936217 -0500
+++ /home/git/blktests/results/nodev/block/018.out.bad 2018-12-06 12:38:51.831116369 -0500
@@ -1,10 +1,10 @@
Running block/018
read 0 s
write 0 s
-read 1 s
+read 2 s
write 0 s
-read 1 s
...
(Run 'diff -u tests/block/018.out /home/git/blktests/results/nodev/block/018.out.bad' to see the entire diff)
# diff -u tests/block/018.out /home/git/blktests/results/nodev/block/018.out.bad --- tests/block/018.out 2018-12-06 12:31:21.458936217 -0500 +++ /home/git/blktests/results/nodev/block/018.out.bad 2018-12-06 12:38:51.831116369 -0500 @@ -1,10 +1,10 @@ Running block/018 read 0 s write 0 s -read 1 s +read 2 s write 0 s -read 1 s -write 1 s read 2 s +write 1 s +read 6 s write 3 s Test complete
Quoted 48 lines · Mike Snitzer
Quoted 8 lines · Mike Snitzer
Hey,
This v4 addresses the compile issues on various archs when
CONFIG_SMPisn't set (by introducing appropriate wrappers ingenhd.h)Testing with this v4 I was unable to reproduce the issue you reported where iostat always reports 0 for avgqu-sz -- but please let me know if you still see problems like that.
I ran blktests and both block/017 and block/018 fail due to this patchset's changes, I'm still trying to grok what the test result is _saying_ but I'll hopefully get there soon:
block/017 (do I/O and check the inflight counter) [failed]
runtime ... 77.675s
--- tests/block/017.out 2018-12-06 12:31:21.457936217 -0500
+++/home/git/blktests/results/nodev/block/017.out.bad2018-12-06 12:36:15.735053586 -0500
@@ -1,14 +1,14 @@
Running block/017
-sysfs inflight reads 1
+sysfs inflight reads 2
sysfs inflight writes 0
-sysfs stat 1
-diskstats 1
-sysfs inflight reads 1
...
(Run 'diff -u tests/block/017.out/home/git/blktests/results/nodev/block/017.out.bad' to see the entire diff)# diff -u tests/block/017.out /home/git/blktests/results/nodev/block/017.out.badhome/git/blktests/results/nodev/block/017.out.bad 2018-12-06 12:51:36.695542332 -0500 renamed from block/017.out 2018-12-06 12:31:21.457936217 -0500 +2
-6@@ -1,14 +1,14 @@ Running block/017 -sysfs inflight reads 1 +sysfs inflight reads 2 sysfs inflight writes 0 -sysfs stat 1 -diskstats 1 -sysfs inflight reads 1 -sysfs inflight writes 1 sysfs stat 2 diskstats 2 -sysfs inflight reads 0 +sysfs inflight reads 2
home/git/blktests/results/nodev/block/017.out.bad 2018-12-06 12:51:36.695542332 -0500 line 14
Quoted 10 lines
home/git/blktests/results/nodev/block/017.out.bad 2018-12-06 12:51:36.695542332 -0500 renamed from block/017.out 2018-12-06 12:31:21.457936217 -0500 +6
-2+sysfs inflight writes 1 +sysfs stat 3 +diskstats 3 +sysfs inflight reads 1 sysfs inflight writes 0 -sysfs stat 0 -diskstats 0 +sysfs stat 1 +diskstats 1 Test complete
Interestingly the 017 test is only testing blk-mq (using null_blk's queue_mode=2). This patchset's changes are specific to bio-based.
If I revert this patchset I still get the failures.
So it would seem something regressed in latest block for-next (I've been testing up through linux-block commit c754a9bf7ee8 ("blk-mq: remove QUEUE_FLAG_POLL from default MQ flags").
Quoted 22 lines
block/018 (do I/O and check iostats times) [failed]
runtime ... 156.073s
--- tests/block/018.out 2018-12-06 12:31:21.458936217 -0500
+++/home/git/blktests/results/nodev/block/018.out.bad2018-12-06 12:38:51.831116369 -0500
@@ -1,10 +1,10 @@
Running block/018
read 0 s
write 0 s
-read 1 s
+read 2 s
write 0 s
-read 1 s
...
(Run 'diff -u tests/block/018.out/home/git/blktests/results/nodev/block/018.out.bad' to see the entire diff)# diff -u tests/block/018.out /home/git/blktests/results/nodev/block/018.out.badhome/git/blktests/results/nodev/block/018.out.bad 2018-12-06 12:38:51.831116369 -0500 renamed from block/018.out 2018-12-06 12:31:21.458936217 -0500 +0
-0@@ -1,10 +1,10 @@ Running block/018 read 0 s write 0 s
home/git/blktests/results/nodev/block/018.out.bad 2018-12-06 12:38:51.831116369 -0500 line 10
Quoted 10 lines
home/git/blktests/results/nodev/block/018.out.bad 2018-12-06 12:38:51.831116369 -0500 renamed from block/018.out 2018-12-06 12:31:21.458936217 -0500 +3
-3-read 1 s +read 2 s write 0 s -read 1 s -write 1 s read 2 s +write 1 s +read 6 s write 3 s Test complete
Same goes for this block/018 test.
Mike
Jens Axboe
maintainerQuoted 9 lines · Mike Snitzer
Hey,
This v4 addresses the compile issues on various archs when
CONFIG_SMPisn't set (by introducing appropriate wrappers ingenhd.h)Testing with this v4 I was unable to reproduce the issue you reported where iostat always reports 0 for avgqu-sz -- but please let me know if you still see problems like that.
Looks good to me, and also passes my testing. Dropped the mq special cases and used this in general, also works fine. I was hoping we could get rid of the tag iteration with this, but that still seems to be more efficient for blk-mq. But at least we're better off for stacked devices with this.
Signature · 1 line
Jens Axboe