* Re: [PATCH] md/raid5: validate journal checksum slots during recovery
From: yu kuai @ 2026-07-10 7:37 UTC (permalink / raw)
To: Guangshuo Li, Song Liu, Li Nan, Xiao Ni, Junrui Luo, linux-raid,
linux-kernel, yu kuai
In-Reply-To: <20260708133534.770770-1-lgs201920130244@gmail.com>
Hi,
在 2026/7/8 21:35, Guangshuo Li 写道:
> The change referenced by the Fixes tag added payload length validation
> before accessing journal metadata during raid5-cache recovery.
>
> However, the DATA and PARITY payload length is computed from the on-disk
> size field without ensuring that the checksum slots read later are
> actually present. struct r5l_payload_data_parity ends with a flexible
> checksum array, so sizeof(struct r5l_payload_data_parity) does not cover
> any checksum entries.
>
> A corrupted journal can set a DATA payload size smaller than one page.
> The computed checksum count then becomes zero and the payload length only
> covers the fixed header, but recovery still reads checksum[0]. For RAID6
> PARITY payloads, recovery also reads checksum[1], so the payload must
> cover two checksum entries.
>
> Make the validated DATA and PARITY payload length include the checksum
> entries that recovery may read. Also make sure fixed payload headers are
> present before reading their fields.
>
> Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")
Does the problem this patch fixes introduced by above fix tag? Or it's just a
new validation, if so please replace with a real fix tag.
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> drivers/md/raid5-cache.c | 108 +++++++++++++++++++++++++++++----------
> 1 file changed, 82 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
> index 7b7546bfa21f..4aef692182b4 100644
> --- a/drivers/md/raid5-cache.c
> +++ b/drivers/md/raid5-cache.c
> @@ -1980,6 +1980,29 @@ r5l_recovery_verify_data_checksum(struct r5l_log *log,
> return (le32_to_cpu(log_checksum) == checksum) ? 0 : -EINVAL;
> }
>
> +static sector_t r5l_recovery_payload_data_parity_len(struct r5conf *conf,
> + const struct r5l_payload_data_parity *payload, bool parity)
> +{
> + unsigned int nr_csum;
> + unsigned int min_csum = 1;
> +
> + /*
> + * The payload size determines how many checksum entries are stored,
> + * but recovery always reads checksum[0]. For RAID6 parity payloads
> + * it also reads checksum[1] for Q. Make the validated payload length
> + * cover every checksum entry that will be read below.
> + */
> + nr_csum = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
> +
> + if (parity && conf->max_degraded == 2)
> + min_csum = 2;
> + if (nr_csum < min_csum)
> + nr_csum = min_csum;
> +
> + return sizeof(*payload) + (sector_t)sizeof(__le32) * nr_csum;
Please use struct_size()
> +}
> +
> +
Two blank line here. Please run checkpatch before submission.
> /*
> * before loading data to stripe cache, we need verify checksum for all data,
> * if there is mismatch for any data page, we drop all data in the mata block
> @@ -1992,6 +2015,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> struct r5conf *conf = mddev->private;
> struct r5l_meta_block *mb = page_address(ctx->meta_page);
> sector_t mb_offset = sizeof(struct r5l_meta_block);
> + sector_t meta_size = le32_to_cpu(mb->meta_size);
> sector_t log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
> struct page *page;
> struct r5l_payload_data_parity *payload;
> @@ -2001,28 +2025,42 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> if (!page)
> return -ENOMEM;
>
> - while (mb_offset < le32_to_cpu(mb->meta_size)) {
> + while (mb_offset < meta_size) {
> sector_t payload_len;
> + u16 type;
>
> payload = (void *)mb + mb_offset;
> payload_flush = (void *)mb + mb_offset;
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (mb_offset + sizeof(payload->header) > meta_size)
> + goto mismatch;
> +
> + type = le16_to_cpu(payload->header.type);
> +
> + if (type == R5LOG_PAYLOAD_DATA) {
> + if (mb_offset + sizeof(*payload) > meta_size)
> goto mismatch;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf,
> + payload,
> + false);
> + if (payload_len > meta_size - mb_offset)
> + goto mismatch;
> +
> if (r5l_recovery_verify_data_checksum(
> log, ctx, page, log_offset,
> payload->checksum[0]) < 0)
> goto mismatch;
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + } else if (type == R5LOG_PAYLOAD_PARITY) {
> + if (mb_offset + sizeof(*payload) > meta_size)
> goto mismatch;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf,
> + payload,
> + true);
> + if (payload_len > meta_size - mb_offset)
> + goto mismatch;
> +
> if (r5l_recovery_verify_data_checksum(
> log, ctx, page, log_offset,
> payload->checksum[0]) < 0)
> @@ -2034,15 +2072,18 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
> BLOCK_SECTORS),
> payload->checksum[1]) < 0)
> goto mismatch;
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> + } else if (type == R5LOG_PAYLOAD_FLUSH) {
> + if (mb_offset + sizeof(*payload_flush) > meta_size)
> + goto mismatch;
> +
> payload_len = sizeof(struct r5l_payload_flush) +
> (sector_t)le32_to_cpu(payload_flush->size);
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (payload_len > meta_size - mb_offset)
> goto mismatch;
> } else /* not R5LOG_PAYLOAD_DATA/PARITY/FLUSH */
> goto mismatch;
>
> - if (le16_to_cpu(payload->header.type) != R5LOG_PAYLOAD_FLUSH) {
> + if (type != R5LOG_PAYLOAD_FLUSH) {
> log_offset = r5l_ring_add(log, log_offset,
> le32_to_cpu(payload->size));
> }
> @@ -2075,7 +2116,8 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> struct r5l_meta_block *mb;
> struct r5l_payload_data_parity *payload;
> struct r5l_payload_flush *payload_flush;
> - int mb_offset;
> + sector_t mb_offset;
> + sector_t meta_size;
> sector_t log_offset;
> sector_t stripe_sect;
> struct stripe_head *sh;
> @@ -2094,22 +2136,31 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
>
> mb = page_address(ctx->meta_page);
> mb_offset = sizeof(struct r5l_meta_block);
> + meta_size = le32_to_cpu(mb->meta_size);
> log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
>
> - while (mb_offset < le32_to_cpu(mb->meta_size)) {
> + while (mb_offset < meta_size) {
> sector_t payload_len;
> + u16 type;
> int dd;
>
> payload = (void *)mb + mb_offset;
> payload_flush = (void *)mb + mb_offset;
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
> + if (mb_offset + sizeof(payload->header) > meta_size)
> + return -EINVAL;
> +
> + type = le16_to_cpu(payload->header.type);
> +
> + if (type == R5LOG_PAYLOAD_FLUSH) {
> int i, count;
>
> + if (mb_offset + sizeof(*payload_flush) > meta_size)
> + return -EINVAL;
> +
> payload_len = sizeof(struct r5l_payload_flush) +
> (sector_t)le32_to_cpu(payload_flush->size);
> - if (mb_offset + payload_len >
> - le32_to_cpu(mb->meta_size))
> + if (payload_len > meta_size - mb_offset)
> return -EINVAL;
>
> count = le32_to_cpu(payload_flush->size) / sizeof(__le64);
> @@ -2130,13 +2181,18 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> }
>
> /* DATA or PARITY payload */
> - payload_len = sizeof(struct r5l_payload_data_parity) +
> - (sector_t)sizeof(__le32) *
> - (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
> - if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
> + if (type != R5LOG_PAYLOAD_DATA && type != R5LOG_PAYLOAD_PARITY)
> + return -EINVAL;
> +
> + if (mb_offset + sizeof(*payload) > meta_size)
> + return -EINVAL;
> +
> + payload_len = r5l_recovery_payload_data_parity_len(conf, payload,
> + type == R5LOG_PAYLOAD_PARITY);
> + if (payload_len > meta_size - mb_offset)
> return -EINVAL;
>
> - stripe_sect = (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) ?
> + stripe_sect = (type == R5LOG_PAYLOAD_DATA) ?
> raid5_compute_sector(
> conf, le64_to_cpu(payload->location), 0, &dd,
> NULL)
> @@ -2183,7 +2239,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> list_add_tail(&sh->lru, cached_stripe_list);
> }
>
> - if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
> + if (type == R5LOG_PAYLOAD_DATA) {
> if (!test_bit(STRIPE_R5C_CACHING, &sh->state) &&
> test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags)) {
> r5l_recovery_replay_one_stripe(conf, sh, ctx);
> @@ -2191,7 +2247,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
> }
> r5l_recovery_load_data(log, sh, ctx, payload,
> log_offset);
> - } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
> + } else if (type == R5LOG_PAYLOAD_PARITY)
> r5l_recovery_load_parity(log, sh, ctx, payload,
> log_offset);
> else
--
Thanks,
Kuai
^ permalink raw reply
* Re: [PATCH v3 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: John Garry @ 2026-07-09 12:11 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, yukuai, magiclinan, xiao, axboe,
vverma, martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid
In-Reply-To: <20260708101341.473750-3-abd.masalkhi@gmail.com>
On 08/07/2026 11:13, Abd-Alrhman Masalkhi wrote:
> Atomic writes in RAID1 must fit within a single barrier unit. Set
> chunk_sectors to BARRIER_UNIT_SECTOR_SIZE so that bios which would cross
> a barrier-unit boundary are rejected by the block layer before reaching
> MD.
More accurate would be to say that we restrict the atomic write limits
so that we never produce a write which straddles BARRIER_UNIT_SECTOR_SIZE.
>
> A bio that passes block-layer validation may still become unserviceable
> within RAID1 due to bad blocks or write-behind constraints. In the former
> case, complete the bio with EIO. In the latter case, disable
> write-behind rather than failing the bio with EIO.
>
> Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
> Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
Reviewed-by: John Garry <john.g.garry@oracle.com>
^ permalink raw reply
* [PATCH] md/raid5: validate journal checksum slots during recovery
From: Guangshuo Li @ 2026-07-08 13:35 UTC (permalink / raw)
To: Song Liu, Yu Kuai, Li Nan, Xiao Ni, Junrui Luo, linux-raid,
linux-kernel
Cc: Guangshuo Li
The change referenced by the Fixes tag added payload length validation
before accessing journal metadata during raid5-cache recovery.
However, the DATA and PARITY payload length is computed from the on-disk
size field without ensuring that the checksum slots read later are
actually present. struct r5l_payload_data_parity ends with a flexible
checksum array, so sizeof(struct r5l_payload_data_parity) does not cover
any checksum entries.
A corrupted journal can set a DATA payload size smaller than one page.
The computed checksum count then becomes zero and the payload length only
covers the fixed header, but recovery still reads checksum[0]. For RAID6
PARITY payloads, recovery also reads checksum[1], so the payload must
cover two checksum entries.
Make the validated DATA and PARITY payload length include the checksum
entries that recovery may read. Also make sure fixed payload headers are
present before reading their fields.
Fixes: b0cc3ae97e89 ("md/raid5: validate payload size before accessing journal metadata")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
drivers/md/raid5-cache.c | 108 +++++++++++++++++++++++++++++----------
1 file changed, 82 insertions(+), 26 deletions(-)
diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
index 7b7546bfa21f..4aef692182b4 100644
--- a/drivers/md/raid5-cache.c
+++ b/drivers/md/raid5-cache.c
@@ -1980,6 +1980,29 @@ r5l_recovery_verify_data_checksum(struct r5l_log *log,
return (le32_to_cpu(log_checksum) == checksum) ? 0 : -EINVAL;
}
+static sector_t r5l_recovery_payload_data_parity_len(struct r5conf *conf,
+ const struct r5l_payload_data_parity *payload, bool parity)
+{
+ unsigned int nr_csum;
+ unsigned int min_csum = 1;
+
+ /*
+ * The payload size determines how many checksum entries are stored,
+ * but recovery always reads checksum[0]. For RAID6 parity payloads
+ * it also reads checksum[1] for Q. Make the validated payload length
+ * cover every checksum entry that will be read below.
+ */
+ nr_csum = le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9);
+
+ if (parity && conf->max_degraded == 2)
+ min_csum = 2;
+ if (nr_csum < min_csum)
+ nr_csum = min_csum;
+
+ return sizeof(*payload) + (sector_t)sizeof(__le32) * nr_csum;
+}
+
+
/*
* before loading data to stripe cache, we need verify checksum for all data,
* if there is mismatch for any data page, we drop all data in the mata block
@@ -1992,6 +2015,7 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
struct r5conf *conf = mddev->private;
struct r5l_meta_block *mb = page_address(ctx->meta_page);
sector_t mb_offset = sizeof(struct r5l_meta_block);
+ sector_t meta_size = le32_to_cpu(mb->meta_size);
sector_t log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
struct page *page;
struct r5l_payload_data_parity *payload;
@@ -2001,28 +2025,42 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
if (!page)
return -ENOMEM;
- while (mb_offset < le32_to_cpu(mb->meta_size)) {
+ while (mb_offset < meta_size) {
sector_t payload_len;
+ u16 type;
payload = (void *)mb + mb_offset;
payload_flush = (void *)mb + mb_offset;
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (mb_offset + sizeof(payload->header) > meta_size)
+ goto mismatch;
+
+ type = le16_to_cpu(payload->header.type);
+
+ if (type == R5LOG_PAYLOAD_DATA) {
+ if (mb_offset + sizeof(*payload) > meta_size)
goto mismatch;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf,
+ payload,
+ false);
+ if (payload_len > meta_size - mb_offset)
+ goto mismatch;
+
if (r5l_recovery_verify_data_checksum(
log, ctx, page, log_offset,
payload->checksum[0]) < 0)
goto mismatch;
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY) {
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ } else if (type == R5LOG_PAYLOAD_PARITY) {
+ if (mb_offset + sizeof(*payload) > meta_size)
goto mismatch;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf,
+ payload,
+ true);
+ if (payload_len > meta_size - mb_offset)
+ goto mismatch;
+
if (r5l_recovery_verify_data_checksum(
log, ctx, page, log_offset,
payload->checksum[0]) < 0)
@@ -2034,15 +2072,18 @@ r5l_recovery_verify_data_checksum_for_mb(struct r5l_log *log,
BLOCK_SECTORS),
payload->checksum[1]) < 0)
goto mismatch;
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
+ } else if (type == R5LOG_PAYLOAD_FLUSH) {
+ if (mb_offset + sizeof(*payload_flush) > meta_size)
+ goto mismatch;
+
payload_len = sizeof(struct r5l_payload_flush) +
(sector_t)le32_to_cpu(payload_flush->size);
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (payload_len > meta_size - mb_offset)
goto mismatch;
} else /* not R5LOG_PAYLOAD_DATA/PARITY/FLUSH */
goto mismatch;
- if (le16_to_cpu(payload->header.type) != R5LOG_PAYLOAD_FLUSH) {
+ if (type != R5LOG_PAYLOAD_FLUSH) {
log_offset = r5l_ring_add(log, log_offset,
le32_to_cpu(payload->size));
}
@@ -2075,7 +2116,8 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
struct r5l_meta_block *mb;
struct r5l_payload_data_parity *payload;
struct r5l_payload_flush *payload_flush;
- int mb_offset;
+ sector_t mb_offset;
+ sector_t meta_size;
sector_t log_offset;
sector_t stripe_sect;
struct stripe_head *sh;
@@ -2094,22 +2136,31 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
mb = page_address(ctx->meta_page);
mb_offset = sizeof(struct r5l_meta_block);
+ meta_size = le32_to_cpu(mb->meta_size);
log_offset = r5l_ring_add(log, ctx->pos, BLOCK_SECTORS);
- while (mb_offset < le32_to_cpu(mb->meta_size)) {
+ while (mb_offset < meta_size) {
sector_t payload_len;
+ u16 type;
int dd;
payload = (void *)mb + mb_offset;
payload_flush = (void *)mb + mb_offset;
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_FLUSH) {
+ if (mb_offset + sizeof(payload->header) > meta_size)
+ return -EINVAL;
+
+ type = le16_to_cpu(payload->header.type);
+
+ if (type == R5LOG_PAYLOAD_FLUSH) {
int i, count;
+ if (mb_offset + sizeof(*payload_flush) > meta_size)
+ return -EINVAL;
+
payload_len = sizeof(struct r5l_payload_flush) +
(sector_t)le32_to_cpu(payload_flush->size);
- if (mb_offset + payload_len >
- le32_to_cpu(mb->meta_size))
+ if (payload_len > meta_size - mb_offset)
return -EINVAL;
count = le32_to_cpu(payload_flush->size) / sizeof(__le64);
@@ -2130,13 +2181,18 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
}
/* DATA or PARITY payload */
- payload_len = sizeof(struct r5l_payload_data_parity) +
- (sector_t)sizeof(__le32) *
- (le32_to_cpu(payload->size) >> (PAGE_SHIFT - 9));
- if (mb_offset + payload_len > le32_to_cpu(mb->meta_size))
+ if (type != R5LOG_PAYLOAD_DATA && type != R5LOG_PAYLOAD_PARITY)
+ return -EINVAL;
+
+ if (mb_offset + sizeof(*payload) > meta_size)
+ return -EINVAL;
+
+ payload_len = r5l_recovery_payload_data_parity_len(conf, payload,
+ type == R5LOG_PAYLOAD_PARITY);
+ if (payload_len > meta_size - mb_offset)
return -EINVAL;
- stripe_sect = (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) ?
+ stripe_sect = (type == R5LOG_PAYLOAD_DATA) ?
raid5_compute_sector(
conf, le64_to_cpu(payload->location), 0, &dd,
NULL)
@@ -2183,7 +2239,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
list_add_tail(&sh->lru, cached_stripe_list);
}
- if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_DATA) {
+ if (type == R5LOG_PAYLOAD_DATA) {
if (!test_bit(STRIPE_R5C_CACHING, &sh->state) &&
test_bit(R5_Wantwrite, &sh->dev[sh->pd_idx].flags)) {
r5l_recovery_replay_one_stripe(conf, sh, ctx);
@@ -2191,7 +2247,7 @@ r5c_recovery_analyze_meta_block(struct r5l_log *log,
}
r5l_recovery_load_data(log, sh, ctx, payload,
log_offset);
- } else if (le16_to_cpu(payload->header.type) == R5LOG_PAYLOAD_PARITY)
+ } else if (type == R5LOG_PAYLOAD_PARITY)
r5l_recovery_load_parity(log, sh, ctx, payload,
log_offset);
else
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2] md: recheck spare changes before starting sync
From: Abd-Alrhman Masalkhi @ 2026-07-08 12:42 UTC (permalink / raw)
To: sashiko-reviews; +Cc: yukuai, linux-raid
In-Reply-To: <20260708113713.B77311F00ACF@smtp.kernel.org>
Hi Kuai,
On Wed, Jul 08, 2026 at 11:37 +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
> --
>
> --- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
> commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
> Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>
> md: recheck spare changes before starting sync
>
> This patch aims to fix a race condition where spare configurations are
> modified without suspending the array. It adds a recheck for spare
> configuration changes after acquiring the reconfig_mutex to avoid operating
> on stale states.
>
> Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)
>
> [ ... ]
>
>> mddev_lock_nointr(mddev);
>> +
>> + /*
>> + * The spare configuration can change before reconfig_mutex is acquired.
>> + * Recheck while holding the lock and suspend if needed.
>> + */
>> + if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
>> + md_spares_need_change(mddev)) {
>> + mddev_unlock(mddev);
>> + mddev_suspend_and_lock_nointr(mddev);
>> + suspend = true;
>> + }
>> +
>
> [Severity: High]
> This is a pre-existing issue, but does this recheck completely close the race
> window?
>
> If a faulty drive has pending I/O, md_spares_need_change() will evaluate
> rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
> greater than zero, md_spares_need_change() returns false. This causes both
> the initial check and this new recheck to bypass suspension.
>
> If an asynchronous I/O completion happens immediately after this recheck,
> nr_pending can decrement to 0. When md_start_sync() continues and eventually
> calls remove_spares(), it will re-evaluate rdev_removeable(), find
> nr_pending == 0, and remove the disk. This nullifies the disk pointer.
>
Shouldn't the Blocked flag guard against this case? It is set before the
Faulty bit and before nr_pending is decremented, and it should only be
cleared while holding reconfig_mutex, or am I missing something?
> Because the array was never suspended, couldn't concurrent I/O re-read the
> nullified pointer and crash? For example, in drivers/md/raid10.c:
>
> err_handle() {
> ...
> struct md_rdev *rrdev = conf->mirrors[d].replacement;
> ...
> if (r10_bio->devs[k].repl_bio) {
> rdev_dec_pending(rrdev, mddev);
> r10_bio->devs[k].repl_bio = NULL;
> }
> }
>
> If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
> will dereference it.
>
> Could remove_spares() be restricted to only remove drives if the array is
> actually suspended, or should suspension be enforced earlier regardless of
> nr_pending?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] md: recheck spare changes before starting sync
From: sashiko-bot @ 2026-07-08 11:37 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708112003.474537-1-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete fix for a TOCTOU race condition causing a NULL pointer dereference when modifying the spare configuration without suspending the array.
--
--- Patch [1]: [PATCH v2] md: recheck spare changes before starting sync ---
commit 0ddb3cd53c96b4255d51183f6d5da4fcd8b938bf
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md: recheck spare changes before starting sync
This patch aims to fix a race condition where spare configurations are
modified without suspending the array. It adds a recheck for spare
configuration changes after acquiring the reconfig_mutex to avoid operating
on stale states.
Link: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -10181,13 +10181,25 @@ static void md_start_sync(struct work_struct *ws)
[ ... ]
> mddev_lock_nointr(mddev);
> +
> + /*
> + * The spare configuration can change before reconfig_mutex is acquired.
> + * Recheck while holding the lock and suspend if needed.
> + */
> + if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
> + md_spares_need_change(mddev)) {
> + mddev_unlock(mddev);
> + mddev_suspend_and_lock_nointr(mddev);
> + suspend = true;
> + }
> +
[Severity: High]
This is a pre-existing issue, but does this recheck completely close the race
window?
If a faulty drive has pending I/O, md_spares_need_change() will evaluate
rdev_removeable(), which checks atomic_read(&rdev->nr_pending). If that is
greater than zero, md_spares_need_change() returns false. This causes both
the initial check and this new recheck to bypass suspension.
If an asynchronous I/O completion happens immediately after this recheck,
nr_pending can decrement to 0. When md_start_sync() continues and eventually
calls remove_spares(), it will re-evaluate rdev_removeable(), find
nr_pending == 0, and remove the disk. This nullifies the disk pointer.
Because the array was never suspended, couldn't concurrent I/O re-read the
nullified pointer and crash? For example, in drivers/md/raid10.c:
err_handle() {
...
struct md_rdev *rrdev = conf->mirrors[d].replacement;
...
if (r10_bio->devs[k].repl_bio) {
rdev_dec_pending(rrdev, mddev);
r10_bio->devs[k].repl_bio = NULL;
}
}
If rrdev is fetched as NULL due to the concurrent removal, rdev_dec_pending()
will dereference it.
Could remove_spares() be restricted to only remove drives if the array is
actually suspended, or should suspension be enforced earlier regardless of
nr_pending?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708112003.474537-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* [PATCH v2] md: recheck spare changes before starting sync
From: Abd-Alrhman Masalkhi @ 2026-07-08 11:20 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, abd.masalkhi
Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi, sashiko-bot
remove_spares() and remove_and_add_spares() modify the array's rdev
configuration. These operations are only safe after the array has been
suspended.
md_start_sync() checks whether spare configuration changes are needed
before taking reconfig_mutex. However, the rdev state can change before
the mutex is acquired, so the initial check can become stale. In that
case, md_choose_sync_action() may remove or replace rdevs while normal
I/O is still accessing them.
The race can occur as follows:
raid10d Worker Normal IO
____________ _______________________ ______________________
raid10_write_request()
wait_blocked_dev()
set Blocked
set Faulty
Skip Faulty rdev
rrdev->nr_pending++
.repl_bio = bio
removeable_rdev = false .
array not suspended .
lock mddev goto err_handle
lock mddev (wait)
.
update sb .
clear Blocked .
.
unlock mddev .
lock mddev (acquires)
remove_spares()
removeable_rdev = true
raid10_remove_disk()
rdev = replacement
replacement = NULL
rdev_dec_pending(NULL)
unlock mddev (NULL)->nr_pending--
In this case, rdev_dec_pending() is called with a NULL pointer,
resulting in a NULL pointer dereference when attempting to decrement
nr_pending.
Fix this by suspending the array when spare configuration changes are
needed, including for non-read-write arrays, and checking again after
taking reconfig_mutex. If the array was not already suspended and a
change is now needed, release the mutex, suspend the array, and
reacquire the mutex before continuing.
Fixes: bc08041b32ab ("md: suspend array in md_start_sync() if array need reconfiguration")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260628142420.1051027-1-abd.masalkhi@gmail.com?part=3
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v2:
- Recheck whether spare configuration changes are needed after taking
reconfig_mutex, and suspend the array before continuing if necessary.
- Account for read only arrays, where reshape_position may not be
MaxSector even though md_start_sync() can still modify the spare
configuration.
- Link to v1: https://lore.kernel.org/linux-raid/20260630075640.1081634-1-abd.masalkhi@gmail.com/
---
drivers/md/md.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 66a41d482e59..c3fc47a82415 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -10174,13 +10174,25 @@ static void md_start_sync(struct work_struct *ws)
* If reshape is still in progress, spares won't be added or removed
* from conf until reshape is done.
*/
- if (mddev->reshape_position == MaxSector &&
+ if ((mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
md_spares_need_change(mddev)) {
suspend = true;
mddev_suspend(mddev, false);
}
mddev_lock_nointr(mddev);
+
+ /*
+ * The spare configuration can change before reconfig_mutex is acquired.
+ * Recheck while holding the lock and suspend if needed.
+ */
+ if (!suspend && (mddev->reshape_position == MaxSector || !md_is_rdwr(mddev)) &&
+ md_spares_need_change(mddev)) {
+ mddev_unlock(mddev);
+ mddev_suspend_and_lock_nointr(mddev);
+ suspend = true;
+ }
+
if (!md_is_rdwr(mddev)) {
/*
* On a read-only array we can:
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: sashiko-bot @ 2026-07-08 10:33 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708101341.473750-2-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] REQ_NOWAIT bios can block waiting for blocked devices in wait_blocked_dev(), violating non-blocking semantics.
- [High] REQ_NOWAIT bios can stall during memory allocation due to hardcoded GFP_NOIO usage.
- [High] REQ_NOWAIT bios split by bio_submit_split_bioset block waiting for the barrier.
--
--- Patch [1]: [PATCH v3 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths ---
commit 3eda7e2f9e31ead6359429f927a806f3d762dc4b
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: fix r10bio leak in raid10_write_request() error paths
This commit fixes a memory leak in raid10_write_request() by freeing the
allocated r10_bio when a write request fails due to REQ_NOWAIT being set.
Closes: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
Link: https://lore.kernel.org/linux-raid/20260628142420.1051027-2-abd.masalkhi@gmail.com/
Link: https://lore.kernel.org/linux-raid/20260623072456.333437-2-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index 0a3cfdd3f5df8..bd322eccdc3ff 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
> @@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> /* Bail out if REQ_NOWAIT is set for the bio */
> if (bio->bi_opf & REQ_NOWAIT) {
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
> for (;;) {
[ ... ]
> @@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> if (bio->bi_opf & REQ_NOWAIT) {
> allow_barrier(conf);
> bio_wouldblock_error(bio);
> + free_r10bio(r10_bio);
> return false;
> }
[Severity: High]
This is a pre-existing issue, but since this patch improves REQ_NOWAIT
handling, can REQ_NOWAIT bios still block waiting for blocked devices?
In raid10_write_request(), wait_blocked_dev() is called unconditionally:
wait_blocked_dev(mddev, r10_bio);
If an rdev is blocked, wait_blocked_dev() calls md_wait_for_blocked_rdev(),
which waits up to 5 seconds via wait_event_timeout(). Should this path check
if the bio is REQ_NOWAIT to avoid stalling the submission thread?
[Severity: High]
This is another pre-existing issue, but can REQ_NOWAIT bios stall during memory
allocation due to hardcoded GFP_NOIO usage?
In __make_request(), the initial allocation uses GFP_NOIO:
r10_bio = mempool_alloc(&conf->r10bio_pool, GFP_NOIO);
And in raid10_write_one_disk(), the child bio allocation also uses GFP_NOIO:
mbio = bio_alloc_clone(rdev->bdev, bio, GFP_NOIO, &mddev->bio_set);
Under memory pressure, GFP_NOIO will trigger direct reclaim and wait for
mempool replenishment. Should these allocations be attempted with GFP_NOWAIT
and aborted with BLK_STS_AGAIN for REQ_NOWAIT bios?
[Severity: High]
This is also a pre-existing issue, but do REQ_NOWAIT bios that are split block
waiting for the barrier?
If a REQ_NOWAIT bio spans multiple chunks, it can be split. The wait_barrier()
call uses a hardcoded false for the nowait parameter:
wait_barrier(conf, false);
This will block if the array barrier is raised. Since returning BLK_STS_AGAIN
is no longer viable once the first half is submitted, is there a way to prevent
stalling REQ_NOWAIT callers on split bios?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708101341.473750-1-abd.masalkhi@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting
From: sashiko-bot @ 2026-07-08 10:31 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: yukuai, linux-raid
In-Reply-To: <20260708101341.473750-4-abd.masalkhi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] A concurrent promotion or removal of a disk causes a NULL pointer dereference in the err_handle cleanup path.
--
--- Patch [3]: [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting ---
commit 39f996c40527039ea2fa463cb02f7e201f0a4e7d
Author: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
md/raid10: consistently fail atomic writes that require splitting
RAID10 currently handles one badblock path explicitly by failing atomic
writes with EIO. However, another badblock path can also reduce the
writable range and force the bio through bio_submit_split_bioset(),
which implicitly completes the bio with EINVAL. This patch explicitly
fails atomic writes in the common split check instead.
Link: https://lore.kernel.org/linux-raid/20260628142420.1051027-4-abd.masalkhi@gmail.com/
> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> index bd322eccdc3ff..3480fc7907f01 100644
> --- a/drivers/md/raid10.c
> +++ b/drivers/md/raid10.c
[ ... ]
> @@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
> r10_bio->sectors = max_sectors;
>
> if (r10_bio->sectors < bio_sectors(bio)) {
> + if (atomic)
> + goto err_handle;
> +
[Severity: High]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if a concurrent disk removal or promotion occurs?
In the err_handle cleanup path, the replacement pointer is locklessly
re-read from the array configuration:
err_handle:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rdev = conf->mirrors[d].rdev;
struct md_rdev *rrdev = conf->mirrors[d].replacement;
If raid10_write_request() executes without holding rcu_read_lock() or
reconfig_mutex (since it can sleep), it might increment nr_pending on the
active replacement disk and set r10_bio->devs[i].repl_bio = bio.
Concurrently, if raid10_remove_disk() promotes the replacement disk, it sets
conf->mirrors[d].replacement = NULL.
When this goto is hit because an atomic write needs splitting, the lockless
re-read will fetch NULL for rrdev. Since r10_bio->devs[k].repl_bio is
non-NULL, the code calls rdev_dec_pending(rrdev, mddev), which will
dereference the NULL pointer.
> allow_barrier(conf);
> bio = bio_submit_split_bioset(bio, r10_bio->sectors,
> &conf->bio_split);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708101341.473750-1-abd.masalkhi@gmail.com?part=3
^ permalink raw reply
* [PATCH v3 7/7] md/raid10: simplify read request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
raid10_read_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Make raid10_read_request() return a status to its callers, consolidate
the read error paths, and free r10_bio from a single location in the
callers. Since the callers allocate r10_bio, they should also be
responsible for freeing it when the request fails.
This makes the read path follow the same ownership model as the write
path and simplifies the error handling flow.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-8-abd.masalkhi@gmail.com/
Changes in v2:
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-8-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 45 +++++++++++++++++++++++++--------------------
1 file changed, 25 insertions(+), 20 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index d94c1f28a6f6..01162c483644 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1143,7 +1143,7 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
return true;
}
-static void raid10_read_request(struct mddev *mddev, struct bio *bio,
+static bool raid10_read_request(struct mddev *mddev, struct bio *bio,
struct r10bio *r10_bio)
{
struct r10conf *conf = mddev->private;
@@ -1191,8 +1191,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return;
+ return false;
}
rdev = read_balance(conf, r10_bio, &max_sectors);
@@ -1202,8 +1201,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
mdname(mddev), b,
(unsigned long long)r10_bio->sector);
}
- raid_end_bio_io(r10_bio);
- return;
+ bio_io_error(bio);
+ goto err_allow_barrier;
}
if (err_rdev)
pr_err_ratelimited("md/raid10:%s: %pg: redirecting sector %llu to another mirror\n",
@@ -1215,10 +1214,8 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
wait_barrier(conf, false);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
r10_bio->sectors = max_sectors;
@@ -1244,10 +1241,16 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
read_bio->bi_private = r10_bio;
mddev_trace_remap(mddev, read_bio, r10_bio->sector);
submit_bio_noacct(read_bio);
- return;
-err_handle:
+
+ return true;
+
+err_dec_pending:
atomic_dec(&rdev->nr_pending);
- raid_end_bio_io(r10_bio);
+
+err_allow_barrier:
+ allow_barrier(conf);
+
+ return false;
}
static void raid10_write_one_disk(struct mddev *mddev, struct r10bio *r10_bio,
@@ -1538,14 +1541,13 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
memset(r10_bio->devs, 0, sizeof(r10_bio->devs[0]) *
conf->geo.raid_disks);
- ret = true;
if (bio_data_dir(bio) == READ)
- raid10_read_request(mddev, bio, r10_bio);
- else {
+ ret = raid10_read_request(mddev, bio, r10_bio);
+ else
ret = raid10_write_request(mddev, bio, r10_bio);
- if (!ret)
- free_r10bio(r10_bio);
- }
+
+ if (!ret)
+ free_r10bio(r10_bio);
return ret;
}
@@ -1875,6 +1877,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sector_t chunk_mask = (conf->geo.chunk_mask & conf->prev.chunk_mask);
int chunk_sects = chunk_mask + 1;
int sectors = bio_sectors(bio);
+ bool write = bio_data_dir(bio) == WRITE;
if (unlikely(bio->bi_opf & REQ_PREFLUSH)
&& md_flush_request(mddev, bio))
@@ -1898,7 +1901,7 @@ static bool raid10_make_request(struct mddev *mddev, struct bio *bio)
sectors = chunk_sects -
(bio->bi_iter.bi_sector &
(chunk_sects - 1));
- if (!__make_request(mddev, bio, sectors))
+ if (!__make_request(mddev, bio, sectors) && write)
md_write_end(mddev);
/* In case raid10d snuck in to freeze_array */
@@ -2866,7 +2869,9 @@ static void handle_read_error(struct mddev *mddev, struct r10bio *r10_bio)
rdev_dec_pending(rdev, mddev);
r10_bio->state = 0;
- raid10_read_request(mddev, r10_bio->master_bio, r10_bio);
+ if (!raid10_read_request(mddev, r10_bio->master_bio, r10_bio))
+ free_r10bio(r10_bio);
+
/*
* allow_barrier after re-submit to ensure no sync io
* can be issued while regular io pending.
--
2.43.0
^ permalink raw reply related
* [PATCH v3 6/7] md/raid10: simplify write request error handling
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
raid10_write_request() currently handles bio completion, barrier
handling, and r10_bio lifetime management in several different error
paths. This results in duplicated cleanup logic and increases the risk
of introducing bugs in future modifications.
Move bio_wouldblock_error() handling to the callers of
regular_request_wait(), consolidate the write error paths, and free
r10_bio from a single location in __make_request() when
raid10_write_request() fails.
It remove redundant local copies of r10_bio->sectors and use a single
max_sectors variable throughout the function.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-7-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-7-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 58 ++++++++++++++++++++++-----------------------
1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 57813f249578..d94c1f28a6f6 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1123,18 +1123,16 @@ static bool regular_request_wait(struct mddev *mddev, struct r10conf *conf,
struct bio *bio, sector_t sectors)
{
/* Bail out if REQ_NOWAIT is set for the bio */
- if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT)) {
- bio_wouldblock_error(bio);
+ if (!wait_barrier(conf, bio->bi_opf & REQ_NOWAIT))
return false;
- }
+
while (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
bio->bi_iter.bi_sector < conf->reshape_progress &&
bio->bi_iter.bi_sector + sectors > conf->reshape_progress) {
allow_barrier(conf);
- if (bio->bi_opf & REQ_NOWAIT) {
- bio_wouldblock_error(bio);
+ if (bio->bi_opf & REQ_NOWAIT)
return false;
- }
+
mddev_add_trace_msg(conf->mddev, "raid10 wait reshape");
wait_event(conf->wait_barrier,
conf->reshape_progress <= bio->bi_iter.bi_sector ||
@@ -1192,6 +1190,7 @@ static void raid10_read_request(struct mddev *mddev, struct bio *bio,
}
if (!regular_request_wait(mddev, conf, bio, r10_bio->sectors)) {
+ bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return;
}
@@ -1354,8 +1353,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
{
struct r10conf *conf = mddev->private;
int i, k;
- sector_t sectors;
- int max_sectors;
+ int max_sectors = r10_bio->sectors;
+ bool nowait = bio->bi_opf & REQ_NOWAIT;
bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
@@ -1363,9 +1362,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
/* Bail out if REQ_NOWAIT is set for the bio */
- if (bio->bi_opf & REQ_NOWAIT) {
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
return false;
}
@@ -1375,28 +1373,25 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
bio_end_sector(bio)));
}
- sectors = r10_bio->sectors;
- if (!regular_request_wait(mddev, conf, bio, sectors)) {
- free_r10bio(r10_bio);
+ if (!regular_request_wait(mddev, conf, bio, max_sectors)) {
+ bio_wouldblock_error(bio);
return false;
}
if (test_bit(MD_RECOVERY_RESHAPE, &mddev->recovery) &&
(mddev->reshape_backwards
? (bio->bi_iter.bi_sector < conf->reshape_safe &&
- bio->bi_iter.bi_sector + sectors > conf->reshape_progress)
- : (bio->bi_iter.bi_sector + sectors > conf->reshape_safe &&
+ bio->bi_iter.bi_sector + max_sectors > conf->reshape_progress)
+ : (bio->bi_iter.bi_sector + max_sectors > conf->reshape_safe &&
bio->bi_iter.bi_sector < conf->reshape_progress))) {
/* Need to update reshape_position in metadata */
mddev->reshape_position = conf->reshape_progress;
set_mask_bits(&mddev->sb_flags, 0,
BIT(MD_SB_CHANGE_DEVS) | BIT(MD_SB_CHANGE_PENDING));
md_wakeup_thread(mddev->thread);
- if (bio->bi_opf & REQ_NOWAIT) {
- allow_barrier(conf);
+ if (nowait) {
bio_wouldblock_error(bio);
- free_r10bio(r10_bio);
- return false;
+ goto err_allow_barrier;
}
mddev_add_trace_msg(conf->mddev,
"raid10 wait reshape metadata");
@@ -1421,8 +1416,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
wait_blocked_dev(mddev, r10_bio);
- max_sectors = r10_bio->sectors;
-
for (i = 0; i < conf->copies; i++) {
int d = r10_bio->devs[i].devnum;
struct md_rdev *rdev, *rrdev;
@@ -1479,15 +1472,15 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
r10_bio->sectors = max_sectors;
if (r10_bio->sectors < bio_sectors(bio)) {
- if (atomic)
- goto err_handle;
+ if (atomic) {
+ bio_io_error(bio);
+ goto err_dec_pending;
+ }
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
- if (!bio) {
- set_bit(R10BIO_Returned, &r10_bio->state);
- goto err_handle;
- }
+ if (!bio)
+ goto err_dec_pending;
r10_bio->master_bio = bio;
}
@@ -1505,7 +1498,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
one_write_done(r10_bio);
return true;
-err_handle:
+err_dec_pending:
for (k = 0; k < i; k++) {
int d = r10_bio->devs[k].devnum;
struct md_rdev *rdev = conf->mirrors[d].rdev;
@@ -1521,7 +1514,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
}
}
- raid_end_bio_io(r10_bio);
+err_allow_barrier:
+ allow_barrier(conf);
+
return false;
}
@@ -1546,8 +1541,11 @@ static bool __make_request(struct mddev *mddev, struct bio *bio, int sectors)
ret = true;
if (bio_data_dir(bio) == READ)
raid10_read_request(mddev, bio, r10_bio);
- else
+ else {
ret = raid10_write_request(mddev, bio, r10_bio);
+ if (!ret)
+ free_r10bio(r10_bio);
+ }
return ret;
}
--
2.43.0
^ permalink raw reply related
* [PATCH v3 5/7] md/raid10: replace wait loop with wait_event_idle()
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
The wait loop is equivalent to wait_event_idle() and can be simplified
by usaing it for improving readability.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-6-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-6-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 2574f60dd771..57813f249578 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1362,22 +1362,17 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
mddev->cluster_ops->area_resyncing(mddev, WRITE,
bio->bi_iter.bi_sector,
bio_end_sector(bio)))) {
- DEFINE_WAIT(w);
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
free_r10bio(r10_bio);
return false;
}
- for (;;) {
- prepare_to_wait(&conf->wait_barrier,
- &w, TASK_IDLE);
- if (!mddev->cluster_ops->area_resyncing(mddev, WRITE,
- bio->bi_iter.bi_sector, bio_end_sector(bio)))
- break;
- schedule();
- }
- finish_wait(&conf->wait_barrier, &w);
+
+ wait_event_idle(conf->wait_barrier,
+ !mddev->cluster_ops->area_resyncing(mddev, WRITE,
+ bio->bi_iter.bi_sector,
+ bio_end_sector(bio)));
}
sectors = r10_bio->sectors;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 4/7] md/raid10: remove unnecessary barrier around bio_submit_split_bioset()
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
raid10_write_request() drops the barrier before calling
bio_submit_split_bioset() and reacquires it afterwards. This is no
longer necessary because the split bio cannot re-enter
raid10_write_request() while the barrier is held.
The allow_barrier()/wait_barrier() pair was introduced by commit
e820d55cb99d ("md: fix raid10 hang issue caused by barrier") when
submit_flushes() called md_handle_request() directly, allowing re-entry
into raid10_write_request(). Since v5.2, submit_flushes() has instead
gone through submit_bio(), eliminating that recursion. submit_flushes()
was later removed entirely by commit b75197e86e6d ("md: Remove flush
handling").
Currently, raid10_write_request() is only entered from the bio
submission path, so the split bio submitted by bio_submit_split_bioset()
cannot recurse back into wait_barrier().
Remove the redundant allow_barrier()/wait_barrier() pair around
bio_submit_split_bioset().
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-5-abd.masalkhi@gmail.com/
Changes in v2:
- Expand the commit message to explain why the
allow_barrier()/wait_barrier() pair is no longer needed.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-5-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 3480fc7907f0..2574f60dd771 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1487,10 +1487,8 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (atomic)
goto err_handle;
- allow_barrier(conf);
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
- wait_barrier(conf, false);
if (!bio) {
set_bit(R10BIO_Returned, &r10_bio->state);
goto err_handle;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 3/7] md/raid10: consistently fail atomic writes that require splitting
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
RAID10 currently handles one badblock path explicitly by failing atomic
writes with EIO. However, another badblock path can also reduce the
writable range and force the bio through bio_submit_split_bioset(),
which implicitly completes the bio with EINVAL.
Fix this by handling atomic writes in the common split check. If RAID10
determines that an atomic write would require splitting, complete the
bio with EIO.
Fixes: a1d9b4fd42d9 ("md/raid10: Atomic write support")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-4-abd.masalkhi@gmail.com/
Changes in v2:
- Drop the early atomic write split check from raid10_write_request()
and rely on queue limits instead.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-4-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index bd322eccdc3f..3480fc7907f0 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1356,6 +1356,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
int i, k;
sector_t sectors;
int max_sectors;
+ bool atomic = bio->bi_opf & REQ_ATOMIC;
if ((mddev_is_clustered(mddev) &&
mddev->cluster_ops->area_resyncing(mddev, WRITE,
@@ -1464,16 +1465,6 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (is_bad) {
int good_sectors;
- /*
- * We cannot atomically write this, so just
- * error in that case. It could be possible to
- * atomically write other mirrors, but the
- * complexity of supporting that is not worth
- * the benefit.
- */
- if (bio->bi_opf & REQ_ATOMIC)
- goto err_handle;
-
good_sectors = first_bad - dev_sector;
if (good_sectors < max_sectors)
max_sectors = good_sectors;
@@ -1493,6 +1484,9 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
r10_bio->sectors = max_sectors;
if (r10_bio->sectors < bio_sectors(bio)) {
+ if (atomic)
+ goto err_handle;
+
allow_barrier(conf);
bio = bio_submit_split_bioset(bio, r10_bio->sectors,
&conf->bio_split);
--
2.43.0
^ permalink raw reply related
* [PATCH v3 2/7] md/raid1: advertise atomic write limits and handle runtime constraints
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
Atomic writes in RAID1 must fit within a single barrier unit. Set
chunk_sectors to BARRIER_UNIT_SECTOR_SIZE so that bios which would cross
a barrier-unit boundary are rejected by the block layer before reaching
MD.
A bio that passes block-layer validation may still become unserviceable
within RAID1 due to bad blocks or write-behind constraints. In the former
case, complete the bio with EIO. In the latter case, disable
write-behind rather than failing the bio with EIO.
Fixes: f2a38abf5f1c ("md/raid1: Atomic write support")
Fixes: a4c55c902670 ("md/raid1: simplify raid1_write_request() error handling")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- Set chunk_sectors to BARRIER_UNIT_SECTOR_SIZE instead of setting
atomic_write_hw_unit_max.
- Avoid enabling write-behind when the atomic write exceeds the
write-behind limit.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-3-abd.masalkhi@gmail.com/
Changes in v2:
- Drop the early atomic write split check from raid1_write_request().
- Advertise the atomic write size limit via queue limits.
- Disable write-behind instead of failing atomic writes when the
BIO_MAX_VECS limit is encountered.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-3-abd.masalkhi@gmail.com/
---
drivers/md/raid1.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index afe2ca96ad8c..6c8beca995e6 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -1522,6 +1522,7 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
int first_clone;
bool write_behind = false;
bool nowait = bio->bi_opf & REQ_NOWAIT;
+ bool atomic = bio->bi_opf & REQ_ATOMIC;
bool is_discard = op_is_discard(bio->bi_opf);
sector_t sector = bio->bi_iter.bi_sector;
@@ -1577,7 +1578,8 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
* write-mostly, which means we could allocate write behind
* bio later.
*/
- if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags))
+ if (!is_discard && rdev && test_bit(WriteMostly, &rdev->flags) &&
+ (!atomic || max_sectors <= BIO_MAX_VECS * (PAGE_SIZE >> 9)))
write_behind = true;
r1_bio->bios[i] = NULL;
@@ -1603,20 +1605,6 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
}
if (is_bad) {
int good_sectors;
-
- /*
- * We cannot atomically write this, so just
- * error in that case. It could be possible to
- * atomically write other mirrors, but the
- * complexity of supporting that is not worth
- * the benefit.
- */
- if (bio->bi_opf & REQ_ATOMIC) {
- bio->bi_status = BLK_STS_NOTSUPP;
- bio_endio(bio);
- goto err_dec_pending;
- }
-
good_sectors = first_bad - sector;
if (good_sectors < max_sectors)
max_sectors = good_sectors;
@@ -1636,7 +1624,13 @@ static bool raid1_write_request(struct mddev *mddev, struct bio *bio,
if (write_behind && mddev->bitmap)
max_sectors = min_t(int, max_sectors,
BIO_MAX_VECS * (PAGE_SIZE >> 9));
+
if (max_sectors < bio_sectors(bio)) {
+ if (atomic) {
+ bio_io_error(bio);
+ goto err_dec_pending;
+ }
+
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
if (!bio)
@@ -3228,6 +3222,7 @@ static int raid1_set_limits(struct mddev *mddev)
md_init_stacking_limits(&lim);
lim.max_write_zeroes_sectors = 0;
lim.max_hw_wzeroes_unmap_sectors = 0;
+ lim.chunk_sectors = BARRIER_UNIT_SECTOR_SIZE;
lim.logical_block_size = mddev->logical_block_size;
lim.features |= BLK_FEAT_ATOMIC_WRITES;
lim.features |= BLK_FEAT_PCI_P2PDMA;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 1/7] md/raid10: fix r10bio leak in raid10_write_request() error paths
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi, sashiko-bot
In-Reply-To: <20260708101341.473750-1-abd.masalkhi@gmail.com>
When raid10_write_request() fails because REQ_NOWAIT is set, the
allocated r10_bio is not freed before returning, resulting in a memory
leak. Free r10_bio before returning from the REQ_NOWAIT error paths.
Fixes: c9aa889b035f ("md: raid10 add nowait support")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-raid/20260613184042.BCEC01F000E9@smtp.kernel.org/
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
Changes in v3:
- No changes.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-2-abd.masalkhi@gmail.com/
Changes in v2:
- No changes.
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-2-abd.masalkhi@gmail.com/
---
drivers/md/raid10.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 0a3cfdd3f5df..bd322eccdc3f 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -1365,6 +1365,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
/* Bail out if REQ_NOWAIT is set for the bio */
if (bio->bi_opf & REQ_NOWAIT) {
bio_wouldblock_error(bio);
+ free_r10bio(r10_bio);
return false;
}
for (;;) {
@@ -1398,6 +1399,7 @@ static bool raid10_write_request(struct mddev *mddev, struct bio *bio,
if (bio->bi_opf & REQ_NOWAIT) {
allow_barrier(conf);
bio_wouldblock_error(bio);
+ free_r10bio(r10_bio);
return false;
}
mddev_add_trace_msg(conf->mddev,
--
2.43.0
^ permalink raw reply related
* [PATCH v3 0/7] md/raid10: fixes, atomic write handling, and error-path cleanup
From: Abd-Alrhman Masalkhi @ 2026-07-08 10:13 UTC (permalink / raw)
To: song, yukuai, magiclinan, xiao, axboe, vverma, john.g.garry,
martin.petersen, abd.masalkhi, linux-kernel
Cc: linux-raid, Abd-Alrhman Masalkhi
Hi,
This v3 of series contains a mix of bug fixes and cleanups for RAID10,
along with a related atomic write fix for RAID1.
Changes in v3:
- Set chunk_sectors to BARRIER_UNIT_SECTOR_SIZE instead of setting
atomic_write_hw_unit_max.
- Avoid enabling write-behind when the atomic write exceeds the
write-behind limit.
- Link to v2: https://lore.kernel.org/linux-raid/20260628142420.1051027-1-abd.masalkhi@gmail.com/
Changes in v2:
- Expand the commit message to explain why the
allow_barrier()/wait_barrier() pair is no longer needed.
- Drop the early atomic write split check from raid1_write_request().
- Advertise the atomic write size limit via queue limits.
- Disable write-behind instead of failing atomic writes when the
BIO_MAX_VECS limit is encountered.
- Drop the early atomic write split check from raid10_write_request()
and rely on queue limits instead.
- Fix a compilation error (bi -> bio).
- Link to v1: https://lore.kernel.org/linux-raid/20260623072456.333437-1-abd.masalkhi@gmail.com/
Thanks,
Abd-alrhman,
Abd-Alrhman Masalkhi (7):
md/raid10: fix r10bio leak in raid10_write_request() error paths
md/raid1: advertise atomic write limits and handle runtime constraints
md/raid10: consistently fail atomic writes that require splitting
md/raid10: remove unnecessary barrier around bio_submit_split_bioset()
md/raid10: replace wait loop with wait_event_idle()
md/raid10: simplify write request error handling
md/raid10: simplify read request error handling
drivers/md/raid1.c | 25 ++++------
drivers/md/raid10.c | 118 +++++++++++++++++++++-----------------------
2 files changed, 65 insertions(+), 78 deletions(-)
--
2.43.0
^ permalink raw reply
* Re: [PATCH 6/8] raid6: defer implementation selection when built-in
From: sashiko-bot @ 2026-07-08 9:23 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-raid, yukuai
In-Reply-To: <20260708090740.1433685-7-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The boot-time RAID6 benchmark runs unconditionally when built-in, ignoring the `CONFIG_RAID6_PQ_BENCHMARK` setting and the single-algorithm optimization.
--
--- Patch [6]: [PATCH 6/8] raid6: defer implementation selection when built-in ---
commit 2c9ec4f0b44515f49715b67c2e70f8628c5abd61
Author: Christoph Hellwig <hch@lst.de>
Subject: raid6: defer implementation selection when built-in
This commit defers the RAID6 implementation selection to avoid holding up
early boot. It follows the pattern used in the RAID5 XOR code, allowing
full use of the timing subsystem for benchmarking.
> diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
> index 6f5c89ab2b17b..e7984bde5157f 100644
> --- a/lib/raid/raid6/algos.c
> +++ b/lib/raid/raid6/algos.c
> @@ -236,16 +236,6 @@ static int __init raid6_select_algo(void)
> int i, cycle;
> int error;
>
> - if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
> - pr_info("raid6: skipped pq benchmark and selected %s\n",
> - raid6_algos[raid6_nr_algos - 1]->name);
> - static_call_update(raid6_gen_syndrome_impl,
> - raid6_algos[raid6_nr_algos - 1]->gen_syndrome);
> - static_call_update(raid6_xor_syndrome_impl,
> - raid6_algos[raid6_nr_algos - 1]->xor_syndrome);
> - return 0;
> - }
> -
> /* prepare the buffer and fill it circularly with gfmul table */
> disk_ptr = kmalloc(PAGE_SIZE * RAID6_TEST_DISKS, GFP_KERNEL);
[ ... ]
> @@ -329,13 +319,40 @@ static int __init raid6_init(void)
> static_call_update(raid6_recov_datap_impl, raid6_recov_algo->datap);
> pr_info("raid6: using %s recovery algorithm\n", raid6_recov_algo->name);
>
> + if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
> + pr_info("raid6: skipped pq benchmark and selected %s\n",
> + raid6_algos[raid6_nr_algos - 1]->name);
> + goto default_algo;
> + }
> +
> +#ifdef MODULE
> return raid6_select_algo();
> +#else
> + /*
> + * Pick the last registered implementation as the temporary default until
> + * calibration happens.
> + */
> +#endif
[ ... ]
> +/*
> + * When built-in we must register the default implementation before md
> + * initializes, but we don't want calibration to run that early as that
> + * would delay the boot process.
> + */
> +#ifndef MODULE
> +device_initcall(raid6_select_algo);
> +#endif
[Severity: Medium]
Will this unconditional device_initcall cause the benchmark to run even when
it is intentionally disabled in the configuration?
While raid6_init() checks CONFIG_RAID6_PQ_BENCHMARK and correctly skips the
early calibration phase, removing that same check from raid6_select_algo()
means this deferred call will unconditionally run the benchmarking loop later
in the boot process. It looks like this bypasses both the config setting and
the single-algorithm optimization entirely.
> subsys_initcall(raid6_init);
> module_exit(raid6_exit);
> MODULE_LICENSE("GPL");
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=6
^ permalink raw reply
* Re: [PATCH 4/8] xor/kunit: add a benchmark
From: sashiko-bot @ 2026-07-08 9:22 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: yukuai, linux-raid
In-Reply-To: <20260708090740.1433685-5-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Potential divide-by-zero kernel panic in `xor_benchmark()` when calculating the benchmark speed if the execution time is 0.
- [Medium] The benchmark holds `preempt_disable()` for the entire duration of 1000 XOR iterations, risking soft lockups on slower hardware or emulators.
--
--- Patch [4]: [PATCH 4/8] xor/kunit: add a benchmark ---
commit 4f82691a9522c9ab25bdc94cf0c1ad74dc8c3b7e
Author: Christoph Hellwig <hch@lst.de>
xor/kunit: add a benchmark
This patch adds a benchmark to test the XOR functions for more
representative block sizes and numbers of disks. It measures the execution
time of 1000 iterations of xor_gen() for varying buffer counts and lengths,
reporting the throughput in GB/s.
> diff --git a/lib/raid/xor/tests/xor_kunit.c b/lib/raid/xor/tests/xor_kunit.c
> index 659ae3edbc25b..648c6da9464c4 100644
> --- a/lib/raid/xor/tests/xor_kunit.c
> +++ b/lib/raid/xor/tests/xor_kunit.c
[ ... ]
> @@ -125,8 +125,70 @@ static void xor_test(struct kunit *test)
> }
> }
>
> +static void xor_benchmark(struct kunit *test)
> +{
[ ... ]
> + 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_ns();
> + for (l = 0; l < num_iters; l++)
> + xor_gen(test_dest, test_buffers, nr, len);
> + t = ktime_get_ns() - t;
> + preempt_enable();
[Severity: Medium]
Does this workload risk triggering the kernel's soft lockup detector?
The test executes a hardcoded 1000 iterations of xor_gen() on up to 32 buffers
of 16KB within a single preempt_disable() block. This amounts to processing
over 500MB of data atomically.
In slower environments like QEMU TCG without KVM, this large computational
workload could take tens of seconds, which might exceed the typical 20-second
soft lockup threshold.
> +
> + speed[j] = div64_u64((u64)len * num_iters * nr, t);
[Severity: High]
Can this result in a divide-by-zero kernel panic?
If the benchmark runs on a system with coarse timekeeping resolution (e.g.,
using the jiffies clocksource), and the CPU completes the 1000 iterations
within a single timer tick, the elapsed time `t` could evaluate to 0.
Since `t` is used directly as the divisor in div64_u64() without being
checked, this might trigger a hardware exception and panic the kernel.
> + }
> +
> + 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]);
> + }
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=4
^ permalink raw reply
* Re: [PATCH 8/8] raid6/kunit: add a benchmark
From: sashiko-bot @ 2026-07-08 9:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: yukuai, linux-raid
In-Reply-To: <20260708090740.1433685-9-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Potential division by zero in benchmark speed calculation if elapsed time `t` evaluates to zero.
- [Medium] Unbounded atomic section (preemption disabled) can cause soft lockups or RCU stalls on slower architectures.
- [Low] Duplicate `RAID6_PQ` Kconfig symbol declaration.
- [Low] Inaccurate commit message referencing XOR functions instead of RAID6 PQ functions.
--
--- Patch [8]: [PATCH 8/8] raid6/kunit: add a benchmark ---
commit 63d135afc471564206ce7921d64339a16ff1484f
Author: Christoph Hellwig <hch@lst.de>
raid6/kunit: add a benchmark
This commit adds a benchmark to the RAID6 KUnit test suite to evaluate P/Q
generation speeds across different disk counts and block sizes. It splits
the max alloc size from the kunit max size and reports the performance in
GB/s.
> Add a benchmark to test the XOR functions for more representative block
> sizes and numbers of disks. This splits the maximum alloc size used for
[Severity: Low]
This isn't a bug, but the commit message states it adds a benchmark for the
"XOR functions", while the patch actually adds a benchmark for the RAID6 P/Q
generation algorithm.
[ ... ]
> diff --git a/lib/raid/Kconfig b/lib/raid/Kconfig
> index c4eeb7c716c89..c52d5c2052946 100644
> --- a/lib/raid/Kconfig
> +++ b/lib/raid/Kconfig
> @@ -60,6 +60,15 @@ config RAID6_PQ_KUNIT_TEST
> This is intended to help people writing architecture-specific
> optimized versions. If unsure, say N.
>
> +config RAID6_PQ_KUNIT_BENCHMARK
> + bool "Benchmark for RAID6 PQ"
> + depends on RAID6_PQ_KUNIT_TEST
> + help
> + Include benchmarks in the KUnit test suite for raid P/Q generation.
> +
> +config RAID6_PQ
> + tristate
> +
[Severity: Low]
Is this duplicate declaration of config RAID6_PQ intentional? It appears to
be defined earlier in this same file.
> config RAID6_PQ_BENCHMARK
> bool "Automatically choose fastest RAID6 PQ functions"
> depends on RAID6_PQ
> diff --git a/lib/raid/raid6/tests/raid6_kunit.c b/lib/raid/raid6/tests/raid6_kunit.c
> index 9f3e671a12241..f927e3456d175 100644
> --- a/lib/raid/raid6/tests/raid6_kunit.c
> +++ b/lib/raid/raid6/tests/raid6_kunit.c
[ ... ]
> @@ -229,6 +230,68 @@ static void raid6_test(struct kunit *test)
> raid6_test_one(test);
> }
>
> +static void raid6_benchmark(struct kunit *test)
> +{
[ ... ]
> + 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, RAID6_KUNIT_ALLOC_BYTES);
> +
> + preempt_disable();
> + t = ktime_get_ns();
> + for (l = 0; l < num_iters; l++)
> + raid6_gen_syndrome(nr_to_test[i],
> + len_to_test[j], test_buffers);
> + t = ktime_get_ns() - t;
> + preempt_enable();
[Severity: Medium]
Could this unbounded atomic section cause soft lockups or RCU stalls on
slower architectures or emulators?
The loop executes 1,000 iterations of raid6_gen_syndrome() with preemption
disabled. On slow hardware or CI emulators, processing this much data might
take multiple seconds and prevent the scheduler from running.
> +
> + speed[j] = div64_u64((u64)len * num_iters * nr, t);
[Severity: Medium]
Is there a risk of a division by zero panic here?
If the KUnit test runs on a system with a coarse-resolution clock (such as
with CONFIG_HIGH_RES_TIMERS disabled) and a sufficiently fast CPU, the
1,000 iterations might complete in less time than a single clock tick.
In this scenario, ktime_get_ns() could return the same value before and
after the loop, causing the elapsed time t to evaluate to exactly 0 before
being passed as the divisor.
> + }
> +
> + 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]);
> + }
> +}
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=8
^ permalink raw reply
* Re: [PATCH 7/8] raid6: improve the runtime selection benchmark
From: sashiko-bot @ 2026-07-08 9:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-raid, yukuai
In-Reply-To: <20260708090740.1433685-8-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Potential division by zero kernel panic when calculating benchmark throughput if the elapsed time `t` evaluates to 0.
- [Medium] The reported throughput for the XOR algorithm is artificially inflated by a factor of 2.
- [Low] The benchmark inner loop variable `int i` redundantly shadows the outer function-scoped variable `unsigned int i`.
--
--- Patch [7]: [PATCH 7/8] raid6: improve the runtime selection benchmark ---
commit ae782577f509b50023ef73d6d96f6fe2f366a500
Author: Christoph Hellwig <hch@lst.de>
raid6: improve the runtime selection benchmark
Use plain ktime_get_ns for the timing, use 8 + 2 disks for a realistic
load, and report the throughput on the data disks as that is what
storage systems are measured on.
> diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
> index e7984bde5157f..c44b986e319e3 100644
> --- a/lib/raid/raid6/algos.c
> +++ b/lib/raid/raid6/algos.c
[ ... ]
> @@ -152,40 +152,39 @@ void raid6_recov_datap(int disks, size_t bytes, int faila, void **ptrs)
> }
> EXPORT_SYMBOL_GPL(raid6_recov_datap);
>
> -#define RAID6_TIME_JIFFIES_LG2 4
> -#define RAID6_TEST_DISKS 8
> +#define BENCH_SIZE SZ_4K
> +#define NR_SRCS 8
> +#define NR_DISKS (NR_SRCS + 2)
> +#define REPS 800U
>
> -static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
> - const int disks)
> +static int raid6_choose_gen(void *dptrs[NR_DISKS], const int disks)
> {
> /* work on the second half of the disks */
> - int start = (disks >> 1) - 1, stop = disks - 3;
> + int start = (disks / 2) - 1, stop = disks - 3;
> const struct raid6_calls *best = NULL;
> unsigned long bestgenperf = 0;
> unsigned int i;
>
> for (i = 0; i < raid6_nr_algos; i++) {
> const struct raid6_calls *algo = raid6_algos[i];
> - unsigned long perf = 0, j0, j1;
> + unsigned long perf = 0;
> + u64 t;
> + int i;
[Severity: Low]
This isn't a bug, but `int i` declared here shadows the function-scoped
`unsigned int i` used for the outer loop.
>
> preempt_disable();
> - j0 = jiffies;
> - while ((j1 = jiffies) == j0)
> - cpu_relax();
> - while (time_before(jiffies,
> - j1 + (1<<RAID6_TIME_JIFFIES_LG2))) {
> - algo->gen_syndrome(disks, PAGE_SIZE, *dptrs);
> - perf++;
> - }
> + t = ktime_get_ns();
> + for (i = 0; i < REPS; i++)
> + algo->gen_syndrome(disks, BENCH_SIZE, dptrs);
> + t = ktime_get_ns() - t;
> preempt_enable();
>
> + /* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
> + perf = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);
[Severity: High]
Is it possible for `t` to be 0 here, leading to a division by zero panic?
If the system uses a coarse clock source (like jiffies during early boot before
high-resolution timers are initialized) and the benchmark iterations complete
within the same timer tick, `t` might evaluate to 0.
[ ... ]
> @@ -197,28 +196,22 @@ static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
> static_call_update(raid6_xor_syndrome_impl, best->xor_syndrome);
>
> pr_info("raid6: using algorithm %s gen() %ld MB/s\n",
> - best->name,
> - (bestgenperf * HZ * (disks - 2)) >>
> - (20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2));
> + best->name, bestgenperf);
>
> if (best->xor_syndrome) {
> - unsigned long perf = 0, j0, j1;
> + u64 t;
> + int i;
[Severity: Low]
This isn't a bug, but `int i` declared here also shadows the function-scoped
`unsigned int i`.
>
> preempt_disable();
> - j0 = jiffies;
> - while ((j1 = jiffies) == j0)
> - cpu_relax();
> - while (time_before(jiffies,
> - j1 + (1 << RAID6_TIME_JIFFIES_LG2))) {
> - best->xor_syndrome(disks, start, stop,
> - PAGE_SIZE, *dptrs);
> - perf++;
> - }
> + t = ktime_get_ns();
> + for (i = 0; i < REPS; i++)
> + best->xor_syndrome(disks, start, stop, BENCH_SIZE,
> + dptrs);
> + t = ktime_get_ns() - t;
> preempt_enable();
>
> - pr_info("raid6: .... xor() %ld MB/s, rmw enabled\n",
> - (perf * HZ * (disks - 2)) >>
> - (20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2 + 1));
> + pr_info("raid6: .... xor() %llu MB/s, rmw enabled\n",
> + div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t));
[Severity: Medium]
Does this calculation artificially inflate the reported throughput for the XOR
algorithm by a factor of 2?
The `xor_syndrome` benchmark operates on a subset of disks from `start` to
`stop` (which is `(disks / 2) - 1` through `disks - 3`). For the default
`NR_DISKS` (10), this processes 4 data disks. However, the throughput
calculation multiplies by `NR_SRCS` (8) instead of the actual number of disks
processed. The previous implementation correctly adjusted for this using an
extra bit shift (`+ 1`), which was dropped in this patch.
[Severity: High]
Like the calculation above, could `t` be 0 here leading to a division by zero
panic?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=7
^ permalink raw reply
* Re: [PATCH 2/8] xor: improve the runtime selection benchmark
From: sashiko-bot @ 2026-07-08 9:18 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-raid, yukuai
In-Reply-To: <20260708090740.1433685-3-hch@lst.de>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A division by zero kernel panic will occur during boot on systems using a coarse clocksource when calculating the benchmark speed.
--
--- Patch [2]: [PATCH 2/8] xor: improve the runtime selection benchmark ---
commit 69c8bf90e6d6fa234ebb61b5b5b7c4dfd2d8e9c1
Author: Christoph Hellwig <hch@lst.de>
xor: improve the runtime selection benchmark
This commit updates the XOR benchmark to use ktime_get_ns() for timing,
increases the workload to 4 source disks, and calculates throughput based
on the data disks.
> diff --git a/lib/raid/xor/xor-core.c b/lib/raid/xor/xor-core.c
> --- a/lib/raid/xor/xor-core.c
> +++ b/lib/raid/xor/xor-core.c
[ ... ]
> @@ -73,59 +72,56 @@ void __init xor_force(struct xor_block_template *tmpl)
[ ... ]
> -static void __init
> -do_xor_speed(struct xor_block_template *tmpl, void *b1, void *b2)
> +static void __init do_xor_speed(struct xor_block_template *tmpl, void *dest,
> + void *srcs[NR_SRCS])
> {
[ ... ]
> + t = ktime_get_ns();
> + for (i = 0; i < REPS; i++) {
> mb(); /* prevent loop optimization */
> - tmpl->xor_gen(b1, srcs, 1, BENCH_SIZE);
> + tmpl->xor_gen(dest, srcs, NR_SRCS, BENCH_SIZE);
> mb();
> - } while (reps++ < REPS || (t0 = ktime_get()) == start);
> - min = ktime_sub(t0, start);
> -
> + }
> + t = ktime_get_ns() - t;
> preempt_enable();
>
> - // bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s]
> - speed = (1000 * reps * BENCH_SIZE) / (unsigned int)ktime_to_ns(min);
> - tmpl->speed = speed;
> + /* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
> + tmpl->speed = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);
[Severity: High]
Could this cause a division by zero panic during boot on systems with a
coarse clocksource?
If the 800 loop iterations execute very quickly (within the same clock tick)
on a platform without high-resolution timers, or during early boot phases
where jiffies are used as the clocksource, the elapsed time t will evaluate
to 0.
The previous code avoided this by looping until the clock advanced before
and after the benchmark:
while ((start = ktime_get()) == t0)
Since t is now used directly as the divisor in div64_u64(), a 0 value will
trigger an unhandled divide-by-zero hardware exception.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260708090740.1433685-1-hch@lst.de?part=2
^ permalink raw reply
* [PATCH 8/8] raid6/kunit: add a benchmark
From: Christoph Hellwig @ 2026-07-08 9:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-raid, linux-kernel
In-Reply-To: <20260708090740.1433685-1-hch@lst.de>
Add a benchmark to test the XOR functions for more representative block
sizes and numbers of disks. This splits the maximum alloc size used for
the benchmark from the max size for the kunit test because recovery
is currently limited to a single page at a time. Hopefully this will be
fixed soon.
The runtime numbers are reported in GB/s as the numbers of modern
implementations are basically unreadable as MB/s. This means
retro-architectures could report 0, but that is an easy tradeoff.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/Kconfig | 9 ++++
lib/raid/raid6/tests/raid6_kunit.c | 66 +++++++++++++++++++++++++++++-
2 files changed, 74 insertions(+), 1 deletion(-)
diff --git a/lib/raid/Kconfig b/lib/raid/Kconfig
index c4eeb7c716c8..c52d5c205294 100644
--- a/lib/raid/Kconfig
+++ b/lib/raid/Kconfig
@@ -60,6 +60,15 @@ config RAID6_PQ_KUNIT_TEST
This is intended to help people writing architecture-specific
optimized versions. If unsure, say N.
+config RAID6_PQ_KUNIT_BENCHMARK
+ bool "Benchmark for RAID6 PQ"
+ depends on RAID6_PQ_KUNIT_TEST
+ help
+ Include benchmarks in the KUnit test suite for raid P/Q generation.
+
+config RAID6_PQ
+ tristate
+
config RAID6_PQ_BENCHMARK
bool "Automatically choose fastest RAID6 PQ functions"
depends on RAID6_PQ
diff --git a/lib/raid/raid6/tests/raid6_kunit.c b/lib/raid/raid6/tests/raid6_kunit.c
index 9f3e671a1224..f927e3456d17 100644
--- a/lib/raid/raid6/tests/raid6_kunit.c
+++ b/lib/raid/raid6/tests/raid6_kunit.c
@@ -18,6 +18,7 @@ MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
#define RAID6_KUNIT_MAX_BUFFERS 64 /* Including P and Q */
#define RAID6_KUNIT_MAX_FAILURES 2
#define RAID6_KUNIT_MAX_BYTES PAGE_SIZE
+#define RAID6_KUNIT_ALLOC_BYTES SZ_16K
static struct rnd_state rng;
static void *test_buffers[RAID6_KUNIT_MAX_BUFFERS];
@@ -229,6 +230,68 @@ static void raid6_test(struct kunit *test)
raid6_test_one(test);
}
+static void raid6_benchmark(struct kunit *test)
+{
+ static const unsigned int nr_to_test[] = {
+ 4, 5, 6, 7, 8, 10, 12, 15, 16, 32,
+ };
+ static const unsigned int len_to_test[] = {
+ SZ_4K, SZ_16K,
+ };
+ unsigned int i, j, l;
+ u64 t;
+
+ if (!IS_ENABLED(CONFIG_RAID6_PQ_KUNIT_BENCHMARK))
+ kunit_skip(test, "not enabled");
+
+ /* warm-up */
+ for (i = 0; i < ARRAY_SIZE(nr_to_test); i++) {
+ for (j = 0; j < ARRAY_SIZE(len_to_test); j++) {
+ for (l = 0; l < 10; l++) {
+ raid6_gen_syndrome(nr_to_test[i],
+ len_to_test[j], test_buffers);
+ }
+ }
+ }
+
+ /*
+ * Preferably this would be a loop over len_to_test, but the kunit
+ * logging always adds a newline to each logged format string.
+ */
+ static_assert(ARRAY_SIZE(len_to_test) == 2);
+ kunit_info(test, " \t%5u bytes\t%5u bytes\n",
+ len_to_test[0], len_to_test[1]);
+
+ for (i = 0; i < ARRAY_SIZE(nr_to_test); i++) {
+ unsigned int nr = nr_to_test[i];
+ u64 speed[ARRAY_SIZE(len_to_test)];
+
+ KUNIT_ASSERT_LE(test, nr, RAID6_KUNIT_MAX_BUFFERS);
+
+ 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, RAID6_KUNIT_ALLOC_BYTES);
+
+ preempt_disable();
+ t = ktime_get_ns();
+ for (l = 0; l < num_iters; l++)
+ raid6_gen_syndrome(nr_to_test[i],
+ len_to_test[j], test_buffers);
+ t = ktime_get_ns() - t;
+ preempt_enable();
+
+ speed[j] = div64_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]);
+ }
+}
+
static const void *raid6_gen_params(struct kunit *test, const void *prev,
char *desc)
{
@@ -256,6 +319,7 @@ static const void *raid6_gen_params(struct kunit *test, const void *prev,
static struct kunit_case raid6_test_cases[] = {
KUNIT_CASE_PARAM(raid6_test, raid6_gen_params),
+ KUNIT_CASE(raid6_benchmark),
{},
};
@@ -270,7 +334,7 @@ static int raid6_suite_init(struct kunit_suite *suite)
* so that it is immediately followed by a guard page. This allows
* buffer overreads to be detected, even in assembly code.
*/
- test_buflen = round_up(RAID6_KUNIT_MAX_BYTES, PAGE_SIZE);
+ test_buflen = round_up(RAID6_KUNIT_ALLOC_BYTES, PAGE_SIZE);
for (i = 0; i < RAID6_KUNIT_MAX_FAILURES; i++) {
test_recov_buffers[i] = vmalloc(test_buflen);
if (!test_recov_buffers[i])
--
2.53.0
^ permalink raw reply related
* [PATCH 7/8] raid6: improve the runtime selection benchmark
From: Christoph Hellwig @ 2026-07-08 9:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-raid, linux-kernel
In-Reply-To: <20260708090740.1433685-1-hch@lst.de>
Use plain ktime_get_ns for the timing, use 8 + 2 disks for a realistic
load, and report the throughput on the data disks as that is what
storage systems are measured on.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/raid6/algos.c | 75 +++++++++++++++++++-----------------------
1 file changed, 34 insertions(+), 41 deletions(-)
diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
index e7984bde5157..c44b986e319e 100644
--- a/lib/raid/raid6/algos.c
+++ b/lib/raid/raid6/algos.c
@@ -152,40 +152,39 @@ void raid6_recov_datap(int disks, size_t bytes, int faila, void **ptrs)
}
EXPORT_SYMBOL_GPL(raid6_recov_datap);
-#define RAID6_TIME_JIFFIES_LG2 4
-#define RAID6_TEST_DISKS 8
+#define BENCH_SIZE SZ_4K
+#define NR_SRCS 8
+#define NR_DISKS (NR_SRCS + 2)
+#define REPS 800U
-static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
- const int disks)
+static int raid6_choose_gen(void *dptrs[NR_DISKS], const int disks)
{
/* work on the second half of the disks */
- int start = (disks >> 1) - 1, stop = disks - 3;
+ int start = (disks / 2) - 1, stop = disks - 3;
const struct raid6_calls *best = NULL;
unsigned long bestgenperf = 0;
unsigned int i;
for (i = 0; i < raid6_nr_algos; i++) {
const struct raid6_calls *algo = raid6_algos[i];
- unsigned long perf = 0, j0, j1;
+ unsigned long perf = 0;
+ u64 t;
+ int i;
preempt_disable();
- j0 = jiffies;
- while ((j1 = jiffies) == j0)
- cpu_relax();
- while (time_before(jiffies,
- j1 + (1<<RAID6_TIME_JIFFIES_LG2))) {
- algo->gen_syndrome(disks, PAGE_SIZE, *dptrs);
- perf++;
- }
+ t = ktime_get_ns();
+ for (i = 0; i < REPS; i++)
+ algo->gen_syndrome(disks, BENCH_SIZE, dptrs);
+ t = ktime_get_ns() - t;
preempt_enable();
+ /* bytes/ns == GB/s, multiply by 1000 to get MB/s [not MiB/s] */
+ perf = div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t);
if (perf > bestgenperf) {
bestgenperf = perf;
best = algo;
}
- pr_info("raid6: %-8s gen() %5ld MB/s\n", algo->name,
- (perf * HZ * (disks-2)) >>
- (20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2));
+ pr_info("raid6: %-8s gen() %5lu MB/s\n", algo->name, perf);
}
if (!best) {
@@ -197,28 +196,22 @@ static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
static_call_update(raid6_xor_syndrome_impl, best->xor_syndrome);
pr_info("raid6: using algorithm %s gen() %ld MB/s\n",
- best->name,
- (bestgenperf * HZ * (disks - 2)) >>
- (20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2));
+ best->name, bestgenperf);
if (best->xor_syndrome) {
- unsigned long perf = 0, j0, j1;
+ u64 t;
+ int i;
preempt_disable();
- j0 = jiffies;
- while ((j1 = jiffies) == j0)
- cpu_relax();
- while (time_before(jiffies,
- j1 + (1 << RAID6_TIME_JIFFIES_LG2))) {
- best->xor_syndrome(disks, start, stop,
- PAGE_SIZE, *dptrs);
- perf++;
- }
+ t = ktime_get_ns();
+ for (i = 0; i < REPS; i++)
+ best->xor_syndrome(disks, start, stop, BENCH_SIZE,
+ dptrs);
+ t = ktime_get_ns() - t;
preempt_enable();
- pr_info("raid6: .... xor() %ld MB/s, rmw enabled\n",
- (perf * HZ * (disks - 2)) >>
- (20 - PAGE_SHIFT + RAID6_TIME_JIFFIES_LG2 + 1));
+ pr_info("raid6: .... xor() %llu MB/s, rmw enabled\n",
+ div64_u64((u64)BENCH_SIZE * REPS * NR_SRCS * 1000, t));
}
return 0;
@@ -230,14 +223,14 @@ static int raid6_choose_gen(void *(*const dptrs)[RAID6_TEST_DISKS],
static int __init raid6_select_algo(void)
{
- const int disks = RAID6_TEST_DISKS;
+ const int disks = NR_DISKS;
+ void *dptrs[NR_DISKS];
char *disk_ptr, *p;
- void *dptrs[RAID6_TEST_DISKS];
int i, cycle;
int error;
/* prepare the buffer and fill it circularly with gfmul table */
- disk_ptr = kmalloc(PAGE_SIZE * RAID6_TEST_DISKS, GFP_KERNEL);
+ disk_ptr = kmalloc_array(NR_DISKS, BENCH_SIZE, GFP_KERNEL);
if (!disk_ptr) {
pr_err("raid6: Yikes! No memory available.\n");
return -ENOMEM;
@@ -245,19 +238,19 @@ static int __init raid6_select_algo(void)
p = disk_ptr;
for (i = 0; i < disks; i++)
- dptrs[i] = p + PAGE_SIZE * i;
+ dptrs[i] = p + BENCH_SIZE * i;
- cycle = ((disks - 2) * PAGE_SIZE) / 65536;
+ cycle = ((disks - 2) * BENCH_SIZE) / 65536;
for (i = 0; i < cycle; i++) {
memcpy(p, raid6_gfmul, 65536);
p += 65536;
}
- if ((disks - 2) * PAGE_SIZE % 65536)
- memcpy(p, raid6_gfmul, (disks - 2) * PAGE_SIZE % 65536);
+ if ((disks - 2) * BENCH_SIZE % 65536)
+ memcpy(p, raid6_gfmul, (disks - 2) * BENCH_SIZE % 65536);
/* select raid gen_syndrome function */
- error = raid6_choose_gen(&dptrs, disks);
+ error = raid6_choose_gen(dptrs, disks);
kfree(disk_ptr);
--
2.53.0
^ permalink raw reply related
* [PATCH 6/8] raid6: defer implementation selection when built-in
From: Christoph Hellwig @ 2026-07-08 9:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-raid, linux-kernel
In-Reply-To: <20260708090740.1433685-1-hch@lst.de>
Don't hold up early boot and defer the selection just like we've been
doing for the RAID5 XOR code since commit 524ccdbdfb52 ("crypto: xor -
defer load time benchmark to a later time").
This will also allow full use of the timing subsystem for benchmarking.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/raid6/algos.c | 37 +++++++++++++++++++++++++++----------
1 file changed, 27 insertions(+), 10 deletions(-)
diff --git a/lib/raid/raid6/algos.c b/lib/raid/raid6/algos.c
index 6f5c89ab2b17..e7984bde5157 100644
--- a/lib/raid/raid6/algos.c
+++ b/lib/raid/raid6/algos.c
@@ -236,16 +236,6 @@ static int __init raid6_select_algo(void)
int i, cycle;
int error;
- if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
- pr_info("raid6: skipped pq benchmark and selected %s\n",
- raid6_algos[raid6_nr_algos - 1]->name);
- static_call_update(raid6_gen_syndrome_impl,
- raid6_algos[raid6_nr_algos - 1]->gen_syndrome);
- static_call_update(raid6_xor_syndrome_impl,
- raid6_algos[raid6_nr_algos - 1]->xor_syndrome);
- return 0;
- }
-
/* prepare the buffer and fill it circularly with gfmul table */
disk_ptr = kmalloc(PAGE_SIZE * RAID6_TEST_DISKS, GFP_KERNEL);
if (!disk_ptr) {
@@ -329,13 +319,40 @@ static int __init raid6_init(void)
static_call_update(raid6_recov_datap_impl, raid6_recov_algo->datap);
pr_info("raid6: using %s recovery algorithm\n", raid6_recov_algo->name);
+ if (!IS_ENABLED(CONFIG_RAID6_PQ_BENCHMARK) || raid6_nr_algos == 1) {
+ pr_info("raid6: skipped pq benchmark and selected %s\n",
+ raid6_algos[raid6_nr_algos - 1]->name);
+ goto default_algo;
+ }
+
+#ifdef MODULE
return raid6_select_algo();
+#else
+ /*
+ * Pick the last registered implementation as the temporary default until
+ * calibration happens.
+ */
+#endif
+default_algo:
+ static_call_update(raid6_gen_syndrome_impl,
+ raid6_algos[raid6_nr_algos - 1]->gen_syndrome);
+ static_call_update(raid6_xor_syndrome_impl,
+ raid6_algos[raid6_nr_algos - 1]->xor_syndrome);
+ return 0;
}
static void __exit raid6_exit(void)
{
}
+/*
+ * When built-in we must register the default implementation before md
+ * initializes, but we don't want calibration to run that early as that
+ * would delay the boot process.
+ */
+#ifndef MODULE
+device_initcall(raid6_select_algo);
+#endif
subsys_initcall(raid6_init);
module_exit(raid6_exit);
MODULE_LICENSE("GPL");
--
2.53.0
^ permalink raw reply related
* [PATCH 5/8] raid6: enable lock context analysis
From: Christoph Hellwig @ 2026-07-08 9:07 UTC (permalink / raw)
To: Andrew Morton; +Cc: Eric Biggers, linux-raid, linux-kernel
In-Reply-To: <20260708090740.1433685-1-hch@lst.de>
The code doesn't have any locking, so this is trivial.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
lib/raid/raid6/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/raid/raid6/Makefile b/lib/raid/raid6/Makefile
index 038d6c74d1ba..5cdb7223de2a 100644
--- a/lib/raid/raid6/Makefile
+++ b/lib/raid/raid6/Makefile
@@ -1,5 +1,7 @@
# SPDX-License-Identifier: GPL-2.0
+CONTEXT_ANALYSIS := y
+
ccflags-y += -I $(src)
ifeq ($(CONFIG_RAID6_PQ_ARCH),y)
--
2.53.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox