On Fri, Mar 18, 2022 at 5:33 PM <mwilck@xxxxxxxx> wrote:
>
> From: Martin Wilck <mwilck@xxxxxxxx>
>
> 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@xxxxxxxx>
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> 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
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
