[dm-devel] [PATCH 3/5] multipathd: avoid busy loop in child() for delayed reconfigure

[mwilck at suse.com]

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>
---
 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