Re: [PATCH v2 3/7] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

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

New issues:
- [High] Hot-added disks silently bypass array-wide serialization when MD_SERIALIZE_POLICY is enabled, defeating write ordering guarantees.

Pre-existing issues:
- [High] REQ_NOWAIT bios can block during clone allocation and serialization, violating non-blocking semantics.
- [High] The serialization logic fails to properly order a write against all overlapping in-flight writes when there are multiple independent collisions.
--

--- Patch [3]: [PATCH v2 3/7] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs ---
commit 26970b9339baa20abd27398542d6d87116b76717
Author: Mykola Marzhan <[email protected]>
md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

This commit changes the gate for non-write-behind serialization in raid1
from the array-wide MD_SERIALIZE_POLICY to the per-device CollisionCheck
flag. This ensures write-behind writes and non-write-behind writes are
correctly serialized against each other on members that require it.
Discards are also now ordered on this path.

> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -1677,7 +1677,14 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  			mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO,
>  					       &mddev->bio_set);

[Severity: High]
This is a pre-existing issue, but does this code block REQ_NOWAIT bios?

Calling bio_alloc_clone() with GFP_NOIO can block in memory reclaim, and
wait_for_serialization() unconditionally sleeps in wait_for_completion().

If the original bio has the REQ_NOWAIT flag set, won't this violate the
non-blocking semantics and potentially stall async submission threads like
io_uring?

> -			if (test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> +			/*
> +			 * Order against in-flight write-behind I/O: a
> +			 * behind write is acked early, and an unordered
> +			 * overwrite could land first, leaving its stale
> +			 * data on the member last. CollisionCheck marks
> +			 * every rdev that owns a serial tree.
> +			 */
> +			if (test_bit(CollisionCheck, &rdev->flags))
>  				wait_for_serialization(rdev, r1_bio);

[Severity: High]
Does this change unintentionally disable serialization for hot-added disks?

If a user enables serialize_policy to enforce strict write ordering, and then
hot-adds a new disk that isn't WriteMostly, mddev_create_serial_pool() in
drivers/md/md.c will exit early:

drivers/md/md.c:mddev_create_serial_pool() {
    ...
    if (rdev && !rdev_need_serial(rdev) &&
        !test_bit(CollisionCheck, &rdev->flags))
        return;
    ...
}

Because this setup exits without allocating a serial tree or setting the
CollisionCheck flag, won't the new per-device check here in
raid1_write_request() silently bypass wait_for_serialization() for the new
disk, even though the array-wide MD_SERIALIZE_POLICY is active?

[Severity: High]
This is a pre-existing issue, but does wait_for_serialization() handle
multiple overlapping writes correctly?

If we have two in-flight writes, A (sectors 0-100) and Y (sectors 120-200),
and a new write B (sectors 50-150) is submitted, it overlaps both. When B
checks for collisions, it finds A and waits.

When A completes, remove_serial() inserts B into the interval tree and wakes
it up:

drivers/md/raid1.c:remove_serial() {
    ...
    raid1_rb_insert(iter_si, &serial->serial_rb);
    complete(&iter_si->ready);
    ...
}

When B re-checks for collisions in check_and_add_serial(), the tree now
contains B and Y. raid1_rb_iter_first() will return B since its start sector
is lower than Y's:

drivers/md/raid1.c:check_and_add_serial() {
    ...
    head_si = raid1_rb_iter_first(&serial->serial_rb, lo, hi);
    if (head_si && head_si != si) {
    ...
}

Since head_si == si (B == B), the collision handler is skipped. Does this
mean B will execute concurrently with Y, bypassing serialization for the
120-150 region?

>  		}

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