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

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

Pre-existing issues:
- [High] Using CollisionCheck to gate serialization for normal writes exposes a pre-existing serial_info_pool allocation failure, causing continuous WARN_ON log spam on the hot path.
- [High] Hot-added devices silently bypass write serialization under MD_SERIALIZE_POLICY, risking data corruption.
--

--- Patch [3]: [PATCH v3 3/8] md/raid1: serialize non-write-behind writes on CollisionCheck rdevs ---
commit d02939f61be1abbb8081faa2ba5a3540f9714678
Author: Mykola Marzhan <[email protected]>

md/raid1: serialize non-write-behind writes on CollisionCheck rdevs

This commit modifies the write path to serialize non-write-behind writes
based on the CollisionCheck flag rather than MD_SERIALIZE_POLICY. This
ensures they are properly ordered against in-flight behind writes. It also
updates the completion path to use the same condition for removing
serialization.

> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index afe2ca96ad8c2..997f79c05db7b 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -564,7 +564,7 @@ static void raid1_end_write_request(struct bio *bio)
>  				call_bio_endio(r1_bio);
>  			}
>  		}
> -	} else if (test_bit(MD_SERIALIZE_POLICY, &rdev->mddev->flags))
> +	} else if (test_bit(CollisionCheck, &rdev->flags))
>  		remove_serial(rdev, lo, hi);

[Severity: High]
This isn't a bug introduced by this patch, but changing the gate to
CollisionCheck exposes the normal write path to a pre-existing error handling
bug that results in continuous WARN_ON spam on the hot path.

If mempool allocation fails during a hot-add of a WriteMostly device,
mddev_create_serial_pool() calls rdevs_uninit_serial(). However,
rdevs_uninit_serial() only clears CollisionCheck on devices already in
mddev->disks, leaving the new device with CollisionCheck set but a NULL
serial_info_pool.

Before this patch, normal writes checked MD_SERIALIZE_POLICY and ignored
CollisionCheck, avoiding the bug. Now, does this cause every normal write to
call wait_for_serialization() (triggering the first WARN_ON) and then
remove_serial() (triggering a second WARN(1) since the IO was never recorded)?

>  	if (r1_bio->bios[mirror] == NULL)
>  		rdev_dec_pending(rdev, conf->mddev);

[ ... ]

> @@ -1677,7 +1677,11 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
>  			mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO,
>  					       &mddev->bio_set);
>  
> -			if (test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> +			/*
> +			 * CollisionCheck marks every rdev with a serial
> +			 * tree; order against in-flight write-behind I/O.
> +			 */
> +			if (test_bit(CollisionCheck, &rdev->flags))
>  				wait_for_serialization(rdev, r1_bio);

[Severity: High]
This is a pre-existing issue, but does checking CollisionCheck instead of
MD_SERIALIZE_POLICY silently bypass write serialization for hot-added devices
under MD_SERIALIZE_POLICY?

If MD_SERIALIZE_POLICY is enabled and a normal device is hot-added,
mddev_create_serial_pool() currently lacks a check for MD_SERIALIZE_POLICY
and returns early without creating a serial tree or setting CollisionCheck:

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

Previously, checking MD_SERIALIZE_POLICY here caused a NULL pointer
dereference when trying to serialize the hot-added device without a serial
tree. By checking CollisionCheck instead, the crash is avoided, but does
this mean overlapping writes can now complete out of order on the
reconstructed mirror, leading to silent data corruption?

>  		}
>

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