* [PATCH] md/raid5: split reshape bios before bitmap accounting
From: Yu Kuai @ 2026-04-19 3:09 UTC (permalink / raw)
To: linux-raid; +Cc: linux-kernel, Li Nan, Yu Kuai, Cheng Cheng
In-Reply-To: <20260419030942.824195-1-yukuai@fnnas.com>
Use the shared mddev_bio_split_at_reshape_offset() helper so RAID5
submits only one-side bios to llbitmap during reshape.
Signed-off-by: Yu Kuai <yukuai@fnnas.com>
---
drivers/md/raid5.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 476f6fc5a97c..7fa74c60c7d8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -6134,6 +6134,14 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
return true;
}
+ bi = mddev_bio_split_at_reshape_offset(mddev, bi, NULL,
+ &conf->bio_split);
+ if (!bi) {
+ if (rw == WRITE)
+ md_write_end(mddev);
+ return true;
+ }
+
logical_sector = bi->bi_iter.bi_sector & ~((sector_t)RAID5_STRIPE_SECTORS(conf)-1);
bi->bi_next = NULL;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH v2] md/raid5: Fix UAF on IO across the reshape position
From: Yu Kuai @ 2026-04-19 3:51 UTC (permalink / raw)
To: Benjamin Marzinski, Song Liu, Li Nan, Xiao Ni, yukuai
Cc: linux-raid, dm-devel, Nigel Croxon
In-Reply-To: <20260408043548.1695157-1-bmarzins@redhat.com>
在 2026/4/8 12:35, Benjamin Marzinski 写道:
> If make_stripe_request() returns STRIPE_WAIT_RESHAPE,
> raid5_make_request() will free the cloned bio. But raid5_make_request()
> can call make_stripe_request() multiple times, writing to the various
> stripes. If that bio got added to the toread or towrite lists of a
> stripe disk in an earlier call to make_stripe_request(), then it's not
> safe to just free the bio if a later part of it is found to cross the
> reshape position. Doing so can lead to a UAF error, when bio_endio()
> is called on the bio for the earlier stripes.
>
> Instead, raid5_make_request() needs to wait until all parts of the bio
> have called bio_endio(). To do this, bios that cross the reshape
> position while the reshape can't make progress are flagged as needing to
> wait for all parts to complete. When raid5_make_request() has a bio that
> failed make_stripe_request() with STRIPE_WAIT_RESHAPE, it sets
> bi->bi_private to a completion struct and waits for completion after
> ending the bio. When the bio_endio() is called for the last time on a
> clone bio with bi->bi_private set, it wakes up the waiter. This
> guarantees that raid5_make_request() doesn't return until the cloned bio
> needing a retry for io across the reshape boundary is safely cleaned up.
>
> There is a simple reproducer available at [1]. Compile the kernel with
> KASAN for more useful reporting when the error is triggered (this is not
> necessary to see the bug).
>
> [1]https://gist.github.com/bmarzins/e48598824305cf2171289e47d7241fa5
>
> Signed-off-by: Benjamin Marzinski<bmarzins@redhat.com>
> ---
>
> Changes from v1:
> - Removed mddev->pending_retry_bios, mddev->retry_bios_wait, and
> md_io_clone->must_retry. Instead, use a completion struct
> pointed to by bi->bi_private, as suggested by Xiao Ni and Yu Kuai.
>
> drivers/md/md.c | 31 ++++++++-----------------------
> drivers/md/md.h | 1 -
> drivers/md/raid5.c | 7 ++++++-
> 3 files changed, 14 insertions(+), 25 deletions(-)
Applied
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH 1/1] MAINTAINERS: Add Xiao Ni as md/raid reviewer
From: Yu Kuai @ 2026-04-19 4:23 UTC (permalink / raw)
To: Xiao Ni, song; +Cc: linan666, linux-raid, linux-kernel, yukuai
In-Reply-To: <20260414022956.48271-1-xiaoraid25@gmail.com>
在 2026/4/14 10:29, Xiao Ni 写道:
> From: Xiao Ni<xiao@kernel.org>
>
> I've been actively involved in the md subsystem, contributing bug
> fixes, performance improvements, and participating in code reviews.
> I will help improve patch review coverage and response time.
>
> Signed-off-by: Xiao Ni<xiao@kernel.org>
> ---
> MAINTAINERS | 1 +
> 1 file changed, 1 insertion(+)
Applied
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md: fix kobject reference leak in md_import_device()
From: Yu Kuai @ 2026-04-19 4:42 UTC (permalink / raw)
To: Guangshuo Li, Song Liu, Greg Kroah-Hartman, linux-raid,
linux-kernel, yukuai
Cc: stable
In-Reply-To: <20260413141759.2970973-1-lgs201920130244@gmail.com>
Hi,
在 2026/4/13 22:17, Guangshuo Li 写道:
> md_import_device() initializes rdev->kobj with kobject_init() before
> checking the device size and loading the superblock.
>
> When one of the later checks fails, the error path still frees rdev
> directly with kfree(). This bypasses the kobject release path and leaves
> the kobject reference unbalanced.
>
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
>
> After kobject_init(), release rdev through kobject_put() instead of
> kfree().
>
> Fixes: f9cb074bff8e ("Kobject: rename kobject_init_ng() to kobject_init()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
> v2:
> - note that the issue was identified by my static analysis tool
> - and confirmed by manual review
>
> drivers/md/md.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 6d73f6e196a9..4ce7512dc834 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -3871,6 +3871,9 @@ static struct md_rdev *md_import_device(dev_t newdev, int super_format, int supe
>
> out_blkdev_put:
> fput(rdev->bdev_file);
> + md_rdev_clear(rdev);
> + kobject_put(&rdev->kobj);
> + return ERR_PTR(err);
I think it's cleaner to move kobject_init() after everything in rdev
is ready.
> out_clear_rdev:
> md_rdev_clear(rdev);
> out_free_rdev:
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md: factor out cloned bio cleanup into md_free_bio()
From: Yu Kuai @ 2026-04-19 4:48 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260414103813.307601-1-abd.masalkhi@gmail.com>
Hi,
在 2026/4/14 18:38, Abd-Alrhman Masalkhi 写道:
> Refactor duplicated cloned bio completion and cleanup logic into
> a new helper, md_free_bio().
>
> md_end_clone_io() and md_free_cloned_bio() previously shared nearly
> identical teardown code, differing only in whether the original
> bio’s endio callback was invoked. Introduce a boolean parameter
> orig_endio to control this behavior and consolidate the logic.
>
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> drivers/md/md.c | 26 +++++++++-----------------
> 1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index ac71640ff3a8..707d605fee61 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9208,7 +9208,7 @@ static void md_bitmap_end(struct mddev *mddev, struct md_io_clone *md_io_clone)
> fn(mddev, md_io_clone->offset, md_io_clone->sectors);
> }
>
> -static void md_end_clone_io(struct bio *bio)
> +static void md_free_bio(struct bio *bio, bool orig_endio)
> {
> struct md_io_clone *md_io_clone = bio->bi_private;
> struct bio *orig_bio = md_io_clone->orig_bio;
> @@ -9224,10 +9224,16 @@ static void md_end_clone_io(struct bio *bio)
> bio_end_io_acct(orig_bio, md_io_clone->start_time);
>
> bio_put(bio);
> - bio_endio(orig_bio);
> + if (orig_endio)
> + bio_endio(orig_bio);
> percpu_ref_put(&mddev->active_io);
> }
>
> +static void md_end_clone_io(struct bio *bio)
> +{
> + md_free_bio(bio, true);
> +}
> +
> static void md_clone_bio(struct mddev *mddev, struct bio **bio)
> {
> struct block_device *bdev = (*bio)->bi_bdev;
> @@ -9262,21 +9268,7 @@ EXPORT_SYMBOL_GPL(md_account_bio);
>
> void md_free_cloned_bio(struct bio *bio)
> {
> - struct md_io_clone *md_io_clone = bio->bi_private;
> - struct bio *orig_bio = md_io_clone->orig_bio;
> - struct mddev *mddev = md_io_clone->mddev;
> -
> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
> - md_bitmap_end(mddev, md_io_clone);
> -
> - if (bio->bi_status && !orig_bio->bi_status)
> - orig_bio->bi_status = bio->bi_status;
> -
> - if (md_io_clone->start_time)
> - bio_end_io_acct(orig_bio, md_io_clone->start_time);
> -
> - bio_put(bio);
> - percpu_ref_put(&mddev->active_io);
> + md_free_bio(bio, false);
> }
> EXPORT_SYMBOL_GPL(md_free_cloned_bio);
This patch is no longer needed after following patch:
https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com
>
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()
From: Yu Kuai @ 2026-04-19 4:51 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, linan666; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260415081941.352389-1-abd.masalkhi@gmail.com>
Hi,
在 2026/4/15 16:19, Abd-Alrhman Masalkhi 写道:
> md_end_clone_io() and md_free_cloned_bio() share identical teardown
> logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and
> call bio_endio() afterwards.
>
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> Changes in v2:
> - Reuse md_free_cloned_bio() instead of introducing a new helper
> - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com
> ---
> drivers/md/md.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index ac71640ff3a8..8565566a447b 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio)
> {
> struct md_io_clone *md_io_clone = bio->bi_private;
> struct bio *orig_bio = md_io_clone->orig_bio;
> - struct mddev *mddev = md_io_clone->mddev;
> -
> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
> - md_bitmap_end(mddev, md_io_clone);
> -
> - if (bio->bi_status && !orig_bio->bi_status)
> - orig_bio->bi_status = bio->bi_status;
> -
> - if (md_io_clone->start_time)
> - bio_end_io_acct(orig_bio, md_io_clone->start_time);
>
> - bio_put(bio);
> + md_free_cloned_bio(bio);
> bio_endio(orig_bio);
> - percpu_ref_put(&mddev->active_io);
> }
>
> static void md_clone_bio(struct mddev *mddev, struct bio **bio)
Same as I replied to v1,
This patch is no longer needed after following patch:
https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: fix race between reshape and chunk-aligned read
From: Yu Kuai @ 2026-04-19 5:14 UTC (permalink / raw)
To: FengWei Shih, song; +Cc: linan122, linux-raid, linux-kernel, yukuai
In-Reply-To: <20260409051722.2865321-1-dannyshih@synology.com>
Hi,
在 2026/4/9 13:17, FengWei Shih 写道:
> raid5_make_request() checks mddev->reshape_position to decide whether
> to allow chunk-aligned reads. However in raid5_start_reshape(), the
> layout configuration (raid_disks, algorithm, etc.) is updated before
> mddev->reshape_position is set:
>
> reshape (raid5_start_reshape) read (raid5_make_request)
> ============================== ===========================
> write_seqcount_begin
> update raid_disks, algorithm...
> set conf->reshape_progress
> write_seqcount_end
> check mddev->reshape_position
> * still MaxSector, allow
> raid5_read_one_chunk()
> * use new layout
> raid5_quiesce()
> set mddev->reshape_position
While we're here, I think it's pretty ugly to disable raid5_read_one_chunk
when reshape is not fully done. So a better solution should be:
- data behind reshape: read with new layout
- data ahead of reshape: read with old layout, reshape will also need to wait
for this IO to be done, before reshape can make progress.
- data intersecting the active reshape window: wait for reshape to make progress.
>
> Since reshape_position is not yet updated, raid5_make_request()
> considers no reshape is in progress and proceeds with the
> chunk-aligned path, but the layout has already changed, causing
> raid5_compute_sector() to return an incorrect physical address.
>
> Fix this by reading conf->reshape_progress under gen_lock in
> raid5_read_one_chunk() and falling back to the stripe path if a
> reshape is in progress.
>
> Signed-off-by: FengWei Shih <dannyshih@synology.com>
> ---
> drivers/md/raid5.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index a8e8d431071b..bded2b86f0ef 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -5421,6 +5421,11 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
> sector_t sector, end_sector;
> int dd_idx;
> bool did_inc;
> + int seq;
> +
> + seq = read_seqcount_begin(&conf->gen_lock);
> + if (unlikely(conf->reshape_progress != MaxSector))
> + return 0;
>
> if (!in_chunk_boundary(mddev, raid_bio)) {
> pr_debug("%s: non aligned\n", __func__);
> @@ -5431,6 +5436,9 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
> &dd_idx, NULL);
> end_sector = sector + bio_sectors(raid_bio);
>
> + if (read_seqcount_retry(&conf->gen_lock, seq))
> + return 0;
> +
> if (r5c_big_stripe_cached(conf, sector))
> return 0;
>
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1,raid10: don't fail devices for invalid IO errors
From: Yu Kuai @ 2026-04-19 5:26 UTC (permalink / raw)
To: Keith Busch, linux-raid, song
Cc: linan122, axboe, Keith Busch, Tomáš Trnka, yukuai
In-Reply-To: <20260416140345.3872265-1-kbusch@meta.com>
在 2026/4/16 22:03, Keith Busch 写道:
> From: Keith Busch<kbusch@kernel.org>
>
> BLK_STS_INVAL indicates the IO request itself was invalid, not that the
> device has failed. When raid1 treats this as a device error, it retries
> on alternate mirrors which fail the same way, eventually exceeding the
> read error threshold and removing the device from the array.
>
> This happens when stacking configurations bypass bio_split_to_limits()
> in the IO path: dm-raid calls md_handle_request() directly without going
> through md_submit_bio(), skipping the alignment validation that would
> otherwise reject invalid bios early. The invalid bio reaches the
> lower block layers, which fail the bio with BLK_STS_INVAL, and raid1
> wrongly interprets this as a device failure.
>
> Add BLK_STS_INVAL to raid1_should_handle_error() so that invalid IO
> errors are propagated back to the caller rather than triggering device
> removal. This is consistent with the previous kernel behavior when
> alignment checks were done earlier in the direct-io path.
>
> Fixes: 5ff3f74e145adc7 ("block: simplify direct io validity check")
> Link:https://lore.kernel.org/linux-block/2982107.4sosBPzcNG@electra/
> Reported-by: Tomáš Trnka<trnka@scm.com>
> Signed-off-by: Keith Busch<kbusch@kernel.org>
> ---
> drivers/md/raid1-10.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
Applied
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid10: fix divide-by-zero in setup_geo() with zero far_copies
From: Yu Kuai @ 2026-04-19 5:43 UTC (permalink / raw)
To: Junrui Luo, Song Liu, Li Nan, NeilBrown, Jonathan Brassow, yukuai
Cc: linux-raid, linux-kernel, Yuhao Jiang, stable
In-Reply-To: <SYBPR01MB7881A5E2556806CC1D318582AF232@SYBPR01MB7881.ausprd01.prod.outlook.com>
Hi,
在 2026/4/16 11:39, Junrui Luo 写道:
> setup_geo() extracts near_copies (nc) and far_copies (fc) from the
> user-provided layout parameter without checking for zero. When fc=0
> with the "improved" far set layout selected, 'geo->far_set_size =
> disks / fc' triggers a divide-by-zero.
>
> Validate nc and fc immediately after extraction, returning -1 if
> either is zero.
>
> Fixes: 475901aff158 ("MD RAID10: Improve redundancy for 'far' and 'offset' algorithms (part 1)")
> Reported-by: Yuhao Jiang<danisjiang@gmail.com>
So again I can't find a report, and Reported-by usually should be followed
by a Closes link to the original report.
Applied with Reported-by tag removed.
> Cc:stable@vger.kernel.org
> Signed-off-by: Junrui Luo<moonafterrain@outlook.com>
> ---
> drivers/md/raid10.c | 2 ++
> 1 file changed, 2 insertions(+)
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid10: fix divide-by-zero in setup_geo() with zero far_copies
From: Yuhao Jiang @ 2026-04-19 6:01 UTC (permalink / raw)
To: yukuai
Cc: Junrui Luo, Song Liu, Li Nan, NeilBrown, Jonathan Brassow,
linux-raid, linux-kernel, stable
In-Reply-To: <beca1657-0180-4f9b-8de1-ca7776c9614a@fnnas.com>
Hi Kuai,
This report was reported by me, so Junrui added me as Reported-by.
Thanks,
On Sun, Apr 19, 2026 at 12:43 AM Yu Kuai <yukuai@fnnas.com> wrote:
>
> Hi,
>
> 在 2026/4/16 11:39, Junrui Luo 写道:
> > setup_geo() extracts near_copies (nc) and far_copies (fc) from the
> > user-provided layout parameter without checking for zero. When fc=0
> > with the "improved" far set layout selected, 'geo->far_set_size =
> > disks / fc' triggers a divide-by-zero.
> >
> > Validate nc and fc immediately after extraction, returning -1 if
> > either is zero.
> >
> > Fixes: 475901aff158 ("MD RAID10: Improve redundancy for 'far' and 'offset' algorithms (part 1)")
> > Reported-by: Yuhao Jiang<danisjiang@gmail.com>
>
> So again I can't find a report, and Reported-by usually should be followed
> by a Closes link to the original report.
>
> Applied with Reported-by tag removed.
>
> > Cc:stable@vger.kernel.org
> > Signed-off-by: Junrui Luo<moonafterrain@outlook.com>
> > ---
> > drivers/md/raid10.c | 2 ++
> > 1 file changed, 2 insertions(+)
>
> --
> Thansk,
> Kuai
--
Yuhao Jiang
^ permalink raw reply
* Re: [PATCH 0/2] md: small cleanups in MD driver
From: Yu Kuai @ 2026-04-19 6:06 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260415140319.376578-1-abd.masalkhi@gmail.com>
在 2026/4/15 22:03, Abd-Alrhman Masalkhi 写道:
> Hi,
>
> This small series contains two cleanups in the MD driver.
>
> Patch 1: replaces a wait loop in md_handle_request() with wait_event()
> Patch 2: replaces a mutex_lock() with mddev_lock_nointr()
>
> Thanks,
> Abd-Alrhman
>
> Abd-Alrhman Masalkhi (2):
> md: replace wait loop with wait_event() in md_handle_request()
> md: use mddev_lock_nointr() in mddev_suspend_and_lock_nointr()
>
> drivers/md/md.c | 10 +---------
> drivers/md/md.h | 2 +-
> 2 files changed, 2 insertions(+), 10 deletions(-)
Applied
--
Thansk,
Kuai
^ permalink raw reply
* Re: [RFC PATCH 2/2] kernel/module: Decouple klp and ftrace from load_module
From: Masami Hiramatsu @ 2026-04-20 2:27 UTC (permalink / raw)
To: Petr Mladek
Cc: Petr Pavlu, Song Chen, rafael, lenb, mturquette, sboyd,
viresh.kumar, agk, snitzer, mpatocka, bmarzins, song, yukuai,
linan122, jason.wessel, danielt, dianders, horms, davem, edumazet,
kuba, pabeni, paulmck, frederic, mcgrof, da.gomez, samitolvanen,
atomlin, jpoimboe, jikos, mbenes, joe.lawrence, rostedt, mhiramat,
mark.rutland, mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <aeD2_FrFL6E3dbAC@pathway.suse.cz>
On Thu, 16 Apr 2026 16:49:32 +0200
Petr Mladek <pmladek@suse.com> wrote:
> On Thu 2026-04-16 13:18:30, Petr Pavlu wrote:
> > On 4/15/26 8:43 AM, Song Chen wrote:
> > > On 4/14/26 22:33, Petr Pavlu wrote:
> > >> On 4/13/26 10:07 AM, chensong_2000@189.cn wrote:
> > >>> diff --git a/include/linux/module.h b/include/linux/module.h
> > >>> index 14f391b186c6..0bdd56f9defd 100644
> > >>> --- a/include/linux/module.h
> > >>> +++ b/include/linux/module.h
> > >>> @@ -308,6 +308,14 @@ enum module_state {
> > >>> MODULE_STATE_COMING, /* Full formed, running module_init. */
> > >>> MODULE_STATE_GOING, /* Going away. */
> > >>> MODULE_STATE_UNFORMED, /* Still setting it up. */
> > >>> + MODULE_STATE_FORMED,
> > >>
> > >> I don't see a reason to add a new module state. Why is it necessary and
> > >> how does it fit with the existing states?
> > >>
> > > because once notifier fails in state MODULE_STATE_UNFORMED (now only ftrace has someting to do in this state), notifier chain will roll back by calling blocking_notifier_call_chain_robust, i'm afraid MODULE_STATE_GOING is going to jeopardise the notifers which don't handle it appropriately, like:
> > >
> > > case MODULE_STATE_COMING:
> > > kmalloc();
> > > case MODULE_STATE_GOING:
> > > kfree();
> >
> > My understanding is that the current module "state machine" operates as
> > follows. Transitions marked with an asterisk (*) are announced via the
> > module notifier.
> >
> > ---> UNFORMED --*> COMING --*> LIVE --*> GOING -.
> > ^ | ^ |
> > | '---------------------* |
> > '---------------------------------------'
> >
> > The new code aims to replace the current ftrace_module_init() call in
> > load_module(). To achieve this, it adds a notification for the UNFORMED
> > state (only when loading a module) and introduces a new FORMED state for
> > rollback. FORMED is purely a fake state because it never appears in
> > module::state. The new structure is as follows:
> >
> > ,--*> (FORMED)
> > |
> > --*> UNFORMED --*> COMING --*> LIVE --*> GOING -.
> > ^ | ^ |
> > | '---------------------* |
> > '---------------------------------------'
> >
> > I'm afraid this is quite complex and inconsistent. Unless it can be kept
> > simple, we would be just replacing one special handling with a different
> > complexity, which is not worth it.
>
> > >>
> > >>> + if (err)
> > >>> + goto ddebug_cleanup;
> > >>> /* Finally it's fully formed, ready to start executing. */
> > >>> err = complete_formation(mod, info);
> > >>> - if (err)
> > >>> + if (err) {
> > >>> + blocking_notifier_call_chain_reverse(&module_notify_list,
> > >>> + MODULE_STATE_FORMED, mod);
> > >>> goto ddebug_cleanup;
> > >>> + }
> > >>> - err = prepare_coming_module(mod);
> > >>> + err = prepare_module_state_transaction(mod,
> > >>> + MODULE_STATE_COMING, MODULE_STATE_GOING);
> > >>> if (err)
> > >>> goto bug_cleanup;
> > >>> @@ -3522,7 +3519,6 @@ static int load_module(struct load_info *info, const char __user *uargs,
> > >>> destroy_params(mod->kp, mod->num_kp);
> > >>> blocking_notifier_call_chain(&module_notify_list,
> > >>> MODULE_STATE_GOING, mod);
> > >>
> > >> My understanding is that all notifier chains for MODULE_STATE_GOING
> > >> should be reversed.
> > > yes, all, from lowest priority notifier to highest.
> > > I will resend patch 1 which was failed due to my proxy setting.
> >
> > What I meant here is that the call:
> >
> > blocking_notifier_call_chain(&module_notify_list, MODULE_STATE_GOING, mod);
> >
> > should be replaced with:
> >
> > blocking_notifier_call_chain_reverse(&module_notify_list, MODULE_STATE_GOING, mod);
> >
> > >
> > >>
> > >>> - klp_module_going(mod);
> > >>> bug_cleanup:
> > >>> mod->state = MODULE_STATE_GOING;
> > >>> /* module_bug_cleanup needs module_mutex protection */
> > >>
> > >> The patch removes the klp_module_going() cleanup call in load_module().
> > >> Similarly, the ftrace_release_mod() call under the ddebug_cleanup label
> > >> should be removed and appropriately replaced with a cleanup via
> > >> a notifier.
> > >>
> > > err = prepare_module_state_transaction(mod,
> > > MODULE_STATE_UNFORMED, MODULE_STATE_FORMED);
> > > if (err)
> > > goto ddebug_cleanup;
> > >
> > > ftrace will be cleanup in blocking_notifier_call_chain_robust rolling back.
> > >
> > > err = prepare_module_state_transaction(mod,
> > > MODULE_STATE_COMING, MODULE_STATE_GOING);
> > >
> > > each notifier including ftrace and klp will be cleanup in blocking_notifier_call_chain_robust rolling back.
> > >
> > > if all notifiers are successful in MODULE_STATE_COMING, they all will be clean up in
> > > coming_cleanup:
> > > mod->state = MODULE_STATE_GOING;
> > > destroy_params(mod->kp, mod->num_kp);
> > > blocking_notifier_call_chain(&module_notify_list,
> > > MODULE_STATE_GOING, mod);
> > >
> > > if something wrong underneath.
> >
> > My point is that the patch leaves a call to ftrace_release_mod() in
> > load_module(), which I expected to be handled via a notifier.
>
> I think that I have got it. The ftrace code needs two notifiers when
> the module is being loaded and two when it is going.
>
> This is why Sond added the new state. But I think that we would
> need two new states to call:
>
> + ftrace_module_init() in MODULE_STATE_UNFORMED
> + ftrace_module_enable() in MODULE_STATE_FORMED
>
> and
>
> + ftrace_free_mem() in MODULE_STATE_PRE_GOING
> + ftrace_free_mem() in MODULE_STATE_GOING
>
>
> By using the ascii art:
>
> -*> UNFORMED -*> FORMED -> COMING -*> LIVE -*> PRE_GOING -*> GOING -.
> | | | ^ ^ ^
> | | '----------------' | |
> | '--------------------------------------' |
> '------------------------------------------------------'
>
>
> But I think that this is not worth it.
Agree.
If this needs to be ordered so strictly, why we will use a "single"
module notifier chain for this complex situation?
I think the notifier call chain is just for notice a single signal,
instead of sending several different signals, especially if there is
any dependency among the callbacks.
If notification callbacks need to be ordered, they are currently
sorted by representing priority numerically, but this is quite
fragile for updating. It has to look up other registered priorities
and adjust the order among dependencies each time. For this reason,
this mechanism is not suitable for global ordering. (It's like line
numbers in BASIC.)
It is probably only useful for representing dependencies between
two components maintained by the same maintainer.
I'm against a general-purpose system that makes everything modular.
It unnecessarily complicates things. If there are processes that
require strict ordering, especially processes that must be performed
before each stage as part of the framework, they should be called
directly from the framework, not via notification callbacks.
This makes it simpler and more robust to maintain.
Only the framework's end users should utilize notification callbacks.
Thank you,
>
> Best Regards,
> Petr
>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store
From: Xiao Ni @ 2026-04-20 5:21 UTC (permalink / raw)
To: Su Yue; +Cc: Su Yue, linux-raid, song, linan122, yukuai, heming.zhao
In-Reply-To: <h5pb6lqs.fsf@damenly.org>
On Thu, Apr 16, 2026 at 10:09 PM Su Yue <l@damenly.org> wrote:
>
> On Wed 15 Apr 2026 at 18:34, Xiao Ni <xni@redhat.com> wrote:
>
> > On Tue, Apr 7, 2026 at 6:26 PM Su Yue <glass.su@suse.com> wrote:
> >>
> >> If bitmap/location is present, mdadm will call
> >> update_array_info()
> >> while growing bitmap from none to internal via
> >> location_store().
> >> md_bitmap_create() is needed to set mddev->bitmap_ops otherwise
> >> mddev->bitmap_ops->get_stats() in update_array_info() will
> >> trigger
> >> kernel NULL pointer dereference.
> >
> >
> > Hi Su Yue
> >
> > How can bitmap/location be present when bitmap is none? Could
> > you
> > provide the test commands that reproduce this problem?
> >
> Sorry for the misleading commit message. It can only be reproduced
> patch 3 is appiled.
> I adjusted the sequence of this patch for easy review because
> md_bitmap_create,destroy
> are touched in patch1,2 and 3. Also if put the patch after 3rd
> patch,
> it will break ability to bisect.
>
> # mdadm --create --assume-clean /dev/md0 -f --bitmap=internal
> --raid-devices=2 --level=mirror --metadata=1.2 /dev/vdc /dev/vdd
> # mdadm --grow /dev/md0 --bitmap=none
> # mdadm --grow /dev/md0 --bitmap=internal # step 3
> # mdadm --grow /dev/md0 --bitmap=none # step 4
> [1] 2325 killed mdadm --grow /dev/md0 --bitmap=none
>
> When step 3 is called,
> md_bitmap_destroy() is called in update_array_info() to set NULL
> mddev->bitmap_ops
> then in step 4 kernel Oops is triggered.
>
>
> I am willing to amend commit message or move it after patch 3 if
> you would like.
Hi Su
Thanks for the detail explanation. After reading patch3, I totoally
understand. The sequence is good to me. And yes, it's better to
explain that this is needed after patch3.
Best Regards
Xiao
>
> --
> Su
>
> >
> > mdadm -CR /dev/md0 -l1 -n2 /dev/loop0 /dev/loop1 --bitmap=none
> > (There
> > is not bitmap/location, because bitmap directory is not created)
> > mdadm /dev/md0 --grow --bitmap=internal
> > Grow.c md_set_array_info runs
> > 451 array.state |= (1 << MD_SB_BITMAP_PRESENT);
> > 452 rv = md_set_array_info(fd, &array);
> > In kernel space, it runs
> > 8125 rv = md_bitmap_create(mddev);
> > 8126 if (!rv)
> > 8127 rv = mddev->bitmap_ops->load(mddev);
> >
> > Best Regards
> > Xiao
> >
> >>
> >> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of
> >> bitmap_ops until creating bitmap")
> >> Signed-off-by: Su Yue <glass.su@suse.com>
> >> ---
> >> drivers/md/md-bitmap.c | 11 ++++++++---
> >> drivers/md/md.c | 4 ++--
> >> drivers/md/md.h | 2 ++
> >> 3 files changed, 12 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> >> index 83378c033c72..2f24aae05552 100644
> >> --- a/drivers/md/md-bitmap.c
> >> +++ b/drivers/md/md-bitmap.c
> >> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const
> >> char *buf, size_t len)
> >> goto out;
> >> }
> >>
> >> - bitmap_destroy(mddev);
> >> + md_bitmap_destroy(mddev);
> >> mddev->bitmap_info.offset = 0;
> >> if (mddev->bitmap_info.file) {
> >> struct file *f =
> >> mddev->bitmap_info.file;
> >> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev,
> >> const char *buf, size_t len)
> >> goto out;
> >> }
> >>
> >> + /*
> >> + * lockless bitmap shoudle have set
> >> bitmap_id
> >> + * using bitmap_type, so always
> >> ID_BITMAP.
> >> + */
> >> + mddev->bitmap_id = ID_BITMAP;
> >> mddev->bitmap_info.offset = offset;
> >> - rv = bitmap_create(mddev);
> >> + rv = md_bitmap_create(mddev);
> >> if (rv)
> >> goto out;
> >>
> >> rv = bitmap_load(mddev);
> >> if (rv) {
> >> mddev->bitmap_info.offset = 0;
> >> - bitmap_destroy(mddev);
> >> + md_bitmap_destroy(mddev);
> >> goto out;
> >> }
> >> }
> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >> index 3ce6f9e9d38e..8b1ecc370ad6 100644
> >> --- a/drivers/md/md.c
> >> +++ b/drivers/md/md.c
> >> @@ -6447,7 +6447,7 @@ static void md_safemode_timeout(struct
> >> timer_list *t)
> >>
> >> static int start_dirty_degraded;
> >>
> >> -static int md_bitmap_create(struct mddev *mddev)
> >> +int md_bitmap_create(struct mddev *mddev)
> >> {
> >> if (mddev->bitmap_id == ID_BITMAP_NONE)
> >> return -EINVAL;
> >> @@ -6458,7 +6458,7 @@ static int md_bitmap_create(struct mddev
> >> *mddev)
> >> return mddev->bitmap_ops->create(mddev);
> >> }
> >>
> >> -static void md_bitmap_destroy(struct mddev *mddev)
> >> +void md_bitmap_destroy(struct mddev *mddev)
> >> {
> >> if (!md_bitmap_registered(mddev))
> >> return;
> >> diff --git a/drivers/md/md.h b/drivers/md/md.h
> >> index ac84289664cd..ed69244af00d 100644
> >> --- a/drivers/md/md.h
> >> +++ b/drivers/md/md.h
> >> @@ -895,6 +895,8 @@ static inline void safe_put_page(struct
> >> page *p)
> >>
> >> int register_md_submodule(struct md_submodule_head *msh);
> >> void unregister_md_submodule(struct md_submodule_head *msh);
> >> +int md_bitmap_create(struct mddev *mddev);
> >> +void md_bitmap_destroy(struct mddev *mddev);
> >>
> >> extern struct md_thread *md_register_thread(
> >> void (*run)(struct md_thread *thread),
> >> --
> >> 2.53.0
> >>
>
^ permalink raw reply
* Re: [PATCH v2 2/5] md/md-bitmap: add an extra sysfs argument to md_bitmap_create and destroy
From: Xiao Ni @ 2026-04-20 5:24 UTC (permalink / raw)
To: Su Yue; +Cc: linux-raid, song, linan122, yukuai, heming.zhao, l
In-Reply-To: <20260407102625.5686-3-glass.su@suse.com>
On Tue, Apr 7, 2026 at 6:26 PM Su Yue <glass.su@suse.com> wrote:
>
> For further use, no functional change.
>
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
> drivers/md/md-bitmap.c | 6 +++---
> drivers/md/md.c | 36 ++++++++++++++++++------------------
> drivers/md/md.h | 4 ++--
> 3 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index 2f24aae05552..ac06c9647bf0 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
> goto out;
> }
>
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> mddev->bitmap_info.offset = 0;
> if (mddev->bitmap_info.file) {
> struct file *f = mddev->bitmap_info.file;
> @@ -2659,14 +2659,14 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
> */
> mddev->bitmap_id = ID_BITMAP;
> mddev->bitmap_info.offset = offset;
> - rv = md_bitmap_create(mddev);
> + rv = md_bitmap_create(mddev, true);
> if (rv)
> goto out;
>
> rv = bitmap_load(mddev);
> if (rv) {
> mddev->bitmap_info.offset = 0;
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> goto out;
> }
> }
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 8b1ecc370ad6..d3c8f77b4fe3 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -678,7 +678,7 @@ static void active_io_release(struct percpu_ref *ref)
>
> static void no_op(struct percpu_ref *r) {}
>
> -static bool mddev_set_bitmap_ops(struct mddev *mddev)
> +static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
> {
> struct bitmap_operations *old = mddev->bitmap_ops;
> struct md_submodule_head *head;
> @@ -703,7 +703,7 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
> mddev->bitmap_ops = (void *)head;
> xa_unlock(&md_submodule);
>
> - if (!mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
> + if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
> if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> pr_warn("md: cannot register extra bitmap attributes for %s\n",
> mdname(mddev));
> @@ -721,9 +721,9 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev)
> return false;
> }
>
> -static void mddev_clear_bitmap_ops(struct mddev *mddev)
> +static void mddev_clear_bitmap_ops(struct mddev *mddev, bool remove_sysfs)
> {
> - if (!mddev_is_dm(mddev) && mddev->bitmap_ops &&
> + if (remove_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops &&
> mddev->bitmap_ops->group)
> sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
>
> @@ -6447,24 +6447,24 @@ static void md_safemode_timeout(struct timer_list *t)
>
> static int start_dirty_degraded;
>
> -int md_bitmap_create(struct mddev *mddev)
> +int md_bitmap_create(struct mddev *mddev, bool create_sysfs)
> {
> if (mddev->bitmap_id == ID_BITMAP_NONE)
> return -EINVAL;
>
> - if (!mddev_set_bitmap_ops(mddev))
> + if (!mddev_set_bitmap_ops(mddev, create_sysfs))
> return -ENOENT;
>
> return mddev->bitmap_ops->create(mddev);
> }
>
> -void md_bitmap_destroy(struct mddev *mddev)
> +void md_bitmap_destroy(struct mddev *mddev, bool remove_sysfs)
> {
> if (!md_bitmap_registered(mddev))
> return;
>
> mddev->bitmap_ops->destroy(mddev);
> - mddev_clear_bitmap_ops(mddev);
> + mddev_clear_bitmap_ops(mddev, remove_sysfs);
> }
>
> int md_run(struct mddev *mddev)
> @@ -6612,7 +6612,7 @@ int md_run(struct mddev *mddev)
> }
> if (err == 0 && pers->sync_request &&
> (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
> - err = md_bitmap_create(mddev);
> + err = md_bitmap_create(mddev, true);
> if (err)
> pr_warn("%s: failed to create bitmap (%d)\n",
> mdname(mddev), err);
> @@ -6685,7 +6685,7 @@ int md_run(struct mddev *mddev)
> pers->free(mddev, mddev->private);
> mddev->private = NULL;
> put_pers(pers);
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> return err;
> }
> EXPORT_SYMBOL_GPL(md_run);
> @@ -6702,7 +6702,7 @@ int do_md_run(struct mddev *mddev)
> if (md_bitmap_registered(mddev)) {
> err = mddev->bitmap_ops->load(mddev);
> if (err) {
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> goto out;
> }
> }
> @@ -6903,7 +6903,7 @@ static void __md_stop(struct mddev *mddev)
> {
> struct md_personality *pers = mddev->pers;
>
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> mddev_detach(mddev);
> spin_lock(&mddev->lock);
> mddev->pers = NULL;
> @@ -7680,16 +7680,16 @@ static int set_bitmap_file(struct mddev *mddev, int fd)
> err = 0;
> if (mddev->pers) {
> if (fd >= 0) {
> - err = md_bitmap_create(mddev);
> + err = md_bitmap_create(mddev, true);
> if (!err)
> err = mddev->bitmap_ops->load(mddev);
>
> if (err) {
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> fd = -1;
> }
> } else if (fd < 0) {
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> }
> }
>
> @@ -7996,12 +7996,12 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
> mddev->bitmap_info.default_offset;
> mddev->bitmap_info.space =
> mddev->bitmap_info.default_space;
> - rv = md_bitmap_create(mddev);
> + rv = md_bitmap_create(mddev, true);
> if (!rv)
> rv = mddev->bitmap_ops->load(mddev);
>
> if (rv)
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> } else {
> struct md_bitmap_stats stats;
>
> @@ -8027,7 +8027,7 @@ static int update_array_info(struct mddev *mddev, mdu_array_info_t *info)
> put_cluster_ops(mddev);
> mddev->safemode_delay = DEFAULT_SAFEMODE_DELAY;
> }
> - md_bitmap_destroy(mddev);
> + md_bitmap_destroy(mddev, true);
> mddev->bitmap_info.offset = 0;
> }
> }
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index ed69244af00d..4aaba3d7015c 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -895,8 +895,8 @@ static inline void safe_put_page(struct page *p)
>
> int register_md_submodule(struct md_submodule_head *msh);
> void unregister_md_submodule(struct md_submodule_head *msh);
> -int md_bitmap_create(struct mddev *mddev);
> -void md_bitmap_destroy(struct mddev *mddev);
> +int md_bitmap_create(struct mddev *mddev, bool create_sysfs);
> +void md_bitmap_destroy(struct mddev *mddev, bool remove_sysfs);
>
> extern struct md_thread *md_register_thread(
> void (*run)(struct md_thread *thread),
> --
> 2.53.0
>
Looks good to me.
Reviewed-by: Xiao Ni <xni@redhat.com>
^ permalink raw reply
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Masami Hiramatsu @ 2026-04-20 5:44 UTC (permalink / raw)
To: chensong_2000
Cc: rafael, lenb, mturquette, sboyd, viresh.kumar, agk, snitzer,
mpatocka, bmarzins, song, yukuai, linan122, jason.wessel, danielt,
dianders, horms, davem, edumazet, kuba, pabeni, paulmck, frederic,
mcgrof, petr.pavlu, da.gomez, samitolvanen, atomlin, jpoimboe,
jikos, mbenes, pmladek, joe.lawrence, rostedt, mark.rutland,
mathieu.desnoyers, linux-modules, linux-kernel,
linux-trace-kernel, linux-acpi, linux-clk, linux-pm,
live-patching, dm-devel, linux-raid, kgdb-bugreport, netdev
In-Reply-To: <20260415070137.17860-1-chensong_2000@189.cn>
Hi Song,
On Wed, 15 Apr 2026 15:01:37 +0800
chensong_2000@189.cn wrote:
> From: Song Chen <chensong_2000@189.cn>
>
> The current notifier chain implementation uses a single-linked list
> (struct notifier_block *next), which only supports forward traversal
> in priority order. This makes it difficult to handle cleanup/teardown
> scenarios that require notifiers to be called in reverse priority order.
What about introducing a new notification callback API that allows you
to describe dependencies between callback functions?
For example, when registering a callback, you could register a string
as an ID and specify whether to call it before or after that ID,
or you could register a comparison function that is called when adding
to a list. (I prefer @name and @depends fields so that it can be easily
maintained.)
This would allow for better dependency building when adding to the list.
>
> A concrete example is the ordering dependency between ftrace and
> livepatch during module load/unload. see the detail here [1].
If this only concerns notification callback issues with the ftrace
and livepatch modules, it's far more robust to simply call the
necessary processing directly when the modules load and unload,
rather than registering notification callbacks externally.
There are fprobe, kprobe and its trace-events, all of them are using
ftrace as its fundation layer. In this case, I always needs to
consider callback order when a module is unloaded.
If ftrace is working as a part of module callbacks, it will conflict
with fprobe/kprobe module callback. Of course we can reorder it with
modifying its priority. But this is ugly, because when we introduce
a new other feature which depends on another layer, we need to
reorder the callback's priority number on the list.
Based on the above, I don't think this can be resolved simply by
changing the list of notification callbacks to a bidirectional list.
Thank you,
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 3/5] md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset
From: Xiao Ni @ 2026-04-20 7:05 UTC (permalink / raw)
To: Su Yue; +Cc: linux-raid, song, linan122, yukuai, heming.zhao, l
In-Reply-To: <20260407102625.5686-4-glass.su@suse.com>
On Tue, Apr 7, 2026 at 6:27 PM Su Yue <glass.su@suse.com> wrote:
>
> Before commit fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
> if CONFIG_MD_BITMAP is enabled, both bitmap none, internal and clustered have
> the sysfs file bitmap/location.
>
> After the commit, if bitmap is none, bitmap/location doesn't exist anymore.
> It breaks 'grow' behavior of a md array of madam with MD_FEATURE_BITMAP_OFFSET.
> Take level=mirror and metadata=1.2 as an example:
>
> $ mdadm --create /dev/md0 -f --bitmap=none --raid-devices=2 --level=mirror \
> --metadata=1.2 /dev/vdd /dev/vde
> $ mdadm --grow /dev/md0 --bitmap=internal
> $ cat /sys/block/md0/md/bitmap/location
> Before:+8
> After: +2
>
> While growing bitmap from none to internal, clustered and llbitmap,
> mdadm/Grow.c:Grow_addbitmap() tries to detect bitmap/location first.
> 1)If bitmap/location exists, it sets bitmap/location after getinfo_super().
> 2)If bitmap/location doesn't exist, mdadm just calls md_set_array_info() then
> mddev->bitmap_info.default_offset will be used.
> Situation can be worse if growing none to clustered, bitmap offset of the node
> calling `madm --grow` will be changed but the other node are reading bitmap sb from
> the old location.
>
> Here introducing a dummy bitmap_operations for ID_BITMAP_NONE to restore sysfs
> file bitmap/location for ID_BITMAP_NONE and ID_BITMAP.
> location_store() now calls md_bitmap_(create|destroy) with false sysfs parameter then
> (add,remove)_internal_bitmap() will be called to manage sysfs entries.
>
> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
> Suggested-by: Yu Kuai <yukuai@fnnas.com>
> Signed-off-by: Su Yue <glass.su@suse.com>
> ---
> drivers/md/md-bitmap.c | 116 ++++++++++++++++++++++++++++++++++++---
> drivers/md/md-bitmap.h | 3 +
> drivers/md/md-llbitmap.c | 12 ++++
> drivers/md/md.c | 16 +++---
> 4 files changed, 130 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index ac06c9647bf0..a8a176428c61 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -240,6 +240,11 @@ static bool bitmap_enabled(void *data, bool flush)
> bitmap->storage.filemap != NULL;
> }
>
> +static bool dummy_bitmap_enabled(void *data, bool flush)
> +{
> + return false;
> +}
> +
> /*
> * check a page and, if necessary, allocate it (or hijack it if the alloc fails)
> *
> @@ -2201,6 +2206,11 @@ static int bitmap_create(struct mddev *mddev)
> return 0;
> }
>
> +static int dummy_bitmap_create(struct mddev *mddev)
> +{
> + return 0;
> +}
> +
> static int bitmap_load(struct mddev *mddev)
> {
> int err = 0;
> @@ -2594,6 +2604,9 @@ location_show(struct mddev *mddev, char *page)
> return len;
> }
>
> +static int create_internal_group(struct mddev *mddev);
> +static void remove_internal_group(struct mddev *mddev);
> +
> static ssize_t
> location_store(struct mddev *mddev, const char *buf, size_t len)
> {
> @@ -2618,7 +2631,8 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
> goto out;
> }
>
> - md_bitmap_destroy(mddev, true);
> + md_bitmap_destroy(mddev, false);
> + remove_internal_group(mddev);
> mddev->bitmap_info.offset = 0;
> if (mddev->bitmap_info.file) {
> struct file *f = mddev->bitmap_info.file;
> @@ -2659,14 +2673,22 @@ location_store(struct mddev *mddev, const char *buf, size_t len)
> */
> mddev->bitmap_id = ID_BITMAP;
> mddev->bitmap_info.offset = offset;
> - rv = md_bitmap_create(mddev, true);
> + rv = md_bitmap_create(mddev, false);
> if (rv)
> goto out;
>
> + rv = create_internal_group(mddev);
> + if (rv) {
> + mddev->bitmap_info.offset = 0;
> + bitmap_destroy(mddev);
> + goto out;
> + }
> +
> rv = bitmap_load(mddev);
> if (rv) {
> + remove_internal_group(mddev);
> mddev->bitmap_info.offset = 0;
> - md_bitmap_destroy(mddev, true);
> + md_bitmap_destroy(mddev, false);
> goto out;
> }
> }
> @@ -2960,8 +2982,7 @@ static struct md_sysfs_entry max_backlog_used =
> __ATTR(max_backlog_used, S_IRUGO | S_IWUSR,
> behind_writes_used_show, behind_writes_used_reset);
>
> -static struct attribute *md_bitmap_attrs[] = {
> - &bitmap_location.attr,
> +static struct attribute *internal_bitmap_attrs[] = {
> &bitmap_space.attr,
> &bitmap_timeout.attr,
> &bitmap_backlog.attr,
> @@ -2972,11 +2993,57 @@ static struct attribute *md_bitmap_attrs[] = {
> NULL
> };
>
> -static struct attribute_group md_bitmap_group = {
> +static struct attribute_group internal_bitmap_group = {
> .name = "bitmap",
> - .attrs = md_bitmap_attrs,
> + .attrs = internal_bitmap_attrs,
> };
>
> +/* Only necessary attrs for compatibility */
> +static struct attribute *common_bitmap_attrs[] = {
> + &bitmap_location.attr,
> + NULL
> +};
> +
> +static const struct attribute_group common_bitmap_group = {
> + .name = "bitmap",
> + .attrs = common_bitmap_attrs,
> +};
> +
> +static int create_internal_group(struct mddev *mddev)
> +{
> + /*
> + * md_bitmap_group and md_bitmap_common_group are using same name
> + * 'bitmap'.
> + */
> + return sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
> +}
> +
> +static void remove_internal_group(struct mddev *mddev)
> +{
> + sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
> +}
> +
> +static int bitmap_register_groups(struct mddev *mddev)
> +{
> + int ret;
> +
> + ret = sysfs_create_group(&mddev->kobj, &common_bitmap_group);
> +
> + if (ret)
> + return ret;
> +
> + ret = sysfs_merge_group(&mddev->kobj, &internal_bitmap_group);
> + if (ret)
> + sysfs_remove_group(&mddev->kobj, &common_bitmap_group);
> +
> + return ret;
> +}
> +
> +static void bitmap_unregister_groups(struct mddev *mddev)
> +{
> + sysfs_unmerge_group(&mddev->kobj, &internal_bitmap_group);
> +}
Hi Su
bitmap_unregister_groups should also remove common_bitmap_group, right?
> +
> static struct bitmap_operations bitmap_ops = {
You already changed md_bitmap_group to internal_bitmap_group. It's
better to change bitmap_ops to internal_bitmap_ops?
> .head = {
> .type = MD_BITMAP,
> @@ -3018,21 +3085,52 @@ static struct bitmap_operations bitmap_ops = {
> .set_pages = bitmap_set_pages,
> .free = md_bitmap_free,
>
> - .group = &md_bitmap_group,
> + .register_groups = bitmap_register_groups,
> + .unregister_groups = bitmap_unregister_groups,
> +};
> +
> +static int none_bitmap_register_groups(struct mddev *mddev)
> +{
> + return sysfs_create_group(&mddev->kobj, &common_bitmap_group);
> +}
> +
> +static struct bitmap_operations none_bitmap_ops = {
> + .head = {
> + .type = MD_BITMAP,
> + .id = ID_BITMAP_NONE,
> + .name = "none",
> + },
> +
> + .enabled = dummy_bitmap_enabled,
> + .create = dummy_bitmap_create,
> + .destroy = bitmap_destroy,
> + .load = bitmap_load,
> + .get_stats = bitmap_get_stats,
> + .free = md_bitmap_free,
> +
> + .register_groups = none_bitmap_register_groups,
> + .unregister_groups = NULL,
How does bitmap/location can be deleted if array is created with no
bitmap? Can the `mdadm --stop` command get stuck?
Best Regards
Xiao
> };
>
> int md_bitmap_init(void)
> {
> + int ret;
> +
> md_bitmap_wq = alloc_workqueue("md_bitmap", WQ_MEM_RECLAIM | WQ_UNBOUND,
> 0);
> if (!md_bitmap_wq)
> return -ENOMEM;
>
> - return register_md_submodule(&bitmap_ops.head);
> + ret = register_md_submodule(&bitmap_ops.head);
> + if (ret)
> + return ret;
> +
> + return register_md_submodule(&none_bitmap_ops.head);
> }
>
> void md_bitmap_exit(void)
> {
> destroy_workqueue(md_bitmap_wq);
> unregister_md_submodule(&bitmap_ops.head);
> + unregister_md_submodule(&none_bitmap_ops.head);
> }
> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
> index b42a28fa83a0..10bc6854798c 100644
> --- a/drivers/md/md-bitmap.h
> +++ b/drivers/md/md-bitmap.h
> @@ -125,6 +125,9 @@ struct bitmap_operations {
> void (*set_pages)(void *data, unsigned long pages);
> void (*free)(void *data);
>
> + int (*register_groups)(struct mddev *mddev);
> + void (*unregister_groups)(struct mddev *mddev);
> +
> struct attribute_group *group;
> };
>
> diff --git a/drivers/md/md-llbitmap.c b/drivers/md/md-llbitmap.c
> index bf398d7476b3..9b3ea4f1d268 100644
> --- a/drivers/md/md-llbitmap.c
> +++ b/drivers/md/md-llbitmap.c
> @@ -1561,6 +1561,16 @@ static struct attribute_group md_llbitmap_group = {
> .attrs = md_llbitmap_attrs,
> };
>
> +static int llbitmap_register_groups(struct mddev *mddev)
> +{
> + return sysfs_create_group(&mddev->kobj, &md_llbitmap_group);
> +}
> +
> +static void llbitmap_unregister_groups(struct mddev *mddev)
> +{
> + sysfs_remove_group(&mddev->kobj, &md_llbitmap_group);
> +}
> +
> static struct bitmap_operations llbitmap_ops = {
> .head = {
> .type = MD_BITMAP,
> @@ -1597,6 +1607,8 @@ static struct bitmap_operations llbitmap_ops = {
> .dirty_bits = llbitmap_dirty_bits,
> .write_all = llbitmap_write_all,
>
> + .register_groups = llbitmap_register_groups,
> + .unregister_groups = llbitmap_unregister_groups,
> .group = &md_llbitmap_group,
> };
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index d3c8f77b4fe3..55a95b227b83 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -683,8 +683,7 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
> struct bitmap_operations *old = mddev->bitmap_ops;
> struct md_submodule_head *head;
>
> - if (mddev->bitmap_id == ID_BITMAP_NONE ||
> - (old && old->head.id == mddev->bitmap_id))
> + if (old && old->head.id == mddev->bitmap_id)
> return true;
>
> xa_lock(&md_submodule);
> @@ -703,8 +702,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
> mddev->bitmap_ops = (void *)head;
> xa_unlock(&md_submodule);
>
> - if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->group) {
> - if (sysfs_create_group(&mddev->kobj, mddev->bitmap_ops->group))
> + if (create_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops->register_groups) {
> + if (mddev->bitmap_ops->register_groups(mddev))
> pr_warn("md: cannot register extra bitmap attributes for %s\n",
> mdname(mddev));
> else
> @@ -724,8 +723,8 @@ static bool mddev_set_bitmap_ops(struct mddev *mddev, bool create_sysfs)
> static void mddev_clear_bitmap_ops(struct mddev *mddev, bool remove_sysfs)
> {
> if (remove_sysfs && !mddev_is_dm(mddev) && mddev->bitmap_ops &&
> - mddev->bitmap_ops->group)
> - sysfs_remove_group(&mddev->kobj, mddev->bitmap_ops->group);
> + mddev->bitmap_ops->unregister_groups)
> + mddev->bitmap_ops->unregister_groups(mddev);
>
> mddev->bitmap_ops = NULL;
> }
> @@ -6610,8 +6609,9 @@ int md_run(struct mddev *mddev)
> (unsigned long long)pers->size(mddev, 0, 0) / 2);
> err = -EINVAL;
> }
> - if (err == 0 && pers->sync_request &&
> - (mddev->bitmap_info.file || mddev->bitmap_info.offset)) {
> + if (err == 0 && pers->sync_request) {
> + if (mddev->bitmap_info.offset == 0)
> + mddev->bitmap_id = ID_BITMAP_NONE;
> err = md_bitmap_create(mddev, true);
> if (err)
> pr_warn("%s: failed to create bitmap (%d)\n",
> --
> 2.53.0
>
^ permalink raw reply
* Re: [PATCH] md: factor out cloned bio cleanup into md_free_bio()
From: Abd-Alrhman Masalkhi @ 2026-04-20 7:30 UTC (permalink / raw)
To: Yu Kuai, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <b33f52c1-0f10-4ac0-8792-4387848e40d4@fnnas.com>
On Sun, Apr 19, 2026 at 12:48 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/14 18:38, Abd-Alrhman Masalkhi 写道:
>> Refactor duplicated cloned bio completion and cleanup logic into
>> a new helper, md_free_bio().
>>
>> md_end_clone_io() and md_free_cloned_bio() previously shared nearly
>> identical teardown code, differing only in whether the original
>> bio’s endio callback was invoked. Introduce a boolean parameter
>> orig_endio to control this behavior and consolidate the logic.
>>
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> drivers/md/md.c | 26 +++++++++-----------------
>> 1 file changed, 9 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index ac71640ff3a8..707d605fee61 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9208,7 +9208,7 @@ static void md_bitmap_end(struct mddev *mddev, struct md_io_clone *md_io_clone)
>> fn(mddev, md_io_clone->offset, md_io_clone->sectors);
>> }
>>
>> -static void md_end_clone_io(struct bio *bio)
>> +static void md_free_bio(struct bio *bio, bool orig_endio)
>> {
>> struct md_io_clone *md_io_clone = bio->bi_private;
>> struct bio *orig_bio = md_io_clone->orig_bio;
>> @@ -9224,10 +9224,16 @@ static void md_end_clone_io(struct bio *bio)
>> bio_end_io_acct(orig_bio, md_io_clone->start_time);
>>
>> bio_put(bio);
>> - bio_endio(orig_bio);
>> + if (orig_endio)
>> + bio_endio(orig_bio);
>> percpu_ref_put(&mddev->active_io);
>> }
>>
>> +static void md_end_clone_io(struct bio *bio)
>> +{
>> + md_free_bio(bio, true);
>> +}
>> +
>> static void md_clone_bio(struct mddev *mddev, struct bio **bio)
>> {
>> struct block_device *bdev = (*bio)->bi_bdev;
>> @@ -9262,21 +9268,7 @@ EXPORT_SYMBOL_GPL(md_account_bio);
>>
>> void md_free_cloned_bio(struct bio *bio)
>> {
>> - struct md_io_clone *md_io_clone = bio->bi_private;
>> - struct bio *orig_bio = md_io_clone->orig_bio;
>> - struct mddev *mddev = md_io_clone->mddev;
>> -
>> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
>> - md_bitmap_end(mddev, md_io_clone);
>> -
>> - if (bio->bi_status && !orig_bio->bi_status)
>> - orig_bio->bi_status = bio->bi_status;
>> -
>> - if (md_io_clone->start_time)
>> - bio_end_io_acct(orig_bio, md_io_clone->start_time);
>> -
>> - bio_put(bio);
>> - percpu_ref_put(&mddev->active_io);
>> + md_free_bio(bio, false);
>> }
>> EXPORT_SYMBOL_GPL(md_free_cloned_bio);
>
> This patch is no longer needed after following patch:
> https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com
Thanks, I'll drop this patch.
>
>>
>
> --
> Thansk,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] md: use md_free_cloned_bio() in md_end_clone_io()
From: Abd-Alrhman Masalkhi @ 2026-04-20 7:34 UTC (permalink / raw)
To: Yu Kuai, song, linan666; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <36f8b219-140d-4d66-85f9-2522215f5a2b@fnnas.com>
Hi Kuai,
On Sun, Apr 19, 2026 at 12:51 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/15 16:19, Abd-Alrhman Masalkhi 写道:
>> md_end_clone_io() and md_free_cloned_bio() share identical teardown
>> logic. Use md_free_cloned_bio() in md_end_clone_io() for cleanup and
>> call bio_endio() afterwards.
>>
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> Changes in v2:
>> - Reuse md_free_cloned_bio() instead of introducing a new helper
>> - Link to v1: https://lore.kernel.org/linux-raid/20260414103813.307601-1-abd.masalkhi@gmail.com
>> ---
>> drivers/md/md.c | 13 +------------
>> 1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index ac71640ff3a8..8565566a447b 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -9212,20 +9212,9 @@ static void md_end_clone_io(struct bio *bio)
>> {
>> struct md_io_clone *md_io_clone = bio->bi_private;
>> struct bio *orig_bio = md_io_clone->orig_bio;
>> - struct mddev *mddev = md_io_clone->mddev;
>> -
>> - if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
>> - md_bitmap_end(mddev, md_io_clone);
>> -
>> - if (bio->bi_status && !orig_bio->bi_status)
>> - orig_bio->bi_status = bio->bi_status;
>> -
>> - if (md_io_clone->start_time)
>> - bio_end_io_acct(orig_bio, md_io_clone->start_time);
>>
>> - bio_put(bio);
>> + md_free_cloned_bio(bio);
>> bio_endio(orig_bio);
>> - percpu_ref_put(&mddev->active_io);
>> }
>>
>> static void md_clone_bio(struct mddev *mddev, struct bio **bio)
>
> Same as I replied to v1,
> This patch is no longer needed after following patch:
> https://lore.kernel.org/r/20260408043548.1695157-1-bmarzins@redhat.com
Thanks, I'll drop this patch.
>
> --
> Thansk,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* [PATCH AUTOSEL 7.0] md/raid0: use kvzalloc/kvfree for strip_zone and devlist allocations
From: Sasha Levin @ 2026-04-20 13:17 UTC (permalink / raw)
To: patches, stable
Cc: Gregory Price, syzbot+924649752adf0d3ac9dd, Yu Kuai, Li Nan,
Sasha Levin, song, linux-raid, linux-kernel
In-Reply-To: <20260420132314.1023554-1-sashal@kernel.org>
From: Gregory Price <gourry@gourry.net>
[ Upstream commit 078d1d8e688d75419abfedcae47eab8e42b991bb ]
syzbot reported a WARNING at mm/page_alloc.c:__alloc_frozen_pages_noprof()
triggered by create_strip_zones() in the RAID0 driver.
When raid_disks is large, the allocation size exceeds MAX_PAGE_ORDER (4MB
on x86), causing WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER).
Convert the strip_zone and devlist allocations from kzalloc/kzalloc_objs to
kvzalloc/kvzalloc_objs, which first attempts a contiguous allocation with
__GFP_NOWARN and then falls back to vmalloc for large sizes. Convert the
corresponding kfree calls to kvfree.
Both arrays are pure metadata lookup tables (arrays of pointers and zone
descriptors) accessed only via indexing, so they do not require physically
contiguous memory.
Reported-by: syzbot+924649752adf0d3ac9dd@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/69adaba8.a00a0220.b130.0005.GAE@google.com/
Signed-off-by: Gregory Price <gourry@gourry.net>
Reviewed-by: Yu Kuai <yukuai@fnnas.com>
Reviewed-by: Li Nan <linan122@huawei.com>
Link: https://lore.kernel.org/linux-raid/20260308234202.3118119-1-gourry@gourry.net/
Signed-off-by: Yu Kuai <yukuai@fnnas.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
This confirms the same bug also triggers on linux-5.15.y stable kernel.
Now I have comprehensive information for my analysis.
---
## PHASE 1: COMMIT MESSAGE FORENSICS
### Step 1.1: PARSE THE SUBJECT LINE
- **Subsystem prefix:** `md/raid0`
- **Action verb:** "use" (converting allocation API calls)
- **One-line summary:** Convert strip_zone and devlist allocations from
kzalloc to kvzalloc to avoid WARNING when allocation exceeds
MAX_PAGE_ORDER.
Record: [md/raid0] [use/convert] [Switch large allocations to kvzalloc
to avoid WARNING on large arrays]
### Step 1.2: PARSE ALL COMMIT MESSAGE TAGS
- **Reported-by:** syzbot+924649752adf0d3ac9dd@syzkaller.appspotmail.com
— syzbot fuzzer report, strong signal
- **Closes:** https://lore.kernel.org/all/69adaba8.a00a0220.b130.0005.GA
E@google.com/ — syzbot bug report
- **Signed-off-by:** Gregory Price <gourry@gourry.net> — patch author
- **Reviewed-by:** Yu Kuai <yukuai@fnnas.com> — MD subsystem maintainer
- **Reviewed-by:** Li Nan <linan122@huawei.com> — MD subsystem developer
- **Link:** https://lore.kernel.org/linux-
raid/20260308234202.3118119-1-gourry@gourry.net/
- **Signed-off-by:** Yu Kuai <yukuai@fnnas.com> — committer/maintainer
Record: Syzbot report, two Reviewed-by from MD maintainers, no Fixes:
tag (expected)
### Step 1.3: ANALYZE THE COMMIT BODY TEXT
The commit explains that when `raid_disks` is large, the allocation size
for `strip_zone` and `devlist` arrays exceeds `MAX_PAGE_ORDER` (4MB on
x86), triggering `WARN_ON_ONCE_GFP(order > MAX_PAGE_ORDER)`. The fix
converts to `kvzalloc`/`kvfree` which first tries contiguous allocation
with `__GFP_NOWARN` and then falls back to vmalloc. The author
explicitly notes these are "pure metadata lookup tables" accessed only
via indexing, so physically contiguous memory is not required.
Record: [Bug: WARNING triggered in page allocator when RAID0 array has
many disks] [Symptom: kernel WARNING at mm/page_alloc.c] [No specific
version info; bug present since original code in 2005] [Root cause:
kzalloc for variable-size arrays that can exceed MAX_PAGE_ORDER]
### Step 1.4: DETECT HIDDEN BUG FIXES
This is a clear fix for a syzbot-reported WARNING. Not disguised at all.
Record: [Not a hidden bug fix — explicitly described as fixing a
WARNING]
---
## PHASE 2: DIFF ANALYSIS — LINE BY LINE
### Step 2.1: INVENTORY THE CHANGES
- **File:** `drivers/md/raid0.c` — 9 lines changed (9 added, 9 removed)
- **Functions modified:**
- `create_strip_zones()` — allocation site + error cleanup path
- `raid0_free()` — normal cleanup path
- **Scope:** Single-file surgical fix. Minimal.
Record: [drivers/md/raid0.c: +9/-9] [create_strip_zones, raid0_free]
[Single-file surgical fix]
### Step 2.2: UNDERSTAND THE CODE FLOW CHANGE
**Hunk 1 (create_strip_zones allocation):**
- Before: `kzalloc_objs()` and `kzalloc()` for strip_zone and devlist
- After: `kvzalloc_objs()` and `kvzalloc()` — same semantics but with
vmalloc fallback
**Hunk 2 (abort label cleanup):**
- Before: `kfree(conf->strip_zone); kfree(conf->devlist);`
- After: `kvfree(conf->strip_zone); kvfree(conf->devlist);`
**Hunk 3 (raid0_free):**
- Before: `kfree(conf->strip_zone); kfree(conf->devlist);`
- After: `kvfree(conf->strip_zone); kvfree(conf->devlist);`
Record: [All three hunks: kzalloc->kvzalloc and kfree->kvfree, perfectly
paired]
### Step 2.3: IDENTIFY THE BUG MECHANISM
Category: **Logic/correctness fix** — Using physically-contiguous
allocation for data that doesn't need it, causing allocation
failures/warnings when size is large.
The code allocates `sizeof(struct strip_zone) * nr_strip_zones` and
`sizeof(struct md_rdev *) * nr_strip_zones * raid_disks`. When
`raid_disks` is large, this exceeds MAX_PAGE_ORDER (4MB), causing a
WARN_ON_ONCE.
The fix is the standard Linux kernel pattern: use `kvzalloc` (which
falls back to vmalloc) for allocations that don't require physical
contiguity.
Record: [Logic/allocation bug] [kzalloc can't handle large allocations >
MAX_PAGE_ORDER; kvzalloc falls back to vmalloc]
### Step 2.4: ASSESS THE FIX QUALITY
- **Obviously correct?** Yes. `kzalloc`→`kvzalloc` and `kfree`→`kvfree`
is an extremely common, well-understood pattern in the kernel.
- **Minimal?** Yes, only 9 lines changed (purely API substitution).
- **Regression risk?** Extremely low. `kvfree` correctly handles both
kmalloc and vmalloc memory. The arrays are metadata lookup tables
accessed via indexing — no DMA or physical contiguity requirement.
Record: [Excellent fix quality, minimal, obviously correct, no
regression risk]
---
## PHASE 3: GIT HISTORY INVESTIGATION
### Step 3.1: BLAME THE CHANGED LINES
- `conf->strip_zone` allocation: dates back to `1da177e4c3f41` (Linux
2.6.12, 2005) with wrapping by `kzalloc_objs` in 2026 (32a92f8c89326)
and earlier by `kcalloc` in 2018 (6396bb221514d2). Original code from
Linus's initial git commit.
- `conf->devlist` allocation: same — dates to `1da177e4c3f41` (2005).
- The kfree calls were refactored in `ed7b00380d957e` (2009) and
`d11854ed05635` (2024) but the fundamental issue (kzalloc for
variable-size metadata) has existed since 2005.
Record: [Buggy code introduced in original Linux 2.6.12 (2005)] [Present
in ALL stable trees]
### Step 3.2: FOLLOW THE FIXES: TAG
No Fixes: tag present (expected for autosel candidates).
Record: [No Fixes: tag — N/A]
### Step 3.3: CHECK FILE HISTORY
Recent `drivers/md/raid0.c` changes are mostly unrelated (alloc_obj
refactoring, mddev flags, dm-raid NULL fix, queue limits). The patch is
standalone.
Record: [No prerequisites identified] [Standalone fix]
### Step 3.4: CHECK THE AUTHOR
Gregory Price is primarily a CXL/mm developer, not the md subsystem
maintainer. But the fix was reviewed and committed by Yu Kuai, who IS
the MD subsystem maintainer.
Record: [Authored by Gregory Price (CXL/mm), reviewed and committed by
Yu Kuai (MD maintainer)]
### Step 3.5: CHECK FOR DEPENDENT/PREREQUISITE COMMITS
The key dependency concern: the mainline patch uses `kzalloc_objs` →
`kvzalloc_objs`, but `kzalloc_objs`/`kvzalloc_objs` macros only exist in
v7.0 (introduced by commit `2932ba8d9c99` in v7.0-rc1). In older stable
trees (6.12, 6.6, 6.1, 5.15), the code uses `kcalloc`/`kzalloc`, so the
backport would need trivial adaptation: `kcalloc` → `kvcalloc` (or
`kvzalloc` with size calculation), not `kzalloc_objs` → `kvzalloc_objs`.
This is a trivial adaptation. For this specific tree (7.0),
`kvzalloc_objs` is available and the patch applies cleanly.
Record: [For 7.0: applies cleanly. For older stable: needs trivial
adaptation of the alloc macro]
---
## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
### Step 4.1: FIND THE ORIGINAL PATCH DISCUSSION
From spinics.net mirror:
- The patch was submitted on 2026-03-08 as a single patch (not a
series).
- Yu Kuai reviewed it on 2026-03-20 with `Reviewed-by`.
- Li Nan reviewed it on 2026-03-21 with `Reviewed-by` and "LGTM".
- Yu Kuai applied it to md-7.1 on 2026-04-07, adding the `Closes:` tag.
- No objections, NAKs, or concerns raised.
Record: [Single patch, two reviewers, both approved, applied by
maintainer]
### Step 4.2: CHECK WHO REVIEWED
- Yu Kuai — MD subsystem maintainer (also the committer)
- Li Nan — MD subsystem developer at Huawei
Both are key people for the MD subsystem. Thorough review.
Record: [Key MD maintainers reviewed the patch]
### Step 4.3: SEARCH FOR THE BUG REPORT
The syzbot report confirms:
- **Upstream bug:** Reported 2026-03-08, fix commit `078d1d8e688d`
identified, patched on some CI instances.
- **5.15 stable bug:** Same WARNING also triggered on linux-5.15.y
(commit `91d48252ad4b`), confirming the bug affects old stable trees.
- Crash trace shows: `WARN_ON_ONCE` at
`__alloc_frozen_pages_noprof+0x23ea/0x2ba0`, triggered through
`create_strip_zones → raid0_run → md_run → do_md_run → md_ioctl →
blkdev_ioctl → vfs_ioctl → __x64_sys_ioctl`.
Record: [Syzbot reproduced on both upstream and linux-5.15.y] [Triggered
via ioctl syscall]
### Step 4.4/4.5: CHECK FOR RELATED PATCHES AND STABLE DISCUSSION
Single standalone patch. No series. No prior stable discussion found.
Record: [Standalone fix, no series context]
---
## PHASE 5: CODE SEMANTIC ANALYSIS
### Step 5.1: IDENTIFY KEY FUNCTIONS
- `create_strip_zones()` — allocates RAID0 metadata
- `raid0_free()` — frees RAID0 metadata
### Step 5.2: TRACE CALLERS
- `create_strip_zones()` is called from `raid0_run()` → called from
`md_run()` → called from `do_md_run()` → called from `md_ioctl()` →
reachable from userspace via `ioctl()`.
- `raid0_free()` is called during RAID0 teardown.
Record: [Reachable from userspace via ioctl syscall — confirmed by
syzbot stack trace]
### Step 5.3-5.5: CALLEES AND SIMILAR PATTERNS
The fix is purely about allocation strategy. No complex call chain
analysis needed.
Record: [Simple allocation API change, no complex callee analysis
needed]
---
## PHASE 6: CROSS-REFERENCING AND STABLE TREE ANALYSIS
### Step 6.1: DOES THE BUGGY CODE EXIST IN STABLE TREES?
Yes — the buggy `kzalloc`/`kcalloc` calls for strip_zone and devlist
have existed since Linux 2.6.12 (2005). Confirmed in v5.15 and v6.12.
Syzbot also reproduced the same WARNING on linux-5.15.y.
Record: [Bug exists in ALL active stable trees]
### Step 6.2: CHECK FOR BACKPORT COMPLICATIONS
- For **7.0 stable**: The patch should apply cleanly since
`kzalloc_objs`/`kvzalloc_objs` macros exist.
- For **older stable trees** (6.12, 6.6, 6.1, 5.15): Needs trivial
adaptation (use `kvcalloc` instead of `kvzalloc_objs`; or `kvzalloc`
with manual size calculation instead of the macro).
Record: [7.0: clean apply. Older: needs trivial adaptation of alloc
macro]
### Step 6.3: CHECK IF RELATED FIXES ARE ALREADY IN STABLE
No. The syzbot report for 5.15 is still marked as unfixed.
Record: [No existing fix in any stable tree]
---
## PHASE 7: SUBSYSTEM AND MAINTAINER CONTEXT
### Step 7.1: IDENTIFY THE SUBSYSTEM
- **Subsystem:** `drivers/md` — MD (Multiple Devices) RAID subsystem
- **Criticality:** IMPORTANT — RAID0 is a widely-used storage
configuration. Many production systems use MD RAID.
Record: [md/raid0] [Criticality: IMPORTANT — widely used storage
subsystem]
### Step 7.2: ASSESS SUBSYSTEM ACTIVITY
Active subsystem with regular commits from Yu Kuai and others.
Record: [Actively maintained subsystem]
---
## PHASE 8: IMPACT AND RISK ASSESSMENT
### Step 8.1: DETERMINE WHO IS AFFECTED
Any user creating a RAID0 array with a large number of disks (where
`nr_strip_zones * raid_disks` allocation exceeds 4MB). This is plausible
in production environments with many disks.
Record: [Affected: RAID0 users with large disk counts]
### Step 8.2: DETERMINE THE TRIGGER CONDITIONS
- **Trigger:** Creating a RAID0 array (via `md_ioctl`) with enough disks
that the metadata allocation exceeds MAX_PAGE_ORDER.
- **How common?** Syzbot triggered it, meaning it's reachable from
unprivileged-ish ioctl. In production, requires many disks.
- **Unprivileged user?** The ioctl path is reachable from userspace
(requires device access, typically root for md devices).
Record: [Triggered via ioctl with large raid_disks, reachable from
userspace]
### Step 8.3: DETERMINE THE FAILURE MODE SEVERITY
- **Primary symptom:** kernel WARNING (WARN_ON_ONCE) in page allocator —
this taints the kernel and may trigger panic in some configurations
(`panic_on_warn`).
- **Secondary consequence:** The allocation fails with -ENOMEM even
though vmalloc could service it, meaning RAID0 arrays with many disks
simply cannot be created (functional failure).
- **Severity:** MEDIUM-HIGH — WARNING triggers kernel taint, potential
panic_on_warn crash, and prevents legitimate RAID0 creation.
Record: [WARNING + allocation failure → kernel taint, possible panic,
RAID0 creation failure] [Severity: MEDIUM-HIGH]
### Step 8.4: CALCULATE RISK-BENEFIT RATIO
- **BENEFIT:** High — fixes a syzbot-reported real bug affecting
multiple stable trees, preventing WARNINGs and enabling RAID0 with
many disks.
- **RISK:** Very low — 9 lines changed, pure API substitution
(kzalloc→kvzalloc, kfree→kvfree), a well-tested kernel pattern.
Record: [High benefit, very low risk]
---
## PHASE 9: FINAL SYNTHESIS
### Step 9.1: COMPILE THE EVIDENCE
**FOR backporting:**
- Syzbot-reported real bug with reproducer
- Same bug reproduced on linux-5.15.y (confirmed in multiple stable
trees)
- Triggers WARN_ON_ONCE, which can cause panic with `panic_on_warn`
- Prevents creation of RAID0 arrays with many disks (functional failure)
- Very small, surgical fix (9 lines, single file)
- Well-understood pattern (kzalloc→kvzalloc) used thousands of times in
the kernel
- Reviewed by TWO MD subsystem maintainers (Yu Kuai, Li Nan)
- No objections or concerns in review
- Standalone fix with no prerequisites (for 7.0 tree)
**AGAINST backporting:**
- For older stable trees (pre-7.0), the `kzalloc_objs`/`kvzalloc_objs`
macro won't exist — needs trivial adaptation
- The trigger requires a large number of disks (not every RAID0 user
hits this)
### Step 9.2: APPLY THE STABLE RULES CHECKLIST
1. **Obviously correct and tested?** YES — trivial API swap, reviewed by
two maintainers, syzbot-tested
2. **Fixes a real bug?** YES — syzbot reproduced WARNING, confirmed on
multiple kernels
3. **Important issue?** YES — WARNING can trigger panic_on_warn; blocks
RAID0 creation
4. **Small and contained?** YES — 9 lines in one file
5. **No new features or APIs?** CORRECT — pure API swap
6. **Can apply to stable?** YES for 7.0; needs minor adaptation for
older trees
### Step 9.3: CHECK FOR EXCEPTION CATEGORIES
Not an exception category — this is a straightforward bug fix.
### Step 9.4: DECISION
Clear YES. This is a textbook stable backport candidate: syzbot-
reported, small surgical fix, obviously correct, reviewed by
maintainers, fixes a real bug in widely-used code.
---
## Verification
- [Phase 1] Parsed tags: found Reported-by: syzbot, Closes: link to
syzbot, Reviewed-by from Yu Kuai and Li Nan, committed by Yu Kuai (MD
maintainer)
- [Phase 2] Diff analysis: 9 lines changed in `drivers/md/raid0.c`, pure
kzalloc→kvzalloc + kfree→kvfree swap in `create_strip_zones()`
(allocation + abort path) and `raid0_free()` (normal path)
- [Phase 3] git blame: strip_zone/devlist allocations date to Linux
2.6.12 (2005), present in all stable trees
- [Phase 3] git blame: `kzalloc_objs` wrapper is v7.0-only (commit
32a92f8c89326), older trees use `kcalloc`/`kzalloc`
- [Phase 3] Verified `kvzalloc_objs` macro exists in v7.0 tree
(include/linux/slab.h line 1057)
- [Phase 3] Verified `kvzalloc` and `kvfree` exist in v5.15, v6.6 (well-
established APIs)
- [Phase 4] spinics.net mirror: confirmed patch reviewed by Yu Kuai
(2026-03-20) and Li Nan (2026-03-21), applied to md-7.1 on 2026-04-07,
no objections
- [Phase 4] Syzbot upstream report: WARNING in create_strip_zones at
mm/page_alloc.c, fix commit 078d1d8e688d confirmed
- [Phase 4] Syzbot 5.15 report: same WARNING triggered on linux-5.15.y
(commit 91d48252ad4b), confirming bug in old stable trees
- [Phase 5] Call trace verified from syzbot: `create_strip_zones` →
`raid0_run` → `md_run` → `md_ioctl` → ioctl syscall (reachable from
userspace)
- [Phase 6] Bug code confirmed in v5.15, v6.12, v6.14 — all use
kzalloc/kcalloc for strip_zone/devlist
- [Phase 6] For 7.0 tree: patch applies cleanly (kvzalloc_objs
available)
- [Phase 6] For older trees: needs trivial adaptation (kcalloc→kvcalloc
instead of kzalloc_objs→kvzalloc_objs)
- [Phase 7] md/raid0 is IMPORTANT subsystem, actively maintained
- [Phase 8] Failure mode: WARN_ON_ONCE (kernel taint, panic_on_warn) +
ENOMEM preventing RAID0 creation; severity MEDIUM-HIGH
**YES**
drivers/md/raid0.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/md/raid0.c b/drivers/md/raid0.c
index ef0045db409fc..5e38a51e349ad 100644
--- a/drivers/md/raid0.c
+++ b/drivers/md/raid0.c
@@ -143,13 +143,13 @@ static int create_strip_zones(struct mddev *mddev, struct r0conf **private_conf)
}
err = -ENOMEM;
- conf->strip_zone = kzalloc_objs(struct strip_zone, conf->nr_strip_zones);
+ conf->strip_zone = kvzalloc_objs(struct strip_zone, conf->nr_strip_zones);
if (!conf->strip_zone)
goto abort;
- conf->devlist = kzalloc(array3_size(sizeof(struct md_rdev *),
- conf->nr_strip_zones,
- mddev->raid_disks),
- GFP_KERNEL);
+ conf->devlist = kvzalloc(array3_size(sizeof(struct md_rdev *),
+ conf->nr_strip_zones,
+ mddev->raid_disks),
+ GFP_KERNEL);
if (!conf->devlist)
goto abort;
@@ -291,8 +291,8 @@ static int create_strip_zones(struct mddev *mddev, struct r0conf **private_conf)
return 0;
abort:
- kfree(conf->strip_zone);
- kfree(conf->devlist);
+ kvfree(conf->strip_zone);
+ kvfree(conf->devlist);
kfree(conf);
*private_conf = ERR_PTR(err);
return err;
@@ -373,8 +373,8 @@ static void raid0_free(struct mddev *mddev, void *priv)
{
struct r0conf *conf = priv;
- kfree(conf->strip_zone);
- kfree(conf->devlist);
+ kvfree(conf->strip_zone);
+ kvfree(conf->devlist);
kfree(conf);
}
--
2.53.0
^ permalink raw reply related
* [PATCH AUTOSEL 7.0-5.10] md/raid5: skip 2-failure compute when other disk is R5_LOCKED
From: Sasha Levin @ 2026-04-20 13:18 UTC (permalink / raw)
To: patches, stable
Cc: FengWei Shih, Yu Kuai, Yu Kuai, Sasha Levin, song, linux-raid,
linux-kernel
In-Reply-To: <20260420132314.1023554-1-sashal@kernel.org>
From: FengWei Shih <dannyshih@synology.com>
[ Upstream commit 52e4324935be917f8f3267354b3cc06bb8ffcec1 ]
When skip_copy is enabled on a doubly-degraded RAID6, a device that is
being written to will be in R5_LOCKED state with R5_UPTODATE cleared.
If a new read triggers fetch_block() while the write is still in
flight, the 2-failure compute path may select this locked device as a
compute target because it is not R5_UPTODATE.
Because skip_copy makes the device page point directly to the bio page,
reconstructing data into it might be risky. Also, since the compute
marks the device R5_UPTODATE, it triggers WARN_ON in ops_run_io()
which checks that R5_SkipCopy and R5_UPTODATE are not both set.
This can be reproduced by running small-range concurrent read/write on
a doubly-degraded RAID6 with skip_copy enabled, for example:
mdadm -C /dev/md0 -l6 -n6 -R -f /dev/loop[0-3] missing missing
echo 1 > /sys/block/md0/md/skip_copy
fio --filename=/dev/md0 --rw=randrw --bs=4k --numjobs=8 \
--iodepth=32 --size=4M --runtime=30 --time_based --direct=1
Fix by checking R5_LOCKED before proceeding with the compute. The
compute will be retried once the lock is cleared on IO completion.
Signed-off-by: FengWei Shih <dannyshih@synology.com>
Reviewed-by: Yu Kuai <yukuai@fnnas.com>
Link: https://lore.kernel.org/linux-raid/20260319053351.3676794-1-dannyshih@synology.com/
Signed-off-by: Yu Kuai <yukuai3@huawei.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
LLM Generated explanations, may be completely bogus:
Now I have all the information needed for my comprehensive analysis.
---
## PHASE 1: COMMIT MESSAGE FORENSICS
### Step 1.1: Subject Line
- **Subsystem**: md/raid5
- **Action verb**: "skip" — describing a fix that avoids proceeding with
a dangerous compute operation
- **Summary**: Skip 2-failure compute when the other disk is R5_LOCKED
- Record: [md/raid5] [skip/fix] [Prevent 2-failure compute from
selecting a locked device as target, avoiding data corruption and
WARN_ON triggers]
### Step 1.2: Tags
- **Signed-off-by**: FengWei Shih <dannyshih@synology.com> (author)
- **Reviewed-by**: Yu Kuai <yukuai@fnnas.com> — **This is the MD
subsystem co-maintainer** (confirmed in MAINTAINERS)
- **Link**: https://lore.kernel.org/linux-
raid/20260319053351.3676794-1-dannyshih@synology.com/
- **Signed-off-by**: Yu Kuai <yukuai3@huawei.com> — Applied by the
subsystem maintainer
- No Fixes: tag (expected for AUTOSEL candidates)
- No Reported-by: tag (but author provides precise reproduction steps)
- Record: Reviewed and applied by subsystem co-maintainer. Author
provides concrete repro.
### Step 1.3: Commit Body Analysis
- **Bug described**: On a doubly-degraded RAID6 with `skip_copy`
enabled, a concurrent read triggers `fetch_block()` during an in-
flight write. The 2-failure compute path selects the locked (being-
written-to) device as a compute target because it's not R5_UPTODATE.
- **Symptom**: WARN_ON in `ops_run_io()` at line 1271, which checks that
R5_SkipCopy and R5_UPTODATE are not both set. Additionally,
reconstructing data into the device page is risky because with
`skip_copy`, the device page points directly to the bio page —
corrupting user data.
- **Reproduction**: Concrete and reproducible with mdadm + fio commands
provided.
- **Root cause**: The 2-failure compute path in `fetch_block()` finds a
non-R5_UPTODATE disk and selects it as the "other" compute target
without checking if it's R5_LOCKED (i.e., has an I/O in flight).
- Record: Race between concurrent read and write on doubly-degraded
RAID6 with skip_copy. Triggers WARN_ON and potential data corruption.
Concrete reproduction steps provided.
### Step 1.4: Hidden Bug Fix Detection
This is NOT a hidden fix — it's an explicit, well-described bug fix. The
commit clearly explains the bug mechanism, failure mode, and how to
reproduce it.
## PHASE 2: DIFF ANALYSIS
### Step 2.1: Inventory
- **Files changed**: 1 (drivers/md/raid5.c)
- **Lines added**: 2
- **Function modified**: `fetch_block()`
- **Scope**: Single-file, single-function, 2-line surgical fix
- Record: Minimal change — 2 lines added in fetch_block() in raid5.c
### Step 2.2: Code Flow Change
**Before**: The 2-failure compute path finds the `other` disk that is
not R5_UPTODATE, then immediately proceeds with the compute operation
(setting R5_Wantcompute on both target disks).
**After**: After finding the `other` disk, the code first checks if it
has R5_LOCKED set. If so, it returns 0 (skip the compute), allowing the
compute to be retried after the lock clears on I/O completion.
The change is in the 2-failure compute branch of `fetch_block()`:
```3918:3919:drivers/md/raid5.c
BUG_ON(other < 0);
// NEW: if (test_bit(R5_LOCKED, &sh->dev[other].flags)) return 0;
pr_debug("Computing stripe %llu blocks %d,%d\n",
```
### Step 2.3: Bug Mechanism
This is a **race condition** combined with **potential data
corruption**:
1. Write path sets R5_SkipCopy on a device, pointing dev->page to the
bio page, and clears R5_UPTODATE (line 1961-1962).
2. The device is R5_LOCKED (I/O in flight).
3. A concurrent read triggers `fetch_block()` → enters the 2-failure
compute path.
4. The loop finds this device as `other` (because it's !R5_UPTODATE).
5. Compute is initiated, writing reconstructed data into `other->page`,
which is actually the user's bio page.
6. The compute then marks the device R5_UPTODATE via
`mark_target_uptodate()` (line 1506).
7. This triggers WARN_ON at line 1270-1271 because both R5_SkipCopy and
R5_UPTODATE are now set.
8. Data could be corrupted because the compute overwrites the bio page.
Record: Race condition causing WARN_ON trigger + potential data
corruption on RAID6 with skip_copy enabled.
### Step 2.4: Fix Quality
- **Obviously correct**: Yes — a device being written to (R5_LOCKED)
should not be selected as a compute target. The fix adds a simple
guard check.
- **Minimal**: 2 lines, surgical.
- **Regression risk**: Minimal. Returning 0 simply defers the compute
until the lock clears — this is the normal retry mechanism already
used elsewhere in the stripe handling.
- **No red flags**: No API changes, no lock changes, no architectural
impact.
## PHASE 3: GIT HISTORY INVESTIGATION
### Step 3.1: Blame
- The 2-failure compute code in `fetch_block()` was introduced in commit
`5599becca4bee7` (2009-08-29, "md/raid6: asynchronous
handle_stripe_fill6"), which is from the v2.6.32 era.
- The `R5_SkipCopy` mechanism was introduced in commit `584acdd49cd24`
(2014-12-15, "md/raid5: activate raid6 rmw feature"), which landed in
v4.1.
- The bug exists since v4.1 when skip_copy was introduced — this created
the interaction where a device could be !R5_UPTODATE but R5_LOCKED
with page pointing to a bio page.
Record: Buggy interaction exists since ~v4.1 (2015). Present in all
active stable trees.
### Step 3.2: Fixes tag
No Fixes: tag present (expected for AUTOSEL). Based on analysis, the
proper Fixes: would point to `584acdd49cd24` where the skip_copy feature
introduced the problematic interaction.
### Step 3.3: File history
Recent changes to raid5.c show active development with fixes like IO
hang fixes, null-pointer deref fixes, etc. This is actively maintained
code.
### Step 3.4: Author
- FengWei Shih works at Synology (a major NAS/storage vendor that
heavily uses RAID6).
- Yu Kuai (reviewer and committer) is the MD subsystem co-maintainer per
MAINTAINERS.
### Step 3.5: Dependencies
- No dependencies. The fix is a standalone 2-line addition checking an
existing flag.
- Verified the code is identical in v5.15, v6.1, and v6.6 stable trees.
## PHASE 4: MAILING LIST AND EXTERNAL RESEARCH
### Step 4.1-4.5
Lore was not accessible due to Anubis anti-bot protection. However:
- The Link: tag in the commit points to the original submission on
linux-raid.
- The patch was reviewed by Yu Kuai (subsystem co-maintainer) and
applied by him.
- The author works at Synology, suggesting they encountered this in
production NAS workloads.
Record: Could not fetch lore discussion. But reviewer is subsystem co-
maintainer, author is from major storage vendor.
## PHASE 5: CODE SEMANTIC ANALYSIS
### Step 5.1: Functions Modified
- `fetch_block()` — the sole function modified.
### Step 5.2: Callers
`fetch_block()` is called from `handle_stripe_fill()` (line 3973) in a
loop over all disks. `handle_stripe_fill()` is called from
`handle_stripe()`, which is the main stripe processing function in
RAID5/6 — called for every I/O operation.
### Step 5.3-5.4: Impact Surface
The call chain is: I/O request → handle_stripe() → handle_stripe_fill()
→ fetch_block(). This is a hot path for all RAID5/6 read operations
during degraded mode.
### Step 5.5: Similar Patterns
The single-failure compute path (the `if` branch above the modified
code, lines 3883-3905) doesn't have this problem because it only
triggers when `s->uptodate == disks - 1`, meaning only one disk is not
up-to-date, and it computes the requesting disk itself. The 2-failure
path is uniquely vulnerable because it selects a *second* disk as
compute target.
## PHASE 6: STABLE TREE ANALYSIS
### Step 6.1: Code Existence
Verified that the exact same 2-failure compute code block exists in
v5.15, v6.1, and v6.6 stable trees. The code is character-for-character
identical.
### Step 6.2: Backport Complications
**None.** The patch will apply cleanly to all stable trees. The
surrounding context lines match exactly.
### Step 6.3: No related fixes already in stable.
## PHASE 7: SUBSYSTEM CONTEXT
### Step 7.1: Subsystem
- **Subsystem**: MD/RAID (drivers/md/) — Software RAID
- **Criticality**: IMPORTANT — RAID6 is widely used in NAS, enterprise
storage, and data center systems. Data integrity issues in RAID are
critical.
### Step 7.2: Activity
Active subsystem with regular fixes and enhancements. Maintained by Song
Liu and Yu Kuai.
## PHASE 8: IMPACT AND RISK ASSESSMENT
### Step 8.1: Affected Users
All users running doubly-degraded RAID6 arrays with skip_copy enabled
during concurrent read/write. This is a realistic production scenario —
a RAID6 array losing two disks (which RAID6 is designed to survive)
while continuing to serve I/O.
### Step 8.2: Trigger Conditions
- Doubly-degraded RAID6 (two disks failed or missing)
- `skip_copy` enabled (configurable via sysfs, default off but commonly
enabled for performance)
- Concurrent read and write to overlapping stripe regions
- Reproducible with the fio command in the commit message
### Step 8.3: Failure Mode Severity
1. **WARN_ON trigger** in `ops_run_io()` — MEDIUM (kernel warning,
potential crash if panic_on_warn)
2. **Data corruption** — CRITICAL: The compute writes reconstructed data
into a bio page that is owned by the user write operation. This can
corrupt user data silently.
3. The commit says "reconstructing data into it might be risky" —
understatement given that the bio page belongs to user space.
**Severity: CRITICAL** (potential data corruption on RAID storage)
### Step 8.4: Risk-Benefit Ratio
- **BENEFIT**: Very high — prevents potential data corruption and
WARN_ON on RAID6 arrays
- **RISK**: Very low — 2-line fix that adds a simple guard check,
returns 0 to defer (existing retry mechanism), no side effects
- **Ratio**: Excellent — minimal risk, high benefit
## PHASE 9: FINAL SYNTHESIS
### Step 9.1: Evidence Summary
**FOR backporting:**
- Fixes a real, reproducible race condition on doubly-degraded RAID6
with skip_copy
- Can lead to data corruption (compute writes into bio page)
- Triggers WARN_ON in ops_run_io() (system stability concern)
- 2-line surgical fix, obviously correct
- Reviewed and applied by subsystem co-maintainer (Yu Kuai)
- Author from Synology (major NAS vendor, real-world scenario)
- Concrete reproduction steps provided
- Code identical in all stable trees (v5.15, v6.1, v6.6) — clean apply
- Bug present since v4.1 (affects all active stable trees)
- No dependencies on other patches
**AGAINST backporting:**
- No explicit Fixes: tag (expected for AUTOSEL)
- Requires specific configuration (doubly-degraded + skip_copy +
concurrent I/O)
- No syzbot report (but has clear reproduction path)
### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **YES** — reviewed by maintainer,
concrete repro
2. Fixes a real bug? **YES** — WARN_ON trigger + potential data
corruption
3. Important issue? **YES** — data corruption on RAID storage is
critical
4. Small and contained? **YES** — 2 lines in one function
5. No new features? **YES** — just a guard check
6. Applies to stable? **YES** — verified identical code in all stable
trees
### Step 9.3: Exception Categories
N/A — this is a standard bug fix, no exception needed.
### Step 9.4: Decision
Clear YES. This is a 2-line fix that prevents potential data corruption
and WARN_ON triggers on doubly-degraded RAID6 arrays. It was reviewed
and merged by the subsystem co-maintainer, is obviously correct, and
applies cleanly to all stable trees.
## Verification
- [Phase 1] Parsed tags: Reviewed-by Yu Kuai (MD co-maintainer), Link to
linux-raid
- [Phase 2] Diff analysis: 2 lines added in fetch_block(), adds
R5_LOCKED check before 2-failure compute
- [Phase 3] git blame: buggy interaction since v4.1 (commit
584acdd49cd24, 2014); 2-failure compute since v2.6.32 (commit
5599becca4bee7, 2009)
- [Phase 3] Verified identical code exists in v5.15 (line 3882), v6.1
(line 3984), v6.6 (line 3991)
- [Phase 3] Yu Kuai confirmed as MD subsystem co-maintainer in
MAINTAINERS file
- [Phase 4] Lore inaccessible (Anubis protection). UNVERIFIED: full
mailing list discussion. However, Reviewed-by from maintainer
mitigates this.
- [Phase 5] fetch_block() called from handle_stripe_fill() →
handle_stripe(), hot path for RAID I/O
- [Phase 5] Traced SkipCopy mechanism: set at line 1961 during write
prep, clears R5_UPTODATE, points dev->page to bio page
- [Phase 5] Traced compute completion: mark_target_uptodate() at line
1506 sets R5_UPTODATE, triggering WARN_ON at line 1270-1271
- [Phase 6] Code exists unchanged in all active stable trees (v5.15,
v6.1, v6.6) — patch applies cleanly
- [Phase 7] MD/RAID subsystem, IMPORTANT criticality, actively
maintained
- [Phase 8] Failure mode: data corruption (CRITICAL) + WARN_ON trigger
(MEDIUM); trigger requires doubly-degraded RAID6 + skip_copy +
concurrent I/O
**YES**
drivers/md/raid5.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index a8e8d431071ba..6e9405a89bc4a 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -3916,6 +3916,8 @@ static int fetch_block(struct stripe_head *sh, struct stripe_head_state *s,
break;
}
BUG_ON(other < 0);
+ if (test_bit(R5_LOCKED, &sh->dev[other].flags))
+ return 0;
pr_debug("Computing stripe %llu blocks %d,%d\n",
(unsigned long long)sh->sector,
disk_idx, other);
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v2 1/5] md/md-bitmap: call md_bitmap_create,destroy in location_store
From: Su Yue @ 2026-04-21 1:26 UTC (permalink / raw)
To: Xiao Ni; +Cc: Su Yue, linux-raid, song, linan122, yukuai, heming.zhao
In-Reply-To: <CALTww28=dKKbx+jrED_e3dzQYRtC=Vh9qk04JPy=rjpE2OA7ww@mail.gmail.com>
On Mon 20 Apr 2026 at 13:21, Xiao Ni <xni@redhat.com> wrote:
> On Thu, Apr 16, 2026 at 10:09 PM Su Yue <l@damenly.org> wrote:
>>
>> On Wed 15 Apr 2026 at 18:34, Xiao Ni <xni@redhat.com> wrote:
>>
>> > On Tue, Apr 7, 2026 at 6:26 PM Su Yue <glass.su@suse.com>
>> > wrote:
>> >>
>> >> If bitmap/location is present, mdadm will call
>> >> update_array_info()
>> >> while growing bitmap from none to internal via
>> >> location_store().
>> >> md_bitmap_create() is needed to set mddev->bitmap_ops
>> >> otherwise
>> >> mddev->bitmap_ops->get_stats() in update_array_info() will
>> >> trigger
>> >> kernel NULL pointer dereference.
>> >
>> >
>> > Hi Su Yue
>> >
>> > How can bitmap/location be present when bitmap is none? Could
>> > you
>> > provide the test commands that reproduce this problem?
>> >
>> Sorry for the misleading commit message. It can only be
>> reproduced
>> patch 3 is appiled.
>> I adjusted the sequence of this patch for easy review because
>> md_bitmap_create,destroy
>> are touched in patch1,2 and 3. Also if put the patch after 3rd
>> patch,
>> it will break ability to bisect.
>>
>> # mdadm --create --assume-clean /dev/md0 -f --bitmap=internal
>> --raid-devices=2 --level=mirror --metadata=1.2 /dev/vdc
>> /dev/vdd
>> # mdadm --grow /dev/md0 --bitmap=none
>> # mdadm --grow /dev/md0 --bitmap=internal # step 3
>> # mdadm --grow /dev/md0 --bitmap=none # step 4
>> [1] 2325 killed mdadm --grow /dev/md0 --bitmap=none
>>
>> When step 3 is called,
>> md_bitmap_destroy() is called in update_array_info() to set
>> NULL
>> mddev->bitmap_ops
>> then in step 4 kernel Oops is triggered.
>>
>>
>> I am willing to amend commit message or move it after patch 3
>> if
>> you would like.
>
> Hi Su
>
> Thanks for the detail explanation. After reading patch3, I
> totoally
> understand. The sequence is good to me. And yes, it's better to
> explain that this is needed after patch3.
>
Sure. I will do it in next version.
--
Su
>
> Best Regards
> Xiao
>>
>> --
>> Su
>>
>> >
>> > mdadm -CR /dev/md0 -l1 -n2 /dev/loop0 /dev/loop1
>> > --bitmap=none
>> > (There
>> > is not bitmap/location, because bitmap directory is not
>> > created)
>> > mdadm /dev/md0 --grow --bitmap=internal
>> > Grow.c md_set_array_info runs
>> > 451 array.state |= (1 << MD_SB_BITMAP_PRESENT);
>> > 452 rv = md_set_array_info(fd, &array);
>> > In kernel space, it runs
>> > 8125 rv = md_bitmap_create(mddev);
>> > 8126 if (!rv)
>> > 8127 rv = mddev->bitmap_ops->load(mddev);
>> >
>> > Best Regards
>> > Xiao
>> >
>> >>
>> >> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of
>> >> bitmap_ops until creating bitmap")
>> >> Signed-off-by: Su Yue <glass.su@suse.com>
>> >> ---
>> >> drivers/md/md-bitmap.c | 11 ++++++++---
>> >> drivers/md/md.c | 4 ++--
>> >> drivers/md/md.h | 2 ++
>> >> 3 files changed, 12 insertions(+), 5 deletions(-)
>> >>
>> >> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> >> index 83378c033c72..2f24aae05552 100644
>> >> --- a/drivers/md/md-bitmap.c
>> >> +++ b/drivers/md/md-bitmap.c
>> >> @@ -2618,7 +2618,7 @@ location_store(struct mddev *mddev,
>> >> const
>> >> char *buf, size_t len)
>> >> goto out;
>> >> }
>> >>
>> >> - bitmap_destroy(mddev);
>> >> + md_bitmap_destroy(mddev);
>> >> mddev->bitmap_info.offset = 0;
>> >> if (mddev->bitmap_info.file) {
>> >> struct file *f =
>> >> mddev->bitmap_info.file;
>> >> @@ -2653,15 +2653,20 @@ location_store(struct mddev *mddev,
>> >> const char *buf, size_t len)
>> >> goto out;
>> >> }
>> >>
>> >> + /*
>> >> + * lockless bitmap shoudle have set
>> >> bitmap_id
>> >> + * using bitmap_type, so always
>> >> ID_BITMAP.
>> >> + */
>> >> + mddev->bitmap_id = ID_BITMAP;
>> >> mddev->bitmap_info.offset = offset;
>> >> - rv = bitmap_create(mddev);
>> >> + rv = md_bitmap_create(mddev);
>> >> if (rv)
>> >> goto out;
>> >>
>> >> rv = bitmap_load(mddev);
>> >> if (rv) {
>> >> mddev->bitmap_info.offset =
>> >> 0;
>> >> - bitmap_destroy(mddev);
>> >> + md_bitmap_destroy(mddev);
>> >> goto out;
>> >> }
>> >> }
>> >> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> >> index 3ce6f9e9d38e..8b1ecc370ad6 100644
>> >> --- a/drivers/md/md.c
>> >> +++ b/drivers/md/md.c
>> >> @@ -6447,7 +6447,7 @@ static void md_safemode_timeout(struct
>> >> timer_list *t)
>> >>
>> >> static int start_dirty_degraded;
>> >>
>> >> -static int md_bitmap_create(struct mddev *mddev)
>> >> +int md_bitmap_create(struct mddev *mddev)
>> >> {
>> >> if (mddev->bitmap_id == ID_BITMAP_NONE)
>> >> return -EINVAL;
>> >> @@ -6458,7 +6458,7 @@ static int md_bitmap_create(struct
>> >> mddev
>> >> *mddev)
>> >> return mddev->bitmap_ops->create(mddev);
>> >> }
>> >>
>> >> -static void md_bitmap_destroy(struct mddev *mddev)
>> >> +void md_bitmap_destroy(struct mddev *mddev)
>> >> {
>> >> if (!md_bitmap_registered(mddev))
>> >> return;
>> >> diff --git a/drivers/md/md.h b/drivers/md/md.h
>> >> index ac84289664cd..ed69244af00d 100644
>> >> --- a/drivers/md/md.h
>> >> +++ b/drivers/md/md.h
>> >> @@ -895,6 +895,8 @@ static inline void safe_put_page(struct
>> >> page *p)
>> >>
>> >> int register_md_submodule(struct md_submodule_head *msh);
>> >> void unregister_md_submodule(struct md_submodule_head
>> >> *msh);
>> >> +int md_bitmap_create(struct mddev *mddev);
>> >> +void md_bitmap_destroy(struct mddev *mddev);
>> >>
>> >> extern struct md_thread *md_register_thread(
>> >> void (*run)(struct md_thread *thread),
>> >> --
>> >> 2.53.0
>> >>
>>
^ permalink raw reply
* Re: [PATCH v2 3/5] md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset
From: Su Yue @ 2026-04-21 2:29 UTC (permalink / raw)
To: Xiao Ni; +Cc: Su Yue, linux-raid, song, linan122, yukuai, heming.zhao
In-Reply-To: <CALTww28XwfDKbcnrka_YMtgTtArjAXEGhbUafVVGEWuAjV2o3w@mail.gmail.com>
On Mon 20 Apr 2026 at 15:05, Xiao Ni <xni@redhat.com> wrote:
> On Tue, Apr 7, 2026 at 6:27 PM Su Yue <glass.su@suse.com> wrote:
>>
>> Before commit fb8cc3b0d9db ("md/md-bitmap: delay registration
>> of bitmap_ops until creating bitmap")
>> if CONFIG_MD_BITMAP is enabled, both bitmap none, internal and
>> clustered have
>> the sysfs file bitmap/location.
>>
>> After the commit, if bitmap is none, bitmap/location doesn't
>> exist anymore.
>> It breaks 'grow' behavior of a md array of madam with
>> MD_FEATURE_BITMAP_OFFSET.
>> Take level=mirror and metadata=1.2 as an example:
>>
>> $ mdadm --create /dev/md0 -f --bitmap=none --raid-devices=2
>> --level=mirror \
>> --metadata=1.2 /dev/vdd /dev/vde
>> $ mdadm --grow /dev/md0 --bitmap=internal
>> $ cat /sys/block/md0/md/bitmap/location
>> Before:+8
>> After: +2
>>
>> While growing bitmap from none to internal, clustered and
>> llbitmap,
>> mdadm/Grow.c:Grow_addbitmap() tries to detect bitmap/location
>> first.
>> 1)If bitmap/location exists, it sets bitmap/location after
>> getinfo_super().
>> 2)If bitmap/location doesn't exist, mdadm just calls
>> md_set_array_info() then
>> mddev->bitmap_info.default_offset will be used.
>> Situation can be worse if growing none to clustered, bitmap
>> offset of the node
>> calling `madm --grow` will be changed but the other node are
>> reading bitmap sb from
>> the old location.
>>
>> Here introducing a dummy bitmap_operations for ID_BITMAP_NONE
>> to restore sysfs
>> file bitmap/location for ID_BITMAP_NONE and ID_BITMAP.
>> location_store() now calls md_bitmap_(create|destroy) with
>> false sysfs parameter then
>> (add,remove)_internal_bitmap() will be called to manage sysfs
>> entries.
>>
>> Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of
>> bitmap_ops until creating bitmap")
>> Suggested-by: Yu Kuai <yukuai@fnnas.com>
>> Signed-off-by: Su Yue <glass.su@suse.com>
>> ---
>> drivers/md/md-bitmap.c | 116
>> ++++++++++++++++++++++++++++++++++++---
>> drivers/md/md-bitmap.h | 3 +
>> drivers/md/md-llbitmap.c | 12 ++++
>> drivers/md/md.c | 16 +++---
>> 4 files changed, 130 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
>> index ac06c9647bf0..a8a176428c61 100644
>> --- a/drivers/md/md-bitmap.c
>> +++ b/drivers/md/md-bitmap.c
>> @@ -240,6 +240,11 @@ static bool bitmap_enabled(void *data,
>> bool flush)
>> bitmap->storage.filemap != NULL;
>> }
>>
>> +static bool dummy_bitmap_enabled(void *data, bool flush)
>> +{
>> + return false;
>> +}
>> +
>> /*
>> * check a page and, if necessary, allocate it (or hijack it
>> if the alloc fails)
>> *
>> @@ -2201,6 +2206,11 @@ static int bitmap_create(struct mddev
>> *mddev)
>> return 0;
>> }
>>
>> +static int dummy_bitmap_create(struct mddev *mddev)
>> +{
>> + return 0;
>> +}
>> +
>> static int bitmap_load(struct mddev *mddev)
>> {
>> int err = 0;
>> @@ -2594,6 +2604,9 @@ location_show(struct mddev *mddev, char
>> *page)
>> return len;
>> }
>>
>> +static int create_internal_group(struct mddev *mddev);
>> +static void remove_internal_group(struct mddev *mddev);
>> +
>> static ssize_t
>> location_store(struct mddev *mddev, const char *buf, size_t
>> len)
>> {
>> @@ -2618,7 +2631,8 @@ location_store(struct mddev *mddev, const
>> char *buf, size_t len)
>> goto out;
>> }
>>
>> - md_bitmap_destroy(mddev, true);
>> + md_bitmap_destroy(mddev, false);
>> + remove_internal_group(mddev);
>> mddev->bitmap_info.offset = 0;
>> if (mddev->bitmap_info.file) {
>> struct file *f =
>> mddev->bitmap_info.file;
>> @@ -2659,14 +2673,22 @@ location_store(struct mddev *mddev,
>> const char *buf, size_t len)
>> */
>> mddev->bitmap_id = ID_BITMAP;
>> mddev->bitmap_info.offset = offset;
>> - rv = md_bitmap_create(mddev, true);
>> + rv = md_bitmap_create(mddev, false);
>> if (rv)
>> goto out;
>>
>> + rv = create_internal_group(mddev);
>> + if (rv) {
>> + mddev->bitmap_info.offset = 0;
>> + bitmap_destroy(mddev);
>> + goto out;
>> + }
>> +
>> rv = bitmap_load(mddev);
>> if (rv) {
>> + remove_internal_group(mddev);
>> mddev->bitmap_info.offset = 0;
>> - md_bitmap_destroy(mddev, true);
>> + md_bitmap_destroy(mddev,
>> false);
>> goto out;
>> }
>> }
>> @@ -2960,8 +2982,7 @@ static struct md_sysfs_entry
>> max_backlog_used =
>> __ATTR(max_backlog_used, S_IRUGO | S_IWUSR,
>> behind_writes_used_show, behind_writes_used_reset);
>>
>> -static struct attribute *md_bitmap_attrs[] = {
>> - &bitmap_location.attr,
>> +static struct attribute *internal_bitmap_attrs[] = {
>> &bitmap_space.attr,
>> &bitmap_timeout.attr,
>> &bitmap_backlog.attr,
>> @@ -2972,11 +2993,57 @@ static struct attribute
>> *md_bitmap_attrs[] = {
>> NULL
>> };
>>
>> -static struct attribute_group md_bitmap_group = {
>> +static struct attribute_group internal_bitmap_group = {
>> .name = "bitmap",
>> - .attrs = md_bitmap_attrs,
>> + .attrs = internal_bitmap_attrs,
>> };
>>
>> +/* Only necessary attrs for compatibility */
>> +static struct attribute *common_bitmap_attrs[] = {
>> + &bitmap_location.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group common_bitmap_group = {
>> + .name = "bitmap",
>> + .attrs = common_bitmap_attrs,
>> +};
>> +
>> +static int create_internal_group(struct mddev *mddev)
>> +{
>> + /*
>> + * md_bitmap_group and md_bitmap_common_group are using
>> same name
>> + * 'bitmap'.
>> + */
>> + return sysfs_merge_group(&mddev->kobj,
>> &internal_bitmap_group);
>> +}
>> +
>> +static void remove_internal_group(struct mddev *mddev)
>> +{
>> + sysfs_unmerge_group(&mddev->kobj,
>> &internal_bitmap_group);
>> +}
>> +
>> +static int bitmap_register_groups(struct mddev *mddev)
>> +{
>> + int ret;
>> +
>> + ret = sysfs_create_group(&mddev->kobj,
>> &common_bitmap_group);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + ret = sysfs_merge_group(&mddev->kobj,
>> &internal_bitmap_group);
>> + if (ret)
>> + sysfs_remove_group(&mddev->kobj,
>> &common_bitmap_group);
>> +
>> + return ret;
>> +}
>> +
>> +static void bitmap_unregister_groups(struct mddev *mddev)
>> +{
>> + sysfs_unmerge_group(&mddev->kobj,
>> &internal_bitmap_group);
>> +}
>
> Hi Su
>
> bitmap_unregister_groups should also remove common_bitmap_group,
> right?
>
As you pasted before, mdadm:Grow.c:
int Grow_addbitmap(char *devname, int fd, struct context *c,
struct shape *s)
{
...
if (array.state & (1 << MD_SB_BITMAP_PRESENT)) {
if (s->btype == BitmapNone) {
array.state &= ~(1 << MD_SB_BITMAP_PRESENT);
if (md_set_array_info(fd, &array) != 0) {
if (array.state & (1 << MD_SB_CLUSTERED))
pr_err("failed to remove clustered
bitmap.\n");
else
pr_err("failed to remove internal bitmap.\n");
return 1;
}
return 0;
}
pr_err("bitmap already present on %s\n", devname);
return 1;
}
...
}
In case of growing from internal to none,
bitmap_unregister_groups() will
be called, if common_bitmap_group is removed, bitmap/location
won't exist.
update_array_info() is a common function used by many call paths.
Also
llbitmap is involved here. I don't want to make situation and code
more
complicated like adding more codes in update_array_info().
>> +
>> static struct bitmap_operations bitmap_ops = {
>
> You already changed md_bitmap_group to internal_bitmap_group.
> It's
> better to change bitmap_ops to internal_bitmap_ops?
>
>> .head = {
>> .type = MD_BITMAP,
>> @@ -3018,21 +3085,52 @@ static struct bitmap_operations
>> bitmap_ops = {
>> .set_pages = bitmap_set_pages,
>> .free = md_bitmap_free,
>>
>> - .group = &md_bitmap_group,
>> + .register_groups = bitmap_register_groups,
>> + .unregister_groups = bitmap_unregister_groups,
>> +};
>> +
>> +static int none_bitmap_register_groups(struct mddev *mddev)
>> +{
>> + return sysfs_create_group(&mddev->kobj,
>> &common_bitmap_group);
>> +}
>> +
>> +static struct bitmap_operations none_bitmap_ops = {
>> + .head = {
>> + .type = MD_BITMAP,
>> + .id = ID_BITMAP_NONE,
>> + .name = "none",
>> + },
>> +
>> + .enabled = dummy_bitmap_enabled,
>> + .create = dummy_bitmap_create,
>> + .destroy = bitmap_destroy,
>> + .load = bitmap_load,
>> + .get_stats = bitmap_get_stats,
>> + .free = md_bitmap_free,
>> +
>> + .register_groups = none_bitmap_register_groups,
>> + .unregister_groups = NULL,
>
> How does bitmap/location can be deleted if array is created with
> no
> bitmap? Can the `mdadm --stop` command get stuck?
>
No. It wont get stuck.
I guess here your concern is the timing of removing
common_bitmap_group.
The life cycle is same as before fb8cc3b0d9db. At the time
llbitmap is
not introduced, because there is no need to switch bitmap ops, so
all
attrs of bitmap are removed in kobject_put() in
mddev_delayed_delete()
queued by mddev_put(). This is now where common_bitmap_group is
being removed.
--
Su
> Best Regards
> Xiao
>
>> };
>>
>> int md_bitmap_init(void)
>> {
>> + int ret;
>> +
>> md_bitmap_wq = alloc_workqueue("md_bitmap",
>> WQ_MEM_RECLAIM | WQ_UNBOUND,
>> 0);
>> if (!md_bitmap_wq)
>> return -ENOMEM;
>>
>> - return register_md_submodule(&bitmap_ops.head);
>> + ret = register_md_submodule(&bitmap_ops.head);
>> + if (ret)
>> + return ret;
>> +
>> + return register_md_submodule(&none_bitmap_ops.head);
>> }
>>
>> void md_bitmap_exit(void)
>> {
>> destroy_workqueue(md_bitmap_wq);
>> unregister_md_submodule(&bitmap_ops.head);
>> + unregister_md_submodule(&none_bitmap_ops.head);
>> }
>> diff --git a/drivers/md/md-bitmap.h b/drivers/md/md-bitmap.h
>> index b42a28fa83a0..10bc6854798c 100644
>> --- a/drivers/md/md-bitmap.h
>> +++ b/drivers/md/md-bitmap.h
>> @@ -125,6 +125,9 @@ struct bitmap_operations {
>> void (*set_pages)(void *data, unsigned long pages);
>> void (*free)(void *data);
>>
>> + int (*register_groups)(struct mddev *mddev);
>> + void (*unregister_groups)(struct mddev *mddev);
>> +
>> struct attribute_group *group;
>> };
>>
>> diff --git a/drivers/md/md-llbitmap.c
>> b/drivers/md/md-llbitmap.c
>> index bf398d7476b3..9b3ea4f1d268 100644
>> --- a/drivers/md/md-llbitmap.c
>> +++ b/drivers/md/md-llbitmap.c
>> @@ -1561,6 +1561,16 @@ static struct attribute_group
>> md_llbitmap_group = {
>> .attrs = md_llbitmap_attrs,
>> };
>>
>> +static int llbitmap_register_groups(struct mddev *mddev)
>> +{
>> + return sysfs_create_group(&mddev->kobj,
>> &md_llbitmap_group);
>> +}
>> +
>> +static void llbitmap_unregister_groups(struct mddev *mddev)
>> +{
>> + sysfs_remove_group(&mddev->kobj, &md_llbitmap_group);
>> +}
>> +
>> static struct bitmap_operations llbitmap_ops = {
>> .head = {
>> .type = MD_BITMAP,
>> @@ -1597,6 +1607,8 @@ static struct bitmap_operations
>> llbitmap_ops = {
>> .dirty_bits = llbitmap_dirty_bits,
>> .write_all = llbitmap_write_all,
>>
>> + .register_groups = llbitmap_register_groups,
>> + .unregister_groups = llbitmap_unregister_groups,
>> .group = &md_llbitmap_group,
>> };
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index d3c8f77b4fe3..55a95b227b83 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -683,8 +683,7 @@ static bool mddev_set_bitmap_ops(struct
>> mddev *mddev, bool create_sysfs)
>> struct bitmap_operations *old = mddev->bitmap_ops;
>> struct md_submodule_head *head;
>>
>> - if (mddev->bitmap_id == ID_BITMAP_NONE ||
>> - (old && old->head.id == mddev->bitmap_id))
>> + if (old && old->head.id == mddev->bitmap_id)
>> return true;
>>
>> xa_lock(&md_submodule);
>> @@ -703,8 +702,8 @@ static bool mddev_set_bitmap_ops(struct
>> mddev *mddev, bool create_sysfs)
>> mddev->bitmap_ops = (void *)head;
>> xa_unlock(&md_submodule);
>>
>> - if (create_sysfs && !mddev_is_dm(mddev) &&
>> mddev->bitmap_ops->group) {
>> - if (sysfs_create_group(&mddev->kobj,
>> mddev->bitmap_ops->group))
>> + if (create_sysfs && !mddev_is_dm(mddev) &&
>> mddev->bitmap_ops->register_groups) {
>> + if (mddev->bitmap_ops->register_groups(mddev))
>> pr_warn("md: cannot register extra
>> bitmap attributes for %s\n",
>> mdname(mddev));
>> else
>> @@ -724,8 +723,8 @@ static bool mddev_set_bitmap_ops(struct
>> mddev *mddev, bool create_sysfs)
>> static void mddev_clear_bitmap_ops(struct mddev *mddev, bool
>> remove_sysfs)
>> {
>> if (remove_sysfs && !mddev_is_dm(mddev) &&
>> mddev->bitmap_ops &&
>> - mddev->bitmap_ops->group)
>> - sysfs_remove_group(&mddev->kobj,
>> mddev->bitmap_ops->group);
>> + mddev->bitmap_ops->unregister_groups)
>> + mddev->bitmap_ops->unregister_groups(mddev);
>>
>> mddev->bitmap_ops = NULL;
>> }
>> @@ -6610,8 +6609,9 @@ int md_run(struct mddev *mddev)
>> (unsigned long long)pers->size(mddev,
>> 0, 0) / 2);
>> err = -EINVAL;
>> }
>> - if (err == 0 && pers->sync_request &&
>> - (mddev->bitmap_info.file ||
>> mddev->bitmap_info.offset)) {
>> + if (err == 0 && pers->sync_request) {
>> + if (mddev->bitmap_info.offset == 0)
>> + mddev->bitmap_id = ID_BITMAP_NONE;
>> err = md_bitmap_create(mddev, true);
>> if (err)
>> pr_warn("%s: failed to create bitmap
>> (%d)\n",
>> --
>> 2.53.0
>>
^ permalink raw reply
* Re: [PATCH v2 0/5] md: bitmap grow fixes
From: Yu Kuai @ 2026-04-21 5:15 UTC (permalink / raw)
To: Su Yue, linux-raid
Cc: song, xni, linan122@huawei.com >> Li Nan, yukuai,
Heming Zhao
In-Reply-To: <20260407102625.5686-1-glass.su@suse.com>
Hi,
在 2026/4/7 18:26, Su Yue 写道:
> Hi, This v2 series is to fixes bugs exposed by mdadm/clustermd-autotest.
> The bugs are not cluster md only but also bitmap related.
>
> The series based on v7.0-rc7 passes tests with mdadm v4.6 without regression.
>
> v2:
> Add a dummy bitmap operations per Kuai's suggestion.
>
> To Yu Kuai:
> A NULL group can't used for internal_bitmap_group since
> the entries from internal_bitmap_attrs should be under bitmap.
> So instead of sysfs_create_groups(), sysfs_create_group() and sysfs_merge_group()
> are still needed /sigh.
Thanks for the set and sorry for the delay.
TBO, this set is still a bit complex than expected. It's true sysfs_create_groups()
can't be used for the groups with same name. However, there is still another api
sysfs_update_groups() that can be used in this case.
I just finish a local version and confirm following 3 commits should be enough:
1) factor out bitmap sysfs creation from mddev_set_bitmap_ops(), and create sysfs entries
after bitmap is created.
2) split bitmap/location out as the common bitmap groups, and convert bitmap filed groups
to struct attribute_group **groups;
3) add a separate bitmap_ops for none bitmap, location_store() can be similar to
patch 3 in your set, just create/remove additional internal_bitmap_group.
> Su Yue (5):
> md/md-bitmap: call md_bitmap_create,destroy in location_store
> md/md-bitmap: add an extra sysfs argument to md_bitmap_create and
> destroy
> md/md-bitmap: add dummy bitmap ops for none to fix wrong bitmap offset
> md: skip ID_BITMAP_NONE when show available bitmap types
> md/md-bitmap: remove member group from bitmap_operations
>
> drivers/md/md-bitmap.c | 121 ++++++++++++++++++++++++++++++++++++---
> drivers/md/md-bitmap.h | 3 +-
> drivers/md/md-llbitmap.c | 13 ++++-
> drivers/md/md.c | 55 +++++++++---------
> drivers/md/md.h | 2 +
> 5 files changed, 155 insertions(+), 39 deletions(-)
>
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH v2 0/5] md: bitmap grow fixes
From: Su Yue @ 2026-04-21 5:39 UTC (permalink / raw)
To: Yu Kuai
Cc: Su Yue, linux-raid, song, xni,
linan122@huawei.com >> Li Nan, Heming Zhao
In-Reply-To: <afb84561-e7c4-4d08-b7ca-bd20e9f8dbda@fnnas.com>
On Tue 21 Apr 2026 at 13:15, "Yu Kuai" <yukuai@fnnas.com> wrote:
> Hi,
>
> 在 2026/4/7 18:26, Su Yue 写道:
>> Hi, This v2 series is to fixes bugs exposed by
>> mdadm/clustermd-autotest.
>> The bugs are not cluster md only but also bitmap related.
>>
>> The series based on v7.0-rc7 passes tests with mdadm v4.6
>> without regression.
>>
>> v2:
>> Add a dummy bitmap operations per Kuai's suggestion.
>>
>> To Yu Kuai:
>> A NULL group can't used for internal_bitmap_group since
>> the entries from internal_bitmap_attrs should be under bitmap.
>> So instead of sysfs_create_groups(), sysfs_create_group() and
>> sysfs_merge_group()
>> are still needed /sigh.
>
> Thanks for the set and sorry for the delay.
>
> TBO, this set is still a bit complex than expected. It's true
> sysfs_create_groups()
> can't be used for the groups with same name. However, there is
> still another api
> sysfs_update_groups() that can be used in this case.
>
IIUC, sysfs_update_groups() can't be used because it first removes
then re-add files
see linux/fs/sysfs/group.c:create_files() for details.
>
> I just finish a local version and confirm following 3 commits
> should be enough:
>
If you would like to send your own patchset. I am happy to give
up my patchset then
review yours :) The only thing I want to fix the issue. Thanks.
--
Su
> 1) factor out bitmap sysfs creation from mddev_set_bitmap_ops(),
> and create sysfs entries
> after bitmap is created.
> 2) split bitmap/location out as the common bitmap groups, and
> convert bitmap filed groups
> to struct attribute_group **groups;
> 3) add a separate bitmap_ops for none bitmap, location_store()
> can be similar to
> patch 3 in your set, just create/remove additional
> internal_bitmap_group.
>
>> Su Yue (5):
>> md/md-bitmap: call md_bitmap_create,destroy in
>> location_store
>> md/md-bitmap: add an extra sysfs argument to
>> md_bitmap_create and
>> destroy
>> md/md-bitmap: add dummy bitmap ops for none to fix wrong
>> bitmap offset
>> md: skip ID_BITMAP_NONE when show available bitmap types
>> md/md-bitmap: remove member group from bitmap_operations
>>
>> drivers/md/md-bitmap.c | 121
>> ++++++++++++++++++++++++++++++++++++---
>> drivers/md/md-bitmap.h | 3 +-
>> drivers/md/md-llbitmap.c | 13 ++++-
>> drivers/md/md.c | 55 +++++++++---------
>> drivers/md/md.h | 2 +
>> 5 files changed, 155 insertions(+), 39 deletions(-)
>>
^ permalink raw reply
* Re: [PATCH] md/raid5: fix race between reshape and chunk-aligned read
From: FengWei Shih @ 2026-04-21 7:09 UTC (permalink / raw)
To: yukuai, song; +Cc: linan122, linux-raid, linux-kernel
In-Reply-To: <68b61dc3-f7fa-49ba-9f26-f07b7fa4768d@fnnas.com>
Hi Kuai,
Yu Kuai 於 2026/4/19 下午 01:14 寫道:
> Hi,
>
> 在 2026/4/9 13:17, FengWei Shih 写道:
>> raid5_make_request() checks mddev->reshape_position to decide whether
>> to allow chunk-aligned reads. However in raid5_start_reshape(), the
>> layout configuration (raid_disks, algorithm, etc.) is updated before
>> mddev->reshape_position is set:
>>
>> reshape (raid5_start_reshape) read (raid5_make_request)
>> ============================== ===========================
>> write_seqcount_begin
>> update raid_disks, algorithm...
>> set conf->reshape_progress
>> write_seqcount_end
>> check mddev->reshape_position
>> * still MaxSector, allow
>> raid5_read_one_chunk()
>> * use new layout
>> raid5_quiesce()
>> set mddev->reshape_position
> While we're here, I think it's pretty ugly to disable raid5_read_one_chunk
> when reshape is not fully done. So a better solution should be:
>
> - data behind reshape: read with new layout
> - data ahead of reshape: read with old layout, reshape will also need to wait
> for this IO to be done, before reshape can make progress.
> - data intersecting the active reshape window: wait for reshape to make progress.
Thanks for the feedback. I agree that using reshape_progress to distinguish
the three cases (ahead/behind/inside reshape) would be more refined.
But disabling chunk-aligned reads during reshape was already the
existing design.
In the original code, the check is at the caller level:
if (rw == READ && mddev->degraded == 0 &&
mddev->reshape_position == MaxSector) {
bi = chunk_aligned_read(mddev, bi);
}
My patch is focused on fixing the race condition in the existing
lockless check of whether
reshape is in progress. So just to confirm: are you suggesting we add a
mechanism to
allow chunk-aligned reads during reshape (based on reshape progress), rather
than simply disabling them?
>> Since reshape_position is not yet updated, raid5_make_request()
>> considers no reshape is in progress and proceeds with the
>> chunk-aligned path, but the layout has already changed, causing
>> raid5_compute_sector() to return an incorrect physical address.
>>
>> Fix this by reading conf->reshape_progress under gen_lock in
>> raid5_read_one_chunk() and falling back to the stripe path if a
>> reshape is in progress.
>>
>> Signed-off-by: FengWei Shih <dannyshih@synology.com>
>> ---
>> drivers/md/raid5.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>> index a8e8d431071b..bded2b86f0ef 100644
>> --- a/drivers/md/raid5.c
>> +++ b/drivers/md/raid5.c
>> @@ -5421,6 +5421,11 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
>> sector_t sector, end_sector;
>> int dd_idx;
>> bool did_inc;
>> + int seq;
>> +
>> + seq = read_seqcount_begin(&conf->gen_lock);
>> + if (unlikely(conf->reshape_progress != MaxSector))
>> + return 0;
>>
>> if (!in_chunk_boundary(mddev, raid_bio)) {
>> pr_debug("%s: non aligned\n", __func__);
>> @@ -5431,6 +5436,9 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio)
>> &dd_idx, NULL);
>> end_sector = sector + bio_sectors(raid_bio);
>>
>> + if (read_seqcount_retry(&conf->gen_lock, seq))
>> + return 0;
>> +
>> if (r5c_big_stripe_cached(conf, sector))
>> return 0;
>>
Thanks,
FengWei Shih
Disclaimer: The contents of this e-mail message and any attachments are confidential and are intended solely for addressee. The information may also be legally privileged. This transmission is sent in trust, for the sole purpose of delivery to the intended recipient. If you have received this transmission in error, any use, reproduction or dissemination of this transmission is strictly prohibited. If you are not the intended recipient, please immediately notify the sender by reply e-mail or phone and delete this message and its attachments, if any.
^ 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