Re: [PATCH v3 7/8] md/raid10: skip futile retries on P2PDMA mapping failures

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

Pre-existing issues:
- [High] narrow_write_error() drops critical bio flags (e.g., REQ_FUA, REQ_PREFLUSH) during write error retries.
- [High] fix_read_error() incorrectly attributes read errors on a replacement device to the primary device.
--

--- Patch [7]: [PATCH v3 7/8] md/raid10: skip futile retries on P2PDMA mapping failures ---
commit 850a2ebdf179522973aa4c7332fa7233fdc47fc1
Author: Mykola Marzhan <[email protected]>
Subject: md/raid10: skip futile retries on P2PDMA mapping failures

This commit handles P2PDMA mapping failures by skipping futile retries
and retrying the write once as a whole range. It also leaves replacement
legs with the default fail-on-write-error policy to avoid silent holes
during rebuilds.

Fixes: 02666132403a ("md: propagate BLK_FEAT_PCI_P2PDMA from member devices to RAID device")

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index f7ef903a3d4ec..4c3da50250a5a 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -2814,9 +2831,12 @@ static void narrow_write_error(struct r10bio *r10_bio, int i)
>  		block_sectors = roundup(1 << rdev->badblocks.shift, lbs);
>  
>  	sector = r10_bio->sector;
> -	sectors = ((r10_bio->sector + block_sectors)
> -		   & ~(sector_t)(block_sectors - 1))
> -		- sector;
> +	if (coarse)
> +		sectors = sect_to_write;
> +	else
> +		sectors = ((r10_bio->sector + block_sectors)
> +			   & ~(sector_t)(block_sectors - 1))
> +			- sector;
>  
>  	while (sect_to_write) {
>  		struct bio *wbio;

[Severity: High]
This is a pre-existing issue, but does narrow_write_error() drop critical
bio flags like REQ_FUA and REQ_PREFLUSH here?

Further down in narrow_write_error(), the clone's operation flags are
overwritten with a bare write:

drivers/md/raid10.c:narrow_write_error() {
    ...
    wbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO, &mddev->bio_set);
    bio_trim(wbio, sector - bio->bi_iter.bi_sector, sectors);
    ...
    wbio->bi_opf = REQ_OP_WRITE;
    ...
}

If the master bio was submitted with REQ_FUA to guarantee data persistence,
these flags are stripped and the retry is issued as a normal asynchronous
write.

If the retry succeeds by placing data in the drive's volatile cache, the
master bio completes successfully. Could a power failure before the cache
is flushed to stable media result in silent data loss, violating the
original REQ_FUA guarantees?

(Note: The identical logic appears to be present in drivers/md/raid1.c in
narrow_write_error() as well).

[ ... ]
> @@ -2856,6 +2876,7 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>  {
>  	int slot = r10_bio->read_slot;
>  	struct bio *bio;
> +	bool p2pdma_error;
>  	struct r10conf *conf = mddev->private;
>  	struct md_rdev *rdev = r10_bio->devs[slot].rdev;
>  
> @@ -2868,17 +2889,24 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
>  	 * frozen.
>  	 */
>  	bio = r10_bio->devs[slot].bio;
> +	/* evaluate before the bio_put() below */
> +	p2pdma_error = bio->bi_status == BLK_STS_TARGET &&
> +		test_bit(R10BIO_P2PDMA, &r10_bio->state);
>  	bio_put(bio);
>  	r10_bio->devs[slot].bio = NULL;
>  
>  	if (mddev->ro)
>  		r10_bio->devs[slot].bio = IO_BLOCKED;
> -	else if (!test_bit(FailFast, &rdev->flags)) {
> +	else if (p2pdma_error) {
> +		/* Peer can't reach this member: just redirect the read. */
> +		r10_bio->devs[slot].bio = IO_BLOCKED;
> +	} else if (test_bit(FailFast, &rdev->flags)) {
> +		md_error(mddev, rdev);
> +	} else {
>  		freeze_array(conf, 1);
>  		fix_read_error(conf, mddev, r10_bio);
>  		unfreeze_array(conf);
> -	} else
> -		md_error(mddev, rdev);
> +	}
>  
>  	rdev_dec_pending(rdev, mddev);
>  	r10_bio->state = 0;

[Severity: High]
This is a pre-existing issue, but does fix_read_error() incorrectly attribute
read errors on a replacement device to the primary device?

When read_balance() selects the replacement device for a read and it fails,
handle_read_error() passes the context to fix_read_error().

However, fix_read_error() ignores the actual device that failed
(r10_bio->devs[slot].rdev) and blindly selects the primary device:

drivers/md/raid10.c:fix_read_error() {
    ...
    /* still own a reference to this rdev, so it cannot
     * have been cleared recently.
     */
    rdev = conf->mirrors[d].rdev;

    if (test_bit(Faulty, &rdev->flags))
    ...
}

Later, bad blocks are also recorded on the primary device:

drivers/md/raid10.c:fix_read_error() {
    ...
    int dn = r10_bio->devs[slot].devnum;
    rdev = conf->mirrors[dn].rdev;

    if (!rdev_set_badblocks(
                rdev,
                r10_bio->devs[slot].addr
                + sect,
                s, 0)) {
    ...
}

Could this result in the healthy primary device being penalized via
exceed_read_errors() and incorrectly targeted for bad block recording,
while the faulty replacement device continues to operate without
correction?

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