Skip to contentSkip to conversation

block: inflight counter fixes

PATCH In review Omar Sandoval 2 participants 4 messages Apr 26, 2018 Block layer

lore
Share & tools
lore

Omar Sandoval

author
#0 lore raw

cover block: inflight counter fixes

From: Omar Sandoval <[email protected]>

Hi, Jens,

I added a blktest (block/017) for the inflight counter after you mentioned that we should have one and it easily found a bug :) Patch 2 fixes the bug found by the test, and patch 1 fixes another bug I noticed. Based on Linus' master.

Thanks!

Omar Sandoval (2):
blk-mq: count allocated but not started requests in iostats inflight
blk-mq: fix sysfs inflight counter

 block/blk-mq.c            | 40 +++++++++++++++++++++++++++------------
 block/blk-mq.h            |  4 +++-
 block/genhd.c             | 12 ++++++++++++
 block/partition-generic.c | 10 ++++++----
 include/linux/genhd.h     |  4 +++-
 5 files changed, 52 insertions(+), 18 deletions(-)
Signature · 1 line
2.17.0
author
#1 lore raw

PATCH 1/2 blk-mq: count allocated but not started requests in iostats inflight

From: Omar Sandoval <[email protected]>

In the legacy block case, we increment the counter right after we allocate the request, not when the driver handles it. In both the legacy and blk-mq cases, part_inc_in_flight() is called from blk_account_io_start() right after we've allocated the request. blk-mq only considers requests started requests as inflight, but this is inconsistent with the legacy definition and the intention in the code. This removes the started condition and instead counts all allocated requests.

Fixes f299b7c7a9de ("blk-mq: provide internal in-flight variant")
Signed-off-by Omar Sandoval <[email protected]>
---
 block/blk-mq.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)
block/blk-mq.c +9 -12
index c3621453ad87..5450cbc61f8d 100644
@@ -95,18 +95,15 @@ static void blk_mq_check_inflight(struct blk_mq_hw_ctx *hctx,
 {
 	struct mq_inflight *mi = priv;
 
-	if (blk_mq_rq_state(rq) == MQ_RQ_IN_FLIGHT) {
-		/*
-		 * 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.
-		 */
-		if (rq->part == mi->part)
-			mi->inflight[0]++;
-		if (mi->part->partno)
-			mi->inflight[1]++;
-	}
+	/*
+	 * 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.
+	 */
+	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,
Signature · 1 line
2.17.0
author
#2 lore raw

PATCH 2/2 blk-mq: fix sysfs inflight counter

In reply to Omar Sandoval

From: Omar Sandoval <[email protected]>

When the blk-mq inflight implementation was added, /proc/diskstats was converted to use it, but /sys/block/$dev/inflight was not. Fix it by adding another helper to count in-flight requests by data direction.

Fixes f299b7c7a9de ("blk-mq: provide internal in-flight variant")
Signed-off-by Omar Sandoval <[email protected]>
---
 block/blk-mq.c            | 19 +++++++++++++++++++
 block/blk-mq.h            |  4 +++-
 block/genhd.c             | 12 ++++++++++++
 block/partition-generic.c | 10 ++++++----
 include/linux/genhd.h     |  4 +++-
 5 files changed, 43 insertions(+), 6 deletions(-)
block/blk-mq.c +19 -0
index 5450cbc61f8d..9ce9cac16c3f 100644
@@ -115,6 +115,25 @@ void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
 	blk_mq_queue_tag_busy_iter(q, blk_mq_check_inflight, &mi);
 }
 
+static void blk_mq_check_inflight_rw(struct blk_mq_hw_ctx *hctx,
+				     struct request *rq, void *priv,
+				     bool reserved)
+{
+	struct mq_inflight *mi = priv;
+
+	if (rq->part == mi->part)
+		mi->inflight[rq_data_dir(rq)]++;
+}
+
+void blk_mq_in_flight_rw(struct request_queue *q, struct hd_struct *part,
+			 unsigned int 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_rw, &mi);
+}
+
 void blk_freeze_queue_start(struct request_queue *q)
 {
 	int freeze_depth;
block/blk-mq.h +3 -1
index 89b5cd3a6c70..e1bb420dc5d6 100644
@@ -188,7 +188,9 @@ static inline bool blk_mq_hw_queue_mapped(struct blk_mq_hw_ctx *hctx)
 }
 
 void blk_mq_in_flight(struct request_queue *q, struct hd_struct *part,
-			unsigned int inflight[2]);
+		      unsigned int inflight[2]);
+void blk_mq_in_flight_rw(struct request_queue *q, struct hd_struct *part,
+			 unsigned int inflight[2]);
 
 static inline void blk_mq_put_dispatch_budget(struct blk_mq_hw_ctx *hctx)
 {
block/genhd.c +12 -0
index dc7e089373b9..c4513fe1adda 100644
@@ -82,6 +82,18 @@ void 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])
+{
+	if (q->mq_ops) {
+		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]);
+}
+
 struct hd_struct *__disk_get_part(struct gendisk *disk, int partno)
 {
 	struct disk_part_tbl *ptbl = rcu_dereference(disk->part_tbl);
block/partition-generic.c +6 -4
index 08dabcd8b6ae..db57cced9b98 100644
@@ -145,13 +145,15 @@ ssize_t part_stat_show(struct device *dev,
 		jiffies_to_msecs(part_stat_read(p, time_in_queue)));
 }
 
-ssize_t part_inflight_show(struct device *dev,
-			struct device_attribute *attr, char *buf)
+ssize_t part_inflight_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
 	struct hd_struct *p = dev_to_part(dev);
+	struct request_queue *q = part_to_disk(p)->queue;
+	unsigned int inflight[2];
 
-	return sprintf(buf, "%8u %8u\n", atomic_read(&p->in_flight[0]),
-		atomic_read(&p->in_flight[1]));
+	part_in_flight_rw(q, p, inflight);
+	return sprintf(buf, "%8u %8u\n", inflight[0], inflight[1]);
 }
 
 #ifdef CONFIG_FAIL_MAKE_REQUEST
include/linux/genhd.h +3 -1
index c826b0b5232a..6cb8a5789668 100644
@@ -368,7 +368,9 @@ static inline void free_part_stats(struct hd_struct *part)
 	part_stat_add(cpu, gendiskp, field, -subnd)
 
 void part_in_flight(struct request_queue *q, struct hd_struct *part,
-			unsigned int inflight[2]);
+		    unsigned int inflight[2]);
+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,
 			int rw);
 void part_inc_in_flight(struct request_queue *q, struct hd_struct *part,
Signature · 1 line
2.17.0
maintainer
#3 lore raw
In reply to Omar Sandoval
Quoted 8 lines · Omar Sandoval

From: Omar Sandoval <[email protected]>

Hi, Jens,

I added a blktest (block/017) for the inflight counter after you mentioned that we should have one and it easily found a bug :) Patch 2 fixes the bug found by the test, and patch 1 fixes another bug I noticed. Based on Linus' master.

That's hilarious! Awesome, thanks Omar.

Signature · 1 line
Jens Axboe