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

[Yu Kuai]

Hi,

在 2024/02/19 15:10, 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@xxxxxxxxxxxxxxx> 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

Nothing is changed, this patch itself fix a long term regression. And I
already change the title to fix dm-raid and md/raid regressions.

> 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).

Following behaviour is not changed with this patchset:

1. dm-raid should stop and frozen sync_thread during suspend;
2. sync_thread can still runing while md/raid is suspended; And my point
is that if you want to forbit new sync_thread, use MD_REOCVERY_FROZEN
instead of suspended;

> > 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);

I agree this make sense, but as I said in the other thread, this is not
enough.

>          __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 :)

I know that, and f52f5c71f3d4 really should set MD_RECOVERY_FROZEN. But
looks like you want to keep the way it used to be, and you don't want to
fix problems that exist in dm-raid for a long term.

If you send your patches before this, I'll be happy to accept them.
However, I know this patchest might be complicated, but I already did
the hard work, and I think this patchset fix the regressions in a better
way, and I'm trying to let dm-raid and md/raid to manage sync_thread the
same safer way.

So far, I think all problems that you concerned are all fixed with this
patchset, and as I said, I'll be happy to dissuss if you think there are
other problems with this patchset.

Thanks,
Kuai


> Regards
> Xiao
> > Thanks,
> > Kuai
>
> .