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

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 写道:

quoted

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 <redacted>
---
Changes in v4:
  - No changes.
  - Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@gmail.com/ (local)

Changes in v3:
  - No changes.
  - Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/ (local)

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/ (local)
---
  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.

quoted

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