Message ID | 20210616030810.4901-1-edwardh@synology.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] block: fix trace completion for chained bio | expand |
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.
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
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.
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);