| Message ID | 20220128041753.32195-2-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/dm: fix bio-based DM IO accounting | expand |
On Thu, Jan 27, 2022 at 11:17:51PM -0500, Mike Snitzer wrote: > + __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), > + bio_op(bio), start_time); > } > +EXPORT_SYMBOL_GPL(bio_start_io_acct_time); > > /** > * bio_start_io_acct - start I/O accounting for bio based drivers > @@ -1084,14 +1096,15 @@ static unsigned long __part_start_io_acct(struct block_device *part, > */ > unsigned long bio_start_io_acct(struct bio *bio) > { > - return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), bio_op(bio)); > + return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), > + bio_op(bio), jiffies); Is droppingthe READ_ONCE safe here? This is a honest question as these helpers still confuse me. The rest looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
On Fri, Jan 28 2022 at 1:13P -0500, Christoph Hellwig <hch@lst.de> wrote: > On Thu, Jan 27, 2022 at 11:17:51PM -0500, Mike Snitzer wrote: > > + __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), > > + bio_op(bio), start_time); > > } > > +EXPORT_SYMBOL_GPL(bio_start_io_acct_time); > > > > /** > > * bio_start_io_acct - start I/O accounting for bio based drivers > > @@ -1084,14 +1096,15 @@ static unsigned long __part_start_io_acct(struct block_device *part, > > */ > > unsigned long bio_start_io_acct(struct bio *bio) > > { > > - return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), bio_op(bio)); > > + return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), > > + bio_op(bio), jiffies); > > Is droppingthe READ_ONCE safe here? This is a honest question as these > helpers still confuse me. I'm not sure either, commit 956d510ee78ca ("block: add disk/bio-based accounting helpers") doesn't offer any insight on the need. Very little kernel code uses READ_ONCE(jiffies). git diff 24d69293d9a561645e0b4d78c2fb179827e35f53^..e722fff238bbfe6308d7778a8c2163c181bf998a shows that at the time the outgoing generic_{start,end}_io_acct didn't use READ_ONCE (nor did any of the drivers that were updated to use the new helpers). ACCESS_ONCE() was used prior to READ_ONCE() -- see commit cbbce82209490 and then 316c1608d15c7. Anyway, looks to be cargo cult at this point right? (if so, implies the use of READ_ONCE() in patch 3 isn't needed either) > The rest looks good: > > Reviewed-by: Christoph Hellwig <hch@lst.de> Thanks.
diff --git a/block/blk-core.c b/block/blk-core.c index 97f8bc8d3a79..d93e3bb9a769 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -1061,20 +1061,32 @@ void update_io_ticks(struct block_device *part, unsigned long now, bool end) } static unsigned long __part_start_io_acct(struct block_device *part, - unsigned int sectors, unsigned int op) + unsigned int sectors, unsigned int op, + unsigned long start_time) { const int sgrp = op_stat_group(op); - unsigned long now = READ_ONCE(jiffies); part_stat_lock(); - update_io_ticks(part, now, false); + update_io_ticks(part, start_time, false); part_stat_inc(part, ios[sgrp]); part_stat_add(part, sectors[sgrp], sectors); part_stat_local_inc(part, in_flight[op_is_write(op)]); part_stat_unlock(); - return now; + return start_time; +} + +/** + * bio_start_io_acct_time - start I/O accounting for bio based drivers + * @bio: bio to start account for + * @start_time: start time that should be passed back to bio_end_io_acct(). + */ +void bio_start_io_acct_time(struct bio *bio, unsigned long start_time) +{ + __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), + bio_op(bio), start_time); } +EXPORT_SYMBOL_GPL(bio_start_io_acct_time); /** * bio_start_io_acct - start I/O accounting for bio based drivers @@ -1084,14 +1096,15 @@ static unsigned long __part_start_io_acct(struct block_device *part, */ unsigned long bio_start_io_acct(struct bio *bio) { - return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), bio_op(bio)); + return __part_start_io_acct(bio->bi_bdev, bio_sectors(bio), + bio_op(bio), jiffies); } EXPORT_SYMBOL_GPL(bio_start_io_acct); unsigned long disk_start_io_acct(struct gendisk *disk, unsigned int sectors, unsigned int op) { - return __part_start_io_acct(disk->part0, sectors, op); + return __part_start_io_acct(disk->part0, sectors, op, jiffies); } EXPORT_SYMBOL(disk_start_io_acct); diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 9c95df26fc26..f35aea98bc35 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1258,6 +1258,7 @@ unsigned long disk_start_io_acct(struct gendisk *disk, unsigned int sectors, void disk_end_io_acct(struct gendisk *disk, unsigned int op, unsigned long start_time); +void bio_start_io_acct_time(struct bio *bio, unsigned long start_time); unsigned long bio_start_io_acct(struct bio *bio); void bio_end_io_acct_remapped(struct bio *bio, unsigned long start_time, struct block_device *orig_bdev);
bio_start_io_acct_time() interface is like bio_start_io_acct() that allows start_time to be passed in. This gives drivers the ability to defer starting accounting until after IO is issued (but possibily not entirely due to bio splitting). Signed-off-by: Mike Snitzer <snitzer@redhat.com> --- block/blk-core.c | 25 +++++++++++++++++++------ include/linux/blkdev.h | 1 + 2 files changed, 20 insertions(+), 6 deletions(-)