* 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] 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 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 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
* [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
* [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
* 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
* 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
* [PATCH] md/raid1: protect head_position for read balance
From: Chen Cheng @ 2026-06-19 4:41 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
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(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 5b9368bd9e70..4cdf4484cab6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -357,12 +357,12 @@ static void raid_end_bio_io(struct r1bio *r1_bio)
*/
static inline void update_head_pos(int disk, struct r1bio *r1_bio)
{
struct r1conf *conf = r1_bio->mddev->private;
- conf->mirrors[disk].head_position =
- r1_bio->sector + (r1_bio->sectors);
+ WRITE_ONCE(conf->mirrors[disk].head_position,
+ r1_bio->sector + r1_bio->sectors);
}
/*
* Find the disk number which triggered given bio
*/
@@ -735,11 +735,11 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
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;
}
/*
* If buffered sequential IO size exceeds optimal iosize, check if there is idle
* disk. If yes, choose the idle disk.
@@ -812,11 +812,12 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
/* At least two disks to choose from so failfast is OK */
if (ctl.readable_disks++ == 1)
set_bit(R1BIO_FailFast, &r1_bio->state);
pending = atomic_read(&rdev->nr_pending);
- dist = abs(r1_bio->sector - conf->mirrors[disk].head_position);
+ dist = abs(r1_bio->sector -
+ READ_ONCE(conf->mirrors[disk].head_position));
/* Don't change to another disk for sequential reads */
if (is_sequential(conf, disk, r1_bio)) {
if (!should_choose_next(conf, disk))
return disk;
--
2.54.0
^ permalink raw reply related
* [PATCH] md/raid5: avoid R5_Overlap races while breaking stripe batches
From: Chen Cheng @ 2026-06-19 4:10 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
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
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);
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Eric Biggers @ 2026-06-18 17:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260618092224.GB17530@lst.de>
On Thu, Jun 18, 2026 at 11:22:24AM +0200, Christoph Hellwig wrote:
> On Wed, Jun 17, 2026 at 05:14:48PM +0000, Eric Biggers wrote:
> > The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
> > tests do:
> >
> > if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
> > kunit_skip(test, "not enabled");
>
> I saw that, but what's the point or just not compiling it?
Just the usual point of avoiding #ifdefs. Developers won't have to
build the file with two different kconfigs to ensure it builds.
- Eric
^ permalink raw reply
* [PATCH] md/raid5: use stripe state snapshot in break_stripe_batch_list()
From: Chen Cheng @ 2026-06-18 13:47 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, linux-kernel
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(-)
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.
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 12:30 UTC (permalink / raw)
To: Paul Menzel, Chen Cheng; +Cc: linux-raid, yukuai, yukuai, linux-kernel
In-Reply-To: <946e31d4-1ea8-4c65-be0a-0f942a9cdf8c@molgen.mpg.de>
在 2026/6/18 20:15, Paul Menzel 写道:
> Dear Chen,
>
>
> Am 18.06.26 um 13:26 schrieb Chen Cheng:
>> 在 2026/6/18 18:36, Paul Menzel 写道:
>
>>> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>>>> From: Chen Cheng <chencheng@fnnas.com>
>>>>
>>>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>>>
>>>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>>>> when bm_seq >= seq_write.
>>>>
>>>> after raid5d() flushes bitmap update and ++seq_write, and
>>>> active_bit_delay() retry to release delayed stripes.
>>>>
>>>> the stripe batch head must carry the newest bm_seq among all
>>>> member stripes, because the whole batch later released according
>>>> to the batch head state and bm_seq.
>>>>
>>>> race scenario:
>>>> ===================
>>>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>>>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>>>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>>>
>>>> the point is, if the head has a lower bm_seq than one of its
>>>> members, the whole batch could be released before that
>>>> member's bitmap is flushed.
>>>> and the on-disk bitmap not record sh1's changes.
>>>
>>> It’s a little hard to read. Could you please improve the wording of the
>>> last paragraph, and maybe also start each sentence with a capital
>>> letter. Maybe also use 75 characters per line.
>>>
>>> Do you have a reproducer by any chance?
>
>> Thanks to review, and , I will follow your advise.
>
> Thank you for reading my comments.
>
>> Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
>> for this one. Because it's reported by sashiko-review bot, and, I think
>> it's a true risk.
>
> Maybe also mention sashiko-review.
>
>> I will try to make a reproducer for this case later, after I figure-out
>> the other KCSAN reports.
>
> A reproducer is not required, I was just interested, how the issue was
> found. So don’t spend too much on it or at all.
Well , I think a good reproducer has to be :
- Concurrency as much path as possible ,
e.g. reshape concurrency with normal I/O, etc...
Optionally ,
- Run some Sanitizer,
- Inject some fault, e.g. bad block, write error, disk drop...
- Provide some background I/O pressure,
- Do some combinations describes above
>
> […]
>
>
> Kind regards,
>
> Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Paul Menzel @ 2026-06-18 12:15 UTC (permalink / raw)
To: Chen Cheng; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <76dd7a7b-b1ad-4f61-9eda-5957f712ed87@fnnas.com>
Dear Chen,
Am 18.06.26 um 13:26 schrieb Chen Cheng:
> 在 2026/6/18 18:36, Paul Menzel 写道:
>> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>>> From: Chen Cheng <chencheng@fnnas.com>
>>>
>>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>>
>>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>>> when bm_seq >= seq_write.
>>>
>>> after raid5d() flushes bitmap update and ++seq_write, and
>>> active_bit_delay() retry to release delayed stripes.
>>>
>>> the stripe batch head must carry the newest bm_seq among all
>>> member stripes, because the whole batch later released according
>>> to the batch head state and bm_seq.
>>>
>>> race scenario:
>>> ===================
>>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>>
>>> the point is, if the head has a lower bm_seq than one of its
>>> members, the whole batch could be released before that
>>> member's bitmap is flushed.
>>> and the on-disk bitmap not record sh1's changes.
>>
>> It’s a little hard to read. Could you please improve the wording of the
>> last paragraph, and maybe also start each sentence with a capital
>> letter. Maybe also use 75 characters per line.
>>
>> Do you have a reproducer by any chance?
> Thanks to review, and , I will follow your advise.
Thank you for reading my comments.
> Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
> for this one. Because it's reported by sashiko-review bot, and, I think
> it's a true risk.
Maybe also mention sashiko-review.
> I will try to make a reproducer for this case later, after I figure-out
> the other KCSAN reports.
A reproducer is not required, I was just interested, how the issue was
found. So don’t spend too much on it or at all.
[…]
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 11:26 UTC (permalink / raw)
To: Paul Menzel; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <5e6ce9e2-4da9-4ded-be8c-fad3ee153d90@molgen.mpg.de>
在 2026/6/18 18:36, Paul Menzel 写道:
> Dear Chen,
>
>
> Thank you for very much.
>
> Am 18.06.26 um 08:55 schrieb Chen Cheng:
>> From: Chen Cheng <chencheng@fnnas.com>
>>
>> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>>
>> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
>> when bm_seq >= seq_write.
>>
>> after raid5d() flushes bitmap update and ++seq_write, and
>> active_bit_delay() retry to release delayed stripes.
>>
>> the stripe batch head must carry the newest bm_seq among all
>> member stripes, because the whole batch later released according
>> to the batch head state and bm_seq.
>>
>> race scenario:
>> ===================
>> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
>> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
>> 3. cpu1 write 102, and cpu0 overwrite with 101;
>>
>> the point is, if the head has a lower bm_seq than one of its
>> members, the whole batch could be released before that
>> member's bitmap is flushed.
>> and the on-disk bitmap not record sh1's changes.
>
> It’s a little hard to read. Could you please improve the wording of the
> last paragraph, and maybe also start each sentence with a capital
> letter. Maybe also use 75 characters per line.
>
> Do you have a reproducer by any chance?
Hi Paul,
Thanks to review, and , I will follow your advise.
Actually, I have some reproducer to hit KCSAN reports in RAID-5, but not
for this one. Because it's reported by sashiko-review bot, and , I think
it's a true risk.
I will try to make a reproducer for this case later , after I figure-out
the other KCSAN reports.
>
>> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
>
> Also add a Fixes: tag?
>
>> ---
>> drivers/md/raid5.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index a08230aac711..ee145a7bf9e8 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct
>> r5conf *conf,
>> /*
>> * at this point, head's BATCH_READY could be cleared, but we
>> * can still add the stripe to batch list
>> */
>> list_add(&sh->batch_list, &head->batch_list);
>> - spin_unlock(&head->batch_head->batch_lock);
>> } else {
>> head->batch_head = head;
>> sh->batch_head = head->batch_head;
>> spin_lock(&head->batch_lock);
>> list_add_tail(&sh->batch_list, &head->batch_list);
>> - spin_unlock(&head->batch_lock);
>> }
>> - if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>> - if (atomic_dec_return(&conf->preread_active_stripes)
>> - < IO_THRESHOLD)
>> - md_wakeup_thread(conf->mddev->thread);
>> -
>> if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
>> int seq = sh->bm_seq;
>> if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
>> sh->batch_head->bm_seq - seq > 0)
>> seq = sh->batch_head->bm_seq;
>> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
>> sh->batch_head->bm_seq = seq;
>> }
>> + spin_unlock(&head->batch_head->batch_lock);
>> +
>> + if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>> + if (atomic_dec_return(&conf->preread_active_stripes)
>> + < IO_THRESHOLD)
>> + md_wakeup_thread(conf->mddev->thread);
>> atomic_inc(&sh->count);
>> unlock_out:
>> unlock_two_stripes(head, sh);
>> out:
>
>
> Kind regards,
>
> Paul
^ permalink raw reply
* Re: [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Paul Menzel @ 2026-06-18 10:36 UTC (permalink / raw)
To: Chen Cheng; +Cc: linux-raid, yukuai, yukuai, chenchneg33, linux-kernel
In-Reply-To: <20260618065544.954309-1-chencheng@fnnas.com>
Dear Chen,
Thank you for very much.
Am 18.06.26 um 08:55 schrieb Chen Cheng:
> From: Chen Cheng <chencheng@fnnas.com>
>
> bm_seq means "stripe delay to flush until bm_seq <= seq_write".
>
> do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
> when bm_seq >= seq_write.
>
> after raid5d() flushes bitmap update and ++seq_write, and
> active_bit_delay() retry to release delayed stripes.
>
> the stripe batch head must carry the newest bm_seq among all
> member stripes, because the whole batch later released according
> to the batch head state and bm_seq.
>
> race scenario:
> ===================
> 1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
> 2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
> 3. cpu1 write 102, and cpu0 overwrite with 101;
>
> the point is, if the head has a lower bm_seq than one of its
> members, the whole batch could be released before that
> member's bitmap is flushed.
> and the on-disk bitmap not record sh1's changes.
It’s a little hard to read. Could you please improve the wording of the
last paragraph, and maybe also start each sentence with a capital
letter. Maybe also use 75 characters per line.
Do you have a reproducer by any chance?
> Signed-off-by: Chen Cheng <chencheng@fnnas.com>
Also add a Fixes: tag?
> ---
> drivers/md/raid5.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a08230aac711..ee145a7bf9e8 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> /*
> * at this point, head's BATCH_READY could be cleared, but we
> * can still add the stripe to batch list
> */
> list_add(&sh->batch_list, &head->batch_list);
> - spin_unlock(&head->batch_head->batch_lock);
> } else {
> head->batch_head = head;
> sh->batch_head = head->batch_head;
> spin_lock(&head->batch_lock);
> list_add_tail(&sh->batch_list, &head->batch_list);
> - spin_unlock(&head->batch_lock);
> }
>
> - if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> - if (atomic_dec_return(&conf->preread_active_stripes)
> - < IO_THRESHOLD)
> - md_wakeup_thread(conf->mddev->thread);
> -
> if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
> int seq = sh->bm_seq;
> if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
> sh->batch_head->bm_seq - seq > 0)
> seq = sh->batch_head->bm_seq;
> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
> sh->batch_head->bm_seq = seq;
> }
> + spin_unlock(&head->batch_head->batch_lock);
> +
> + if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
> + if (atomic_dec_return(&conf->preread_active_stripes)
> + < IO_THRESHOLD)
> + md_wakeup_thread(conf->mddev->thread);
>
> atomic_inc(&sh->count);
> unlock_out:
> unlock_two_stripes(head, sh);
> out:
Kind regards,
Paul
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: David Laight @ 2026-06-18 9:59 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Eric Biggers, Andrew Morton, linux-kernel, linux-crypto, x86,
linux-raid
In-Reply-To: <20260618092500.GC17530@lst.de>
On Thu, 18 Jun 2026 11:25:00 +0200
Christoph Hellwig <hch@lst.de> wrote:
> On Wed, Jun 17, 2026 at 11:05:16AM +0100, David Laight wrote:
> > > FYI, one or 2 sources are basically useless as they RAID5 configs
> > > that have no benefits over simple mirroring and thus the numbers
> > > aren't too interesting.
> >
> > With three disks you xor two buffers (src_count == 1) to get the parity
> > to write to the third - so that is a valid RAID5 config.
>
> Sure. It did not say it is invalid, it just isn't very useful.
>
It gives you 2/3 the total capacity, mirroring only gives 1/2.
But for a lot of uses I suspect disks are cheap enough that mirroring
is usually the 'best bet'.
It also means that there are a set of catastrophic failures that can
be more easily recovered.
It is probably more important to ensure all the disks in a raid set
come from different production lines - otherwise they are likely to
fail at the same time.
At least modern disks don't (seem to) suffer from 'sticky oil' which
would tend to stop them restarting after they got cold while turned off.
Raid doesn't help when multiple disks fail to start!
David
^ permalink raw reply
* Re: [PATCH v3] lib/raid/xor: x86: Add AVX-512 optimized xor_gen()
From: Christoph Hellwig @ 2026-06-18 9:25 UTC (permalink / raw)
To: David Laight
Cc: Christoph Hellwig, Eric Biggers, Andrew Morton, linux-kernel,
linux-crypto, x86, linux-raid
In-Reply-To: <20260617110516.0a70950e@pumpkin>
On Wed, Jun 17, 2026 at 11:05:16AM +0100, David Laight wrote:
> > FYI, one or 2 sources are basically useless as they RAID5 configs
> > that have no benefits over simple mirroring and thus the numbers
> > aren't too interesting.
>
> With three disks you xor two buffers (src_count == 1) to get the parity
> to write to the third - so that is a valid RAID5 config.
Sure. It did not say it is invalid, it just isn't very useful.
^ permalink raw reply
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Christoph Hellwig @ 2026-06-18 9:22 UTC (permalink / raw)
To: Eric Biggers; +Cc: Christoph Hellwig, Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260617171448.GD785086@google.com>
On Wed, Jun 17, 2026 at 05:14:48PM +0000, Eric Biggers wrote:
> The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
> tests do:
>
> if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
> kunit_skip(test, "not enabled");
I saw that, but what's the point or just not compiling it?
> > + static_assert(ARRAY_SIZE(len_to_test) == 2);
> > + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> > + nr, speed[0], speed[1]);
>
> As mentioned in the other thread, this measures the speed at which the
> source data is consumed, which differs from the code in
> lib/raid/xor/xor-core.c that measures the speed at which the destination
> data is produced. Probably best to make them consistent.
The throughput for the generated parity isn't very interesting. It is
the overall throughput that determines the performance. And yes,
the selection benchmark is a complete mess - the single disk XOR is
not a relevant performance metric.
^ permalink raw reply
* [PATCH] md/raid5: protect batch_head->bm_seq updates
From: Chen Cheng @ 2026-06-18 6:55 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
bm_seq means "stripe delay to flush until bm_seq <= seq_write".
do_release_stripe() keeps STRIPE_BIT_DELAY stripes on bitmap_list
when bm_seq >= seq_write.
after raid5d() flushes bitmap update and ++seq_write, and
active_bit_delay() retry to release delayed stripes.
the stripe batch head must carry the newest bm_seq among all
member stripes, because the whole batch later released according
to the batch head state and bm_seq.
race scenario:
===================
1. cpu0 - sh0->bm_seq=101; cpu1 - sh1->bm_seq=102;
2. both cpu0 and cpu1 read batch_head->bm_seq = 100;
3. cpu1 write 102, and cpu0 overwrite with 101;
the point is, if the head has a lower bm_seq than one of its
members, the whole batch could be released before that
member's bitmap is flushed.
and the on-disk bitmap not record sh1's changes.
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid5.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a08230aac711..ee145a7bf9e8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -980,32 +980,31 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
/*
* at this point, head's BATCH_READY could be cleared, but we
* can still add the stripe to batch list
*/
list_add(&sh->batch_list, &head->batch_list);
- spin_unlock(&head->batch_head->batch_lock);
} else {
head->batch_head = head;
sh->batch_head = head->batch_head;
spin_lock(&head->batch_lock);
list_add_tail(&sh->batch_list, &head->batch_list);
- spin_unlock(&head->batch_lock);
}
- if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
- if (atomic_dec_return(&conf->preread_active_stripes)
- < IO_THRESHOLD)
- md_wakeup_thread(conf->mddev->thread);
-
if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
int seq = sh->bm_seq;
if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
sh->batch_head->bm_seq - seq > 0)
seq = sh->batch_head->bm_seq;
set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
sh->batch_head->bm_seq = seq;
}
+ spin_unlock(&head->batch_head->batch_lock);
+
+ if (test_and_clear_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
+ if (atomic_dec_return(&conf->preread_active_stripes)
+ < IO_THRESHOLD)
+ md_wakeup_thread(conf->mddev->thread);
atomic_inc(&sh->count);
unlock_out:
unlock_two_stripes(head, sh);
out:
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: Chen Cheng @ 2026-06-18 3:41 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260618031232.831A41F000E9@smtp.kernel.org>
在 2026/6/18 11:12, sashiko-bot@kernel.org 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] Wrap-safe sequence comparison introduces signed integer overflow, triggering UBSAN.
>
> Pre-existing issues:
> - [High] Concurrent lockless read-modify-write of `batch_head->bm_seq` can result in lost bitmap flush sequence updates.
> --
>
> commit 38cbf25fbeef1204525d320e539132e2704c3653
> Author: Chen Cheng <chencheng@fnnas.com>
>
> md/raid5: let stripe batch bm_seq comparison wrap-safe
>
> This commit alters the comparison of sequence numbers in the RAID5
> stripe batching path to correctly handle wrap-around by using subtraction.
>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index d0d7f96fd7cd9..5882670f7d0c9 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -996,7 +996,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
>> if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
>> int seq = sh->bm_seq;
>> if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
>
> [Severity: High]
> This isn't a bug introduced by this patch, but it appears that the shared
> batch_head->bm_seq is being accessed and updated locklessly here. If multiple
> threads concurrently append stripes to different parts of the batch list,
> could Thread A and Thread B concurrently read batch_head->bm_seq, compare it,
> and write it back, causing a race?
>
> An unguarded read-modify-write might allow a thread writing an older
> sequence number to overwrite a newer one written by another thread. If bm_seq
> gets overwritten with an older value, could the batch start processing before
> the newer bitmap flush completes, potentially risking data corruption on power
> loss?
>
>> - sh->batch_head->bm_seq > seq)
>> + sh->batch_head->bm_seq - seq > 0)
>
good catch..
> [Severity: Medium]
> Since both sh->batch_head->bm_seq and seq are signed ints, does this wrap-safe
> comparison introduce a signed integer overflow when the sequence wraps?
>
> While the kernel is compiled with -fno-strict-overflow so it functions as
> intended, subtracting two signed integers whose difference exceeds the
> representable range of a 32-bit signed integer can trigger Undefined Behavior
> Sanitizer (UBSAN) splats.
>
> Could this be written by casting to unsigned before subtraction and back to
> signed, such as (s32)((u32)a - (u32)b) > 0, to avoid UBSAN warnings and
> potential kernel panics on environments with panic_on_warn?
>
>> seq = sh->batch_head->bm_seq;
>> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
>> sh->batch_head->bm_seq = seq;
>
I am fine with both way .. raid5 use the template "a - b > 0" directly
for a long time
e.g. "sh->bm_seq - conf->seq_write > 0" in do_release_stripe()
(s32)((u32)a - (u32)b) > 0 is more rigorous, and "a - b > 0" much cleaner .
^ permalink raw reply
* Re: [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: sashiko-bot @ 2026-06-18 3:12 UTC (permalink / raw)
To: Chen Cheng; +Cc: yukuai, linux-raid
In-Reply-To: <20260618025735.915113-1-chencheng@fnnas.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Wrap-safe sequence comparison introduces signed integer overflow, triggering UBSAN.
Pre-existing issues:
- [High] Concurrent lockless read-modify-write of `batch_head->bm_seq` can result in lost bitmap flush sequence updates.
--
commit 38cbf25fbeef1204525d320e539132e2704c3653
Author: Chen Cheng <chencheng@fnnas.com>
md/raid5: let stripe batch bm_seq comparison wrap-safe
This commit alters the comparison of sequence numbers in the RAID5
stripe batching path to correctly handle wrap-around by using subtraction.
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d0d7f96fd7cd9..5882670f7d0c9 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -996,7 +996,7 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
> if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
> int seq = sh->bm_seq;
> if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
[Severity: High]
This isn't a bug introduced by this patch, but it appears that the shared
batch_head->bm_seq is being accessed and updated locklessly here. If multiple
threads concurrently append stripes to different parts of the batch list,
could Thread A and Thread B concurrently read batch_head->bm_seq, compare it,
and write it back, causing a race?
An unguarded read-modify-write might allow a thread writing an older
sequence number to overwrite a newer one written by another thread. If bm_seq
gets overwritten with an older value, could the batch start processing before
the newer bitmap flush completes, potentially risking data corruption on power
loss?
> - sh->batch_head->bm_seq > seq)
> + sh->batch_head->bm_seq - seq > 0)
[Severity: Medium]
Since both sh->batch_head->bm_seq and seq are signed ints, does this wrap-safe
comparison introduce a signed integer overflow when the sequence wraps?
While the kernel is compiled with -fno-strict-overflow so it functions as
intended, subtracting two signed integers whose difference exceeds the
representable range of a 32-bit signed integer can trigger Undefined Behavior
Sanitizer (UBSAN) splats.
Could this be written by casting to unsigned before subtraction and back to
signed, such as (s32)((u32)a - (u32)b) > 0, to avoid UBSAN warnings and
potential kernel panics on environments with panic_on_warn?
> seq = sh->batch_head->bm_seq;
> set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
> sh->batch_head->bm_seq = seq;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260618025735.915113-1-chencheng@fnnas.com?part=1
^ permalink raw reply
* [PATCH] md/raid5: let stripe batch bm_seq comparison wrap-safe
From: Chen Cheng @ 2026-06-18 2:57 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
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(-)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 6d01c35b5499..a08230aac711 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -997,11 +997,11 @@ static void stripe_add_to_batch_list(struct r5conf *conf,
md_wakeup_thread(conf->mddev->thread);
if (test_and_clear_bit(STRIPE_BIT_DELAY, &sh->state)) {
int seq = sh->bm_seq;
if (test_bit(STRIPE_BIT_DELAY, &sh->batch_head->state) &&
- sh->batch_head->bm_seq > seq)
+ sh->batch_head->bm_seq - seq > 0)
seq = sh->batch_head->bm_seq;
set_bit(STRIPE_BIT_DELAY, &sh->batch_head->state);
sh->batch_head->bm_seq = seq;
}
--
2.54.0
^ permalink raw reply related
* [PATCH] md/raid10: protect head_position accesses consistency
From: Chen Cheng @ 2026-06-18 2:52 UTC (permalink / raw)
To: linux-raid, yukuai, yukuai; +Cc: chencheng, chenchneg33, linux-kernel
From: Chen Cheng <chencheng@fnnas.com>
the completion paths update conf->mirrors[x].head_position
without a lock, while read_balance() reads the same field
locklessly.
protect conf->mirror[x].head_position consistency for
multi-CPUs by READ_ONCE()/WRITE_ONCE().
the report #1:
=====================
BUG: KCSAN: data-race in find_bio_disk.isra.0 / read_balance
write to 0xffff8f0309679fd0 of 8 bytes by interrupt on cpu 9:
find_bio_disk.isra.0+0x108/0x180
raid10_end_write_request+0xd2/0x530
bio_endio+0x3c9/0x3e0
[...]
read to 0xffff8f0309679fd0 of 8 bytes by task 338766 on cpu 7:
read_balance+0x2ec/0x700
raid10_read_request+0x17b/0x550
raid10_make_request+0x27e/0x1080
md_handle_request+0x2c5/0x700
[...]
value changed: 0x000000000032f440 -> 0x000000000014f8e0
the report #2:
=======================
BUG: KCSAN: data-race in raid10_end_read_request / read_balance
write to 0xffff8f03291091b0 of 8 bytes by interrupt on cpu 9:
raid10_end_read_request+0xf0/0x400
bio_endio+0x3c9/0x3e0
[...]
read to 0xffff8f03291091b0 of 8 bytes by task 336943 on cpu 8:
read_balance+0x2ec/0x700
raid10_read_request+0x17b/0x550
raid10_make_request+0x27e/0x1080
md_handle_request+0x2c5/0x700
[...]
value changed: 0x000000000041c188 -> 0x000000000041c1b8
Signed-off-by: Chen Cheng <chencheng@fnnas.com>
---
drivers/md/raid10.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index caf1da15aa87..55aa052a957b 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -343,12 +343,12 @@ static void raid_end_bio_io(struct r10bio *r10_bio)
*/
static inline void update_head_pos(int slot, struct r10bio *r10_bio)
{
struct r10conf *conf = r10_bio->mddev->private;
- conf->mirrors[r10_bio->devs[slot].devnum].head_position =
- r10_bio->devs[slot].addr + (r10_bio->sectors);
+ WRITE_ONCE(conf->mirrors[r10_bio->devs[slot].devnum].head_position,
+ r10_bio->devs[slot].addr + (r10_bio->sectors));
}
/*
* Find the disk number which triggered given bio
*/
@@ -841,11 +841,11 @@ static struct md_rdev *read_balance(struct r10conf *conf,
/* for far > 1 always use the lowest address */
else if (geo->far_copies > 1)
new_distance = r10_bio->devs[slot].addr;
else
new_distance = abs(r10_bio->devs[slot].addr -
- conf->mirrors[disk].head_position);
+ READ_ONCE(conf->mirrors[disk].head_position));
if (new_distance < best_dist) {
best_dist = new_distance;
best_dist_slot = slot;
best_dist_rdev = rdev;
--
2.54.0
^ permalink raw reply related
* Re: [PATCH 2/2] xor/kunit: add a benchmark
From: Eric Biggers @ 2026-06-17 17:14 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Andrew Morton, linux-kernel, linux-raid
In-Reply-To: <20260617054416.3841334-3-hch@lst.de>
On Wed, Jun 17, 2026 at 07:44:04AM +0200, Christoph Hellwig wrote:
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25..5939b78b3944 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
> @@ -125,8 +125,71 @@ static void xor_test(struct kunit *test)
> }
> }
>
> +#ifdef CONFIG_XOR_BENCHMARK
> +static void xor_benchmark(struct kunit *test)
The #ifdef can be avoided using kunit_skip(), as the crypto and CRC
tests do:
if (!IS_ENABLED(CONFIG_XOR_BENCHMARK))
kunit_skip(test, "not enabled");
> + for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
> + unsigned int len = len_to_test[j];
> + const unsigned long num_iters = 1000;
> +
> + KUNIT_ASSERT_GT(test, len, 0);
> + KUNIT_ASSERT_LE(test, len, XOR_KUNIT_MAX_BYTES);
> +
> + preempt_disable();
> + t = ktime_get();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = ktime_get_ns() - t;
> + preempt_enable();
First one should be ktime_get_ns(), not ktime_get().
> +
> + speed[j] = div_u64((u64)len * num_iters * nr, t);
> + }
> +
> + static_assert(ARRAY_SIZE(len_to_test) == 2);
> + kunit_info(test, "%3u disks:\t%5llu GB/s\t%5llu GB/s\n",
> + nr, speed[0], speed[1]);
As mentioned in the other thread, this measures the speed at which the
source data is consumed, which differs from the code in
lib/raid/xor/xor-core.c that measures the speed at which the destination
data is produced. Probably best to make them consistent.
- Eric
^ 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