linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3] block: fix trace completion for chained bio
@ 2021-06-16  3:08 edwardh
  2021-06-16  4:53 ` Christoph Hellwig
  0 siblings, 1 reply; 4+ messages in thread
From: edwardh @ 2021-06-16  3:08 UTC (permalink / raw)
  To: axboe, neilb
  Cc: linux-block, linux-kernel, s3t, bingjingc, cccheng, Edward Hsieh,
	Wade Liang

From: Edward Hsieh <edwardh@synology.com>

For chained bio, trace_block_bio_complete in bio_endio is currently called
only by the parent bio once upon all chained bio completed.
However, the sector and size for the parent bio are modified in bio_split.
Therefore, the size and sector of the complete events might not match the
queue events in blktrace.

The original fix of bio completion trace <fbbaf700e7b1> ("block: trace
completion of all bios.") wants multiple complete events to correspond
to one queue event but missed this.

The issue can be reproduced by md/raid5 read with bio cross chunks.

To fix, move trace completion into the loop for every chained bio to call.

To make sense of the tail call optimization, the bio_chained_endio
handing should be at the end of bio_endio(), move blk_throtl_bio_endio()
and bio_uninit() to the else closure of bio_chain_endio condition.
blk_throtl_bio_endio updates latency for the throttle group, and
considering the throttle group tracks the latency of the current device,
we should record the latency after all chained bio finish. And since the
resources of the chained bio are released by bio_put() in
__bio_chain_endio(), calling bio_uninit() for chained bio is not necessary.

Fixes: fbbaf700e7b1 ("block: trace completion of all bios.")
Reviewed-by: Wade Liang <wadel@synology.com>
Reviewed-by: BingJing Chang <bingjingc@synology.com>
Signed-off-by: Edward Hsieh <edwardh@synology.com>
---
 block/bio.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 44205dfb6b60..dcb23e75f083 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1375,8 +1375,7 @@ static inline bool bio_remaining_done(struct bio *bio)
  *
  *   bio_endio() can be called several times on a bio that has been chained
  *   using bio_chain().  The ->bi_end_io() function will only be called the
- *   last time.  At this point the BLK_TA_COMPLETE tracing event will be
- *   generated if BIO_TRACE_COMPLETION is set.
+ *   last time.
  **/
 void bio_endio(struct bio *bio)
 {
@@ -1389,6 +1388,11 @@ void bio_endio(struct bio *bio)
 	if (bio->bi_bdev)
 		rq_qos_done_bio(bio->bi_bdev->bd_disk->queue, bio);
 
+	if (bio->bi_bdev && bio_flagged(bio, BIO_TRACE_COMPLETION)) {
+		trace_block_bio_complete(bio->bi_bdev->bd_disk->queue, bio);
+		bio_clear_flag(bio, BIO_TRACE_COMPLETION);
+	}
+
 	/*
 	 * Need to have a real endio function for chained bios, otherwise
 	 * various corner cases will break (like stacking block devices that
@@ -1400,18 +1404,13 @@ void bio_endio(struct bio *bio)
 	if (bio->bi_end_io == bio_chain_endio) {
 		bio = __bio_chain_endio(bio);
 		goto again;
+	} else {
+		blk_throtl_bio_endio(bio);
+		/* release cgroup info */
+		bio_uninit(bio);
+		if (bio->bi_end_io)
+			bio->bi_end_io(bio);
 	}
-
-	if (bio->bi_bdev && bio_flagged(bio, BIO_TRACE_COMPLETION)) {
-		trace_block_bio_complete(bio->bi_bdev->bd_disk->queue, bio);
-		bio_clear_flag(bio, BIO_TRACE_COMPLETION);
-	}
-
-	blk_throtl_bio_endio(bio);
-	/* release cgroup info */
-	bio_uninit(bio);
-	if (bio->bi_end_io)
-		bio->bi_end_io(bio);
 }
 EXPORT_SYMBOL(bio_endio);
 
-- 
2.31.1


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: fix trace completion for chained bio
  2021-06-16  3:08 [PATCH v3] block: fix trace completion for chained bio edwardh
@ 2021-06-16  4:53 ` Christoph Hellwig
  2021-06-24  6:50   ` Edward Hsieh
  0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2021-06-16  4:53 UTC (permalink / raw)
  To: edwardh
  Cc: axboe, neilb, linux-block, linux-kernel, s3t, bingjingc, cccheng,
	Wade Liang

On Wed, Jun 16, 2021 at 11:08:10AM +0800, edwardh wrote:
> @@ -1400,18 +1404,13 @@ void bio_endio(struct bio *bio)
>  	if (bio->bi_end_io == bio_chain_endio) {
>  		bio = __bio_chain_endio(bio);
>  		goto again;
> +	} else {
> +		blk_throtl_bio_endio(bio);
> +		/* release cgroup info */
> +		bio_uninit(bio);
> +		if (bio->bi_end_io)
> +			bio->bi_end_io(bio);

No need for an else after a goto.

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: fix trace completion for chained bio
  2021-06-16  4:53 ` Christoph Hellwig
@ 2021-06-24  6:50   ` Edward Hsieh
  2021-06-24  7:02     ` Christoph Hellwig
  0 siblings, 1 reply; 4+ messages in thread
From: Edward Hsieh @ 2021-06-24  6:50 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: axboe, neilb, linux-block, linux-kernel, s3t, bingjingc, cccheng,
	Wade Liang



On 6/16/2021 12:53 PM, Christoph Hellwig wrote:
> On Wed, Jun 16, 2021 at 11:08:10AM +0800, edwardh wrote:
>> @@ -1400,18 +1404,13 @@ void bio_endio(struct bio *bio)
>>   	if (bio->bi_end_io == bio_chain_endio) {
>>   		bio = __bio_chain_endio(bio);
>>   		goto again;
>> +	} else {
>> +		blk_throtl_bio_endio(bio);
>> +		/* release cgroup info */
>> +		bio_uninit(bio);
>> +		if (bio->bi_end_io)
>> +			bio->bi_end_io(bio);
> 
> No need for an else after a goto.
> 

We are suggested by Neil Brown in the last version that from the
comment, the bio_chain_endio handling is used *only* to avoid
deep recursion so it should be at the end of the function.
Therefore, the position of blk_throtl_bio_endio() and bio_uninit()
are a bit odd.

We believe that blk_throtl_bio_endio() and bio_uninit() is in
the correct position now. And adding an else closure is our
attempt to make it more clear.

If it's not necessary then V2 patch should work to fix the
missing completion traces. Should we resend PATCH V2?

Thank you,
Edward

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v3] block: fix trace completion for chained bio
  2021-06-24  6:50   ` Edward Hsieh
@ 2021-06-24  7:02     ` Christoph Hellwig
  0 siblings, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2021-06-24  7:02 UTC (permalink / raw)
  To: Edward Hsieh
  Cc: Christoph Hellwig, axboe, neilb, linux-block, linux-kernel, s3t,
	bingjingc, cccheng, Wade Liang

On Thu, Jun 24, 2021 at 02:50:30PM +0800, Edward Hsieh wrote:
> We are suggested by Neil Brown in the last version that from the
> comment, the bio_chain_endio handling is used *only* to avoid
> deep recursion so it should be at the end of the function.
> Therefore, the position of blk_throtl_bio_endio() and bio_uninit()
> are a bit odd.

This is BS.  Placement of an else after a returning statement does
not change code generation.

> If it's not necessary then V2 patch should work to fix the
> missing completion traces. Should we resend PATCH V2?

Please send an updated version.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2021-06-24  7:02 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-16  3:08 [PATCH v3] block: fix trace completion for chained bio edwardh
2021-06-16  4:53 ` Christoph Hellwig
2021-06-24  6:50   ` Edward Hsieh
2021-06-24  7:02     ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).