md: use md_free_cloned_bio() in md_end_clone_io()

[PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Abd-Alrhman Masalkhi 2 months ago

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

Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Yu Kuai 1 month, 3 weeks ago

Hi,

在 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);
> -	percpu_ref_put(&mddev->active_io);
>   }
>   
>   static void md_clone_bio(struct mddev *mddev, struct bio **bio)

Same as I replied to v1,
This patch is no longer needed after following patch:
https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com

-- 
Thansk,
Kuai

Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Abd-Alrhman Masalkhi 1 month, 3 weeks ago

Hi Kuai,

On Sun, Apr 19, 2026 at 12:51 +0800, Yu Kuai wrote:
> Hi,
>
> 在 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);
>> -	percpu_ref_put(&mddev->active_io);
>>   }
>>   
>>   static void md_clone_bio(struct mddev *mddev, struct bio **bio)
>
> Same as I replied to v1,
> This patch is no longer needed after following patch:
> https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com

Thanks, I'll drop this patch.
>
> -- 
> Thansk,
> Kuai

-- 
Best Regards,
Abd-Alrhman

Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Li Nan 2 months ago


在 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

Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Abd-Alrhman Masalkhi 2 months ago

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

Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()

Posted by Li Nan 1 month, 4 weeks ago


在 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