Re: [PATCH v4 4/9] md/raid1: don't use write-behind for P2PDMA bios

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

Pre-existing issues:
- [High] raid1_write_request() can block on resource allocations and serialization when processing a REQ_NOWAIT bio, violating non-blocking guarantees.
--

--- Patch [4]: [PATCH v4 4/9] md/raid1: don't use write-behind for P2PDMA bios ---
commit 1a1224f9f3e6ee3f02e62a2f1a4ff02688f39f0d
Author: Mykola Marzhan <[email protected]>

md/raid1: don't use write-behind for P2PDMA bios

This commit skips write-behind for P2PDMA bios as they are written directly
to all members, including write-mostly ones. A CPU copy via bio_copy_data()
defeats the point of a peer-to-peer transfer.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 997f79c05db7b..7d769b6460ca7 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1523,6 +1523,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  	bool write_behind = false;
>  	bool nowait = bio->bi_opf & REQ_NOWAIT;
>  	bool is_discard = op_is_discard(bio->bi_opf);
> +	bool is_p2pdma = md_bio_is_p2pdma(bio);
>  	sector_t sector = bio->bi_iter.bi_sector;
>  
>  	if (mddev_is_clustered(mddev) &&
[ ... ]
> @@ -1575,9 +1576,12 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  		/*
>  		 * The write-behind io is only attempted on drives marked as
>  		 * write-mostly, which means we could allocate write behind
> -		 * bio later.
> +		 * bio later. P2PDMA bios are excluded: write-behind copies
> +		 * the data with bio_copy_data(), a CPU copy that cannot be
> +		 * assumed safe or fast on P2PDMA (device BAR) pages.
>  		 */
> -		if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags))
> +		if (!is_discard && !is_p2pdma && rdev &&
> +		    test_bit(WriteMostly, &rdev->flags))
>  			write_behind = true;
>  
>  		r1_bio->bios[i] = NULL;

[Severity: High]
This is a pre-existing issue, but does raid1_write_request() properly support
REQ_NOWAIT semantics without blocking?

Earlier in raid1_write_request(), we allocate r1_bio unconditionally using
GFP_NOIO, which can sleep under memory pressure:

    r1_bio = alloc_r1bio(mddev, bio);

alloc_r1bio() {
    ...
    r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
    ...
}

Later down in raid1_write_request(), if CollisionCheck is set, we can
unconditionally put the thread to sleep, ignoring the REQ_NOWAIT context:

    if (test_bit(CollisionCheck, &rdev->flags))
        wait_for_serialization(rdev, r1_bio);

wait_for_serialization() {
    ...
    si = mempool_alloc(mddev->serial_info_pool, GFP_NOIO);
    ...
    wait_for_completion(&si->ready);
}

Could a REQ_NOWAIT write bio submitted to an md raid1 array block under
memory pressure or in-flight I/O overlapping, causing thread starvation in
io_uring workers?

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