Re: [PATCH v3 2/4] md: Handle pers->run failure in level_store

[Bruce Johnston <bjohnsto@redhat.com>]

One of the things I have been looking at is how dm-raid does takeover.
As you might expect, it behaves much differently than how md.c does
takeover. In the case of dm-raid, it basically builds up a brand new
array from scratch that is read_only and frozen. This is all done in the
raid_ctr function in dm-raid.c. Then when you want to swap, you
basically suspend the old array, do the appropriate swap code and then
turn the new array to read write and unfreeze the threads.
pers->takeover is never called in this scenario as far as I can tell,
but pers->run is through a call to md_run, but because the threads are
frozen, no I/O happens until the swap is done.  

I'm not saying this is the correct model to follow here, but
it does possibly suggest we might be able to do something similar in
terms of sharing resources.  

I know this is not as easy problem to solve and any fix may require
extensive changes, but I feel like we should try to solve this now
before things become more complicated. 

On Thu, Sep 24, 2026 at 07:53:50PM +0800, Zhihao Cheng wrote:
> 在 2026/9/24 2:42, Bruce Johnston 写道:
> Hi Bruce,
> > Hi. I've been looking into takeover for a little while as well. I was
> > planning to write up something to post into the mailing list about it
> > but say your submission and figured I would mention it here. I believe
> > just handling a failure in pers->run is insufficient to fixing takeover
> > errors.
> > 
> > IMO the contract for level_store should be that if something fails, we
> > end up where we were before the function was called. So if we try to go
> > from 1->5 and something fails, we are back at a working valid level 1.
> Indeed, this is the most correct implementation for users, the level_store
> operation should be atomic, and my patch is just a simple workaround
> implementation.
> > 
> > The current level_store function roughly does the following:
> > 1. Suspend & Validate: Suspends the mddev device and performs initial
> >     validation checks.
> > 2. Takeover: Invokes pers->takeover().
> > 3. Swap Personality: Swaps the array to the new RAID personality.
> > 4. Free Old State: Frees the old personality configuration and internal
> >     information.
> > 5. Update sysfs fields from old to new
> > 6. Run Personality: Calls pers->run to initialize the new RAID
> >     personality.
> > 7. Resume Device: Resumes the mddev device.
> > 
> > By the time we get to run, we have swapped and free'd the old
> > personality.
> > 
> > The code in md_run around pers->run cleanup is reasonable because if
> > something fails, the array isn't started. You can't really do anything
> > after that. But in takeover, on a failed run, we're now in a state where
> > we have a half swapped personality. How do we recover from that? Are we
> > just going to stop the array?
> > 
> > There are also additional issues in level_store where the personality
> > functions like pers->takeover do things like remove unsupported flags
> > from the mddev based on the new personality but never repair them on
> > failures.
> > 
> > I've been investigating possible takeover failure solutions but none of
> > them are easy. For example, we could try creating a new mddev at the
> > start of level_store and using that for the pers->foo calls but there
> > are a lot of shared resources so it becomes tricky.
> 
> Separating shared resources (eg. private, thread, bitmap, all_mddevs, etc.)
> is a trivial and error-prone task, and we need to have a clear understanding
> of the lifecycle of these shared resources.
> 
> > We could try
> > something like an undo log (each change tell how to undo itself) but
> > that would need to be passed into the personality functions or attached
> > to the mddev.
> 
> The undo log solution will also bring significant changes, and maintaining
> the newly added level_store process in the future will be relatively
> difficult.
> 
> So far, I cannot find a more suitable solution.
> > 
> > Should we have a discussion on this issue? I feel like we have
> > some holes here that need fixing.
> > 
> > On Wed, Sep 23, 2026 at 07:21:56PM +0800, Zhihao Cheng wrote:
> > > Set 'raid_disks' after raid5_run() failure will trigger an
> > > null-ptr-deref problem:
> > >   BUG: kernel NULL pointer dereference, address: 0000000000000038
> > >   RIP: 0010:raid5_check_reshape+0xad
> > >   Call Trace:
> > >    update_raid_disks+0x124
> > >    raid_disks_store+0x145
> > >    md_attr_store+0xd7
> > >    sysfs_kf_write+0x7c
> > > 
> > > The trigger process is simple:
> > >   mdadm --create /dev/md0 --level=1 --raid-devices=2 /dev/sda /dev/sdb
> > >   --force --assume-clean # create raid1
> > >   echo 5 > /sys/block/md0/md/level
> > >    level_store
> > >     mddev->pers = pers
> > >     mddev->private = priv
> > >     raid5_run
> > >      fail to abort (eg. raid5_create_ctx_pool fails)
> > >      mddev->private = NULL
> > >   echo 10 > /sys/block/md0/md/raid_disks
> > >    raid_disks_store
> > >     if (mddev->pers) // true
> > >      update_raid_disks
> > >       raid5_check_reshape
> > >        conf = mddev->private
> > >         conf->algorithm = mddev->new_layout // null-ptr-deref !
> > > 
> > > Similar process exists in do_md_stop->__md_stop_writes->raid5_quiesce.
> > > Similar process exists in raid10 too.
> > > 
> > > Fix it by handling the error from pers->run, next active-type order will
> > > restart the mddev.
> > > 
> > > Fixes: 245f46c2c221e ("md: add ->takeover method to support changing the personality managing an array")
> > > Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> > > ---
> > >   drivers/md/md.c | 29 ++++++++++++++++++++++++++++-
> > >   1 file changed, 28 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/md/md.c b/drivers/md/md.c
> > > index 02798f2dbf0e..ff78bf8656ff 100644
> > > --- a/drivers/md/md.c
> > > +++ b/drivers/md/md.c
> > > @@ -97,6 +97,7 @@ static struct workqueue_struct *md_misc_wq;
> > >   static int remove_and_add_spares(struct mddev *mddev,
> > >   				 struct md_rdev *this);
> > >   static void mddev_detach(struct mddev *mddev);
> > > +static void __md_stop(struct mddev *mddev);
> > >   static void export_rdev(struct md_rdev *rdev);
> > >   static void md_wakeup_thread_directly(struct md_thread __rcu **thread);
> > > @@ -4243,7 +4244,33 @@ level_store(struct mddev *mddev, const char *buf, size_t len)
> > >   		mddev->in_sync = 1;
> > >   		timer_delete_sync(&mddev->safemode_timer);
> > >   	}
> > > -	pers->run(mddev);
> > > +	rv = pers->run(mddev);
> > > +	if (rv) {
> > > +		/*
> > > +		 * ->run() has released the private data of the new personality,
> > > +		 * while the old one has already been released as well. There is
> > > +		 * nothing to fall back to, so stop the array to avoid leaving
> > > +		 * 'mddev->pers' pointing to a personality which has no private
> > > +		 * data, and reminds user to try to active the mddev again.
> > > +		 */
> > > +		pr_warn("md: %s: failed to run %s after takeover, please try to active\n",
> > > +			mdname(mddev), pers->head.name);
> > > +		if (mddev->pers->sync_request && mddev->to_remove == NULL)
> > > +			mddev->to_remove = &md_redundancy_group;
> > > +		if (md_bitmap_enabled(mddev, true))
> > > +			mddev->bitmap_ops->flush(mddev);
> > > +		clear_bit(MD_SERIALIZE_POLICY, &mddev->flags);
> > > +		mddev_destroy_serial_pool(mddev, NULL);
> > > +		__md_stop(mddev);
> > > +		rdev_for_each(rdev, mddev)
> > > +			if (rdev->raid_disk >= 0)
> > > +				sysfs_unlink_rdev(mddev, rdev);
> > > +		set_capacity_and_notify(mddev->gendisk, 0);
> > > +		mddev->changed = 1;
> > > +		md_new_event();
> > > +		sysfs_notify_dirent_safe(mddev->sysfs_state);
> > > +		goto out_unlock;
> > > +	}
> > >   	set_bit(MD_SB_CHANGE_DEVS, &mddev->sb_flags);
> > >   	if (!mddev->thread)
> > >   		md_update_sb(mddev, 1);
> > > -- 
> > > 2.52.0
> > > 
> > > 
> > 
> > 
> > .
> > 
>