Re: [PATCH 2/2] md/raid5: reserve stripe cache for user I/O during rebuild

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added stripe cache reservation logic is completely ineffective because it fails to throttle the calling loop in md_do_sync().
--

--- Patch [2]: [PATCH 2/2] md/raid5: reserve stripe cache for user I/O during rebuild ---
commit fb8a80692f7a386d69a8bee1d9b364db1d33380f
Author: Hiroshi Nishida <[email protected]>

md/raid5: reserve stripe cache for user I/O during rebuild

This commit attempts to prevent burst-starvation of user I/O during a RAID5
rebuild by reserving a portion of the stripe cache. It adds checks in the
read-ahead loop of raid5_sync_request() to yield when there is competing
user I/O.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 574880e4f23fe..6575bdb84bb23 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6656,6 +6656,27 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
>  	     submitted < RAID5_SYNC_WINDOW && win_sector < max_sector &&
>  	     win_sector < mddev->resync_max;
>  	     submitted++, win_sector += RAID5_STRIPE_SECTORS(conf)) {
> +		/*
> +		 * Yield to user I/O: stop the read-ahead if anyone is waiting
> +		 * for a stripe.  The check is intentionally racy -- a waiter
> +		 * appearing just after is serviced by the next sync_request
> +		 * call, so no barrier is needed.
> +		 */
> +		if (waitqueue_active(&conf->wait_for_stripe))
> +			break;
> +		/*
> +		 * Reserve cache for user I/O only when it is actually competing.
> +		 * preread_active_stripes counts stripes queued for write I/O
> +		 * (including the read phase of RMW); sync stripes never set
> +		 * STRIPE_PREREAD_ACTIVE, so during a pure rebuild it stays zero
> +		 * and the window fills freely.  Competing user reads do not bump
> +		 * the counter but are caught by the waitqueue_active() check
> +		 * above.
> +		 */
> +		if (atomic_read(&conf->preread_active_stripes) > 0 &&
> +		    atomic_read(&conf->active_stripes) >=
> +		    conf->max_nr_stripes / RAID5_SYNC_HWMARK)
> +			break;

[Severity: High]
Does this actually throttle the rebuild as intended?

When we break out of this loop here, raid5_sync_request() returns the number
of sectors processed so far back to md_do_sync(). Since md_do_sync()
continuously loops calling the sync_request handler until the rate-limit window
is met, it will immediately call raid5_sync_request() again.

When raid5_sync_request() is re-entered, the very first stripe is allocated
unconditionally without checking the high-water mark:

drivers/md/raid5.c:raid5_sync_request() {
    ...
	sh = raid5_get_active_stripe(conf, NULL, sector_nr, R5_GAS_NOBLOCK);
    ...
}

Will the sync thread just rapidly consume stripes one-by-one, completely
bypassing the 50% cache reservation and filling the cache 100% anyway?

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