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

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