| Message ID | 20220314213036.12074-4-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipathd: fix __delayed_reconfig logic | expand |
On Mon, Mar 14, 2022 at 10:30:34PM +0100, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@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. > Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com> With the understanding that this fix still leaves a bug that needs to be resolved. > Fixes: 250708c ("multipathd: improve delayed reconfigure") > Fixes: 4b104e6 ("multipathd: pass in the type of reconfigure") > Signed-off-by: Martin Wilck <mwilck@suse.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 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
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);