Received-SPF: pass (google.com: domain of linux-kernel+bounces-70173-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240201092559.910982-1-yukuai1@huaweicloud.com>
 <20240201092559.910982-2-yukuai1@huaweicloud.com> <CALTww2-ZhRBJOD3jXs=xKFaD=iR=dtoC9h2rUQi5Stpi+tJ9Bw@mail.gmail.com>
 <64d27757-9387-09dc-48e8-a9eedd67f075@huaweicloud.com> <CALTww28E=k6fXJURG77KwHb7M2OByLrcE8g7GNkQDTtcOV48hQ@mail.gmail.com>
 <d4a2689e-b5cc-f268-9fb2-84c10e5eb0f4@huaweicloud.com> <CALTww28bUzmQASme3XOz0CY=o86f1EUU23ENmnf42UVyuGzQ4Q@mail.gmail.com>
 <c1195efd-dd83-317e-3067-cd4891ae013e@huaweicloud.com>
In-Reply-To: <c1195efd-dd83-317e-3067-cd4891ae013e@huaweicloud.com>
From: Xiao Ni <xni@redhat.com>
Date: Sun, 18 Feb 2024 11:15:03 +0800
Message-ID: <CALTww2-7tTMdf_XZ60pNKH_QCq3OUX2P==VPXZo3f-dHzVhmnw@mail.gmail.com>
Subject: Re: [PATCH v5 01/14] md: don't ignore suspended array in md_check_recovery()
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: mpatocka@redhat.com, heinzm@redhat.com, blazej.kucman@linux.intel.com, 
	agk@redhat.com, snitzer@kernel.org, dm-devel@lists.linux.dev, song@kernel.org, 
	jbrassow@f14.redhat.com, neilb@suse.de, shli@fb.com, akpm@osdl.org, 
	linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org, yi.zhang@huawei.com, 
	yangerkun@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sun, Feb 18, 2024 at 10:34=E2=80=AFAM Yu Kuai <yukuai1@huaweicloud.com> =
wrote:
>
> Hi,
>
> =E5=9C=A8 2024/02/18 10:27, Xiao Ni =E5=86=99=E9=81=93:
> > On Sun, Feb 18, 2024 at 9:46=E2=80=AFAM Yu Kuai <yukuai1@huaweicloud.co=
m> wrote:
> >>
> >> Hi,
> >>
> >> =E5=9C=A8 2024/02/18 9:33, Xiao Ni =E5=86=99=E9=81=93:
> >>> 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 suspe=
nded.

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