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

[sashiko-bot@kernel.org]

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