diff mbox

[1/1] block: Use per-cpu partition in_flight counters.

Message ID tc260ulmupc.fsf@vmbox.dyndns.org (mailing list archive)
State New, archived
Headers show

Commit Message

Michael Callahan May 10, 2016, 10:09 p.m. UTC 
Move the partition in_flight counters from hd_struct to disk_stats so
that they become tracked on a per-cpu basis.

Sign-off-by: Michael Callahan <michaelcallahan@fb.com>
---

This patch is incomplete as it just comments out use of in_flight in dm.c as
that code tracks io statistics in it's own special way.  Any thoughts on
how to fix dm.c are welcome.

---
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jens Axboe May 10, 2016, 10:18 p.m. UTC | #1 
On 05/10/2016 04:09 PM, Michael Callahan wrote:
> Move the partition in_flight counters from hd_struct to disk_stats so
> that they become tracked on a per-cpu basis.
>
> Sign-off-by: Michael Callahan <michaelcallahan@fb.com>

^^ Signed-off-by

> ---
>
> This patch is incomplete as it just comments out use of in_flight in dm.c as
> that code tracks io statistics in it's own special way.  Any thoughts on
> how to fix dm.c are welcome.

This has been done and rejected before. The problem is that you are now 
having to do a full per cpu loop in part_in_flight(), which is a 
non-starter. Unless you can make that part more clever, than it's not 
going to be a great idea.

Generally, doing patches like this, you should include test results 
showing why this is a good idea. A good commit message is a "why" for 
the patch.
Michael Callahan May 16, 2016, 8:06 p.m. UTC | #2 
I don't feel strongly about pursuing this one.  I could not measure a
difference either way.

This mailing list appears to be for patch submissions.  Where are the
requests for review and discussion of half baked ideas happening?


On Tue, May 10, 2016 at 4:18 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 05/10/2016 04:09 PM, Michael Callahan wrote:
>>
>> Move the partition in_flight counters from hd_struct to disk_stats so
>> that they become tracked on a per-cpu basis.
>>
>> Sign-off-by: Michael Callahan <michaelcallahan@fb.com>
>
>
> ^^ Signed-off-by
>
>> ---
>>
>> This patch is incomplete as it just comments out use of in_flight in dm.c
>> as
>> that code tracks io statistics in it's own special way.  Any thoughts on
>> how to fix dm.c are welcome.
>
>
> This has been done and rejected before. The problem is that you are now
> having to do a full per cpu loop in part_in_flight(), which is a
> non-starter. Unless you can make that part more clever, than it's not going
> to be a great idea.
>
> Generally, doing patches like this, you should include test results showing
> why this is a good idea. A good commit message is a "why" for the patch.
>
> --
> Jens Axboe
>
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jens Axboe May 16, 2016, 9:17 p.m. UTC | #3 
On 05/16/2016 02:06 PM, Michael Callahan wrote:
> I don't feel strongly about pursuing this one.  I could not measure a
> difference either way.

That answers that, then :-)

> This mailing list appears to be for patch submissions.  Where are the
> requests for review and discussion of half baked ideas happening?

The list serves both purposes. If you're just tossing something out 
there, prefixing the submission with RFC (request for comment) could 
help. Otherwise people assume it's a full patch submission, something 
that you are ready to land in mainline.
diff mbox

Patch

diff --git a/block/bio.c b/block/bio.c
index 807d25e..35d185f 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1686,7 +1686,7 @@  void generic_start_io_acct(int rw, unsigned long sectors,
 	part_round_stats(cpu, part);
 	part_stat_inc(cpu, part, ios[rw]);
 	part_stat_add(cpu, part, sectors[rw], sectors);
-	part_inc_in_flight(part, rw);
+	part_inc_in_flight(cpu, part, rw);
 
 	part_stat_unlock();
 }
@@ -1700,7 +1700,7 @@  void generic_end_io_acct(int rw, struct hd_struct *part,
 
 	part_stat_add(cpu, part, ticks[rw], duration);
 	part_round_stats(cpu, part);
-	part_dec_in_flight(part, rw);
+	part_dec_in_flight(cpu, part, rw);
 
 	part_stat_unlock();
 }
diff --git a/block/blk-core.c b/block/blk-core.c
index b60537b..b8656a0 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -2303,7 +2303,7 @@  void blk_account_io_done(struct request *req)
 		part_stat_inc(cpu, part, ios[rw]);
 		part_stat_add(cpu, part, ticks[rw], duration);
 		part_round_stats(cpu, part);
-		part_dec_in_flight(part, rw);
+		part_dec_in_flight(cpu, part, rw);
 
 		hd_struct_put(part);
 		part_stat_unlock();
@@ -2361,7 +2361,7 @@  void blk_account_io_start(struct request *rq, bool new_io)
 			hd_struct_get(part);
 		}
 		part_round_stats(cpu, part);
-		part_inc_in_flight(part, rw);
+		part_inc_in_flight(cpu, part, rw);
 		rq->part = part;
 	}
 
diff --git a/block/blk-merge.c b/block/blk-merge.c
index 2613531..f564c5d 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -633,7 +633,7 @@  static void blk_account_io_merge(struct request *req)
 		part = req->part;
 
 		part_round_stats(cpu, part);
-		part_dec_in_flight(part, rq_data_dir(req));
+		part_dec_in_flight(cpu, part, rq_data_dir(req));
 
 		hd_struct_put(part);
 		part_stat_unlock();
diff --git a/block/partition-generic.c b/block/partition-generic.c
index d7eb77e..7054d5d 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -141,8 +141,8 @@  ssize_t part_inflight_show(struct device *dev,
 {
 	struct hd_struct *p = dev_to_part(dev);
 
-	return sprintf(buf, "%8u %8u\n", atomic_read(&p->in_flight[0]),
-		atomic_read(&p->in_flight[1]));
+	return sprintf(buf, "%8u %8u\n", part_stat_read(p, in_flight[0]),
+		part_stat_read(p, in_flight[1]));
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index 3d3ac13..620d755 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -719,8 +719,6 @@  static void start_io_acct(struct dm_io *io)
 	cpu = part_stat_lock();
 	part_round_stats(cpu, &dm_disk(md)->part0);
 	part_stat_unlock();
-	atomic_set(&dm_disk(md)->part0.in_flight[rw],
-		atomic_inc_return(&md->pending[rw]));
 
 	if (unlikely(dm_stats_used(&md->stats)))
 		dm_stats_account_io(&md->stats, bio->bi_rw, bio->bi_iter.bi_sector,
@@ -746,7 +744,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 */
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 5c70676..73a1e7e 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -84,6 +84,7 @@  struct disk_stats {
 	unsigned long ios[2];
 	unsigned long merges[2];
 	unsigned long ticks[2];
+	unsigned long in_flight[2];
 	unsigned long io_ticks;
 	unsigned long time_in_queue;
 };
@@ -119,7 +120,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
@@ -395,23 +395,24 @@  static inline void free_part_stats(struct hd_struct *part)
 #define part_stat_sub(cpu, gendiskp, field, subnd)			\
 	part_stat_add(cpu, gendiskp, field, -subnd)
 
-static inline void part_inc_in_flight(struct hd_struct *part, int rw)
+static inline void part_inc_in_flight(int cpu, struct hd_struct *part, int rw)
 {
-	atomic_inc(&part->in_flight[rw]);
+	part_stat_inc(cpu, part, in_flight[rw]);
 	if (part->partno)
-		atomic_inc(&part_to_disk(part)->part0.in_flight[rw]);
+		part_stat_inc(cpu, &part_to_disk(part)->part0, in_flight[rw]);
 }
 
-static inline void part_dec_in_flight(struct hd_struct *part, int rw)
+static inline void part_dec_in_flight(int cpu, struct hd_struct *part, int rw)
 {
-	atomic_dec(&part->in_flight[rw]);
+	part_stat_dec(cpu, part, in_flight[rw]);
 	if (part->partno)
-		atomic_dec(&part_to_disk(part)->part0.in_flight[rw]);
+		part_stat_dec(cpu, &part_to_disk(part)->part0, in_flight[rw]);
 }
 
 static inline int part_in_flight(struct hd_struct *part)
 {
-	return atomic_read(&part->in_flight[0]) + atomic_read(&part->in_flight[1]);
+	return part_stat_read(part, in_flight[0]) +
+		part_stat_read(part, in_flight[1]);
 }
 
 static inline struct partition_meta_info *alloc_part_info(struct gendisk *disk)