* [PATCH 2/2] md: widen badblock sectors param from int to sector_t
From: Hiroshi Nishida @ 2026-07-10 13:23 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260710132329.7273-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.
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 1/2] md: change chunk_sectors and stripe cache counts to unsigned int
From: Hiroshi Nishida @ 2026-07-10 13:23 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
In-Reply-To: <20260710132329.7273-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.
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 0/2] md: widen size/count types for large arrays
From: Hiroshi Nishida @ 2026-07-10 13:23 UTC (permalink / raw)
To: Song Liu, Yu Kuai
Cc: Li Nan, Xiao Ni, linux-raid, linux-kernel, Hiroshi Nishida
Two small type-widening fixes for large arrays. Neither changes
behaviour or memory use on any current configuration.
1/2 changes chunk_sectors and the stripe-cache count fields to unsigned
int, matching how they are used and avoiding implicit sign
extension in the cache shrinker's subtraction.
2/2 widens the badblocks sector parameters from int to sector_t so the
helpers do not narrow a sector_t argument on very large devices.
These are independent of the other md/raid5 patches I'm sending and can
be applied on their own.
Hiroshi Nishida (2):
md: change chunk_sectors and stripe cache counts to unsigned int
md: widen badblock sectors param from int to sector_t
drivers/md/md.c | 4 ++--
drivers/md/md.h | 10 +++++-----
drivers/md/raid5.c | 14 +++++++-------
drivers/md/raid5.h | 8 ++++----
4 files changed, 18 insertions(+), 18 deletions(-)
base-commit: 55b77337bdd088c77461588e5ec094421b89911b
--
2.43.0
^ permalink raw reply
* Re: [PATCH 0/8] md/raid5: scalability and rebuild-path improvements
From: Hiroshi Nishida @ 2026-07-10 13:09 UTC (permalink / raw)
To: yukuai; +Cc: Song Liu, Li Nan, Xiao Ni, linux-raid, linux-kernel
In-Reply-To: <aa24dd05-f231-4657-8fef-8201138da65f@fygo.io>
Hi Yu Kuai,
Thanks again for the review, and in particular for:
> I can accept make those values configurable, but not direct
> modifications.
I've reworked the tunables along exactly those lines and will send them as
a separate, self-contained series, "md/raid5: size stripe-cache and worker
tuning from the hardware".
Rather than raising any fixed constant, each value now derives a default
from the hardware and stays overridable:
- the stripe-cache hash lock count (was a fixed 8) is sized from the CPU
count, clamped to 8..32;
- the stripe_cache_size ceiling (was a fixed 32768) scales with memory,
but never drops below 32768;
- the initial stripe_cache_size (was a fixed 256) scales gently with
memory, 256..4096;
- the default group_thread_cnt (was 0, i.e. single-threaded) is derived
from the CPU count (half the online CPUs per NUMA node, capped);
- the stripe batch size (was a fixed 8) is exposed as a parameter.
Ahead of those, the series opens with a one-line prerequisite fix:
alloc_thread_groups() sizes the per-node worker_groups[] array by
num_possible_nodes() but indexes it by cpu_to_node(), so a sparse NUMA node
map can index off the end -- reachable once the worker-group default (the
last patch) is on. It is Fixes:-tagged and can be taken on its own.
The key point for the consumer-NAS concern you raised: each default only
rises on hardware that can back it -- the lock and worker counts scale with
the core count, the cache sizes with RAM -- so a genuinely small system (a
few cores and a few GB) keeps today's values and footprint. And each one is
overridable via a module parameter (group_thread_cnt also via its existing
sysfs attribute), including all the way back to today's behaviour. So nothing is
imposed; the default simply tracks the machine instead of a constant, and
a wide array on a large host no longer needs a recompile or manual
per-array tuning to use the memory and cores it has.
And on performance -- on real NVMe this time, not a ramdisk: on a 16-disk
raid6 array (a 32-vCPU / 16-core host, steady state, interleaved runs) the
hardware-derived defaults run 2.1-3.2x the stock defaults (4K random write
~39k -> ~100k IOPS), essentially all of it from patch 6 turning on the
worker groups. Patches 2-5 (the cache and lock sizing) are
throughput-neutral on real SSD, as you'd expect -- they earn their place as
configurability and sane defaults, not a speed claim. And there is no
regression at the small end: a 4-CPU box derives 2 workers and is never
slower on any workload, and a box with two or fewer CPUs derives 0 and is
byte-for-byte unchanged.
It has been through KASAN + lockdep + DEBUG_LIST on RAID5
(create/rebuild/scrub, plus the bitmap add/remove that drives the
lock-all-hash-locks quiesce path), at both the derived defaults and pinned
values including nr_stripe_hash_locks=32.
The two unrelated parts of the original series -- the type-widening /
correctness fixes and the resync/recovery dispatch changes -- I'll send as
their own small series, as discussed.
Thanks,
2026年7月5日(日) 19:56 yu kuai <yukuai@fygo.io>:
>
> Hi,
>
> 在 2026/6/24 23:54, 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):
>
> Testing with ramdisk does serve as a useful reference, but it does not reflect
> real world usage.
>
> >
> > 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%
>
> With a quick review I saw many static configurations is changed, I agree
> these changes can improve arrays with ssd/nvme and a system with large
> memory available. However, we already tested with hdd and about 8G memory
> available, these changes will not improve performance at all, with the
> extra memory overhead.
>
> I can accept make those values configurable, but not direct modifications.
> As validation is required for numerous scenarios. Memory resources are precious
> especially for most consumer NAS devices.
>
> >
> > 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
> >
> --
> Thanks,
> Kuai
--
Hiroshi Nishida
nishidafmly@gmail.com
^ permalink raw reply
* Re: [PATCH v4 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: sashiko-bot @ 2026-07-10 10:31 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260710101521.1714-2-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Allocation of `r10_bio` with `GFP_NOIO` can sleep, violating `REQ_NOWAIT` semantics.
- [High] Functions in the IO path (`wait_blocked_dev()` and `wait_barrier()`) sleep unconditionally, violating `REQ_NOWAIT` semantics.
--
--- Patch [1]: [PATCH v4 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths ---
commit d87114f7731cb53cc142eafbf2338e5b721f84ad
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: fix r10bio leak in raid10_write_request() error paths
When raid10_write_request() fails because REQ_NOWAIT is set, the
allocated r10_bio is not freed before returning, resulting in a memory
leak. Free r10_bio before returning from the REQ_NOWAIT error paths.
Closes: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..bd322eccdc3ff 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> /* Bail out if REQ_NOWAIT is set for the bio */
> if (bio->bi_opf & REQ_NOWAIT) {
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[Severity: High]
This isn't a bug introduced by this patch, but while looking at REQ_NOWAIT
handling, it appears the initial allocation of r10_bio in __make_request()
and raid10_handle_discard() ignores the REQ_NOWAIT flag.
Both functions call:
r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
Since GFP_NOIO includes __GFP_DIRECT_RECLAIM, this can sleep if the mempool
is depleted.
Could this violate the strict non-blocking guarantees of REQ_NOWAIT IO and
potentially starve async workers?
> for (;;) {
[ ... ]
> @@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> if (bio->bi_opf & REQ_NOWAIT) {
> allow_barrier(conf);
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[Severity: High]
This is a pre-existing issue, but further down in the IO path, several
functions seem to sleep unconditionally even for REQ_NOWAIT bios.
For example, wait_blocked_dev() does:
md_wait_for_blocked_rdev(blocked_rdev, mddev);
wait_barrier(conf, false);
goto retry_wait;
Also, in raid10_write_request() and raid10_read_request(), when a bio spans a
chunk boundary and is split, the code unconditionally blocks:
allow_barrier(conf);
bio = bio_submit_split_bioset(bio, max_sectors, &conf->bio_split);
wait_barrier(conf, false);
Could these calls to wait_barrier(conf, false) and md_wait_for_blocked_rdev()
deadlock async submitters that rely on the non-blocking guarantee?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710101521.1714-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v4 3/7] md/raid10: consistently fail atomic writes that require splitting
From: sashiko-bot @ 2026-07-10 10:28 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260710101521.1714-4-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent disk removal during a failed or split write causes a NULL pointer dereference in the error handling path.
--
--- Patch [3]: [PATCH v4 3/7] md/raid10: consistently fail atomic writes that require splitting ---
commit a682adb077534fe2522eacd5e431dab764a60d3f
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: consistently fail atomic writes that require splitting
This patch addresses atomic writes in RAID10 by checking if they require
splitting. If an atomic write would be split due to badblocks or other
limits, it now correctly fails the bio with EIO rather than allowing the
implicit EINVAL from bio_submit_split_bioset().
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index bd322eccdc3ff..3480fc7907f01 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> r10_bio->sectors = max_sectors;
>
> if (r10_bio->sectors < bio_sectors(bio)) {
> + if (atomic)
> + goto err_handle;
[Severity: High]
This is a pre-existing issue, but can this jump to the error handling path
trigger a NULL pointer dereference if a disk removal happens concurrently?
If raid10_write_request() initiates an IO and skips a Faulty primary rdev,
it uses the replacement (setting repl_bio and incrementing rrdev->nr_pending).
If md_check_recovery() concurrently calls raid10_remove_disk() for the Faulty
primary, it can promote the replacement to primary and set p->replacement
to NULL.
When this atomic check fails, it jumps to the error unwinding loop which
does:
struct md_rdev *rrdev = conf->mirrors[d].replacement;
...
if (r10_bio->devs[k].repl_bio) {
rdev_dec_pending(rrdev, mddev);
r10_bio->devs[k].repl_bio = NULL;
}
Since replacement is now NULL, rrdev is NULL, and rdev_dec_pending() will
dereference it.
Looking at raid10_end_write_request(), it implements a fallback to handle
concurrent promotion by falling back to conf->mirrors[d].rdev when
replacement is NULL.
Does the error unwinding loop in raid10_write_request() need a similar
fallback?
> +
> allow_barrier(conf);
> bio = bio_submit_split_bioset(bio, r10_bio->sectors,
> &conf->bio_split);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710101521.1714-1-abd.masalkhi@gmail.com?part=3
^ permalink raw reply
* [PATCH v4 7/7] md/raid10: simplify read request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
raid10_read_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Make raid10_read_request() return a status to its callers, consolidate
the read error paths, and free r10_bio from a single location in the
callers. Since the callers allocate r10_bio, they should also be
responsible for freeing it when the request fails.
This makes the read path follow the same ownership model as the write
path and simplifies the error handling flow.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-8-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/
Changes in v2:
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d94c1f28a6f6..01162c483644 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
return true;
}
-static void raid10_read_request(struct mddev *mddev, struct bio *bio,
+static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
struct r10bio *r10_bio)
{
struct r10conf *conf = mddev->private;
@@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return;
+ return false;
}
rdev = read_balance(conf, r10_bio, &max_sectors);
@@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
mdname(mddev), b,
(unsigned long long)r10_bio->sector);
}
- raid_end_bio_io(r10_bio);
- return;
+ bio_io_error(bio);
+ goto err_allow_barrier;
}
if (err_rdev)
pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
@@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
wait_barrier(conf, false);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
r10_bio->sectors = max_sectors;
@@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
read_bio->bi_private = r10_bio;
mddev_trace_remap(mddev, read_bio, r10_bio->sector);
submit_bio_noacct(read_bio);
- return;
-err_handle:
+
+ return true;
+
+err_dec_pending:
atomic_dec(&rdev->nr_pending);
- raid_end_bio_io(r10_bio);
+
+err_allow_barrier:
+ allow_barrier(conf);
+
+ return false;
}
static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
@@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
conf->geo.raid_disks);
- ret = true;
if (bio_data_dir(bio) == READ)
- raid10_read_request(mddev, bio, r10_bio);
- else {
+ ret = raid10_read_request(mddev, bio, r10_bio);
+ else
ret = raid10_write_request(mddev, bio, r10_bio);
- if (!ret)
- free_r10bio(r10_bio);
- }
+
+ if (!ret)
+ free_r10bio(r10_bio);
return ret;
}
@@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
int chunk_sects = chunk_mask + 1;
int sectors = bio_sectors(bio);
+ bool write = bio_data_dir(bio) == WRITE;
if (unlikely(bio->bi_opf & REQ_PREFLUSH)
&& md_flush_request(mddev, bio))
@@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sectors = chunk_sects -
(bio->bi_iter.bi_sector &
(chunk_sects - 1));
- if (!__make_request(mddev, bio, sectors))
+ if (!__make_request(mddev, bio, sectors) && write)
md_write_end(mddev);
/* In case raid10d snuck in to freeze_array */
@@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
rdev_dec_pending(rdev, mddev);
r10_bio->state = 0;
- raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
+ if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
+ free_r10bio(r10_bio);
+
/*
* allow_barrier after re-submit to ensure no sync io
* can be issued while regular io pending.
--
2.43.0
^ permalink raw reply related
* [PATCH v4 6/7] md/raid10: simplify write request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
raid10_write_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Move bio_wouldblock_error() handling to the callers of
regular_request_wait(), consolidate the write error paths, and free
r10_bio from a single location in __make_request() when
raid10_write_request() fails.
It remove redundant local copies of r10_bio->sectors and use a single
max_sectors variable throughout the function.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-7-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-7-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-7-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 58 ++++++++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 57813f249578..d94c1f28a6f6 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1123,18 +1123,16 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
struct bio *bio, sector_t sectors)
{
/* Bail out if REQ_NOWAIT is set for the bio */
- if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) {
- bio_wouldblock_error(bio);
+ if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT))
return false;
- }
+
while (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
bio->bi_iter.bi_sector < conf->reshape_progress &&
bio->bi_iter.bi_sector + sectors > conf->reshape_progress) {
allow_barrier(conf);
- if (bio->bi_opf & REQ_NOWAIT) {
- bio_wouldblock_error(bio);
+ if (bio->bi_opf & REQ_NOWAIT)
return false;
- }
+
mddev_add_trace_msg(conf->mddev, "raid10 wait reshape");
wait_event(conf->wait_barrier,
conf->reshape_progress <= bio->bi_iter.bi_sector ||
@@ -1192,6 +1190,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
}
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
+ bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return;
}
@@ -1354,8 +1353,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
{
struct r10conf *conf = mddev->private;
int i, k;
- sector_t sectors;
- int max_sectors;
+ int max_sectors = r10_bio->sectors;
+ bool nowait = bio->bi_opf & REQ_NOWAIT;
bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
@@ -1363,9 +1362,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
/* Bail out if REQ_NOWAIT is set for the bio */
- if (bio->bi_opf & REQ_NOWAIT) {
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
return false;
}
@@ -1375,28 +1373,25 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio_end_sector(bio)));
}
- sectors = r10_bio->sectors;
- if (!regular_request_wait(mddev, conf, bio, sectors)) {
- free_r10bio(r10_bio);
+ if (!regular_request_wait(mddev, conf, bio, max_sectors)) {
+ bio_wouldblock_error(bio);
return false;
}
if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
(mddev->reshape_backwards
? (bio->bi_iter.bi_sector < conf->reshape_safe &&
- bio->bi_iter.bi_sector + sectors > conf->reshape_progress)
- : (bio->bi_iter.bi_sector + sectors > conf->reshape_safe &&
+ bio->bi_iter.bi_sector + max_sectors > conf->reshape_progress)
+ : (bio->bi_iter.bi_sector + max_sectors > conf->reshape_safe &&
bio->bi_iter.bi_sector < conf->reshape_progress))) {
/* Need to update reshape_position in metadata */
mddev->reshape_position = conf->reshape_progress;
set_mask_bits(&mddev->sb_flags, 0,
BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
md_wakeup_thread(mddev->thread);
- if (bio->bi_opf & REQ_NOWAIT) {
- allow_barrier(conf);
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return false;
+ goto err_allow_barrier;
}
mddev_add_trace_msg(conf->mddev,
"raid10 wait reshape metadata");
@@ -1421,8 +1416,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
wait_blocked_dev(mddev, r10_bio);
- max_sectors = r10_bio->sectors;
-
for (i = 0; i < conf->copies; i++) {
int d = r10_bio->devs[i].devnum;
struct md_rdev *rdev, *rrdev;
@@ -1479,15 +1472,15 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
r10_bio->sectors = max_sectors;
if (r10_bio->sectors < bio_sectors(bio)) {
- if (atomic)
- goto err_handle;
+ if (atomic) {
+ bio_io_error(bio);
+ goto err_dec_pending;
+ }
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
}
@@ -1505,7 +1498,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
one_write_done(r10_bio);
return true;
-err_handle:
+err_dec_pending:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rdev = conf->mirrors[d].rdev;
@@ -1521,7 +1514,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
}
}
- raid_end_bio_io(r10_bio);
+err_allow_barrier:
+ allow_barrier(conf);
+
return false;
}
@@ -1546,8 +1541,11 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
ret = true;
if (bio_data_dir(bio) == READ)
raid10_read_request(mddev, bio, r10_bio);
- else
+ else {
ret = raid10_write_request(mddev, bio, r10_bio);
+ if (!ret)
+ free_r10bio(r10_bio);
+ }
return ret;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v4 5/7] md/raid10: replace wait loop with wait_event_idle()
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
The wait loop is equivalent to wait_event_idle() and can be simplified
by usaing it for improving readability.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-6-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-6-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-6-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 2574f60dd771..57813f249578 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1362,22 +1362,17 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
- DEFINE_WAIT(w);
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return false;
}
- for (;;) {
- prepare_to_wait(&conf->wait_barrier,
- &w, TASK_IDLE);
- if (!mddev->cluster_ops->area_resyncing(mddev, WRITE,
- bio->bi_iter.bi_sector, bio_end_sector(bio)))
- break;
- schedule();
- }
- finish_wait(&conf->wait_barrier, &w);
+
+ wait_event_idle(conf->wait_barrier,
+ !mddev->cluster_ops->area_resyncing(mddev, WRITE,
+ bio->bi_iter.bi_sector,
+ bio_end_sector(bio)));
}
sectors = r10_bio->sectors;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 4/7] md/raid10: remove unnecessary barrier around bio_submit_split_bioset()
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
raid10_write_request() drops the barrier before calling
bio_submit_split_bioset() and reacquires it afterwards. This is no
longer necessary because the split bio cannot re-enter
raid10_write_request() while the barrier is held.
The allow_barrier()/wait_barrier() pair was introduced by commit
e820d55cb99d ("md: fix raid10 hang issue caused by barrier") when
submit_flushes() called md_handle_request() directly, allowing re-entry
into raid10_write_request(). Since v5.2, submit_flushes() has instead
gone through submit_bio(), eliminating that recursion. submit_flushes()
was later removed entirely by commit b75197e86e6d ("md: Remove flush
handling").
Currently, raid10_write_request() is only entered from the bio
submission path, so the split bio submitted by bio_submit_split_bioset()
cannot recurse back into wait_barrier().
Remove the redundant allow_barrier()/wait_barrier() pair around
bio_submit_split_bioset().
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-5-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-5-abd.masalkhi@gmail.com/
Changes in v2:
- Expand the commit message to explain why the
allow_barrier()/wait_barrier() pair is no longer needed.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-5-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 3480fc7907f0..2574f60dd771 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1487,10 +1487,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (atomic)
goto err_handle;
- allow_barrier(conf);
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
- wait_barrier(conf, false);
if (!bio) {
set_bit(R10BIO_Returned, &r10_bio->state);
goto err_handle;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 3/7] md/raid10: consistently fail atomic writes that require splitting
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
RAID10 currently handles one badblock path explicitly by failing atomic
writes with EIO. However, another badblock path can also reduce the
writable range and force the bio through bio_submit_split_bioset(),
which implicitly completes the bio with EINVAL.
Fix this by handling atomic writes in the common split check. If RAID10
determines that an atomic write would require splitting, complete the
bio with EIO.
Fixes: a1d9b4fd42d9 ("md/raid10: Atomic write support")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-4-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-4-abd.masalkhi@gmail.com/
Changes in v2:
- Drop the early atomic write split check from raid10_write_request()
and rely on queue limits instead.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-4-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index bd322eccdc3f..3480fc7907f0 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1356,6 +1356,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
int i, k;
sector_t sectors;
int max_sectors;
+ bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
mddev->cluster_ops->area_resyncing(mddev, WRITE,
@@ -1464,16 +1465,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (is_bad) {
int good_sectors;
- /*
- * We cannot atomically write this, so just
- * error in that case. It could be possible to
- * atomically write other mirrors, but the
- * complexity of supporting that is not worth
- * the benefit.
- */
- if (bio->bi_opf & REQ_ATOMIC)
- goto err_handle;
-
good_sectors = first_bad - dev_sector;
if (good_sectors < max_sectors)
max_sectors = good_sectors;
@@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
r10_bio->sectors = max_sectors;
if (r10_bio->sectors < bio_sectors(bio)) {
+ if (atomic)
+ goto err_handle;
+
allow_barrier(conf);
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
--
2.43.0
^ permalink raw reply related
* [PATCH v4 2/7] md/raid1: restrict atomic write limits and handle runtime constraints
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
Restrict the RAID1 atomic write limits by setting chunk_sectors to
BARRIER_UNIT_SECTOR_SIZE so that atomic writes never straddle a barrier
unit.
A bio that passes block-layer validation may still become unserviceable
within RAID1 due to bad blocks or write-behind constraints. In the former
case, complete the bio with EIO. In the latter case, disable
write-behind rather than failing the bio with EIO.
Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
Reviewed-by: John Garry <john.g.garry@oracle.com>
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- Improve the commit message.
- Add Reviewed-by tag from John Garry.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-3-abd.masalkhi@gmail.com/
Changes in v3:
- Set chunk_sectors to BARRIER_UNIT_SECTOR_SIZE instead of setting
atomic_write_hw_unit_max.
- Avoid enabling write-behind when the atomic write exceeds the
write-behind limit.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-3-abd.masalkhi@gmail.com/
Changes in v2:
- Drop the early atomic write split check from raid1_write_request().
- Advertise the atomic write size limit via queue limits.
- Disable write-behind instead of failing atomic writes when the
BIO_MAX_VECS limit is encountered.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/
---
drivers/md/raid1.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index afe2ca96ad8c..6c8beca995e6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
int first_clone;
bool write_behind = false;
bool nowait = bio->bi_opf & REQ_NOWAIT;
+ bool atomic = bio->bi_opf & REQ_ATOMIC;
bool is_discard = op_is_discard(bio->bi_opf);
sector_t sector = bio->bi_iter.bi_sector;
@@ -1577,7 +1578,8 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
* write-mostly, which means we could allocate write behind
* bio later.
*/
- if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags))
+ if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags) &&
+ (!atomic || max_sectors <= BIO_MAX_VECS * (PAGE_SIZE >> 9)))
write_behind = true;
r1_bio->bios[i] = NULL;
@@ -1603,20 +1605,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
}
if (is_bad) {
int good_sectors;
-
- /*
- * We cannot atomically write this, so just
- * error in that case. It could be possible to
- * atomically write other mirrors, but the
- * complexity of supporting that is not worth
- * the benefit.
- */
- if (bio->bi_opf & REQ_ATOMIC) {
- bio->bi_status = BLK_STS_NOTSUPP;
- bio_endio(bio);
- goto err_dec_pending;
- }
-
good_sectors = first_bad - sector;
if (good_sectors < max_sectors)
max_sectors = good_sectors;
@@ -1636,7 +1624,13 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
if (write_behind && mddev->bitmap)
max_sectors = min_t(int, max_sectors,
BIO_MAX_VECS * (PAGE_SIZE >> 9));
+
if (max_sectors < bio_sectors(bio)) {
+ if (atomic) {
+ bio_io_error(bio);
+ goto err_dec_pending;
+ }
+
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
if (!bio)
@@ -3228,6 +3222,7 @@ static int raid1_set_limits(struct mddev *mddev)
md_init_stacking_limits(&lim);
lim.max_write_zeroes_sectors = 0;
lim.max_hw_wzeroes_unmap_sectors = 0;
+ lim.chunk_sectors = BARRIER_UNIT_SECTOR_SIZE;
lim.logical_block_size = mddev->logical_block_size;
lim.features |= BLK_FEAT_ATOMIC_WRITES;
lim.features |= BLK_FEAT_PCI_P2PDMA;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, sashiko-bot
In-Reply-To: <20260710101521.1714-1-abd.masalkhi@gmail.com>
When raid10_write_request() fails because REQ_NOWAIT is set, the
allocated r10_bio is not freed before returning, resulting in a memory
leak. Free r10_bio before returning from the REQ_NOWAIT error paths.
Fixes: c9aa889b035f ("md: raid10 add nowait support")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v4:
- No changes.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-2-abd.masalkhi@gmail.com/
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-2-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-2-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0a3cfdd3f5df..bd322eccdc3f 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
+ free_r10bio(r10_bio);
return false;
}
for (;;) {
@@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (bio->bi_opf & REQ_NOWAIT) {
allow_barrier(conf);
bio_wouldblock_error(bio);
+ free_r10bio(r10_bio);
return false;
}
mddev_add_trace_msg(conf->mddev,
--
2.43.0
^ permalink raw reply related
* [PATCH v4 0/7] md/raid10: fixes, atomic write handling, and error-path cleanup
From: Abd-Alrhman Masalkhi @ 2026-07-10 10:15 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
Hi,
This v4 of series contains a mix of bug fixes and cleanups for RAID10,
along with a related atomic write fix for RAID1.
Changes in v4:
- Improve the commit message of patch 2.
- Add Reviewed-by tag from John Garry to patch 2.
- Link to v3: https://lore.kernel.org/linux-raid/20260708101341.473750-1-abd.masalkhi@gmail.com/
Changes in v3:
- Set chunk_sectors to BARRIER_UNIT_SECTOR_SIZE instead of setting
atomic_write_hw_unit_max.
- Avoid enabling write-behind when the atomic write exceeds the
write-behind limit.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-1-abd.masalkhi@gmail.com/
Changes in v2:
- Expand the commit message to explain why the
allow_barrier()/wait_barrier() pair is no longer needed.
- Drop the early atomic write split check from raid1_write_request().
- Advertise the atomic write size limit via queue limits.
- Disable write-behind instead of failing atomic writes when the
BIO_MAX_VECS limit is encountered.
- Drop the early atomic write split check from raid10_write_request()
and rely on queue limits instead.
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-1-abd.masalkhi@gmail.com/
Thanks,
Abd-alrhman,
Abd-Alrhman Masalkhi (7):
md/raid10: fix r10bio leak in raid10_write_request() error paths
md/raid1: restrict atomic write limits and handle runtime constraints
md/raid10: consistently fail atomic writes that require splitting
md/raid10: remove unnecessary barrier around bio_submit_split_bioset()
md/raid10: replace wait loop with wait_event_idle()
md/raid10: simplify write request error handling
md/raid10: simplify read request error handling
drivers/md/raid1.c | 25 ++++------
drivers/md/raid10.c | 118 +++++++++++++++++++++-----------------------
2 files changed, 65 insertions(+), 78 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH v3 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: Abd-Alrhman Masalkhi @ 2026-07-10 9:42 UTC (permalink / raw)
To: John Garry, song, yukuai, magiclinan, xiao, axboe, vverma,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <350a2662-7de0-45e6-a905-76dbd0a99e98@oracle.com>
On Thu, Jul 09, 2026 at 13:11 +0100, John Garry wrote:
> On 08/07/2026 11:13, Abd-Alrhman Masalkhi wrote:
>> Atomic writes in RAID1 must fit within a single barrier unit. Set
>> chunk_sectors to BARRIER_UNIT_SECTOR_SIZE so that bios which would cross
>> a barrier-unit boundary are rejected by the block layer before reaching
>> MD.
>
> More accurate would be to say that we restrict the atomic write limits
> so that we never produce a write which straddles BARRIER_UNIT_SECTOR_SIZE.
>
Yes, that is more accurate. I'll update the commit message accordingly.
Thanks for the suggestion.
>>
>> A bio that passes block-layer validation may still become unserviceable
>> within RAID1 due to bad blocks or write-behind constraints. In the former
>> case, complete the bio with EIO. In the latter case, disable
>> write-behind rather than failing the bio with EIO.
>>
>> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
>> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> Reviewed-by: John Garry <john.g.garry@oracle.com>
>
>
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] md: recheck spare changes before starting sync
From: yu kuai @ 2026-07-10 7:50 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, magiclinan, xiao, abd.masalkhi
Cc: linux-raid, linux-kernel, yu kuai
In-Reply-To: <20260708112003.474537-1-abd.masalkhi@gmail.com>
在 2026/7/8 19:20, Abd-Alrhman Masalkhi 写道:
> remove_spares() and remove_and_add_spares() modify the array's rdev
> configuration. These operations are only safe after the array has been
> suspended.
>
> md_start_sync() checks whether spare configuration changes are needed
> before taking reconfig_mutex. However, the rdev state can change before
> the mutex is acquired, so the initial check can become stale. In that
> case, md_choose_sync_action() may remove or replace rdevs while normal
> I/O is still accessing them.
>
> The race can occur as follows:
>
> raid10d Worker Normal IO
> ____________ _______________________ ______________________
>
> raid10_write_request()
> wait_blocked_dev()
> set Blocked
> set Faulty
> Skip Faulty rdev
> rrdev->nr_pending++
> .repl_bio = bio
> removeable_rdev = false .
> array not suspended .
> lock mddev goto err_handle
> lock mddev (wait)
> .
> update sb .
> clear Blocked .
> .
> unlock mddev .
> lock mddev (acquires)
> remove_spares()
> removeable_rdev = true
>
> raid10_remove_disk()
> rdev = replacement
> replacement = NULL
> rdev_dec_pending(NULL)
> unlock mddev (NULL)->nr_pending--
>
> In this case, rdev_dec_pending() is called with a NULL pointer,
> resulting in a NULL pointer dereference when attempting to decrement
> nr_pending.
>
> Fix this by suspending the array when spare configuration changes are
> needed, including for non-read-write arrays, and checking again after
> taking reconfig_mutex. If the array was not already suspended and a
> change is now needed, release the mutex, suspend the array, and
> reacquire the mutex before continuing.
>
> Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
> Reported-by: sashiko-bot<sashiko-bot@kernel.org>
> Closes:https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
> Signed-off-by: Abd-Alrhman Masalkhi<abd.masalkhi@gmail.com>
> ---
> Changes in v2:
> - Recheck whether spare configuration changes are needed after taking
> reconfig_mutex, and suspend the array before continuing if necessary.
> - Account for read only arrays, where reshape_position may not be
> MaxSector even though md_start_sync() can still modify the spare
> configuration.
> - Link to v1:https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
> ---
> drivers/md/md.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
Reviewed-by: Yu Kuai <yukuai@fygo.io>
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: validate journal checksum slots during recovery
From: yu kuai @ 2026-07-10 7:37 UTC (permalink / raw)
To: Guangshuo Li, Song Liu, Li Nan, Xiao Ni, Junrui Luo, linux-raid,
linux-kernel, yu kuai
In-Reply-To: <20260708133534.770770-1-lgs201920130244@gmail.com>
Hi,
在 2026/7/8 21:35, Guangshuo Li 写道:
> The change referenced by the Fixes tag added payload length validation
> before accessing journal metadata during raid5-cache recovery.
>
> However, the DATA and PARITY payload length is computed from the on-disk
> size field without ensuring that the checksum slots read later are
> actually present. struct r5l_payload_data_parity ends with a flexible
> checksum array, so sizeof(struct r5l_payload_data_parity) does not cover
> any checksum entries.
>
> A corrupted journal can set a DATA payload size smaller than one page.
> The computed checksum count then becomes zero and the payload length only
> covers the fixed header, but recovery still reads checksum[0]. For RAID6
> PARITY payloads, recovery also reads checksum[1], so the payload must
> cover two checksum entries.
>
> Make the validated DATA and PARITY payload length include the checksum
> entries that recovery may read. Also make sure fixed payload headers are
> present before reading their fields.
>
> Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")
Does the problem this patch fixes introduced by above fix tag? Or it's just a
new validation, if so please replace with a real fix tag.
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> drivers/md/raid5-cache.c | 108 +++++++++++++++++++++++++++++----------
> 1 file changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21f..4aef692182b4 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -1980,6 +1980,29 @@ r5l_recovery_verify_data_checksum(struct r5l_log *log,
> return (le32_to_cpu(log_checksum) == checksum) ? 0 : -EINVAL;
> }
>
> +static sector_t r5l_recovery_payload_data_parity_len(struct r5conf *conf,
> + const struct r5l_payload_data_parity *payload, bool parity)
> +{
> + unsigned int nr_csum;
> + unsigned int min_csum = 1;
> +
> + /*
> + * The payload size determines how many checksum entries are stored,
> + * but recovery always reads checksum[0]. For RAID6 parity payloads
> + * it also reads checksum[1] for Q. Make the validated payload length
> + * cover every checksum entry that will be read below.
> + */
> + nr_csum = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
> +
> + if (parity && conf->max_degraded == 2)
> + min_csum = 2;
> + if (nr_csum < min_csum)
> + nr_csum = min_csum;
> +
> + return sizeof(*payload) + (sector_t)sizeof(__le32) * nr_csum;
Please use struct_size()
> +}
> +
> +
Two blank line here. Please run checkpatch before submission.
> /*
> * before loading data to stripe cache, we need verify checksum for all data,
> * if there is mismatch for any data page, we drop all data in the mata block
> @@ -1992,6 +2015,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> struct r5conf *conf = mddev->private;
> struct r5l_meta_block *mb = page_address(ctx->meta_page);
> sector_t mb_offset = sizeof(struct r5l_meta_block);
> + sector_t meta_size = le32_to_cpu(mb->meta_size);
> sector_t log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
> struct page *page;
> struct r5l_payload_data_parity *payload;
> @@ -2001,28 +2025,42 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> if (!page)
> return -ENOMEM;
>
> - while (mb_offset < le32_to_cpu(mb->meta_size)) {
> + while (mb_offset < meta_size) {
> sector_t payload_len;
> + u16 type;
>
> payload = (void *)mb + mb_offset;
> payload_flush = (void *)mb + mb_offset;
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (mb_offset + sizeof(payload->header) > meta_size)
> + goto mismatch;
> +
> + type = le16_to_cpu(payload->header.type);
> +
> + if (type == R5LOG_PAYLOAD_DATA) {
> + if (mb_offset + sizeof(*payload) > meta_size)
> goto mismatch;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf,
> + payload,
> + false);
> + if (payload_len > meta_size - mb_offset)
> + goto mismatch;
> +
> if (r5l_recovery_verify_data_checksum(
> log, ctx, page, log_offset,
> payload->checksum[0]) < 0)
> goto mismatch;
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + } else if (type == R5LOG_PAYLOAD_PARITY) {
> + if (mb_offset + sizeof(*payload) > meta_size)
> goto mismatch;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf,
> + payload,
> + true);
> + if (payload_len > meta_size - mb_offset)
> + goto mismatch;
> +
> if (r5l_recovery_verify_data_checksum(
> log, ctx, page, log_offset,
> payload->checksum[0]) < 0)
> @@ -2034,15 +2072,18 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> BLOCK_SECTORS),
> payload->checksum[1]) < 0)
> goto mismatch;
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> + } else if (type == R5LOG_PAYLOAD_FLUSH) {
> + if (mb_offset + sizeof(*payload_flush) > meta_size)
> + goto mismatch;
> +
> payload_len = sizeof(struct r5l_payload_flush) +
> (sector_t)le32_to_cpu(payload_flush->size);
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (payload_len > meta_size - mb_offset)
> goto mismatch;
> } else /* not R5LOG_PAYLOAD_DATA/PARITY/FLUSH */
> goto mismatch;
>
> - if (le16_to_cpu(payload->header.type) != R5LOG_PAYLOAD_FLUSH) {
> + if (type != R5LOG_PAYLOAD_FLUSH) {
> log_offset = r5l_ring_add(log, log_offset,
> le32_to_cpu(payload->size));
> }
> @@ -2075,7 +2116,8 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> struct r5l_meta_block *mb;
> struct r5l_payload_data_parity *payload;
> struct r5l_payload_flush *payload_flush;
> - int mb_offset;
> + sector_t mb_offset;
> + sector_t meta_size;
> sector_t log_offset;
> sector_t stripe_sect;
> struct stripe_head *sh;
> @@ -2094,22 +2136,31 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>
> mb = page_address(ctx->meta_page);
> mb_offset = sizeof(struct r5l_meta_block);
> + meta_size = le32_to_cpu(mb->meta_size);
> log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
>
> - while (mb_offset < le32_to_cpu(mb->meta_size)) {
> + while (mb_offset < meta_size) {
> sector_t payload_len;
> + u16 type;
> int dd;
>
> payload = (void *)mb + mb_offset;
> payload_flush = (void *)mb + mb_offset;
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> + if (mb_offset + sizeof(payload->header) > meta_size)
> + return -EINVAL;
> +
> + type = le16_to_cpu(payload->header.type);
> +
> + if (type == R5LOG_PAYLOAD_FLUSH) {
> int i, count;
>
> + if (mb_offset + sizeof(*payload_flush) > meta_size)
> + return -EINVAL;
> +
> payload_len = sizeof(struct r5l_payload_flush) +
> (sector_t)le32_to_cpu(payload_flush->size);
> - if (mb_offset + payload_len >
> - le32_to_cpu(mb->meta_size))
> + if (payload_len > meta_size - mb_offset)
> return -EINVAL;
>
> count = le32_to_cpu(payload_flush->size) / sizeof(__le64);
> @@ -2130,13 +2181,18 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> }
>
> /* DATA or PARITY payload */
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (type != R5LOG_PAYLOAD_DATA && type != R5LOG_PAYLOAD_PARITY)
> + return -EINVAL;
> +
> + if (mb_offset + sizeof(*payload) > meta_size)
> + return -EINVAL;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf, payload,
> + type == R5LOG_PAYLOAD_PARITY);
> + if (payload_len > meta_size - mb_offset)
> return -EINVAL;
>
> - stripe_sect = (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) ?
> + stripe_sect = (type == R5LOG_PAYLOAD_DATA) ?
> raid5_compute_sector(
> conf, le64_to_cpu(payload->location), 0, &dd,
> NULL)
> @@ -2183,7 +2239,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> list_add_tail(&sh->lru, cached_stripe_list);
> }
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> + if (type == R5LOG_PAYLOAD_DATA) {
> if (!test_bit(STRIPE_R5C_CACHING, &sh->state) &&
> test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags)) {
> r5l_recovery_replay_one_stripe(conf, sh, ctx);
> @@ -2191,7 +2247,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> }
> r5l_recovery_load_data(log, sh, ctx, payload,
> log_offset);
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
> + } else if (type == R5LOG_PAYLOAD_PARITY)
> r5l_recovery_load_parity(log, sh, ctx, payload,
> log_offset);
> else
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v3 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: John Garry @ 2026-07-09 12:11 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, axboe,
vverma, martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260708101341.473750-3-abd.masalkhi@gmail.com>
On 08/07/2026 11:13, Abd-Alrhman Masalkhi wrote:
> Atomic writes in RAID1 must fit within a single barrier unit. Set
> chunk_sectors to BARRIER_UNIT_SECTOR_SIZE so that bios which would cross
> a barrier-unit boundary are rejected by the block layer before reaching
> MD.
More accurate would be to say that we restrict the atomic write limits
so that we never produce a write which straddles BARRIER_UNIT_SECTOR_SIZE.
>
> A bio that passes block-layer validation may still become unserviceable
> within RAID1 due to bad blocks or write-behind constraints. In the former
> case, complete the bio with EIO. In the latter case, disable
> write-behind rather than failing the bio with EIO.
>
> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
^ permalink raw reply
* [PATCH] md/raid5: validate journal checksum slots during recovery
From: Guangshuo Li @ 2026-07-08 13:35 UTC (permalink / raw)
To: Song Liu, Yu Kuai, Li Nan, Xiao Ni, Junrui Luo, linux-raid,
linux-kernel
Cc: Guangshuo Li
The change referenced by the Fixes tag added payload length validation
before accessing journal metadata during raid5-cache recovery.
However, the DATA and PARITY payload length is computed from the on-disk
size field without ensuring that the checksum slots read later are
actually present. struct r5l_payload_data_parity ends with a flexible
checksum array, so sizeof(struct r5l_payload_data_parity) does not cover
any checksum entries.
A corrupted journal can set a DATA payload size smaller than one page.
The computed checksum count then becomes zero and the payload length only
covers the fixed header, but recovery still reads checksum[0]. For RAID6
PARITY payloads, recovery also reads checksum[1], so the payload must
cover two checksum entries.
Make the validated DATA and PARITY payload length include the checksum
entries that recovery may read. Also make sure fixed payload headers are
present before reading their fields.
Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
drivers/md/raid5-cache.c | 108 +++++++++++++++++++++++++++++----------
1 file changed, 82 insertions(+), 26 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 7b7546bfa21f..4aef692182b4 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1980,6 +1980,29 @@ r5l_recovery_verify_data_checksum(struct r5l_log *log,
return (le32_to_cpu(log_checksum) == checksum) ? 0 : -EINVAL;
}
+static sector_t r5l_recovery_payload_data_parity_len(struct r5conf *conf,
+ const struct r5l_payload_data_parity *payload, bool parity)
+{
+ unsigned int nr_csum;
+ unsigned int min_csum = 1;
+
+ /*
+ * The payload size determines how many checksum entries are stored,
+ * but recovery always reads checksum[0]. For RAID6 parity payloads
+ * it also reads checksum[1] for Q. Make the validated payload length
+ * cover every checksum entry that will be read below.
+ */
+ nr_csum = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
+
+ if (parity && conf->max_degraded == 2)
+ min_csum = 2;
+ if (nr_csum < min_csum)
+ nr_csum = min_csum;
+
+ return sizeof(*payload) + (sector_t)sizeof(__le32) * nr_csum;
+}
+
+
/*
* before loading data to stripe cache, we need verify checksum for all data,
* if there is mismatch for any data page, we drop all data in the mata block
@@ -1992,6 +2015,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
struct r5conf *conf = mddev->private;
struct r5l_meta_block *mb = page_address(ctx->meta_page);
sector_t mb_offset = sizeof(struct r5l_meta_block);
+ sector_t meta_size = le32_to_cpu(mb->meta_size);
sector_t log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
struct page *page;
struct r5l_payload_data_parity *payload;
@@ -2001,28 +2025,42 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
if (!page)
return -ENOMEM;
- while (mb_offset < le32_to_cpu(mb->meta_size)) {
+ while (mb_offset < meta_size) {
sector_t payload_len;
+ u16 type;
payload = (void *)mb + mb_offset;
payload_flush = (void *)mb + mb_offset;
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (mb_offset + sizeof(payload->header) > meta_size)
+ goto mismatch;
+
+ type = le16_to_cpu(payload->header.type);
+
+ if (type == R5LOG_PAYLOAD_DATA) {
+ if (mb_offset + sizeof(*payload) > meta_size)
goto mismatch;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf,
+ payload,
+ false);
+ if (payload_len > meta_size - mb_offset)
+ goto mismatch;
+
if (r5l_recovery_verify_data_checksum(
log, ctx, page, log_offset,
payload->checksum[0]) < 0)
goto mismatch;
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ } else if (type == R5LOG_PAYLOAD_PARITY) {
+ if (mb_offset + sizeof(*payload) > meta_size)
goto mismatch;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf,
+ payload,
+ true);
+ if (payload_len > meta_size - mb_offset)
+ goto mismatch;
+
if (r5l_recovery_verify_data_checksum(
log, ctx, page, log_offset,
payload->checksum[0]) < 0)
@@ -2034,15 +2072,18 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
BLOCK_SECTORS),
payload->checksum[1]) < 0)
goto mismatch;
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
+ } else if (type == R5LOG_PAYLOAD_FLUSH) {
+ if (mb_offset + sizeof(*payload_flush) > meta_size)
+ goto mismatch;
+
payload_len = sizeof(struct r5l_payload_flush) +
(sector_t)le32_to_cpu(payload_flush->size);
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (payload_len > meta_size - mb_offset)
goto mismatch;
} else /* not R5LOG_PAYLOAD_DATA/PARITY/FLUSH */
goto mismatch;
- if (le16_to_cpu(payload->header.type) != R5LOG_PAYLOAD_FLUSH) {
+ if (type != R5LOG_PAYLOAD_FLUSH) {
log_offset = r5l_ring_add(log, log_offset,
le32_to_cpu(payload->size));
}
@@ -2075,7 +2116,8 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
struct r5l_meta_block *mb;
struct r5l_payload_data_parity *payload;
struct r5l_payload_flush *payload_flush;
- int mb_offset;
+ sector_t mb_offset;
+ sector_t meta_size;
sector_t log_offset;
sector_t stripe_sect;
struct stripe_head *sh;
@@ -2094,22 +2136,31 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
mb = page_address(ctx->meta_page);
mb_offset = sizeof(struct r5l_meta_block);
+ meta_size = le32_to_cpu(mb->meta_size);
log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
- while (mb_offset < le32_to_cpu(mb->meta_size)) {
+ while (mb_offset < meta_size) {
sector_t payload_len;
+ u16 type;
int dd;
payload = (void *)mb + mb_offset;
payload_flush = (void *)mb + mb_offset;
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
+ if (mb_offset + sizeof(payload->header) > meta_size)
+ return -EINVAL;
+
+ type = le16_to_cpu(payload->header.type);
+
+ if (type == R5LOG_PAYLOAD_FLUSH) {
int i, count;
+ if (mb_offset + sizeof(*payload_flush) > meta_size)
+ return -EINVAL;
+
payload_len = sizeof(struct r5l_payload_flush) +
(sector_t)le32_to_cpu(payload_flush->size);
- if (mb_offset + payload_len >
- le32_to_cpu(mb->meta_size))
+ if (payload_len > meta_size - mb_offset)
return -EINVAL;
count = le32_to_cpu(payload_flush->size) / sizeof(__le64);
@@ -2130,13 +2181,18 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
}
/* DATA or PARITY payload */
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (type != R5LOG_PAYLOAD_DATA && type != R5LOG_PAYLOAD_PARITY)
+ return -EINVAL;
+
+ if (mb_offset + sizeof(*payload) > meta_size)
+ return -EINVAL;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf, payload,
+ type == R5LOG_PAYLOAD_PARITY);
+ if (payload_len > meta_size - mb_offset)
return -EINVAL;
- stripe_sect = (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) ?
+ stripe_sect = (type == R5LOG_PAYLOAD_DATA) ?
raid5_compute_sector(
conf, le64_to_cpu(payload->location), 0, &dd,
NULL)
@@ -2183,7 +2239,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
list_add_tail(&sh->lru, cached_stripe_list);
}
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
+ if (type == R5LOG_PAYLOAD_DATA) {
if (!test_bit(STRIPE_R5C_CACHING, &sh->state) &&
test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags)) {
r5l_recovery_replay_one_stripe(conf, sh, ctx);
@@ -2191,7 +2247,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
}
r5l_recovery_load_data(log, sh, ctx, payload,
log_offset);
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
+ } else if (type == R5LOG_PAYLOAD_PARITY)
r5l_recovery_load_parity(log, sh, ctx, payload,
log_offset);
else
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] md: recheck spare changes before starting sync
From: Abd-Alrhman Masalkhi @ 2026-07-08 12:42 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260708113713.B77311F00ACF@smtp.kernel.org>
Hi Kuai,
On Wed, Jul 08, 2026 at 11:37 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
> --
>
> --- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
> commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md: recheck spare changes before starting sync
>
> This patch aims to fix a race condition where spare configurations are
> modified without suspending the array. It adds a recheck for spare
> configuration changes after acquiring the reconfig_mutex to avoid operating
> on stale states.
>
> Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)
>
> [ ... ]
>
>> mddev_lock_nointr(mddev);
>> +
>> + /*
>> + * The spare configuration can change before reconfig_mutex is acquired.
>> + * Recheck while holding the lock and suspend if needed.
>> + */
>> + if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
>> + md_spares_need_change(mddev)) {
>> + mddev_unlock(mddev);
>> + mddev_suspend_and_lock_nointr(mddev);
>> + suspend = true;
>> + }
>> +
>
> [Severity: High]
> This is a pre-existing issue, but does this recheck completely close the race
> window?
>
> If a faulty drive has pending I/O, md_spares_need_change() will evaluate
> rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
> greater than zero, md_spares_need_change() returns false. This causes both
> the initial check and this new recheck to bypass suspension.
>
> If an asynchronous I/O completion happens immediately after this recheck,
> nr_pending can decrement to 0. When md_start_sync() continues and eventually
> calls remove_spares(), it will re-evaluate rdev_removeable(), find
> nr_pending == 0, and remove the disk. This nullifies the disk pointer.
>
Shouldn't the Blocked flag guard against this case? It is set before the
Faulty bit and before nr_pending is decremented, and it should only be
cleared while holding reconfig_mutex, or am I missing something?
> Because the array was never suspended, couldn't concurrent I/O re-read the
> nullified pointer and crash? For example, in drivers/md/raid10.c:
>
> err_handle() {
> ...
> struct md_rdev *rrdev = conf->mirrors[d].replacement;
> ...
> if (r10_bio->devs[k].repl_bio) {
> rdev_dec_pending(rrdev, mddev);
> r10_bio->devs[k].repl_bio = NULL;
> }
> }
>
> If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
> will dereference it.
>
> Could remove_spares() be restricted to only remove drives if the array is
> actually suspended, or should suspension be enforced earlier regardless of
> nr_pending?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] md: recheck spare changes before starting sync
From: sashiko-bot @ 2026-07-08 11:37 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708112003.474537-1-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
--
--- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md: recheck spare changes before starting sync
This patch aims to fix a race condition where spare configurations are
modified without suspending the array. It adds a recheck for spare
configuration changes after acquiring the reconfig_mutex to avoid operating
on stale states.
Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)
[ ... ]
> mddev_lock_nointr(mddev);
> +
> + /*
> + * The spare configuration can change before reconfig_mutex is acquired.
> + * Recheck while holding the lock and suspend if needed.
> + */
> + if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
> + md_spares_need_change(mddev)) {
> + mddev_unlock(mddev);
> + mddev_suspend_and_lock_nointr(mddev);
> + suspend = true;
> + }
> +
[Severity: High]
This is a pre-existing issue, but does this recheck completely close the race
window?
If a faulty drive has pending I/O, md_spares_need_change() will evaluate
rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
greater than zero, md_spares_need_change() returns false. This causes both
the initial check and this new recheck to bypass suspension.
If an asynchronous I/O completion happens immediately after this recheck,
nr_pending can decrement to 0. When md_start_sync() continues and eventually
calls remove_spares(), it will re-evaluate rdev_removeable(), find
nr_pending == 0, and remove the disk. This nullifies the disk pointer.
Because the array was never suspended, couldn't concurrent I/O re-read the
nullified pointer and crash? For example, in drivers/md/raid10.c:
err_handle() {
...
struct md_rdev *rrdev = conf->mirrors[d].replacement;
...
if (r10_bio->devs[k].repl_bio) {
rdev_dec_pending(rrdev, mddev);
r10_bio->devs[k].repl_bio = NULL;
}
}
If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
will dereference it.
Could remove_spares() be restricted to only remove drives if the array is
actually suspended, or should suspension be enforced earlier regardless of
nr_pending?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* [PATCH v2] md: recheck spare changes before starting sync
From: Abd-Alrhman Masalkhi @ 2026-07-08 11:20 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, abd.masalkhi
Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi, sashiko-bot
remove_spares() and remove_and_add_spares() modify the array's rdev
configuration. These operations are only safe after the array has been
suspended.
md_start_sync() checks whether spare configuration changes are needed
before taking reconfig_mutex. However, the rdev state can change before
the mutex is acquired, so the initial check can become stale. In that
case, md_choose_sync_action() may remove or replace rdevs while normal
I/O is still accessing them.
The race can occur as follows:
raid10d Worker Normal IO
____________ _______________________ ______________________
raid10_write_request()
wait_blocked_dev()
set Blocked
set Faulty
Skip Faulty rdev
rrdev->nr_pending++
.repl_bio = bio
removeable_rdev = false .
array not suspended .
lock mddev goto err_handle
lock mddev (wait)
.
update sb .
clear Blocked .
.
unlock mddev .
lock mddev (acquires)
remove_spares()
removeable_rdev = true
raid10_remove_disk()
rdev = replacement
replacement = NULL
rdev_dec_pending(NULL)
unlock mddev (NULL)->nr_pending--
In this case, rdev_dec_pending() is called with a NULL pointer,
resulting in a NULL pointer dereference when attempting to decrement
nr_pending.
Fix this by suspending the array when spare configuration changes are
needed, including for non-read-write arrays, and checking again after
taking reconfig_mutex. If the array was not already suspended and a
change is now needed, release the mutex, suspend the array, and
reacquire the mutex before continuing.
Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
- Recheck whether spare configuration changes are needed after taking
reconfig_mutex, and suspend the array before continuing if necessary.
- Account for read only arrays, where reshape_position may not be
MaxSector even though md_start_sync() can still modify the spare
configuration.
- Link to v1: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
---
drivers/md/md.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 66a41d482e59..c3fc47a82415 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10174,13 +10174,25 @@ static void md_start_sync(struct work_struct *ws)
* If reshape is still in progress, spares won't be added or removed
* from conf until reshape is done.
*/
- if (mddev->reshape_position == MaxSector &&
+ if ((mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
md_spares_need_change(mddev)) {
suspend = true;
mddev_suspend(mddev, false);
}
mddev_lock_nointr(mddev);
+
+ /*
+ * The spare configuration can change before reconfig_mutex is acquired.
+ * Recheck while holding the lock and suspend if needed.
+ */
+ if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
+ md_spares_need_change(mddev)) {
+ mddev_unlock(mddev);
+ mddev_suspend_and_lock_nointr(mddev);
+ suspend = true;
+ }
+
if (!md_is_rdwr(mddev)) {
/*
* On a read-only array we can:
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: sashiko-bot @ 2026-07-08 10:33 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708101341.473750-2-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] REQ_NOWAIT bios can block waiting for blocked devices in wait_blocked_dev(), violating non-blocking semantics.
- [High] REQ_NOWAIT bios can stall during memory allocation due to hardcoded GFP_NOIO usage.
- [High] REQ_NOWAIT bios split by bio_submit_split_bioset block waiting for the barrier.
--
--- Patch [1]: [PATCH v3 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths ---
commit 3eda7e2f9e31ead6359429f927a806f3d762dc4b
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: fix r10bio leak in raid10_write_request() error paths
This commit fixes a memory leak in raid10_write_request() by freeing the
allocated r10_bio when a write request fails due to REQ_NOWAIT being set.
Closes: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-raid/20260628142420.1051027-2-abd.masalkhi@gmail.com/
Link: https://lore.kernel.org/linux-raid/20260623072456.333437-2-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..bd322eccdc3ff 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> /* Bail out if REQ_NOWAIT is set for the bio */
> if (bio->bi_opf & REQ_NOWAIT) {
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
> for (;;) {
[ ... ]
> @@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> if (bio->bi_opf & REQ_NOWAIT) {
> allow_barrier(conf);
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[Severity: High]
This is a pre-existing issue, but since this patch improves REQ_NOWAIT
handling, can REQ_NOWAIT bios still block waiting for blocked devices?
In raid10_write_request(), wait_blocked_dev() is called unconditionally:
wait_blocked_dev(mddev, r10_bio);
If an rdev is blocked, wait_blocked_dev() calls md_wait_for_blocked_rdev(),
which waits up to 5 seconds via wait_event_timeout(). Should this path check
if the bio is REQ_NOWAIT to avoid stalling the submission thread?
[Severity: High]
This is another pre-existing issue, but can REQ_NOWAIT bios stall during memory
allocation due to hardcoded GFP_NOIO usage?
In __make_request(), the initial allocation uses GFP_NOIO:
r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
And in raid10_write_one_disk(), the child bio allocation also uses GFP_NOIO:
mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO, &mddev->bio_set);
Under memory pressure, GFP_NOIO will trigger direct reclaim and wait for
mempool replenishment. Should these allocations be attempted with GFP_NOWAIT
and aborted with BLK_STS_AGAIN for REQ_NOWAIT bios?
[Severity: High]
This is also a pre-existing issue, but do REQ_NOWAIT bios that are split block
waiting for the barrier?
If a REQ_NOWAIT bio spans multiple chunks, it can be split. The wait_barrier()
call uses a hardcoded false for the nowait parameter:
wait_barrier(conf, false);
This will block if the array barrier is raised. Since returning BLK_STS_AGAIN
is no longer viable once the first half is submitted, is there a way to prevent
stalling REQ_NOWAIT callers on split bios?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708101341.473750-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting
From: sashiko-bot @ 2026-07-08 10:31 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708101341.473750-4-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] A concurrent promotion or removal of a disk causes a NULL pointer dereference in the err_handle cleanup path.
--
--- Patch [3]: [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting ---
commit 39f996c40527039ea2fa463cb02f7e201f0a4e7d
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: consistently fail atomic writes that require splitting
RAID10 currently handles one badblock path explicitly by failing atomic
writes with EIO. However, another badblock path can also reduce the
writable range and force the bio through bio_submit_split_bioset(),
which implicitly completes the bio with EINVAL. This patch explicitly
fails atomic writes in the common split check instead.
Link: https://lore.kernel.org/linux-raid/20260628142420.1051027-4-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index bd322eccdc3ff..3480fc7907f01 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> r10_bio->sectors = max_sectors;
>
> if (r10_bio->sectors < bio_sectors(bio)) {
> + if (atomic)
> + goto err_handle;
> +
[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if a concurrent disk removal or promotion occurs?
In the err_handle cleanup path, the replacement pointer is locklessly
re-read from the array configuration:
err_handle:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rdev = conf->mirrors[d].rdev;
struct md_rdev *rrdev = conf->mirrors[d].replacement;
If raid10_write_request() executes without holding rcu_read_lock() or
reconfig_mutex (since it can sleep), it might increment nr_pending on the
active replacement disk and set r10_bio->devs[i].repl_bio = bio.
Concurrently, if raid10_remove_disk() promotes the replacement disk, it sets
conf->mirrors[d].replacement = NULL.
When this goto is hit because an atomic write needs splitting, the lockless
re-read will fetch NULL for rrdev. Since r10_bio->devs[k].repl_bio is
non-NULL, the code calls rdev_dec_pending(rrdev, mddev), which will
dereference the NULL pointer.
> allow_barrier(conf);
> bio = bio_submit_split_bioset(bio, r10_bio->sectors,
> &conf->bio_split);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708101341.473750-1-abd.masalkhi@gmail.com?part=3
^ permalink raw reply
* [PATCH v3 7/7] md/raid10: simplify read request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
raid10_read_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Make raid10_read_request() return a status to its callers, consolidate
the read error paths, and free r10_bio from a single location in the
callers. Since the callers allocate r10_bio, they should also be
responsible for freeing it when the request fails.
This makes the read path follow the same ownership model as the write
path and simplifies the error handling flow.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/
Changes in v2:
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d94c1f28a6f6..01162c483644 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
return true;
}
-static void raid10_read_request(struct mddev *mddev, struct bio *bio,
+static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
struct r10bio *r10_bio)
{
struct r10conf *conf = mddev->private;
@@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return;
+ return false;
}
rdev = read_balance(conf, r10_bio, &max_sectors);
@@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
mdname(mddev), b,
(unsigned long long)r10_bio->sector);
}
- raid_end_bio_io(r10_bio);
- return;
+ bio_io_error(bio);
+ goto err_allow_barrier;
}
if (err_rdev)
pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
@@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
wait_barrier(conf, false);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
r10_bio->sectors = max_sectors;
@@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
read_bio->bi_private = r10_bio;
mddev_trace_remap(mddev, read_bio, r10_bio->sector);
submit_bio_noacct(read_bio);
- return;
-err_handle:
+
+ return true;
+
+err_dec_pending:
atomic_dec(&rdev->nr_pending);
- raid_end_bio_io(r10_bio);
+
+err_allow_barrier:
+ allow_barrier(conf);
+
+ return false;
}
static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
@@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
conf->geo.raid_disks);
- ret = true;
if (bio_data_dir(bio) == READ)
- raid10_read_request(mddev, bio, r10_bio);
- else {
+ ret = raid10_read_request(mddev, bio, r10_bio);
+ else
ret = raid10_write_request(mddev, bio, r10_bio);
- if (!ret)
- free_r10bio(r10_bio);
- }
+
+ if (!ret)
+ free_r10bio(r10_bio);
return ret;
}
@@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
int chunk_sects = chunk_mask + 1;
int sectors = bio_sectors(bio);
+ bool write = bio_data_dir(bio) == WRITE;
if (unlikely(bio->bi_opf & REQ_PREFLUSH)
&& md_flush_request(mddev, bio))
@@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sectors = chunk_sects -
(bio->bi_iter.bi_sector &
(chunk_sects - 1));
- if (!__make_request(mddev, bio, sectors))
+ if (!__make_request(mddev, bio, sectors) && write)
md_write_end(mddev);
/* In case raid10d snuck in to freeze_array */
@@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
rdev_dec_pending(rdev, mddev);
r10_bio->state = 0;
- raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
+ if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
+ free_r10bio(r10_bio);
+
/*
* allow_barrier after re-submit to ensure no sync io
* can be issued while regular io pending.
--
2.43.0
^ permalink raw reply related
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