Re: [PATCH v4 7/7] md/raid10: simplify read request error handling

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

Hi Kuai,

Thanks for the feedback.

On Thu, Jul 16, 2026 at 17:52 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/7/10 18:15, Abd-Alrhman Masalkhi 写道:
>> raid10_read_request() currently handles bio completion, barrier
>> handling, and r10_bio lifetime management in several different error
>> paths. This results in duplicated cleanup logic and increases the risk
>> of introducing bugs in future modifications.
>>
>> Make raid10_read_request() return a status to its callers, consolidate
>> the read error paths, and free r10_bio from a single location in the
>> callers. Since the callers allocate r10_bio, they should also be
>> responsible for freeing it when the request fails.
>>
>> This makes the read path follow the same ownership model as the write
>> path and simplifies the error handling flow.
>>
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> Changes in v4:
>>   - No changes.
>>   - Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@gmail.com/
>>
>> Changes in v3:
>>   - No changes.
>>   - Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/
>>
>> Changes in v2:
>>   - Fix a compilation error (bi -> bio).
>>   - Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
>> ---
>>   drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
>>   1 file changed, 25 insertions(+), 20 deletions(-)
>
> I don't think patch 6 and 7 are actually simplifications, and 7 in particular introduces
> a fragile dependency I'd rather not take as-is. The diff lines also said this and I'd
> rather not take as-is.
>

I see your point about the fragile dependency, especially in
handle_read_error(). I'll drop those patchs from this series and revisit
the cleanup in a future submission if I can find a better approach that
doesn't introduce that dependency.

>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index d94c1f28a6f6..01162c483644 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
>>   	return true;
>>   }
>>   
>> -static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>> +static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
>>   				struct r10bio *r10_bio)
>>   {
>>   	struct r10conf *conf = mddev->private;
>> @@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>   
>>   	if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
>>   		bio_wouldblock_error(bio);
>> -		free_r10bio(r10_bio);
>> -		return;
>> +		return false;
>>   	}
>>   
>>   	rdev = read_balance(conf, r10_bio, &max_sectors);
>> @@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>   					    mdname(mddev), b,
>>   					    (unsigned long long)r10_bio->sector);
>>   		}
>> -		raid_end_bio_io(r10_bio);
>> -		return;
>> +		bio_io_error(bio);
>> +		goto err_allow_barrier;
>>   	}
>>   	if (err_rdev)
>>   		pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
>> @@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>   		bio = bio_submit_split_bioset(bio, max_sectors,
>>   					      &conf->bio_split);
>>   		wait_barrier(conf, false);
>> -		if (!bio) {
>> -			set_bit(R10BIO_Returned, &r10_bio->state);
>> -			goto err_handle;
>> -		}
>> +		if (!bio)
>> +			goto err_dec_pending;
>>   
>>   		r10_bio->master_bio = bio;
>>   		r10_bio->sectors = max_sectors;
>> @@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
>>   	read_bio->bi_private = r10_bio;
>>   	mddev_trace_remap(mddev, read_bio, r10_bio->sector);
>>   	submit_bio_noacct(read_bio);
>> -	return;
>> -err_handle:
>> +
>> +	return true;
>> +
>> +err_dec_pending:
>>   	atomic_dec(&rdev->nr_pending);
>> -	raid_end_bio_io(r10_bio);
>> +
>> +err_allow_barrier:
>> +	allow_barrier(conf);
>> +
>> +	return false;
>>   }
>>   
>>   static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
>> @@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>>   	memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>>   			conf->geo.raid_disks);
>>   
>> -	ret = true;
>>   	if (bio_data_dir(bio) == READ)
>> -		raid10_read_request(mddev, bio, r10_bio);
>> -	else {
>> +		ret = raid10_read_request(mddev, bio, r10_bio);
>> +	else
>>   		ret = raid10_write_request(mddev, bio, r10_bio);
>> -		if (!ret)
>> -			free_r10bio(r10_bio);
>> -	}
>> +
>> +	if (!ret)
>> +		free_r10bio(r10_bio);
>>   
>>   	return ret;
>>   }
>> @@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
>>   	sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
>>   	int chunk_sects = chunk_mask + 1;
>>   	int sectors = bio_sectors(bio);
>> +	bool write = bio_data_dir(bio) == WRITE;
>>   
>>   	if (unlikely(bio->bi_opf & REQ_PREFLUSH)
>>   	    && md_flush_request(mddev, bio))
>> @@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
>>   		sectors = chunk_sects -
>>   			(bio->bi_iter.bi_sector &
>>   			 (chunk_sects - 1));
>> -	if (!__make_request(mddev, bio, sectors))
>> +	if (!__make_request(mddev, bio, sectors) && write)
>>   		md_write_end(mddev);
>>   
>>   	/* In case raid10d snuck in to freeze_array */
>> @@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>>   
>>   	rdev_dec_pending(rdev, mddev);
>>   	r10_bio->state = 0;
>> -	raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
>> +	if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
>> +		free_r10bio(r10_bio);
>> +
>>   	/*
>>   	 * allow_barrier after re-submit to ensure no sync io
>>   	 * can be issued while regular io pending.
>
> -- 
> Thanks,
> Kuai

-- 
Best Regards,
Abd-Alrhman