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

[yu kuai]

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@xxxxxxxxx>
> ---
> Changes in v4:
>   - No changes.
>   - Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@xxxxxxxxx/
>
> Changes in v3:
>   - No changes.
>   - Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@xxxxxxxxx/
>
> Changes in v2:
>   - Fix a compilation error (bi -> bio).
>   - Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@xxxxxxxxx/
> ---
>   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.

>
> 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