* Re: [PATCH] md/raid1: protect head_position for read balance
From: sashiko-bot @ 2026-06-19 4:51 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260619044114.1208456-1-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Medium] The commit fixes a KCSAN-reported data race on `head_position` in `raid1.c` but misses the identical data race in the sibling `raid10.c` subsystem.
- [Medium] The patch addresses a KCSAN data race for `head_position` but misses identical concurrent data races on adjacent heuristic variables `next_seq_sect` and `seq_start`, leading to a potential double-fetch bug and unsigned integer underflow.
--
commit 754549019263d81271c296e50e19909fb366125b
Author: Chen Cheng <chencheng@fnnas.com>
md/raid1: protect head_position for read balance
KCSAN reports a data race between raid1_end_read_request() and
raid1_read_request().
The completion path updates conf->mirrors[disk].head_position in
update_head_pos() without a lock, while the read-balance heuristic reads
the same field locklessly in is_sequential() and choose_best_rdev().
[Severity: Medium]
This is a pre-existing issue, but does raid10 also need this same fix?
Looking at drivers/md/raid10.c, update_head_pos() updates
conf->mirrors[r10_bio->devs[slot].devnum].head_position locklessly, and
read_balance() reads it locklessly:
drivers/md/raid10.c:read_balance() {
...
new_distance = abs(r10_bio->devs[slot].addr -
conf->mirrors[disk].head_position);
...
}
Since raid10 shares this read-balancing heuristic, should the data race be
addressed there as well to prevent torn reads?
[ ... ]
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index 5b9368bd9e700..4cdf4484cab6c 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
[ ... ]
> @@ -737,7 +737,7 @@ static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
> {
> /* TODO: address issues with this check and concurrency. */
> return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
> - conf->mirrors[disk].head_position == r1_bio->sector;
> + READ_ONCE(conf->mirrors[disk].head_position) == r1_bio->sector;
> }
[Severity: Medium]
This is a pre-existing issue, but should next_seq_sect and seq_start also
be protected to prevent double-fetch bugs and underflows?
While this patch secures head_position, next_seq_sect is still read locklessly
here and in should_choose_next():
drivers/md/raid1.c:should_choose_next() {
...
return opt_iosize > 0 && mirror->seq_start != MaxSector &&
mirror->next_seq_sect > opt_iosize &&
mirror->next_seq_sect - opt_iosize >= mirror->seq_start;
}
Because mirror->next_seq_sect is read multiple times without READ_ONCE(),
could the compiler emit multiple memory loads?
If a concurrent update from update_read_sectors() reduces next_seq_sect
after the safety check (mirror->next_seq_sect > opt_iosize) but before the
subtraction, could it cause an unsigned integer underflow, bypassing the
seq_start check?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260619044114.1208456-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* Re: [PATCH] md/raid1: protect head_position for read balance
From: Chen Cheng @ 2026-06-19 4:55 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260619045116.E12901F000E9@smtp.kernel.org>
在 2026/6/19 12:51, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Medium] The commit fixes a KCSAN-reported data race on `head_position` in `raid1.c` but misses the identical data race in the sibling `raid10.c` subsystem.
> - [Medium] The patch addresses a KCSAN data race for `head_position` but misses identical concurrent data races on adjacent heuristic variables `next_seq_sect` and `seq_start`, leading to a potential double-fetch bug and unsigned integer underflow.
> --
>
> commit 754549019263d81271c296e50e19909fb366125b
> Author: Chen Cheng <chencheng@fnnas.com>
>
> md/raid1: protect head_position for read balance
>
> KCSAN reports a data race between raid1_end_read_request() and
> raid1_read_request().
>
> The completion path updates conf->mirrors[disk].head_position in
> update_head_pos() without a lock, while the read-balance heuristic reads
> the same field locklessly in is_sequential() and choose_best_rdev().
>
> [Severity: Medium]
> This is a pre-existing issue, but does raid10 also need this same fix?
>
> Looking at drivers/md/raid10.c, update_head_pos() updates
> conf->mirrors[r10_bio->devs[slot].devnum].head_position locklessly, and
> read_balance() reads it locklessly:
>
> drivers/md/raid10.c:read_balance() {
> ...
> new_distance = abs(r10_bio->devs[slot].addr -
> conf->mirrors[disk].head_position);
> ...
> }
>
> Since raid10 shares this read-balancing heuristic, should the data race be
> addressed there as well to prevent torn reads?
>
> [ ... ]
>
sure, already submited in another patch .
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index 5b9368bd9e700..4cdf4484cab6c 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
> [ ... ]
>> @@ -737,7 +737,7 @@ static bool is_sequential(struct r1conf *conf, int disk, struct r1bio *r1_bio)
>> {
>> /* TODO: address issues with this check and concurrency. */
>> return conf->mirrors[disk].next_seq_sect == r1_bio->sector ||
>> - conf->mirrors[disk].head_position == r1_bio->sector;
>> + READ_ONCE(conf->mirrors[disk].head_position) == r1_bio->sector;
>> }
>
> [Severity: Medium]
> This is a pre-existing issue, but should next_seq_sect and seq_start also
> be protected to prevent double-fetch bugs and underflows?
>
> While this patch secures head_position, next_seq_sect is still read locklessly
> here and in should_choose_next():
>
> drivers/md/raid1.c:should_choose_next() {
> ...
> return opt_iosize > 0 && mirror->seq_start != MaxSector &&
> mirror->next_seq_sect > opt_iosize &&
> mirror->next_seq_sect - opt_iosize >= mirror->seq_start;
> }
>
> Because mirror->next_seq_sect is read multiple times without READ_ONCE(),
> could the compiler emit multiple memory loads?
>
> If a concurrent update from update_read_sectors() reduces next_seq_sect
> after the safety check (mirror->next_seq_sect > opt_iosize) but before the
> subtraction, could it cause an unsigned integer underflow, bypassing the
> seq_start check?
>
good catch , and , I notice either. I will submit in separate patch .
^ permalink raw reply
* [PATCH] md/raid5: read batch_head under stripe_lock in make_stripe_request
From: Chen Cheng @ 2026-06-19 6:55 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
KCSAN reports race in raid5_make_request() vs. stripe_add_to_batch_list()
write: sh->batch_head = head->batch_head / head
in stripe_add_to_batch_list() via make_stripe_request()
read: (!sh->batch_head || sh == sh->batch_head)
in make_stripe_request()
variable: struct stripe_head::batch_head
sh->batch_head is documented as protected by stripe_lock.
KCSAN report:
BUG: KCSAN: data-race in raid5_make_request / raid5_make_request
write to 0xffff8f03062432d8 of 8 bytes by task 210246 on cpu 6:
raid5_make_request+0x175e/0x2ab0
md_handle_request+0x2c5/0x700
md_submit_bio+0x126/0x320
[.........]
btrfs_sync_file+0x181/0x970
vfs_fsync_range+0x71/0x110
do_fsync+0x46/0xa0
__x64_sys_fsync+0x20/0x30
read to 0xffff8f03062432d8 of 8 bytes by task 210251 on cpu 0:
raid5_make_request+0x7c7/0x2ab0
md_handle_request+0x2c5/0x700
md_submit_bio+0x126/0x320
[.........]
btrfs_remap_file_range+0x266/0x980
vfs_clone_file_range+0x16d/0x610
ioctl_file_clone+0x64/0xd0
do_vfs_ioctl+0x87f/0xbc0
__x64_sys_ioctl+0xb8/0x130
value changed: 0x0000000000000000 -> 0xffff8f0307798728
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 5521051a9425..efc63740f867 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6108,14 +6108,16 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
ctx->do_flush = false;
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
+ spin_lock_irq(&sh->stripe_lock);
if ((!sh->batch_head || sh == sh->batch_head) &&
(bi->bi_opf & REQ_SYNC) &&
!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
atomic_inc(&conf->preread_active_stripes);
+ spin_unlock_irq(&sh->stripe_lock);
release_stripe_plug(mddev, sh);
return STRIPE_SUCCESS;
out_release:
--
2.54.0
^ permalink raw reply related
* [PATCH v2] md/raid5: read batch_head under stripe_lock in make_stripe_request
From: Chen Cheng @ 2026-06-19 8:11 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
KCSAN reports race in raid5_make_request() vs. stripe_add_to_batch_list()
Writer flow (stripe_add_to_batch_list):
1. grab `head` stripe;
2. lock_two_stripes(head, sh);
3. re-check stripe_can_batch() for both head and sh, which requires
STRIPE_BATCH_READY set on both;
4. write head->batch_head = head and sh->batch_head = head;
5. unlock_two_stripes.
STRIPE_BATCH_READY is cleared in two places:
- clear_batch_ready(), at the entry of handle_stripe();
- __add_stripe_bio(), for non-batchable bios.
And, both need to acquire `stripe_lock`.
Under stripe_lock, if STRIPE_BATCH_READY is clear, then:
- New writers cannot install a batch_head;
- Existing writers have already finished.
So .. handle_stripe() readers can ready `batch_head` locklessly.
Fix way:
Writer side make_stripe_request() under STRIPE_BATCH_READY, so , need
to be protected by stripe_lock when read something..
v1 -> v2:
- re-expalin how stripe_lock and batch_head work in commit message , and ,
- modify comment in raid5.h.
Fixs: f4aec6a097387
KCSAN report:
======================================
BUG: KCSAN: data-race in raid5_make_request / raid5_make_request
write to 0xffff8f03062432d8 of 8 bytes by task 210246 on cpu 6:
raid5_make_request+0x175e/0x2ab0
md_handle_request+0x2c5/0x700
md_submit_bio+0x126/0x320
[.........]
btrfs_sync_file+0x181/0x970
vfs_fsync_range+0x71/0x110
do_fsync+0x46/0xa0
__x64_sys_fsync+0x20/0x30
read to 0xffff8f03062432d8 of 8 bytes by task 210251 on cpu 0:
raid5_make_request+0x7c7/0x2ab0
md_handle_request+0x2c5/0x700
md_submit_bio+0x126/0x320
[.........]
btrfs_remap_file_range+0x266/0x980
vfs_clone_file_range+0x16d/0x610
ioctl_file_clone+0x64/0xd0
do_vfs_ioctl+0x87f/0xbc0
__x64_sys_ioctl+0xb8/0x130
value changed: 0x0000000000000000 -> 0xffff8f0307798728
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 2 ++
drivers/md/raid5.h | 8 +++++++-
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 5521051a9425..efc63740f867 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6108,14 +6108,16 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
ctx->do_flush = false;
}
set_bit(STRIPE_HANDLE, &sh->state);
clear_bit(STRIPE_DELAYED, &sh->state);
+ spin_lock_irq(&sh->stripe_lock);
if ((!sh->batch_head || sh == sh->batch_head) &&
(bi->bi_opf & REQ_SYNC) &&
!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
atomic_inc(&conf->preread_active_stripes);
+ spin_unlock_irq(&sh->stripe_lock);
release_stripe_plug(mddev, sh);
return STRIPE_SUCCESS;
out_release:
diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
index 1c7b710fc9c1..9ff825697ba3 100644
--- a/drivers/md/raid5.h
+++ b/drivers/md/raid5.h
@@ -221,11 +221,17 @@ struct stripe_head {
enum reconstruct_states reconstruct_state;
spinlock_t stripe_lock;
int cpu;
struct r5worker_group *group;
- struct stripe_head *batch_head; /* protected by stripe lock */
+ /*
+ * Writer protected by stripe_lock.
+ * Reader hold stripe_lock when STRIPE_BATCH_READY is set.
+ * Without STRIPE_BATCH_READY means no concurrent write,
+ * lockless read is ok.
+ */
+ struct stripe_head *batch_head;
spinlock_t batch_lock; /* only header's lock is useful */
struct list_head batch_list; /* protected by head's batch lock*/
union {
struct r5l_io_unit *log_io;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH v2 2/4] md/raid10: fix writes_pending leak on write request failures
From: yu kuai @ 2026-06-20 18:25 UTC (permalink / raw)
To: sashiko-reviews, Abd-Alrhman Masalkhi; +Cc: linux-raid, yukuai
In-Reply-To: <20260613184042.BCEC01F000E9@smtp.kernel.org>
Hi,
在 2026/6/14 2:40, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The `r10_bio` structure allocated from the mempool is leaked when `raid10_write_request()` bails out early due to `REQ_NOWAIT`.
> --
This looks like a real issue, do you plan to fix this as well?
>
> commit 3940ccdb4c7c7e19c20f9b5e2e1d12545c2558b5
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md/raid10: fix writes_pending leak on write request failures
>
> raid10_make_request() acquires a writes_pending reference with
> md_write_start() before dispatching write requests. Several failure
> paths in raid10_write_request() complete the bio and return without
> reaching the normal write completion path, causing the corresponding
> md_write_end() to be skipped.
>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index cee5a253a281d..c123a8c76ddcd 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
> [ ... ]
>> @@ -1365,7 +1365,7 @@ static void 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);
>> - return;
>> + return false;
> [Severity: High]
> This isn't a bug introduced by this patch, but does this early return leak
> the r10_bio structure?
>
> __make_request() allocates r10_bio using:
> r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
>
> It then passes it to raid10_write_request(). When the bio has REQ_NOWAIT set
> during a clustered area resync, we complete the bio with an error and return
> false without freeing the r10_bio. Could repeatedly triggering this exhaust
> the r10bio_pool, causing deadlocks on subsequent operations?
>
> Note that the regular_request_wait() error path right below correctly calls
> free_r10bio(r10_bio) before returning.
>
>> }
>> for (;;) {
> [ ... ]
>> @@ -1398,7 +1398,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
>> if (bio->bi_opf & REQ_NOWAIT) {
>> allow_barrier(conf);
>> bio_wouldblock_error(bio);
>> - return;
>> + return false;
> [Severity: High]
> This is a pre-existing issue, but does this path also leak the r10_bio
> mempool allocation when waiting for a reshape metadata update?
>
> Similar to the clustered area resync case above, the r10_bio allocated in
> __make_request() does not appear to be freed before returning here.
>
>> }
>> mddev_add_trace_msg(conf->mddev,
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2 0/4] md/raid1,raid10: fix write-path reference leaks and clean up error handling
From: yu kuai @ 2026-06-20 18:31 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, axboe, hare,
john.g.garry, martin.petersen, vverma
Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260613182810.1317258-1-abd.masalkhi@gmail.com>
在 2026/6/14 2:28, Abd-Alrhman Masalkhi 写道:
> Hi,
>
> This series fixes several write-path failure handling issues in raid1 and
> raid10 and then follows up with a cleanup of raid1_write_request().
>
> The first two patches fix writes_pending leaks caused by failure paths
> that complete bios without reaching the normal write completion path.
> The raid1 fix also addresses a barrier reference leak when
> wait_blocked_rdev() fails after wait_barrier() succeeds.
>
> The third patch fixes additional writes_pending and barrier reference
> leaks in raid10 discard handling.
>
> The final patch simplifies raid1_write_request() error handling.
>
> Patches:
> md/raid1: fix writes_pending and barrier reference leaks on write failures
> md/raid10: fix writes_pending leak on write request failures
> md/raid10: fix writes_pending and barrier reference leaks on discard failures
> md/raid1: simplify raid1_write_request() error handling
>
> Changes in v2:
> - fix writes_pending leaks in addition to the barrier reference leaks
> - add raid10 fixes for analogous write and discard failure paths
> - add a follow-up cleanup patch to simplify raid1_write_request()
> - simplifies raid1_write_request() error handling.
> - Link v1: https://lore.kernel.org/linux-raid/20260611132500.763528-1-abd.masalkhi@gmail.com/
>
> Thanks,
> Abd-Alrhman
>
> Abd-Alrhman Masalkhi (4):
> md/raid1: fix writes_pending and barrier reference leaks on write
> failures
> md/raid10: fix writes_pending leak on write request failures
> md/raid10: fix writes_pending and barrier reference leaks on discard
> failures
> md/raid1: simplify raid1_write_request() error handling
>
> drivers/md/raid1.c | 74 ++++++++++++++++++++++++---------------------
> drivers/md/raid10.c | 28 ++++++++++++-----
> 2 files changed, 60 insertions(+), 42 deletions(-)
Applied to md-7.2
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md: use READ_ONCE() for rdev_attr_show() mddev check
From: yu kuai @ 2026-06-20 19:29 UTC (permalink / raw)
To: Runyu Xiao, song, linux-raid, yukuai
Cc: lilingfeng3, linux-kernel, jianhao.xu
In-Reply-To: <20260611083510.476945-1-runyu.xiao@seu.edu.cn>
Hi,
在 2026/6/11 16:35, Runyu Xiao 写道:
> md_kick_rdev_from_array() clears rdev->mddev with
> WRITE_ONCE(rdev->mddev, NULL), and rdev_attr_store() already snapshots
> that same shared pointer with READ_ONCE(rdev->mddev). rdev_attr_show()
> still tests the pointer with a plain lockless `if (!rdev->mddev)` before
> calling entry->show().
>
> A running system can reach this by reading rdev sysfs attributes while a
> device removal path is tearing the same md_rdev down. In that window,
> rdev_attr_show() can pass a stale plain guard while sibling
> rdev_attr_store() already observes NULL and returns -ENODEV, leaving the
> show path to operate after the shared mddev pointer has been revoked.
>
> Use READ_ONCE() in rdev_attr_show() so this sysfs read side matches the
> existing visibility contract on rdev->mddev.
>
> Fixes: 9cfcf99e7ed6 ("md: get rdev->mddev with READ_ONCE()")
> Signed-off-by: Runyu Xiao <runyu.xiao@seu.edu.cn>
> ---
> drivers/md/md.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 92ec4be20db8..a021cf4a798d 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3704,7 +3704,7 @@ rdev_attr_show(struct kobject *kobj, struct attribute *attr, char *page)
>
> if (!entry->show)
> return -EIO;
> - if (!rdev->mddev)
> + if (!READ_ONCE(rdev->mddev))
> return -ENODEV;
unlike rdev_attr_store(), kernel can panic if abnormal value is read. However, this is just a
read and the value is not used at all, READ_ONCE() is not used here on purpose by 9cfcf99e7ed6.
> return entry->show(rdev, page);
> }
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v4 1/3] md: suspend array before raid10 reshape via sync_action
From: yu kuai @ 2026-06-20 19:43 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260603035925.217847-2-chencheng@fnnas.com>
Hi,
在 2026/6/3 11:59, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> The sync_action=reshape path currently enters mddev_start_reshape() with
> reconfig_mutex held but without suspending the array first. For raid10,
> that means raid10_start_reshape() has to drop reconfig_mutex and reacquire
> the array through mddev_suspend_and_lock_nointr() before it can safely
> switch geometry-dependent state.
>
> Use mddev_suspend_and_lock() for ACTION_RESHAPE in action_store(), so
> the sysfs reshape path reaches mddev_start_reshape() with the array
> already suspended and locked.
>
> Other sync_action operations keep using mddev_lock() unchanged.
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/md.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd..5bc937e149ac 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -5256,30 +5256,39 @@ static int mddev_start_reshape(struct mddev *mddev)
>
> static ssize_t
> action_store(struct mddev *mddev, const char *page, size_t len)
> {
> int ret;
> + bool suspended = false;
> enum sync_action action;
>
> if (!mddev->pers || !mddev->pers->sync_request)
> return -EINVAL;
>
> + action = md_sync_action_by_name(page);
> retry:
> if (work_busy(&mddev->sync_work))
> flush_work(&mddev->sync_work);
>
> - ret = mddev_lock(mddev);
> + if (action == ACTION_RESHAPE) {
> + ret = mddev_suspend_and_lock(mddev);
suspend during retry is too heavy. Please move suspend above retry.
> + suspended = true;
> + } else {
> + ret = mddev_lock(mddev);
> + suspended = false;
> + }
> if (ret)
> return ret;
>
> if (work_busy(&mddev->sync_work)) {
> - mddev_unlock(mddev);
> + if (suspended)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
> goto retry;
> }
>
> - action = md_sync_action_by_name(page);
> -
> /* TODO: mdadm rely on "idle" to start sync_thread. */
> if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) {
> switch (action) {
> case ACTION_FROZEN:
> md_frozen_sync_thread(mddev);
> @@ -5344,11 +5353,14 @@ action_store(struct mddev *mddev, const char *page, size_t len)
> md_wakeup_thread(mddev->thread);
> sysfs_notify_dirent_safe(mddev->sysfs_action);
> ret = len;
>
> out:
> - mddev_unlock(mddev);
> + if (suspended)
> + mddev_unlock_and_resume(mddev);
> + else
> + mddev_unlock(mddev);
> return ret;
> }
>
> static struct md_sysfs_entry md_scan_mode =
> __ATTR_PREALLOC(sync_action, S_IRUGO|S_IWUSR, action_show, action_store);
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs
From: yu kuai @ 2026-06-20 20:02 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260603035925.217847-4-chencheng@fnnas.com>
Hi,
在 2026/6/3 11:59, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> After reshape changes raid_disks, an in-flight r10bio from the old geometry
> can still be completed or freed later. In that case, using the current
> geometry to walk r10_bio->devs[] is unsafe. A failure was reproduced with a
> simple write workload while reshaping a raid10 array from 4 disks to 5 disks.
> e.g.:
>
> mdadm -C /dev/md777 -l10 -n4 /dev/sda /dev/sdb /dev/sdc /dev/sdd
> mkfs.ext4 /dev/md777
> mount /dev/md777 /mnt/test
> fsstress -d /mnt/test -n 24000 -p 8 -l 24 &
> mdadm /dev/md777 --add /dev/sde
> mdadm --grow /dev/md777 --raid-devices=5 \
> --backup-file=/tmp/md-reshape-backup
>
> the sequence above can trigger:
>
> BUG: KASAN: slab-out-of-bounds in free_r10bio+0x1c4/0x260 [raid10]
> Read of size 8 at addr ffff00008c2dfac8 by task ksoftirqd/0/15
> free_r10bio
> raid_end_bio_io
> one_write_done
> raid10_end_write_request
>
> The buggy object was 200 bytes long, which matches an r10bio with space for
> only four devs[] entries. However, put_all_bios() and find_bio_disk() walk
> r10_bio->devs[] using the current conf->geo.raid_disks value. Once reshape
> switches conf->geo.raid_disks from 4 to 5, an old 4-slot r10bio can be
> completed or freed as if it had 5 slots, and the walk overruns devs[4]. The
I don't understand, is this still possible after patch 1.
> same stale-width mismatch can also surface during a 5-disk to 4-disk reshape.
>
> Track the number of valid devs[] entries in each reused r10bio with
> used_nr_devs. Initialize it whenever an r10bio is prepared for regular I/O,
> discard, or resync/recovery/reshape work, and use it to bound devs[] walks
> in put_all_bios() and find_bio_disk().
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid10.c | 8 ++++++--
> drivers/md/raid10.h | 2 ++
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 5eca34432e63..f134b93fd593 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -273,11 +273,11 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
>
> static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
> {
> int i;
>
> - for (i = 0; i < conf->geo.raid_disks; i++) {
> + for (i = 0; i < r10_bio->used_nr_devs; i++) {
> struct bio **bio = & r10_bio->devs[i].bio;
> if (!BIO_SPECIAL(*bio))
> bio_put(*bio);
> *bio = NULL;
> bio = &r10_bio->devs[i].repl_bio;
> @@ -370,11 +370,11 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
> struct bio *bio, int *slotp, int *replp)
> {
> int slot;
> int repl = 0;
>
> - for (slot = 0; slot < conf->geo.raid_disks; slot++) {
> + for (slot = 0; slot < r10_bio->used_nr_devs; slot++) {
> if (r10_bio->devs[slot].bio == bio)
> break;
> if (r10_bio->devs[slot].repl_bio == bio) {
> repl = 1;
> break;
> @@ -1561,10 +1561,11 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>
> r10_bio->mddev = mddev;
> r10_bio->sector = bio->bi_iter.bi_sector;
> r10_bio->state = 0;
> r10_bio->read_slot = -1;
> + r10_bio->used_nr_devs = conf->geo.raid_disks;
> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
> conf->geo.raid_disks);
>
> if (bio_data_dir(bio) == READ)
> raid10_read_request(mddev, bio, r10_bio);
> @@ -1749,10 +1750,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
> r10_bio->mddev = mddev;
> r10_bio->state = 0;
> r10_bio->sectors = 0;
> r10_bio->read_slot = -1;
> + r10_bio->used_nr_devs = geo->raid_disks;
> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * geo->raid_disks);
> wait_blocked_dev(mddev, r10_bio);
>
> /*
> * For far layout it needs more than one r10bio to cover all regions.
> @@ -3083,10 +3085,12 @@ static struct r10bio *raid10_alloc_init_r10buf(struct r10conf *conf)
> test_bit(MD_RECOVERY_RESHAPE, &conf->mddev->recovery))
> nalloc = conf->copies; /* resync */
> else
> nalloc = 2; /* recovery */
>
> + r10bio->used_nr_devs = nalloc;
> +
> for (i = 0; i < nalloc; i++) {
> bio = r10bio->devs[i].bio;
> rp = bio->bi_private;
> bio_reset(bio, NULL, 0);
> bio->bi_private = rp;
> diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
> index b711626a5db7..4751119f9770 100644
> --- a/drivers/md/raid10.h
> +++ b/drivers/md/raid10.h
> @@ -125,10 +125,12 @@ struct r10bio {
> struct bio *master_bio;
> /*
> * if the IO is in READ direction, then this is where we read
> */
> int read_slot;
> + /* Used to bound devs[] walks when the object is reused. */
> + unsigned int used_nr_devs;
>
> struct list_head retry_list;
> /*
> * if the IO is in WRITE direction, then multiple bios are used,
> * one for each copy.
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v4 2/3] md/raid10: make r10bio_pool use fixed-size objects
From: yu kuai @ 2026-06-20 20:05 UTC (permalink / raw)
To: Chen Cheng, linux-raid; +Cc: linux-kernel, yukuai
In-Reply-To: <20260603035925.217847-3-chencheng@fnnas.com>
Hi,
在 2026/6/3 11:59, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> raid10 currently sizes regular r10bio_pool objects from
> conf->geo.raid_disks, which makes the mempool element width depend on
> the current geometry.
>
> That breaks across reshape. Regular r10bio objects are preallocated and
> reused, so after a geometry change the pool may still contain objects
> allocated for the old width. A later request under the new geometry can
> then reuse an r10bio whose devs[] array is still sized for the previous
> raid_disks value.
>
> Fix this by backing r10bio_pool with a fixed-size kmalloc mempool sized
> for the maximum width needed across the current reshape transition.
> Apply the same sizing rule to standalone r10bio objects allocated from
> r10buf_pool_alloc().
>
> This removes the geometry-dependent allocation width from regular
> r10bio_pool objects and prevents reshape from reusing pool entries that
> are too small for the new layout.
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid10.c | 48 +++++++++++++++++++++++++++++++++------------
> drivers/md/raid10.h | 2 +-
> 2 files changed, 36 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index cee5a253a281..5eca34432e63 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -101,17 +101,32 @@ static void end_reshape(struct r10conf *conf);
> static inline struct r10bio *get_resync_r10bio(struct bio *bio)
> {
> return get_resync_pages(bio)->raid_bio;
> }
>
> -static void * r10bio_pool_alloc(gfp_t gfp_flags, void *data)
> +static inline unsigned int calc_r10bio_pool_disks(struct mddev *mddev)
> {
> - struct r10conf *conf = data;
> - int size = offsetof(struct r10bio, devs[conf->geo.raid_disks]);
> + /* If delta_disks < 0, use bigger r10bio->devs[] is ok. */
> + return mddev->raid_disks + max(0, mddev->delta_disks);
I don't understand this change first, I get what you want to do after taking
a look at patch 3. However, I think geo.raid_disks is safe here. A r10_bio
that is allocated from old pool should never be reused under new geometry.
> +}
> +
> +static inline int calc_r10bio_size(struct mddev *mddev)
> +{
> + return offsetof(struct r10bio, devs[calc_r10bio_pool_disks(mddev)]);
> +}
> +
> +static mempool_t *create_r10bio_pool(struct mddev *mddev)
> +{
> + int size = calc_r10bio_size(mddev);
> +
> + return mempool_create_kmalloc_pool(NR_RAID_BIOS, size);
> +}
> +
> +static struct r10bio *alloc_r10bio(struct mddev *mddev, gfp_t gfp_flags)
> +{
> + int size = calc_r10bio_size(mddev);
>
> - /* allocate a r10bio with room for raid_disks entries in the
> - * bios array */
> return kzalloc(size, gfp_flags);
> }
>
> #define RESYNC_SECTORS (RESYNC_BLOCK_SIZE >> 9)
> /* amount of memory to reserve for resync requests */
> @@ -135,11 +150,11 @@ static void * r10buf_pool_alloc(gfp_t gfp_flags, void *data)
> struct bio *bio;
> int j;
> int nalloc, nalloc_rp;
> struct resync_pages *rps;
>
> - r10_bio = r10bio_pool_alloc(gfp_flags, conf);
> + r10_bio = alloc_r10bio(conf->mddev, gfp_flags);
> if (!r10_bio)
> return NULL;
>
> if (test_bit(MD_RECOVERY_SYNC, &conf->mddev->recovery) ||
> test_bit(MD_RECOVERY_RESHAPE, &conf->mddev->recovery))
> @@ -275,11 +290,11 @@ static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
> static void free_r10bio(struct r10bio *r10_bio)
> {
> struct r10conf *conf = r10_bio->mddev->private;
>
> put_all_bios(conf, r10_bio);
> - mempool_free(r10_bio, &conf->r10bio_pool);
> + mempool_free(r10_bio, conf->r10bio_pool);
> }
>
> static void put_buf(struct r10bio *r10_bio)
> {
> struct r10conf *conf = r10_bio->mddev->private;
> @@ -1537,11 +1552,11 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
> static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
> {
> struct r10conf *conf = mddev->private;
> struct r10bio *r10_bio;
>
> - r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
> + r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
>
> r10_bio->master_bio = bio;
> r10_bio->sectors = sectors;
>
> r10_bio->mddev = mddev;
> @@ -1729,11 +1744,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
> last_stripe_index *= geo->far_copies;
> end_disk_offset = (bio_end & geo->chunk_mask) +
> (last_stripe_index << geo->chunk_shift);
>
> retry_discard:
> - r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
> + r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
> r10_bio->mddev = mddev;
> r10_bio->state = 0;
> r10_bio->sectors = 0;
> r10_bio->read_slot = -1;
> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * geo->raid_disks);
> @@ -3830,11 +3845,11 @@ static int setup_geo(struct geom *geo, struct mddev *mddev, enum geo_type new)
> static void raid10_free_conf(struct r10conf *conf)
> {
> if (!conf)
> return;
>
> - mempool_exit(&conf->r10bio_pool);
> + mempool_destroy(conf->r10bio_pool);
> kfree(conf->mirrors);
> kfree(conf->mirrors_old);
> kfree(conf->mirrors_new);
> safe_put_page(conf->tmppage);
> bioset_exit(&conf->bio_split);
> @@ -3877,13 +3892,12 @@ static struct r10conf *setup_conf(struct mddev *mddev)
> if (!conf->tmppage)
> goto out;
>
> conf->geo = geo;
> conf->copies = copies;
> - err = mempool_init(&conf->r10bio_pool, NR_RAID_BIOS, r10bio_pool_alloc,
> - rbio_pool_free, conf);
> - if (err)
> + conf->r10bio_pool = create_r10bio_pool(mddev);
> + if (!conf->r10bio_pool)
> goto out;
>
> err = bioset_init(&conf->bio_split, BIO_POOL_SIZE, 0, 0);
> if (err)
> goto out;
> @@ -4373,10 +4387,11 @@ static int raid10_start_reshape(struct mddev *mddev)
> struct geom new;
> struct r10conf *conf = mddev->private;
> struct md_rdev *rdev;
> int spares = 0;
> int ret;
> + mempool_t *new_pool;
>
> if (test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
> return -EBUSY;
>
> if (setup_geo(&new, mddev, geo_start) != conf->copies)
> @@ -4409,10 +4424,17 @@ static int raid10_start_reshape(struct mddev *mddev)
>
> if (spares < mddev->delta_disks)
> return -EINVAL;
>
> conf->offset_diff = min_offset_diff;
> + if (mddev->delta_disks > 0) {
> + new_pool = create_r10bio_pool(mddev);
> + if (!new_pool)
> + return -ENOMEM;
> + mempool_destroy(conf->r10bio_pool);
> + conf->r10bio_pool = new_pool;
> + }
> spin_lock_irq(&conf->device_lock);
> if (conf->mirrors_new) {
> memcpy(conf->mirrors_new, conf->mirrors,
> sizeof(struct raid10_info)*conf->prev.raid_disks);
> smp_mb();
> diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
> index ec79d87fb92f..b711626a5db7 100644
> --- a/drivers/md/raid10.h
> +++ b/drivers/md/raid10.h
> @@ -85,11 +85,11 @@ struct r10conf {
> int have_replacement; /* There is at least one
> * replacement device.
> */
> wait_queue_head_t wait_barrier;
>
> - mempool_t r10bio_pool;
> + mempool_t *r10bio_pool;
> mempool_t r10buf_pool;
> struct page *tmppage;
> struct bio_set bio_split;
>
> /* When taking over an array from a different personality, we store
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: honor REQ_NOWAIT when waiting for behind writes
From: Yu Kuai @ 2026-06-20 20:18 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, vverma,
axboe
Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260611083514.754922-1-abd.masalkhi@gmail.com>
Hi,
在 2026/6/11 16:35, Abd-Alrhman Masalkhi 写道:
> raid1 supports REQ_NOWAIT reads by avoiding waits in the barrier path
> through wait_read_barrier(). However, a read can still block on a
> WriteMostly device when the array uses a bitmap and there are
> outstanding behind writes.
>
> In that case raid1 unconditionally calls wait_behind_writes(), which
> may sleep until all behind writes complete. As a result, a REQ_NOWAIT
> read can block despite the caller explicitly requesting non-blocking
> behavior.
>
> This ensures that raid1 consistently honors REQ_NOWAIT reads across all
> paths that may otherwise wait for behind writes.
>
> Fixes: 5aa705039c4f ("md: raid1 add nowait support")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> drivers/md/md-bitmap.c | 9 +++++++--
> drivers/md/md-bitmap.h | 2 +-
> drivers/md/md-llbitmap.c | 13 ++++++++-----
> drivers/md/md.c | 2 +-
> drivers/md/raid1.c | 10 +++++++---
> 5 files changed, 24 insertions(+), 12 deletions(-)
Applied to md-7.2.
One note, remove nowait support is already in my to-do lists. There is a case
that can't be handled for now, for example:
2 disk raid1, issue no wait write IO, rdev1 succeed while rdev2 failed. In this
case, we don't know if rdev2 failed issue because nowait or disk really have
badblocks. So we can't either record badblocks or issue again without nowait,
for consequence, rdev1 and rdev2 will end up with different data.
I think the best solution is to remove nowait support for mdraid. Feel free
to cook a patch or if you have something else in mind.
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 028b9ca8ce52..1206e31f323a 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2063,18 +2063,23 @@ static void bitmap_end_behind_write(struct mddev *mddev)
> bitmap->mddev->bitmap_info.max_write_behind);
> }
>
> -static void bitmap_wait_behind_writes(struct mddev *mddev)
> +static bool bitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
> {
> struct bitmap *bitmap = mddev->bitmap;
>
> /* wait for behind writes to complete */
> if (bitmap && atomic_read(&bitmap->behind_writes) > 0) {
> + if (nowait)
> + return false;
> +
> pr_debug("md:%s: behind writes in progress - waiting to stop.\n",
> mdname(mddev));
> /* need to kick something here to make sure I/O goes? */
> wait_event(bitmap->behind_wait,
> atomic_read(&bitmap->behind_writes) == 0);
> }
> +
> + return true;
> }
>
> static void bitmap_destroy(struct mddev *mddev)
> @@ -2084,7 +2089,7 @@ static void bitmap_destroy(struct mddev *mddev)
> if (!bitmap) /* there was no bitmap */
> return;
>
> - bitmap_wait_behind_writes(mddev);
> + bitmap_wait_behind_writes(mddev, false);
> if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
> mddev_destroy_serial_pool(mddev, NULL);
>
> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
> index 214f623c7e79..f46674bdfeb9 100644
> --- a/drivers/md/md-bitmap.h
> +++ b/drivers/md/md-bitmap.h
> @@ -98,7 +98,7 @@ struct bitmap_operations {
>
> void (*start_behind_write)(struct mddev *mddev);
> void (*end_behind_write)(struct mddev *mddev);
> - void (*wait_behind_writes)(struct mddev *mddev);
> + bool (*wait_behind_writes)(struct mddev *mddev, bool nowait);
>
> md_bitmap_fn *start_write;
> md_bitmap_fn *end_write;
> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index 1adc5b117821..5a4e2abaa757 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1574,16 +1574,19 @@ static void llbitmap_end_behind_write(struct mddev *mddev)
> wake_up(&llbitmap->behind_wait);
> }
>
> -static void llbitmap_wait_behind_writes(struct mddev *mddev)
> +static bool llbitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
> {
> struct llbitmap *llbitmap = mddev->bitmap;
>
> - if (!llbitmap)
> - return;
> + if (llbitmap && atomic_read(&llbitmap->behind_writes) > 0) {
> + if (nowait)
> + return false;
>
> - wait_event(llbitmap->behind_wait,
> - atomic_read(&llbitmap->behind_writes) == 0);
> + wait_event(llbitmap->behind_wait,
> + atomic_read(&llbitmap->behind_writes) == 0);
> + }
>
> + return true;
> }
>
> static ssize_t bits_show(struct mddev *mddev, char *page)
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 096bb64e87bd..d1465bcd86c8 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -7050,7 +7050,7 @@ EXPORT_SYMBOL_GPL(md_stop_writes);
> static void mddev_detach(struct mddev *mddev)
> {
> if (md_bitmap_enabled(mddev, false))
> - mddev->bitmap_ops->wait_behind_writes(mddev);
> + mddev->bitmap_ops->wait_behind_writes(mddev, false);
> if (mddev->pers && mddev->pers->quiesce && !is_md_suspended(mddev)) {
> mddev->pers->quiesce(mddev, 1);
> mddev->pers->quiesce(mddev, 0);
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index b1ed4cc6ade4..b2d7c13b64bd 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -1341,6 +1341,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> int max_sectors;
> int rdisk;
> bool r1bio_existed = !!r1_bio;
> + bool nowait = bio->bi_opf & REQ_NOWAIT;
>
> /*
> * An md cloned bio indicates we are in the error path.
> @@ -1360,8 +1361,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> * Still need barrier for READ in case that whole
> * array is frozen.
> */
> - if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> - bio->bi_opf & REQ_NOWAIT)) {
> + if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
> bio_wouldblock_error(bio);
> return;
> }
> @@ -1402,7 +1402,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> * over-take any writes that are 'behind'
> */
> mddev_add_trace_msg(mddev, "raid1 wait behind writes");
> - mddev->bitmap_ops->wait_behind_writes(mddev);
> + if (!mddev->bitmap_ops->wait_behind_writes(mddev, nowait)) {
> + bio_wouldblock_error(bio);
> + set_bit(R1BIO_Returned, &r1_bio->state);
> + goto err_handle;
> + }
> }
>
> if (max_sectors < bio_sectors(bio)) {
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: free r1_bio when REQ_NOWAIT is set and read would block on retry
From: yu kuai @ 2026-06-20 20:30 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, vverma,
axboe
Cc: linux-raid, linux-kernel, sashiko-bot, yukuai
In-Reply-To: <20260611101350.759154-1-abd.masalkhi@gmail.com>
在 2026/6/11 18:13, Abd-Alrhman Masalkhi 写道:
> When a read is retried, raid1_read_request() may be called with a
> pre-allocated r1_bio. If wait_read_barrier() fails for a REQ_NOWAIT
> read, the bio is completed and the function returns immediately. In this
> case the existing r1_bio is leaked.
>
> This fixes a leak of pre-allocated r1_bio structures for retried reads.
>
> Fixes: 5aa705039c4f ("md: raid1 add nowait support")
> Reported-by: sashiko-bot<sashiko-bot@kernel.org>
> Closes:https://sashiko.dev/#/patchset/20260611083514.754922-1-abd.masalkhi@gmail.com?part=1
> Signed-off-by: Abd-Alrhman Masalkhi<abd.masalkhi@gmail.com>
> ---
> drivers/md/raid1.c | 6 ++++++
> 1 file changed, 6 insertions(+)
Applied to md-7.2
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: protect head_position for read balance
From: yu kuai @ 2026-06-20 20:44 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260619044114.1208456-1-chencheng@fnnas.com>
在 2026/6/19 12:41, Chen Cheng 写道:
> From: Chen Cheng<chencheng@fnnas.com>
>
> KCSAN reports a data race between raid1_end_read_request() and
> raid1_read_request().
>
> The completion path updates conf->mirrors[disk].head_position in
> update_head_pos() without a lock, while the read-balance heuristic reads
> the same field locklessly in is_sequential() and choose_best_rdev().
>
> KCSAN report:
> =========================
> BUG: KCSAN: data-race in raid1_end_read_request / raid1_read_request
>
> write to 0xffff8f0306ba7868 of 8 bytes by interrupt on cpu 9:
> raid1_end_read_request+0xb5/0x440
> bio_endio+0x3c9/0x3e0
> blk_update_request+0x257/0x770
> scsi_end_request+0x4d/0x520
> scsi_io_completion+0x6f/0x990
> scsi_finish_command+0x188/0x280
> scsi_complete+0xac/0x160
> blk_complete_reqs+0x8e/0xb0
> blk_done_softirq+0x1d/0x30
> [...]
>
> read to 0xffff8f0306ba7868 of 8 bytes by task 667002 on cpu 11:
> raid1_read_request+0x497/0x1a10
> raid1_make_request+0xdf/0x1950
> md_handle_request+0x2c5/0x700
> md_submit_bio+0x126/0x320
> __submit_bio+0x2ec/0x3a0
> submit_bio_noacct_nocheck+0x572/0x890
> [...]
>
> value changed: 0x0000000000000078 -> 0x00000000005fe448
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
> drivers/md/raid1.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
Applied to md-7.2
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: yu kuai @ 2026-06-20 21:13 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: chenchneg33, linux-kernel
In-Reply-To: <20260617142839.882378-1-chencheng@fnnas.com>
Hi,
在 2026/6/17 22:28, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> kcsan detect race :
> - raid5d() closes the current bitmap batch by updating
> conf->seq_flush under conf->device_lock.
> - __add_stripe_bio() read conf->seq_flush without that
> lock when assigning sh->bm_seq.
>
> so, protect seq_flush/seq_write consistency for multiple CPUs by
> READ_ONCE()/WRITE_ONCE() under the path without held device_lock.
>
> re-explain the stripe batch sequence number update flow:
> 1. sh->bm_seq declare which batch number the stripe belongs to
> when perform bitmap-related write.
> ==> bm_seq = seq_flush+1
>
> 2. stripe be handled,
> * if sh->bm_seq - conf->seq_write > 0, means the
> batch stripes **newer than** the last written
> batch, it cannot proceed yet, queued on bitmap_list.
> * otherwise , has already proceed.
>
> 3. raid5d() `++seq_flush` to closes the current batch, means
> * no more stripes join that old batch
> * just-closed batch ready to write-out to disk
>
> 4. raid5d() calls bitmap hooks unplug() or writeout, then,
> `++seq_write` to the same as bm_seq.
>
> - seq_flush - for producer, to close batches.
> - seq_write - for consumer, the checkpoint number.
>
> the report:
> ====================================
> BUG: KCSAN: data-race in __add_stripe_bio / raid5d
>
> write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
> raid5d+0x1d9/0xba0
> [.....]
>
> read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
> __add_stripe_bio+0x332/0x400
> raid5_make_request+0x6ac/0x2930
> md_handle_request+0x4a2/0xa40
> md_submit_bio+0x109/0x1a0
> __submit_bio+0x2ec/0x390
> [.....]
>
> v1 -> v2:
> - remove WRITE_ONCE(conf->seq_write) in held device_lock path.
> - remove READ_ONCE(conf->seq_flush) in held device_lock path.
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a320b71d7117..b2c5a1930841 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -3536,11 +3536,11 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
> pr_debug("added bi b#%llu to stripe s#%llu, disk %d, logical %llu\n",
> (*bip)->bi_iter.bi_sector, sh->sector, dd_idx,
> sh->dev[dd_idx].sector);
>
> if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
> - sh->bm_seq = conf->seq_flush+1;
> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> set_bit(STRIPE_BIT_DELAY, &sh->state);
> }
> }
>
> /*
> @@ -5827,11 +5827,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
> md_write_inc(mddev, bi);
> sh->overwrite_disks++;
> }
> spin_unlock_irq(&sh->stripe_lock);
> if (conf->mddev->bitmap) {
> - sh->bm_seq = conf->seq_flush + 1;
> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
> set_bit(STRIPE_BIT_DELAY, &sh->state);
> }
>
> set_bit(STRIPE_HANDLE, &sh->state);
> clear_bit(STRIPE_DELAYED, &sh->state);
> @@ -6877,16 +6877,17 @@ static void raid5d(struct md_thread *thread)
> clear_bit(R5_DID_ALLOC, &conf->cache_state);
>
> if (
> !list_empty(&conf->bitmap_list)) {
> /* Now is a good time to flush some bitmap updates */
> - conf->seq_flush++;
> + int seq = conf->seq_flush + 1;
checkpatch should warn this about missing blank line after declaration.
> + WRITE_ONCE(conf->seq_flush, seq);
WRITE_ONCE(a, a + 1) is safe, you can see lots of examples.
> spin_unlock_irq(&conf->device_lock);
> if (md_bitmap_enabled(mddev, true))
> mddev->bitmap_ops->unplug(mddev, true);
> spin_lock_irq(&conf->device_lock);
> - conf->seq_write = conf->seq_flush;
> + conf->seq_write = seq;
> activate_bit_delay(conf, conf->temp_inactive_list);
> }
> raid5_activate_delayed(conf);
>
> while ((bio = remove_bio_from_retry(conf, &offset))) {
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: yu kuai @ 2026-06-20 21:27 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: chenchneg33, linux-kernel
In-Reply-To: <20260618025735.915113-1-chencheng@fnnas.com>
在 2026/6/18 10:57, Chen Cheng 写道:
> From: Chen Cheng<chencheng@fnnas.com>
>
> Once the 32-bit seq wraps, a newer bm_seq can look smaller
> than old, so .. covert to wrap-safe calculate way.
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied to md-7.2
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: use stripe state snapshot in break_stripe_batch_list()
From: yu kuai @ 2026-06-20 21:37 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260618134748.1168360-1-chencheng@fnnas.com>
Hi,
在 2026/6/18 21:47, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> The patch just suppress KCSAN noise. No functional change.
>
> RAID-5 can group multi full-stripe-write aka stripe_head into a
> batch aka batch_list, with one head_sh leading them. Call
> break_stripe_batch_list() when the batch is finished, or,
> a stripe has to be dropped out of the batch.
>
> break_stripe_batch_list() reads stripe state several times while
> request paths can update thost state words concurrently with
> lockless bitops, which reported by KCSAN.
>
> Use a snapshot to guarantees that the value used for
> warning, copying, and handle checks is internally consistent
> at current read moment.
>
> KCSAN report:
> ==============================================
> BUG: KCSAN: data-race in __add_stripe_bio / break_stripe_batch_list
>
> write (marked) to 0xffff8e89d4f0b988 of 8 bytes by task 4323 on cpu 3:
> __add_stripe_bio+0x35e/0x400
> raid5_make_request+0x6ac/0x2930
> md_handle_request+0x4a2/0xa40
> md_submit_bio+0x109/0x1a0
> __submit_bio+0x2ec/0x390
> submit_bio_noacct_nocheck+0x457/0x710
> submit_bio_noacct+0x2a7/0xc20
> submit_bio+0x56/0x250
> blkdev_direct_IO+0x54c/0xda0
> blkdev_write_iter+0x38f/0x570
> aio_write+0x22b/0x490
> io_submit_one+0xa51/0xf70
>
> read to 0xffff8e89d4f0b988 of 8 bytes by task 4290 on cpu 4:
> break_stripe_batch_list+0x3ce/0x480
> handle_stripe_clean_event+0x720/0x9b0
> handle_stripe+0x32fb/0x4500
> handle_active_stripes.isra.0+0x6e0/0xa50
> raid5d+0x7e0/0xba0
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
You have another patch that is relied on this patch, they should be in one
patchset. What's worse, they are sent in the reverse order :(
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 26c24986e01c..a376560be92e 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -4906,35 +4906,39 @@ static int clear_batch_ready(struct stripe_head *sh)
> static void break_stripe_batch_list(struct stripe_head *head_sh,
> unsigned long handle_flags)
> {
> struct stripe_head *sh, *next;
> int i;
> + unsigned long state;
>
> list_for_each_entry_safe(sh, next, &head_sh->batch_list, batch_list) {
>
> list_del_init(&sh->batch_list);
>
> - WARN_ONCE(sh->state & ((1 << STRIPE_ACTIVE) |
> - (1 << STRIPE_SYNCING) |
> - (1 << STRIPE_REPLACED) |
> - (1 << STRIPE_DELAYED) |
> - (1 << STRIPE_BIT_DELAY) |
> - (1 << STRIPE_FULL_WRITE) |
> - (1 << STRIPE_BIOFILL_RUN) |
> - (1 << STRIPE_COMPUTE_RUN) |
> - (1 << STRIPE_DISCARD) |
> - (1 << STRIPE_BATCH_READY) |
> - (1 << STRIPE_BATCH_ERR)),
> - "stripe state: %lx\n", sh->state);
> - WARN_ONCE(head_sh->state & ((1 << STRIPE_DISCARD) |
> - (1 << STRIPE_REPLACED)),
> - "head stripe state: %lx\n", head_sh->state);
> + state = READ_ONCE(sh->state);
> + WARN_ONCE(state & ((1 << STRIPE_ACTIVE) |
> + (1 << STRIPE_SYNCING) |
> + (1 << STRIPE_REPLACED) |
> + (1 << STRIPE_DELAYED) |
> + (1 << STRIPE_BIT_DELAY) |
> + (1 << STRIPE_FULL_WRITE) |
> + (1 << STRIPE_BIOFILL_RUN) |
> + (1 << STRIPE_COMPUTE_RUN) |
> + (1 << STRIPE_DISCARD) |
> + (1 << STRIPE_BATCH_READY) |
> + (1 << STRIPE_BATCH_ERR)),
> + "stripe state: %lx\n", state);
> +
> + state = READ_ONCE(head_sh->state);
> + WARN_ONCE(state & ((1 << STRIPE_DISCARD) |
> + (1 << STRIPE_REPLACED)),
> + "head stripe state: %lx\n", state);
>
> set_mask_bits(&sh->state, ~(STRIPE_EXPAND_SYNC_FLAGS |
> (1 << STRIPE_PREREAD_ACTIVE) |
> (1 << STRIPE_ON_UNPLUG_LIST)),
> - head_sh->state & (1 << STRIPE_INSYNC));
> + state & (1 << STRIPE_INSYNC));
>
> sh->check_state = head_sh->check_state;
> sh->reconstruct_state = head_sh->reconstruct_state;
> spin_lock_irq(&sh->stripe_lock);
> sh->batch_head = NULL;
> @@ -4943,22 +4947,25 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
> if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
> wake_up_bit(&sh->dev[i].flags, R5_Overlap);
> sh->dev[i].flags = head_sh->dev[i].flags &
> (~((1 << R5_WriteError) | (1 << R5_Overlap)));
> }
> - if (handle_flags == 0 ||
> - sh->state & handle_flags)
> +
> + state = READ_ONCE(sh->state);
> + if (handle_flags == 0 || (state & handle_flags))
> set_bit(STRIPE_HANDLE, &sh->state);
> raid5_release_stripe(sh);
> }
> spin_lock_irq(&head_sh->stripe_lock);
> head_sh->batch_head = NULL;
> spin_unlock_irq(&head_sh->stripe_lock);
> for (i = 0; i < head_sh->disks; i++)
> if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
> wake_up_bit(&head_sh->dev[i].flags, R5_Overlap);
> - if (head_sh->state & handle_flags)
> +
> + state = READ_ONCE(head_sh->state);
> + if (state & handle_flags)
> set_bit(STRIPE_HANDLE, &head_sh->state);
> }
>
> /*
> * handle_stripe - do things to a stripe.
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: use stripe state snapshot in break_stripe_batch_list()
From: yu kuai @ 2026-06-20 21:38 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260618134748.1168360-1-chencheng@fnnas.com>
在 2026/6/18 21:47, Chen Cheng 写道:
> From: Chen Cheng<chencheng@fnnas.com>
>
> The patch just suppress KCSAN noise. No functional change.
>
> RAID-5 can group multi full-stripe-write aka stripe_head into a
> batch aka batch_list, with one head_sh leading them. Call
> break_stripe_batch_list() when the batch is finished, or,
> a stripe has to be dropped out of the batch.
>
> break_stripe_batch_list() reads stripe state several times while
> request paths can update thost state words concurrently with
> lockless bitops, which reported by KCSAN.
>
> Use a snapshot to guarantees that the value used for
> warning, copying, and handle checks is internally consistent
> at current read moment.
>
> KCSAN report:
> ==============================================
> BUG: KCSAN: data-race in __add_stripe_bio / break_stripe_batch_list
>
> write (marked) to 0xffff8e89d4f0b988 of 8 bytes by task 4323 on cpu 3:
> __add_stripe_bio+0x35e/0x400
> raid5_make_request+0x6ac/0x2930
> md_handle_request+0x4a2/0xa40
> md_submit_bio+0x109/0x1a0
> __submit_bio+0x2ec/0x390
> submit_bio_noacct_nocheck+0x457/0x710
> submit_bio_noacct+0x2a7/0xc20
> submit_bio+0x56/0x250
> blkdev_direct_IO+0x54c/0xda0
> blkdev_write_iter+0x38f/0x570
> aio_write+0x22b/0x490
> io_submit_one+0xa51/0xf70
>
> read to 0xffff8e89d4f0b988 of 8 bytes by task 4290 on cpu 4:
> break_stripe_batch_list+0x3ce/0x480
> handle_stripe_clean_event+0x720/0x9b0
> handle_stripe+0x32fb/0x4500
> handle_active_stripes.isra.0+0x6e0/0xa50
> raid5d+0x7e0/0xba0
>
> Signed-off-by: Chen Cheng<chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 45 ++++++++++++++++++++++++++-------------------
> 1 file changed, 26 insertions(+), 19 deletions(-)
Applied to md-7.2
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: avoid R5_Overlap races while breaking stripe batches
From: yu kuai @ 2026-06-20 21:51 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: chenchneg33, linux-kernel
In-Reply-To: <20260619041013.1207148-1-chencheng@fnnas.com>
在 2026/6/19 12:10, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> KCSAN report a race in break_stripe_batch_list() vs. raid5_make_request()
> on sh->dev[i].flags (plain word write vs. atomic bit op)..
>
> and .. one possible scenario is:
>
> CPU1 CPU2
> break_stripe_batch_list(sh1)
> -> handle sh2
> -> lock(sh2)
> -> sh2->batch_head = NULL
> -> unlock(sh2)
> -> test_and_clear_bit(R5_Overlap, sh2->dev[i].flags)
> -> wake_up_bit(sh2->dev[i].flags)
> raid5_make_request()
> -> add_all_stripe_bios(sh2)
> -> lock(sh2)
> -> stripe_bio_overlaps(sh2) returns true
> batch_head is NULL, so new bio overlap
> exist bio on sh2 -> true
> -> set_bit(R5_Overlap, sh2->dev[i].flags)
> -> unlock(sh2)
> -> wait_on_bit(sh2->dev[i].flags)
> -> sh2->dev[i].flags = sh1->dev[i].flags & ~R5_Overlap
>
> No wait_up_bit(), CPU2 could be wait_on_bit() forever...
>
> Fix by :
> - Expand the protect zone.
> - Use batch_head's device flag's snaphot when no held head_sh->stripe_lock.
> - Move sh/head_sh->batch_head = NULL to the end of protected zone , and ,
> any concurrent add_all_stripe_bios() grabs sh->stripe_lock now either:
> - see batch_head != null, and , is rejected by stripe_bio_overlaps()
> under the lock (no R5_Overlap wait ) , or ,
> - sees batch_head == NULL, only after dev[i].flags has already been
> set and the prior R5_Overlap waiters worken.
>
> KCSAN report:
> ================================================
> BUG: KCSAN: data-race in break_stripe_batch_list / raid5_make_request
>
> write (marked) to 0xffff8e89c8117548 of 8 bytes by task 4042 on cpu 0:
> raid5_make_request+0xea0/0x2930
> md_handle_request+0x4a2/0xa40
> md_submit_bio+0x109/0x1a0
> __submit_bio+0x2ec/0x390
> submit_bio_noacct_nocheck+0x457/0x710
> submit_bio_noacct+0x2a7/0xc20
> submit_bio+0x56/0x250
> blkdev_direct_IO+0x54c/0xda0
> blkdev_write_iter+0x38f/0x570
> aio_write+0x22b/0x490
> io_submit_one+0xa51/0xf70
> __x64_sys_io_submit+0xf7/0x220
> x64_sys_call+0x1907/0x1c60
> do_syscall_64+0x130/0x570
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> read to 0xffff8e89c8117548 of 8 bytes by task 4010 on cpu 5:
> break_stripe_batch_list+0x249/0x480
> handle_stripe_clean_event+0x720/0x9b0
> handle_stripe+0x32fb/0x4500
> handle_active_stripes.isra.0+0x6e0/0xa50
> raid5d+0x7e0/0xba0
> md_thread+0x15a/0x2d0
> kthread+0x1e3/0x220
> ret_from_fork+0x37a/0x410
> ret_from_fork_asm+0x1a/0x30
>
> value changed: 0x0000000000000019 -> 0x0000000000000099 --> R5_Overlap
>
> Fixes: fb642b92c267b
Fix this tag and applied to md-7.2
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a376560be92e..5521051a9425 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -4939,30 +4939,30 @@ static void break_stripe_batch_list(struct stripe_head *head_sh,
> state & (1 << STRIPE_INSYNC));
>
> sh->check_state = head_sh->check_state;
> sh->reconstruct_state = head_sh->reconstruct_state;
> spin_lock_irq(&sh->stripe_lock);
> - sh->batch_head = NULL;
> - spin_unlock_irq(&sh->stripe_lock);
> for (i = 0; i < sh->disks; i++) {
> if (test_and_clear_bit(R5_Overlap, &sh->dev[i].flags))
> wake_up_bit(&sh->dev[i].flags, R5_Overlap);
> - sh->dev[i].flags = head_sh->dev[i].flags &
> + sh->dev[i].flags = READ_ONCE(head_sh->dev[i].flags) &
> (~((1 << R5_WriteError) | (1 << R5_Overlap)));
> }
> + sh->batch_head = NULL;
> + spin_unlock_irq(&sh->stripe_lock);
>
> state = READ_ONCE(sh->state);
> if (handle_flags == 0 || (state & handle_flags))
> set_bit(STRIPE_HANDLE, &sh->state);
> raid5_release_stripe(sh);
> }
> spin_lock_irq(&head_sh->stripe_lock);
> - head_sh->batch_head = NULL;
> - spin_unlock_irq(&head_sh->stripe_lock);
> for (i = 0; i < head_sh->disks; i++)
> if (test_and_clear_bit(R5_Overlap, &head_sh->dev[i].flags))
> wake_up_bit(&head_sh->dev[i].flags, R5_Overlap);
> + head_sh->batch_head = NULL;
> + spin_unlock_irq(&head_sh->stripe_lock);
>
> state = READ_ONCE(head_sh->state);
> if (state & handle_flags)
> set_bit(STRIPE_HANDLE, &head_sh->state);
> }
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md/raid5: read batch_head under stripe_lock in make_stripe_request
From: yu kuai @ 2026-06-20 22:01 UTC (permalink / raw)
To: Chen Cheng, linux-raid, yukuai; +Cc: linux-kernel
In-Reply-To: <20260619081109.1218112-1-chencheng@fnnas.com>
Hi,
在 2026/6/19 16:11, Chen Cheng 写道:
> From: Chen Cheng <chencheng@fnnas.com>
>
> KCSAN reports race in raid5_make_request() vs. stripe_add_to_batch_list()
>
> Writer flow (stripe_add_to_batch_list):
> 1. grab `head` stripe;
> 2. lock_two_stripes(head, sh);
> 3. re-check stripe_can_batch() for both head and sh, which requires
> STRIPE_BATCH_READY set on both;
> 4. write head->batch_head = head and sh->batch_head = head;
> 5. unlock_two_stripes.
>
> STRIPE_BATCH_READY is cleared in two places:
> - clear_batch_ready(), at the entry of handle_stripe();
> - __add_stripe_bio(), for non-batchable bios.
> And, both need to acquire `stripe_lock`.
>
> Under stripe_lock, if STRIPE_BATCH_READY is clear, then:
> - New writers cannot install a batch_head;
> - Existing writers have already finished.
> So .. handle_stripe() readers can ready `batch_head` locklessly.
This does not explain the race clearly, I still have no clue yet.
>
> Fix way:
> Writer side make_stripe_request() under STRIPE_BATCH_READY, so , need
> to be protected by stripe_lock when read something..
>
> v1 -> v2:
> - re-expalin how stripe_lock and batch_head work in commit message , and ,
> - modify comment in raid5.h.
>
> Fixs: f4aec6a097387
Weird fix tag again.
>
>
> KCSAN report:
> ======================================
> BUG: KCSAN: data-race in raid5_make_request / raid5_make_request
>
> write to 0xffff8f03062432d8 of 8 bytes by task 210246 on cpu 6:
> raid5_make_request+0x175e/0x2ab0
> md_handle_request+0x2c5/0x700
> md_submit_bio+0x126/0x320
> [.........]
> btrfs_sync_file+0x181/0x970
> vfs_fsync_range+0x71/0x110
> do_fsync+0x46/0xa0
> __x64_sys_fsync+0x20/0x30
>
> read to 0xffff8f03062432d8 of 8 bytes by task 210251 on cpu 0:
> raid5_make_request+0x7c7/0x2ab0
> md_handle_request+0x2c5/0x700
> md_submit_bio+0x126/0x320
> [.........]
> btrfs_remap_file_range+0x266/0x980
> vfs_clone_file_range+0x16d/0x610
> ioctl_file_clone+0x64/0xd0
> do_vfs_ioctl+0x87f/0xbc0
> __x64_sys_ioctl+0xb8/0x130
>
> value changed: 0x0000000000000000 -> 0xffff8f0307798728
Is this a mismatch report?
>
>
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
> ---
> drivers/md/raid5.c | 2 ++
> drivers/md/raid5.h | 8 +++++++-
> 2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 5521051a9425..efc63740f867 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6108,14 +6108,16 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
> ctx->do_flush = false;
> }
>
> set_bit(STRIPE_HANDLE, &sh->state);
> clear_bit(STRIPE_DELAYED, &sh->state);
> + spin_lock_irq(&sh->stripe_lock);
> if ((!sh->batch_head || sh == sh->batch_head) &&
> (bi->bi_opf & REQ_SYNC) &&
> !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> atomic_inc(&conf->preread_active_stripes);
> + spin_unlock_irq(&sh->stripe_lock);
>
> release_stripe_plug(mddev, sh);
> return STRIPE_SUCCESS;
>
> out_release:
> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 1c7b710fc9c1..9ff825697ba3 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -221,11 +221,17 @@ struct stripe_head {
> enum reconstruct_states reconstruct_state;
> spinlock_t stripe_lock;
> int cpu;
> struct r5worker_group *group;
>
> - struct stripe_head *batch_head; /* protected by stripe lock */
> + /*
> + * Writer protected by stripe_lock.
> + * Reader hold stripe_lock when STRIPE_BATCH_READY is set.
> + * Without STRIPE_BATCH_READY means no concurrent write,
> + * lockless read is ok.
> + */
> + struct stripe_head *batch_head;
> spinlock_t batch_lock; /* only header's lock is useful */
> struct list_head batch_list; /* protected by head's batch lock*/
>
> union {
> struct r5l_io_unit *log_io;
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v2 2/4] md/raid10: fix writes_pending leak on write request failures
From: Abd-Alrhman Masalkhi @ 2026-06-21 11:05 UTC (permalink / raw)
To: yu kuai, sashiko-reviews; +Cc: linux-raid, yukuai
In-Reply-To: <ecaef394-23a2-43ec-a85b-2e25786914aa@fygo.io>
Hi Kaui,
On Sun, Jun 21, 2026 at 02:25 +0800, yu kuai wrote:
> Hi,
>
> 在 2026/6/14 2:40, sashiko-bot@kernel.org 写道:
>> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>>
>> Pre-existing issues:
>> - [High] The `r10_bio` structure allocated from the mempool is leaked when `raid10_write_request()` bails out early due to `REQ_NOWAIT`.
>> --
> This looks like a real issue, do you plan to fix this as well?
Yes, I plan to fix that issue as well. I'll include this fix along with
the other patches.
>>
>> commit 3940ccdb4c7c7e19c20f9b5e2e1d12545c2558b5
>> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>>
>> md/raid10: fix writes_pending leak on write request failures
>>
>> raid10_make_request() acquires a writes_pending reference with
>> md_write_start() before dispatching write requests. Several failure
>> paths in raid10_write_request() complete the bio and return without
>> reaching the normal write completion path, causing the corresponding
>> md_write_end() to be skipped.
>>
>>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>>> index cee5a253a281d..c123a8c76ddcd 100644
>>> --- a/drivers/md/raid10.c
>>> +++ b/drivers/md/raid10.c
>> [ ... ]
>>> @@ -1365,7 +1365,7 @@ static void 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);
>>> - return;
>>> + return false;
>> [Severity: High]
>> This isn't a bug introduced by this patch, but does this early return leak
>> the r10_bio structure?
>>
>> __make_request() allocates r10_bio using:
>> r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
>>
>> It then passes it to raid10_write_request(). When the bio has REQ_NOWAIT set
>> during a clustered area resync, we complete the bio with an error and return
>> false without freeing the r10_bio. Could repeatedly triggering this exhaust
>> the r10bio_pool, causing deadlocks on subsequent operations?
>>
>> Note that the regular_request_wait() error path right below correctly calls
>> free_r10bio(r10_bio) before returning.
>>
>>> }
>>> for (;;) {
>> [ ... ]
>>> @@ -1398,7 +1398,7 @@ static void raid10_write_request(struct mddev *mddev, struct bio *bio,
>>> if (bio->bi_opf & REQ_NOWAIT) {
>>> allow_barrier(conf);
>>> bio_wouldblock_error(bio);
>>> - return;
>>> + return false;
>> [Severity: High]
>> This is a pre-existing issue, but does this path also leak the r10_bio
>> mempool allocation when waiting for a reshape metadata update?
>>
>> Similar to the clustered area resync case above, the r10_bio allocated in
>> __make_request() does not appear to be freed before returning here.
>>
>>> }
>>> mddev_add_trace_msg(conf->mddev,
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH] md/raid1: honor REQ_NOWAIT when waiting for behind writes
From: Abd-Alrhman Masalkhi @ 2026-06-21 18:08 UTC (permalink / raw)
To: Yu Kuai, song, yukuai, magiclinan, xiao, vverma, axboe
Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <61361f20-f81c-46c3-bdd2-3de24e90a0aa@fnnas.com>
Hi Kuai,
On Sun, Jun 21, 2026 at 04:18 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/6/11 16:35, Abd-Alrhman Masalkhi 写道:
>> raid1 supports REQ_NOWAIT reads by avoiding waits in the barrier path
>> through wait_read_barrier(). However, a read can still block on a
>> WriteMostly device when the array uses a bitmap and there are
>> outstanding behind writes.
>>
>> In that case raid1 unconditionally calls wait_behind_writes(), which
>> may sleep until all behind writes complete. As a result, a REQ_NOWAIT
>> read can block despite the caller explicitly requesting non-blocking
>> behavior.
>>
>> This ensures that raid1 consistently honors REQ_NOWAIT reads across all
>> paths that may otherwise wait for behind writes.
>>
>> Fixes: 5aa705039c4f ("md: raid1 add nowait support")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> drivers/md/md-bitmap.c | 9 +++++++--
>> drivers/md/md-bitmap.h | 2 +-
>> drivers/md/md-llbitmap.c | 13 ++++++++-----
>> drivers/md/md.c | 2 +-
>> drivers/md/raid1.c | 10 +++++++---
>> 5 files changed, 24 insertions(+), 12 deletions(-)
>
> Applied to md-7.2.
>
> One note, remove nowait support is already in my to-do lists. There is a case
> that can't be handled for now, for example:
>
> 2 disk raid1, issue no wait write IO, rdev1 succeed while rdev2 failed. In this
> case, we don't know if rdev2 failed issue because nowait or disk really have
> badblocks. So we can't either record badblocks or issue again without nowait,
> for consequence, rdev1 and rdev2 will end up with different data.
>
> I think the best solution is to remove nowait support for mdraid. Feel free
> to cook a patch or if you have something else in mind.
>
What if on a partial nowait failure we just end the master bio as
success, but set NEEDED on that chunk's bitmap counter and start_sync()
picks it up for resync? That way we don't have to decide why rdev2
failed at all, resync just copies from rdev1 to rdev2 without nowait,
so if it's a real bad block, end_sync_write() records it then.
We kinda have the idea of ending the bio before the write lands on all
mirrors in write-behind already, though it's not quite the same, there
the deferred write still lands, here we ACK after it already failed and
lean on resync to redo it.
My worry is the loaded case: AGAIN under queue pressure isn't that rare,
so we might end up triggering resync frequently. Do you think that's
acceptable, or is dropping nowait better? Happy to prototype either way.
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index 028b9ca8ce52..1206e31f323a 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -2063,18 +2063,23 @@ static void bitmap_end_behind_write(struct mddev *mddev)
>> bitmap->mddev->bitmap_info.max_write_behind);
>> }
>>
>> -static void bitmap_wait_behind_writes(struct mddev *mddev)
>> +static bool bitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
>> {
>> struct bitmap *bitmap = mddev->bitmap;
>>
>> /* wait for behind writes to complete */
>> if (bitmap && atomic_read(&bitmap->behind_writes) > 0) {
>> + if (nowait)
>> + return false;
>> +
>> pr_debug("md:%s: behind writes in progress - waiting to stop.\n",
>> mdname(mddev));
>> /* need to kick something here to make sure I/O goes? */
>> wait_event(bitmap->behind_wait,
>> atomic_read(&bitmap->behind_writes) == 0);
>> }
>> +
>> + return true;
>> }
>>
>> static void bitmap_destroy(struct mddev *mddev)
>> @@ -2084,7 +2089,7 @@ static void bitmap_destroy(struct mddev *mddev)
>> if (!bitmap) /* there was no bitmap */
>> return;
>>
>> - bitmap_wait_behind_writes(mddev);
>> + bitmap_wait_behind_writes(mddev, false);
>> if (!test_bit(MD_SERIALIZE_POLICY, &mddev->flags))
>> mddev_destroy_serial_pool(mddev, NULL);
>>
>> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
>> index 214f623c7e79..f46674bdfeb9 100644
>> --- a/drivers/md/md-bitmap.h
>> +++ b/drivers/md/md-bitmap.h
>> @@ -98,7 +98,7 @@ struct bitmap_operations {
>>
>> void (*start_behind_write)(struct mddev *mddev);
>> void (*end_behind_write)(struct mddev *mddev);
>> - void (*wait_behind_writes)(struct mddev *mddev);
>> + bool (*wait_behind_writes)(struct mddev *mddev, bool nowait);
>>
>> md_bitmap_fn *start_write;
>> md_bitmap_fn *end_write;
>> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
>> index 1adc5b117821..5a4e2abaa757 100644
>> --- a/drivers/md/md-llbitmap.c
>> +++ b/drivers/md/md-llbitmap.c
>> @@ -1574,16 +1574,19 @@ static void llbitmap_end_behind_write(struct mddev *mddev)
>> wake_up(&llbitmap->behind_wait);
>> }
>>
>> -static void llbitmap_wait_behind_writes(struct mddev *mddev)
>> +static bool llbitmap_wait_behind_writes(struct mddev *mddev, bool nowait)
>> {
>> struct llbitmap *llbitmap = mddev->bitmap;
>>
>> - if (!llbitmap)
>> - return;
>> + if (llbitmap && atomic_read(&llbitmap->behind_writes) > 0) {
>> + if (nowait)
>> + return false;
>>
>> - wait_event(llbitmap->behind_wait,
>> - atomic_read(&llbitmap->behind_writes) == 0);
>> + wait_event(llbitmap->behind_wait,
>> + atomic_read(&llbitmap->behind_writes) == 0);
>> + }
>>
>> + return true;
>> }
>>
>> static ssize_t bits_show(struct mddev *mddev, char *page)
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 096bb64e87bd..d1465bcd86c8 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -7050,7 +7050,7 @@ EXPORT_SYMBOL_GPL(md_stop_writes);
>> static void mddev_detach(struct mddev *mddev)
>> {
>> if (md_bitmap_enabled(mddev, false))
>> - mddev->bitmap_ops->wait_behind_writes(mddev);
>> + mddev->bitmap_ops->wait_behind_writes(mddev, false);
>> if (mddev->pers && mddev->pers->quiesce && !is_md_suspended(mddev)) {
>> mddev->pers->quiesce(mddev, 1);
>> mddev->pers->quiesce(mddev, 0);
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index b1ed4cc6ade4..b2d7c13b64bd 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -1341,6 +1341,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> int max_sectors;
>> int rdisk;
>> bool r1bio_existed = !!r1_bio;
>> + bool nowait = bio->bi_opf & REQ_NOWAIT;
>>
>> /*
>> * An md cloned bio indicates we are in the error path.
>> @@ -1360,8 +1361,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> * Still need barrier for READ in case that whole
>> * array is frozen.
>> */
>> - if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
>> - bio->bi_opf & REQ_NOWAIT)) {
>> + if (!wait_read_barrier(conf, bio->bi_iter.bi_sector, nowait)) {
>> bio_wouldblock_error(bio);
>> return;
>> }
>> @@ -1402,7 +1402,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> * over-take any writes that are 'behind'
>> */
>> mddev_add_trace_msg(mddev, "raid1 wait behind writes");
>> - mddev->bitmap_ops->wait_behind_writes(mddev);
>> + if (!mddev->bitmap_ops->wait_behind_writes(mddev, nowait)) {
>> + bio_wouldblock_error(bio);
>> + set_bit(R1BIO_Returned, &r1_bio->state);
>> + goto err_handle;
>> + }
>> }
>>
>> if (max_sectors < bio_sectors(bio)) {
>
> --
> Thanks,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] md/raid5: protect bitmap batch counters aka seq_flush/seq_write consistency
From: Chen Cheng @ 2026-06-22 1:52 UTC (permalink / raw)
To: yukuai, linux-raid; +Cc: chenchneg33, linux-kernel
In-Reply-To: <d7011132-d213-4dea-9c1d-c8fd9d6e93e0@fygo.io>
在 2026/6/21 05:13, yu kuai 写道:
> Hi,
>
> 在 2026/6/17 22:28, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> kcsan detect race :
>> - raid5d() closes the current bitmap batch by updating
>> conf->seq_flush under conf->device_lock.
>> - __add_stripe_bio() read conf->seq_flush without that
>> lock when assigning sh->bm_seq.
>>
>> so, protect seq_flush/seq_write consistency for multiple CPUs by
>> READ_ONCE()/WRITE_ONCE() under the path without held device_lock.
>>
>> re-explain the stripe batch sequence number update flow:
>> 1. sh->bm_seq declare which batch number the stripe belongs to
>> when perform bitmap-related write.
>> ==> bm_seq = seq_flush+1
>>
>> 2. stripe be handled,
>> * if sh->bm_seq - conf->seq_write > 0, means the
>> batch stripes **newer than** the last written
>> batch, it cannot proceed yet, queued on bitmap_list.
>> * otherwise , has already proceed.
>>
>> 3. raid5d() `++seq_flush` to closes the current batch, means
>> * no more stripes join that old batch
>> * just-closed batch ready to write-out to disk
>>
>> 4. raid5d() calls bitmap hooks unplug() or writeout, then,
>> `++seq_write` to the same as bm_seq.
>>
>> - seq_flush - for producer, to close batches.
>> - seq_write - for consumer, the checkpoint number.
>>
>> the report:
>> ====================================
>> BUG: KCSAN: data-race in __add_stripe_bio / raid5d
>>
>> write to 0xffff88ba5625d470 of 4 bytes by task 82401 on cpu 0:
>> raid5d+0x1d9/0xba0
>> [.....]
>>
>> read to 0xffff88ba5625d470 of 4 bytes by task 82421 on cpu 8:
>> __add_stripe_bio+0x332/0x400
>> raid5_make_request+0x6ac/0x2930
>> md_handle_request+0x4a2/0xa40
>> md_submit_bio+0x109/0x1a0
>> __submit_bio+0x2ec/0x390
>> [.....]
>>
>> v1 -> v2:
>> - remove WRITE_ONCE(conf->seq_write) in held device_lock path.
>> - remove READ_ONCE(conf->seq_flush) in held device_lock path.
>>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>> ---
>> drivers/md/raid5.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index a320b71d7117..b2c5a1930841 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -3536,11 +3536,11 @@ static void __add_stripe_bio(struct stripe_head *sh, struct bio *bi,
>> pr_debug("added bi b#%llu to stripe s#%llu, disk %d, logical %llu\n",
>> (*bip)->bi_iter.bi_sector, sh->sector, dd_idx,
>> sh->dev[dd_idx].sector);
>>
>> if (conf->mddev->bitmap && firstwrite && !sh->batch_head) {
>> - sh->bm_seq = conf->seq_flush+1;
>> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>> }
>>
>> /*
>> @@ -5827,11 +5827,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
>> md_write_inc(mddev, bi);
>> sh->overwrite_disks++;
>> }
>> spin_unlock_irq(&sh->stripe_lock);
>> if (conf->mddev->bitmap) {
>> - sh->bm_seq = conf->seq_flush + 1;
>> + sh->bm_seq = READ_ONCE(conf->seq_flush) + 1;
>> set_bit(STRIPE_BIT_DELAY, &sh->state);
>> }
>>
>> set_bit(STRIPE_HANDLE, &sh->state);
>> clear_bit(STRIPE_DELAYED, &sh->state);
>> @@ -6877,16 +6877,17 @@ static void raid5d(struct md_thread *thread)
>> clear_bit(R5_DID_ALLOC, &conf->cache_state);
>>
>> if (
>> !list_empty(&conf->bitmap_list)) {
>> /* Now is a good time to flush some bitmap updates */
>> - conf->seq_flush++;
>> + int seq = conf->seq_flush + 1;
>
> checkpatch should warn this about missing blank line after declaration.
ok.
>
>> + WRITE_ONCE(conf->seq_flush, seq);
>
> WRITE_ONCE(a, a + 1) is safe, you can see lots of examples.
Gotta re-use `conf->seq_flush` snapshot aka. `seq` below..
>
>> spin_unlock_irq(&conf->device_lock);
>> if (md_bitmap_enabled(mddev, true))
>> mddev->bitmap_ops->unplug(mddev, true);
>> spin_lock_irq(&conf->device_lock);
>> - conf->seq_write = conf->seq_flush;
>> + conf->seq_write = seq;
>> activate_bit_delay(conf, conf->temp_inactive_list);
>> }
>> raid5_activate_delayed(conf);
>>
>> while ((bio = remove_bio_from_retry(conf, &offset))) {
>
^ permalink raw reply
* Re: [PATCH v2] md/raid5: read batch_head under stripe_lock in make_stripe_request
From: Chen Cheng @ 2026-06-22 3:08 UTC (permalink / raw)
To: yukuai, linux-raid; +Cc: linux-kernel
In-Reply-To: <e958cb97-556a-4320-8e76-9aed18a435bc@fygo.io>
在 2026/6/21 06:01, yu kuai 写道:
> Hi,
>
> 在 2026/6/19 16:11, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> KCSAN reports race in raid5_make_request() vs. stripe_add_to_batch_list()
>>
>> Writer flow (stripe_add_to_batch_list):
>> 1. grab `head` stripe;
>> 2. lock_two_stripes(head, sh);
>> 3. re-check stripe_can_batch() for both head and sh, which requires
>> STRIPE_BATCH_READY set on both;
>> 4. write head->batch_head = head and sh->batch_head = head;
>> 5. unlock_two_stripes.
>>
>> STRIPE_BATCH_READY is cleared in two places:
>> - clear_batch_ready(), at the entry of handle_stripe();
>> - __add_stripe_bio(), for non-batchable bios.
>> And, both need to acquire `stripe_lock`.
>>
>> Under stripe_lock, if STRIPE_BATCH_READY is clear, then:
>> - New writers cannot install a batch_head;
>> - Existing writers have already finished.
>> So .. handle_stripe() readers can ready `batch_head` locklessly.
>
> This does not explain the race clearly, I still have no clue yet.
From the semantic correctness perspective, I think the lock is needed.
From the race consequence perspective, the worst consequence I can see
is that it could add to a batch member stripe. But
`conf->preread_active_stripes` should only add to batch head or lone stripe.
the scenario:
=========================
sh1 and sh2 are neighbor, wich means,
if sh1 start with sector X, then, sh2 start with sectorX + STRIPE_SECTORS,
CPU0 CPU1
make_stripe_request(sh2)
-> add_all_stripe_bios(sh2) make_stripe_request(sh2)
-> add_all_stripe_bios(sh2)
-> stripe_add_to_batch_list(sh2)
-> lock_two_stripes(sh1, sh2)
-> sh1->batch_head = sh1
-> sh2->batch_head = sh1
-> test_and_clear_bit(
STRIPE_PREREAD_ACTIVE,
&sh2->state)
-> unlock_two_stripes(sh1, sh2)
-> if ((!sh2->batch_head ||
sh2 == sh2->batch_head) &&
REQ_SYNC &&
!test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh2->state))
atomic_inc(&conf->preread_active_stripes)
After CPU2 batches 'sh', CPU1 can still treat it as a lone stripe and
charge preread_active_stripes. Since CPU2 has already run the follower
side compensation, the later increment has no matching decrement.
>
>>
>> Fix way:
>> Writer side make_stripe_request() under STRIPE_BATCH_READY, so , need
>> to be protected by stripe_lock when read something..
>>
>> v1 -> v2:
>> - re-expalin how stripe_lock and batch_head work in commit message , and ,
>> - modify comment in raid5.h.
>>
>> Fixs: f4aec6a097387
>
> Weird fix tag again.
>
>>
>>
>> KCSAN report:
>> ======================================
>> BUG: KCSAN: data-race in raid5_make_request / raid5_make_request
>>
>> write to 0xffff8f03062432d8 of 8 bytes by task 210246 on cpu 6:
>> raid5_make_request+0x175e/0x2ab0
>> md_handle_request+0x2c5/0x700
>> md_submit_bio+0x126/0x320
>> [.........]
>> btrfs_sync_file+0x181/0x970
>> vfs_fsync_range+0x71/0x110
>> do_fsync+0x46/0xa0
>> __x64_sys_fsync+0x20/0x30
>>
>> read to 0xffff8f03062432d8 of 8 bytes by task 210251 on cpu 0:
>> raid5_make_request+0x7c7/0x2ab0
>> md_handle_request+0x2c5/0x700
>> md_submit_bio+0x126/0x320
>> [.........]
>> btrfs_remap_file_range+0x266/0x980
>> vfs_clone_file_range+0x16d/0x610
>> ioctl_file_clone+0x64/0xd0
>> do_vfs_ioctl+0x87f/0xbc0
>> __x64_sys_ioctl+0xb8/0x130
>>
>> value changed: 0x0000000000000000 -> 0xffff8f0307798728
>
> Is this a mismatch report?
>
>>
>>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>> ---
>> drivers/md/raid5.c | 2 ++
>> drivers/md/raid5.h | 8 +++++++-
>> 2 files changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index 5521051a9425..efc63740f867 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -6108,14 +6108,16 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
>> ctx->do_flush = false;
>> }
>>
>> set_bit(STRIPE_HANDLE, &sh->state);
>> clear_bit(STRIPE_DELAYED, &sh->state);
>> + spin_lock_irq(&sh->stripe_lock);
>> if ((!sh->batch_head || sh == sh->batch_head) &&
>> (bi->bi_opf & REQ_SYNC) &&
>> !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>> atomic_inc(&conf->preread_active_stripes);
>> + spin_unlock_irq(&sh->stripe_lock);
>>
>> release_stripe_plug(mddev, sh);
>> return STRIPE_SUCCESS;
>>
>> out_release:
>> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
>> index 1c7b710fc9c1..9ff825697ba3 100644
>> --- a/drivers/md/raid5.h
>> +++ b/drivers/md/raid5.h
>> @@ -221,11 +221,17 @@ struct stripe_head {
>> enum reconstruct_states reconstruct_state;
>> spinlock_t stripe_lock;
>> int cpu;
>> struct r5worker_group *group;
>>
>> - struct stripe_head *batch_head; /* protected by stripe lock */
>> + /*
>> + * Writer protected by stripe_lock.
>> + * Reader hold stripe_lock when STRIPE_BATCH_READY is set.
>> + * Without STRIPE_BATCH_READY means no concurrent write,
>> + * lockless read is ok.
>> + */
>> + struct stripe_head *batch_head;
>> spinlock_t batch_lock; /* only header's lock is useful */
>> struct list_head batch_list; /* protected by head's batch lock*/
>>
>> union {
>> struct r5l_io_unit *log_io;
>
^ permalink raw reply
* Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs
From: Chen Cheng @ 2026-06-22 6:34 UTC (permalink / raw)
To: yukuai, linux-raid; +Cc: linux-kernel
In-Reply-To: <21def978-3c6e-4a9f-8c60-c213e760fb7c@fygo.io>
在 2026/6/21 04:02, yu kuai 写道:
> Hi,
>
> 在 2026/6/3 11:59, Chen Cheng 写道:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> After reshape changes raid_disks, an in-flight r10bio from the old geometry
>> can still be completed or freed later. In that case, using the current
>> geometry to walk r10_bio->devs[] is unsafe. A failure was reproduced with a
>> simple write workload while reshaping a raid10 array from 4 disks to 5 disks.
>> e.g.:
>>
>> mdadm -C /dev/md777 -l10 -n4 /dev/sda /dev/sdb /dev/sdc /dev/sdd
>> mkfs.ext4 /dev/md777
>> mount /dev/md777 /mnt/test
>> fsstress -d /mnt/test -n 24000 -p 8 -l 24 &
>> mdadm /dev/md777 --add /dev/sde
>> mdadm --grow /dev/md777 --raid-devices=5 \
>> --backup-file=/tmp/md-reshape-backup
>>
>> the sequence above can trigger:
>>
>> BUG: KASAN: slab-out-of-bounds in free_r10bio+0x1c4/0x260 [raid10]
>> Read of size 8 at addr ffff00008c2dfac8 by task ksoftirqd/0/15
>> free_r10bio
>> raid_end_bio_io
>> one_write_done
>> raid10_end_write_request
>>
>> The buggy object was 200 bytes long, which matches an r10bio with space for
>> only four devs[] entries. However, put_all_bios() and find_bio_disk() walk
>> r10_bio->devs[] using the current conf->geo.raid_disks value. Once reshape
>> switches conf->geo.raid_disks from 4 to 5, an old 4-slot r10bio can be
>> completed or freed as if it had 5 slots, and the walk overruns devs[4]. The
> I don't understand, is this still possible after patch 1.
patch 1 and 2 just prepare work.
The true dangerous is in-flight r10bio which use old geometry.
And patch 3 record and use r10bio's own width to free r10bio's bios.
>> same stale-width mismatch can also surface during a 5-disk to 4-disk reshape.
>>
>> Track the number of valid devs[] entries in each reused r10bio with
>> used_nr_devs. Initialize it whenever an r10bio is prepared for regular I/O,
>> discard, or resync/recovery/reshape work, and use it to bound devs[] walks
>> in put_all_bios() and find_bio_disk().
>>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>> ---
>> drivers/md/raid10.c | 8 ++++++--
>> drivers/md/raid10.h | 2 ++
>> 2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
>> index 5eca34432e63..f134b93fd593 100644
>> --- a/drivers/md/raid10.c
>> +++ b/drivers/md/raid10.c
>> @@ -273,11 +273,11 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
>>
>> static void put_all_bios(struct r10conf *conf, struct r10bio *r10_bio)
>> {
>> int i;
>>
>> - for (i = 0; i < conf->geo.raid_disks; i++) {
>> + for (i = 0; i < r10_bio->used_nr_devs; i++) {
>> struct bio **bio = & r10_bio->devs[i].bio;
>> if (!BIO_SPECIAL(*bio))
>> bio_put(*bio);
>> *bio = NULL;
>> bio = &r10_bio->devs[i].repl_bio;
>> @@ -370,11 +370,11 @@ static int find_bio_disk(struct r10conf *conf, struct r10bio *r10_bio,
>> struct bio *bio, int *slotp, int *replp)
>> {
>> int slot;
>> int repl = 0;
>>
>> - for (slot = 0; slot < conf->geo.raid_disks; slot++) {
>> + for (slot = 0; slot < r10_bio->used_nr_devs; slot++) {
>> if (r10_bio->devs[slot].bio == bio)
>> break;
>> if (r10_bio->devs[slot].repl_bio == bio) {
>> repl = 1;
>> break;
>> @@ -1561,10 +1561,11 @@ static void __make_request(struct mddev *mddev, struct bio *bio, int sectors)
>>
>> r10_bio->mddev = mddev;
>> r10_bio->sector = bio->bi_iter.bi_sector;
>> r10_bio->state = 0;
>> r10_bio->read_slot = -1;
>> + r10_bio->used_nr_devs = conf->geo.raid_disks;
>> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
>> conf->geo.raid_disks);
>>
>> if (bio_data_dir(bio) == READ)
>> raid10_read_request(mddev, bio, r10_bio);
>> @@ -1749,10 +1750,11 @@ static int raid10_handle_discard(struct mddev *mddev, struct bio *bio)
>> r10_bio = mempool_alloc(conf->r10bio_pool, GFP_NOIO);
>> r10_bio->mddev = mddev;
>> r10_bio->state = 0;
>> r10_bio->sectors = 0;
>> r10_bio->read_slot = -1;
>> + r10_bio->used_nr_devs = geo->raid_disks;
>> memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) * geo->raid_disks);
>> wait_blocked_dev(mddev, r10_bio);
>>
>> /*
>> * For far layout it needs more than one r10bio to cover all regions.
>> @@ -3083,10 +3085,12 @@ static struct r10bio *raid10_alloc_init_r10buf(struct r10conf *conf)
>> test_bit(MD_RECOVERY_RESHAPE, &conf->mddev->recovery))
>> nalloc = conf->copies; /* resync */
>> else
>> nalloc = 2; /* recovery */
>>
>> + r10bio->used_nr_devs = nalloc;
>> +
>> for (i = 0; i < nalloc; i++) {
>> bio = r10bio->devs[i].bio;
>> rp = bio->bi_private;
>> bio_reset(bio, NULL, 0);
>> bio->bi_private = rp;
>> diff --git a/drivers/md/raid10.h b/drivers/md/raid10.h
>> index b711626a5db7..4751119f9770 100644
>> --- a/drivers/md/raid10.h
>> +++ b/drivers/md/raid10.h
>> @@ -125,10 +125,12 @@ struct r10bio {
>> struct bio *master_bio;
>> /*
>> * if the IO is in READ direction, then this is where we read
>> */
>> int read_slot;
>> + /* Used to bound devs[] walks when the object is reused. */
>> + unsigned int used_nr_devs;
>>
>> struct list_head retry_list;
>> /*
>> * if the IO is in WRITE direction, then multiple bios are used,
>> * one for each copy.
>
^ permalink raw reply
* Re: [PATCH v4 3/3] md/raid10: bound reused r10bio devs[] walks by used_nr_devs
From: yu kuai @ 2026-06-22 7:53 UTC (permalink / raw)
To: Chen Cheng, linux-raid; +Cc: linux-kernel, yukuai
In-Reply-To: <f877d9f4-4912-4331-8c14-e7726adf24bc@fnnas.com>
Hi，
在 2026/6/22 14:34, Chen Cheng 写道:
> The true dangerous is in-flight r10bio which use old geometry.
You didn't explain why there can be inflight r10bio when array is suspended,
if this can happen, this is the root cause need to be fixed.
--
Thanks,
Kuai
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox