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

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 <[email protected]>

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/[email protected]?part=2