[RFC PATCH 0/5] md/raid1: improve choose_best_rdev read balance

[RFC PATCH 0/5] md/raid1: improve choose_best_rdev read balance

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

Prefer a nonrot disk when one can take the read. When pending is
the same, use a real round-robin among the disks we may pick.

fio libaio direct=1, NVMe none, SATA mq-deadline (stock -> this):

- 2x GM9000: 1M QD16 j1 7.031 -> 13.886 GB/s, 50/50; 4k QD1 50/50
- 2x/3x HDWG740: 0.294 -> 0.514 / 0.630 GB/s
- GM9000+HDD seq: 2.468 or 0.280 -> 7.112 GB/s, 100/0 NVMe
- Fanxiang+HDD seq, HDD first: 0.278 -> 0.562 GB/s
- GM9000+HDD 4k QD8/16: clat -47% / -45%


Chen Cheng (5):
  md/raid1: balance reads across non-rotational disks
  md/raid1: do not move nonrot reads onto a rot disk
  md/raid1: do not send random reads to a rot disk
  md/raid1: use rot policy when no nonrot disk is readable
  md/raid1: clarify choose_best_rdev comments

 drivers/md/raid1.c | 122 ++++++++++++++++++++++++++++++++++++++-------
 drivers/md/raid1.h |   1 +
 2 files changed, 104 insertions(+), 19 deletions(-)

-- 
2.55.0

[RFC PATCH 1/5] md/raid1: balance reads across non-rotational disks

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

choose_best_rdev() picks one disk for each read.
min_pending is UINT_MAX; store it as unsigned int, like raid10.

Current:
1. Sequential read: stay on the current disk.
2. Switch only if should_choose_next() is true.
3. should_choose_next() needs bdev_io_opt() > 0.
4. Non-sequential read: pick the disk with the lowest nr_pending.
5. The compare uses strict '>'. Same pending keeps the first disk.

Problem:
1. Many client NVMe set optimal_io_size to 0. Then
   should_choose_next() never runs. One sequential stream stays on
   one disk. Why not use the idle disk?
2. Same for rot-only RAID1. One rot disk takes the whole stream.
   The other rot disk is idle. Why not use it?
3. Low-depth random reads often have the same pending. Why always
   stay on slot 0?

Improve:
1. If a sequential disk already has pending I/O, do not return it
   at once. Let pending pick an idle disk.
2. On nonrot arrays, if pending is the same, rotate a start slot
   (0..raid_disks-1).
3. Only bump read_rr when the array has a nonrot member.

Tested with fio libaio direct=1 (NVMe scheduler none, SATA
scheduler mq-deadline):

- 2x Predator GM9000 (optimal_io_size=0):
  a) 4k randread QD1 jobs=1: 0.084 GB/s, 100/0 -> 0.084 GB/s, 50/50
  b) 1M read QD16 jobs=1: 7.031 -> 13.886 GB/s (+97%), 66/34 -> 50/50
  c) 1M read QD16 jobs=2: 14.22 GB/s, 50/50 both sides

- 4x Intel MEMPEK1J016GA (Optane pmem, optimal_io_size=0):
  1M read QD16 jobs=1: ~0.85 GB/s on one member -> ~3.0+ GB/s,
  ~25% per disk

- RAID1 of two then three TOSHIBA HDWG740:
  2 disks, 1M read QD16 jobs=1: 0.294 GB/s, 100/0 -> 0.514 GB/s, 50/50
  3 disks, 1M read QD16 jobs=1: 0.294 GB/s, 100/0 -> 0.630 GB/s, 33/33/33
  2 and 3 disks, 4k randread QD1 jobs=1: 0.001 GB/s, all on
  one disk (rot-only random does not use read_rr)

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 25 +++++++++++++++++++++----
 drivers/md/raid1.h |  1 +
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f0646fb24371..319b24bcab5b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -780,27 +780,38 @@ static bool rdev_readable(struct md_rdev *rdev, struct r1bio *r1_bio)
 }
 
 struct read_balance_ctl {
 	sector_t closest_dist;
 	int closest_dist_disk;
-	int min_pending;
+	unsigned int min_pending;
 	int min_pending_disk;
 	int sequential_disk;
 	int readable_disks;
 };
 
+static int raid1_rr_pos(int disk, int start, int n)
+{
+	return ((disk % n) - start + n) % n;
+}
+
 static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 {
 	int disk;
+	int rr_start = 0;
+	bool has_nonrot = READ_ONCE(conf->nonrot_disks);
 	struct read_balance_ctl ctl = {
 		.closest_dist_disk      = -1,
 		.closest_dist           = MaxSector,
 		.min_pending_disk       = -1,
 		.min_pending            = UINT_MAX,
 		.sequential_disk	= -1,
 	};
 
+	if (has_nonrot)
+		rr_start = (unsigned int)atomic_inc_return(&conf->read_rr) %
+			   conf->raid_disks;
+
 	for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
 		struct md_rdev *rdev;
 		sector_t dist;
 		unsigned int pending;
 
@@ -819,11 +830,11 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 		dist = abs(r1_bio->sector -
 			   READ_ONCE(conf->mirrors[disk].head_position));
 
 		/* Don't change to another disk for sequential reads */
 		if (is_sequential(conf, disk, r1_bio)) {
-			if (!should_choose_next(conf, disk))
+			if (!should_choose_next(conf, disk) && !pending)
 				return disk;
 
 			/*
 			 * Add 'pending' to avoid choosing this disk if
 			 * there is other idle disk.
@@ -834,11 +845,16 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 			 * will be chosen.
 			 */
 			ctl.sequential_disk = disk;
 		}
 
-		if (ctl.min_pending > pending) {
+		if (ctl.min_pending > pending ||
+		    (has_nonrot && ctl.min_pending == pending &&
+		     ctl.min_pending_disk >= 0 &&
+		     raid1_rr_pos(disk, rr_start, conf->raid_disks) <
+		     raid1_rr_pos(ctl.min_pending_disk, rr_start,
+				  conf->raid_disks))) {
 			ctl.min_pending = pending;
 			ctl.min_pending_disk = disk;
 		}
 
 		if (ctl.closest_dist > dist) {
@@ -859,11 +875,11 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 	 * non-rotational, choose the disk with less pending request even the
 	 * disk is rotational, which might/might not be optimal for raids with
 	 * mixed ratation/non-rotational disks depending on workload.
 	 */
 	if (ctl.min_pending_disk != -1 &&
-	    (READ_ONCE(conf->nonrot_disks) || ctl.min_pending == 0))
+	    (has_nonrot || ctl.min_pending == 0))
 		return ctl.min_pending_disk;
 	else
 		return ctl.closest_dist_disk;
 }
 
@@ -3091,10 +3107,11 @@ static struct r1conf *setup_conf(struct mddev *mddev)
 		goto abort;
 
 	err = -EINVAL;
 	spin_lock_init(&conf->device_lock);
 	conf->raid_disks = mddev->raid_disks;
+	atomic_set(&conf->read_rr, -1);
 	rdev_for_each(rdev, mddev) {
 		int disk_idx = rdev->raid_disk;
 
 		if (disk_idx >= conf->raid_disks || disk_idx < 0)
 			continue;
diff --git a/drivers/md/raid1.h b/drivers/md/raid1.h
index c98d43a7ae99..d5de976d171d 100644
--- a/drivers/md/raid1.h
+++ b/drivers/md/raid1.h
@@ -54,10 +54,11 @@ struct r1conf {
 	struct raid1_info	*mirrors;	/* twice 'raid_disks' to
 						 * allow for replacements.
 						 */
 	int			raid_disks;
 	int			nonrot_disks;
+	atomic_t		read_rr;
 
 	spinlock_t		device_lock;
 
 	/* list of 'struct r1bio' that need to be processed by raid1d,
 	 * whether to retry a read, writeout a resync or recovery
-- 
2.55.0

Re: [RFC PATCH 1/5] md/raid1: balance reads across non-rotational disks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The use of a global atomic counter (`atomic_inc_return(&conf->read_rr)`) in the read fast-path introduces severe cacheline contention on high-concurrency NVMe arrays, heavily throttling peak read IOPS scalability.
--

--- Patch [1]: [RFC PATCH 1/5] md/raid1: balance reads across non-rotational disks ---
commit fe7aa5a93e1157781321a0273156730cd901aaec
Author: Chen Cheng <chencheng@fnnas.com>
md/raid1: balance reads across non-rotational disks

This commit optimizes read balancing for non-rotational disks in RAID1 by
distributing reads more evenly when pending IOs are equal. It introduces a
round-robin start slot rotation across disks to prevent low-depth random
reads from always sticking to the first slot.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index f0646fb24371f..319b24bcab5b7 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -799,6 +806,10 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  		.sequential_disk	= -1,
>  	};
>  
> +	if (has_nonrot)
> +		rr_start = (unsigned int)atomic_inc_return(&conf->read_rr) %
> +			   conf->raid_disks;

[Severity: Medium]
Will updating a global atomic counter on every read operation introduce
severe cacheline bouncing across CPU cores?

For high-concurrency NVMe arrays processing millions of IOPS, could this
global atomic increment in the read fast-path become a major scalability
bottleneck due to exclusive cacheline ownership?

> +
>  	for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
>  		struct md_rdev *rdev;
>  		sector_t dist;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818070646.1029149-1-chencheng@fnnas.com?part=1

[RFC PATCH 2/5] md/raid1: do not move nonrot reads onto a rot disk

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

The previous change: if a sequential disk already has pending I/O,
the next read can go to an idle disk.

Current:
1. A sequential disk can give the next read to an idle peer.
2. On a mixed array that peer can be a rot disk.
3. After a write, every disk has the same head_position.
4. Then every disk looks sequential.

Problem:
1. The first sequential disk in slot order may be a rot disk. Then
   we return it and never see the nonrot disk. We want the nonrot
   disk.
2. When pending is the same, rot and nonrot share one round-robin.
   A rot disk can win. We want the nonrot disk.

Improve:
1. If a nonrot disk is readable, do not stop on a sequential rot
   disk.
2. Remember sequential_disk once. A nonrot disk may replace a rot
   one.
3. If a nonrot disk is readable, do not keep a rot disk as the
   sequential fallback.
4. When pending is the same, prefer nonrot. Rotate only among
   nonrot disks.
5. Rot-only sequential reads still stay on the current disk when
   no peer is idle.

Tested with fio libaio direct=1 (NVMe scheduler none, SATA
scheduler mq-deadline), after a short write so both
head_positions match:

- RAID1 of Predator GM9000 + SATA HDD:
  1M read QD16 jobs=1, nonrot first:
    2.468 GB/s, 92/8 NVMe/HDD -> 7.112 GB/s, 100/0 NVMe
  1M read QD16 jobs=1, rot first:
    0.280 GB/s, 100/0 HDD -> 7.112 GB/s, 100/0 NVMe

- RAID1 of Fanxiang S103Pro + SATA HDD:
  1M read QD16 jobs=1, nonrot first:
    0.562 GB/s, 100/0 Fanxiang both sides
  1M read QD16 jobs=1, rot first:
    0.278 GB/s, 100/0 HDD -> 0.562 GB/s, 100/0 Fanxiang

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 42 +++++++++++++++++++++++++++++++++---------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 319b24bcab5b..36520e48826f 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -784,17 +784,36 @@ struct read_balance_ctl {
 	int closest_dist_disk;
 	unsigned int min_pending;
 	int min_pending_disk;
 	int sequential_disk;
 	int readable_disks;
+	bool min_pending_nonrot;
+	bool sequential_nonrot;
 };
 
 static int raid1_rr_pos(int disk, int start, int n)
 {
 	return ((disk % n) - start + n) % n;
 }
 
+static bool is_better_disk(unsigned int pending, int disk, bool nonrot,
+			   const struct read_balance_ctl *ctl,
+			   int rr_start, int n)
+{
+	if (ctl->min_pending_disk < 0)
+		return true;
+	if (ctl->min_pending < pending)
+		return false;
+	if (ctl->min_pending > pending)
+		return true;
+	if (nonrot && !ctl->min_pending_nonrot)
+		return true;
+	return nonrot && ctl->min_pending_nonrot &&
+	       raid1_rr_pos(disk, rr_start, n) <
+	       raid1_rr_pos(ctl->min_pending_disk, rr_start, n);
+}
+
 static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 {
 	int disk;
 	int rr_start = 0;
 	bool has_nonrot = READ_ONCE(conf->nonrot_disks);
@@ -812,10 +831,11 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 
 	for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
 		struct md_rdev *rdev;
 		sector_t dist;
 		unsigned int pending;
+		bool nonrot;
 
 		if (r1_bio->bios[disk] == IO_BLOCKED)
 			continue;
 
 		rdev = conf->mirrors[disk].rdev;
@@ -827,14 +847,16 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 			set_bit(R1BIO_FailFast, &r1_bio->state);
 
 		pending = atomic_read(&rdev->nr_pending);
 		dist = abs(r1_bio->sector -
 			   READ_ONCE(conf->mirrors[disk].head_position));
+		nonrot = test_bit(Nonrot, &rdev->flags);
 
 		/* Don't change to another disk for sequential reads */
 		if (is_sequential(conf, disk, r1_bio)) {
-			if (!should_choose_next(conf, disk) && !pending)
+			if (!should_choose_next(conf, disk) && !pending &&
+			    (nonrot || !has_nonrot))
 				return disk;
 
 			/*
 			 * Add 'pending' to avoid choosing this disk if
 			 * there is other idle disk.
@@ -842,21 +864,22 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 			pending++;
 			/*
 			 * If there is no other idle disk, this disk
 			 * will be chosen.
 			 */
-			ctl.sequential_disk = disk;
+			if (ctl.sequential_disk < 0 ||
+			    (nonrot && !ctl.sequential_nonrot)) {
+				ctl.sequential_disk = disk;
+				ctl.sequential_nonrot = nonrot;
+			}
 		}
 
-		if (ctl.min_pending > pending ||
-		    (has_nonrot && ctl.min_pending == pending &&
-		     ctl.min_pending_disk >= 0 &&
-		     raid1_rr_pos(disk, rr_start, conf->raid_disks) <
-		     raid1_rr_pos(ctl.min_pending_disk, rr_start,
-				  conf->raid_disks))) {
+		if (is_better_disk(pending, disk, nonrot, &ctl,
+				   rr_start, conf->raid_disks)) {
 			ctl.min_pending = pending;
 			ctl.min_pending_disk = disk;
+			ctl.min_pending_nonrot = nonrot;
 		}
 
 		if (ctl.closest_dist > dist) {
 			ctl.closest_dist = dist;
 			ctl.closest_dist_disk = disk;
@@ -865,11 +888,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 
 	/*
 	 * sequential IO size exceeds optimal iosize, however, there is no other
 	 * idle disk, so choose the sequential disk.
 	 */
-	if (ctl.sequential_disk != -1 && ctl.min_pending != 0)
+	if (ctl.sequential_disk != -1 && ctl.min_pending != 0 &&
+	    (ctl.sequential_nonrot || !has_nonrot))
 		return ctl.sequential_disk;
 
 	/*
 	 * If all disks are rotational, choose the closest disk. If any disk is
 	 * non-rotational, choose the disk with less pending request even the
-- 
2.55.0

Re: [RFC PATCH 2/5] md/raid1: do not move nonrot reads onto a rot disk

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Sequential and distance-based heuristics are broken for rotational disks when non-rotational disks fail because `has_nonrot` does not check readability.
- [High] Sequential reads on rotational disks will thrash in mixed arrays because idle sequential disks fail the early-return check and are artificially penalized.
--

--- Patch [2]: [RFC PATCH 2/5] md/raid1: do not move nonrot reads onto a rot disk ---
commit bee044f7822d405084d8b841ba855a8f7a81b6a4
Author: Chen Cheng <chencheng@fnnas.com>

md/raid1: do not move nonrot reads onto a rot disk

This patch updates the read balancing logic in RAID1 to prefer non-rotational
disks over rotational ones in mixed arrays. It modifies choose_best_rdev() to
ensure that non-rotational disks are favored during sequential I/O and
round-robin selection, aiming to prevent rotational disks from slowing down
faster NVMe or SSD drives.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 319b24bcab5b7..36520e48826f2 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -829,10 +849,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  		pending = atomic_read(&rdev->nr_pending);
>  		dist = abs(r1_bio->sector -
>  			   READ_ONCE(conf->mirrors[disk].head_position));
> +		nonrot = test_bit(Nonrot, &rdev->flags);
>  
>  		/* Don't change to another disk for sequential reads */
>  		if (is_sequential(conf, disk, r1_bio)) {
> -			if (!should_choose_next(conf, disk) && !pending)
> +			if (!should_choose_next(conf, disk) && !pending &&
> +			    (nonrot || !has_nonrot))
>  				return disk;

[Severity: High]
Does has_nonrot check if the non-rotational disks are actually readable?

Since has_nonrot is initialized as READ_ONCE(conf->nonrot_disks) at the top
of the function, it appears to evaluate to true if non-rotational disks are
configured in the array, even if they have failed, are rebuilding, or have
bad blocks.

If the non-rotational disks are unreadable, and we are evaluating a
sequential read on a rotational disk, (nonrot || !has_nonrot) will evaluate
to false. This causes the logic to bypass the early return for the
sequential disk.


[Severity: High]
Will this logic also cause sequential reads to thrash between idle rotational
disks in a healthy mixed array?

If the array has non-rotational disks (has_nonrot == true) that are currently
busy, but multiple rotational disks are idle:

1. A sequential read for HDD1 is evaluated.
2. The early return is bypassed because (nonrot || !has_nonrot) is false.
3. HDD1's pending count is artificially incremented to 1 via pending++.
4. HDD2 is evaluated, and since it is idle (pending == 0), it wins the
   min_pending contest against HDD1.

>  			/*
> @@ -844,17 +866,18 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  			 * If there is no other idle disk, this disk
>  			 * will be chosen.
>  			 */
> -			ctl.sequential_disk = disk;
> +			if (ctl.sequential_disk < 0 ||
> +			    (nonrot && !ctl.sequential_nonrot)) {
> +				ctl.sequential_disk = disk;
> +				ctl.sequential_nonrot = nonrot;
> +			}
>  		}
>  
> -		if (ctl.min_pending > pending ||
> -		    (has_nonrot && ctl.min_pending == pending &&
> -		     ctl.min_pending_disk >= 0 &&
> -		     raid1_rr_pos(disk, rr_start, conf->raid_disks) <
> -		     raid1_rr_pos(ctl.min_pending_disk, rr_start,
> -				  conf->raid_disks))) {
> +		if (is_better_disk(pending, disk, nonrot, &ctl,
> +				   rr_start, conf->raid_disks)) {
>  			ctl.min_pending = pending;
>  			ctl.min_pending_disk = disk;
> +			ctl.min_pending_nonrot = nonrot;
>  		}
>  
>  		if (ctl.closest_dist > dist) {
> @@ -867,7 +890,8 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  	 * sequential IO size exceeds optimal iosize, however, there is no other
>  	 * idle disk, so choose the sequential disk.
>  	 */
> -	if (ctl.sequential_disk != -1 && ctl.min_pending != 0)
> +	if (ctl.sequential_disk != -1 && ctl.min_pending != 0 &&
> +	    (ctl.sequential_nonrot || !has_nonrot))
>  		return ctl.sequential_disk;

[Severity: High]
Is it intentional that the sequential disk fallback is bypassed here?

Because HDD2 was idle (ctl.min_pending == 0), this sequential fallback
condition fails. The function will then fall through and return
ctl.min_pending_disk (HDD2), moving the I/O away from the sequential disk
(HDD1) and causing the read to ping-pong between the rotational disks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818070646.1029149-1-chencheng@fnnas.com?part=2

[RFC PATCH 3/5] md/raid1: do not send random reads to a rot disk

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

The previous change keeps sequential nonrot I/O off a rot disk.

Current:
1. Sequential reads stay on nonrot when a nonrot disk is readable.
2. Random reads still pick the disk with the lowest nr_pending.
3. Rot disks also join that compare.

Problem:
1. A nonrot disk is fast. It can have more pending I/O.
2. A rot disk is slow. It can have fewer pending I/O.
3. Then the next random read goes to the rot disk.
4. Why send a random 4k read to the slow disk?

Improve:
1. If a nonrot disk is readable, do not use rot disks for
   min_pending.
2. Random reads stay on nonrot disks.
3. If no nonrot disk is readable, still pick a rot disk by head
   position.

Tested with fio libaio direct=1 (NVMe scheduler none, SATA
scheduler mq-deadline):

- RAID1 of Predator GM9000 + SATA HDD:
  4k randread QD1 jobs=1: 0.097 GB/s, 100/0 NVMe -> 0.084 GB/s,
    100/0 NVMe. Both disks have pending 0, so stock already stayed
    on NVMe.
  4k randread QD8 jobs=1: 0.363 GB/s, 99.6/0.4 NVMe/HDD,
    clat 87 us -> 0.671 GB/s, 100/0 NVMe, clat 46 us (-47%)
  4k randread QD16 jobs=1: 0.668 GB/s, 99.7/0.3 NVMe/HDD,
    clat 95 us -> 1.215 GB/s, 100/0 NVMe, clat 52 us (-45%)

- RAID1 of Fanxiang S103Pro + SATA HDD:
  4k randread QD1 jobs=1: 0.077 GB/s, 100/0 Fanxiang -> 0.073 GB/s,
    100/0 Fanxiang
  4k randread QD8 jobs=1: 0.278 GB/s, 99.5/0.5 Fanxiang/HDD,
    clat 114 us -> 0.389 GB/s, 100/0 Fanxiang, clat 78 us (-31%)
  4k randread QD16 jobs=1: 0.395 GB/s, 99.6/0.4 Fanxiang/HDD,
    clat 162 us -> 0.398 GB/s, 100/0 Fanxiang, clat 159 us
    (already at the Fanxiang limit)

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 36520e48826f..523b55d42779 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -832,10 +832,11 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 	for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
 		struct md_rdev *rdev;
 		sector_t dist;
 		unsigned int pending;
 		bool nonrot;
+		bool can_pick;
 
 		if (r1_bio->bios[disk] == IO_BLOCKED)
 			continue;
 
 		rdev = conf->mirrors[disk].rdev;
@@ -848,15 +849,16 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 
 		pending = atomic_read(&rdev->nr_pending);
 		dist = abs(r1_bio->sector -
 			   READ_ONCE(conf->mirrors[disk].head_position));
 		nonrot = test_bit(Nonrot, &rdev->flags);
+		can_pick = nonrot || !has_nonrot;
 
 		/* Don't change to another disk for sequential reads */
 		if (is_sequential(conf, disk, r1_bio)) {
 			if (!should_choose_next(conf, disk) && !pending &&
-			    (nonrot || !has_nonrot))
+			    can_pick)
 				return disk;
 
 			/*
 			 * Add 'pending' to avoid choosing this disk if
 			 * there is other idle disk.
@@ -871,11 +873,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 				ctl.sequential_disk = disk;
 				ctl.sequential_nonrot = nonrot;
 			}
 		}
 
-		if (is_better_disk(pending, disk, nonrot, &ctl,
+		if (can_pick &&
+		    is_better_disk(pending, disk, nonrot, &ctl,
 				   rr_start, conf->raid_disks)) {
 			ctl.min_pending = pending;
 			ctl.min_pending_disk = disk;
 			ctl.min_pending_nonrot = nonrot;
 		}
-- 
2.55.0

[RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

has_nonrot selects mixed policy or rot-only policy.

Current:
1. has_nonrot is true if conf->nonrot_disks > 0.
2. nonrot_disks counts every nonrot disk.
3. A Faulty disk, a rebuild disk, and a WriteMostly disk still
   count.

Problem:
1. The array is NVMe + HDD. The NVMe fails. Only the HDD can
   take reads.
2. nonrot_disks is still 1. The code thinks this is a mixed
   array.
3. Sequential reads on the HDD do not stay on the HDD. Mixed
   policy will not keep a rot disk.
4. Every read still advances the nonrot round-robin, though no
   nonrot disk can take the read.

Improve:
1. Look at disks that can take this read.
2. If none of them is nonrot, use the rot-only policy.

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 523b55d42779..f476d4dea4be 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -793,10 +793,28 @@ struct read_balance_ctl {
 static int raid1_rr_pos(int disk, int start, int n)
 {
 	return ((disk % n) - start + n) % n;
 }
 
+static bool raid1_has_readable_nonrot(struct r1conf *conf,
+				      struct r1bio *r1_bio)
+{
+	int disk;
+
+	for (disk = 0; disk < conf->raid_disks * 2; disk++) {
+		struct md_rdev *rdev;
+
+		if (r1_bio->bios[disk] == IO_BLOCKED)
+			continue;
+		rdev = conf->mirrors[disk].rdev;
+		if (rdev_readable(rdev, r1_bio) &&
+		    test_bit(Nonrot, &rdev->flags))
+			return true;
+	}
+	return false;
+}
+
 static bool is_better_disk(unsigned int pending, int disk, bool nonrot,
 			   const struct read_balance_ctl *ctl,
 			   int rr_start, int n)
 {
 	if (ctl->min_pending_disk < 0)
@@ -814,11 +832,11 @@ static bool is_better_disk(unsigned int pending, int disk, bool nonrot,
 
 static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 {
 	int disk;
 	int rr_start = 0;
-	bool has_nonrot = READ_ONCE(conf->nonrot_disks);
+	bool has_nonrot = raid1_has_readable_nonrot(conf, r1_bio);
 	struct read_balance_ctl ctl = {
 		.closest_dist_disk      = -1,
 		.closest_dist           = MaxSector,
 		.min_pending_disk       = -1,
 		.min_pending            = UINT_MAX,
-- 
2.55.0

Re: [RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch leaves behind dead code related to `conf->nonrot_disks`.

Pre-existing issues:
- [High] Lockless iteration over `conf->mirrors` lacks `READ_ONCE()` and RCU protection, leading to TOCTOU NULL pointer dereferences and Use-After-Free during concurrent device removal.
--

--- Patch [4]: [RFC PATCH 4/5] md/raid1: use rot policy when no nonrot disk is readable ---
commit 53dd92b2fd0628bc1951a803d06d7fa324cf3f85
Author: Chen Cheng <chencheng@fnnas.com>

md/raid1: use rot policy when no nonrot disk is readable

has_nonrot selects mixed policy or rot-only policy. Currently, has_nonrot
is true if conf->nonrot_disks > 0, which counts every nonrot disk including
faulty or WriteMostly disks. This commit updates the logic to look at disks
that can actually take the read, and if none are nonrot, uses the rot-only
policy.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 523b55d427791..f476d4dea4be5 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -795,6 +795,24 @@ static int raid1_rr_pos(int disk, int start, int n)
>  	return ((disk % n) - start + n) % n;
>  }
>  
> +static bool raid1_has_readable_nonrot(struct r1conf *conf,
> +				      struct r1bio *r1_bio)
> +{
> +	int disk;
> +
> +	for (disk = 0; disk < conf->raid_disks * 2; disk++) {
> +		struct md_rdev *rdev;
> +
> +		if (r1_bio->bios[disk] == IO_BLOCKED)
> +			continue;
> +		rdev = conf->mirrors[disk].rdev;
> +		if (rdev_readable(rdev, r1_bio) &&
> +		    test_bit(Nonrot, &rdev->flags))
> +			return true;

[Severity: High]
This is a pre-existing issue, but does this lockless iteration over
conf->mirrors lack READ_ONCE() and RCU protection?

In raid1_has_readable_nonrot(), we read conf->mirrors[disk].rdev locklessly.
If HOT_REMOVE_DISK runs concurrently, raid1_remove_conf() can zero the rdev
pointer. Without READ_ONCE(), the compiler might reload the pointer between
the NULL check inside rdev_readable() and test_bit(Nonrot, &rdev->flags),
causing a NULL pointer dereference.

Additionally, since raid1_read_request() (which calls read_balance() and then
raid1_has_readable_nonrot()) does not hold rcu_read_lock(), could the reader
be preempted, allowing md_kick_rdev_from_array() to complete synchronize_rcu()
and free the rdev, resulting in a Use-After-Free?

This identical pattern is also present in choose_best_rdev().

> +	}
> +	return false;
> +}
> +

[ ... ]

> @@ -816,7 +834,7 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>  {
>  	int disk;
>  	int rr_start = 0;
> -	bool has_nonrot = READ_ONCE(conf->nonrot_disks);
> +	bool has_nonrot = raid1_has_readable_nonrot(conf, r1_bio);

[Severity: Low]
Does this change leave conf->nonrot_disks as dead code?

By removing the only reader of conf->nonrot_disks in choose_best_rdev(),
should the nonrot_disks field also be removed from struct r1conf in
drivers/md/raid1.h, as well as the updates to it in raid1_add_conf() and
raid1_remove_conf()?

>  	struct read_balance_ctl ctl = {
>  		.closest_dist_disk      = -1,
>  		.closest_dist           = MaxSector,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818070646.1029149-1-chencheng@fnnas.com?part=4

[RFC PATCH 5/5] md/raid1: clarify choose_best_rdev comments

[Chen Cheng]

From: Chen Cheng <chencheng@fnnas.com>

Write the rot/nonrot read policy in short comments next to the code.

No functional change.

Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
 drivers/md/raid1.c | 48 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index f476d4dea4be..897eef3a022d 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -788,15 +788,17 @@ struct read_balance_ctl {
 	int readable_disks;
 	bool min_pending_nonrot;
 	bool sequential_nonrot;
 };
 
+/* Offset from rr start. Replacement uses the same slot as primary. */
 static int raid1_rr_pos(int disk, int start, int n)
 {
 	return ((disk % n) - start + n) % n;
 }
 
+/* True if some readable member is nonrot. */
 static bool raid1_has_readable_nonrot(struct r1conf *conf,
 				      struct r1bio *r1_bio)
 {
 	int disk;
 
@@ -811,10 +813,11 @@ static bool raid1_has_readable_nonrot(struct r1conf *conf,
 			return true;
 	}
 	return false;
 }
 
+/* Lower pending wins. Same pending: prefer nonrot, then rr order. */
 static bool is_better_disk(unsigned int pending, int disk, bool nonrot,
 			   const struct read_balance_ctl *ctl,
 			   int rr_start, int n)
 {
 	if (ctl->min_pending_disk < 0)
@@ -828,10 +831,24 @@ static bool is_better_disk(unsigned int pending, int disk, bool nonrot,
 	return nonrot && ctl->min_pending_nonrot &&
 	       raid1_rr_pos(disk, rr_start, n) <
 	       raid1_rr_pos(ctl->min_pending_disk, rr_start, n);
 }
 
+/*
+ * Choose a readable disk for this read.
+ *
+ * Prefer nonrot. Use rot only if no nonrot disk is readable.
+ *
+ * Sequential idle: keep this disk. Mixed array: do not keep a
+ * rot disk (after a write every disk looks sequential).
+ * Sequential busy: try an idle disk of the same class. If none
+ * is idle, keep the sequential disk.
+ *
+ * Else fewest pending I/Os among disks we may pick. Same
+ * pending: nonrot, then round-robin. Rot-only with no idle
+ * disk: closest head.
+ */
 static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 {
 	int disk;
 	int rr_start = 0;
 	bool has_nonrot = raid1_has_readable_nonrot(conf, r1_bio);
@@ -867,26 +884,32 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 
 		pending = atomic_read(&rdev->nr_pending);
 		dist = abs(r1_bio->sector -
 			   READ_ONCE(conf->mirrors[disk].head_position));
 		nonrot = test_bit(Nonrot, &rdev->flags);
+		/*
+		 * If a nonrot disk is readable, pick only nonrot.
+		 * Else we must use rot.
+		 */
 		can_pick = nonrot || !has_nonrot;
 
-		/* Don't change to another disk for sequential reads */
+		/*
+		 * Idle sequential disk: return it now, unless
+		 * should_choose_next() wants another disk.
+		 * Mixed array: do not return a rot disk.
+		 */
 		if (is_sequential(conf, disk, r1_bio)) {
 			if (!should_choose_next(conf, disk) && !pending &&
 			    can_pick)
 				return disk;
 
-			/*
-			 * Add 'pending' to avoid choosing this disk if
-			 * there is other idle disk.
-			 */
+			/* Make an idle disk win over this busy one. */
 			pending++;
 			/*
-			 * If there is no other idle disk, this disk
-			 * will be chosen.
+			 * Remember the first sequential disk.
+			 * A nonrot disk may replace a rot disk.
+			 * A later nonrot disk may not replace an earlier one.
 			 */
 			if (ctl.sequential_disk < 0 ||
 			    (nonrot && !ctl.sequential_nonrot)) {
 				ctl.sequential_disk = disk;
 				ctl.sequential_nonrot = nonrot;
@@ -906,22 +929,21 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
 			ctl.closest_dist_disk = disk;
 		}
 	}
 
 	/*
-	 * sequential IO size exceeds optimal iosize, however, there is no other
-	 * idle disk, so choose the sequential disk.
+	 * Keep the sequential disk if no idle peer should take it.
+	 * If a nonrot disk is readable: keep only a nonrot sequential disk.
+	 * If not: an idle rot disk may take it.
 	 */
 	if (ctl.sequential_disk != -1 && ctl.min_pending != 0 &&
 	    (ctl.sequential_nonrot || !has_nonrot))
 		return ctl.sequential_disk;
 
 	/*
-	 * If all disks are rotational, choose the closest disk. If any disk is
-	 * non-rotational, choose the disk with less pending request even the
-	 * disk is rotational, which might/might not be optimal for raids with
-	 * mixed ratation/non-rotational disks depending on workload.
+	 * No readable nonrot disk: closest disk, unless some disk is idle.
+	 * Some readable nonrot disk: that nonrot disk with fewest pending I/Os.
 	 */
 	if (ctl.min_pending_disk != -1 &&
 	    (has_nonrot || ctl.min_pending == 0))
 		return ctl.min_pending_disk;
 	else
-- 
2.55.0