Date: Mon, 19 Feb 2024 16:19:29 +0800
From: Yu Kuai <yukuai1@...weicloud.com>
To: Xiao Ni <xni@...hat.com>, 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,
 Jonathan Brassow <jbrassow@...hat.com>, "yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v5 01/14] md: don't ignore suspended array in
 md_check_recovery()

Hi,

在 2024/02/19 15:10, Xiao Ni 写道:
> On Sun, Feb 18, 2024 at 4:48 PM Yu Kuai <yukuai1@...weicloud.com> wrote:
>>
>> Hi,
>>
>> 在 2024/02/18 16:07, Xiao Ni 写道:
>>> 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@...weicloud.com> 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.
>>
>> 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
>>
> 
> .
> 

Powered by blists - more mailing lists