Re: [PATCH 4/6] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones

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

Pre-existing issues:
- [High] narrow_write_error() silently drops REQ_FUA, REQ_PREFLUSH, and REQ_SYNC flags during block-by-block write retries by overwriting the bio's op flags with a bare REQ_OP_WRITE.
--

--- Patch [4]: [PATCH 4/6] md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones ---
commit 2b72bc4b60daf0429fd1c6c62c181f2b0048e81d
Author: Mykola Marzhan <[email protected]>

md/raid1,raid10: keep REQ_NOMERGE on narrow_write_error() retry clones

This commit ensures that REQ_NOMERGE is retained on P2PDMA retry bios in
narrow_write_error(). It addresses an issue where resetting bi_opf to a
bare REQ_OP_WRITE dropped this protection, potentially allowing unsafe
merging.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 57f64e8901024..a300323211911 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c

[ ... ]

> @@ -2565,6 +2565,9 @@ static void narrow_write_error(struct r1bio *r1_bio, int i)
>  		}
>  
>  		wbio->bi_opf = REQ_OP_WRITE;

[Severity: High]
This isn't a bug introduced by this patch, but does this overwrite of
bi_opf with a bare REQ_OP_WRITE drop important operational flags?

If the original write contains data integrity flags like REQ_FUA, REQ_PREFLUSH,
or REQ_SYNC, losing these flags during a retry means the retried write might
only be placed in the disk's volatile cache.

Since bio_alloc_clone() already copies bi_opf from the master bio, overwriting
it here wipes out those flags, which could risk silent data corruption on power
loss if a transient failure triggers this retry path in narrow_write_error().

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;
>  		wbio->bi_iter.bi_sector = r1_bio->sector;
>  		wbio->bi_iter.bi_size = r1_bio->sectors << 9;
>  
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..f7ef903a3d4ec 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c

[ ... ]

> @@ -2831,6 +2831,9 @@ static void narrow_write_error(struct r10bio *r10_bio, int i)
>  		wbio->bi_iter.bi_sector = wsector +
>  				   choose_data_offset(r10_bio, rdev);
>  		wbio->bi_opf = REQ_OP_WRITE;

[Severity: High]
This isn't a bug introduced by this patch, but similarly here in raid10
narrow_write_error(), does assigning a bare REQ_OP_WRITE silently drop flags
like REQ_FUA and REQ_PREFLUSH?

If an original write has these flags set, the retry might succeed without
them, leading the array to consider the disk perfectly synced while the data
only resides in the disk's volatile cache.

> +		/* Keep P2PDMA retry bios unmergeable, like the original */
> +		if (md_bio_is_p2pdma(wbio))
> +			wbio->bi_opf |= REQ_NOMERGE;
>  
>  		if (submit_bio_wait(wbio) &&
>  		    !rdev_set_badblocks(rdev, wsector, sectors, 0)) {

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