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

[Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>]

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