Re: [PATCH 8/8] 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 patch fails to bound the rebuild's share of the stripe cache to 50%. Breaking the read-ahead batch loop just causes the outer md_do_sync() loop to allocate stripes one-by-one, relentlessly filling the cache to 100% anyway.
--

commit 29ed9bbd7953c5d6ceeb0158cc53c81fd4d3aaf9
Author: Hiroshi Nishida <[email protected]>

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

This commit attempts to prevent rebuild operations from monopolizing the
stripe cache and starving concurrent user I/O. It adds yield points in the
resync read-ahead loop to stop submitting stripes if there are active
waiters or if the cache is more than half full.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> --- 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)) {

[ ... ]

> +		/*
> +		 * 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 breaking out of the read-ahead loop here actually limit the cache
usage to 50%?

When this breaks, raid5_sync_request() returns control to md_do_sync(),
which operates in a loop and will immediately call raid5_sync_request() 
again for the next sector.

Because raid5_sync_request() unconditionally allocates its first stripe
earlier in the function before entering this batch loop, won't the outer
loop just continue allocating stripes one-by-one until the cache reaches
100% capacity and triggers the fallback blocking path anyway?

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