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

> [High] Breaking the read-ahead loop doesn't bound the rebuild to 50% -- the
> outer md_do_sync() loop just calls raid5_sync_request() again and the
> unconditional first-stripe allocation keeps filling the cache to 100%.

You're right, and it defeats the patch's purpose.
The 50% check only gates the window batch; the first stripe of each
raid5_sync_request() call is allocated
unconditionally before the loop, so md_do_sync() keeps driving one
stripe per call past 50%
until the cache is full and the existing blocking get throttles at ~100% anyway.
The "reserve half the cache for user I/O" claim doesn't hold.

Making it actually bound the rebuild would mean gating the
first-stripe allocation and
having raid5_sync_request() yield without making progress --
effectively reworking the sync throttle, which is more than this
change should take on,
and the user-latency benefit was never something I could measure
cleanly.
So I'll drop this patch from v2.  (It's coupled to the read-ahead
window patch, which I'll keep and fix per the other thread.)

Thanks,

2026年6月24日(水) 9:12 <[email protected]>:

>
> 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



--
Hiroshi Nishida
[email protected]