Message-ID: <CALTww2-_uvkB7M=_J_6DgW1kfzW2rpQgp0vyKt7EYvON41adGw@mail.gmail.com>
Date: Sun, 18 Feb 2024 16:07:02 +0800
From: Xiao Ni <xni@...hat.com>
To: Yu Kuai <yukuai1@...weicloud.com>
Cc: mpatocka@...hat.com, heinzm@...hat.com, blazej.kucman@...ux.intel.com, 
	agk@...hat.com, snitzer@...nel.org, dm-devel@...ts.linux.dev, song@...nel.org, 
	neilb@...e.de, shli@...com, akpm@...l.org, linux-kernel@...r.kernel.org, 
	linux-raid@...r.kernel.org, yi.zhang@...wei.com, yangerkun@...wei.com, 
	"yukuai (C)" <yukuai3@...wei.com>, Jonathan Brassow <jbrassow@...hat.com>
Subject: Re: [PATCH v5 01/14] md: don't ignore suspended array in md_check_recovery()

On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@...weicloud.com> wrote:
>
> Hi,
>
> 在 2024/02/18 13:07, Xiao Ni 写道:
> > On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@...weicloud.com> wrote:
> >>
> >> Hi,
> >>
> >> 在 2024/02/18 11:15, Xiao Ni 写道:
> >>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@...weicloudcom> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> 在 2024/02/18 10:27, Xiao Ni 写道:
> >>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@...weicloud.com> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> 在 2024/02/18 9:33, Xiao Ni 写道:
> >>>>>>> The deadlock problem mentioned in this patch should not be right?
> >>>>>>
> >>>>>> No, I think it's right. Looks like you are expecting other problems,
> >>>>>> like mentioned in patch 6, to be fixed by this patch.
> >>>>>
> >>>>> Hi Kuai
> >>>>>
> >>>>> Could you explain why step1 and step2 from this comment can happen
> >>>>> simultaneously? From the log, the process should be
> >>>>> The process is :
> >>>>> dev_remove->dm_destroy->__dm_destroy->dm_table_postsuspend_targets(raid_postsuspend)
> >>>>> -> dm_table_destroy(raid_dtr).
> >>>>> After suspending the array, it calls raid_dtr. So these two functions
> >>>>> can't happen simultaneously.
> >>>>
> >>>> You're removing the target directly, however, dm can suspend the disk
> >>>> directly, you can simplily:
> >>>>
> >>>> 1) dmsetup suspend xxx
> >>>> 2) dmsetup remove xxx
> >>>
> >>> For dm-raid, the design of suspend stops sync thread first and then it
> >>> calls mddev_suspend to suspend array. So I'm curious why the sync
> >>> thread can still exit when array is suspended. I know the reason now.
> >>> Because before f52f5c71f (md: fix stopping sync thread), the process
> >>> is raid_postsuspend->md_stop_writes->__md_stop_writes
> >>> (__md_stop_writes sets MD_RECOVERY_FROZEN). In patch f52f5c71f, it
> >>> doesn't set MD_RECOVERY_FROZEN in __md_stop_writes anymore.
> >>>
> >>> The process changes to
> >>> 1. raid_postsuspend->md_stop_writes->__md_stop_writes->stop_sync_thread
> >>> (wait until MD_RECOVERY_RUNNING clears)
> >>> 2. md thread -> md_check_recovery -> unregister_sync_thread ->
> >>> md_reap_sync_thread (clears MD_RECOVERY_RUNNING, stop_sync_thread
> >>> returns, md_reap_sync_thread sets MD_RECOVERY_NEEDED)
> >>> 3. raid_postsuspend->mddev_suspend
> >>> 4. md sync thread starts again because __md_stop_writes doesn't set
> >>> MD_RECOVERY_FROZEN.
> >>> It's the reason why we can see sync thread still happens when raid is suspended.
> >>>
> >>> So the patch fix this problem should:
> >>
> >> As I said, this is really a different problem from this patch, and it is
> >> fixed seperately by patch 9. Please take a look at that patch.
> >
> > I think we're talking about the same problem. In patch07 it has a new
> > api md_frozen_sync_thread. It sets MD_RECOVERY_FROZEN before
> > stop_sync_thread. This is right. If we use this api in
> > raid_postsuspend, sync thread can't restart. So the deadlock can't
> > happen anymore?
>
> We are not talking about the same problem at all. This patch just fix a
> simple problem in md/raid(not dm-raid). And the deadlock can also be
> triggered for md/raid the same.
>
> - mddev_suspend() doesn't handle sync_thread at all;
> - md_check_recovery() ignore suspended array;
>
> Please keep in mind this patch just fix the above case. The deadlock in
> dm-raid is just an example of problems caused by this. Fix the deadlock
> other way doesn't mean this case is fine.

Because this patch set is used to fix dm raid deadlocks. But this
patch changes logic, it looks like more a feature - "we can start/stop
sync thread when array is suspended". Because this patch is added many
years ago and dm raid works well. If we change this, there is
possibilities to introduce new problems. Now we should try to walk
slowly.

And is it a deadlock? After resume, the sync thread can be
started/stopped again. Could you explain the deadlock more?

>
> >
> > And patch01 is breaking one logic which seems right:
> >
> > commit 68866e425be2ef2664aa5c691bb3ab789736acf5
> > Author: Jonathan Brassow <jbrassow@....redhat.com>
> > Date:   Wed Jun 8 15:10:08 2011 +1000
> >
> >      MD: no sync IO while suspended
> >
> >      Disallow resync I/O while the RAID array is suspended.
> >
> > We're trying to fix deadlock problems. But it's not good to fix a
> > problem by breaking an existing rule.
>
> The existing rule itself is problematic. Above patch doesn't do well.
>
> It's just a simple problem here, should sync thread also stop in
> mddev_suspend? If you want do do this, you can submit a patch, in the
> right way, we'll see how this will work.

I don't want to change the logic of mddev_suspend. mddev_suspend is
only used to suspend array. Cc Jon who is the author of this patch.
>
> - keep this patch to remove checking of suspended array;
> - set MD_RECOVERY_FORZEN and stop sync thread in mddev_suspend,
> 'reconfig_mutex' will be needed again, and lots of callers need to be
> checked.
>
> Thanks,
> Kuai
>
> >
> > Regards
> > Xiao
> >
> >
> >>
> >> Thanks,
> >> Kuai
> >>
> >>>
> >>> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >>> index 9e41a9aaba8b..666761466f02 100644
> >>> --- a/drivers/md/md.c
> >>> +++ b/drivers/md/md.c
> >>> @@ -6315,6 +6315,7 @@ static void md_clean(struct mddev *mddev)
> >>>
> >>>    static void __md_stop_writes(struct mddev *mddev)
> >>>    {
> >>> +       set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
> >>>           stop_sync_thread(mddev, true, false);
> >>>           del_timer_sync(&mddev->safemode_timer);
> >>>
> >>> Like other places which call stop_sync_thread, it needs to set the
> >>> MD_RECOVERY_FROZEN bit.
> >>>
> >>> Regards
> >>> Xiao
> >>>
> >>>>
> >>>> Please also take a look at other patches, why step 1) can't stop sync
> >>>> thread.
> >>>>
> >>>> Thanks,
> >>>> Kuai
> >>>>
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Noted that this patch just fix one case that MD_RECOVERY_RUNNING can't
> >>>>>> be cleared, I you are testing this patch alone, please make sure that
> >>>>>> you still triggered the exactly same case:
> >>>>>>
> >>>>>> - MD_RCOVERY_RUNNING can't be cleared while array is suspended.
> >>>>>
> >>>>> I'm not testing this patch. I want to understand the patch well. So I
> >>>>> need to understand the issue first. I can't understand how this
> >>>>> deadlock (step1,step2) happens.
> >>>>>
> >>>>> Regards
> >>>>> Xiao
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Kuai
> >>>>>>
> >>>>>
> >>>>> .
> >>>>>
> >>>>
> >>>
> >>> .
> >>>
> >>
> >
> > .
> >
>

Powered by blists - more mailing lists