[dm-devel] [PATCH v2 03/11] multipathd: avoid busy loop in child() for delayed reconfigure

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck at suse.com> wrote:
>
> From: Martin Wilck <mwilck at suse.com>
>
> If need_to_delay_reconfig() returned true, the logic introduced
> by 250708c ("multipathd: improve delayed reconfigure") and
> 4b104e6 ("multipathd: pass in the type of reconfigure") could cause
> child() to run in a tight loop, switching back and forth between
> DAEMON_CONFIGURE and DAEMON_IDLE states without actually calling
> reconfigure().
>
> Move the logic to postpone reconfigure() calls from __post_config_state()
> into child(), entirely, to avoid this situation. This means that child()
> needs to poll for reconfigure_pending besides running_state changes.
>
> Fixes: 250708c ("multipathd: improve delayed reconfigure")
> Fixes: 4b104e6 ("multipathd: pass in the type of reconfigure")
> Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  multipathd/main.c | 48 +++++++++++++++++++----------------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 7ecf3bd..d033a9a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -288,38 +288,12 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
>  /* Don't access this variable without holding config_lock */
>  static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
>
> -static void enable_delayed_reconfig(void)
> -{
> -       pthread_mutex_lock(&config_lock);
> -       __delayed_reconfig = true;
> -       pthread_mutex_unlock(&config_lock);
> -}
> -
>  /* must be called with config_lock held */
>  static void __post_config_state(enum daemon_status state)
>  {
>         if (state != running_state && running_state != DAEMON_SHUTDOWN) {
>                 enum daemon_status old_state = running_state;
>
> -               /*
> -                * Handle a pending reconfigure request.
> -                * DAEMON_IDLE is set from child() after reconfigure(),
> -                * or from checkerloop() after completing checkers.
> -                * In either case, child() will see DAEMON_CONFIGURE
> -                * again and start another reconfigure cycle.
> -                */
> -               if (reconfigure_pending != FORCE_RELOAD_NONE &&
> -                   state == DAEMON_IDLE &&
> -                   (old_state == DAEMON_CONFIGURE ||
> -                    old_state == DAEMON_RUNNING)) {
> -                       /*
> -                        * notify systemd of transient idle state, lest systemd
> -                        * thinks the reload lasts forever.
> -                        */
> -                       do_sd_notify(old_state, DAEMON_IDLE);
> -                       old_state = DAEMON_IDLE;
> -                       state = DAEMON_CONFIGURE;
> -               }
>                 running_state = state;
>                 pthread_cond_broadcast(&config_cond);
>                 do_sd_notify(old_state, state);
> @@ -3390,8 +3364,21 @@ child (__attribute__((unused)) void *param)
>                 pthread_cleanup_push(config_cleanup, NULL);
>                 pthread_mutex_lock(&config_lock);
>                 while (running_state != DAEMON_CONFIGURE &&
> -                      running_state != DAEMON_SHUTDOWN)
> +                      running_state != DAEMON_SHUTDOWN &&
> +                      /*
> +                       * Check if another reconfigure request was scheduled
> +                       * while we last ran reconfigure().
> +                       * We have to test __delayed_reconfig here
> +                       * to avoid a busy loop
> +                       */
> +                      (reconfigure_pending == FORCE_RELOAD_NONE
> +                        || __delayed_reconfig))
>                         pthread_cond_wait(&config_cond, &config_lock);
> +
> +               if (running_state != DAEMON_CONFIGURE &&
> +                   running_state != DAEMON_SHUTDOWN)
> +                       /* This sets running_state to DAEMON_CONFIGURE */
> +                       __post_config_state(DAEMON_CONFIGURE);
>                 state = running_state;
>                 pthread_cleanup_pop(1);
>                 if (state == DAEMON_SHUTDOWN)
> @@ -3412,8 +3399,11 @@ child (__attribute__((unused)) void *param)
>                         pthread_mutex_unlock(&config_lock);
>
>                         rc = reconfigure(vecs, reload_type);
> -               } else
> -                       enable_delayed_reconfig();
> +               } else {
> +                       pthread_mutex_lock(&config_lock);
> +                       __delayed_reconfig = true;
> +                       pthread_mutex_unlock(&config_lock);
> +               }
>                 lock_cleanup_pop(vecs->lock);
>                 if (!rc)
>                         post_config_state(DAEMON_IDLE);
> --
> 2.35.1
>