[PATCH] md/raid5: fix inverted condition in rdev_set_badblocks call

[PATCH] md/raid5: fix inverted condition in rdev_set_badblocks call

[zhanghao]

Commit fd4d44c14ff6 ("md: mark rdev Faulty when badblocks setting fails")
attempted to unify badblock handling but introduced a logic inversion in
raid5_end_read_request().

The original check was:

    if (!(set_bad && In_sync && rdev_set_badblocks(...)))
            md_error(...);

which records badblocks when `set_bad && In_sync`, and calls md_error()
either when recording fails or when the rdev is not In_sync. The new
condition `if (!(set_bad && In_sync))` skips rdev_set_badblocks() exactly
during recovery/reshape (In_sync is set), so failed badblock updates no
longer trigger md_error() and the rdev is not marked Faulty. This allows
reconstruction to continue on a disk with a saturated badblock table,
potentially propagating corrupted data to the replacement disk.

Fix the condition to match the intended behavior: call rdev_set_badblocks()
when `set_bad && In_sync`, and let the callee handle md_error() on failure.

Fixes: fd4d44c14ff6 ("md: mark rdev Faulty when badblocks setting fails")
Cc: stable@vger.kernel.org
Signed-off-by: zhanghao <zh_exit@163.com>
---
 drivers/md/raid5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d52adf81065a..dcfb1bfa9ebf 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2866,7 +2866,7 @@ static void raid5_end_read_request(struct bio * bi)
 		else {
 			clear_bit(R5_ReadError, &sh->dev[i].flags);
 			clear_bit(R5_ReWrite, &sh->dev[i].flags);
-			if (!(set_bad && test_bit(In_sync, &rdev->flags)))
+			if (set_bad && test_bit(In_sync, &rdev->flags))
 				rdev_set_badblocks(rdev, sh->sector,
 						   RAID5_STRIPE_SECTORS(conf), 0);
 		}
-- 
2.43.0

Re: [PATCH] md/raid5: fix inverted condition in rdev_set_badblocks call

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] md/raid5: missing md_error() in raid5_end_read_request() when not setting badblocks
--

commit 3b60bf439402ec230798b452291264b5f0962518
Author: zhanghao <zh_exit@163.com>

md/raid5: fix inverted condition in rdev_set_badblocks call

This commit fixes an inverted condition in raid5_end_read_request() so that
rdev_set_badblocks() is called when set_bad and In_sync are set.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index b91545ce090de..d5e226c6cc68a 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2834,7 +2834,7 @@ static void raid5_end_read_request(struct bio * bi)
>  		else {
>  			clear_bit(R5_ReadError, &sh->dev[i].flags);
>  			clear_bit(R5_ReWrite, &sh->dev[i].flags);
> -			if (!(set_bad && test_bit(In_sync, &rdev->flags)))
> +			if (set_bad && test_bit(In_sync, &rdev->flags))
>  				rdev_set_badblocks(rdev, sh->sector,
>  						   RAID5_STRIPE_SECTORS(conf), 0);
>  		}

[Severity: High]
Does this need an else branch calling md_error(conf->mddev, rdev)?

Prior to commit fd4d44c14ff6 ("md: mark rdev Faulty when badblocks setting
fails"), the non-retry branch in raid5_end_read_request() had:

	if (!(set_bad && test_bit(In_sync, &rdev->flags) &&
	      rdev_set_badblocks(rdev, sh->sector, RAID5_STRIPE_SECTORS(conf), 0)))
		md_error(conf->mddev, rdev);

That called md_error() both when rdev_set_badblocks() failed and when
!(set_bad && test_bit(In_sync, &rdev->flags)) was true. Since
rdev_set_badblocks() only calls md_error() when it is actually invoked and
badblocks_set() fails, omitting the else branch leaves the cases where
!(set_bad && test_bit(In_sync, &rdev->flags)) holds without calling
md_error().

Specifically, in raid5_end_read_request(), retry == 0 and
!(set_bad && test_bit(In_sync, &rdev->flags)) occurs when:

1. A read error happens on a replacement device (R5_ReadRepl is set, leaving
   set_bad = 0 and retry = 0).
2. Consecutive read errors exceed max_nr_stripes (atomic_read(&rdev->read_errors)
   > max_nr_stripes, which logs "Too many read errors, failing device" with
   set_bad = 0 and retry = 0).
3. An uncorrectable read error or failed R5_ReWrite sets set_bad = 1 on a
   device where In_sync is not set.

In these paths, R5_ReadError and R5_ReWrite are cleared while R5_UPTODATE
remains clear, and neither Faulty nor a badblock is recorded on rdev. When
handle_stripe() -> analyse_stripe() runs again, will it continue to treat
rdev as healthy and re-issue the failing read in an infinite loop?

Should this be:

	if (set_bad && test_bit(In_sync, &rdev->flags))
		rdev_set_badblocks(rdev, sh->sector,
				   RAID5_STRIPE_SECTORS(conf), 0);
	else
		md_error(conf->mddev, rdev);

instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017074343.59513-1-zh_exit@163.com?part=1

[PATCH v2] md/raid5: fix inverted condition in rdev_set_badblocks call

[zhanghao]

Commit fd4d44c14ff6 ("md: mark rdev Faulty when badblocks setting fails")
attempted to unify badblock handling but introduced a logic inversion in
raid5_end_read_request().

The original check was:

    if (!(set_bad && In_sync && rdev_set_badblocks(...)))
            md_error(...);

which records badblocks when `set_bad && In_sync`, and calls md_error()
either when recording fails or when the rdev is not In_sync. The new
condition `if (!(set_bad && In_sync))` skips rdev_set_badblocks() exactly
during recovery/reshape (In_sync is set), so failed badblock updates no
longer trigger md_error() and the rdev is not marked Faulty. This allows
reconstruction to continue on a disk with a saturated badblock table,
potentially propagating corrupted data to the replacement disk.

Restore the intended behavior by recording badblocks when `set_bad &&
In_sync`, and calling md_error() in the else branch so that non-In_sync
rdevs, replacement devices, and the too-many-read-errors path are still
marked Faulty.

Fixes: fd4d44c14ff6 ("md: mark rdev Faulty when badblocks setting fails")
Cc: stable@vger.kernel.org
Signed-off-by: zhanghao <zh_exit@163.com>
---
v2:
- Add the else branch calling md_error() for the !(set_bad && In_sync)
  cases, as pointed out by Sashiko: replacement device read errors,
  the too-many-read-errors path, and non-In_sync uncorrectable reads
  would otherwise no longer mark the rdev Faulty.

 drivers/md/raid5.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d52adf81065a..25395a9c996a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2866,9 +2866,11 @@ static void raid5_end_read_request(struct bio * bi)
 		else {
 			clear_bit(R5_ReadError, &sh->dev[i].flags);
 			clear_bit(R5_ReWrite, &sh->dev[i].flags);
-			if (!(set_bad && test_bit(In_sync, &rdev->flags)))
+			if (set_bad && test_bit(In_sync, &rdev->flags))
 				rdev_set_badblocks(rdev, sh->sector,
 						   RAID5_STRIPE_SECTORS(conf), 0);
+			else
+				md_error(conf->mddev, rdev);
 		}
 	}
 	rdev_dec_pending(rdev, conf->mddev);
-- 
2.43.0