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

> [High] TOCTOU on still_degraded: the degraded check was moved before the
> blocking stripe get + schedule_timeout, so a drive failing during the sleep
> leaves a stale still_degraded that can wrongly clear the NEEDED bit.

Agreed -- this is a real regression I introduced.
I hoisted the still_degraded computation so the window could reuse it,
but that placed it before the blocking get and the
schedule_timeout_uninterruptible(1),
whereas stock computes it after.
In v2 I'll move the still_degraded computation back below the blocking
get of the first stripe (restoring stock's ordering).
The remaining window stripes are fetched with R5_GAS_NOBLOCK and don't
sleep, so they can reuse that
value without reopening the race across a sleep.

  > [High] The batch loop ignores md_bitmap_start_sync()'s return and
force-syncs
  > clean stripes in the window.

Also correct.
Stock makes the skip decision once at the top
(!md_bitmap_start_sync(..., true) && sync_blocks >= ...),
but the window loop doesn't, so a clean bitmap region the window
crosses gets synced unnecessarily.
In v2 I'll have the window honor the bitmap and stop at the boundary of the
dirty region being resynced, rather than extending into already-in-sync stripes.

Both are correctness details the per-stripe path got right and my
batching dropped;
I'll fold the fixes into v2 and re-test the bitmap resync path.

Thanks,

2026年6月24日(水) 9:12 <[email protected]>:
>
> 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



-- 
Hiroshi Nishida
[email protected]