Re: [PATCH v2 3/3] md/raid10: skip clean regions in bulk during recovery

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Fallback to an unscaled `max_sync` mixes array and device sectors, overshooting the recovery cursor and silently skipping dirty sectors.
--

--- Patch [3]: [PATCH v2 3/3] md/raid10: skip clean regions in bulk during recovery ---
commit 4abce2392f76451810793b3efca0299da30d9898
Author: Yunye Zhao <yunye.zhao@linux.alibaba.com>

md/raid10: skip clean regions in bulk during recovery

During recovery of a degraded RAID10 array with a mostly-clean bitmap, this
patch calculates the skipped clean span in bulk. It tracks the minimum clean
span across skipped devices, converts it to device sectors, and skips it in
one step to reduce CPU spinning.

> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 54cddb3a98cd6..8acbd5ef618b4 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c

[ ... ]

> @@ -3514,6 +3530,8 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>  			}
>  		}
>  		if (biolist == NULL) {
> +			sector_t skip_sectors = max_sync;
> +
>  			while (r10_bio) {
>  				struct r10bio *rb2 = r10_bio;
>  				r10_bio = (struct r10bio*) rb2->master_bio;
> @@ -3521,7 +3539,29 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
>  				put_buf(rb2);
>  			}
>  			*skipped = 1;
> -			return max_sync;
> +
> +			/*
> +			 * min_sync_blocks is in array sectors, but the return
> +			 * value advances the per-device cursor; for a near
> +			 * layout the two spaces differ by the factor
> +			 * raid_disks / near_copies.
> +			 */
> +			if (!missing_source &&
> +			    conf->geo.far_copies == 1 && !conf->geo.far_offset &&
> +			    conf->geo.raid_disks % conf->geo.near_copies == 0 &&
> +			    min_sync_blocks != MaxSector) {
> +				sector_t clean_sectors = min_sync_blocks;
> +
> +				clean_sectors *= conf->geo.near_copies;
> +				sector_div(clean_sectors, conf->geo.raid_disks);
> +				clean_sectors &= ~chunk_mask;
> +				if (clean_sectors > sync_end - sector_nr)
> +					clean_sectors = sync_end - sector_nr;
> +				if (clean_sectors > skip_sectors)
> +					skip_sectors = clean_sectors;
> +			}
> +
> +			return skip_sectors;

[Severity: Critical]
This is a pre-existing issue, but does returning the unscaled max_sync mix
array and device sectors here?

When recovering a degraded RAID10 array, md_bitmap_start_sync() returns
sync_blocks in array sectors, which is then assigned to max_sync.

If a clean span is small or not chunk-aligned, the truncation:

    clean_sectors &= ~chunk_mask;

can force the scaled clean_sectors to 0. When that happens, the condition:

    if (clean_sectors > skip_sectors)
        skip_sectors = clean_sectors;

fails, leaving skip_sectors as the unscaled max_sync value.

Because raid10_sync_request() then returns skip_sectors (an array-sector
distance) to advance the physical per-device cursor in md_do_sync(), could
the physical cursor overstep the actual clean physical span?

For layouts where array sectors advance faster than device sectors
(e.g., near_copies > 1), won't this silently skip dirty sectors and leave
mirrors out of sync?

>  		}
>  	} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723135535.101995-1-yunye.zhao@linux.alibaba.com?part=3