[PATCH 0/2] md/raid5: reduce resync/recovery dispatch overhead

[PATCH 0/2] md/raid5: reduce resync/recovery dispatch overhead

[Hiroshi Nishida]

Two changes to how resync/recovery submits work, reducing per-stripe
overhead on the rebuild path.  Neither adds memory.

  1/2 submits a window of stripes per raid5_sync_request() call instead
      of one at a time.  On a rebuild this reduces the number of
      raid5_sync_request() invocations by ~30x (a call-count measurement,
      not a throughput figure) and bounds the per-stripe jiffy-sleep that
      otherwise throttles progress when the stripe cache is under
      pressure.

  2/2 reserves a small amount of stripe cache for user I/O during a
      rebuild, so foreground I/O is not starved by the resync stream.  A
      pure rebuild with no competing I/O is unchanged.

Both touch only the resync/recovery path and are independent of the other
md patches I'm sending separately.

Hiroshi Nishida (2):
  md/raid5: submit a window of stripes during resync/recovery
  md/raid5: reserve stripe cache for user I/O during rebuild

 drivers/md/raid5.c | 68 +++++++++++++++++++++++++++++++++++++---------
 drivers/md/raid5.h |  2 ++
 2 files changed, 57 insertions(+), 13 deletions(-)


base-commit: 55b77337bdd088c77461588e5ec094421b89911b
-- 
2.43.0

[PATCH 1/2] md/raid5: submit a window of stripes during resync/recovery

[Hiroshi Nishida]

raid5_sync_request() dispatches one stripe per call: it fetches a single
stripe head, marks it for sync, and returns one stripe's worth of
sectors.  When the stripe cache is full the NOBLOCK fetch fails and it
re-enters a one-jiffy throttle sleep (schedule_timeout_uninterruptible(1))
before retrying.  Because that sleep is taken per stripe, sustained cache
pressure bounds sync progress to roughly HZ stripes/second regardless of
how fast the member devices are.

Dispatch up to RAID5_SYNC_WINDOW (32) stripes per call instead.  Only the
first stripe of the window keeps the original behaviour (block, then the
one-jiffy throttle if the cache was full); the remaining stripes are
requested with R5_GAS_NOBLOCK and the loop stops as soon as the cache is
full.  So at most one throttle sleep is taken per window rather than per
stripe, and when the cache has free slots a single call can queue a batch
instead of one stripe at a time.

With a warm cache the window stays near full: counting raid5_sync_request()
invocations across a rebuild showed it averaging ~30 of the 32 stripes per
call, i.e. roughly 30x fewer calls into the sync path for the same resync.

The return value reports the number of stripes actually submitted, so
md_do_sync()'s recovery_active accounting stays balanced, and the window
is bounded by both the end of the sync region (max_sector) and
mddev->resync_max, so a user- or cluster-imposed sync ceiling is not
overshot.

This does not change which data is read or written during resync or
recovery.

Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
 drivers/md/raid5.c | 47 +++++++++++++++++++++++++++++++++-------------
 drivers/md/raid5.h |  1 +
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0c5c9fb0606e..574880e4f23f 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6563,7 +6563,8 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 	struct stripe_head *sh;
 	sector_t sync_blocks;
 	bool still_degraded = false;
-	int i;
+	int i, submitted;
+	sector_t win_sector;
 
 	if (sector_nr >= max_sector) {
 		/* just being told to finish up .. nothing much to do */
@@ -6620,16 +6621,7 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
 	if (md_bitmap_enabled(mddev, false))
 		mddev->bitmap_ops->cond_end_sync(mddev, sector_nr, false);
 
-	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(struct mddev *mddev, sector_t sector_n
 			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);
+	}
 	md_bitmap_start_sync(mddev, sector_nr, &sync_blocks, still_degraded);
 	set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
 	set_bit(STRIPE_HANDLE, &sh->state);
-
 	raid5_release_stripe(sh);
 
-	return RAID5_STRIPE_SECTORS(conf);
+	/* Submit remaining stripes in the window non-blocking.  Stop early
+	 * if the stripe cache is full: the disk queue is already saturated.
+	 * Bound by resync_max so a user- or cluster-imposed sync ceiling is
+	 * not overshot.
+	 */
+	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);
+		raid5_release_stripe(sh);
+	}
+
+	return submitted * RAID5_STRIPE_SECTORS(conf);
 }
 
 static int  retry_aligned_read(struct r5conf *conf, struct bio *raid_bio,
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index cb5feae04db2..63b630118782 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -491,6 +491,7 @@ struct disk_info {
 #define NR_HASH			(PAGE_SIZE / sizeof(struct hlist_head))
 #define HASH_MASK		(NR_HASH - 1)
 #define MAX_STRIPE_BATCH	8
+#define RAID5_SYNC_WINDOW	32	/* stripes to pre-submit per sync_request call */
 
 /* NOTE NR_STRIPE_HASH_LOCKS must remain below 64.
  * This is because we sometimes take all the spinlocks
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The batched sync loop can greedily drain the stripe cache without yielding, causing severe latency spikes and potentially starving user I/O.
--

--- Patch [1]: [PATCH 1/2] md/raid5: submit a window of stripes during resync/recovery ---
commit 2b3cdb45d82bdea6112cba5dad14b59ec3acb23a
Author: Hiroshi Nishida <nishidafmly@gmail.com>

md/raid5: submit a window of stripes during resync/recovery

This patch updates the RAID5 sync path to dispatch a batch of up to 32
stripes per call instead of just one. It throttles only on the first stripe,
allowing subsequent stripes to be submitted without blocking to improve
resync performance.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..574880e4f23fe 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -6640,13 +6632,42 @@ static inline sector_t raid5_sync_request(struct mddev *mddev, sector_t sector_n
[ ... ]
> +	/* Submit remaining stripes in the window non-blocking.  Stop early
> +	 * if the stripe cache is full: the disk queue is already saturated.
> +	 * Bound by resync_max so a user- or cluster-imposed sync ceiling is
> +	 * not overshot.
> +	 */
> +	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;

[Severity: High]
Can this batched sync loop greedily drain the stripe cache without yielding to
user I/O?

During array resync or recovery, this loop acquires up to 32 free stripes
sequentially. Could this starve user I/O threads that are blocked on the
conf->wait_for_stripe waitqueue, since it doesn't check if user I/O is waiting
(e.g., via waitqueue_active(&conf->wait_for_stripe)) before getting the next
stripe?

> +		md_bitmap_start_sync(mddev, win_sector, &sync_blocks,
> +				     still_degraded);
> +		set_bit(STRIPE_SYNC_REQUESTED, &sh->state);
> +		set_bit(STRIPE_HANDLE, &sh->state);
> +		raid5_release_stripe(sh);
> +	}
> +
> +	return submitted * RAID5_STRIPE_SECTORS(conf);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710132339.7284-1-nishidafmly@gmail.com?part=1

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

[Hiroshi Nishida]

The resync read-ahead window (RAID5_SYNC_WINDOW) can fill the stripe
cache with rebuild stripes and starve concurrent user I/O, producing a
burst-starvation flip-flop between rebuild and application throughput.

Add two yield points to the window-submission loop:
 - stop the window immediately if any thread is waiting for a stripe
   (waitqueue_active(&conf->wait_for_stripe)); the check is intentionally
   racy -- a waiter appearing just after is serviced by the next
   sync_request call, so no barrier is needed.
 - stop expanding once active_stripes reaches half the cache
   (max_nr_stripes / RAID5_SYNC_HWMARK), but only when
   preread_active_stripes > 0, i.e. user write I/O is actually competing.
   Sync stripes never set STRIPE_PREREAD_ACTIVE, so during a pure rebuild
   the counter stays zero and the window fills freely; rebuild-only
   throughput is unchanged.

This bounds the share of the stripe cache a rebuild may hold while user
I/O is present, so application latency no longer collapses during the
read-ahead bursts, without throttling a rebuild that has the array to
itself.

Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
 drivers/md/raid5.c | 21 +++++++++++++++++++++
 drivers/md/raid5.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 574880e4f23f..6575bdb84bb2 100644
--- 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)) {
+		/*
+		 * Yield to user I/O: stop the read-ahead if anyone is waiting
+		 * for a stripe.  The check is intentionally racy -- a waiter
+		 * appearing just after is serviced by the next sync_request
+		 * call, so no barrier is needed.
+		 */
+		if (waitqueue_active(&conf->wait_for_stripe))
+			break;
+		/*
+		 * 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;
 		sh = raid5_get_active_stripe(conf, NULL, win_sector,
 					     R5_GAS_NOBLOCK);
 		if (!sh)
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 63b630118782..263330af4e7d 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -492,6 +492,7 @@ struct disk_info {
 #define HASH_MASK		(NR_HASH - 1)
 #define MAX_STRIPE_BATCH	8
 #define RAID5_SYNC_WINDOW	32	/* stripes to pre-submit per sync_request call */
+#define RAID5_SYNC_HWMARK	2	/* rebuild uses at most 1/N of stripe cache */
 
 /* NOTE NR_STRIPE_HASH_LOCKS must remain below 64.
  * This is because we sometimes take all the spinlocks
-- 
2.43.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly added stripe cache reservation logic is completely ineffective because it fails to throttle the calling loop in md_do_sync().
--

--- Patch [2]: [PATCH 2/2] md/raid5: reserve stripe cache for user I/O during rebuild ---
commit fb8a80692f7a386d69a8bee1d9b364db1d33380f
Author: Hiroshi Nishida <nishidafmly@gmail.com>

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

This commit attempts to prevent burst-starvation of user I/O during a RAID5
rebuild by reserving a portion of the stripe cache. It adds checks in the
read-ahead loop of raid5_sync_request() to yield when there is competing
user I/O.

> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 574880e4f23fe..6575bdb84bb23 100644
> --- 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)) {
> +		/*
> +		 * Yield to user I/O: stop the read-ahead if anyone is waiting
> +		 * for a stripe.  The check is intentionally racy -- a waiter
> +		 * appearing just after is serviced by the next sync_request
> +		 * call, so no barrier is needed.
> +		 */
> +		if (waitqueue_active(&conf->wait_for_stripe))
> +			break;
> +		/*
> +		 * 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 this actually throttle the rebuild as intended?

When we break out of this loop here, raid5_sync_request() returns the number
of sectors processed so far back to md_do_sync(). Since md_do_sync()
continuously loops calling the sync_request handler until the rate-limit window
is met, it will immediately call raid5_sync_request() again.

When raid5_sync_request() is re-entered, the very first stripe is allocated
unconditionally without checking the high-water mark:

drivers/md/raid5.c:raid5_sync_request() {
    ...
	sh = raid5_get_active_stripe(conf, NULL, sector_nr, R5_GAS_NOBLOCK);
    ...
}

Will the sync thread just rapidly consume stripes one-by-one, completely
bypassing the 50% cache reservation and filling the cache 100% anyway?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710132339.7284-1-nishidafmly@gmail.com?part=2