* Re: [PATCH] md/raid1: fix len reuse across rdevs in choose_first_rdev()
From: Abd-Alrhman Masalkhi @ 2026-04-28 11:53 UTC (permalink / raw)
To: Yu Kuai, song, paul.e.luse, xni, yukuai; +Cc: linux-raid, linux-kernel
In-Reply-To: <5c71cb6b-d860-4bcb-a900-a27544be7a17@fnnas.com>
Hi Kaui,
On Tue, Apr 28, 2026 at 16:23 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/26 17:35, Abd-Alrhman Masalkhi 写道:
>> choose_first_rdev() initializes the variable len before iterating over
>> all rdevs, but passes it by reference to raid1_check_read_range(), which
>> it might update *len and return 0 depending on the layout of the bad
>> block region. As a result, 'len' can be modified during the first
>> iteration and reused for subsequent rdevs, causing later devices to be
>> evaluated with an incorrect length value.
>>
>> Fixes: 31a73331752d3 ("md/raid1: factor out read_first_rdev() from read_balance()")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> drivers/md/raid1.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index b549be9174bb..5f5dbf79c903 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -591,12 +591,12 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>> int *max_sectors)
>> {
>> sector_t this_sector = r1_bio->sector;
>> - int len = r1_bio->sectors;
>> int disk;
>>
>> for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
>> struct md_rdev *rdev;
>> int read_len;
>> + int len = r1_bio->sectors;
>>
>> if (r1_bio->bios[disk] == IO_BLOCKED)
>> continue;
>
> This patch is wrong, choose_first_rdev() is used when raid1_should_read_first() is true,
> meaning the read overlaps an unsynced/resyncing area. Reset len can cause the problem that
> reading the same area can return different data.
>
Thank you for the detailed explanation. After carefully re-reading the
code and your feedback, I understand why the patch is wrong.
> --
> Thansk,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH] md: skip redundant raid_disks update when value is unchanged
From: Abd-Alrhman Masalkhi @ 2026-04-28 9:56 UTC (permalink / raw)
To: Yu Kuai, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <9748ccf8-7d4f-495f-8083-eb640008f073@fnnas.com>
On Tue, Apr 28, 2026 at 16:25 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/25 16:58, Abd-Alrhman Masalkhi 写道:
>> Calling update_raid_disks() with the same value as the current one
>> can trigger unnecessary work. For example, RAID1 will reallocate
>> resources such as the mempool for r1bio.
>>
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> It returns -EINVAL for the same value. If silent success is preferred
>> instead, please let me know so I adjust its behavior.
>> ---
>> drivers/md/md.c | 9 ++++++---
>> 1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/md/md.c b/drivers/md/md.c
>> index 0e55639211f2..cb66c4ebbafa 100644
>> --- a/drivers/md/md.c
>> +++ b/drivers/md/md.c
>> @@ -4409,9 +4409,12 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
>> err = mddev_suspend_and_lock(mddev);
>> if (err)
>> return err;
>> - if (mddev->pers)
>> - err = update_raid_disks(mddev, n);
>> - else if (mddev->reshape_position != MaxSector) {
>> + if (mddev->pers) {
>> + if (n != mddev->raid_disks)
>> + err = update_raid_disks(mddev, n);
>> + else
>> + err = -EINVAL;
>
> Changing return value in this case is not expected, especially from
> success to failure.
>
I’ll adjust the behavior to return success instead.
>> + } else if (mddev->reshape_position != MaxSector) {
>> struct md_rdev *rdev;
>> int olddisks = mddev->raid_disks - mddev->delta_disks;
>>
>
> --
> Thansk,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Abd-Alrhman Masalkhi @ 2026-04-28 9:46 UTC (permalink / raw)
To: Yu Kuai, song, shli, neilb, linux-raid, linux-kernel, yukuai
In-Reply-To: <2cf6f585-a0de-4c84-9cfc-05e1f6fde549@fnnas.com>
Hi Kuai,
Thank you for the feedback.
On Tue, Apr 28, 2026 at 16:54 +0800, Yu Kuai wrote:
> Hi,
>
> 在 2026/4/27 18:34, Abd-Alrhman Masalkhi 写道:
>> Splitting a bio while executing in the raid1 thread can lead to
>> recursion, as task->bio_list is NULL in this context.
>>
>> In addition, resubmitting an md_cloned_bio after splitting may lead to
>> a deadlock if the array is suspended before the md driver calls
>> percpu_ref_tryget_live(&mddev->active_io) on it's path to
>> pers->make_request().
>
> I don't understand, I agree this is problematic in the suspend case, but
> what's wrong with task->bio_list being NULL? This can only cause the reverse
> order because the split bio will submit first. However this is not a big deal
> as this is the slow error patch.
>
> If suspend is the only problem here, the simple fix is to add checking
> in md_handle_request().
>
I meant that when current->bio_list is NULL, raid1_read_request()
can recurse into itself, as shown in the following trace-cmd output:
raid1_read_request() { <---
bio_submit_split_bioset() {
bio_split() {..}
bio_chain();
submit_bio_noacct_nocheck() {
__submit_bio() {
md_submit_bio() {
md_handle_request() {
raid1_make_request() {
raid1_read_request() { <---
md_account_bio() {
If this behavior is not an issue, I will follow your suggestion and
only add the check in md_handle_request().
>>
>> Avoid splitting the bio in this context and require that it is either
>> read in full or not at all.
>>
>> This prevents recursion and avoids potential deadlocks during array
>> suspension.
>>
>> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> I sent an email about this issue two days ago, but at the time I was not
>> sure whether it was a real problem or a misunderstanding on my part.
>>
>> After further analysis, it appears that this issue can occur.
>>
>> Apologies for the earlier confusion, and thank you for your time.
>>
>> Abd-Alrhman
>> ---
>> drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index cc9914bd15c1..14f6d6625811 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>>
>> /* choose the first disk even if it has some bad blocks. */
>> read_len = raid1_check_read_range(rdev, this_sector, &len);
>> - if (read_len > 0) {
>> + if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
>> update_read_sectors(conf, disk, this_sector, read_len);
>> *max_sectors = read_len;
>> return disk;
>> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>> }
>>
>> if (bb_disk != -1) {
>> - *max_sectors = bb_read_len;
>> - update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
>> + if (!*max_sectors || bb_read_len == r1_bio->sectors) {
>> + *max_sectors = bb_read_len;
>> + update_read_sectors(conf, bb_disk, this_sector,
>> + bb_read_len);
>> + } else {
>> + bb_disk = -1;
>> + }
>> }
>>
>> return bb_disk;
>> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>> * disks and disks with bad blocks for now. Only pay attention to key disk
>> * choice.
>> *
>> - * 3) If we've made it this far, now look for disks with bad blocks and choose
>> - * the one with most number of sectors.
>> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
>> + * of bad blocks), look for disks with bad blocks and choose the one with
>> + * the most sectors.
>> *
>> * 4) If we are all the way at the end, we have no choice but to use a disk even
>> * if it is write mostly.
>> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
>> /*
>> * If we are here it means we didn't find a perfectly good disk so
>> * now spend a bit more time trying to find one with the most good
>> - * sectors.
>> + * sectors. but only if we are tolerant of bad blocks.
>> */
>> - disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> - if (disk >= 0)
>> - return disk;
>> + if (!*max_sectors) {
>> + disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> + if (disk >= 0)
>> + return disk;
>> + }
>>
>> return choose_slow_rdev(conf, r1_bio, max_sectors);
>> }
>> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> /*
>> * make_request() can abort the operation when read-ahead is being
>> * used and no empty request is available.
>> + *
>> + * If we allow splitting the bio while executing in the raid1 thread,
>> + * we may end up recursing (current->bio_list is NULL), and we might
>> + * also deadlock if we try to suspend the array, since we are
>> + * resubmitting an md_cloned_bio. Therefore, we must be read either
>> + * all the sectors or none.
>> */
>> + max_sectors = r1bio_existed;
>> rdisk = read_balance(conf, r1_bio, &max_sectors);
>> if (rdisk < 0) {
>> /* couldn't find anywhere to read from */
>
> --
> Thansk,
> Kuai
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH 0/3] md/raid1: small cleanups in MD and raid1 drivers
From: Yu Kuai @ 2026-04-28 9:01 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260423101303.48196-1-abd.masalkhi@gmail.com>
在 2026/4/23 18:13, Abd-Alrhman Masalkhi 写道:
> Hi,
>
> This small series contains three cleanups in the MD and raid1 drivers.
>
> Thanks,
> Abd-Alrhman
>
> Abd-Alrhman Masalkhi (3):
> md/raid1: replace wait loop with wait_event_idle() in
> raid1_write_request()
> md: use mddev_is_dm() instead of open-coding gendisk checks
> md: use ATTRIBUTE_GROUPS() for md default sysfs attributes
>
> drivers/md/md.c | 16 ++++------------
> drivers/md/raid1.c | 15 ++++-----------
> 2 files changed, 8 insertions(+), 23 deletions(-)
Applied to md-7.1
>
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Yu Kuai @ 2026-04-28 8:54 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, shli, neilb, linux-raid, linux-kernel,
yukuai
In-Reply-To: <20260427103446.300378-1-abd.masalkhi@gmail.com>
Hi,
在 2026/4/27 18:34, Abd-Alrhman Masalkhi 写道:
> Splitting a bio while executing in the raid1 thread can lead to
> recursion, as task->bio_list is NULL in this context.
>
> In addition, resubmitting an md_cloned_bio after splitting may lead to
> a deadlock if the array is suspended before the md driver calls
> percpu_ref_tryget_live(&mddev->active_io) on it's path to
> pers->make_request().
I don't understand, I agree this is problematic in the suspend case, but
what's wrong with task->bio_list being NULL? This can only cause the reverse
order because the split bio will submit first. However this is not a big deal
as this is the slow error patch.
If suspend is the only problem here, the simple fix is to add checking
in md_handle_request().
>
> Avoid splitting the bio in this context and require that it is either
> read in full or not at all.
>
> This prevents recursion and avoids potential deadlocks during array
> suspension.
>
> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> I sent an email about this issue two days ago, but at the time I was not
> sure whether it was a real problem or a misunderstanding on my part.
>
> After further analysis, it appears that this issue can occur.
>
> Apologies for the earlier confusion, and thank you for your time.
>
> Abd-Alrhman
> ---
> drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index cc9914bd15c1..14f6d6625811 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>
> /* choose the first disk even if it has some bad blocks. */
> read_len = raid1_check_read_range(rdev, this_sector, &len);
> - if (read_len > 0) {
> + if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
> update_read_sectors(conf, disk, this_sector, read_len);
> *max_sectors = read_len;
> return disk;
> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
> }
>
> if (bb_disk != -1) {
> - *max_sectors = bb_read_len;
> - update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
> + if (!*max_sectors || bb_read_len == r1_bio->sectors) {
> + *max_sectors = bb_read_len;
> + update_read_sectors(conf, bb_disk, this_sector,
> + bb_read_len);
> + } else {
> + bb_disk = -1;
> + }
> }
>
> return bb_disk;
> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
> * disks and disks with bad blocks for now. Only pay attention to key disk
> * choice.
> *
> - * 3) If we've made it this far, now look for disks with bad blocks and choose
> - * the one with most number of sectors.
> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
> + * of bad blocks), look for disks with bad blocks and choose the one with
> + * the most sectors.
> *
> * 4) If we are all the way at the end, we have no choice but to use a disk even
> * if it is write mostly.
> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
> /*
> * If we are here it means we didn't find a perfectly good disk so
> * now spend a bit more time trying to find one with the most good
> - * sectors.
> + * sectors. but only if we are tolerant of bad blocks.
> */
> - disk = choose_bb_rdev(conf, r1_bio, max_sectors);
> - if (disk >= 0)
> - return disk;
> + if (!*max_sectors) {
> + disk = choose_bb_rdev(conf, r1_bio, max_sectors);
> + if (disk >= 0)
> + return disk;
> + }
>
> return choose_slow_rdev(conf, r1_bio, max_sectors);
> }
> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> /*
> * make_request() can abort the operation when read-ahead is being
> * used and no empty request is available.
> + *
> + * If we allow splitting the bio while executing in the raid1 thread,
> + * we may end up recursing (current->bio_list is NULL), and we might
> + * also deadlock if we try to suspend the array, since we are
> + * resubmitting an md_cloned_bio. Therefore, we must be read either
> + * all the sectors or none.
> */
> + max_sectors = r1bio_existed;
> rdisk = read_balance(conf, r1_bio, &max_sectors);
> if (rdisk < 0) {
> /* couldn't find anywhere to read from */
--
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-28 8:37 UTC (permalink / raw)
To: yukuai
Cc: Junrui Luo, Song Liu, Li Nan, NeilBrown, Jonathan Brassow,
linux-raid, linux-kernel, stable
In-Reply-To: <282278bc-7d71-4049-89f4-a9f3968504dd@fnnas.com>
Hi Kuai,
Looks like different maintainers have different rules. :(
Can you send me the patchwork resource?
Thanks.
On Tue, Apr 28, 2026 at 4:32 PM Yu Kuai <yukuai@fnnas.com> wrote:
>
> Hi,
>
> 在 2026/4/19 13:59, Yuhao Jiang 写道:
> > Hi Kuai,
> >
> > This report was reported by me, so Junrui added me as Reported-by.
>
> This is fine, however, please do not add downstream reported-by tag.
> If you want to add the reported-by tag, please report the problem to
> patchwork first. :)
>
> >
> > 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 <mailto:Cc%3Astable@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
>
> --
> Thansk,
> Kuai
--
Yuhao Jiang
^ permalink raw reply
* Re: [RFC PATCH] dm-raid: only requeue bios when dm is suspending.
From: Yu Kuai @ 2026-04-28 8:35 UTC (permalink / raw)
To: Benjamin Marzinski, Yang Xiuwei
Cc: Yu Kuai, Li Nan, Song Liu, linux-raid, dm-devel, Xiao Ni,
Nigel Croxon, yukuai
In-Reply-To: <20260414190350.1946406-1-bmarzins@redhat.com>
Hi,
在 2026/4/15 3:03, Benjamin Marzinski 写道:
> returning DM_MAPIO_REQUEUE from the target map() function only requeues
> the bio during noflush suspends. During regular operations or during
> flushing suspends, it fails the bio. Failing the bio during flushing
> suspends is the correct behavior here. We cannot handle the bio, and we
> cannot suspends while it is outstanding. But during normal operations,
> we should not push the bio back to do. Instead, wait for the reshape
> to be resumed.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>
> Yang Xiuwei, if you are still able to see I/O errors during LVM testing,
> does this patch fix them?
>
> drivers/md/dm-raid.c | 7 +++++++
> drivers/md/md.h | 1 +
> drivers/md/raid5.c | 6 ++++--
> 3 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index 4bacdc499984..cac61d57e7e2 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -3831,6 +3831,7 @@ static void raid_presuspend(struct dm_target *ti)
> * resume, raid_postsuspend() is too late.
> */
> set_bit(RT_FLAG_RS_FROZEN, &rs->runtime_flags);
> + WRITE_ONCE(mddev->dm_suspending, 1);
>
> if (!reshape_interrupted(mddev))
> return;
> @@ -3847,6 +3848,9 @@ static void raid_presuspend(struct dm_target *ti)
> static void raid_presuspend_undo(struct dm_target *ti)
> {
> struct raid_set *rs = ti->private;
> + struct mddev *mddev = &rs->md;
> +
> + WRITE_ONCE(mddev->dm_suspending, 0);
>
> clear_bit(RT_FLAG_RS_FROZEN, &rs->runtime_flags);
> }
> @@ -3854,6 +3858,7 @@ static void raid_presuspend_undo(struct dm_target *ti)
> static void raid_postsuspend(struct dm_target *ti)
> {
> struct raid_set *rs = ti->private;
> + struct mddev *mddev = &rs->md;
>
> if (!test_and_set_bit(RT_FLAG_RS_SUSPENDED, &rs->runtime_flags)) {
> /*
> @@ -3864,6 +3869,8 @@ static void raid_postsuspend(struct dm_target *ti)
> mddev_suspend(&rs->md, false);
> rs->md.ro = MD_RDONLY;
> }
> + WRITE_ONCE(mddev->dm_suspending, 0);
> +
> }
>
> static void attempt_restore_of_faulty_devices(struct raid_set *rs)
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index ac84289664cd..e8d7332c5cb9 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -463,6 +463,7 @@ struct mddev {
> int delta_disks, new_level, new_layout;
> int new_chunk_sectors;
> int reshape_backwards;
> + int dm_suspending;
This patch looks fine, however, can you also optimize it by a new
flag instead a new int field ?
>
> struct md_thread __rcu *thread; /* management thread */
> struct md_thread __rcu *sync_thread; /* doing resync or reconstruct */
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 8854e024f311..d528263f92a3 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -6042,8 +6042,10 @@ static enum stripe_result make_stripe_request(struct mddev *mddev,
> raid5_release_stripe(sh);
> out:
> if (ret == STRIPE_SCHEDULE_AND_RETRY && reshape_interrupted(mddev)) {
> - bi->bi_status = BLK_STS_RESOURCE;
> - ret = STRIPE_WAIT_RESHAPE;
> + if (!mddev_is_dm(mddev) || READ_ONCE(mddev->dm_suspending)) {
> + bi->bi_status = BLK_STS_RESOURCE;
> + ret = STRIPE_WAIT_RESHAPE;
> + }
> pr_err_ratelimited("dm-raid456: io across reshape position while reshape can't make progress");
> }
> return ret;
--
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-28 8:32 UTC (permalink / raw)
To: Yuhao Jiang
Cc: Junrui Luo, Song Liu, Li Nan, NeilBrown, Jonathan Brassow,
linux-raid, linux-kernel, stable, yukuai
In-Reply-To: <CAHYQsXRN6uof4yyDR6qGteQ=wZTt86VUx7km6k=LbNAQ3wxGiQ@mail.gmail.com>
Hi,
在 2026/4/19 13:59, Yuhao Jiang 写道:
> Hi Kuai,
>
> This report was reported by me, so Junrui added me as Reported-by.
This is fine, however, please do not add downstream reported-by tag.
If you want to add the reported-by tag, please report the problem to
patchwork first. :)
>
> 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 <mailto:Cc%3Astable@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
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid5: fix race between reshape and chunk-aligned read
From: Yu Kuai @ 2026-04-28 8:29 UTC (permalink / raw)
To: FengWei Shih, song; +Cc: linan122, linux-raid, linux-kernel, yukuai
In-Reply-To: <d298e159-1e47-4c97-80cd-41c4c1c74a04@synology.com>
Hi,
在 2026/4/21 15:09, FengWei Shih 写道:
> 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?
Yes, I think this is a huge performance improvement. And in this case it's totally
fine to do chunk aligned read with old layout since reshape do not start yet.
>
>>> 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.
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md: skip redundant raid_disks update when value is unchanged
From: Yu Kuai @ 2026-04-28 8:25 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song; +Cc: linux-raid, linux-kernel, yukuai
In-Reply-To: <20260425085843.3725-1-abd.masalkhi@gmail.com>
Hi,
在 2026/4/25 16:58, Abd-Alrhman Masalkhi 写道:
> Calling update_raid_disks() with the same value as the current one
> can trigger unnecessary work. For example, RAID1 will reallocate
> resources such as the mempool for r1bio.
>
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> It returns -EINVAL for the same value. If silent success is preferred
> instead, please let me know so I adjust its behavior.
> ---
> drivers/md/md.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0e55639211f2..cb66c4ebbafa 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4409,9 +4409,12 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
> err = mddev_suspend_and_lock(mddev);
> if (err)
> return err;
> - if (mddev->pers)
> - err = update_raid_disks(mddev, n);
> - else if (mddev->reshape_position != MaxSector) {
> + if (mddev->pers) {
> + if (n != mddev->raid_disks)
> + err = update_raid_disks(mddev, n);
> + else
> + err = -EINVAL;
Changing return value in this case is not expected, especially from
success to failure.
> + } else if (mddev->reshape_position != MaxSector) {
> struct md_rdev *rdev;
> int olddisks = mddev->raid_disks - mddev->delta_disks;
>
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: fix len reuse across rdevs in choose_first_rdev()
From: Yu Kuai @ 2026-04-28 8:23 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi, song, paul.e.luse, xni, yukuai
Cc: linux-raid, linux-kernel
In-Reply-To: <20260426093506.14316-1-abd.masalkhi@gmail.com>
Hi,
在 2026/4/26 17:35, Abd-Alrhman Masalkhi 写道:
> choose_first_rdev() initializes the variable len before iterating over
> all rdevs, but passes it by reference to raid1_check_read_range(), which
> it might update *len and return 0 depending on the layout of the bad
> block region. As a result, 'len' can be modified during the first
> iteration and reused for subsequent rdevs, causing later devices to be
> evaluated with an incorrect length value.
>
> Fixes: 31a73331752d3 ("md/raid1: factor out read_first_rdev() from read_balance()")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> drivers/md/raid1.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index b549be9174bb..5f5dbf79c903 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -591,12 +591,12 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
> int *max_sectors)
> {
> sector_t this_sector = r1_bio->sector;
> - int len = r1_bio->sectors;
> int disk;
>
> for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
> struct md_rdev *rdev;
> int read_len;
> + int len = r1_bio->sectors;
>
> if (r1_bio->bios[disk] == IO_BLOCKED)
> continue;
This patch is wrong, choose_first_rdev() is used when raid1_should_read_first() is true,
meaning the read overlaps an unsynced/resyncing area. Reset len can cause the problem that
reading the same area can return different data.
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH v13 0/3] md/md-bitmap: restore bitmap grow through sysfs
From: Yu Kuai @ 2026-04-28 8:21 UTC (permalink / raw)
To: Su Yue
Cc: Song Liu, glass.su, Li Nan, Xiao Ni, linux-raid, linux-kernel,
yukuai
In-Reply-To: <qzo35t50.fsf@damenly.org>
Hi,
在 2026/4/25 16:41, Su Yue 写道:
> On Sat 25 Apr 2026 at 10:46, Yu Kuai <yukuai@fnnas.com> wrote:
>
>> mdadm --grow adds an internal bitmap by writing bitmap/location for an
>> array that currently has no bitmap. That requires the bitmap directory
>> and location attribute to exist before the classic bitmap backend is
>> created.
>>
>> This series separates bitmap backend lifetime from bitmap sysfs
>> lifetime,
>> splits the sysfs layout into common and backend-specific groups, and
>> adds
>> a small "none" bitmap backend. The none backend keeps bitmap/location
>> available while no real bitmap is active, and the location store path
>> can
>> then switch between the none backend and the classic bitmap backend
>> without tearing down the common bitmap sysfs directory.
>>
>> Patch 1 factors bitmap creation and destruction into helpers that do not
>> touch sysfs registration.
>>
>> Patch 2 splits the classic bitmap sysfs files into a common group and an
>> internal-bitmap group, and converts bitmap backend operations to use a
>> sysfs group array.
>>
>> Patch 3 adds the none backend and uses it to restore mdadm --grow bitmap
>> addition through bitmap/location.
>>
>> Changes since v12:
>> - Keep the factoring patch focused on no-sysfs bitmap lifetime helpers.
>> - Make bitmap operation lookup depend only on the current bitmap id
>> matching the installed backend.
>> - Trim the none backend to only the operations required by the active
>> call paths.
>> - Rework bitmap/location error handling with explicit cleanup labels.
>> - Restore the none backend after bitmap removal and creation/load
>> failures so bitmap/location stays available.
>>
>> Validation:
>> - create a RAID1 array with --bitmap=none
>> - verify /sys/block/md0/md/bitmap/location exists and reports "none"
>> - mdadm --grow /dev/md0 --bitmap=internal
>> - verify location switches to "+8", mdadm reports "Intent Bitmap:
>> Internal", and /proc/mdstat reports a bitmap
>> - mdadm --grow /dev/md0 --bitmap=none
>> - verify location switches back to "none" and only the common
>> location
>> attribute remains under md/bitmap
>> - repeat the internal/none switch once more
>> - Checked the QEMU serial log for panic, Oops, BUG, WARNING, Call Trace,
>> RCU stall, and hung-task patterns; none were found.
>>
>> Yu Kuai (3):
>> md: factor bitmap creation away from sysfs handling
>> md/md-bitmap: split bitmap sysfs groups
>> md/md-bitmap: add a none backend for bitmap grow
>>
>
> Thanks for all.
> Would you like to add tag Fixes: fb8cc3b0d9db for the whole series
> while merging?
Sure, sorry that I forgot the fix tag.
>
> --
> Su
>
>> drivers/md/md-bitmap.c | 131 +++++++++++++++++++++++++++++++++++----
>> drivers/md/md-bitmap.h | 2 +-
>> drivers/md/md-llbitmap.c | 7 ++-
>> drivers/md/md.c | 125 ++++++++++++++++++++++++++-----------
>> drivers/md/md.h | 3 +
>> 5 files changed, 218 insertions(+), 50 deletions(-)
Applied with following fix tag for patch 3:
Fixes: fb8cc3b0d9db ("md/md-bitmap: delay registration of bitmap_ops until creating bitmap")
>>
>> base-commit: c85d314b135ff569c1031f2ef8e40368bcfe72ac
--
Thansk,
Kuai
^ permalink raw reply
* Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Abd-Alrhman Masalkhi @ 2026-04-28 8:16 UTC (permalink / raw)
To: Paul Menzel; +Cc: song, yukuai, shli, neilb, linux-raid, linux-kernel
In-Reply-To: <c859ab17-cfc6-4e54-9a79-8b0d2b145adc@molgen.mpg.de>
Hi Paul,
On Mon, Apr 27, 2026 at 16:49 +0200, Paul Menzel wrote:
> Dear Abd-Alrhman,
>
>
> Thank you for your patch.
>
RAID 10 seems to have a similar issue, i will fix it too.
> Am 27.04.26 um 12:34 schrieb Abd-Alrhman Masalkhi:
>> Splitting a bio while executing in the raid1 thread can lead to
>> recursion, as task->bio_list is NULL in this context.
>>
>> In addition, resubmitting an md_cloned_bio after splitting may lead to
>> a deadlock if the array is suspended before the md driver calls
>> percpu_ref_tryget_live(&mddev->active_io) on it's path to
>> pers->make_request().
>>
>> Avoid splitting the bio in this context and require that it is either
>> read in full or not at all.
>>
>> This prevents recursion and avoids potential deadlocks during array
>> suspension.
>
> Do you have a reproducer?
>
>> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> I sent an email about this issue two days ago, but at the time I was not
>> sure whether it was a real problem or a misunderstanding on my part.
>>
>> After further analysis, it appears that this issue can occur.
>>
>> Apologies for the earlier confusion, and thank you for your time.
>>
>> Abd-Alrhman
>
> I suggest to always share the URL (lore.kernel.org), when referencing
> another thread. If relevant, maybe even reference your message with a
> Link: tag in the commit message.
>
>> ---
>> drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index cc9914bd15c1..14f6d6625811 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>>
>> /* choose the first disk even if it has some bad blocks. */
>> read_len = raid1_check_read_range(rdev, this_sector, &len);
>> - if (read_len > 0) {
>> + if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
>> update_read_sectors(conf, disk, this_sector, read_len);
>> *max_sectors = read_len;
>> return disk;
>> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>> }
>>
>> if (bb_disk != -1) {
>> - *max_sectors = bb_read_len;
>> - update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
>> + if (!*max_sectors || bb_read_len == r1_bio->sectors) {
>> + *max_sectors = bb_read_len;
>> + update_read_sectors(conf, bb_disk, this_sector,
>> + bb_read_len);
>> + } else {
>> + bb_disk = -1;
>> + }
>> }
>>
>> return bb_disk;
>> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>> * disks and disks with bad blocks for now. Only pay attention to key disk
>> * choice.
>> *
>> - * 3) If we've made it this far, now look for disks with bad blocks and choose
>> - * the one with most number of sectors.
>> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
>> + * of bad blocks), look for disks with bad blocks and choose the one with
>> + * the most sectors.
>> *
>> * 4) If we are all the way at the end, we have no choice but to use a disk even
>> * if it is write mostly.
>> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
>> /*
>> * If we are here it means we didn't find a perfectly good disk so
>> * now spend a bit more time trying to find one with the most good
>> - * sectors.
>> + * sectors. but only if we are tolerant of bad blocks.
>
> s/but/But/
>
>> */
>> - disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> - if (disk >= 0)
>> - return disk;
>> + if (!*max_sectors) {
>> + disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> + if (disk >= 0)
>> + return disk;
>> + }
>>
>> return choose_slow_rdev(conf, r1_bio, max_sectors);
>> }
>> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> /*
>> * make_request() can abort the operation when read-ahead is being
>> * used and no empty request is available.
>> + *
>> + * If we allow splitting the bio while executing in the raid1 thread,
>> + * we may end up recursing (current->bio_list is NULL), and we might
>> + * also deadlock if we try to suspend the array, since we are
>> + * resubmitting an md_cloned_bio. Therefore, we must be read either
>
> … we must read …
>
>> + * all the sectors or none.
>> */
>> + max_sectors = r1bio_existed;
>
> Excuse my ignorance, but I do not get why a bool is assigned to an int
> representing the maximum sector value.
>
>> rdisk = read_balance(conf, r1_bio, &max_sectors);
>> if (rdisk < 0) {
>> /* couldn't find anywhere to read from */
>
>
> Kind regards,
>
> Paul
--
Best Regards,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Abd-Alrhman Masalkhi @ 2026-04-27 17:44 UTC (permalink / raw)
To: Paul Menzel; +Cc: song, yukuai, shli, neilb, linux-raid, linux-kernel
In-Reply-To: <c859ab17-cfc6-4e54-9a79-8b0d2b145adc@molgen.mpg.de>
Hi Paul,
Thank you for the feedback.
On Mon, Apr 27, 2026 at 16:49 +0200, Paul Menzel wrote:
> Dear Abd-Alrhman,
>
>
> Thank you for your patch.
>
> Am 27.04.26 um 12:34 schrieb Abd-Alrhman Masalkhi:
>> Splitting a bio while executing in the raid1 thread can lead to
>> recursion, as task->bio_list is NULL in this context.
>>
>> In addition, resubmitting an md_cloned_bio after splitting may lead to
>> a deadlock if the array is suspended before the md driver calls
>> percpu_ref_tryget_live(&mddev->active_io) on it's path to
>> pers->make_request().
>>
>> Avoid splitting the bio in this context and require that it is either
>> read in full or not at all.
>>
>> This prevents recursion and avoids potential deadlocks during array
>> suspension.
>
> Do you have a reproducer?
I found this issue while reviewing the code and trying to understand the
read path.
The problem can be triggered when the first rdev cannot complete the
md_cloned_bio successfully, and RAID1 selects another rdev that cannot
fulfil the entire request. In that case, raid1_read_request() will split
the bio (the md_cloned_bio) via bio_submit_split_bioset(), which in turn
calls submit_bio_noacct_nocheck(). Since current->bio_list is NULL in
this context, this leads to raid1_read_request() being called again,
resulting in recursion.
I have also created a test to confirm that this can occur by modifying
the code with the following patch:
drivers/md/raid1.c | 52 ++++++++++++++++++++++++++++++++++++++++------
1 file changed, 46 insertions(+), 6 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index cc9914bd15c1..145e3ad0b1b8 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -362,11 +362,34 @@ static int find_bio_disk(struct r1bio *r1_bio, struct bio *bio)
static void raid1_end_read_request(struct bio *bio)
{
+ static int tmp = 75;
int uptodate = !bio->bi_status;
struct r1bio *r1_bio = bio->bi_private;
struct r1conf *conf = r1_bio->mddev->private;
struct md_rdev *rdev = conf->mirrors[r1_bio->read_disk].rdev;
+ if (tmp > 2) {
+ tmp --;
+ pr_info("--tmp = %d\n", tmp);
+ } else if (tmp == 2) {
+ if (r1_bio->sectors > 2 && uptodate) {
+ pr_info("I will start omitting errors\n");
+ bio->bi_status = BLK_STS_IOERR;
+ uptodate = false;
+ tmp = 0;
+ }
+ } else {
+ if (r1_bio->sectors > 2) {
+ if (tmp) {
+ bio->bi_status = BLK_STS_IOERR;
+ uptodate = false;
+ tmp = false;
+ } else {
+ tmp = true;
+ }
+ }
+ }
+
/*
* this branch is our 'one mirror IO has finished' event handler:
*/
@@ -607,7 +630,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
/* choose the first disk even if it has some bad blocks. */
read_len = raid1_check_read_range(rdev, this_sector, &len);
- if (read_len > 0) {
+ if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
update_read_sectors(conf, disk, this_sector, read_len);
*max_sectors = read_len;
return disk;
@@ -704,8 +727,12 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
}
if (bb_disk != -1) {
- *max_sectors = bb_read_len;
- update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
+ if (!*max_sectors || bb_read_len == r1_bio->sectors) {
+ *max_sectors = bb_read_len;
+ update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
+ } else {
+ bb_disk = -1;
+ }
}
return bb_disk;
@@ -884,9 +911,11 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
* now spend a bit more time trying to find one with the most good
* sectors.
*/
- disk = choose_bb_rdev(conf, r1_bio, max_sectors);
- if (disk >= 0)
- return disk;
+ if (!*max_sectors) {
+ disk = choose_bb_rdev(conf, r1_bio, max_sectors);
+ if (disk >= 0)
+ return disk;
+ }
return choose_slow_rdev(conf, r1_bio, max_sectors);
}
@@ -1320,6 +1349,11 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
int rdisk;
bool r1bio_existed = !!r1_bio;
+ if (mddev->thread && mddev->thread->tsk == current) {
+ pr_info("taask->bio_list = %p, %d\n", current->bio_list,
+ r1bio_existed);
+ }
+
/*
* If r1_bio is set, we are blocking the raid1d thread
* so there is a tiny risk of deadlock. So ask for
@@ -1347,6 +1381,7 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
* make_request() can abort the operation when read-ahead is being
* used and no empty request is available.
*/
+ max_sectors = r1bio_existed;
rdisk = read_balance(conf, r1_bio, &max_sectors);
if (rdisk < 0) {
/* couldn't find anywhere to read from */
@@ -1376,7 +1411,12 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
mddev->bitmap_ops->wait_behind_writes(mddev);
}
+ if (r1bio_existed)
+ max_sectors = max_sectors - 1;
+
if (max_sectors < bio_sectors(bio)) {
+ /* we are not allowed */
+ /* BUG_ON(r1bio_existed); */
bio = bio_submit_split_bioset(bio, max_sectors,
&conf->bio_split);
if (!bio) {
--
2.43.0
using trace-cmd, it shows the recursion clearly, its output:
raid1_read_request() {
bio_submit_split_bioset() {
bio_split() {
bio_alloc_clone() {
bio_alloc_bioset() {
mempool_alloc_noprof() {
__cond_resched();
mempool_alloc_slab() {
kmem_cache_alloc_noprof();
}
}
bio_associate_blkg() {
__rcu_read_lock();
kthread_blkcg();
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
}
__rcu_read_unlock();
}
}
bio_clone_blkg_association() {
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
__rcu_read_lock();
__rcu_read_unlock();
}
}
}
}
bio_chain();
should_fail_bio();
submit_bio_noacct_nocheck() {
blk_cgroup_bio_start();
__submit_bio() {
__rcu_read_lock();
__rcu_read_unlock();
md_submit_bio() {
bio_split_to_limits() {
bio_split_rw() {
bio_split_io_at();
bio_submit_split();
}
}
md_handle_request() {
__rcu_read_lock();
__rcu_read_unlock();
raid1_make_request() {
raid1_read_request() {
_printk() {
vprintk() {
vprintk_default() {
vprintk_emit() {
panic_on_other_cpu();
nbcon_get_default_prio() {
panic_on_this_cpu();
nbcon_get_cpu_emergency_nesting() {
printk_percpu_data_ready();
}
}
is_printk_legacy_deferred() {
is_printk_cpu_sync_owner();
}
vprintk_store() {
local_clock();
printk_parse_prefix();
is_printk_force_console();
prb_reserve() {
data_alloc() {
data_push_tail();
}
space_used.isra.0();
}
printk_sprint() {
printk_parse_prefix();
}
prb_final_commit() {
desc_update_last_finalized() {
_prb_read_valid() {
desc_read_finalized_seq();
}
_prb_read_valid() {
desc_read_finalized_seq();
panic_on_this_cpu();
}
}
}
}
console_trylock() {
panic_on_other_cpu();
__printk_safe_enter();
down_trylock() {
_raw_spin_lock_irqsave();
_raw_spin_unlock_irqrestore();
}
__printk_safe_exit();
}
console_unlock() {
nbcon_get_default_prio() {
panic_on_this_cpu();
nbcon_get_cpu_emergency_nesting() {
printk_percpu_data_ready();
}
}
is_printk_legacy_deferred() {
is_printk_cpu_sync_owner();
}
console_flush_one_record() {
nbcon_get_default_prio() {
panic_on_this_cpu();
nbcon_get_cpu_emergency_nesting() {
printk_percpu_data_ready();
}
}
is_printk_legacy_deferred() {
is_printk_cpu_sync_owner();
}
__srcu_read_lock();
printk_get_next_message() {
prb_read_valid() {
_prb_read_valid() {
desc_read_finalized_seq();
get_data();
desc_read_finalized_seq();
}
}
}
panic_on_other_cpu();
__srcu_read_unlock();
}
console_flush_one_record() {
nbcon_get_default_prio() {
panic_on_this_cpu();
nbcon_get_cpu_emergency_nesting() {
printk_percpu_data_ready();
}
}
is_printk_legacy_deferred() {
is_printk_cpu_sync_owner();
}
__srcu_read_lock();
printk_get_next_message() {
prb_read_valid() {
_prb_read_valid() {
desc_read_finalized_seq();
panic_on_this_cpu();
}
}
}
__srcu_read_unlock();
}
__printk_safe_enter();
up() {
_raw_spin_lock_irqsave();
_raw_spin_unlock_irqrestore();
}
__printk_safe_exit();
prb_read_valid() {
_prb_read_valid() {
desc_read_finalized_seq();
panic_on_this_cpu();
}
}
}
__wake_up_klogd();
}
}
}
}
mempool_alloc_noprof() {
__cond_resched();
mempool_kmalloc() {
__kmalloc_noprof();
}
}
md_account_bio() {
__rcu_read_lock();
__rcu_read_unlock();
bio_alloc_clone() {
bio_alloc_bioset() {
mempool_alloc_noprof() {
__cond_resched();
mempool_alloc_slab() {
kmem_cache_alloc_noprof();
}
}
bio_associate_blkg() {
__rcu_read_lock();
kthread_blkcg();
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
}
__rcu_read_unlock();
}
}
bio_clone_blkg_association() {
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
__rcu_read_lock();
__rcu_read_unlock();
}
}
}
bio_start_io_acct() {
update_io_ticks();
}
}
bio_alloc_clone() {
bio_alloc_bioset() {
mempool_alloc_noprof() {
__cond_resched();
mempool_alloc_slab() {
kmem_cache_alloc_noprof();
}
}
bio_associate_blkg() {
__rcu_read_lock();
kthread_blkcg();
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
}
__rcu_read_unlock();
}
}
bio_clone_blkg_association() {
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
__rcu_read_lock();
__rcu_read_unlock();
}
}
}
submit_bio_noacct() {
__cond_resched();
submit_bio_noacct_nocheck() {
blk_cgroup_bio_start();
}
}
}
}
__rcu_read_lock();
__rcu_read_unlock();
}
}
__rcu_read_lock();
__rcu_read_unlock();
}
__submit_bio() {
blk_mq_submit_bio() {
__rcu_read_lock();
__rcu_read_unlock();
bio_split_rw() {
bio_split_io_at();
bio_submit_split();
}
blk_attempt_plug_merge();
blk_mq_sched_bio_merge();
__blk_mq_alloc_requests() {
blk_mq_get_tag() {
__blk_mq_get_tag();
}
blk_mq_rq_ctx_init.isra.0();
}
ktime_get();
update_io_ticks();
blk_add_rq_to_plug();
}
}
}
}
bio_alloc_clone() {
bio_alloc_bioset() {
mempool_alloc_noprof() {
__cond_resched();
mempool_alloc_slab() {
kmem_cache_alloc_noprof() {
__slab_alloc.isra.0() {
___slab_alloc();
}
}
}
}
bio_associate_blkg() {
__rcu_read_lock();
kthread_blkcg();
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
}
__rcu_read_unlock();
}
}
bio_clone_blkg_association() {
bio_associate_blkg_from_css() {
__rcu_read_lock();
__rcu_read_unlock();
__rcu_read_lock();
__rcu_read_unlock();
}
}
}
submit_bio_noacct() {
__cond_resched();
submit_bio_noacct_nocheck() {
blk_cgroup_bio_start();
__submit_bio() {
blk_mq_submit_bio() {
__rcu_read_lock();
__rcu_read_unlock();
bio_split_rw() {
bio_split_io_at();
bio_submit_split();
}
blk_attempt_plug_merge();
blk_mq_sched_bio_merge();
__blk_mq_alloc_requests() {
blk_mq_get_tag() {
__blk_mq_get_tag();
}
blk_mq_rq_ctx_init.isra.0();
}
update_io_ticks() {
bdev_count_inflight();
}
blk_add_rq_to_plug();
}
}
}
}
}
>
>> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
>> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
>> ---
>> I sent an email about this issue two days ago, but at the time I was not
>> sure whether it was a real problem or a misunderstanding on my part.
>>
>> After further analysis, it appears that this issue can occur.
>>
>> Apologies for the earlier confusion, and thank you for your time.
>>
>> Abd-Alrhman
>
> I suggest to always share the URL (lore.kernel.org), when referencing
> another thread. If relevant, maybe even reference your message with a
> Link: tag in the commit message.
Yes, i will make sure to do that next time. Here is the link:
https://lore.kernel.org/linux-raid/20260425142938.5555-1-abd.masalkhi@gmail.com/T
>
>> ---
>> drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
>> 1 file changed, 24 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
>> index cc9914bd15c1..14f6d6625811 100644
>> --- a/drivers/md/raid1.c
>> +++ b/drivers/md/raid1.c
>> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>>
>> /* choose the first disk even if it has some bad blocks. */
>> read_len = raid1_check_read_range(rdev, this_sector, &len);
>> - if (read_len > 0) {
>> + if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
>> update_read_sectors(conf, disk, this_sector, read_len);
>> *max_sectors = read_len;
>> return disk;
>> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>> }
>>
>> if (bb_disk != -1) {
>> - *max_sectors = bb_read_len;
>> - update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
>> + if (!*max_sectors || bb_read_len == r1_bio->sectors) {
>> + *max_sectors = bb_read_len;
>> + update_read_sectors(conf, bb_disk, this_sector,
>> + bb_read_len);
>> + } else {
>> + bb_disk = -1;
>> + }
>> }
>>
>> return bb_disk;
>> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
>> * disks and disks with bad blocks for now. Only pay attention to key disk
>> * choice.
>> *
>> - * 3) If we've made it this far, now look for disks with bad blocks and choose
>> - * the one with most number of sectors.
>> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
>> + * of bad blocks), look for disks with bad blocks and choose the one with
>> + * the most sectors.
>> *
>> * 4) If we are all the way at the end, we have no choice but to use a disk even
>> * if it is write mostly.
>> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
>> /*
>> * If we are here it means we didn't find a perfectly good disk so
>> * now spend a bit more time trying to find one with the most good
>> - * sectors.
>> + * sectors. but only if we are tolerant of bad blocks.
>
> s/but/But/
>
I will fix this in v2.
>> */
>> - disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> - if (disk >= 0)
>> - return disk;
>> + if (!*max_sectors) {
>> + disk = choose_bb_rdev(conf, r1_bio, max_sectors);
>> + if (disk >= 0)
>> + return disk;
>> + }
>>
>> return choose_slow_rdev(conf, r1_bio, max_sectors);
>> }
>> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
>> /*
>> * make_request() can abort the operation when read-ahead is being
>> * used and no empty request is available.
>> + *
>> + * If we allow splitting the bio while executing in the raid1 thread,
>> + * we may end up recursing (current->bio_list is NULL), and we might
>> + * also deadlock if we try to suspend the array, since we are
>> + * resubmitting an md_cloned_bio. Therefore, we must be read either
>
> … we must read …
>
I will fix this in v2.
>> + * all the sectors or none.
>> */
>> + max_sectors = r1bio_existed;
>
> Excuse my ignorance, but I do not get why a bool is assigned to an int
> representing the maximum sector value.
>
I modified read_balance() to interpret *max_sectors as a flag. If it is
0, the read path is allowed to be tolerant of bad blocks; otherwise, it
is not. In both cases, *max_sectors will eventually be updated to the
maximum number of readable sectors if a suitable disk is found.
I used r1bio_existed to initialize this value, so assigning
max_sectors = r1bio_existed effectively encodes this behavior
without introducing an additional parameter.
>> rdisk = read_balance(conf, r1_bio, &max_sectors);
>> if (rdisk < 0) {
>> /* couldn't find anywhere to read from */
>
>
> Kind regards,
>
> Paul
--
Best Regards,
Abd-Alrhman
^ permalink raw reply related
* Re: [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Paul Menzel @ 2026-04-27 14:49 UTC (permalink / raw)
To: Abd-Alrhman Masalkhi; +Cc: song, yukuai, shli, neilb, linux-raid, linux-kernel
In-Reply-To: <20260427103446.300378-1-abd.masalkhi@gmail.com>
Dear Abd-Alrhman,
Thank you for your patch.
Am 27.04.26 um 12:34 schrieb Abd-Alrhman Masalkhi:
> Splitting a bio while executing in the raid1 thread can lead to
> recursion, as task->bio_list is NULL in this context.
>
> In addition, resubmitting an md_cloned_bio after splitting may lead to
> a deadlock if the array is suspended before the md driver calls
> percpu_ref_tryget_live(&mddev->active_io) on it's path to
> pers->make_request().
>
> Avoid splitting the bio in this context and require that it is either
> read in full or not at all.
>
> This prevents recursion and avoids potential deadlocks during array
> suspension.
Do you have a reproducer?
> Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
> Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
> ---
> I sent an email about this issue two days ago, but at the time I was not
> sure whether it was a real problem or a misunderstanding on my part.
>
> After further analysis, it appears that this issue can occur.
>
> Apologies for the earlier confusion, and thank you for your time.
>
> Abd-Alrhman
I suggest to always share the URL (lore.kernel.org), when referencing
another thread. If relevant, maybe even reference your message with a
Link: tag in the commit message.
> ---
> drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
> 1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> index cc9914bd15c1..14f6d6625811 100644
> --- a/drivers/md/raid1.c
> +++ b/drivers/md/raid1.c
> @@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
>
> /* choose the first disk even if it has some bad blocks. */
> read_len = raid1_check_read_range(rdev, this_sector, &len);
> - if (read_len > 0) {
> + if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
> update_read_sectors(conf, disk, this_sector, read_len);
> *max_sectors = read_len;
> return disk;
> @@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
> }
>
> if (bb_disk != -1) {
> - *max_sectors = bb_read_len;
> - update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
> + if (!*max_sectors || bb_read_len == r1_bio->sectors) {
> + *max_sectors = bb_read_len;
> + update_read_sectors(conf, bb_disk, this_sector,
> + bb_read_len);
> + } else {
> + bb_disk = -1;
> + }
> }
>
> return bb_disk;
> @@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
> * disks and disks with bad blocks for now. Only pay attention to key disk
> * choice.
> *
> - * 3) If we've made it this far, now look for disks with bad blocks and choose
> - * the one with most number of sectors.
> + * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
> + * of bad blocks), look for disks with bad blocks and choose the one with
> + * the most sectors.
> *
> * 4) If we are all the way at the end, we have no choice but to use a disk even
> * if it is write mostly.
> @@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
> /*
> * If we are here it means we didn't find a perfectly good disk so
> * now spend a bit more time trying to find one with the most good
> - * sectors.
> + * sectors. but only if we are tolerant of bad blocks.
s/but/But/
> */
> - disk = choose_bb_rdev(conf, r1_bio, max_sectors);
> - if (disk >= 0)
> - return disk;
> + if (!*max_sectors) {
> + disk = choose_bb_rdev(conf, r1_bio, max_sectors);
> + if (disk >= 0)
> + return disk;
> + }
>
> return choose_slow_rdev(conf, r1_bio, max_sectors);
> }
> @@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
> /*
> * make_request() can abort the operation when read-ahead is being
> * used and no empty request is available.
> + *
> + * If we allow splitting the bio while executing in the raid1 thread,
> + * we may end up recursing (current->bio_list is NULL), and we might
> + * also deadlock if we try to suspend the array, since we are
> + * resubmitting an md_cloned_bio. Therefore, we must be read either
… we must read …
> + * all the sectors or none.
> */
> + max_sectors = r1bio_existed;
Excuse my ignorance, but I do not get why a bool is assigned to an int
representing the maximum sector value.
> rdisk = read_balance(conf, r1_bio, &max_sectors);
> if (rdisk < 0) {
> /* couldn't find anywhere to read from */
Kind regards,
Paul
^ permalink raw reply
* [PATCH] md/raid1: fix bio splitting in raid1 thread to avoid recursion and deadlock
From: Abd-Alrhman Masalkhi @ 2026-04-27 10:34 UTC (permalink / raw)
To: song, yukuai, shli, neilb, linux-raid, linux-kernel; +Cc: Abd-Alrhman Masalkhi
Splitting a bio while executing in the raid1 thread can lead to
recursion, as task->bio_list is NULL in this context.
In addition, resubmitting an md_cloned_bio after splitting may lead to
a deadlock if the array is suspended before the md driver calls
percpu_ref_tryget_live(&mddev->active_io) on it's path to
pers->make_request().
Avoid splitting the bio in this context and require that it is either
read in full or not at all.
This prevents recursion and avoids potential deadlocks during array
suspension.
Fixes: 689389a06ce7 ("md/raid1: simplify handle_read_error().")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
I sent an email about this issue two days ago, but at the time I was not
sure whether it was a real problem or a misunderstanding on my part.
After further analysis, it appears that this issue can occur.
Apologies for the earlier confusion, and thank you for your time.
Abd-Alrhman
---
drivers/md/raid1.c | 33 ++++++++++++++++++++++++---------
1 file changed, 24 insertions(+), 9 deletions(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index cc9914bd15c1..14f6d6625811 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -607,7 +607,7 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
/* choose the first disk even if it has some bad blocks. */
read_len = raid1_check_read_range(rdev, this_sector, &len);
- if (read_len > 0) {
+ if (read_len > 0 && (!*max_sectors || read_len == r1_bio->sectors)) {
update_read_sectors(conf, disk, this_sector, read_len);
*max_sectors = read_len;
return disk;
@@ -704,8 +704,13 @@ static int choose_slow_rdev(struct r1conf *conf, struct r1bio *r1_bio,
}
if (bb_disk != -1) {
- *max_sectors = bb_read_len;
- update_read_sectors(conf, bb_disk, this_sector, bb_read_len);
+ if (!*max_sectors || bb_read_len == r1_bio->sectors) {
+ *max_sectors = bb_read_len;
+ update_read_sectors(conf, bb_disk, this_sector,
+ bb_read_len);
+ } else {
+ bb_disk = -1;
+ }
}
return bb_disk;
@@ -852,8 +857,9 @@ static int choose_best_rdev(struct r1conf *conf, struct r1bio *r1_bio)
* disks and disks with bad blocks for now. Only pay attention to key disk
* choice.
*
- * 3) If we've made it this far, now look for disks with bad blocks and choose
- * the one with most number of sectors.
+ * 3) If we've made it this far and *max_sectors is 0 (i.e., we are tolerant
+ * of bad blocks), look for disks with bad blocks and choose the one with
+ * the most sectors.
*
* 4) If we are all the way at the end, we have no choice but to use a disk even
* if it is write mostly.
@@ -882,11 +888,13 @@ static int read_balance(struct r1conf *conf, struct r1bio *r1_bio,
/*
* If we are here it means we didn't find a perfectly good disk so
* now spend a bit more time trying to find one with the most good
- * sectors.
+ * sectors. but only if we are tolerant of bad blocks.
*/
- disk = choose_bb_rdev(conf, r1_bio, max_sectors);
- if (disk >= 0)
- return disk;
+ if (!*max_sectors) {
+ disk = choose_bb_rdev(conf, r1_bio, max_sectors);
+ if (disk >= 0)
+ return disk;
+ }
return choose_slow_rdev(conf, r1_bio, max_sectors);
}
@@ -1346,7 +1354,14 @@ static void raid1_read_request(struct mddev *mddev, struct bio *bio,
/*
* make_request() can abort the operation when read-ahead is being
* used and no empty request is available.
+ *
+ * If we allow splitting the bio while executing in the raid1 thread,
+ * we may end up recursing (current->bio_list is NULL), and we might
+ * also deadlock if we try to suspend the array, since we are
+ * resubmitting an md_cloned_bio. Therefore, we must be read either
+ * all the sectors or none.
*/
+ max_sectors = r1bio_existed;
rdisk = read_balance(conf, r1_bio, &max_sectors);
if (rdisk < 0) {
/* couldn't find anywhere to read from */
--
2.43.0
^ permalink raw reply related
* Re: [RFC PATCH 2/2] kernel/module: Decouple klp and ftrace from load_module
From: Song Chen @ 2026-04-26 14:26 UTC (permalink / raw)
To: Masami Hiramatsu (Google), Petr Mladek
Cc: Petr Pavlu, 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, 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: <20260420112707.aa3627ca9f975eeaf7d8ea0e@kernel.org>
Hi,
On 4/20/26 10:27, Masami Hiramatsu (Google) wrote:
> 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,
>
>
my motivation is to decouple ftrace and klp from module loader and make
blocking_notifier_chain more generic, but it doesn't become generic
completely. I understand your and Petr's comments and agree.
Thanks
Best regards
Song
>>
>> Best Regards,
>> Petr
>>
>
>
^ permalink raw reply
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 14:14 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
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: <20260420144429.57b45f2beece690bceea96ec@kernel.org>
Hi Hiramatsu san,
On 4/20/26 13:44, Masami Hiramatsu (Google) wrote:
> 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.
>
Is the new notification callback API going to replace
blocking_notifier_chain in module loader? or an expansion inside
blocking_notifier_chain but introducing less complexity?
>>
>> 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,
>
understood, many thanks for your proposal, i will think about it.
best regards,
Song
^ permalink raw reply
* Re: [RFC PATCH 1/2] kernel/notifier: replace single-linked list with double-linked list for reverse traversal
From: Song Chen @ 2026-04-26 13:56 UTC (permalink / raw)
To: Petr Mladek, Masami Hiramatsu
Cc: chensong_2000, 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, 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: <aec90caYZDHDAHgw@pathway.suse.cz>
Hi,
On 4/21/26 17:05, Petr Mladek wrote:
> On Mon 2026-04-20 14:44:29, Masami Hiramatsu wrote:
>> 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 looks too complex. It would make sense only
> when this API has more users.
>
> Also this won't be enough for the ftrace/livepatch callbacks.
> They need to be ordered against against each other. But they
> also need to be called before/after all other callbacks.
> For example, when the module is loaded:
>
> + 1st frace
> + 2nd livepatch
> + then other notifiers
>
> See the commit c1bf08ac26e92122 ("ftrace: Be first to run code
> modification on modules").
>
>> 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.
>
> I agree. I would keep it as is (hardcoded).
>
> Best Regards,
> Petr
>
Thanks for the feedback, the necessity doesn't convincing enough. I will
try the proposal from Masami Hiramatsu.
Best regards,
Song
^ permalink raw reply
* [PATCH] md/raid1: fix len reuse across rdevs in choose_first_rdev()
From: Abd-Alrhman Masalkhi @ 2026-04-26 9:35 UTC (permalink / raw)
To: song, yukuai, paul.e.luse, xni
Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi
choose_first_rdev() initializes the variable len before iterating over
all rdevs, but passes it by reference to raid1_check_read_range(), which
it might update *len and return 0 depending on the layout of the bad
block region. As a result, 'len' can be modified during the first
iteration and reused for subsequent rdevs, causing later devices to be
evaluated with an incorrect length value.
Fixes: 31a73331752d3 ("md/raid1: factor out read_first_rdev() from read_balance()")
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
drivers/md/raid1.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index b549be9174bb..5f5dbf79c903 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -591,12 +591,12 @@ static int choose_first_rdev(struct r1conf *conf, struct r1bio *r1_bio,
int *max_sectors)
{
sector_t this_sector = r1_bio->sector;
- int len = r1_bio->sectors;
int disk;
for (disk = 0 ; disk < conf->raid_disks * 2 ; disk++) {
struct md_rdev *rdev;
int read_len;
+ int len = r1_bio->sectors;
if (r1_bio->bios[disk] == IO_BLOCKED)
continue;
--
2.43.0
^ permalink raw reply related
* [RFC] md/raid1: possible deadlock introduced in raid1_read_request()
From: Abd-Alrhman Masalkhi @ 2026-04-25 14:29 UTC (permalink / raw)
To: song, yukuai; +Cc: linux-raid, linux-kernel
hi,
The raid1_read_request() function splits a bio regardless of whether it
is an original bio or an md-cloned bio (regardless of the r1bio_existed
value).
When an md_cloned_bio is resubmitted, raid1_read_request() treats it as a
new original bio instead of recognizing it as an md_cloned_bio.
If I understand this correctly, this results in allocating a new r1bio,
etc. More importantly, this may lead to a deadlock if we tried to
suspended the array before the md driver calls percpu_ref_tryget_live()
(&mddev->active_io) on the path down to pers->make_request().
I am considering two possible approaches, ending the bio if max_sectors
is smaller than bio_sectors(bio), or modifing read_balance() to select
a disk that can handle the full bio if r1bio_existed was set.
Is this understanding correct? and What might be the preferred approach
in this case?
Thanks,
Abd-Alrhman
^ permalink raw reply
* Re: [PATCH v2] raid6: arm64: add SVE optimized implementation for syndrome generation
From: Demian Shulhan @ 2026-04-25 11:46 UTC (permalink / raw)
To: Mark Brown
Cc: Ard Biesheuvel, Robin Murphy, Christoph Hellwig, Mark Rutland,
Song Liu, Yu Kuai, Will Deacon, Catalin Marinas, linux-arm-kernel,
Li Nan, linux-raid, linux-kernel
In-Reply-To: <2c2eb793-c0a4-4b65-93f1-0ece5cf820d7@sirena.org.uk>
Hi all!
In the future I will try to look at optimisation of current algorithms
instead of SVE realisation, looks like there are few interesting
cases, I will share details in future changes.
Thanks all for the discussion!
пт, 17 квіт. 2026 р. о 18:36 Mark Brown <broonie@kernel.org> пише:
>
> On Fri, Apr 17, 2026 at 04:43:06PM +0200, Ard Biesheuvel wrote:
>
> > On arm64, kernel mode NEON is mostly used to gain access to AES and SHA
> > instructions, and only to a lesser degree to speed up ordinary
> > arithmetic, and so XOR is somewhat of an outlier here.
>
> > Given that Neoverse V1 apparently already carves up ordinary arithmetic
> > performed on 256-bit vectors and operates on 128 bits at a time, I am
> > rather skeptical that we're likely to see any SVE implementations of the
> > crypto extensions soon that are meaningfully faster, given that these
> > are presumably much costlier to implement in terms of gate count, and
> > therefore likely to be split up even on SVE implementations that can
> > perform ordinary arithmetic on 256+ bit vectors in a single cycle. Note
> > that even the arm64 SIMD accelerated CRC implementations rely heavily on
> > 64x64->128 polynomial multiplication.
>
> I'd not be surprised to see something that delivers useful benefits
> using SVE at some point.
>
> > IOW, before we consider kernel mode SVE, I'd like to see some benchmarks
> > for other algorithms too.
>
> Definitely, it needs a solid win to merge anything. I do want to get
> back to the situation where we've got out of tree infrastructure patches
> so that people working on algorithms have something to base their work
> on (and see the overheads using SVE incurs) but unless theres's a
> practical user they should stay out of tree.
^ permalink raw reply
* [PATCH] md: skip redundant raid_disks update when value is unchanged
From: Abd-Alrhman Masalkhi @ 2026-04-25 8:58 UTC (permalink / raw)
To: song, yukuai; +Cc: linux-raid, linux-kernel, Abd-Alrhman Masalkhi
Calling update_raid_disks() with the same value as the current one
can trigger unnecessary work. For example, RAID1 will reallocate
resources such as the mempool for r1bio.
Signed-off-by: Abd-Alrhman Masalkhi <abd.masalkhi@gmail.com>
---
It returns -EINVAL for the same value. If silent success is preferred
instead, please let me know so I adjust its behavior.
---
drivers/md/md.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 0e55639211f2..cb66c4ebbafa 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4409,9 +4409,12 @@ raid_disks_store(struct mddev *mddev, const char *buf, size_t len)
err = mddev_suspend_and_lock(mddev);
if (err)
return err;
- if (mddev->pers)
- err = update_raid_disks(mddev, n);
- else if (mddev->reshape_position != MaxSector) {
+ if (mddev->pers) {
+ if (n != mddev->raid_disks)
+ err = update_raid_disks(mddev, n);
+ else
+ err = -EINVAL;
+ } else if (mddev->reshape_position != MaxSector) {
struct md_rdev *rdev;
int olddisks = mddev->raid_disks - mddev->delta_disks;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v13 0/3] md/md-bitmap: restore bitmap grow through sysfs
From: Su Yue @ 2026-04-25 8:41 UTC (permalink / raw)
To: Yu Kuai; +Cc: Song Liu, glass.su, Li Nan, Xiao Ni, linux-raid, linux-kernel
In-Reply-To: <20260425024615.1696892-1-yukuai@fnnas.com>
On Sat 25 Apr 2026 at 10:46, Yu Kuai <yukuai@fnnas.com> wrote:
> mdadm --grow adds an internal bitmap by writing bitmap/location
> for an
> array that currently has no bitmap. That requires the bitmap
> directory
> and location attribute to exist before the classic bitmap
> backend is
> created.
>
> This series separates bitmap backend lifetime from bitmap sysfs
> lifetime,
> splits the sysfs layout into common and backend-specific groups,
> and adds
> a small "none" bitmap backend. The none backend keeps
> bitmap/location
> available while no real bitmap is active, and the location store
> path can
> then switch between the none backend and the classic bitmap
> backend
> without tearing down the common bitmap sysfs directory.
>
> Patch 1 factors bitmap creation and destruction into helpers
> that do not
> touch sysfs registration.
>
> Patch 2 splits the classic bitmap sysfs files into a common
> group and an
> internal-bitmap group, and converts bitmap backend operations to
> use a
> sysfs group array.
>
> Patch 3 adds the none backend and uses it to restore mdadm
> --grow bitmap
> addition through bitmap/location.
>
> Changes since v12:
> - Keep the factoring patch focused on no-sysfs bitmap lifetime
> helpers.
> - Make bitmap operation lookup depend only on the current bitmap
> id
> matching the installed backend.
> - Trim the none backend to only the operations required by the
> active
> call paths.
> - Rework bitmap/location error handling with explicit cleanup
> labels.
> - Restore the none backend after bitmap removal and
> creation/load
> failures so bitmap/location stays available.
>
> Validation:
> - create a RAID1 array with --bitmap=none
> - verify /sys/block/md0/md/bitmap/location exists and reports
> "none"
> - mdadm --grow /dev/md0 --bitmap=internal
> - verify location switches to "+8", mdadm reports "Intent
> Bitmap:
> Internal", and /proc/mdstat reports a bitmap
> - mdadm --grow /dev/md0 --bitmap=none
> - verify location switches back to "none" and only the common
> location
> attribute remains under md/bitmap
> - repeat the internal/none switch once more
> - Checked the QEMU serial log for panic, Oops, BUG, WARNING,
> Call Trace,
> RCU stall, and hung-task patterns; none were found.
>
> Yu Kuai (3):
> md: factor bitmap creation away from sysfs handling
> md/md-bitmap: split bitmap sysfs groups
> md/md-bitmap: add a none backend for bitmap grow
>
Thanks for all.
Would you like to add tag Fixes: fb8cc3b0d9db for the whole series
while merging?
--
Su
> drivers/md/md-bitmap.c | 131
> +++++++++++++++++++++++++++++++++++----
> drivers/md/md-bitmap.h | 2 +-
> drivers/md/md-llbitmap.c | 7 ++-
> drivers/md/md.c | 125
> ++++++++++++++++++++++++++-----------
> drivers/md/md.h | 3 +
> 5 files changed, 218 insertions(+), 50 deletions(-)
>
> base-commit: c85d314b135ff569c1031f2ef8e40368bcfe72ac
^ permalink raw reply
* Re: [PATCH v13 3/3] md/md-bitmap: add a none backend for bitmap grow
From: Su Yue @ 2026-04-25 8:39 UTC (permalink / raw)
To: Yu Kuai; +Cc: Song Liu, glass.su, Li Nan, Xiao Ni, linux-raid, linux-kernel
In-Reply-To: <20260425024615.1696892-4-yukuai@fnnas.com>
On Sat 25 Apr 2026 at 10:46, Yu Kuai <yukuai@fnnas.com> wrote:
> Add a real none bitmap backend that exposes the common bitmap
> sysfs
> group and use it to keep bitmap/location available when an array
> has no
> bitmap.
>
> Then switch the bitmap location sysfs path to move only between
> none
> and the classic bitmap backend, using the no-sysfs bitmap
> helpers while
> merging or unmerging the internal bitmap sysfs group.
>
> This restores mdadm --grow bitmap addition through
> bitmap/location.
>
> Signed-off-by: Yu Kuai <yukuai@fnnas.com>
>
Reviewed-by: Su Yue <glass.su@suse.com>
> ---
> drivers/md/md-bitmap.c | 108
> ++++++++++++++++++++++++++++++++++++++---
> drivers/md/md.c | 42 +++++++++++++---
> drivers/md/md.h | 3 ++
> 3 files changed, 137 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/md/md-bitmap.c b/drivers/md/md-bitmap.c
> index eba649703a1c..028b9ca8ce52 100644
> --- a/drivers/md/md-bitmap.c
> +++ b/drivers/md/md-bitmap.c
> @@ -216,6 +216,7 @@ struct bitmap {
> };
>
> static struct workqueue_struct *md_bitmap_wq;
> +static struct attribute_group md_bitmap_internal_group;
>
> static int __bitmap_resize(struct bitmap *bitmap, sector_t
> blocks,
> int chunksize, bool init);
> @@ -2580,6 +2581,30 @@ static int bitmap_resize(struct mddev
> *mddev, sector_t blocks, int chunksize)
> return __bitmap_resize(bitmap, blocks, chunksize, false);
> }
>
> +static bool bitmap_none_enabled(void *data, bool flush)
> +{
> + return false;
> +}
> +
> +static int bitmap_none_create(struct mddev *mddev)
> +{
> + return 0;
> +}
> +
> +static int bitmap_none_load(struct mddev *mddev)
> +{
> + return 0;
> +}
> +
> +static void bitmap_none_destroy(struct mddev *mddev)
> +{
> +}
> +
> +static int bitmap_none_get_stats(void *data, struct
> md_bitmap_stats *stats)
> +{
> + return -ENOENT;
> +}
> +
> static ssize_t
> location_show(struct mddev *mddev, char *page)
> {
> @@ -2618,7 +2643,11 @@ location_store(struct mddev *mddev, const
> char *buf, size_t len)
> goto out;
> }
>
> - bitmap_destroy(mddev);
> + sysfs_unmerge_group(&mddev->kobj,
> &md_bitmap_internal_group);
> + md_bitmap_destroy_nosysfs(mddev);
> + mddev->bitmap_id = ID_BITMAP_NONE;
> + if (!mddev_set_bitmap_ops_nosysfs(mddev))
> + goto none_err;
> mddev->bitmap_info.offset = 0;
> if (mddev->bitmap_info.file) {
> struct file *f = mddev->bitmap_info.file;
> @@ -2654,16 +2683,25 @@ location_store(struct mddev *mddev,
> const char *buf, size_t len)
> }
>
> mddev->bitmap_info.offset = offset;
> - rv = bitmap_create(mddev);
> + md_bitmap_destroy_nosysfs(mddev);
> + mddev->bitmap_id = ID_BITMAP;
> + if (!mddev_set_bitmap_ops_nosysfs(mddev))
> + goto bitmap_err;
> +
> + rv = md_bitmap_create_nosysfs(mddev);
> if (rv)
> - goto out;
> + goto create_err;
>
> - rv = bitmap_load(mddev);
> + rv = mddev->bitmap_ops->load(mddev);
> if (rv) {
> mddev->bitmap_info.offset = 0;
> - bitmap_destroy(mddev);
> - goto out;
> + goto load_err;
> }
> +
> + rv = sysfs_merge_group(&mddev->kobj,
> + &md_bitmap_internal_group);
> + if (rv)
> + goto merge_err;
> }
> }
> if (!mddev->external) {
> @@ -2679,6 +2717,22 @@ location_store(struct mddev *mddev, const
> char *buf, size_t len)
> if (rv)
> return rv;
> return len;
> +
> +merge_err:
> + mddev->bitmap_info.offset = 0;
> +load_err:
> + md_bitmap_destroy_nosysfs(mddev);
> +create_err:
> + mddev->bitmap_info.offset = 0;
> + mddev->bitmap_id = ID_BITMAP_NONE;
> + if (!mddev_set_bitmap_ops_nosysfs(mddev))
> + rv = -ENOENT;
> + goto out;
> +bitmap_err:
> + rv = -ENOENT;
> +none_err:
> + mddev->bitmap_info.offset = 0;
> + goto out;
> }
>
> static struct md_sysfs_entry bitmap_location =
> @@ -2987,6 +3041,27 @@ static const struct attribute_group
> *bitmap_groups[] = {
> NULL,
> };
>
> +static const struct attribute_group *bitmap_none_groups[] = {
> + &md_bitmap_common_group,
> + NULL,
> +};
> +
> +static struct bitmap_operations bitmap_none_ops = {
> + .head = {
> + .type = MD_BITMAP,
> + .id = ID_BITMAP_NONE,
> + .name = "none",
> + },
> +
> + .enabled = bitmap_none_enabled,
> + .create = bitmap_none_create,
> + .load = bitmap_none_load,
> + .destroy = bitmap_none_destroy,
> + .get_stats = bitmap_none_get_stats,
> +
> + .groups = bitmap_none_groups,
> +};
> +
> static struct bitmap_operations bitmap_ops = {
> .head = {
> .type = MD_BITMAP,
> @@ -3033,16 +3108,33 @@ static struct bitmap_operations
> bitmap_ops = {
>
> int md_bitmap_init(void)
> {
> + int err;
> +
> 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);
> + err = register_md_submodule(&bitmap_none_ops.head);
> + if (err)
> + goto err_wq;
> +
> + err = register_md_submodule(&bitmap_ops.head);
> + if (err)
> + goto err_none;
> +
> + return 0;
> +
> +err_none:
> + unregister_md_submodule(&bitmap_none_ops.head);
> +err_wq:
> + destroy_workqueue(md_bitmap_wq);
> + return err;
> }
>
> void md_bitmap_exit(void)
> {
> - destroy_workqueue(md_bitmap_wq);
> unregister_md_submodule(&bitmap_ops.head);
> + unregister_md_submodule(&bitmap_none_ops.head);
> + destroy_workqueue(md_bitmap_wq);
> }
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0ef81d116191..7937b927d923 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -705,7 +705,7 @@ static void md_bitmap_sysfs_del(struct mddev
> *mddev)
> sysfs_remove_group(&mddev->kobj,
> mddev->bitmap_ops->groups[0]);
> }
>
> -static bool mddev_set_bitmap_ops_nosysfs(struct mddev *mddev)
> +bool mddev_set_bitmap_ops_nosysfs(struct mddev *mddev)
> {
> struct md_submodule_head *head;
>
> @@ -4275,7 +4275,7 @@ bitmap_type_show(struct mddev *mddev, char
> *page)
>
> xa_lock(&md_submodule);
> xa_for_each(&md_submodule, i, head) {
> - if (head->type != MD_BITMAP)
> + if (head->type != MD_BITMAP || head->id == ID_BITMAP_NONE)
> continue;
>
> if (mddev->bitmap_id == head->id)
> @@ -6535,7 +6535,7 @@ static enum md_submodule_id
> md_bitmap_get_id_from_sb(struct mddev *mddev)
> return id;
> }
>
> -static int md_bitmap_create_nosysfs(struct mddev *mddev)
> +int md_bitmap_create_nosysfs(struct mddev *mddev)
> {
> enum md_submodule_id orig_id = mddev->bitmap_id;
> enum md_submodule_id sb_id;
> @@ -6544,8 +6544,10 @@ static int
> md_bitmap_create_nosysfs(struct mddev *mddev)
> if (mddev->bitmap_id == ID_BITMAP_NONE)
> return -EINVAL;
>
> - if (!mddev_set_bitmap_ops_nosysfs(mddev))
> + if (!mddev_set_bitmap_ops_nosysfs(mddev)) {
> + mddev->bitmap_id = orig_id;
> return -ENOENT;
> + }
>
> err = mddev->bitmap_ops->create(mddev);
> if (!err)
> @@ -6559,8 +6561,10 @@ static int
> md_bitmap_create_nosysfs(struct mddev *mddev)
> mddev->bitmap_ops = NULL;
>
> sb_id = md_bitmap_get_id_from_sb(mddev);
> - if (sb_id == ID_BITMAP_NONE || sb_id == orig_id)
> + if (sb_id == ID_BITMAP_NONE || sb_id == orig_id) {
> + mddev->bitmap_id = orig_id;
> return err;
> + }
>
> pr_info("md: %s: bitmap version mismatch, switching from %d to
> %d\n",
> mdname(mddev), orig_id, sb_id);
> @@ -6594,7 +6598,7 @@ static int md_bitmap_create(struct mddev
> *mddev)
> return 0;
> }
>
> -static void md_bitmap_destroy_nosysfs(struct mddev *mddev)
> +void md_bitmap_destroy_nosysfs(struct mddev *mddev)
> {
> if (!md_bitmap_registered(mddev))
> return;
> @@ -6612,6 +6616,16 @@ static void md_bitmap_destroy(struct
> mddev *mddev)
> md_bitmap_destroy_nosysfs(mddev);
> }
>
> +static void md_bitmap_set_none(struct mddev *mddev)
> +{
> + mddev->bitmap_id = ID_BITMAP_NONE;
> + if (!mddev_set_bitmap_ops_nosysfs(mddev))
> + return;
> +
> + if (!mddev_is_dm(mddev) && mddev->bitmap_ops->groups)
> + md_bitmap_sysfs_add(mddev);
> +}
> +
> int md_run(struct mddev *mddev)
> {
> int err;
> @@ -6821,6 +6835,10 @@ int md_run(struct mddev *mddev)
> if (mddev->sb_flags)
> md_update_sb(mddev, 0);
>
> + if (IS_ENABLED(CONFIG_MD_BITMAP) && !mddev->bitmap_info.file
> &&
> + !mddev->bitmap_info.offset)
> + md_bitmap_set_none(mddev);
> +
> md_new_event();
> return 0;
>
> @@ -7766,7 +7784,8 @@ static int set_bitmap_file(struct mddev
> *mddev, int fd)
> {
> int err = 0;
>
> - if (!md_bitmap_registered(mddev))
> + if (!md_bitmap_registered(mddev) ||
> + mddev->bitmap_id == ID_BITMAP_NONE)
> return -EINVAL;
>
> if (mddev->pers) {
> @@ -7831,10 +7850,12 @@ static int set_bitmap_file(struct mddev
> *mddev, int fd)
>
> if (err) {
> md_bitmap_destroy(mddev);
> + md_bitmap_set_none(mddev);
> fd = -1;
> }
> } else if (fd < 0) {
> md_bitmap_destroy(mddev);
> + md_bitmap_set_none(mddev);
> }
> }
>
> @@ -8141,12 +8162,16 @@ 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;
> + mddev->bitmap_id = ID_BITMAP;
> rv = md_bitmap_create(mddev);
> if (!rv)
> rv = mddev->bitmap_ops->load(mddev);
>
> - if (rv)
> + if (rv) {
> md_bitmap_destroy(mddev);
> + mddev->bitmap_info.offset = 0;
> + md_bitmap_set_none(mddev);
> + }
> } else {
> struct md_bitmap_stats stats;
>
> @@ -8174,6 +8199,7 @@ static int update_array_info(struct mddev
> *mddev, mdu_array_info_t *info)
> }
> md_bitmap_destroy(mddev);
> mddev->bitmap_info.offset = 0;
> + md_bitmap_set_none(mddev);
> }
> }
> md_update_sb(mddev, 1);
> diff --git a/drivers/md/md.h b/drivers/md/md.h
> index d3d4e2150dc8..52c378086046 100644
> --- a/drivers/md/md.h
> +++ b/drivers/md/md.h
> @@ -934,6 +934,9 @@ extern void md_allow_write(struct mddev
> *mddev);
> extern void md_wait_for_blocked_rdev(struct md_rdev *rdev,
> struct mddev *mddev);
> extern void md_set_array_sectors(struct mddev *mddev, sector_t
> array_sectors);
> extern int md_check_no_bitmap(struct mddev *mddev);
> +bool mddev_set_bitmap_ops_nosysfs(struct mddev *mddev);
> +int md_bitmap_create_nosysfs(struct mddev *mddev);
> +void md_bitmap_destroy_nosysfs(struct mddev *mddev);
> extern int md_integrity_register(struct mddev *mddev);
> extern int strict_strtoul_scaled(const char *cp, unsigned long
> *res, int scale);
^ 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