| Message ID | 20240201092559.910982-2-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | dm-raid/md/raid: fix v6.7 regressions | expand |
On Thu, Feb 1, 2024 at 5:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > From: Yu Kuai <yukuai3@huawei.com> > > mddev_suspend() never stop sync_thread, hence it doesn't make sense to > ignore suspended array in md_check_recovery(), which might cause > sync_thread can't be unregistered. > > After commit f52f5c71f3d4 ("md: fix stopping sync thread"), following > hang can be triggered by test shell/integrity-caching.sh: Hi Kuai After applying this patch, it's still stuck at mddev_suspend. Maybe the deadlock can be fixed by other patches from the patch set. But this patch can't fix this issue. If so, the comment is not right. > > 1) suspend the array: > raid_postsuspend > mddev_suspend > > 2) stop the array: > raid_dtr > md_stop > __md_stop_writes > stop_sync_thread > set_bit(MD_RECOVERY_INTR, &mddev->recovery); > md_wakeup_thread_directly(mddev->sync_thread); > wait_event(..., !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) > > 3) sync thread done: > md_do_sync > set_bit(MD_RECOVERY_DONE, &mddev->recovery); > md_wakeup_thread(mddev->thread); > > 4) daemon thread can't unregister sync thread: > md_check_recovery > if (mddev->suspended) > return; -> return directly > md_read_sync_thread > clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); > -> MD_RECOVERY_RUNNING can't be cleared, hence step 2 hang; I add some debug logs when stopping dmraid with lvremove command. The step you mentioned are sequential but not async. The process is : dev_remove->dm_destroy->__dm_destroy->dm_table_postsuspend_targets(raid_postsuspend) -> dm_table_destroy(raid_dtr). It looks like mddev_suspend is waiting for active_io to be zero. Best Regards Xiao > This problem is not just related to dm-raid, fix it by ignoring > suspended array in md_check_recovery(). And follow up patches will > improve dm-raid better to frozen sync thread during suspend. > > Reported-by: Mikulas Patocka <mpatocka@redhat.com> > Closes: https://lore.kernel.org/all/8fb335e-6d2c-dbb5-d7-ded8db5145a@redhat.com/ > Fixes: 68866e425be2 ("MD: no sync IO while suspended") > Fixes: f52f5c71f3d4 ("md: fix stopping sync thread") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/md/md.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 2266358d8074..07b80278eaa5 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -9469,9 +9469,6 @@ static void md_start_sync(struct work_struct *ws) > */ > void md_check_recovery(struct mddev *mddev) > { > - if (READ_ONCE(mddev->suspended)) > - return; > - > if (mddev->bitmap) > md_bitmap_daemon_work(mddev); > > -- > 2.39.2 >
Hi, 在 2024/02/16 14:58, Xiao Ni 写道: > On Thu, Feb 1, 2024 at 5:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> From: Yu Kuai <yukuai3@huawei.com> >> >> mddev_suspend() never stop sync_thread, hence it doesn't make sense to >> ignore suspended array in md_check_recovery(), which might cause >> sync_thread can't be unregistered. >> >> After commit f52f5c71f3d4 ("md: fix stopping sync thread"), following >> hang can be triggered by test shell/integrity-caching.sh: > > Hi Kuai > > After applying this patch, it's still stuck at mddev_suspend. Maybe > the deadlock can be fixed by other patches from the patch set. But > this patch can't fix this issue. If so, the comment is not right. This patch alone can't fix the problem that mddev_suspend() can stuck thoroughly, patches 1-4 will all be needed. Thanks, Kuai > >> >> 1) suspend the array: >> raid_postsuspend >> mddev_suspend >> >> 2) stop the array: >> raid_dtr >> md_stop >> __md_stop_writes >> stop_sync_thread >> set_bit(MD_RECOVERY_INTR, &mddev->recovery); >> md_wakeup_thread_directly(mddev->sync_thread); >> wait_event(..., !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) >> >> 3) sync thread done: >> md_do_sync >> set_bit(MD_RECOVERY_DONE, &mddev->recovery); >> md_wakeup_thread(mddev->thread); >> >> 4) daemon thread can't unregister sync thread: >> md_check_recovery >> if (mddev->suspended) >> return; -> return directly >> md_read_sync_thread >> clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); >> -> MD_RECOVERY_RUNNING can't be cleared, hence step 2 hang; > > I add some debug logs when stopping dmraid with lvremove command. The > step you mentioned are sequential but not async. The process is : > dev_remove->dm_destroy->__dm_destroy->dm_table_postsuspend_targets(raid_postsuspend) > -> dm_table_destroy(raid_dtr). It looks like mddev_suspend is waiting > for active_io to be zero. > > Best Regards > Xiao > >> This problem is not just related to dm-raid, fix it by ignoring >> suspended array in md_check_recovery(). And follow up patches will >> improve dm-raid better to frozen sync thread during suspend. >> >> Reported-by: Mikulas Patocka <mpatocka@redhat.com> >> Closes: https://lore.kernel.org/all/8fb335e-6d2c-dbb5-d7-ded8db5145a@redhat.com/ >> Fixes: 68866e425be2 ("MD: no sync IO while suspended") >> Fixes: f52f5c71f3d4 ("md: fix stopping sync thread") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> drivers/md/md.c | 3 --- >> 1 file changed, 3 deletions(-) >> >> diff --git a/drivers/md/md.c b/drivers/md/md.c >> index 2266358d8074..07b80278eaa5 100644 >> --- a/drivers/md/md.c >> +++ b/drivers/md/md.c >> @@ -9469,9 +9469,6 @@ static void md_start_sync(struct work_struct *ws) >> */ >> void md_check_recovery(struct mddev *mddev) >> { >> - if (READ_ONCE(mddev->suspended)) >> - return; >> - >> if (mddev->bitmap) >> md_bitmap_daemon_work(mddev); >> >> -- >> 2.39.2 >> > > > . >
On Sun, Feb 18, 2024 at 9:15 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2024/02/16 14:58, Xiao Ni 写道: > > On Thu, Feb 1, 2024 at 5:30 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> > >> From: Yu Kuai <yukuai3@huawei.com> > >> > >> mddev_suspend() never stop sync_thread, hence it doesn't make sense to > >> ignore suspended array in md_check_recovery(), which might cause > >> sync_thread can't be unregistered. > >> > >> After commit f52f5c71f3d4 ("md: fix stopping sync thread"), following > >> hang can be triggered by test shell/integrity-caching.sh: > > > > Hi Kuai > > > > After applying this patch, it's still stuck at mddev_suspend. Maybe > > the deadlock can be fixed by other patches from the patch set. But > > this patch can't fix this issue. If so, the comment is not right. > > This patch alone can't fix the problem that mddev_suspend() can stuck > thoroughly, patches 1-4 will all be needed. > > Thanks, > Kuai > > > > >> > >> 1) suspend the array: > >> raid_postsuspend > >> mddev_suspend > >> > >> 2) stop the array: > >> raid_dtr > >> md_stop > >> __md_stop_writes > >> stop_sync_thread > >> set_bit(MD_RECOVERY_INTR, &mddev->recovery); > >> md_wakeup_thread_directly(mddev->sync_thread); > >> wait_event(..., !test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) > >> > >> 3) sync thread done: > >> md_do_sync > >> set_bit(MD_RECOVERY_DONE, &mddev->recovery); > >> md_wakeup_thread(mddev->thread); > >> > >> 4) daemon thread can't unregister sync thread: > >> md_check_recovery > >> if (mddev->suspended) > >> return; -> return directly > >> md_read_sync_thread > >> clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery); > >> -> MD_RECOVERY_RUNNING can't be cleared, hence step 2 hang; > > > > I add some debug logs when stopping dmraid with lvremove command. The > > step you mentioned are sequential but not async. The process is : > > dev_remove->dm_destroy->__dm_destroy->dm_table_postsuspend_targets(raid_postsuspend) > > -> dm_table_destroy(raid_dtr). It looks like mddev_suspend is waiting > > for active_io to be zero. The deadlock problem mentioned in this patch should not be right? Regards Xiao > > > > Best Regards > > Xiao > > > >> This problem is not just related to dm-raid, fix it by ignoring > >> suspended array in md_check_recovery(). And follow up patches will > >> improve dm-raid better to frozen sync thread during suspend. > >> > >> Reported-by: Mikulas Patocka <mpatocka@redhat.com> > >> Closes: https://lore.kernel.org/all/8fb335e-6d2c-dbb5-d7-ded8db5145a@redhat.com/ > >> Fixes: 68866e425be2 ("MD: no sync IO while suspended") > >> Fixes: f52f5c71f3d4 ("md: fix stopping sync thread") > >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> > >> --- > >> drivers/md/md.c | 3 --- > >> 1 file changed, 3 deletions(-) > >> > >> diff --git a/drivers/md/md.c b/drivers/md/md.c > >> index 2266358d8074..07b80278eaa5 100644 > >> --- a/drivers/md/md.c > >> +++ b/drivers/md/md.c > >> @@ -9469,9 +9469,6 @@ static void md_start_sync(struct work_struct *ws) > >> */ > >> void md_check_recovery(struct mddev *mddev) > >> { > >> - if (READ_ONCE(mddev->suspended)) > >> - return; > >> - > >> if (mddev->bitmap) > >> md_bitmap_daemon_work(mddev); > >> > >> -- > >> 2.39.2 > >> > > > > > > . > > >
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.
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.
Thanks,
Kuai
On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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. > > 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 >
Hi, 在 2024/02/18 10:27, Xiao Ni 写道: > On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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 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 >> > > . >
On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2024/02/18 10:27, Xiao Ni 写道: > > On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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: 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 > >> > > > > . > > >
Hi, 在 2024/02/18 11:15, Xiao Ni 写道: > On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> Hi, >> >> 在 2024/02/18 10:27, Xiao Ni 写道: >>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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. 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 >>>> >>> >>> . >>> >> > > . >
On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2024/02/18 11:15, Xiao Ni 写道: > > On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> > >> Hi, > >> > >> 在 2024/02/18 10:27, Xiao Ni 写道: > >>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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? And patch01 is breaking one logic which seems right: commit 68866e425be2ef2664aa5c691bb3ab789736acf5 Author: Jonathan Brassow <jbrassow@f14.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. 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 > >>>> > >>> > >>> . > >>> > >> > > > > . > > >
Hi, 在 2024/02/18 13:07, Xiao Ni 写道: > On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> Hi, >> >> 在 2024/02/18 11:15, Xiao Ni 写道: >>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>> >>>> Hi, >>>> >>>> 在 2024/02/18 10:27, Xiao Ni 写道: >>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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. > > And patch01 is breaking one logic which seems right: > > commit 68866e425be2ef2664aa5c691bb3ab789736acf5 > Author: Jonathan Brassow <jbrassow@f14.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. - 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 >>>>>> >>>>> >>>>> . >>>>> >>>> >>> >>> . >>> >> > > . >
On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2024/02/18 13:07, Xiao Ni 写道: > > On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> > >> Hi, > >> > >> 在 2024/02/18 11:15, Xiao Ni 写道: > >>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>> 在 2024/02/18 10:27, Xiao Ni 写道: > >>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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@f14.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 > >>>>>> > >>>>> > >>>>> . > >>>>> > >>>> > >>> > >>> . > >>> > >> > > > > . > > >
Hi, 在 2024/02/18 16:07, Xiao Ni 写道: > On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> Hi, >> >> 在 2024/02/18 13:07, Xiao Ni 写道: >>> On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>> >>>> Hi, >>>> >>>> 在 2024/02/18 11:15, Xiao Ni 写道: >>>>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> 在 2024/02/18 10:27, Xiao Ni 写道: >>>>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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. 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 at all and will never change. Other patches actually tries to gurantee 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. :) Thanks, Kuai
On Sun, Feb 18, 2024 at 4:48 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > > Hi, > > 在 2024/02/18 16:07, Xiao Ni 写道: > > On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >> > >> Hi, > >> > >> 在 2024/02/18 13:07, Xiao Ni 写道: > >>> On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >>>> > >>>> Hi, > >>>> > >>>> 在 2024/02/18 11:15, Xiao Ni 写道: > >>>>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> 在 2024/02/18 10:27, Xiao Ni 写道: > >>>>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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 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 >
Hi, 在 2024/02/19 15:10, Xiao Ni 写道: > On Sun, Feb 18, 2024 at 4:48 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: >> >> Hi, >> >> 在 2024/02/18 16:07, Xiao Ni 写道: >>> On Sun, Feb 18, 2024 at 2:22 PM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>> >>>> Hi, >>>> >>>> 在 2024/02/18 13:07, Xiao Ni 写道: >>>>> On Sun, Feb 18, 2024 at 11:24 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>>>> >>>>>> Hi, >>>>>> >>>>>> 在 2024/02/18 11:15, Xiao Ni 写道: >>>>>>> On Sun, Feb 18, 2024 at 10:34 AM Yu Kuai <yukuai1@huaweicloud.com> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> 在 2024/02/18 10:27, Xiao Ni 写道: >>>>>>>>> On Sun, Feb 18, 2024 at 9:46 AM Yu Kuai <yukuai1@huaweicloud.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 >> > > . >
diff --git a/drivers/md/md.c b/drivers/md/md.c index 2266358d8074..07b80278eaa5 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9469,9 +9469,6 @@ static void md_start_sync(struct work_struct *ws) */ void md_check_recovery(struct mddev *mddev) { - if (READ_ONCE(mddev->suspended)) - return; - if (mddev->bitmap) md_bitmap_daemon_work(mddev);