* Re: [GIT PULL] md-7.2-20260623
From: Jens Axboe @ 2026-06-24 12:33 UTC (permalink / raw)
To: Yu Kuai; +Cc: linux-raid, linux-block, Abd-Alrhman Masalkhi, Chen Cheng
In-Reply-To: <20260623090327.1097428-1-yukuai@kernel.org>
On 6/23/26 3:03 AM, Yu Kuai wrote:
> Hi Jens,
>
> Please consider pulling the following changes into your block-7.2
> branch.
>
> This pull request contains:
>
> Bug Fixes:
> - Fix raid1 writes_pending and barrier reference leaks on write failures.
> (Abd-Alrhman Masalkhi)
> - Fix raid10 writes_pending leak on write request failures.
> (Abd-Alrhman Masalkhi)
> - Fix raid10 writes_pending and barrier reference leaks on discard failures.
> (Abd-Alrhman Masalkhi)
> - Fix raid1 REQ_NOWAIT handling while waiting for behind writes.
> (Abd-Alrhman Masalkhi)
> - Fix raid1 r1_bio leak when a REQ_NOWAIT retry would block.
> (Abd-Alrhman Masalkhi)
> - Fix raid1 read-balance head_position data race. (Chen Cheng)
> - Fix raid5 stripe batch bm_seq wraparound comparison. (Chen Cheng)
> - Fix raid5 stripe batch state snapshot KCSAN noise. (Chen Cheng)
> - Fix raid5 R5_Overlap races while breaking stripe batches. (Chen Cheng)
>
> Improvements:
> - Add raid5 discard IO accounting. (Yu Kuai)
> - Always convert raid5 llbitmap bits for discard. (Yu Kuai)
>
> Cleanups:
> - Simplify raid1_write_request() error handling. (Abd-Alrhman Masalkhi)
Pulled, thanks.
--
Jens Axboe
^ permalink raw reply
* [PATCH 0/5] md: minor cleanups in md core and raid5/raid1/raid10
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
A handful of small, no-functional-change cleanups noticed while reading
the md code:
- drop an unused parameter from check_decay_read_errors();
- use max() in raid5_calc_degraded();
- make is_mddev_idle() take a bool init flag, matching the helper it
forwards to;
- declare a sector count as sector_t rather than int in status_resync();
- rewrite a self-contradictory comment above the resync ETA math.
No behavioral change in any patch. Built and functionally sanity-tested
(RAID5/RAID6 create, rebuild, scrub) on x86_64, including a run with KASAN
and lockdep enabled -- with concurrent /proc/mdstat polling to exercise
status_resync() -- and no reports.
Hiroshi Nishida (5):
md/raid1,raid10: drop unused mddev arg from check_decay_read_errors()
md/raid5: use max() in raid5_calc_degraded()
md: make is_mddev_idle() take a bool init flag
md: use sector_t for recovery_active in status_resync()
md: clarify the resync ETA comment in status_resync()
drivers/md/md.c | 26 ++++++++++----------------
drivers/md/raid1-10.c | 4 ++--
drivers/md/raid5.c | 4 +---
3 files changed, 13 insertions(+), 21 deletions(-)
base-commit: 55b77337bdd088c77461588e5ec094421b89911b
--
2.43.0
^ permalink raw reply
* [PATCH 1/5] md/raid1,raid10: drop unused mddev arg from check_decay_read_errors()
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155421.211626-1-nishidafmly@gmail.com>
check_decay_read_errors() only ever touches the passed rdev; the mddev
parameter has been unused. Remove it and update the sole caller in
exceed_read_errors().
No functional change.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid1-10.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid1-10.c b/drivers/md/raid1-10.c
index 56a56a4da4f8..edba52eb3116 100644
--- a/drivers/md/raid1-10.c
+++ b/drivers/md/raid1-10.c
@@ -171,7 +171,7 @@ static inline void raid1_prepare_flush_writes(struct mddev *mddev)
* We halve the read error count for every hour that has elapsed
* since the last recorded read error.
*/
-static inline void check_decay_read_errors(struct mddev *mddev, struct md_rdev *rdev)
+static inline void check_decay_read_errors(struct md_rdev *rdev)
{
long cur_time_mon;
unsigned long hours_since_last;
@@ -206,7 +206,7 @@ static inline bool exceed_read_errors(struct mddev *mddev, struct md_rdev *rdev)
int max_read_errors = atomic_read(&mddev->max_corr_read_errors);
int read_errors;
- check_decay_read_errors(mddev, rdev);
+ check_decay_read_errors(rdev);
read_errors = atomic_inc_return(&rdev->read_errors);
if (read_errors > max_read_errors) {
pr_notice("md/"RAID_1_10_NAME":%s: %pg: Raid device exceeded read_error threshold [cur %d:max %d]\n",
--
2.43.0
^ permalink raw reply related
* [PATCH 2/5] md/raid5: use max() in raid5_calc_degraded()
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155421.211626-1-nishidafmly@gmail.com>
The tail of raid5_calc_degraded() open-codes a max of the two computed
degraded counts. Use max() for clarity.
No functional change.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid5.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0c5c9fb0606e..2b2aa4f2431c 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -741,9 +741,7 @@ int raid5_calc_degraded(struct r5conf *conf)
if (conf->raid_disks <= conf->previous_raid_disks)
degraded2++;
}
- if (degraded2 > degraded)
- return degraded2;
- return degraded;
+ return max(degraded, degraded2);
}
static bool has_failed(struct r5conf *conf)
--
2.43.0
^ permalink raw reply related
* [PATCH 3/5] md: make is_mddev_idle() take a bool init flag
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155421.211626-1-nishidafmly@gmail.com>
is_mddev_idle() took an int init flag but passes it straight through to
is_rdev_holder_idle(), which already takes a bool. Make the types
consistent and update the two callers to pass true/false.
No functional change.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/md.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..829b4a3f545b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9219,7 +9219,7 @@ static bool is_rdev_holder_idle(struct md_rdev *rdev, bool init)
*
* Noted this checking rely on IO accounting is enabled.
*/
-static bool is_mddev_idle(struct mddev *mddev, int init)
+static bool is_mddev_idle(struct mddev *mddev, bool init)
{
unsigned long last_events = mddev->normal_io_events;
struct gendisk *disk;
@@ -9771,7 +9771,7 @@ void md_do_sync(struct md_thread *thread)
pr_debug("md: using maximum available idle IO bandwidth (but not more than %d KB/sec) for %s.\n",
speed_max(mddev), desc);
- is_mddev_idle(mddev, 1); /* this initializes IO event counters */
+ is_mddev_idle(mddev, true); /* this initializes IO event counters */
io_sectors = 0;
for (m = 0; m < SYNC_MARKS; m++) {
@@ -9920,7 +9920,7 @@ void md_do_sync(struct md_thread *thread)
goto repeat;
}
if (!sync_io_within_limit(mddev) &&
- !is_mddev_idle(mddev, 0)) {
+ !is_mddev_idle(mddev, false)) {
/*
* Give other IO more of a chance.
* The faster the devices, the less we wait.
--
2.43.0
^ permalink raw reply related
* [PATCH 4/5] md: use sector_t for recovery_active in status_resync()
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155421.211626-1-nishidafmly@gmail.com>
recovery_active holds a sector count read from the sector-typed atomic
mddev->recovery_active and is then combined with the sector_t values
curr_mark_cnt and resync_mark_cnt. Declaring it as a plain int needlessly
narrows it and mixes signedness into sector_t arithmetic; declare it
sector_t to match.
No functional change.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/md.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 829b4a3f545b..0cd85cc92ed3 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8816,8 +8816,8 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
{
sector_t max_sectors, resync, res;
unsigned long dt, db = 0;
- sector_t rt, curr_mark_cnt, resync_mark_cnt;
- int scale, recovery_active;
+ sector_t rt, curr_mark_cnt, resync_mark_cnt, recovery_active;
+ int scale;
unsigned int per_milli;
if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery) ||
--
2.43.0
^ permalink raw reply related
* [PATCH 5/5] md: clarify the resync ETA comment in status_resync()
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155421.211626-1-nishidafmly@gmail.com>
The comment above the remaining-time computation was self-contradictory:
it said the original algorithm was being kept but "is not really
necessary", then described that algorithm under an "Original algorithm:"
heading as if it had been replaced. The code still uses it. Rewrite the
comment to simply describe what the code does.
Comment only; no functional change.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/md.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0cd85cc92ed3..a5c0da0d1133 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -8916,17 +8916,11 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
* db: blocks written from mark until now
* rt: remaining time
*
- * rt is a sector_t, which is always 64bit now. We are keeping
- * the original algorithm, but it is not really necessary.
- *
- * Original algorithm:
- * So we divide before multiply in case it is 32bit and close
- * to the limit.
- * We scale the divisor (db) by 32 to avoid losing precision
- * near the end of resync when the number of remaining sectors
- * is close to 'db'.
- * We then divide rt by 32 after multiplying by db to compensate.
- * The '+1' avoids division by zero if db is very small.
+ * rt is computed as (remaining sectors) * dt / db. To keep precision
+ * near the end of resync, when the remaining count is close to db, the
+ * divisor db is scaled up by 32 before the divide and rt is scaled back
+ * down by 32 afterwards. The '+1' avoids division by zero when db is
+ * very small.
*/
dt = ((jiffies - mddev->resync_mark) / HZ);
if (!dt) dt++;
--
2.43.0
^ permalink raw reply related
* [PATCH 0/8] md/raid5: scalability and rebuild-path improvements
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
This series collects small, individually low-risk md/raid5 changes for
large, many-core, many-disk arrays. Their common theme is reducing
per-stripe and stripe-cache contention, so the benefit appears mainly
when the raid5 stripe-handling worker threads are in use
(group_thread_cnt > 0); at the default group_thread_cnt = 0 (a single
handling thread) the series is essentially neutral.
- patches 1-3 remove signed arithmetic from a hot-path divisor, lift an
arbitrary stripe-cache size cap, and widen a badblock length argument
that currently truncates large ranges;
- patch 4 raises NR_STRIPE_HASH_LOCKS (8 -> 32) to spread stripe-hash
contention on high core-count systems;
- patches 5 and 8 reduce per-stripe overhead in the resync/recovery
path and bound the share of the stripe cache a rebuild may hold while
user I/O is competing;
- patch 6 allocates each worker group's array on its own NUMA node;
- patch 7 raises MAX_STRIPE_BATCH (8 -> 32).
Measured effect, treatment vs baseline, % change in mean IOPS (N=3),
swept over group_thread_cnt (RAID6 4+2, 22-core host, ramdisk members):
workload gtc=0 gtc=2 gtc=4 gtc=8
random 4K write (RMW) +4.2% +8.1% +17.4% +6.5%
DB mixed 75/25 8K +0.4% +4.2% +10.3% +4.7%
high-concurrency 70/30 4K +3.9% +1.2% +10.0% +0.2%
OLTP 70/30 16K -0.3% +4.7% +10.1% +9.3%
partial-stripe write 8K +1.1% +4.8% +11.2% +14.2%
At the default single handling thread (group_thread_cnt = 0) the series is
neutral (no regression). As worker threads are added the gain grows,
peaking broadly around group_thread_cnt = 4 at roughly +10-17% across the
whole mix; at gtc = 8 the write-heavy workloads keep gaining while the
read-heavy high-concurrency case has saturated. (Per-run cv was <1%
except the random-write test, ~5-9%, from a cold first run.)
These numbers are on a ramdisk, which removes device latency and so
overstates the CPU-side contention effect relative to a real device;
they show the direction and the group_thread_cnt dependence, not an
absolute speedup. The stripe-hash/batch patches (4, 7) and the cache cap
(2) drive this; patch 6 only matters on multi-socket systems (not
exercised above) and patches 5/8 act on the resync/recovery path rather
than this steady-state workload.
Reproduction (stock mdadm + fio):
mdadm --create /dev/md0 --level=6 --raid-devices=6 --chunk=512 \
--assume-clean <6 members>
echo 16384 > /sys/block/md0/md/stripe_cache_size
echo N > /sys/block/md0/md/group_thread_cnt # N = 0,2,4,8
fio --filename=/dev/md0 --direct=1 --ioengine=libaio --group_reporting \
--time_based --runtime=15 --name=w <per-workload opts>:
random write : --rw=randwrite --bs=4k --numjobs=4 --iodepth=32
DB mixed : --rw=randrw --rwmixread=75 --bs=8k --numjobs=8 --iodepth=16
high-concur. : --rw=randrw --rwmixread=70 --bs=4k --numjobs=16 --iodepth=8
OLTP : --rw=randrw --rwmixread=70 --bs=16k --numjobs=6 --iodepth=16
partial-stripe : --rw=randwrite --bs=8k --numjobs=4 --iodepth=32
Each patch stands on its own; I am happy to drop or defer any that is not
justified on its own merit.
Functional testing on RAID5 and RAID6: create, fail a member, rebuild
onto a spare / re-add, full data read-back verified, and scrub
("check") reporting mismatch_cnt == 0. The series was also exercised
with KASAN and lockdep enabled -- including heavy group_thread_cnt
churn on a multi-node setup to stress the per-NUMA-node worker
allocation and the raid5_quiesce hash-lock-all path -- with no reports.
Hiroshi Nishida (8):
md: change chunk_sectors and stripe cache counts to unsigned int
md/raid5: raise stripe cache limit from 32768 to 262144
md: widen badblock sectors param from int to sector_t
md/raid5: raise NR_STRIPE_HASH_LOCKS from 8 to 32
md/raid5: submit a window of stripes during resync/recovery
md/raid5: allocate worker groups per NUMA node
md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
md/raid5: reserve stripe cache for user I/O during rebuild
drivers/md/md.c | 4 +-
drivers/md/md.h | 10 ++--
drivers/md/raid5.c | 129 ++++++++++++++++++++++++++++++++-------------
drivers/md/raid5.h | 33 ++++++++----
4 files changed, 121 insertions(+), 55 deletions(-)
base-commit: 55b77337bdd088c77461588e5ec094421b89911b
--
2.43.0
^ permalink raw reply
* [PATCH 1/8] md: change chunk_sectors and stripe cache counts to unsigned int
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
and min_nr_stripes are never negative. Using signed int is semantically
wrong and prevents the compiler from optimizing division/modulo by
power-of-two chunk sizes to right shifts in the hot I/O path.
Change all struct fields and derived local variables to unsigned int:
mddev->chunk_sectors
mddev->new_chunk_sectors
r5conf->chunk_sectors
r5conf->prev_chunk_sectors
r5conf->max_nr_stripes
r5conf->min_nr_stripes
Local: sectors_per_chunk, new_chunk, chunk_sectors
The min() in r5c_check_cached_full_stripe() required both operands to
match signedness; this is now satisfied with max_nr_stripes unsigned.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/md.h | 4 ++--
drivers/md/raid5.c | 14 +++++++-------
drivers/md/raid5.h | 8 ++++----
3 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index d8daf0f75cbb..b9ad26844799 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -437,7 +437,7 @@ struct mddev {
int external; /* metadata is
* managed externally */
char metadata_type[17]; /* externally set*/
- int chunk_sectors;
+ unsigned int chunk_sectors;
time64_t ctime, utime;
int level, layout;
char clevel[16];
@@ -466,7 +466,7 @@ struct mddev {
*/
sector_t reshape_position;
int delta_disks, new_level, new_layout;
- int new_chunk_sectors;
+ unsigned int new_chunk_sectors;
int reshape_backwards;
struct md_thread __rcu *thread; /* management thread */
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 0c5c9fb0606e..28828e083c2b 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
sector_t new_sector;
int algorithm = previous ? conf->prev_algo
: conf->algorithm;
- int sectors_per_chunk = previous ? conf->prev_chunk_sectors
+ unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
: conf->chunk_sectors;
int raid_disks = previous ? conf->previous_raid_disks
: conf->raid_disks;
@@ -3166,7 +3166,7 @@ sector_t raid5_compute_blocknr(struct stripe_head *sh, int i, int previous)
int raid_disks = sh->disks;
int data_disks = raid_disks - conf->max_degraded;
sector_t new_sector = sh->sector, check;
- int sectors_per_chunk = previous ? conf->prev_chunk_sectors
+ unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
: conf->chunk_sectors;
int algorithm = previous ? conf->prev_algo
: conf->algorithm;
@@ -3584,7 +3584,7 @@ static void end_reshape(struct r5conf *conf);
static void stripe_set_idx(sector_t stripe, struct r5conf *conf, int previous,
struct stripe_head *sh)
{
- int sectors_per_chunk =
+ unsigned int sectors_per_chunk =
previous ? conf->prev_chunk_sectors : conf->chunk_sectors;
int dd_idx;
int chunk_offset = sector_div(stripe, sectors_per_chunk);
@@ -6103,7 +6103,7 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
static sector_t raid5_bio_lowest_chunk_sector(struct r5conf *conf,
struct bio *bi)
{
- int sectors_per_chunk = conf->chunk_sectors;
+ unsigned int sectors_per_chunk = conf->chunk_sectors;
int raid_disks = conf->raid_disks;
int dd_idx;
struct stripe_head sh;
@@ -7930,7 +7930,7 @@ static int raid5_run(struct mddev *mddev)
sector_t here_new, here_old;
int old_disks;
int max_degraded = (mddev->level == 6 ? 2 : 1);
- int chunk_sectors;
+ unsigned int chunk_sectors;
int new_data_disks;
if (journal_dev) {
@@ -8832,7 +8832,7 @@ static int raid5_check_reshape(struct mddev *mddev)
* to be used by a reshape pass.
*/
struct r5conf *conf = mddev->private;
- int new_chunk = mddev->new_chunk_sectors;
+ unsigned int new_chunk = mddev->new_chunk_sectors;
if (mddev->new_layout >= 0 && !algorithm_valid_raid5(mddev->new_layout))
return -EINVAL;
@@ -8866,7 +8866,7 @@ static int raid5_check_reshape(struct mddev *mddev)
static int raid6_check_reshape(struct mddev *mddev)
{
- int new_chunk = mddev->new_chunk_sectors;
+ unsigned int new_chunk = mddev->new_chunk_sectors;
if (mddev->new_layout >= 0 && !algorithm_valid_raid6(mddev->new_layout))
return -EINVAL;
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index cb5feae04db2..5cd9d0f36b6e 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -572,12 +572,12 @@ struct r5conf {
/* only protect corresponding hash list and inactive_list */
spinlock_t hash_locks[NR_STRIPE_HASH_LOCKS];
struct mddev *mddev;
- int chunk_sectors;
+ unsigned int chunk_sectors;
int level, algorithm, rmw_level;
int max_degraded;
int raid_disks;
- int max_nr_stripes;
- int min_nr_stripes;
+ unsigned int max_nr_stripes;
+ unsigned int min_nr_stripes;
#if PAGE_SIZE != DEFAULT_STRIPE_SIZE
unsigned long stripe_size;
unsigned int stripe_shift;
@@ -595,7 +595,7 @@ struct r5conf {
*/
sector_t reshape_safe;
int previous_raid_disks;
- int prev_chunk_sectors;
+ unsigned int prev_chunk_sectors;
int prev_algo;
short generation; /* increments with every reshape */
seqcount_spinlock_t gen_lock; /* lock against generation changes */
--
2.43.0
^ permalink raw reply related
* [PATCH 2/8] md/raid5: raise stripe cache limit from 32768 to 262144
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
Stripe cache memory is approximately max_nr_stripes * num_disks *
stripe_size, where stripe_size is 4KB (PAGE_SIZE). The old hardcoded
ceiling of 32768 stripes therefore limits a 12-disk array to ~1.5GB of
stripe cache. On servers with 256GB+ RAM backing wide arrays, a larger
cache reduces stalls on stripe-cache misses under heavy write workloads.
Define RAID5_MAX_NR_STRIPES = 262144 (256K) in raid5.h and use it in
raid5_set_cache_size() instead of the magic 32768 literal. This allows
up to ~12GB of stripe cache on a 12-disk array. The default stripe count
(NR_STRIPES = 256) is unchanged, so there is no memory impact unless the
administrator raises stripe_cache_size explicitly.
Also fix two local variables in raid5_cache_count() that were declared
as int but read from unsigned int fields; change to unsigned int to
avoid implicit sign-extension in the subtraction.
Tested: echo 262144 > /sys/block/md0/md/stripe_cache_size accepted;
262145 correctly returns EINVAL.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid5.c | 6 +++---
drivers/md/raid5.h | 6 ++++++
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 28828e083c2b..9cb4ed3bd85c 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6923,7 +6923,7 @@ raid5_set_cache_size(struct mddev *mddev, int size)
int result = 0;
struct r5conf *conf = mddev->private;
- if (size <= 16 || size > 32768)
+ if (size <= 16 || size > RAID5_MAX_NR_STRIPES)
return -EINVAL;
WRITE_ONCE(conf->min_nr_stripes, size);
@@ -7505,8 +7505,8 @@ static unsigned long raid5_cache_count(struct shrinker *shrink,
struct shrink_control *sc)
{
struct r5conf *conf = shrink->private_data;
- int max_stripes = READ_ONCE(conf->max_nr_stripes);
- int min_stripes = READ_ONCE(conf->min_nr_stripes);
+ unsigned int max_stripes = READ_ONCE(conf->max_nr_stripes);
+ unsigned int min_stripes = READ_ONCE(conf->min_nr_stripes);
if (max_stripes < min_stripes)
/* unlikely, but not impossible */
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 5cd9d0f36b6e..57349737d393 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -479,6 +479,12 @@ struct disk_info {
*/
#define NR_STRIPES 256
+/* Maximum user-settable stripe cache size, in stripes. Stripe cache memory is
+ * roughly max_nr_stripes * num_disks * stripe_size (stripe_size = 4KB), so the
+ * old cap of 32768 limited a 12-disk array to ~1.5GB. Raise to 262144 to allow
+ * larger caches on big-RAM systems; the default (NR_STRIPES) is unchanged.
+ */
+#define RAID5_MAX_NR_STRIPES 262144U
#if PAGE_SIZE == DEFAULT_STRIPE_SIZE
#define STRIPE_SIZE PAGE_SIZE
--
2.43.0
^ permalink raw reply related
* [PATCH 3/8] md: widen badblock sectors param from int to sector_t
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
The badblocks core API -- badblocks_set(), badblocks_clear() and
badblocks_check() -- and the is_badblock() helper all take the range
length as sector_t. The md wrappers rdev_set_badblocks(),
rdev_clear_badblocks() and rdev_has_badblock(), however, declared the
same length as int, narrowing sector_t to int and back again in the
middle of an otherwise 64-bit clean path.
Change the sectors parameter to sector_t in these three wrappers so it
matches the core API and is_badblock(). No functional change: current
callers pass per-I/O or per-resync-chunk lengths well within int range.
This just removes a gratuitous truncation point and keeps the type
consistent end to end.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/md.c | 4 ++--
drivers/md/md.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d1465bcd86c8..61f40fa41e78 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10553,7 +10553,7 @@ EXPORT_SYMBOL(md_finish_reshape);
/* Bad block management */
/* Returns true on success, false on failure */
-bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
int is_new)
{
struct mddev *mddev = rdev->mddev;
@@ -10593,7 +10593,7 @@ bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
}
EXPORT_SYMBOL_GPL(rdev_set_badblocks);
-void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
int is_new)
{
if (is_new)
diff --git a/drivers/md/md.h b/drivers/md/md.h
index b9ad26844799..95835a3286aa 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -311,7 +311,7 @@ static inline int is_badblock(struct md_rdev *rdev, sector_t s, sector_t sectors
}
static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
- int sectors)
+ sector_t sectors)
{
sector_t first_bad;
sector_t bad_sectors;
@@ -319,9 +319,9 @@ static inline int rdev_has_badblock(struct md_rdev *rdev, sector_t s,
return is_badblock(rdev, s, sectors, &first_bad, &bad_sectors);
}
-extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+extern bool rdev_set_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
int is_new);
-extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, int sectors,
+extern void rdev_clear_badblocks(struct md_rdev *rdev, sector_t s, sector_t sectors,
int is_new);
struct md_cluster_info;
struct md_cluster_operations;
--
2.43.0
^ permalink raw reply related
* [PATCH 4/8] md/raid5: raise NR_STRIPE_HASH_LOCKS from 8 to 32
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
The stripe cache hash is striped across NR_STRIPE_HASH_LOCKS spinlocks
(see stripe_hash_locks_hash()). The value has been 8 since the per-hash
locking was introduced, which is small for modern many-core servers:
with only 8 buckets, stripe cache lookup and allocation contend on the
same few locks once the CPU count greatly exceeds the bucket count.
Raise it to 32. Two constraints bound the choice:
- STRIPE_HASH_LOCKS_MASK is (NR_STRIPE_HASH_LOCKS - 1) and is used as
a bitmask in stripe_hash_locks_hash(), so the value must be a power
of two.
- raid5_quiesce() acquires every hash lock plus device_lock at once
via lock_all_device_hash_locks_irq(). That holds
NR_STRIPE_HASH_LOCKS + 1 locks simultaneously, which must stay below
MAX_LOCK_DEPTH (48) so the held-lock array does not overflow when
lockdep is enabled.
32 is the largest power of two that satisfies both: 32 + 1 leaves
headroom under 48, whereas 64 would exceed it. (The pre-existing
"must remain below 64" comment understates this; MAX_LOCK_DEPTH is the
real ceiling.) STRIPE_HASH_LOCKS_MASK and all NR_STRIPE_HASH_LOCKS-
sized arrays scale automatically.
This is purely a lock-striping change; it does not affect stripe cache
correctness. The benefit appears on many-core systems, while on small
systems the extra buckets are harmless.
Tested: loop-device RAID-5 create, write/verify, fail a disk, rebuild
onto a spare and scrub all complete cleanly.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid5.h | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 57349737d393..3efab71ebef7 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -498,12 +498,14 @@ struct disk_info {
#define HASH_MASK (NR_HASH - 1)
#define MAX_STRIPE_BATCH 8
-/* NOTE NR_STRIPE_HASH_LOCKS must remain below 64.
- * This is because we sometimes take all the spinlocks
- * and creating that much locking depth can cause
- * problems.
+/* NR_STRIPE_HASH_LOCKS must be a power of two, since
+ * STRIPE_HASH_LOCKS_MASK masks with (NR_STRIPE_HASH_LOCKS - 1).
+ * It must also be small enough that taking all of them at once in
+ * lock_all_device_hash_locks_irq(), plus device_lock, keeps the held
+ * lock count below MAX_LOCK_DEPTH (48) with lockdep enabled. 32 is the
+ * largest power of two that satisfies both constraints.
*/
-#define NR_STRIPE_HASH_LOCKS 8
+#define NR_STRIPE_HASH_LOCKS 32
#define STRIPE_HASH_LOCKS_MASK (NR_STRIPE_HASH_LOCKS - 1)
struct r5worker {
--
2.43.0
^ permalink raw reply related
* [PATCH 5/8] md/raid5: submit a window of stripes during resync/recovery
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
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.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
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 9cb4ed3bd85c..8e9edaaca667 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 3efab71ebef7..7aeba1fc7f09 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -497,6 +497,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 */
/* NR_STRIPE_HASH_LOCKS must be a power of two, since
* STRIPE_HASH_LOCKS_MASK masks with (NR_STRIPE_HASH_LOCKS - 1).
--
2.43.0
^ permalink raw reply related
* [PATCH 6/8] md/raid5: allocate worker groups per NUMA node
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
alloc_thread_groups() previously allocated all r5worker arrays in a
single kcalloc() block, assigning workers for NUMA node N from node 0
memory. On multi-socket systems this causes remote memory traffic on
every worker->work and worker->temp_inactive_list access.
Replace the single allocation with kzalloc_node(size, GFP_NOIO, i) per
group so each node's workers live in local memory. Because the workers
are now separate per-node allocations, both free sites --
free_thread_groups() and the reallocation path in
raid5_store_group_thread_cnt() -- are updated to free each group's
allocation individually instead of only group 0's.
Also fix a latent bug: the original kcalloc() had its nmemb and size
arguments swapped (harmless due to commutativity but semantically wrong).
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid5.c | 39 ++++++++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 8e9edaaca667..c8787ab7b309 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -7297,8 +7297,12 @@ raid5_store_group_thread_cnt(struct mddev *mddev, const char *page, size_t len)
conf->worker_groups = new_groups;
spin_unlock_irq(&conf->device_lock);
- if (old_groups)
- kfree(old_groups[0].workers);
+ if (old_groups) {
+ int node;
+
+ for (node = 0; node < num_possible_nodes(); node++)
+ kfree(old_groups[node].workers);
+ }
kfree(old_groups);
}
}
@@ -7336,7 +7340,6 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
{
int i, j, k;
ssize_t size;
- struct r5worker *workers;
if (cnt == 0) {
*group_cnt = 0;
@@ -7344,24 +7347,24 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
return 0;
}
*group_cnt = num_possible_nodes();
- size = sizeof(struct r5worker) * cnt;
- workers = kcalloc(size, *group_cnt, GFP_NOIO);
*worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,
GFP_NOIO);
- if (!*worker_groups || !workers) {
- kfree(workers);
- kfree(*worker_groups);
+ if (!*worker_groups)
return -ENOMEM;
- }
+ size = sizeof(struct r5worker) * cnt;
for (i = 0; i < *group_cnt; i++) {
- struct r5worker_group *group;
+ struct r5worker_group *group = &(*worker_groups)[i];
+ struct r5worker *workers;
+
+ workers = kzalloc_node(size, GFP_NOIO, i);
+ if (!workers)
+ goto out_free;
- group = &(*worker_groups)[i];
INIT_LIST_HEAD(&group->handle_list);
INIT_LIST_HEAD(&group->loprio_list);
group->conf = conf;
- group->workers = workers + i * cnt;
+ group->workers = workers;
for (j = 0; j < cnt; j++) {
struct r5worker *worker = group->workers + j;
@@ -7374,12 +7377,22 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
}
return 0;
+
+out_free:
+ while (--i >= 0)
+ kfree((*worker_groups)[i].workers);
+ kfree(*worker_groups);
+ *worker_groups = NULL;
+ return -ENOMEM;
}
static void free_thread_groups(struct r5conf *conf)
{
+ int i;
+
if (conf->worker_groups)
- kfree(conf->worker_groups[0].workers);
+ for (i = 0; i < conf->group_cnt; i++)
+ kfree(conf->worker_groups[i].workers);
kfree(conf->worker_groups);
conf->worker_groups = NULL;
}
--
2.43.0
^ permalink raw reply related
* [PATCH 7/8] md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
handle_active_stripes() dequeues up to MAX_STRIPE_BATCH stripes under
device_lock per pass, then processes them after dropping the lock.
Raising the batch from 8 to 32 amortizes each device_lock acquisition
over more stripes, reducing lock cycles per unit of stripe work.
The worker-spawn heuristic in raid5_wakeup_stripe_thread() also divided
by MAX_STRIPE_BATCH; split it into a separate STRIPE_BATCH_WORKERS=8 so
the spawn sensitivity is preserved unchanged while the batch size grows.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
Signed-off-by: Hiroshi Nishida <nishidafmly@gmail.com>
---
drivers/md/raid5.c | 2 +-
drivers/md/raid5.h | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index c8787ab7b309..ad6230415af3 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++) {
if (group->workers[i].working == false) {
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 7aeba1fc7f09..1f37dabd727b 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 */
/* NR_STRIPE_HASH_LOCKS must be a power of two, since
--
2.43.0
^ permalink raw reply related
* [PATCH 8/8] md/raid5: reserve stripe cache for user I/O during rebuild
From: Hiroshi Nishida @ 2026-06-24 15:54 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260624155452.211646-1-nishidafmly@gmail.com>
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.
Assisted-by: Claude:claude-opus-4-8 [Claude Code]
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 ad6230415af3..480f3aa069ef 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 1f37dabd727b..7833cc07597f 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -499,6 +499,7 @@ struct disk_info {
#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 */
+#define RAID5_SYNC_HWMARK 2 /* rebuild uses at most 1/N of stripe cache */
/* NR_STRIPE_HASH_LOCKS must be a power of two, since
* STRIPE_HASH_LOCKS_MASK masks with (NR_STRIPE_HASH_LOCKS - 1).
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 5/5] md: clarify the resync ETA comment in status_resync()
From: sashiko-bot @ 2026-06-24 15:58 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155421.211626-6-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The newly added comment incorrectly states that the divisor `db` is scaled up by 32, when the code actually scales it down.
--
commit 21672ecf7d2b823245c32b9d2699f7cf1a5c2e7c
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: clarify the resync ETA comment in status_resync()
This commit updates the block comment above the remaining-time computation
in status_resync() to clarify the calculation logic. It is a comment-only
change.
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0cd85cc92ed31..a5c0da0d11334 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -8916,17 +8916,11 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
> * db: blocks written from mark until now
> * rt: remaining time
> *
> - * rt is a sector_t, which is always 64bit now. We are keeping
> - * the original algorithm, but it is not really necessary.
> - *
> - * Original algorithm:
> - * So we divide before multiply in case it is 32bit and close
> - * to the limit.
> - * We scale the divisor (db) by 32 to avoid losing precision
> - * near the end of resync when the number of remaining sectors
> - * is close to 'db'.
> - * We then divide rt by 32 after multiplying by db to compensate.
> - * The '+1' avoids division by zero if db is very small.
> + * rt is computed as (remaining sectors) * dt / db. To keep precision
> + * near the end of resync, when the remaining count is close to db, the
> + * divisor db is scaled up by 32 before the divide and rt is scaled back
[Severity: Low]
Is this new comment accurate regarding the math applied to db?
The comment states that the divisor db is scaled up by 32, but looking at
the actual code:
rt = max_sectors - resync;
rt = div64_u64(rt, db/32+1);
This divides db by 32, effectively scaling it down rather than up (which in
turn scales the resulting quotient up). Does this comment need to be
adjusted to reflect the db/32 downscaling?
> + * down by 32 afterwards. The '+1' avoids division by zero when db is
> + * very small.
> */
> dt = ((jiffies - mddev->resync_mark) / HZ);
> if (!dt) dt++;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=5
^ permalink raw reply
* Re: [PATCH 4/5] md: use sector_t for recovery_active in status_resync()
From: sashiko-bot @ 2026-06-24 16:03 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: yukuai, linux-raid
In-Reply-To: <20260624155421.211626-5-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message falsely claims `mddev->recovery_active` is a 'sector-typed atomic' and that declaring a local variable as `int` 'needlessly narrows it', but the structure field remains a standard 32-bit `atomic_t`.
--
commit 7ec77a6b233ce4d151f7fc723e5f312b20b4ac92
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: use sector_t for recovery_active in status_resync()
This commit changes the local variable recovery_active in status_resync()
from int to sector_t. The commit message states this is to match the type
of mddev->recovery_active and avoid needlessly narrowing the value.
> recovery_active holds a sector count read from the sector-typed atomic
> mddev->recovery_active and is then combined with the sector_t values
> curr_mark_cnt and resync_mark_cnt. Declaring it as a plain int needlessly
> narrows it and mixes signedness into sector_t arithmetic; declare it
> sector_t to match.
[Severity: Medium]
Is this description accurate? Looking at struct mddev in drivers/md/md.h,
recovery_active is defined as a standard atomic_t:
drivers/md/md.h:struct mddev {
...
atomic_t recovery_active;
...
}
Since atomic_t is a 32-bit signed integer, and atomic_read() returns an int,
does changing this local variable to sector_t actually prevent narrowing? The
implicit cast from int to sector_t will now simply happen at assignment
rather than during the arithmetic operations.
Could the commit message be updated to reflect that mddev->recovery_active
is a standard atomic_t, rather than a "sector-typed atomic"?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=4
^ permalink raw reply
* Re: [PATCH 6/8] md/raid5: allocate worker groups per NUMA node
From: sashiko-bot @ 2026-06-24 16:07 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-7-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch incorrectly uses a sequential loop index `i` as the NUMA node ID parameter to `kzalloc_node()`.
Pre-existing issues:
- [High] The `worker_groups` array is sized using `num_possible_nodes()` but is indexed by the raw NUMA node ID, leading to out-of-bounds accesses on systems with sparse NUMA node IDs.
--
commit bce08bf6ce446b00da625ff4028f5782b756e17c
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md/raid5: allocate worker groups per NUMA node
alloc_thread_groups() previously allocated all r5worker arrays in a
single kcalloc() block, assigning workers for NUMA node N from node 0
memory. On multi-socket systems this causes remote memory traffic on
every worker->work and worker->temp_inactive_list access.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8e9edaaca667b..c8787ab7b309c 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -7336,7 +7340,6 @@ static int alloc_thread_groups(struct r5conf *conf, int cnt, int *group_cnt,
[ ... ]
> *group_cnt = num_possible_nodes();
> - size = sizeof(struct r5worker) * cnt;
> - workers = kcalloc(size, *group_cnt, GFP_NOIO);
> *worker_groups = kzalloc_objs(struct r5worker_group, *group_cnt,
> GFP_NOIO);
[Severity: High]
This isn't a bug introduced by this patch, but could this array sizing cause
an out-of-bounds access on systems with sparse NUMA nodes?
We size the worker_groups array using num_possible_nodes(), which is a count.
On systems with sparse NUMA node IDs (for example, nodes 0 and 8), the array
would only have 2 elements. Later, in raid5_wakeup_stripe_thread(), we index
into this array using cpu_to_group(cpu) which expands to cpu_to_node(cpu).
If a CPU belongs to node 8, we would access conf->worker_groups[8], exceeding
the allocated size and corrupting memory.
> - if (!*worker_groups || !workers) {
> - kfree(workers);
> - kfree(*worker_groups);
> + if (!*worker_groups)
> return -ENOMEM;
> - }
>
> + size = sizeof(struct r5worker) * cnt;
> for (i = 0; i < *group_cnt; i++) {
> - struct r5worker_group *group;
> + struct r5worker_group *group = &(*worker_groups)[i];
> + struct r5worker *workers;
> +
> + workers = kzalloc_node(size, GFP_NOIO, i);
[Severity: High]
Could passing the sequential loop index i directly to kzalloc_node cause a
NULL pointer dereference on systems with sparse NUMA nodes?
Since num_possible_nodes returns a count rather than an upper bound on IDs,
on a system with nodes 0 and 8, i would be evaluated as 1. Passing an
unpopulated node ID to kzalloc_node without validation can lead to a crash
when accessing NODE_DATA(nid). Should this loop use
for_each_node_state(nid, N_MEMORY) or similar to get the actual node ID
instead of using the loop counter?
> + if (!workers)
> + goto out_free;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=6
^ permalink raw reply
* Re: [PATCH 7/8] md/raid5: raise MAX_STRIPE_BATCH from 8 to 32
From: sashiko-bot @ 2026-06-24 16:09 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: yukuai, linux-raid
In-Reply-To: <20260624155452.211646-8-nishidafmly@gmail.com>
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 <nishidafmly@gmail.com>
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/20260624155452.211646-1-nishidafmly@gmail.com?part=7
^ permalink raw reply
* Re: [PATCH 5/8] md/raid5: submit a window of stripes during resync/recovery
From: sashiko-bot @ 2026-06-24 16:12 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-6-nishidafmly@gmail.com>
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 <nishidafmly@gmail.com>
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/20260624155452.211646-1-nishidafmly@gmail.com?part=5
^ permalink raw reply related
* Re: [PATCH 8/8] md/raid5: reserve stripe cache for user I/O during rebuild
From: sashiko-bot @ 2026-06-24 16:12 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-9-nishidafmly@gmail.com>
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 <nishidafmly@gmail.com>
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/20260624155452.211646-1-nishidafmly@gmail.com?part=8
^ permalink raw reply
* Re: [PATCH 1/8] md: change chunk_sectors and stripe cache counts to unsigned int
From: sashiko-bot @ 2026-06-24 16:16 UTC (permalink / raw)
To: Hiroshi Nishida; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155452.211646-2-nishidafmly@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [Medium] The commit's core justification is false. Changing the types to unsigned does not enable compiler right-shift optimizations for variable divisions.
- [Low] Unupdated printf-style format specifiers passing unsigned fields to '%d' cause compiler warnings (-Wformat) and break builds configured with -Werror.
Pre-existing issues:
- [Critical] The macro `round_down` is incorrectly used with a non-power-of-two divisor, causing silent data corruption in bitmap sector calculations.
--
commit 86528a8b22367a5ee16a7cb19a1d56bf58f6aa67
Author: Hiroshi Nishida <nishidafmly@gmail.com>
md: change chunk_sectors and stripe cache counts to unsigned int
This commit changes several structural fields and local variables like
chunk_sectors and max_nr_stripes from signed to unsigned int. The stated
rationale is to prevent negative values and enable right-shift compiler
optimizations for power-of-two division in the hot I/O path.
> chunk_sectors, new_chunk_sectors, prev_chunk_sectors, max_nr_stripes,
> and min_nr_stripes are never negative. Using signed int is semantically
> wrong and prevents the compiler from optimizing division/modulo by
> power-of-two chunk sizes to right shifts in the hot I/O path.
[Severity: Medium]
Is this optimization rationale accurate? The hot I/O path uses sector_div(),
which expands to do_div(). The do_div() macro explicitly casts the divisor
to a 32-bit unsigned integer, so the division was already operating on an
unsigned value.
Additionally, because sectors_per_chunk is a runtime variable rather than a
compile-time constant, would the compiler be able to optimize it into a
right shift without a conditional branch?
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d8daf0f75cbbe..b9ad26844799d 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
[ ... ]
> @@ -437,7 +437,7 @@ struct mddev {
> int external; /* metadata is
> * managed externally */
> char metadata_type[17]; /* externally set*/
> - int chunk_sectors;
> + unsigned int chunk_sectors;
[Severity: Low]
Does changing this field to unsigned int cause compiler warnings with
existing format strings?
For example, chunk_size_show() in drivers/md/md.c appears to use %d to
format mddev->chunk_sectors:
chunk_size_show() {
...
return sprintf(page, "%d (%d)\n",
mddev->new_chunk_sectors << 9,
mddev->chunk_sectors << 9);
}
Will this break builds configured with -Werror?
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index cb5feae04db27..5cd9d0f36b6e0 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
[ ... ]
> @@ -572,12 +572,12 @@ struct r5conf {
> /* only protect corresponding hash list and inactive_list */
> spinlock_t hash_locks[NR_STRIPE_HASH_LOCKS];
> struct mddev *mddev;
> - int chunk_sectors;
> + unsigned int chunk_sectors;
> int level, algorithm, rmw_level;
> int max_degraded;
> int raid_disks;
> - int max_nr_stripes;
> - int min_nr_stripes;
> + unsigned int max_nr_stripes;
> + unsigned int min_nr_stripes;
[Severity: Low]
Will the changes to min_nr_stripes and max_nr_stripes also trigger format
warnings?
Places like raid5_end_read_request() and raid5_set_cache_size() still use
%d to print these fields. For instance, in raid5_set_cache_size():
pr_err("md/raid:%s: Cannot increase cache size, ret=%d, new_size=%d, min_nr_stripes=%d, max_nr_stripes=%d\n",
...
conf->min_nr_stripes,
conf->max_nr_stripes);
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 0c5c9fb0606ee..28828e083c2b3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
[ ... ]
> @@ -2970,7 +2970,7 @@ sector_t raid5_compute_sector(struct r5conf *conf, sector_t r_sector,
> sector_t new_sector;
> int algorithm = previous ? conf->prev_algo
> : conf->algorithm;
> - int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> + unsigned int sectors_per_chunk = previous ? conf->prev_chunk_sectors
> : conf->chunk_sectors;
[Severity: Critical]
This is a pre-existing issue, but could the use of sectors_per_chunk
cause problems when passed to rounding macros?
Looking at raid5_bitmap_sector():
raid5_bitmap_sector() {
...
sectors_per_chunk = conf->chunk_sectors *
(conf->raid_disks - conf->max_degraded);
start = round_down(start, sectors_per_chunk);
end = round_up(end, sectors_per_chunk);
...
}
The round_down() and round_up() macros are designed specifically for
power-of-two divisors. If the number of data disks is not a power of two,
sectors_per_chunk will not be a power of two, which would yield incorrect
sector boundaries.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260624155452.211646-1-nishidafmly@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 4/5] md: use sector_t for recovery_active in status_resync()
From: Hiroshi Nishida @ 2026-06-24 16:48 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260624160331.9C5F51F000E9@smtp.kernel.org>
> [Medium] mddev->recovery_active is a standard atomic_t, not "sector-typed";
> changing the local to sector_t doesn't prevent narrowing.
You're right -- mddev->recovery_active is an atomic_t and
atomic_read() returns int, so there's nothing wider to narrow; the
int->sector_t conversion just moves to the assignment.
The change is only a type-consistency tidy (the local is used solely
with the sector_t curr_mark_cnt/resync_mark_cnt), not a narrowing fix.
I'll reword the commit message in v2 accordingly and drop the
"sector-typed atomic"/"narrows" wording.
2026年6月24日(水) 9:03 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The commit message falsely claims `mddev->recovery_active` is a 'sector-typed atomic' and that declaring a local variable as `int` 'needlessly narrows it', but the structure field remains a standard 32-bit `atomic_t`.
> --
>
> commit 7ec77a6b233ce4d151f7fc723e5f312b20b4ac92
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md: use sector_t for recovery_active in status_resync()
>
> This commit changes the local variable recovery_active in status_resync()
> from int to sector_t. The commit message states this is to match the type
> of mddev->recovery_active and avoid needlessly narrowing the value.
>
> > recovery_active holds a sector count read from the sector-typed atomic
> > mddev->recovery_active and is then combined with the sector_t values
> > curr_mark_cnt and resync_mark_cnt. Declaring it as a plain int needlessly
> > narrows it and mixes signedness into sector_t arithmetic; declare it
> > sector_t to match.
>
> [Severity: Medium]
> Is this description accurate? Looking at struct mddev in drivers/md/md.h,
> recovery_active is defined as a standard atomic_t:
>
> drivers/md/md.h:struct mddev {
> ...
> atomic_t recovery_active;
> ...
> }
>
> Since atomic_t is a 32-bit signed integer, and atomic_read() returns an int,
> does changing this local variable to sector_t actually prevent narrowing? The
> implicit cast from int to sector_t will now simply happen at assignment
> rather than during the arithmetic operations.
>
> Could the commit message be updated to reflect that mddev->recovery_active
> is a standard atomic_t, rather than a "sector-typed atomic"?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=4
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH 5/5] md: clarify the resync ETA comment in status_resync()
From: Hiroshi Nishida @ 2026-06-24 16:50 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <20260624155828.3C6F91F000E9@smtp.kernel.org>
> The comment states that the divisor db is scaled up by 32, but the code
> rt = div64_u64(rt, db/32+1); divides db by 32 -- scaling it down.
You're right -- the code divides by db/32, which scales the divisor
*down* by 32 (so the intermediate quotient is 32x larger and keeps
low-order bits a plain divide by db would lose), and rt is then
rescaled by the >> 5.
My rewritten comment had the direction backwards. I'll fix it in v2:
rt is, in exact arithmetic, (remaining sectors) * dt / db. To keep
precision near the end of resync, when the remaining count is close to db,
the dividend is divided by db/32 rather than db -- i.e. the divisor is
scaled down by 32, so the intermediate quotient is 32x larger -- and rt is
scaled back down by 32 (>> 5) afterwards.
Thanks,
2026年6月24日(水) 8:58 <sashiko-bot@kernel.org>:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] The newly added comment incorrectly states that the divisor `db` is scaled up by 32, when the code actually scales it down.
> --
>
> commit 21672ecf7d2b823245c32b9d2699f7cf1a5c2e7c
> Author: Hiroshi Nishida <nishidafmly@gmail.com>
>
> md: clarify the resync ETA comment in status_resync()
>
> This commit updates the block comment above the remaining-time computation
> in status_resync() to clarify the calculation logic. It is a comment-only
> change.
>
> > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > index 0cd85cc92ed31..a5c0da0d11334 100644
> > --- a/drivers/md/md.c
> > +++ b/drivers/md/md.c
> > @@ -8916,17 +8916,11 @@ static int status_resync(struct seq_file *seq, struct mddev *mddev)
> > * db: blocks written from mark until now
> > * rt: remaining time
> > *
> > - * rt is a sector_t, which is always 64bit now. We are keeping
> > - * the original algorithm, but it is not really necessary.
> > - *
> > - * Original algorithm:
> > - * So we divide before multiply in case it is 32bit and close
> > - * to the limit.
> > - * We scale the divisor (db) by 32 to avoid losing precision
> > - * near the end of resync when the number of remaining sectors
> > - * is close to 'db'.
> > - * We then divide rt by 32 after multiplying by db to compensate.
> > - * The '+1' avoids division by zero if db is very small.
> > + * rt is computed as (remaining sectors) * dt / db. To keep precision
> > + * near the end of resync, when the remaining count is close to db, the
> > + * divisor db is scaled up by 32 before the divide and rt is scaled back
>
> [Severity: Low]
> Is this new comment accurate regarding the math applied to db?
>
> The comment states that the divisor db is scaled up by 32, but looking at
> the actual code:
>
> rt = max_sectors - resync;
> rt = div64_u64(rt, db/32+1);
>
> This divides db by 32, effectively scaling it down rather than up (which in
> turn scales the resulting quotient up). Does this comment need to be
> adjusted to reflect the db/32 downscaling?
>
> > + * down by 32 afterwards. The '+1' avoids division by zero when db is
> > + * very small.
> > */
> > dt = ((jiffies - mddev->resync_mark) / HZ);
> > if (!dt) dt++;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260624155421.211626-1-nishidafmly@gmail.com?part=5
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox