[dm-devel] [PATCH v2 04/11] multipathd: reset __delayed_reconfig from ev_add_map()

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck at suse.com> wrote:
>
> From: Martin Wilck <mwilck at suse.com>
>
> With the previous patch, one race condition between child() and
> the uevent handler (ev_add_map()) remains: 1. child() sets
> __delayed_reconfig, 2. ev_add_map() calls schedule_reconfigure() and
> thus DAEMON_CONFIGURE, 3. child() sets DAEMON_IDLE. This would cause
> the pending reconfigure request to be missed.
>
> To fix this, reset __delayed_reconfig immediately from ev_add_map()
> (and likewise, missing_uev_wait_tick()). This way the wait loop in
> child() terminates on the reconfigure_pending condition.
>
> Also, these schedule_reconfigure() callers don't really request a
> reconfigure() call; they just unblock processing previously requested
> reconfigure() calls. Add a new helper, unblock_reconfigure(), that
> does just that.
>
> Suggested-by: Benjamin Marzinski <bmarzins at redhat.com>
> Signed-off-by: Martin Wilck <mwilck at suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>
> ---
>  multipathd/main.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index d033a9a..1454a18 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -155,16 +155,6 @@ int should_exit(void)
>         return get_running_state() == DAEMON_SHUTDOWN;
>  }
>
> -static bool get_delayed_reconfig(void)
> -{
> -       bool val;
> -
> -       pthread_mutex_lock(&config_lock);
> -       val = __delayed_reconfig;
> -       pthread_mutex_unlock(&config_lock);
> -       return val;
> -}
> -
>  /*
>   * global copy of vecs for use in sig handlers
>   */
> @@ -308,6 +298,27 @@ void post_config_state(enum daemon_status state)
>         pthread_cleanup_pop(1);
>  }
>
> +static bool unblock_reconfigure(void)
> +{
> +       bool was_delayed;
> +
> +       pthread_mutex_lock(&config_lock);
> +       was_delayed = __delayed_reconfig;
> +       if (was_delayed) {
> +               __delayed_reconfig = false;
> +               /*
> +                * In IDLE state, make sure child() is woken up
> +                * Otherwise it will wake up when state switches to IDLE
> +                */
> +               if (running_state == DAEMON_IDLE)
> +                       __post_config_state(DAEMON_CONFIGURE);
> +       }
> +       pthread_mutex_unlock(&config_lock);
> +       if (was_delayed)
> +               condlog(3, "unblocked delayed reconfigure");
> +       return was_delayed;
> +}
> +
>  void schedule_reconfigure(enum force_reload_types requested_type)
>  {
>         pthread_mutex_lock(&config_lock);
> @@ -790,12 +801,9 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
>                 dm_get_info(mpp->alias, &mpp->dmi);
>                 if (mpp->wait_for_udev) {
>                         mpp->wait_for_udev = 0;
> -                       if (get_delayed_reconfig() &&
> -                           !need_to_delay_reconfig(vecs)) {
> -                               condlog(2, "reconfigure (delayed)");
> -                               schedule_reconfigure(FORCE_RELOAD_WEAK);
> +                       if (!need_to_delay_reconfig(vecs) &&
> +                           unblock_reconfigure())
>                                 return 0;
> -                       }
>                 }
>                 /*
>                  * Not really an error -- we generate our own uevent
> @@ -1899,11 +1907,8 @@ missing_uev_wait_tick(struct vectors *vecs)
>                 }
>         }
>
> -       if (timed_out && get_delayed_reconfig() &&
> -           !need_to_delay_reconfig(vecs)) {
> -               condlog(2, "reconfigure (delayed)");
> -               schedule_reconfigure(FORCE_RELOAD_WEAK);
> -       }
> +       if (timed_out && !need_to_delay_reconfig(vecs))
> +               unblock_reconfigure();
>  }
>
>  static void
> --
> 2.35.1
>