Re: [PATCH v5 01/14] md: don't ignore suspended array in md_check_recovery()

[Xiao Ni]

On Sun, Feb 18, 2024 at 4:48 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> 在 2024/02/18 16:07, Xiao Ni 写道:
> > On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> 在 2024/02/18 13:07, Xiao Ni 写道:
> >>> On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloudcom> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> 在 2024/02/18 11:15, Xiao Ni 写道:
> >>>>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> 在 2024/02/18 10:27, Xiao Ni 写道:
> >>>>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> 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.
>
> This patch itself really is quite simple, it fixes problems for md/raid,
> and can be triggered by dm-raid as well. This patch will be needed
> regardless of dm-raid, and it's absolutely not a feature.

Hi Kuai

Yes, this patch is simple. But it changes the original logic. Do we
really need to do this? And as the title of the patch set, it's used
to fix regression problems. We need to avoid much changes, find out
the root cause and fix them. It's better to use another patch set to
do more jobs. For example, allow sync request when array is suspended
(But I don't want to do this change).
>
> For dm-raid, there is no doubt that sync_thread should be stopped before
> suspend, and keep frozen until resume, and this behaviour is not changed

Agree with this
> at all and will never change. Other patches actually tries to gurantee

In fact, we only need to use one line code to do this. We don't need
so many patches. It only needs to set MD_RECOVERY_FROZEN before stop
sync thread.

        set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
        __md_stop_writes(mddev);

> this. If you think this patch can introduce new problems for dm-raid,
> please be more specific.
>
> The problem in dm-raid is that it relies on __md_stop_writes() to stop
> and frozen sync_thread, while it also relies that MD_RECOVERY_FROZEN is
> not set, and this is abuse of MD_RECOVERY_FROZEN. And if you still think
> there are problems with considering of the entire patchset, feel free to
> discuss. :)

In fact, dmraid sets MD_RECOVERY_FROZEN before f52f5c71f3d4 (md: fix
stopping sync thread).  It calls __md_stop_writes and this function
sets MD_RECOVERY_FROZEN. Thanks for your patience :)

Regards
Xiao
>
> Thanks,
> Kuai
>