Re: [PATCH 7/8] md/raid5: raise MAX_STRIPE_BATCH from 8 to 32

 > [High] Raising MAX_STRIPE_BATCH to 32 while keeping the spawn threshold at 8
 > lets the first worker grab all 32 stripes and starves the others.

You're right.  In stock, MAX_STRIPE_BATCH does double duty -- it caps
the greedy dequeue
in handle_active_stripes() AND is the spawn divisor in
raid5_wakeup_stripe_thread() --
and keeping those equal is what balances the workers woken against the
work each one takes.
My patch grew the dequeue cap to 32 but left the spawn divisor at 8,
so for ~32 pending stripes it wakes 4 workerswhile the first greedily
takes all 32 and
the rest find empty lists -- serializing the batch and adding
pointless lock contention, exactly as you describe.

The lock-amortization benefit was never isolated, and re-coupling the two (spawn
divisor = batch) just trades it for coarser worker granularity, so
I'll drop this
patch from v2 rather than guess at the tuning.

Thanks,

2026年6月24日(水) 9:09 <[email protected]>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Increasing MAX_STRIPE_BATCH to 32 without proportionally increasing STRIPE_BATCH_WORKERS serializes stripe processing and causes worker starvation.
> --
>
> commit b1a0a169dabd8b46b6b9ca40375034cf879934c6
> Author: Hiroshi Nishida <[email protected]>
>
> md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
>
> This commit increases the maximum stripe batch size from 8 to 32 to
> amortize device_lock acquisition over more stripes. It also introduces
> STRIPE_BATCH_WORKERS to decouple the worker spawning threshold from the
> batch size.
>
> > diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> > index c8787ab7b309c..ad6230415af33 100644
> > --- a/drivers/md/raid5.c
> > +++ b/drivers/md/raid5.c
> [ ... ]
> > @@ -210,7 +210,7 @@ static void raid5_wakeup_stripe_thread(struct stripe_head *sh)
> >       /* at least one worker should run to avoid race */
> >       queue_work_on(sh->cpu, raid5_wq, &group->workers[0].work);
> >
> > -     thread_cnt = group->stripes_cnt / MAX_STRIPE_BATCH - 1;
> > +     thread_cnt = group->stripes_cnt / STRIPE_BATCH_WORKERS - 1;
> >       /* wakeup more workers */
> >       for (i = 1; i < conf->worker_cnt_per_group && thread_cnt > 0; i++) {
>
> [Severity: High]
> Will increasing the stripe batch size while keeping the worker spawn threshold
> small serialize stripe processing and starve the worker threads?
>
> With this change, raid5_wakeup_stripe_thread() still wakes up an additional
> worker for every 8 pending stripes (e.g., waking 4 threads for 32 stripes).
>
> However, the first awakened worker executing handle_active_stripes() will
> greedily dequeue up to MAX_STRIPE_BATCH (32) stripes under the lock:
>
> drivers/md/raid5.c:handle_active_stripes() {
>     ...
>     while (batch_size < MAX_STRIPE_BATCH &&
>            (sh = __get_priority_stripe(conf, group)) != NULL)
>         batch[batch_size++] = sh;
>     ...
> }
>
> It appears the first thread will steal all 32 pending stripes and process
> them sequentially, while the other awakened threads acquire the lock,
> find the lists empty, and exit without doing any work.
>
> Could this cause a performance regression by serializing CPU-intensive parity
> operations onto a single thread, while also creating unnecessary lock
> contention from the starved threads?
>
> > diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> > index 7aeba1fc7f09e..1f37dabd727be 100644
> > --- a/drivers/md/raid5.h
> > +++ b/drivers/md/raid5.h
> [ ... ]
> > @@ -496,7 +496,8 @@ struct disk_info {
> >  #define BYPASS_THRESHOLD     1
> >  #define NR_HASH                      (PAGE_SIZE / sizeof(struct hlist_head))
> >  #define HASH_MASK            (NR_HASH - 1)
> > -#define MAX_STRIPE_BATCH     8
> > +#define MAX_STRIPE_BATCH     32      /* stripes per handle_active_stripes pass */
> > +#define STRIPE_BATCH_WORKERS 8       /* stripes-per-worker threshold for spawning */
> >  #define RAID5_SYNC_WINDOW    32      /* stripes to pre-submit per sync_request call */
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7



-- 
Hiroshi Nishida
[email protected]