Re: [PATCH -next v3 3/7] md: delay choosing sync action to md_start_sync()

[Xiao Ni]

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> From: Yu Kuai <yukuai3@xxxxxxxxxx>
>
> Before this patch, for read-write array:
>
> 1) md_check_recover() found that something need to be done, and it'll
>    try to grab 'reconfig_mutex'. The case that md_check_recover() need
>    to do something:
>    - array is not suspend;
>    - super_block need to be updated;
>    - 'MD_RECOVERY_NEEDED' or ''MD_RECOVERY_DONE' is set;
>    - unusual case related to safemode;
>
> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
>    md_check_recover() will try to choose a sync action, and then queue a
>    work md_start_sync().
>
> 3) md_start_sync() register sync_thread;
>
> After this patch,
>
> 1) is the same;
> 2) if 'MD_RECOVERY_RUNNING' is not set, and 'MD_RECOVERY_NEEDED' is set,
>    queue a work md_start_sync() directly;
> 3) md_start_sync() will try to choose a sync action, and then register
>    sync_thread();
>
> Because 'MD_RECOVERY_RUNNING' is cleared when sync_thread is done, 2)
> and 3) and md_do_sync() is always ran in serial and they can never
> concurrent, this change should not introduce any behavior change for now.
>
> Also fix a problem that md_start_sync() can clear 'MD_RECOVERY_RUNNING'
> without protection in error path, which might affect the logical in
> md_check_recovery().
>
> The advantage to change this is that array reconfiguration is
> independent from daemon now, and it'll be much easier to synchronize it
> with io, consider that io may rely on daemon thread to be done.
>
> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> ---
>  drivers/md/md.c | 73 ++++++++++++++++++++++++++-----------------------
>  1 file changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 0cb9fa703a0c..561cac13ff96 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9293,6 +9293,22 @@ static bool md_choose_sync_action(struct mddev *mddev, int *spares)
>  static void md_start_sync(struct work_struct *ws)
>  {
>         struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> +       int spares = 0;
> +
> +       mddev_lock_nointr(mddev);
> +
> +       if (!md_choose_sync_action(mddev, &spares))
> +               goto not_running;
> +
> +       if (!mddev->pers->sync_request)
> +               goto not_running;
> +
> +       /*
> +        * We are adding a device or devices to an array which has the bitmap
> +        * stored on all devices. So make sure all bitmap pages get written.
> +        */
> +       if (spares)
> +               md_bitmap_write_all(mddev->bitmap);
>
>         rcu_assign_pointer(mddev->sync_thread,
>                            md_register_thread(md_do_sync, mddev, "resync"));
> @@ -9300,20 +9316,27 @@ static void md_start_sync(struct work_struct *ws)
>                 pr_warn("%s: could not start resync thread...\n",
>                         mdname(mddev));
>                 /* leave the spares where they are, it shouldn't hurt */
> -               clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> -               clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> -               clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> -               clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> -               clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> -               wake_up(&resync_wait);
> -               if (test_and_clear_bit(MD_RECOVERY_RECOVER,
> -                                      &mddev->recovery))
> -                       if (mddev->sysfs_action)
> -                               sysfs_notify_dirent_safe(mddev->sysfs_action);
> -       } else
> -               md_wakeup_thread(mddev->sync_thread);
> +               goto not_running;
> +       }
> +
> +       mddev_unlock(mddev);
> +       md_wakeup_thread(mddev->sync_thread);
>         sysfs_notify_dirent_safe(mddev->sysfs_action);
>         md_new_event();
> +       return;
> +
> +not_running:
> +       clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> +       clear_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> +       clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> +       clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> +       clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
> +       mddev_unlock(mddev);
> +
> +       wake_up(&resync_wait);
> +       if (test_and_clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery) &&
> +           mddev->sysfs_action)
> +               sysfs_notify_dirent_safe(mddev->sysfs_action);
>  }
>
>  /*
> @@ -9381,7 +9404,6 @@ void md_check_recovery(struct mddev *mddev)
>                 return;
>
>         if (mddev_trylock(mddev)) {
> -               int spares = 0;
>                 bool try_set_sync = mddev->safemode != 0;
>
>                 if (!mddev->external && mddev->safemode == 1)
> @@ -9468,31 +9490,14 @@ void md_check_recovery(struct mddev *mddev)
>                 clear_bit(MD_RECOVERY_INTR, &mddev->recovery);
>                 clear_bit(MD_RECOVERY_DONE, &mddev->recovery);
>
> -               if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
> -                   test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
> -                       goto not_running;
> -               if (!md_choose_sync_action(mddev, &spares))
> -                       goto not_running;
> -               if (mddev->pers->sync_request) {
> -                       if (spares) {
> -                               /* We are adding a device or devices to an array
> -                                * which has the bitmap stored on all devices.
> -                                * So make sure all bitmap pages get written
> -                                */
> -                               md_bitmap_write_all(mddev->bitmap);
> -                       }
> +               if (test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) &&
> +                   !test_bit(MD_RECOVERY_FROZEN, &mddev->recovery)) {
>                         queue_work(md_misc_wq, &mddev->sync_work);
> -                       goto unlock;
> -               }
> -       not_running:
> -               if (!mddev->sync_thread) {
> +               } else {
>                         clear_bit(MD_RECOVERY_RUNNING, &mddev->recovery);
>                         wake_up(&resync_wait);
> -                       if (test_and_clear_bit(MD_RECOVERY_RECOVER,
> -                                              &mddev->recovery))
> -                               if (mddev->sysfs_action)
> -                                       sysfs_notify_dirent_safe(mddev->sysfs_action);
>                 }
> +
>         unlock:
>                 wake_up(&mddev->sb_wait);
>                 mddev_unlock(mddev);
> --
> 2.39.2
>

I like the idea. Thanks for the effort.

Reviewed-by: Xiao Ni <xni@xxxxxxxxxx>