Received-SPF: pass (google.com: domain of linux-kernel+bounces-70202-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240201092559.910982-1-yukuai1@huaweicloud.com> <20240201092559.910982-10-yukuai1@huaweicloud.com>
In-Reply-To: <20240201092559.910982-10-yukuai1@huaweicloud.com>
From: Xiao Ni <xni@redhat.com>
Date: Sun, 18 Feb 2024 12:53:42 +0800
Message-ID: <CALTww2_ppGe29wMOsLS45kR4YS6TyCTBswmeKyVE+-H6XmoN+g@mail.gmail.com>
Subject: Re: [PATCH v5 09/14] dm-raid: really frozen sync_thread during suspend
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, 
	yukuai3@huawei.com, 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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Kuai

On Thu, Feb 1, 2024 at 5:30=E2=80=AFPM Yu Kuai <yukuai1@huaweicloud.com> wr=
ote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> 1) The flag MD_RECOVERY_FROZEN doesn't mean that sync thread is frozen,
>    it only prevent new sync_thread to start, and it can't stop the
>    running sync thread;

Agree with this

> 2) The flag MD_RECOVERY_FROZEN doesn't mean that writes are stopped, use
>    it as condition for md_stop_writes() in raid_postsuspend() doesn't
>    look correct.

I don't agree with it. __md_stop_writes stops sync thread, so it needs
to check this flag. And It looks like the name __md_stop_writes is not
right. Does it really stop write io? mddev_suspend should be the
function that stop write request. From my understanding,
raid_postsuspend does two jobs. One is stopping sync thread. Two is
suspending array.

> 3) raid_message can set/clear the flag MD_RECOVERY_FROZEN at anytime,
>    and if MD_RECOVERY_FROZEN is cleared while the array is suspended,
>    new sync_thread can start unexpected.

md_action_store doesn't check this either. If the array is suspended
and MD_RECOVERY_FROZEN is cleared, before patch01, sync thread can't
happen. So it looks like patch01 breaks the logic.

Regards
Xiao


>
> Fix above problems by using the new helper to suspend the array during
> suspend, also disallow raid_message() to change sync_thread status
> during suspend.
>
> Note that after commit f52f5c71f3d4 ("md: fix stopping sync thread"), the
> test shell/lvconvert-raid-reshape.sh start to hang in stop_sync_thread(),
> and with previous fixes, the test won't hang there anymore, however, the
> test will still fail and complain that ext4 is corrupted. And with this
> patch, the test won't hang due to stop_sync_thread() or fail due to ext4
> is corrupted anymore. However, there is still a deadlock related to
> dm-raid456 that will be fixed in following patches.
>
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Closes: https://lore.kernel.org/all/e5e8afe2-e9a8-49a2-5ab0-958d4065c55e@=
redhat.com/
> Fixes: 1af2048a3e87 ("dm raid: fix deadlock caused by premature md_stop_w=
rites()")
> Fixes: 9dbd1aa3a81c ("dm raid: add reshaping support to the target")
> Fixes: f52f5c71f3d4 ("md: fix stopping sync thread")
> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/dm-raid.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/md/dm-raid.c b/drivers/md/dm-raid.c
> index eb009d6bb03a..5ce3c6020b1b 100644
> --- a/drivers/md/dm-raid.c
> +++ b/drivers/md/dm-raid.c
> @@ -3240,11 +3240,12 @@ static int raid_ctr(struct dm_target *ti, unsigne=
d int argc, char **argv)
>         rs->md.ro =3D 1;
>         rs->md.in_sync =3D 1;
>
> -       /* Keep array frozen until resume. */
> -       set_bit(MD_RECOVERY_FROZEN, &rs->md.recovery);
> -
>         /* Has to be held on running the array */
>         mddev_suspend_and_lock_nointr(&rs->md);
> +
> +       /* Keep array frozen until resume. */
> +       md_frozen_sync_thread(&rs->md);
> +
>         r =3D md_run(&rs->md);
>         rs->md.in_sync =3D 0; /* Assume already marked dirty */
>         if (r) {
> @@ -3722,6 +3723,9 @@ static int raid_message(struct dm_target *ti, unsig=
ned int argc, char **argv,
>         if (!mddev->pers || !mddev->pers->sync_request)
>                 return -EINVAL;
>
> +       if (test_bit(RT_FLAG_RS_SUSPENDED, &rs->runtime_flags))
> +               return -EBUSY;
> +
>         if (!strcasecmp(argv[0], "frozen"))
>                 set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>         else
> @@ -3791,15 +3795,31 @@ static void raid_io_hints(struct dm_target *ti, s=
truct queue_limits *limits)
>         blk_limits_io_opt(limits, chunk_size_bytes * mddev_data_stripes(r=
s));
>  }
>
> +static void raid_presuspend(struct dm_target *ti)
> +{
> +       struct raid_set *rs =3D ti->private;
> +
> +       mddev_lock_nointr(&rs->md);
> +       md_frozen_sync_thread(&rs->md);
> +       mddev_unlock(&rs->md);
> +}
> +
> +static void raid_presuspend_undo(struct dm_target *ti)
> +{
> +       struct raid_set *rs =3D ti->private;
> +
> +       mddev_lock_nointr(&rs->md);
> +       md_unfrozen_sync_thread(&rs->md);
> +       mddev_unlock(&rs->md);
> +}
> +
>  static void raid_postsuspend(struct dm_target *ti)
>  {
>         struct raid_set *rs =3D ti->private;
>
>         if (!test_and_set_bit(RT_FLAG_RS_SUSPENDED, &rs->runtime_flags)) =
{
>                 /* Writes have to be stopped before suspending to avoid d=
eadlocks. */
> -               if (!test_bit(MD_RECOVERY_FROZEN, &rs->md.recovery))
> -                       md_stop_writes(&rs->md);
> -
> +               md_stop_writes(&rs->md);
>                 mddev_suspend(&rs->md, false);
>         }
>  }
> @@ -4012,8 +4032,6 @@ static int raid_preresume(struct dm_target *ti)
>         }
>
>         /* Check for any resize/reshape on @rs and adjust/initiate */
> -       /* Be prepared for mddev_resume() in raid_resume() */
> -       set_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>         if (mddev->recovery_cp && mddev->recovery_cp < MaxSector) {
>                 set_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
>                 mddev->resync_min =3D mddev->recovery_cp;
> @@ -4056,9 +4074,9 @@ static void raid_resume(struct dm_target *ti)
>                         rs_set_capacity(rs);
>
>                 mddev_lock_nointr(mddev);
> -               clear_bit(MD_RECOVERY_FROZEN, &mddev->recovery);
>                 mddev->ro =3D 0;
>                 mddev->in_sync =3D 0;
> +               md_unfrozen_sync_thread(mddev);
>                 mddev_unlock_and_resume(mddev);
>         }
>  }
> @@ -4074,6 +4092,8 @@ static struct target_type raid_target =3D {
>         .message =3D raid_message,
>         .iterate_devices =3D raid_iterate_devices,
>         .io_hints =3D raid_io_hints,
> +       .presuspend =3D raid_presuspend,
> +       .presuspend_undo =3D raid_presuspend_undo,
>         .postsuspend =3D raid_postsuspend,
>         .preresume =3D raid_preresume,
>         .resume =3D raid_resume,
> --
> 2.39.2
>