[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

[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

[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

[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