Re: [PATCH v5 07/10] md/raid1,raid10: skip futile retries on P2PDMA mapping failures

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

Pre-existing issues:
- [High] The `rounddown` and `roundup` macros evaluate to a value but do not modify their arguments in place, causing bad block alignment to fail.
--

--- Patch [7]: [PATCH v5 07/10] md/raid1,raid10: skip futile retries on P2PDMA mapping failures ---
commit 7daa9c9f8cade5fa7f5dd4ab20ed50fa6c654eff
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: skip futile retries on P2PDMA mapping failures

This patch skips replacement and FailFast eviction for BLK_STS_P2PDMA errors
in raid1 and raid10, keeping the WriteErrorSeen flag so the range stays
fenced. It also bypasses block-by-block submit_bio_wait retries for
P2PDMA errors, instead recording the whole range as a bad block.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 19c4dec450de0..83ebf40237d57 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -2624,10 +2624,15 @@ static void handle_write_finished(struct r1conf *conf, struct r1bio *r1_bio)
>  			 * narrow down and record precise write
>  			 * errors.
>  			 */
> +			struct md_rdev *rdev = conf->mirrors[m].rdev;
>  
>  			fail = true;
> -			narrow_write_error(r1_bio, m);
> -			rdev_dec_pending(conf->mirrors[m].rdev,
> -					 conf->mddev);
> +			if (r1_bio->bios[m]->bi_status == BLK_STS_P2PDMA)
> +				rdev_set_badblocks(rdev, r1_bio->sector,
> +						   r1_bio->sectors, 0);

[Severity: High]
This is a pre-existing issue, but will these rdev_set_badblocks calls
fail to align correctly due to how rounddown and roundup are used
in the badblocks subsystem?

Looking at _badblocks_set in block/badblocks.c, the alignment macros
are evaluated but their results are discarded rather than assigned back to
s and next:

block/badblocks.c:_badblocks_set() {
    ...
    if (bb->shift > 0) {
        rounddown(s, 1 << bb->shift);
        roundup(next, 1 << bb->shift);
    }
    ...
}

Does this cause MD RAID configurations with bb->shift > 0 to track
unaligned, smaller bad blocks instead of the full chunk?

If so, this could allow later I/O requests to bypass the check and access
bad hardware sectors. The same discarded macro results appear to be present
in _badblocks_clear and _badblocks_check as well:

block/badblocks.c:_badblocks_clear() {
    ...
    roundup(s, 1 << bb->shift);
    rounddown(target, 1 << bb->shift);
    ...
}

block/badblocks.c:_badblocks_check() {
    ...
    rounddown(s, 1 << bb->shift);
    roundup(target, 1 << bb->shift);
    ...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7