block: fix iolat timestamp and restore accounting semantics

PATCH In review Dennis Zhou 3 participants 6 messages Dec 11, 2018 – Dec 13, 2018

Subsystems (3)

Block layer cgroup cgroup blkio

lore

Share & tools

lore

Dennis Zhou

author

2018-12-11 23:01 UTC #0 lore raw

PATCH v2 1/1 block: fix iolat timestamp and restore accounting semantics

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.

The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.

This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().

[1] https://lore.kernel.org/lkml/[email protected]/

Fixes 5cdf2e3fea5e ("blkcg: associate blkg when associating a device")

Signed-off-by Dennis Zhou <[email protected]>

Cc Josef Bacik <[email protected]>

---
v2:
- Switched to reusing BIO_QUEUE_ENTERED rather than adding a second timestamp to the bio struct.

 block/blk-iolatency.c |  2 +-
 block/blk-mq.c        | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

block/blk-iolatency.c +1 -1

index bee092727cad..e408282bdc4c 100644
@@ -593,7 +593,7 @@ static void blkcg_iolatency_done_bio(struct rq_qos *rqos, struct bio *bio)
 	bool enabled = false;
 
 	blkg = bio->bi_blkg;
-	if (!blkg)
+	if (!blkg || !bio_flagged(bio, BIO_QUEUE_ENTERED))
 		return;
 
 	iolat = blkg_to_lat(bio->bi_blkg);

block/blk-mq.c +12 -0

index 9690f4f8de7e..05ac940e6671 100644
@@ -1920,6 +1920,17 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 	struct request *same_queue_rq = NULL;
 	blk_qc_t cookie;
 
+	/*
+	 * The flag BIO_QUEUE_ENTERED is used for two purposes.  First, it
+	 * determines if a bio is being split and has already entered the queue.
+	 * This happens in blk_queue_split() where we can recursively call
+	 * generic_make_request().  The second use is to mark bios that will
+	 * call rq_qos_throttle() and subseqently blk_mq_get_request().  These
+	 * are the bios that fail plug-merging and bio-merging with the primary
+	 * use case for this being the blk-iolatency controller.
+	 */
+	bio_clear_flag(bio, BIO_QUEUE_ENTERED);
+
 	blk_queue_bounce(q, &bio);
 
 	blk_queue_split(q, &bio);

@@ -1934,6 +1945,7 @@ static blk_qc_t blk_mq_make_request(struct request_queue *q, struct bio *bio)
 	if (blk_mq_sched_bio_merge(q, bio))
 		return BLK_QC_T_NONE;
 
+	bio_set_flag(bio, BIO_QUEUE_ENTERED);
 	rq_qos_throttle(q, bio);
 
 	rq = blk_mq_get_request(q, bio, &data);

Signature · 1 line

2.17.1

Jens Axboe

maintainer

2018-12-13 19:48 UTC #1 lore raw

Quoted 13 lines · Dennis Zhou

The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.
The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.

Quoted 7 lines

This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().

I'm having a hard time convincing myself that this is correct... Maybe we should just add a new flag for this specific use case? Or feel free to convince me otherwise.

Signature · 1 line

Jens Axboe

Josef Bacik

maintainer

2018-12-13 19:52 UTC #2 lore raw

Quoted 22 lines · Jens Axboe

Quoted 13 lines · Dennis Zhou
The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.
The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.
Quoted 7 lines
This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().

Quoted 4 lines

I'm having a hard time convincing myself that this is correct... Maybe we should just add a new flag for this specific use case? Or feel free to convince me otherwise.

I mean it'll work for now, but then when somebody else wants to do something similar *cough*io.weight*cough* it'll need a new flag. I kind of hate adding a new flag for every controller, but then again it's not like there's thousands of these things. I'm having a hard time coming up with a solution other than a per-tracker flag. As for this specific version, I still think it needs to be in iolatency itself, trying to make it generic just means it'll get fucked up again later down the line. Thanks,

Josef

Jens Axboe

maintainer

2018-12-13 19:59 UTC #3 lore raw

Quoted 30 lines · Josef Bacik

Quoted 24 lines · Jens Axboe
Quoted 15 lines · Dennis Zhou
The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.
The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.
Quoted 7 lines
This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().
Quoted 4 lines
I'm having a hard time convincing myself that this is correct... Maybe we should just add a new flag for this specific use case? Or feel free to convince me otherwise.

Quoted 8 lines

I mean it'll work for now, but then when somebody else wants to do something similar *cough*io.weight*cough* it'll need a new flag. I kind of hate adding a new flag for every controller, but then again it's not like there's thousands of these things. I'm having a hard time coming up with a solution other than a per-tracker flag. As for this specific version, I still think it needs to be in iolatency itself, trying to make it generic just means it'll get fucked up again later down the line. Thanks,

We definitely don't have that many flags, and I'd hate to add a per-whatever flag for this.

But do we need that? We really just need single flag for this, my main worry was overloading ENTERED. Especially since we're adding different clearing and setting for it. If we had a specific one, if it's set, we would need to disallow merging for it, I guess.

And there's already BIO_THROTTLED...

Signature · 1 line

Jens Axboe

Josef Bacik

maintainer

2018-12-13 20:03 UTC #4 lore raw

Quoted 43 lines · Jens Axboe

Quoted 30 lines · Josef Bacik
Quoted 24 lines · Jens Axboe
Quoted 15 lines · Dennis Zhou
The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.
The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.
Quoted 7 lines
This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().
Quoted 4 lines
I'm having a hard time convincing myself that this is correct... Maybe we should just add a new flag for this specific use case? Or feel free to convince me otherwise.
Quoted 8 lines
I mean it'll work for now, but then when somebody else wants to do something similar *cough*io.weight*cough* it'll need a new flag. I kind of hate adding a new flag for every controller, but then again it's not like there's thousands of these things. I'm having a hard time coming up with a solution other than a per-tracker flag. As for this specific version, I still think it needs to be in iolatency itself, trying to make it generic just means it'll get fucked up again later down the line. Thanks,
We definitely don't have that many flags, and I'd hate to add a per-whatever flag for this.

Quoted 7 lines

But do we need that? We really just need single flag for this, my main worry was overloading ENTERED. Especially since we're adding different clearing and setting for it. If we had a specific one, if it's set, we would need to disallow merging for it, I guess.
And there's already BIO_THROTTLED...

Oh well I guess we only really want to know if we saw the BIO, which should be able to be shared by all the rq_qos implementations. I think I'd rather just have a BIO_TRACKED to indicate it went through the normal rq_qos path. Thanks,

Josef

Jens Axboe

maintainer

2018-12-13 20:10 UTC #5 lore raw

Quoted 49 lines · Josef Bacik

Quoted 48 lines · Jens Axboe
Quoted 30 lines · Josef Bacik
Quoted 24 lines · Jens Axboe
Quoted 15 lines · Dennis Zhou
The blk-iolatency controller measures the time from rq_qos_throttle() to rq_qos_done_bio() and attributes this time to the first bio that needs to create the request. This means if a bio is plug-mergeable or bio-mergeable, it gets to bypass the blk-iolatency controller.
The recent series, to tag all bios w/ blkgs in [1] changed the timing incorrectly as well. First, the iolatency controller was tagging bios and using that information if it should process it in rq_qos_done_bio(). However, now that all bios are tagged, this caused the atomic_t for the struct rq_wait inflight count to underflow resulting in a stall. Second, now the timing was using the duration a bio from generic_make_request() rather than the timing mentioned above.
Quoted 7 lines
This patch fixes these issues by reusing the BLK_QUEUE_ENTERED flag to determine if a bio has entered the request layer and is responsible for starting a request. Stacked drivers don't recurse through blk_mq_make_request(), so the overhead of using time between generic_make_request() and the blk_mq_get_request() should be minimal. blk-iolatency now checks if this flag is set to determine if it should process the bio in rq_qos_done_bio().
Quoted 4 lines
I'm having a hard time convincing myself that this is correct... Maybe we should just add a new flag for this specific use case? Or feel free to convince me otherwise.
Quoted 8 lines
I mean it'll work for now, but then when somebody else wants to do something similar *cough*io.weight*cough* it'll need a new flag. I kind of hate adding a new flag for every controller, but then again it's not like there's thousands of these things. I'm having a hard time coming up with a solution other than a per-tracker flag. As for this specific version, I still think it needs to be in iolatency itself, trying to make it generic just means it'll get fucked up again later down the line. Thanks,
We definitely don't have that many flags, and I'd hate to add a per-whatever flag for this.
But do we need that? We really just need single flag for this, my main worry was overloading ENTERED. Especially since we're adding different clearing and setting for it. If we had a specific one, if it's set, we would need to disallow merging for it, I guess.

Quoted 7 lines

Quoted 2 lines
And there's already BIO_THROTTLED...
Oh well I guess we only really want to know if we saw the BIO, which should be able to be shared by all the rq_qos implementations. I think I'd rather just have a BIO_TRACKED to indicate it went through the normal rq_qos path. Thanks,

Agree, so how about renaming BIO_THROTTLED to BIO_TRACKED and using that?

Signature · 1 line

Jens Axboe