Re: [PATCH -next v3 2/7] md: factor out a helper to choose sync action from md_check_recovery()

[Xiao Ni]

On Sun, Aug 20, 2023 at 5:13 PM Yu Kuai <yukuai1@xxxxxxxxxxxxxxx> wrote:
>
> From: Yu Kuai <yukuai3@xxxxxxxxxx>
>
> There are no functional changes, on the one hand make the code cleaner,
> on the other hand prevent following checkpatch error in the next patch to
> delay choosing sync action to md_start_sync().
>
> ERROR: do not use assignment in if condition
> +       } else if ((spares = remove_and_add_spares(mddev, NULL))) {
>
> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> ---
>  drivers/md/md.c | 70 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 90815be1e80f..0cb9fa703a0c 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -9246,6 +9246,50 @@ static int remove_and_add_spares(struct mddev *mddev,
>         return spares;
>  }
>
> +static bool md_choose_sync_action(struct mddev *mddev, int *spares)
> +{
> +       /* Check if reshape is in progress first. */
> +       if (mddev->reshape_position != MaxSector) {
> +               if (mddev->pers->check_reshape == NULL ||
> +                   mddev->pers->check_reshape(mddev) != 0)
> +                       return false;
> +
> +               set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /*
> +        * Remove any failed drives, then add spares if possible. Spares are
> +        * also removed and re-added, to allow the personality to fail the
> +        * re-add.
> +        */
> +       *spares = remove_and_add_spares(mddev, NULL);
> +       if (*spares) {
> +               clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> +
> +               /* Start new recovery. */
> +               set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /* Check if recovery is in progress. */
> +       if (mddev->recovery_cp < MaxSector) {
> +               set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> +               clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> +               return true;
> +       }
> +
> +       /* Delay to choose resync/check/repair in md_do_sync(). */
> +       if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
> +               return true;
> +
> +       /* Nothing to be done */
> +       return false;
> +}
> +
>  static void md_start_sync(struct work_struct *ws)
>  {
>         struct mddev *mddev = container_of(ws, struct mddev, sync_work);
> @@ -9427,32 +9471,8 @@ void md_check_recovery(struct mddev *mddev)
>                 if (!test_and_clear_bit(MD_RECOVERY_NEEDED, &mddev->recovery) ||
>                     test_bit(MD_RECOVERY_FROZEN, &mddev->recovery))
>                         goto not_running;
> -               /* no recovery is running.
> -                * remove any failed drives, then
> -                * add spares if possible.
> -                * Spares are also removed and re-added, to allow
> -                * the personality to fail the re-add.
> -                */
> -
> -               if (mddev->reshape_position != MaxSector) {
> -                       if (mddev->pers->check_reshape == NULL ||
> -                           mddev->pers->check_reshape(mddev) != 0)
> -                               /* Cannot proceed */
> -                               goto not_running;
> -                       set_bit(MD_RECOVERY_RESHAPE, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if ((spares = remove_and_add_spares(mddev, NULL))) {
> -                       clear_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_CHECK, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_REQUESTED, &mddev->recovery);
> -                       set_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if (mddev->recovery_cp < MaxSector) {
> -                       set_bit(MD_RECOVERY_SYNC, &mddev->recovery);
> -                       clear_bit(MD_RECOVERY_RECOVER, &mddev->recovery);
> -               } else if (!test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
> -                       /* nothing to be done ... */
> +               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
> --
> 2.39.2
>

Reviewed-by: Xiao Ni <xni@xxxxxxxxxx>