* [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() @ 2026-04-15 8:19 Abd-Alrhman Masalkhi 2026-04-16 6:47 ` Li Nan 0 siblings, 1 reply; 4+ messages in thread From: Abd-Alrhman Masalkhi @ 2026-04-15 8:19 UTC (permalink / raw) To: song, yukuai, linan666; +Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi md_end_clone_io() and md_free_cloned_bio() share identical teardown logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and call bio_endio() afterwards. Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com> --- Changes in v2: - Reuse md_free_cloned_bio() instead of introducing a new helper - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com --- drivers/md/md.c | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/drivers/md/md.c b/drivers/md/md.c index ac71640ff3a8..8565566a447b 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio) { struct md_io_clone *md_io_clone = bio->bi_private; struct bio *orig_bio = md_io_clone->orig_bio; - struct mddev *mddev = md_io_clone->mddev; - - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false)) - md_bitmap_end(mddev, md_io_clone); - - if (bio->bi_status && !orig_bio->bi_status) - orig_bio->bi_status = bio->bi_status; - - if (md_io_clone->start_time) - bio_end_io_acct(orig_bio, md_io_clone->start_time); - bio_put(bio); + md_free_cloned_bio(bio); bio_endio(orig_bio); - percpu_ref_put(&mddev->active_io); } static void md_clone_bio(struct mddev *mddev, struct bio **bio) -- 2.43.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() 2026-04-15 8:19 [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() Abd-Alrhman Masalkhi @ 2026-04-16 6:47 ` Li Nan 2026-04-16 7:39 ` Abd-Alrhman Masalkhi 0 siblings, 1 reply; 4+ messages in thread From: Li Nan @ 2026-04-16 6:47 UTC (permalink / raw) To: Abd-Alrhman Masalkhi, song, yukuai, linan666; +Cc: linux-raid, linux-kernel 在 2026/4/15 16:19, Abd-Alrhman Masalkhi 写道: > md_end_clone_io() and md_free_cloned_bio() share identical teardown > logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and > call bio_endio() afterwards. > > Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com> > --- > Changes in v2: > - Reuse md_free_cloned_bio() instead of introducing a new helper > - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com > --- > drivers/md/md.c | 13 +------------ > 1 file changed, 1 insertion(+), 12 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index ac71640ff3a8..8565566a447b 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio) > { > struct md_io_clone *md_io_clone = bio->bi_private; > struct bio *orig_bio = md_io_clone->orig_bio; > - struct mddev *mddev = md_io_clone->mddev; > - > - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false)) > - md_bitmap_end(mddev, md_io_clone); > - > - if (bio->bi_status && !orig_bio->bi_status) > - orig_bio->bi_status = bio->bi_status; > - > - if (md_io_clone->start_time) > - bio_end_io_acct(orig_bio, md_io_clone->start_time); > > - bio_put(bio); > + md_free_cloned_bio(bio); > bio_endio(orig_bio); Is it safe to end orig_bio after putting active_io? > - percpu_ref_put(&mddev->active_io); > } > > static void md_clone_bio(struct mddev *mddev, struct bio **bio) -- Thanks, Nan ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() 2026-04-16 6:47 ` Li Nan @ 2026-04-16 7:39 ` Abd-Alrhman Masalkhi 2026-04-17 1:29 ` Li Nan 0 siblings, 1 reply; 4+ messages in thread From: Abd-Alrhman Masalkhi @ 2026-04-16 7:39 UTC (permalink / raw) To: Li Nan, song, yukuai, linan666; +Cc: linux-raid, linux-kernel Hi Nan, thank you for the feedback. On Thu, Apr 16, 2026 at 14:47 +0800, Li Nan wrote: > 在 2026/4/15 16:19, Abd-Alrhman Masalkhi 写道: >> md_end_clone_io() and md_free_cloned_bio() share identical teardown >> logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and >> call bio_endio() afterwards. >> >> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com> >> --- >> Changes in v2: >> - Reuse md_free_cloned_bio() instead of introducing a new helper >> - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com >> --- >> drivers/md/md.c | 13 +------------ >> 1 file changed, 1 insertion(+), 12 deletions(-) >> >> diff --git a/drivers/md/md.c b/drivers/md/md.c >> index ac71640ff3a8..8565566a447b 100644 >> --- a/drivers/md/md.c >> +++ b/drivers/md/md.c >> @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio) >> { >> struct md_io_clone *md_io_clone = bio->bi_private; >> struct bio *orig_bio = md_io_clone->orig_bio; >> - struct mddev *mddev = md_io_clone->mddev; >> - >> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false)) >> - md_bitmap_end(mddev, md_io_clone); >> - >> - if (bio->bi_status && !orig_bio->bi_status) >> - orig_bio->bi_status = bio->bi_status; >> - >> - if (md_io_clone->start_time) >> - bio_end_io_acct(orig_bio, md_io_clone->start_time); >> >> - bio_put(bio); >> + md_free_cloned_bio(bio); >> bio_endio(orig_bio); > > Is it safe to end orig_bio after putting active_io? As I understand it, active_io is primarily used to account for in-flight bios, allowing the array to be safely suspended once no bios are outstanding. I have made a diagram, while i am reviewing the md driver: mddev->active_io md driver: md_handle_request() mddev->active_io +1 pers->make_request() md_account_bio() +1 if (err) md_free_cloned_bio -1 mddev->active_io -1 lower driver: bio_endio() md_end_clone_io -1 As shown, it incremented in two places, the first is before passing orig_bio to pers->make_request(), and decremented after it returns (regardless of what make_request() does with it). and the second is when cloning the original bio, and it should be decremented after the cloned bio is released. In this context, it should not matter whether bio_endio(orig_bio) is called before or after decrementing active_io, as long as the cloned bio teardown is completed correctly. Am I missing anything that would make this unsafe? > >> - percpu_ref_put(&mddev->active_io); >> } >> >> static void md_clone_bio(struct mddev *mddev, struct bio **bio) > > -- > Thanks, > Nan > -- Best Regards, Abd-Alrhman ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() 2026-04-16 7:39 ` Abd-Alrhman Masalkhi @ 2026-04-17 1:29 ` Li Nan 0 siblings, 0 replies; 4+ messages in thread From: Li Nan @ 2026-04-17 1:29 UTC (permalink / raw) To: Abd-Alrhman Masalkhi, Li Nan, song, yukuai; +Cc: linux-raid, linux-kernel 在 2026/4/16 15:39, Abd-Alrhman Masalkhi 写道: > Hi Nan, > > thank you for the feedback. > > On Thu, Apr 16, 2026 at 14:47 +0800, Li Nan wrote: >> 在 2026/4/15 16:19, Abd-Alrhman Masalkhi 写道: >>> md_end_clone_io() and md_free_cloned_bio() share identical teardown >>> logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and >>> call bio_endio() afterwards. >>> >>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com> >>> --- >>> Changes in v2: >>> - Reuse md_free_cloned_bio() instead of introducing a new helper >>> - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com >>> --- >>> drivers/md/md.c | 13 +------------ >>> 1 file changed, 1 insertion(+), 12 deletions(-) >>> >>> diff --git a/drivers/md/md.c b/drivers/md/md.c >>> index ac71640ff3a8..8565566a447b 100644 >>> --- a/drivers/md/md.c >>> +++ b/drivers/md/md.c >>> @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio) >>> { >>> struct md_io_clone *md_io_clone = bio->bi_private; >>> struct bio *orig_bio = md_io_clone->orig_bio; >>> - struct mddev *mddev = md_io_clone->mddev; >>> - >>> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false)) >>> - md_bitmap_end(mddev, md_io_clone); >>> - >>> - if (bio->bi_status && !orig_bio->bi_status) >>> - orig_bio->bi_status = bio->bi_status; >>> - >>> - if (md_io_clone->start_time) >>> - bio_end_io_acct(orig_bio, md_io_clone->start_time); >>> >>> - bio_put(bio); >>> + md_free_cloned_bio(bio); >>> bio_endio(orig_bio); >> >> Is it safe to end orig_bio after putting active_io? > > As I understand it, active_io is primarily used to account for > in-flight bios, allowing the array to be safely suspended once no > bios are outstanding. > > I have made a diagram, while i am reviewing the md driver: > > mddev->active_io > > md driver: > md_handle_request() > mddev->active_io +1 > pers->make_request() > md_account_bio() +1 > if (err) > md_free_cloned_bio -1 > mddev->active_io -1 > > lower driver: > bio_endio() > md_end_clone_io -1 > > As shown, it incremented in two places, the first is before passing > orig_bio to pers->make_request(), and decremented after it returns > (regardless of what make_request() does with it). and the second > is when cloning the original bio, and it should be decremented after > the cloned bio is released. > > In this context, it should not matter whether bio_endio(orig_bio) is > called before or after decrementing active_io, as long as the cloned > bio teardown is completed correctly. > > Am I missing anything that would make this unsafe? > Thanks for your reply, LGTM. Reviewed-by: Li Nan <linan122@huawei.com> -- Thanks, Nan ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-04-17 1:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-04-15 8:19 [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io() Abd-Alrhman Masalkhi 2026-04-16 6:47 ` Li Nan 2026-04-16 7:39 ` Abd-Alrhman Masalkhi 2026-04-17 1:29 ` Li Nan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox