Re: [PATCH v2 15/20] md/raid10: wire llbitmap reshape lifecycle

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] raid10_start_reshape skips initializing llbitmap's reshape geometry, causing bitmap corruption during and after reshape.
- [Low] Hardcoded check for `ID_LLBITMAP` in `raid10_finish_reshape` breaks the polymorphism of `mddev->bitmap_ops`.
--

commit 84732a3d166ed5b144ce82fb596703bb8b35f73c
Author: Yu Kuai <[email protected]>

md/raid10: wire llbitmap reshape lifecycle

Prepare llbitmap before RAID10 starts growing, checkpoint the bitmap
before advancing reshape_position, finish the llbitmap geometry update
when reshape completes, and export the old and new tracked sizes.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 1242b8d3bb6bd..e50e4adee389f 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -4394,6 +4394,12 @@ static int raid10_start_reshape(struct mddev *mddev)
>  
>  	if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
>  		return -EBUSY;
> +	if (md_bitmap_enabled(mddev, false) &&
> +	    mddev->bitmap_ops->reshape_can_start) {
> +		ret = mddev->bitmap_ops->reshape_can_start(mddev);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Does this fully wire up the llbitmap reshape lifecycle?

Further down in raid10_start_reshape(), the call to mddev->bitmap_ops->resize()
is gated by `if (mddev->delta_disks && mddev->bitmap)`.

If a reshape is started that only grows the chunk size (where
mddev->delta_disks == 0), won't this skip initializing the llbitmap's reshape
geometry?

If resize() is bypassed, llbitmap->reshape_chunksize would remain at the old
chunk size, causing llbitmap_encode_range() for the new layout to map IOs
using the old chunk size. Could this set bits for incorrect regions, leading
to dirty data not being tracked properly and silent data corruption upon
array recovery?

[ ... ]

> @@ -5039,10 +5062,15 @@ static void end_reshape_request(struct r10bio *r10_bio)
>  static void raid10_finish_reshape(struct mddev *mddev)
>  {
>  	struct r10conf *conf = mddev->private;
> +	bool llbitmap = mddev->bitmap_id == ID_LLBITMAP &&
> +		md_bitmap_enabled(mddev, false);
>  
>  	if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
>  		return;
>  
> +	if (llbitmap && mddev->bitmap_ops->reshape_finish)
> +		mddev->bitmap_ops->reshape_finish(mddev);

[Severity: Low]
Is this hardcoded check for ID_LLBITMAP necessary?

By explicitly checking mddev->bitmap_id, this seems to defeat the polymorphism
of mddev->bitmap_ops. If another bitmap implementation introduces
reshape_finish(), wouldn't it be silently ignored?

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