Received-SPF: pass (google.com: domain of linux-kernel+bounces-43999-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: <20231228125553.2697765-1-yukuai1@huaweicloud.com> <20231228125553.2697765-4-yukuai1@huaweicloud.com>
In-Reply-To: <20231228125553.2697765-4-yukuai1@huaweicloud.com>
From: Song Liu <song@kernel.org>
Date: Mon, 29 Jan 2024 22:37:01 -0800
Message-ID: <CAPhsuW5ck33wdFznkpXzZmyW3ux3gCf-yhQnevdirjVJkmzmEA@mail.gmail.com>
Subject: Re: [PATCH -next 3/3] md: use interruptible apis in idle/frozen_sync_thread()
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: yukuai3@huawei.com, neilb@suse.de, linux-raid@vger.kernel.org, 
	linux-kernel@vger.kernel.org, yi.zhang@huawei.com, yangerkun@huawei.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

Sorry for the late reply.

The first two patches of the set look good, so I applied them to
md-tmp-6.9 branch. However, this one needs a respin.

On Thu, Dec 28, 2023 at 4:58=E2=80=AFAM Yu Kuai <yukuai1@huaweicloud.com> w=
rote:
>
> From: Yu Kuai <yukuai3@huawei.com>
>
> Before refactoring idle and frozen from action_store, interruptible apis
> is used so that hungtask warning won't be triggered if it takes too long
> to finish idle/frozen sync_thread. So change to use interruptible apis.

This paragraph is confusing. Please rephrase it.

>
> In order not to make stop_sync_thread() more complicated, factor out a
> helper prepare_to_stop_sync_thread() to replace stop_sync_thread().
>
> Also return error to user if idle/frozen_sync_thread() failed, otherwise
> user will be misleaded.

s/misleaded/misled/

>
> Fixes: 130443d60b1b ("md: refactor idle/frozen_sync_thread() to fix deadl=
ock")
> Fixes: 8e8e2518fcec ("md: Close race when setting 'action' to 'idle'.")

Please add more information about what is being fixed here, so that
we can make a clear decision on whether the fix needs to be back
ported to stable kernels.

> Signed-off-by: Yu Kuai <yukuai3@huawei.com>
> ---
>  drivers/md/md.c | 105 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 67 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 60f99768a1a9..9ea05de79fe4 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -4846,26 +4846,34 @@ action_show(struct mddev *mddev, char *page)
>         return sprintf(page, "%s\n", type);
>  }
>
> +static bool sync_thread_stopped(struct mddev *mddev, int *sync_seq)

I think we need a comment for this.

> +{
> +       if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery))
> +               return true;
> +
> +       if (sync_seq && *sync_seq !=3D atomic_read(&mddev->sync_seq))
> +               return true;
> +
> +       return false;
> +}
> +
>  /**
> - * stop_sync_thread() - wait for sync_thread to stop if it's running.
> + * prepare_to_stop_sync_thread() - prepare to stop sync_thread if it's r=
unning.
>   * @mddev:     the array.
> - * @locked:    if set, reconfig_mutex will still be held after this func=
tion
> - *             return; if not set, reconfig_mutex will be released after=
 this
> - *             function return.
> - * @check_seq: if set, only wait for curent running sync_thread to stop,=
 noted
> - *             that new sync_thread can still start.
> + * @unlock:    whether or not caller want to release reconfig_mutex if
> + *             sync_thread is not running.
> + *
> + * Return true if sync_thread is running, release reconfig_mutex and do
> + * preparatory work to stop sync_thread, caller should wait for
> + * sync_thread_stopped() to return true. Return false if sync_thread is =
not
> + * running, reconfig_mutex will be released if @unlock is set.
>   */

I found prepare_to_stop_sync_thread very hard to reason. Please try to
rephrase the comment or refactor the code. Maybe it makes sense to put
the following logic and its variations to a separate function:

        if (prepare_to_stop_sync_thread(mddev, false)) {
                wait_event(resync_wait, sync_thread_stopped(mddev, NULL));
                mddev_lock_nointr(mddev);
        }

Thanks,
Song

> -static void stop_sync_thread(struct mddev *mddev, bool locked, bool chec=
k_seq)
> +static bool prepare_to_stop_sync_thread(struct mddev *mddev, bool unlock=
)
>  {
> -       int sync_seq;

[...]