Re: [PATCH 5/8] md/raid5: submit a window of stripes during resync/recovery

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The batch loop ignores the return value of md_bitmap_start_sync(), forcing clean stripes to be unnecessarily synchronized.
- [High] Time-of-check to time-of-use race on the array's degraded state, leading to silent data corruption via incorrect bitmap updates.
--

commit 01d1e80ee6aa253c48c233e7fc218fbdac9522fa
Author: Hiroshi Nishida <[email protected]>

md/raid5: submit a window of stripes during resync/recovery

This commit alters raid5_sync_request() to dispatch up to 32 stripes per
call instead of one. It attempts to submit the remaining stripes in the
window with a non-blocking flag to avoid the per-stripe throttle sleep.

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 9cb4ed3bd85c5..8e9edaaca667b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6620,16 +6621,7 @@ static inline sector_t raid5_sync_request(...)
[ ... ]
> -	sh = raid5_get_active_stripe(conf, NULL, sector_nr,
> -				     R5_GAS_NOBLOCK);
> -	if (sh == NULL) {
> -		sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> -		/* make sure we don't swamp the stripe cache if someone else
> -		 * is trying to get access
> -		 */
> -		schedule_timeout_uninterruptible(1);
> -	}
> -	/* Need to check if array will still be degraded after recovery/resync
> +	/* Check once whether array will still be degraded after recovery/resync.
>  	 * Note in case of > 1 drive failures it's possible we're rebuilding
>  	 * one drive while leaving another faulty drive in array.
>  	 */
> @@ -6640,13 +6632,42 @@ static inline sector_t raid5_sync_request(...)
>  			still_degraded = true;
>  	}
>  
> +	/* First stripe: block if stripe cache is full, then throttle. */
> +	sh = raid5_get_active_stripe(conf, NULL, sector_nr, R5_GAS_NOBLOCK);
> +	if (sh == NULL) {
> +		sh = raid5_get_active_stripe(conf, NULL, sector_nr, 0);
> +		/* make sure we don't swamp the stripe cache if someone else
> +		 * is trying to get access
> +		 */
> +		schedule_timeout_uninterruptible(1);
> +	}

[Severity: High]
Does moving the degraded check before the stripe allocation introduce a
time-of-check to time-of-use race?

If a drive fails during the potential sleep in raid5_get_active_stripe() or
schedule_timeout_uninterruptible(), the stale still_degraded value is used
below to update the bitmap. This could incorrectly clear the NEEDED bit,
potentially leaving parity inconsistent if the system crashes before the sync
completes.

>  	md_bitmap_start_sync(mddev, sector_nr, &sync_blocks, still_degraded);
>  	set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
>  	set_bit(STRIPE_HANDLE, &sh->state);

[ ... ]

> +	win_sector = sector_nr + RAID5_STRIPE_SECTORS(conf);
> +	for (submitted = 1;
> +	     submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
> +	     win_sector < mddev->resync_max;
> +	     submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
> +		sh = raid5_get_active_stripe(conf, NULL, win_sector,
> +					     R5_GAS_NOBLOCK);
> +		if (!sh)
> +			break;
> +		md_bitmap_start_sync(mddev, win_sector, &sync_blocks,
> +				     still_degraded);
> +		set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> +		set_bit(STRIPE_HANDLE, &sh->state);

[Severity: High]
Does this batch loop unnecessarily synchronize clean stripes?

The boolean return value of md_bitmap_start_sync() is ignored here. If the
first stripe needs sync but subsequent stripes in the window are clean, this
loop unconditionally queues every stripe for synchronization regardless of
its clean/dirty state, forcing read-rebuild-write cycles on clean stripes.

> +		raid5_release_stripe(sh);
> +	}
> +
> +	return submitted * RAID5_STRIPE_SECTORS(conf);
>  }

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