[RFC PATCH 0/6] multipath-tools: Fix remaining shutdown delay issues

[RFC PATCH 0/6] multipath-tools: Fix remaining shutdown delay issues

[Martin Wilck]

Hi Chongyun, Ben, all,

this patch set addresses the points where I can see that handling of
shutdown signals may be delayed, as discussed previously. Quoting my
previous post:

Let's summarize how multipathd exit works today:

 1. signal arrives
    (signal may be blocked while uxlsnr is busy, see above)
 2. signal is unblocked in uxlsnr thread (in ppoll())
 3. signal handler sets exit_sig()
 4. uxlsnr calls handle_signals()
 5. handle_signals()->exit_daemon() sets DAEMON_SHUTDOWN() and posts
config_cond (child may busy in reconfigure())
 6. child detects DAEMON_SHUTDOWN and quits main loop
 7. child locks vecs->lock (may cause wait)
 8. sets dm_queue_if_no_path, cancels threads, and exits.

I can imagine delays in step 1, 5, and 7, but not in ppoll().

This series addresses 1) in patch 5 and 6, 5) in patch 3, and 7) in patch 4.
The series also contains the part of Chongyun's previously posted patch
which I agree with.

The set is compile tested, but no more so far. Chongyun, I'd be grateful
if you could review it, and give it a try in your test setup.

Chongyun Wu (1):
  multipathd: fix daemon not really shutdown

Martin Wilck (5):
  multipathd: protect all access to running_state
  multipathd: allow shutdown during configure()
  multipathd: cancel threads early during shutdown
  multipathd: add code to handle blocked signals
  multipathd: uxlsnr: handle signals while busy

 libmultipath/configure.c |   5 ++
 libmultipath/discovery.c |   4 ++
 libmultipath/exit.h      |   5 ++
 mpathpersist/main.c      |   5 ++
 multipath/main.c         |   6 ++
 multipathd/cli.c         |  50 ++++++++++----
 multipathd/cli.h         |   2 +-
 multipathd/main.c        | 143 ++++++++++++++++++++++++++++-----------
 multipathd/main.h        |   1 +
 9 files changed, 166 insertions(+), 55 deletions(-)
 create mode 100644 libmultipath/exit.h

-- 
2.19.2

[RFC PATCH 1/6] multipathd: fix daemon not really shutdown

[Martin Wilck]

From: Chongyun Wu <wu.chongyun@h3c.com>

Test environment: 25 hosts, each host have more than 100 luns,
each lun have two paths.
Some times when we try to ceate new multipath will encounter "could
not create uxsock:98" but the multipathd still running not shutdown
and can't response any multipathd commands also.

After reproduce this issue and debug, found below fixes might work:
(1) set_config_state() after pthread_cond_timedwait() other threads
might changed the running_state from DAEMON_SHUTDOWN to other status
like DAEMON_IDLE, which will make the shutdown process stopped.
I found logs to prove this really happened, so we need add judgement
here too.

(2) [this part removed by mwilck]

Signed-off-by: Chongyun Wu <wu.chongyun@h3c.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 99145293..6a5d105a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -247,7 +247,7 @@ int set_config_state(enum daemon_status state)
 			rc = pthread_cond_timedwait(&config_cond,
 						    &config_lock, &ts);
 		}
-		if (!rc) {
+		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
 			running_state = state;
 			pthread_cond_broadcast(&config_cond);
 #ifdef USE_SYSTEMD
-- 
2.19.2

[RFC PATCH 2/6] multipathd: protect all access to running_state

[Martin Wilck]

Chonyun Wu's latest patch has shown that the handling of the daemon
state variable running_state is racy and difficult to get right. It's
not a good candidate for a "benign race" annotation. So, as a first
step to sanitizing it, make sure all accesses to the state variable
are protected by config_lock.

The patch also replaces "if" with "while" in several places where the
code was supposed to wait until a certain state was reached. It's
important that DAEMON_SHUTDOWN terminates all loops of this kind.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 79 ++++++++++++++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 31 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 6a5d105a..6fc6a3ac 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -126,11 +126,21 @@ int poll_dmevents = 0;
 #else
 int poll_dmevents = 1;
 #endif
-enum daemon_status running_state = DAEMON_INIT;
+enum daemon_status _running_state = DAEMON_INIT;
 pid_t daemon_pid;
 pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
 pthread_cond_t config_cond;
 
+static inline enum daemon_status get_running_state(void)
+{
+	enum daemon_status st;
+
+	pthread_mutex_lock(&config_lock);
+	st = _running_state;
+	pthread_mutex_unlock(&config_lock);
+	return st;
+}
+
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -148,7 +158,7 @@ static volatile sig_atomic_t log_reset_sig;
 const char *
 daemon_status(void)
 {
-	switch (running_state) {
+	switch (get_running_state()) {
 	case DAEMON_INIT:
 		return "init";
 	case DAEMON_START:
@@ -168,10 +178,10 @@ daemon_status(void)
 /*
  * I love you too, systemd ...
  */
-const char *
-sd_notify_status(void)
+static const char *
+sd_notify_status(enum daemon_status state)
 {
-	switch (running_state) {
+	switch (state) {
 	case DAEMON_INIT:
 		return "STATUS=init";
 	case DAEMON_START:
@@ -188,17 +198,18 @@ sd_notify_status(void)
 }
 
 #ifdef USE_SYSTEMD
-static void do_sd_notify(enum daemon_status old_state)
+static void do_sd_notify(enum daemon_status old_state,
+			 enum daemon_status new_state)
 {
 	/*
 	 * Checkerloop switches back and forth between idle and running state.
 	 * No need to tell systemd each time.
 	 * These notifications cause a lot of overhead on dbus.
 	 */
-	if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
+	if ((new_state == DAEMON_IDLE || new_state == DAEMON_RUNNING) &&
 	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
 		return;
-	sd_notify(0, sd_notify_status());
+	sd_notify(0, sd_notify_status(new_state));
 }
 #endif
 
@@ -207,15 +218,16 @@ static void config_cleanup(void *arg)
 	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;
+	if (state != _running_state && _running_state != DAEMON_SHUTDOWN) {
+		enum daemon_status old_state = _running_state;
 
-		running_state = state;
+		_running_state = state;
 		pthread_cond_broadcast(&config_cond);
 #ifdef USE_SYSTEMD
-		do_sd_notify(old_state);
+		do_sd_notify(old_state, state);
 #endif
 	}
 }
@@ -234,12 +246,12 @@ int set_config_state(enum daemon_status state)
 
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
-	if (running_state != state) {
-		enum daemon_status old_state = running_state;
+	if (_running_state != state) {
+		enum daemon_status old_state = _running_state;
 
-		if (running_state == DAEMON_SHUTDOWN)
+		if (_running_state == DAEMON_SHUTDOWN)
 			rc = EINVAL;
-		else if (running_state != DAEMON_IDLE) {
+		else if (_running_state != DAEMON_IDLE) {
 			struct timespec ts;
 
 			clock_gettime(CLOCK_MONOTONIC, &ts);
@@ -247,11 +259,11 @@ int set_config_state(enum daemon_status state)
 			rc = pthread_cond_timedwait(&config_cond,
 						    &config_lock, &ts);
 		}
-		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
-			running_state = state;
+		if (!rc && (_running_state != DAEMON_SHUTDOWN)) {
+			_running_state = state;
 			pthread_cond_broadcast(&config_cond);
 #ifdef USE_SYSTEMD
-			do_sd_notify(old_state);
+			do_sd_notify(old_state, state);
 #endif
 		}
 	}
@@ -1405,17 +1417,20 @@ uev_trigger (struct uevent * uev, void * trigger_data)
 	int r = 0;
 	struct vectors * vecs;
 	struct uevent *merge_uev, *tmp;
+	enum daemon_status state;
 
 	vecs = (struct vectors *)trigger_data;
 
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
-	if (running_state != DAEMON_IDLE &&
-	    running_state != DAEMON_RUNNING)
+	while (_running_state != DAEMON_IDLE &&
+	       _running_state != DAEMON_RUNNING &&
+	       _running_state != DAEMON_SHUTDOWN)
 		pthread_cond_wait(&config_cond, &config_lock);
+	state = _running_state;
 	pthread_cleanup_pop(1);
 
-	if (running_state == DAEMON_SHUTDOWN)
+	if (state == DAEMON_SHUTDOWN)
 		return 0;
 
 	/*
@@ -2661,6 +2676,7 @@ child (void * param)
 	struct config *conf;
 	char *envp;
 	int queue_without_daemon;
+	enum daemon_status state;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
@@ -2756,8 +2772,9 @@ child (void * param)
 	rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
 	if (!rc) {
 		/* Wait for uxlsnr startup */
-		while (running_state == DAEMON_IDLE)
+		while (_running_state == DAEMON_IDLE)
 			pthread_cond_wait(&config_cond, &config_lock);
+		state = _running_state;
 	}
 	pthread_cleanup_pop(1);
 
@@ -2765,7 +2782,7 @@ child (void * param)
 		condlog(0, "failed to create cli listener: %d", rc);
 		goto failed;
 	}
-	else if (running_state != DAEMON_CONFIGURE) {
+	else if (state != DAEMON_CONFIGURE) {
 		condlog(0, "cli listener failed to start");
 		goto failed;
 	}
@@ -2805,15 +2822,17 @@ child (void * param)
 	}
 	pthread_attr_destroy(&misc_attr);
 
-	while (running_state != DAEMON_SHUTDOWN) {
+	while (1) {
 		pthread_cleanup_push(config_cleanup, NULL);
 		pthread_mutex_lock(&config_lock);
-		if (running_state != DAEMON_CONFIGURE &&
-		    running_state != DAEMON_SHUTDOWN) {
+		while (_running_state != DAEMON_CONFIGURE &&
+		       _running_state != DAEMON_SHUTDOWN)
 			pthread_cond_wait(&config_cond, &config_lock);
-		}
+		state = _running_state;
 		pthread_cleanup_pop(1);
-		if (running_state == DAEMON_CONFIGURE) {
+		if (state == DAEMON_SHUTDOWN)
+			break;
+		if (state == DAEMON_CONFIGURE) {
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
@@ -2983,8 +3002,6 @@ main (int argc, char *argv[])
 
 	ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
 				   "Manipulated through RCU");
-	ANNOTATE_BENIGN_RACE_SIZED(&running_state, sizeof(running_state),
-		"Suppress complaints about unprotected running_state reads");
 	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
 		"Suppress complaints about this scalar variable");
 
-- 
2.19.2

[RFC PATCH 3/6] multipathd: allow shutdown during configure()

[Martin Wilck]

reconfigure() can be a long-running operation; both initial path
discovery and initial map setup can take a long time. Allow
the main program to indicate that the process should be
interrupted if a shutdown signal was received.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c |  5 +++++
 libmultipath/discovery.c |  4 ++++
 libmultipath/exit.h      |  5 +++++
 mpathpersist/main.c      |  5 +++++
 multipath/main.c         |  6 ++++++
 multipathd/main.c        | 18 ++++++++++++++++++
 6 files changed, 43 insertions(+)
 create mode 100644 libmultipath/exit.h

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 84ae5f56..2b062b35 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -43,6 +43,7 @@
 #include "wwids.h"
 #include "sysfs.h"
 #include "io_err_stat.h"
+#include "exit.h"
 
 /* group paths in pg by host adapter
  */
@@ -1021,6 +1022,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
 
 	vector_foreach_slot (pathvec, pp1, k) {
 		int invalid;
+
+		if (should_exit())
+			return CP_FAIL;
+
 		/* skip this path for some reason */
 
 		/* 1. if path has no unique id or wwid blacklisted */
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 7f983a63..3834685c 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -33,6 +33,7 @@
 #include "unaligned.h"
 #include "prioritizers/alua_rtpg.h"
 #include "foreign.h"
+#include "exit.h"
 
 int
 alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
@@ -160,6 +161,9 @@ path_discovery (vector pathvec, int flag)
 	udev_list_entry_foreach(entry,
 				udev_enumerate_get_list_entry(udev_iter)) {
 		const char *devtype;
+
+		if (should_exit())
+			break;
 		devpath = udev_list_entry_get_name(entry);
 		condlog(4, "Discover device %s", devpath);
 		udevice = udev_device_new_from_syspath(udev, devpath);
diff --git a/libmultipath/exit.h b/libmultipath/exit.h
new file mode 100644
index 00000000..1954c5ce
--- /dev/null
+++ b/libmultipath/exit.h
@@ -0,0 +1,5 @@
+#ifndef _EXIT_H
+#define _EXIT_H
+
+extern int should_exit(void);
+#endif /* _EXIT_H */
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 10cba452..fa831a07 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -7,6 +7,7 @@
 #include "vector.h"
 #include "config.h"
 #include "structs.h"
+#include "exit.h"
 #include <getopt.h>
 #include <libudev.h>
 #include "mpath_persist.h"
@@ -42,6 +43,10 @@ int construct_transportid(const char * inp, struct transportid transid[], int nu
 
 int logsink;
 struct config *multipath_conf;
+int should_exit(void)
+{
+	return 0;
+}
 
 struct config *get_multipath_config(void)
 {
diff --git a/multipath/main.c b/multipath/main.c
index f40c179b..a5ca10ca 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -63,8 +63,14 @@
 #include "propsel.h"
 #include "time-util.h"
 #include "file.h"
+#include "exit.h"
 
 int logsink;
+int should_exit(void)
+{
+	return 0;
+}
+
 struct udev *udev;
 struct config *multipath_conf;
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 6fc6a3ac..413beee0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -67,6 +67,7 @@ static int use_watchdog;
 #include "uevent.h"
 #include "log.h"
 #include "uxsock.h"
+#include "exit.h"
 
 #include "mpath_cmd.h"
 #include "mpath_persist.h"
@@ -141,6 +142,11 @@ static inline enum daemon_status get_running_state(void)
 	return st;
 }
 
+int should_exit(void)
+{
+	return get_running_state() == DAEMON_SHUTDOWN;
+}
+
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -2355,6 +2361,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
 	vector_foreach_slot (vecs->pathvec, pp, i){
@@ -2371,6 +2380,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	/*
 	 * create new set of maps & push changed ones into dm
 	 * In the first call, use FORCE_RELOAD_WEAK to avoid making
@@ -2385,6 +2397,9 @@ configure (struct vectors * vecs)
 		goto fail;
 	}
 
+	if (should_exit())
+		goto fail;
+
 	/*
 	 * may need to remove some maps which are no longer relevant
 	 * e.g., due to blacklist changes in conf file
@@ -2396,6 +2411,9 @@ configure (struct vectors * vecs)
 
 	dm_lib_release();
 
+	if (should_exit())
+		goto fail;
+
 	sync_maps_state(mpvec);
 	vector_foreach_slot(mpvec, mpp, i){
 		if (remember_wwid(mpp->wwid) == 1)
-- 
2.19.2

[RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Martin Wilck]

Cancel the other threads before taking vecs->lock. This avoids
delays during shutdown caused e.g. by the checker thread holding
the vecs lock.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 413beee0..569a27ac 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2872,23 +2872,24 @@ child (void * param)
 		}
 	}
 
-	lock(&vecs->lock);
+	pthread_cancel(check_thr);
+	pthread_cancel(uevent_thr);
+	pthread_cancel(uxlsnr_thr);
+	pthread_cancel(uevq_thr);
+	if (poll_dmevents)
+		pthread_cancel(dmevent_thr);
+
 	conf = get_multipath_config();
 	queue_without_daemon = conf->queue_without_daemon;
 	put_multipath_config(conf);
+
+	lock(&vecs->lock);
 	if (queue_without_daemon == QUE_NO_DAEMON_OFF)
 		vector_foreach_slot(vecs->mpvec, mpp, i)
 			dm_queue_if_no_path(mpp->alias, 0);
 	remove_maps_and_stop_waiters(vecs);
 	unlock(&vecs->lock);
 
-	pthread_cancel(check_thr);
-	pthread_cancel(uevent_thr);
-	pthread_cancel(uxlsnr_thr);
-	pthread_cancel(uevq_thr);
-	if (poll_dmevents)
-		pthread_cancel(dmevent_thr);
-
 	pthread_join(check_thr, NULL);
 	pthread_join(uevent_thr, NULL);
 	pthread_join(uxlsnr_thr, NULL);
-- 
2.19.2

[RFC PATCH 5/6] multipathd: add code to handle blocked signals

[Martin Wilck]

multipathd blocks all signals except in the uxlsnr thread.
Add some code to handle signals even while they're blocked.

If a shutdown signal is received, deliver_pending_signals() returns
TRUE. This is not the same as should_exit(), it's just a friendly
warning to the caller that shutdown/thread cancellation is imminent.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 25 +++++++++++++++++++++++++
 multipathd/main.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index 569a27ac..6276d34c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2614,6 +2614,31 @@ signal_init(void)
 	signal_set(SIGPIPE, sigend);
 }
 
+bool deliver_pending_signals(void)
+{
+	sigset_t set;
+	bool ret = exit_sig;
+
+	if (sigpending(&set) != 0)
+		return false;
+
+	if (sigismember(&set, SIGTERM) ||
+	    sigismember(&set, SIGPIPE) ||
+	    sigismember(&set, SIGINT)) {
+		sigend(SIGTERM);
+		ret = true;
+	}
+	if (sigismember(&set, SIGHUP))
+		sighup(SIGHUP);
+	if (sigismember(&set, SIGUSR1))
+		sigusr1(SIGUSR1);
+	if (sigismember(&set, SIGUSR2))
+		sigusr2(SIGUSR2);
+
+	handle_signals(false);
+	return ret;
+}
+
 static void
 setscheduler (void)
 {
diff --git a/multipathd/main.h b/multipathd/main.h
index 8fd426b0..2b77b44b 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -39,6 +39,7 @@ void * mpath_pr_event_handler_fn (void * );
 int update_map_pr(struct multipath *mpp);
 void * mpath_pr_event_handler_fn (void * pathp );
 void handle_signals(bool);
+bool deliver_pending_signals(void);
 int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
 		       int reset);
 #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
-- 
2.19.2

[RFC PATCH 6/6] multipathd: uxlsnr: handle signals while busy

[Martin Wilck]

The uxlsnr thread is responsible for receiving and handling
signals in multipathd. This works well while it is idle and
waiting for connections in ppoll(). But if it's busy, cli commands
may need to take the vecs lock, which might take a long time.

Use multipathd's ability to handle pending signals to avoid
shutdown delays.

If we find a deadly signal pending, try to use the remaining cycles
to provide a meaningful error message to the client rather than
simply break the connection.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/cli.c  | 50 ++++++++++++++++++++++++++++++++++-------------
 multipathd/cli.h  |  2 +-
 multipathd/main.c |  4 +++-
 3 files changed, 40 insertions(+), 16 deletions(-)

diff --git a/multipathd/cli.c b/multipathd/cli.c
index a75afe3f..824c01b6 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -12,7 +12,7 @@
 #include "util.h"
 #include "version.h"
 #include <readline/readline.h>
-
+#include "main.h"
 #include "cli.h"
 
 static vector keys;
@@ -453,13 +453,45 @@ genhelp_handler (const char *cmd, int error)
 	return reply;
 }
 
+static int cli_timedlock(struct mutex_lock *a, long ms)
+{
+	struct timespec tmo;
+	const long delta_ms = 100;
+	const long MILLION = 1000L * 1000;
+	const long delta_ns = MILLION * delta_ms;
+	const long BILLION = 1000L * MILLION;
+	int r;
+
+	if (ms <= 0 || clock_gettime(CLOCK_REALTIME, &tmo) != 0) {
+		if (deliver_pending_signals())
+			return EINTR;
+		pthread_testcancel();
+		pthread_mutex_lock(&a->mutex);
+		return 0;
+	}
+
+	for(; ms > 0; ms -= delta_ms) {
+		if (deliver_pending_signals())
+			return EINTR;
+		pthread_testcancel();
+		tmo.tv_nsec += (ms >= delta_ms ? delta_ns : MILLION * ms);
+		if (tmo.tv_nsec >= BILLION) {
+			tmo.tv_nsec -= BILLION;
+			tmo.tv_sec++;
+		}
+		r = pthread_mutex_timedlock(&a->mutex, &tmo);
+		if (r != ETIMEDOUT)
+			return r;
+	}
+	return ETIMEDOUT;
+}
+
 int
-parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
+parse_cmd(char *cmd, char **reply, int *len, void *data, long timeout_ms)
 {
 	int r;
 	struct handler * h;
 	vector cmdvec = NULL;
-	struct timespec tmo;
 
 	r = get_cmdvec(cmd, &cmdvec);
 
@@ -481,22 +513,12 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
 	/*
 	 * execute handler
 	 */
-	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
-		tmo.tv_sec += timeout;
-	} else {
-		tmo.tv_sec = 0;
-	}
 	if (h->locked) {
 		int locked = 0;
 		struct vectors * vecs = (struct vectors *)data;
 
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
-		if (tmo.tv_sec) {
-			r = timedlock(&vecs->lock, &tmo);
-		} else {
-			lock(&vecs->lock);
-			r = 0;
-		}
+		r = cli_timedlock(&vecs->lock, timeout_ms);
 		if (r == 0) {
 			locked = 1;
 			pthread_testcancel();
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 7cc7e4be..fb20e0d2 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -123,7 +123,7 @@ int alloc_handlers (void);
 int add_handler (uint64_t fp, int (*fn)(void *, char **, int *, void *));
 int set_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
 int set_unlocked_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
-int parse_cmd (char * cmd, char ** reply, int * len, void *, int);
+int parse_cmd(char *cmd, char **reply, int *len, void *data, long timeout_ms);
 int load_keys (void);
 char * get_keyparam (vector v, uint64_t code);
 void free_keys (vector vec);
diff --git a/multipathd/main.c b/multipathd/main.c
index 6276d34c..9ed0cadd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1395,11 +1395,13 @@ uxsock_trigger (char * str, char ** reply, int * len, bool is_root,
 		return 1;
 	}
 
-	r = parse_cmd(str, reply, len, vecs, uxsock_timeout / 1000);
+	r = parse_cmd(str, reply, len, vecs, uxsock_timeout);
 
 	if (r > 0) {
 		if (r == ETIMEDOUT)
 			*reply = STRDUP("timeout\n");
+		else if (r == EINTR)
+			*reply = STRDUP("daemon exiting\n");
 		else
 			*reply = STRDUP("fail\n");
 		if (*reply)
-- 
2.19.2

Re: [RFC PATCH 0/6] multipath-tools: Fix remaining shutdown delay issues

[Chongyun Wu]

Hi Martin,

Your analysis and patch is good for me. I will pick up those patches and 
test in our test environment, I will let you know the result when I 
finish the test, thanks.

Chongyun Wu

On 2019/1/5 1:59, Martin Wilck wrote:
> Hi Chongyun, Ben, all,
> 
> this patch set addresses the points where I can see that handling of
> shutdown signals may be delayed, as discussed previously. Quoting my
> previous post:
> 
> Let's summarize how multipathd exit works today:
> 
>   1. signal arrives
>      (signal may be blocked while uxlsnr is busy, see above)
>   2. signal is unblocked in uxlsnr thread (in ppoll())
>   3. signal handler sets exit_sig()
>   4. uxlsnr calls handle_signals()
>   5. handle_signals()->exit_daemon() sets DAEMON_SHUTDOWN() and posts
> config_cond (child may busy in reconfigure())
>   6. child detects DAEMON_SHUTDOWN and quits main loop
>   7. child locks vecs->lock (may cause wait)
>   8. sets dm_queue_if_no_path, cancels threads, and exits.
> 
> I can imagine delays in step 1, 5, and 7, but not in ppoll().
> 
> This series addresses 1) in patch 5 and 6, 5) in patch 3, and 7) in patch 4.
> The series also contains the part of Chongyun's previously posted patch
> which I agree with.
> 
> The set is compile tested, but no more so far. Chongyun, I'd be grateful
> if you could review it, and give it a try in your test setup.
> 
> Chongyun Wu (1):
>    multipathd: fix daemon not really shutdown
> 
> Martin Wilck (5):
>    multipathd: protect all access to running_state
>    multipathd: allow shutdown during configure()
>    multipathd: cancel threads early during shutdown
>    multipathd: add code to handle blocked signals
>    multipathd: uxlsnr: handle signals while busy
> 
>   libmultipath/configure.c |   5 ++
>   libmultipath/discovery.c |   4 ++
>   libmultipath/exit.h      |   5 ++
>   mpathpersist/main.c      |   5 ++
>   multipath/main.c         |   6 ++
>   multipathd/cli.c         |  50 ++++++++++----
>   multipathd/cli.h         |   2 +-
>   multipathd/main.c        | 143 ++++++++++++++++++++++++++++-----------
>   multipathd/main.h        |   1 +
>   9 files changed, 166 insertions(+), 55 deletions(-)
>   create mode 100644 libmultipath/exit.h
> 

Re: [RFC PATCH 3/6] multipathd: allow shutdown during configure()

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:11PM +0100, Martin Wilck wrote:
> reconfigure() can be a long-running operation; both initial path
> discovery and initial map setup can take a long time. Allow
> the main program to indicate that the process should be
> interrupted if a shutdown signal was received.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c |  5 +++++
>  libmultipath/discovery.c |  4 ++++
>  libmultipath/exit.h      |  5 +++++
>  mpathpersist/main.c      |  5 +++++
>  multipath/main.c         |  6 ++++++
>  multipathd/main.c        | 18 ++++++++++++++++++
>  6 files changed, 43 insertions(+)
>  create mode 100644 libmultipath/exit.h
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 84ae5f56..2b062b35 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -43,6 +43,7 @@
>  #include "wwids.h"
>  #include "sysfs.h"
>  #include "io_err_stat.h"
> +#include "exit.h"
>  
>  /* group paths in pg by host adapter
>   */
> @@ -1021,6 +1022,10 @@ int coalesce_paths (struct vectors * vecs, vector newmp, char * refwwid,
>  
>  	vector_foreach_slot (pathvec, pp1, k) {
>  		int invalid;
> +
> +		if (should_exit())
> +			return CP_FAIL;

Admittedly, we are about to exit here, so it's not a big deal, but
simply returning instead of doing

	ret = CP_FAIL;
	goto out;

will leak "size_mismatch_seen".


> +
>  		/* skip this path for some reason */
>  
>  		/* 1. if path has no unique id or wwid blacklisted */
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 7f983a63..3834685c 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -33,6 +33,7 @@
>  #include "unaligned.h"
>  #include "prioritizers/alua_rtpg.h"
>  #include "foreign.h"
> +#include "exit.h"
>  
>  int
>  alloc_path_with_pathinfo (struct config *conf, struct udev_device *udevice,
> @@ -160,6 +161,9 @@ path_discovery (vector pathvec, int flag)
>  	udev_list_entry_foreach(entry,
>  				udev_enumerate_get_list_entry(udev_iter)) {
>  		const char *devtype;
> +
> +		if (should_exit())
> +			break;
>  		devpath = udev_list_entry_get_name(entry);
>  		condlog(4, "Discover device %s", devpath);
>  		udevice = udev_device_new_from_syspath(udev, devpath);
> diff --git a/libmultipath/exit.h b/libmultipath/exit.h
> new file mode 100644
> index 00000000..1954c5ce
> --- /dev/null
> +++ b/libmultipath/exit.h
> @@ -0,0 +1,5 @@
> +#ifndef _EXIT_H
> +#define _EXIT_H
> +
> +extern int should_exit(void);
> +#endif /* _EXIT_H */
> diff --git a/mpathpersist/main.c b/mpathpersist/main.c
> index 10cba452..fa831a07 100644
> --- a/mpathpersist/main.c
> +++ b/mpathpersist/main.c
> @@ -7,6 +7,7 @@
>  #include "vector.h"
>  #include "config.h"
>  #include "structs.h"
> +#include "exit.h"
>  #include <getopt.h>
>  #include <libudev.h>
>  #include "mpath_persist.h"
> @@ -42,6 +43,10 @@ int construct_transportid(const char * inp, struct transportid transid[], int nu
>  
>  int logsink;
>  struct config *multipath_conf;
> +int should_exit(void)
> +{
> +	return 0;
> +}
>  
>  struct config *get_multipath_config(void)
>  {
> diff --git a/multipath/main.c b/multipath/main.c
> index f40c179b..a5ca10ca 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -63,8 +63,14 @@
>  #include "propsel.h"
>  #include "time-util.h"
>  #include "file.h"
> +#include "exit.h"
>  
>  int logsink;
> +int should_exit(void)
> +{
> +	return 0;
> +}
> +
>  struct udev *udev;
>  struct config *multipath_conf;
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6fc6a3ac..413beee0 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c

In child(), if we break out of this initial config early (which is
unlikely, but since uxlsnr is up, it is possible), we probably shouldn't
be doing

	sd_notify(0, "READY=1");

-Ben

> @@ -67,6 +67,7 @@ static int use_watchdog;
>  #include "uevent.h"
>  #include "log.h"
>  #include "uxsock.h"
> +#include "exit.h"
>  
>  #include "mpath_cmd.h"
>  #include "mpath_persist.h"
> @@ -141,6 +142,11 @@ static inline enum daemon_status get_running_state(void)
>  	return st;
>  }
>  
> +int should_exit(void)
> +{
> +	return get_running_state() == DAEMON_SHUTDOWN;
> +}
> +
>  /*
>   * global copy of vecs for use in sig handlers
>   */
> @@ -2355,6 +2361,9 @@ configure (struct vectors * vecs)
>  		goto fail;
>  	}
>  
> +	if (should_exit())
> +		goto fail;
> +
>  	conf = get_multipath_config();
>  	pthread_cleanup_push(put_multipath_config, conf);
>  	vector_foreach_slot (vecs->pathvec, pp, i){
> @@ -2371,6 +2380,9 @@ configure (struct vectors * vecs)
>  		goto fail;
>  	}
>  
> +	if (should_exit())
> +		goto fail;
> +
>  	/*
>  	 * create new set of maps & push changed ones into dm
>  	 * In the first call, use FORCE_RELOAD_WEAK to avoid making
> @@ -2385,6 +2397,9 @@ configure (struct vectors * vecs)
>  		goto fail;
>  	}
>  
> +	if (should_exit())
> +		goto fail;
> +
>  	/*
>  	 * may need to remove some maps which are no longer relevant
>  	 * e.g., due to blacklist changes in conf file
> @@ -2396,6 +2411,9 @@ configure (struct vectors * vecs)
>  
>  	dm_lib_release();
>  
> +	if (should_exit())
> +		goto fail;
> +
>  	sync_maps_state(mpvec);
>  	vector_foreach_slot(mpvec, mpp, i){
>  		if (remember_wwid(mpp->wwid) == 1)
> -- 
> 2.19.2

Re: [RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:12PM +0100, Martin Wilck wrote:
> Cancel the other threads before taking vecs->lock. This avoids
> delays during shutdown caused e.g. by the checker thread holding
> the vecs lock.

Before this change, multipathd was guaranteed that once a thread had
locked the vecs lock, and checked if it had been cancelled, it could not
be cancelled after that until it unlocked the vecs lock.  Undoing this
guarantee will likely make it possible for multipathd to leak memory
where it wasn't possible before. However, since it's happening during
shutdown, I'm not sure that we really need to care. Or, perhaps you
audited this, and I'm wrong.

At any rate, it's at least worth mentioning.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 413beee0..569a27ac 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2872,23 +2872,24 @@ child (void * param)
>  		}
>  	}
>  
> -	lock(&vecs->lock);
> +	pthread_cancel(check_thr);
> +	pthread_cancel(uevent_thr);
> +	pthread_cancel(uxlsnr_thr);
> +	pthread_cancel(uevq_thr);
> +	if (poll_dmevents)
> +		pthread_cancel(dmevent_thr);
> +
>  	conf = get_multipath_config();
>  	queue_without_daemon = conf->queue_without_daemon;
>  	put_multipath_config(conf);
> +
> +	lock(&vecs->lock);
>  	if (queue_without_daemon == QUE_NO_DAEMON_OFF)
>  		vector_foreach_slot(vecs->mpvec, mpp, i)
>  			dm_queue_if_no_path(mpp->alias, 0);
>  	remove_maps_and_stop_waiters(vecs);
>  	unlock(&vecs->lock);
>  
> -	pthread_cancel(check_thr);
> -	pthread_cancel(uevent_thr);
> -	pthread_cancel(uxlsnr_thr);
> -	pthread_cancel(uevq_thr);
> -	if (poll_dmevents)
> -		pthread_cancel(dmevent_thr);
> -
>  	pthread_join(check_thr, NULL);
>  	pthread_join(uevent_thr, NULL);
>  	pthread_join(uxlsnr_thr, NULL);
> -- 
> 2.19.2

Re: [RFC PATCH 5/6] multipathd: add code to handle blocked signals

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:13PM +0100, Martin Wilck wrote:
> multipathd blocks all signals except in the uxlsnr thread.
> Add some code to handle signals even while they're blocked.
> 
> If a shutdown signal is received, deliver_pending_signals() returns
> TRUE. This is not the same as should_exit(), it's just a friendly
> warning to the caller that shutdown/thread cancellation is imminent.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 25 +++++++++++++++++++++++++
>  multipathd/main.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 569a27ac..6276d34c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2614,6 +2614,31 @@ signal_init(void)
>  	signal_set(SIGPIPE, sigend);
>  }
>  
> +bool deliver_pending_signals(void)
> +{
> +	sigset_t set;
> +	bool ret = exit_sig;
> +
> +	if (sigpending(&set) != 0)
> +		return false;

shouldn't this return ret instead of false?

-Ben

> +
> +	if (sigismember(&set, SIGTERM) ||
> +	    sigismember(&set, SIGPIPE) ||
> +	    sigismember(&set, SIGINT)) {
> +		sigend(SIGTERM);
> +		ret = true;
> +	}
> +	if (sigismember(&set, SIGHUP))
> +		sighup(SIGHUP);
> +	if (sigismember(&set, SIGUSR1))
> +		sigusr1(SIGUSR1);
> +	if (sigismember(&set, SIGUSR2))
> +		sigusr2(SIGUSR2);
> +
> +	handle_signals(false);
> +	return ret;
> +}
> +
>  static void
>  setscheduler (void)
>  {
> diff --git a/multipathd/main.h b/multipathd/main.h
> index 8fd426b0..2b77b44b 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -39,6 +39,7 @@ void * mpath_pr_event_handler_fn (void * );
>  int update_map_pr(struct multipath *mpp);
>  void * mpath_pr_event_handler_fn (void * pathp );
>  void handle_signals(bool);
> +bool deliver_pending_signals(void);
>  int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
>  		       int reset);
>  #define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
> -- 
> 2.19.2

Re: [RFC PATCH 6/6] multipathd: uxlsnr: handle signals while busy

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:14PM +0100, Martin Wilck wrote:
> The uxlsnr thread is responsible for receiving and handling
> signals in multipathd. This works well while it is idle and
> waiting for connections in ppoll(). But if it's busy, cli commands
> may need to take the vecs lock, which might take a long time.
> 
> Use multipathd's ability to handle pending signals to avoid
> shutdown delays.
> 
> If we find a deadly signal pending, try to use the remaining cycles
> to provide a meaningful error message to the client rather than
> simply break the connection.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/cli.c  | 50 ++++++++++++++++++++++++++++++++++-------------
>  multipathd/cli.h  |  2 +-
>  multipathd/main.c |  4 +++-
>  3 files changed, 40 insertions(+), 16 deletions(-)
> 
> diff --git a/multipathd/cli.c b/multipathd/cli.c
> index a75afe3f..824c01b6 100644
> --- a/multipathd/cli.c
> +++ b/multipathd/cli.c
> @@ -12,7 +12,7 @@
>  #include "util.h"
>  #include "version.h"
>  #include <readline/readline.h>
> -
> +#include "main.h"
>  #include "cli.h"
>  
>  static vector keys;
> @@ -453,13 +453,45 @@ genhelp_handler (const char *cmd, int error)
>  	return reply;
>  }
>  
> +static int cli_timedlock(struct mutex_lock *a, long ms)
> +{
> +	struct timespec tmo;
> +	const long delta_ms = 100;
> +	const long MILLION = 1000L * 1000;
> +	const long delta_ns = MILLION * delta_ms;
> +	const long BILLION = 1000L * MILLION;
> +	int r;
> +
> +	if (ms <= 0 || clock_gettime(CLOCK_REALTIME, &tmo) != 0) {
> +		if (deliver_pending_signals())
> +			return EINTR;

Why call pthread_testcancel() before pthread_mutex_lock()?

I realize that the cancels are no longer blocked by holding the vecs
lock, but deliver_pending_signals() should have already told us if we
are about to shutdown. If we're not, it's very unlikely that we will be
cancelled between that check and the pthread_testcancel(). However, the
pthread_mutex_lock() might take some time, so it makes more sense to
check if we were cancelled after that. Or is there some other reason for
the check being there?

-Ben

> +		pthread_testcancel();
> +		pthread_mutex_lock(&a->mutex);
> +		return 0;
> +	}
> +
> +	for(; ms > 0; ms -= delta_ms) {
> +		if (deliver_pending_signals())
> +			return EINTR;
> +		pthread_testcancel();
> +		tmo.tv_nsec += (ms >= delta_ms ? delta_ns : MILLION * ms);
> +		if (tmo.tv_nsec >= BILLION) {
> +			tmo.tv_nsec -= BILLION;
> +			tmo.tv_sec++;
> +		}
> +		r = pthread_mutex_timedlock(&a->mutex, &tmo);
> +		if (r != ETIMEDOUT)
> +			return r;
> +	}
> +	return ETIMEDOUT;
> +}
> +
>  int
> -parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
> +parse_cmd(char *cmd, char **reply, int *len, void *data, long timeout_ms)
>  {
>  	int r;
>  	struct handler * h;
>  	vector cmdvec = NULL;
> -	struct timespec tmo;
>  
>  	r = get_cmdvec(cmd, &cmdvec);
>  
> @@ -481,22 +513,12 @@ parse_cmd (char * cmd, char ** reply, int * len, void * data, int timeout )
>  	/*
>  	 * execute handler
>  	 */
> -	if (clock_gettime(CLOCK_REALTIME, &tmo) == 0) {
> -		tmo.tv_sec += timeout;
> -	} else {
> -		tmo.tv_sec = 0;
> -	}
>  	if (h->locked) {
>  		int locked = 0;
>  		struct vectors * vecs = (struct vectors *)data;
>  
>  		pthread_cleanup_push(cleanup_lock, &vecs->lock);
> -		if (tmo.tv_sec) {
> -			r = timedlock(&vecs->lock, &tmo);
> -		} else {
> -			lock(&vecs->lock);
> -			r = 0;
> -		}
> +		r = cli_timedlock(&vecs->lock, timeout_ms);
>  		if (r == 0) {
>  			locked = 1;
>  			pthread_testcancel();
> diff --git a/multipathd/cli.h b/multipathd/cli.h
> index 7cc7e4be..fb20e0d2 100644
> --- a/multipathd/cli.h
> +++ b/multipathd/cli.h
> @@ -123,7 +123,7 @@ int alloc_handlers (void);
>  int add_handler (uint64_t fp, int (*fn)(void *, char **, int *, void *));
>  int set_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
>  int set_unlocked_handler_callback (uint64_t fp, int (*fn)(void *, char **, int *, void *));
> -int parse_cmd (char * cmd, char ** reply, int * len, void *, int);
> +int parse_cmd(char *cmd, char **reply, int *len, void *data, long timeout_ms);
>  int load_keys (void);
>  char * get_keyparam (vector v, uint64_t code);
>  void free_keys (vector vec);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6276d34c..9ed0cadd 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1395,11 +1395,13 @@ uxsock_trigger (char * str, char ** reply, int * len, bool is_root,
>  		return 1;
>  	}
>  
> -	r = parse_cmd(str, reply, len, vecs, uxsock_timeout / 1000);
> +	r = parse_cmd(str, reply, len, vecs, uxsock_timeout);
>  
>  	if (r > 0) {
>  		if (r == ETIMEDOUT)
>  			*reply = STRDUP("timeout\n");
> +		else if (r == EINTR)
> +			*reply = STRDUP("daemon exiting\n");
>  		else
>  			*reply = STRDUP("fail\n");
>  		if (*reply)
> -- 
> 2.19.2

Re: [RFC PATCH 1/6] multipathd: fix daemon not really shutdown

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:09PM +0100, Martin Wilck wrote:
> From: Chongyun Wu <wu.chongyun@h3c.com>
> 
> Test environment: 25 hosts, each host have more than 100 luns,
> each lun have two paths.
> Some times when we try to ceate new multipath will encounter "could
> not create uxsock:98" but the multipathd still running not shutdown
> and can't response any multipathd commands also.
> 
> After reproduce this issue and debug, found below fixes might work:
> (1) set_config_state() after pthread_cond_timedwait() other threads
> might changed the running_state from DAEMON_SHUTDOWN to other status
> like DAEMON_IDLE, which will make the shutdown process stopped.
> I found logs to prove this really happened, so we need add judgement
> here too.
> 
> (2) [this part removed by mwilck]
> 

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> Signed-off-by: Chongyun Wu <wu.chongyun@h3c.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 99145293..6a5d105a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -247,7 +247,7 @@ int set_config_state(enum daemon_status state)
>  			rc = pthread_cond_timedwait(&config_cond,
>  						    &config_lock, &ts);
>  		}
> -		if (!rc) {
> +		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
>  			running_state = state;
>  			pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -- 
> 2.19.2

Re: [RFC PATCH 2/6] multipathd: protect all access to running_state

[Benjamin Marzinski]

On Fri, Jan 04, 2019 at 06:59:10PM +0100, Martin Wilck wrote:
> Chonyun Wu's latest patch has shown that the handling of the daemon
> state variable running_state is racy and difficult to get right. It's
> not a good candidate for a "benign race" annotation. So, as a first
> step to sanitizing it, make sure all accesses to the state variable
> are protected by config_lock.
> 
> The patch also replaces "if" with "while" in several places where the
> code was supposed to wait until a certain state was reached. It's
> important that DAEMON_SHUTDOWN terminates all loops of this kind.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 79 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 31 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6a5d105a..6fc6a3ac 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -126,11 +126,21 @@ int poll_dmevents = 0;
>  #else
>  int poll_dmevents = 1;
>  #endif
> -enum daemon_status running_state = DAEMON_INIT;
> +enum daemon_status _running_state = DAEMON_INIT;
>  pid_t daemon_pid;
>  pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
>  pthread_cond_t config_cond;
>  
> +static inline enum daemon_status get_running_state(void)
> +{
> +	enum daemon_status st;
> +
> +	pthread_mutex_lock(&config_lock);
> +	st = _running_state;
> +	pthread_mutex_unlock(&config_lock);
> +	return st;
> +}
> +
>  /*
>   * global copy of vecs for use in sig handlers
>   */
> @@ -148,7 +158,7 @@ static volatile sig_atomic_t log_reset_sig;
>  const char *
>  daemon_status(void)
>  {
> -	switch (running_state) {
> +	switch (get_running_state()) {
>  	case DAEMON_INIT:
>  		return "init";
>  	case DAEMON_START:
> @@ -168,10 +178,10 @@ daemon_status(void)
>  /*
>   * I love you too, systemd ...
>   */
> -const char *
> -sd_notify_status(void)
> +static const char *
> +sd_notify_status(enum daemon_status state)
>  {
> -	switch (running_state) {
> +	switch (state) {
>  	case DAEMON_INIT:
>  		return "STATUS=init";
>  	case DAEMON_START:
> @@ -188,17 +198,18 @@ sd_notify_status(void)
>  }
>  
>  #ifdef USE_SYSTEMD
> -static void do_sd_notify(enum daemon_status old_state)
> +static void do_sd_notify(enum daemon_status old_state,
> +			 enum daemon_status new_state)
>  {
>  	/*
>  	 * Checkerloop switches back and forth between idle and running state.
>  	 * No need to tell systemd each time.
>  	 * These notifications cause a lot of overhead on dbus.
>  	 */
> -	if ((running_state == DAEMON_IDLE || running_state == DAEMON_RUNNING) &&
> +	if ((new_state == DAEMON_IDLE || new_state == DAEMON_RUNNING) &&
>  	    (old_state == DAEMON_IDLE || old_state == DAEMON_RUNNING))
>  		return;
> -	sd_notify(0, sd_notify_status());
> +	sd_notify(0, sd_notify_status(new_state));
>  }
>  #endif
>  
> @@ -207,15 +218,16 @@ static void config_cleanup(void *arg)
>  	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;
> +	if (state != _running_state && _running_state != DAEMON_SHUTDOWN) {
> +		enum daemon_status old_state = _running_state;
>  
> -		running_state = state;
> +		_running_state = state;
>  		pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -		do_sd_notify(old_state);
> +		do_sd_notify(old_state, state);
>  #endif
>  	}
>  }
> @@ -234,12 +246,12 @@ int set_config_state(enum daemon_status state)
>  
>  	pthread_cleanup_push(config_cleanup, NULL);
>  	pthread_mutex_lock(&config_lock);
> -	if (running_state != state) {
> -		enum daemon_status old_state = running_state;
> +	if (_running_state != state) {
> +		enum daemon_status old_state = _running_state;
>  
> -		if (running_state == DAEMON_SHUTDOWN)
> +		if (_running_state == DAEMON_SHUTDOWN)
>  			rc = EINVAL;
> -		else if (running_state != DAEMON_IDLE) {
> +		else if (_running_state != DAEMON_IDLE) {
>  			struct timespec ts;
>  
>  			clock_gettime(CLOCK_MONOTONIC, &ts);
> @@ -247,11 +259,11 @@ int set_config_state(enum daemon_status state)
>  			rc = pthread_cond_timedwait(&config_cond,
>  						    &config_lock, &ts);
>  		}
> -		if (!rc && (running_state != DAEMON_SHUTDOWN)) {
> -			running_state = state;
> +		if (!rc && (_running_state != DAEMON_SHUTDOWN)) {
> +			_running_state = state;
>  			pthread_cond_broadcast(&config_cond);
>  #ifdef USE_SYSTEMD
> -			do_sd_notify(old_state);
> +			do_sd_notify(old_state, state);
>  #endif
>  		}
>  	}
> @@ -1405,17 +1417,20 @@ uev_trigger (struct uevent * uev, void * trigger_data)
>  	int r = 0;
>  	struct vectors * vecs;
>  	struct uevent *merge_uev, *tmp;
> +	enum daemon_status state;
>  
>  	vecs = (struct vectors *)trigger_data;
>  
>  	pthread_cleanup_push(config_cleanup, NULL);
>  	pthread_mutex_lock(&config_lock);
> -	if (running_state != DAEMON_IDLE &&
> -	    running_state != DAEMON_RUNNING)
> +	while (_running_state != DAEMON_IDLE &&
> +	       _running_state != DAEMON_RUNNING &&
> +	       _running_state != DAEMON_SHUTDOWN)
>  		pthread_cond_wait(&config_cond, &config_lock);
> +	state = _running_state;
>  	pthread_cleanup_pop(1);
>  
> -	if (running_state == DAEMON_SHUTDOWN)
> +	if (state == DAEMON_SHUTDOWN)
>  		return 0;
>  
>  	/*
> @@ -2661,6 +2676,7 @@ child (void * param)
>  	struct config *conf;
>  	char *envp;
>  	int queue_without_daemon;
> +	enum daemon_status state;
>  
>  	mlockall(MCL_CURRENT | MCL_FUTURE);
>  	signal_init();
> @@ -2756,8 +2772,9 @@ child (void * param)
>  	rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
>  	if (!rc) {
>  		/* Wait for uxlsnr startup */
> -		while (running_state == DAEMON_IDLE)
> +		while (_running_state == DAEMON_IDLE)
>  			pthread_cond_wait(&config_cond, &config_lock);
> +		state = _running_state;
>  	}
>  	pthread_cleanup_pop(1);
>  
> @@ -2765,7 +2782,7 @@ child (void * param)
>  		condlog(0, "failed to create cli listener: %d", rc);
>  		goto failed;
>  	}
> -	else if (running_state != DAEMON_CONFIGURE) {
> +	else if (state != DAEMON_CONFIGURE) {
>  		condlog(0, "cli listener failed to start");
>  		goto failed;
>  	}
> @@ -2805,15 +2822,17 @@ child (void * param)
>  	}
>  	pthread_attr_destroy(&misc_attr);
>  
> -	while (running_state != DAEMON_SHUTDOWN) {
> +	while (1) {
>  		pthread_cleanup_push(config_cleanup, NULL);
>  		pthread_mutex_lock(&config_lock);
> -		if (running_state != DAEMON_CONFIGURE &&
> -		    running_state != DAEMON_SHUTDOWN) {
> +		while (_running_state != DAEMON_CONFIGURE &&
> +		       _running_state != DAEMON_SHUTDOWN)
>  			pthread_cond_wait(&config_cond, &config_lock);
> -		}
> +		state = _running_state;
>  		pthread_cleanup_pop(1);
> -		if (running_state == DAEMON_CONFIGURE) {
> +		if (state == DAEMON_SHUTDOWN)
> +			break;
> +		if (state == DAEMON_CONFIGURE) {
>  			pthread_cleanup_push(cleanup_lock, &vecs->lock);
>  			lock(&vecs->lock);
>  			pthread_testcancel();
> @@ -2983,8 +3002,6 @@ main (int argc, char *argv[])
>  
>  	ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
>  				   "Manipulated through RCU");
> -	ANNOTATE_BENIGN_RACE_SIZED(&running_state, sizeof(running_state),
> -		"Suppress complaints about unprotected running_state reads");
>  	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
>  		"Suppress complaints about this scalar variable");
>  
> -- 
> 2.19.2

Re: [RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Martin Wilck]

On Wed, 2019-01-16 at 17:40 -0600, Benjamin Marzinski wrote:
> On Fri, Jan 04, 2019 at 06:59:12PM +0100, Martin Wilck wrote:
> > Cancel the other threads before taking vecs->lock. This avoids
> > delays during shutdown caused e.g. by the checker thread holding
> > the vecs lock.
> 
> Before this change, multipathd was guaranteed that once a thread had
> locked the vecs lock, and checked if it had been cancelled, it could
> not
> be cancelled after that until it unlocked the vecs lock.  Undoing
> this
> guarantee will likely make it possible for multipathd to leak memory
> where it wasn't possible before. 

Thanks for pointing that out.

I wasn't aware of this guarantee. In my latest valgrind tests, valgrind
reported no leaks, but multipathd was also not "clean" in the sense
that every chunk of memory malloc()d had been explicitly free()d at
exit. IIRC that hadn't been caused by any patches added recently.
I haven't had time to look at that further, I was satisfied with no
real leaks being reported.

We do free the global data structures in "vecs" when we exit. So any
possible leaks caused by this patch must be cases where temporary
memory is allocated without proper pthread_cleanup_push(), or where a
thread was cancelled between allocation of memory and setting a
reference to it in the global data structures - e.g. between allocation
of a path and adding it to vecs->pathvec.

I haven't audited either class of leaks. I believe the first class
should have been eliminated by earlier phthread_cancel audits. Fixing
the second class would require designing some really clever helpers, I
guess. But as argued above, I really don't think it matters much if it
concerns only leaks-at-shutdown.

I'll put this on my todo list, but not at the highest prio.
For this RFC, we need to decide whether it's more important to be leak-
free on shutdown, or to react quickly on shutdown requests.

Martin
-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [RFC PATCH 5/6] multipathd: add code to handle blocked signals

[Martin Wilck]

On Wed, 2019-01-16 at 17:59 -0600, Benjamin Marzinski wrote:
> On Fri, Jan 04, 2019 at 06:59:13PM +0100, Martin Wilck wrote:
> > multipathd blocks all signals except in the uxlsnr thread.
> > Add some code to handle signals even while they're blocked.
> > 
> > If a shutdown signal is received, deliver_pending_signals() returns
> > TRUE. This is not the same as should_exit(), it's just a friendly
> > warning to the caller that shutdown/thread cancellation is
> > imminent.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/main.c | 25 +++++++++++++++++++++++++
> >  multipathd/main.h |  1 +
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 569a27ac..6276d34c 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2614,6 +2614,31 @@ signal_init(void)
> >  	signal_set(SIGPIPE, sigend);
> >  }
> >  
> > +bool deliver_pending_signals(void)
> > +{
> > +	sigset_t set;
> > +	bool ret = exit_sig;
> > +
> > +	if (sigpending(&set) != 0)
> > +		return false;
> 
> shouldn't this return ret instead of false?

OK, for tidyness. The only error code of sigpending() is EFAULT which
is unlikely to occur for stack memory, so the whole error check is a
bit pointless.

Thanks
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [RFC PATCH 6/6] multipathd: uxlsnr: handle signals while busy

[Martin Wilck]

On Wed, 2019-01-16 at 18:04 -0600, Benjamin Marzinski wrote:
> On Fri, Jan 04, 2019 at 06:59:14PM +0100, Martin Wilck wrote:
> > The uxlsnr thread is responsible for receiving and handling
> > signals in multipathd. This works well while it is idle and
> > waiting for connections in ppoll(). But if it's busy, cli commands
> > may need to take the vecs lock, which might take a long time.
> > 
> > Use multipathd's ability to handle pending signals to avoid
> > shutdown delays.
> > 
> > If we find a deadly signal pending, try to use the remaining cycles
> > to provide a meaningful error message to the client rather than
> > simply break the connection.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/cli.c  | 50 ++++++++++++++++++++++++++++++++++---------
> > ----
> >  multipathd/cli.h  |  2 +-
> >  multipathd/main.c |  4 +++-
> >  3 files changed, 40 insertions(+), 16 deletions(-)
> > 
> > diff --git a/multipathd/cli.c b/multipathd/cli.c
> > index a75afe3f..824c01b6 100644
> > --- a/multipathd/cli.c
> > +++ b/multipathd/cli.c
> > @@ -12,7 +12,7 @@
> >  #include "util.h"
> >  #include "version.h"
> >  #include <readline/readline.h>
> > -
> > +#include "main.h"
> >  #include "cli.h"
> >  
> >  static vector keys;
> > @@ -453,13 +453,45 @@ genhelp_handler (const char *cmd, int error)
> >  	return reply;
> >  }
> >  
> > +static int cli_timedlock(struct mutex_lock *a, long ms)
> > +{
> > +	struct timespec tmo;
> > +	const long delta_ms = 100;
> > +	const long MILLION = 1000L * 1000;
> > +	const long delta_ns = MILLION * delta_ms;
> > +	const long BILLION = 1000L * MILLION;
> > +	int r;
> > +
> > +	if (ms <= 0 || clock_gettime(CLOCK_REALTIME, &tmo) != 0) {
> > +		if (deliver_pending_signals())
> > +			return EINTR;
> 
> Why call pthread_testcancel() before pthread_mutex_lock()?
> 
> I realize that the cancels are no longer blocked by holding the vecs
> lock, but deliver_pending_signals() should have already told us if we
> are about to shutdown. If we're not, it's very unlikely that we will
> be
> cancelled between that check and the pthread_testcancel(). However,
> the
> pthread_mutex_lock() might take some time, so it makes more sense to
> check if we were cancelled after that. Or is there some other reason
> for
> the check being there?

Call it a habit - checking for cancellation before taking a possibly
blocking, long-running path. I call pthread_testcancel() again both
after return from cli_timedlock() and in each loop iteration before
calling pthread_mutex_timedlock(), so the cancellation test after
locking is in place. I agree that being cancelled in that piece of code
is very unlikely, but this is not about likelihood.

Actually, it's kind of pointless to call pthread_testcancel() in code
that's only executed in the context of the uxlsnr thread. Rather, we
should immediately exit the thread when handle_signal() or
deliver_pending_signals() tell us that shutdown is pending. Then
child() wouldn't need to cancel the uxlsnr at all any more.
I postponed that to after this patch set though.

Thanks,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [RFC PATCH 0/6] multipath-tools: Fix remaining shutdown delay issues

[Chongyun Wu]

Hi Martin,

Those patches works well and this issue not happened again, thanks.

On 2019/1/5 1:59, Martin Wilck wrote:
> Hi Chongyun, Ben, all,
> 
> this patch set addresses the points where I can see that handling of
> shutdown signals may be delayed, as discussed previously. Quoting my
> previous post:
> 
> Let's summarize how multipathd exit works today:
> 
>   1. signal arrives
>      (signal may be blocked while uxlsnr is busy, see above)
>   2. signal is unblocked in uxlsnr thread (in ppoll())
>   3. signal handler sets exit_sig()
>   4. uxlsnr calls handle_signals()
>   5. handle_signals()->exit_daemon() sets DAEMON_SHUTDOWN() and posts
> config_cond (child may busy in reconfigure())
>   6. child detects DAEMON_SHUTDOWN and quits main loop
>   7. child locks vecs->lock (may cause wait)
>   8. sets dm_queue_if_no_path, cancels threads, and exits.
> 
> I can imagine delays in step 1, 5, and 7, but not in ppoll().
> 
> This series addresses 1) in patch 5 and 6, 5) in patch 3, and 7) in patch 4.
> The series also contains the part of Chongyun's previously posted patch
> which I agree with.

Tested-by: Chongyun Wu <wu.chongyun@h3c.com>

> 
> The set is compile tested, but no more so far. Chongyun, I'd be grateful
> if you could review it, and give it a try in your test setup.
> 
> Chongyun Wu (1):
>    multipathd: fix daemon not really shutdown
> 
> Martin Wilck (5):
>    multipathd: protect all access to running_state
>    multipathd: allow shutdown during configure()
>    multipathd: cancel threads early during shutdown
>    multipathd: add code to handle blocked signals
>    multipathd: uxlsnr: handle signals while busy
> 
>   libmultipath/configure.c |   5 ++
>   libmultipath/discovery.c |   4 ++
>   libmultipath/exit.h      |   5 ++
>   mpathpersist/main.c      |   5 ++
>   multipath/main.c         |   6 ++
>   multipathd/cli.c         |  50 ++++++++++----
>   multipathd/cli.h         |   2 +-
>   multipathd/main.c        | 143 ++++++++++++++++++++++++++++-----------
>   multipathd/main.h        |   1 +
>   9 files changed, 166 insertions(+), 55 deletions(-)
>   create mode 100644 libmultipath/exit.h
> 

Re: [RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Martin Wilck]

Hi Ben,

I need to get back to this old discussion. I didn't resend this patch
last year, because I tried to figure out how to solve the memory leaks
you mentioned.

On Thu, 2019-01-17 at 10:59 +0100, Martin Wilck wrote:
> On Wed, 2019-01-16 at 17:40 -0600, Benjamin Marzinski wrote:
> > On Fri, Jan 04, 2019 at 06:59:12PM +0100, Martin Wilck wrote:
> > > Cancel the other threads before taking vecs->lock. This avoids
> > > delays during shutdown caused e.g. by the checker thread holding
> > > the vecs lock.
> > 
> > Before this change, multipathd was guaranteed that once a thread
> > had
> > locked the vecs lock, and checked if it had been cancelled, it
> > could
> > not
> > be cancelled after that until it unlocked the vecs lock.  Undoing
> > this
> > guarantee will likely make it possible for multipathd to leak
> > memory
> > where it wasn't possible before. 
> 
> Thanks for pointing that out.
> 
> I wasn't aware of this guarantee. In my latest valgrind tests,
> valgrind
> reported no leaks, but multipathd was also not "clean" in the sense
> that every chunk of memory malloc()d had been explicitly free()d at
> exit. IIRC that hadn't been caused by any patches added recently.
> I haven't had time to look at that further, I was satisfied with no
> real leaks being reported.
> 
> We do free the global data structures in "vecs" when we exit. So any
> possible leaks caused by this patch must be cases where temporary
> memory is allocated without proper pthread_cleanup_push(), or where a
> thread was cancelled between allocation of memory and setting a
> reference to it in the global data structures - e.g. between
> allocation
> of a path and adding it to vecs->pathvec.
> 
> I haven't audited either class of leaks. I believe the first class
> should have been eliminated by earlier phthread_cancel audits. Fixing
> the second class would require designing some really clever helpers,
> I
> guess. But as argued above, I really don't think it matters much if
> it
> concerns only leaks-at-shutdown.
> 
> I'll put this on my todo list, but not at the highest prio.
> For this RFC, we need to decide whether it's more important to be
> leak-
> free on shutdown, or to react quickly on shutdown requests.

Last year, I had started looking into this, and produced a first patch,
bca3729 ("libmultipath: alias.c: prepare for cancel-safe allocation").
I've just revisited this. I think what I did back then was broken.

 out:
+       pthread_cleanup_push(free, alias);
        fclose(f);
+       pthread_cleanup_pop(0);


is confusing and hard to read. When I just saw it, several months after
having written it myself, I thought it was a bug. Actually, it *is*
broken, as you pointed out already in 
https://www.redhat.com/archives/dm-devel/2019-October/msg00211.html,
because we need to avoid leaking the fd, too.

How would code look like that would protect both from the memory and fd
leak due to cancellation? Maybe like this:

char *get_user_friendly_alias(...)
{
        char *alias = NULL;
        ...

        /* simplified, we need a wrapper for fclose */
	pthread_cleanup_push(fclose, f);

	id = lookup_binding(f, wwid, &alias, prefix);
	if (id < 0)
                goto out_fclose;

	pthread_cleanup_push(free, alias);
	if (fflush(f) != 0) {
		condlog(0, "cannot fflush bindings file stream : %s",
			strerror(errno));
		free(alias);
		alias = NULL;
                goto out_free;
	} else if (can_write && !bindings_read_only && !alias)
		alias = allocate_binding(fd, wwid, id, prefix);

out_free:
        /* This is necessary to preserve nesting */
	pthread_cleanup_pop(0); /* free */
out_fclose:
	pthread_cleanup_pop(0); /* fclose */
        /* This is necessary because fclose() is a cancellation point */
	pthread_cleanup_push(free, alias);
	fclose(f);
	pthread_cleanup_pop(0); /* free */

        return alias;
}

I hope you concur that this is awfully ugly. Everyone is invited to
find a solution that doesn't require 3x pthread_cleanup_push()/pop(),
without completely rewriting the code.

IMO avoiding the fd leak is more important than avoiding the memory 
leak of "alias". I'm going to submit a patch that does exactly that.

In general: I think completely avoiding memory leaks in multithreaded
code that allows almost arbitrary cancellation is not a worthwhile
goal. After all, except for the waiter and checker threads,
all other threads are only cancelled when multipathd exits. Yes, this
makes it harder to assess potential memory leaks with valgrind, because
we can't easily distinguish real memory leaks from leaks caused by
cancellation. But that's about it, and I think the distinction is
actually not that hard, because the leaks caused by cancellation would
be sporadic, and wouldn't pile up during longer runs.

So, I propose not to go further in this direction. IOW, we shouldn't
write code like bca3729 any more. We don't have to avoid it at all cost
(for example, it's always good to link allocated memory to some global
data structure as soon as it makes sense to do so). But I think that
"pthread_cleanup_push(free, xyz)" is often not worth the code
uglification it causes. If it conflicts with other cleanup actions, and
can't be cleanly nested like above, we should definitely not do it.

Moreover, I believe that reacting quickly on cancellation / exit
requests is more important than avoiding cancellation-caused memory
leaks. Therefore I plan to resend the "cancel threads early" patch,
unless you come up with more strong reasons not to do so.

Another possibility to "fix" cancellation issues and get rid of ugly
pthread_cleanup_push() calls would be changing our cancellation policy
to PTHREAD_CANCEL_DISABLE, and check for cancellation only at certain
points during runtime (basically, before and after blocking / waiting
on something). But I don't think that's going to work well, for the
same reason - we'd run high risk to get those multipathd shutdown
issues back which we've overcome only recently.

Thoughts?

Martin

Re: [RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Benjamin Marzinski]

On Thu, Aug 20, 2020 at 10:39:19PM +0200, Martin Wilck wrote:
> Hi Ben,
> 
> I need to get back to this old discussion. I didn't resend this patch
> last year, because I tried to figure out how to solve the memory leaks
> you mentioned.
> 
> On Thu, 2019-01-17 at 10:59 +0100, Martin Wilck wrote:
> > On Wed, 2019-01-16 at 17:40 -0600, Benjamin Marzinski wrote:
> > > On Fri, Jan 04, 2019 at 06:59:12PM +0100, Martin Wilck wrote:
> > > > Cancel the other threads before taking vecs->lock. This avoids
> > > > delays during shutdown caused e.g. by the checker thread holding
> > > > the vecs lock.
> > > 
> > > Before this change, multipathd was guaranteed that once a thread
> > > had
> > > locked the vecs lock, and checked if it had been cancelled, it
> > > could
> > > not
> > > be cancelled after that until it unlocked the vecs lock.  Undoing
> > > this
> > > guarantee will likely make it possible for multipathd to leak
> > > memory
> > > where it wasn't possible before. 
> > 
> > Thanks for pointing that out.
> > 
> > I wasn't aware of this guarantee. In my latest valgrind tests,
> > valgrind
> > reported no leaks, but multipathd was also not "clean" in the sense
> > that every chunk of memory malloc()d had been explicitly free()d at
> > exit. IIRC that hadn't been caused by any patches added recently.
> > I haven't had time to look at that further, I was satisfied with no
> > real leaks being reported.
> > 
> > We do free the global data structures in "vecs" when we exit. So any
> > possible leaks caused by this patch must be cases where temporary
> > memory is allocated without proper pthread_cleanup_push(), or where a
> > thread was cancelled between allocation of memory and setting a
> > reference to it in the global data structures - e.g. between
> > allocation
> > of a path and adding it to vecs->pathvec.
> > 
> > I haven't audited either class of leaks. I believe the first class
> > should have been eliminated by earlier phthread_cancel audits. Fixing
> > the second class would require designing some really clever helpers,
> > I
> > guess. But as argued above, I really don't think it matters much if
> > it
> > concerns only leaks-at-shutdown.
> > 
> > I'll put this on my todo list, but not at the highest prio.
> > For this RFC, we need to decide whether it's more important to be
> > leak-
> > free on shutdown, or to react quickly on shutdown requests.
> 
> Last year, I had started looking into this, and produced a first patch,
> bca3729 ("libmultipath: alias.c: prepare for cancel-safe allocation").
> I've just revisited this. I think what I did back then was broken.
> 
>  out:
> +       pthread_cleanup_push(free, alias);
>         fclose(f);
> +       pthread_cleanup_pop(0);
> 
> 
> is confusing and hard to read. When I just saw it, several months after
> having written it myself, I thought it was a bug. Actually, it *is*
> broken, as you pointed out already in 
> https://www.redhat.com/archives/dm-devel/2019-October/msg00211.html,
> because we need to avoid leaking the fd, too.
> 
> How would code look like that would protect both from the memory and fd
> leak due to cancellation? Maybe like this:
> 
> char *get_user_friendly_alias(...)
> {
>         char *alias = NULL;
>         ...
> 
>         /* simplified, we need a wrapper for fclose */
> 	pthread_cleanup_push(fclose, f);
> 
> 	id = lookup_binding(f, wwid, &alias, prefix);
> 	if (id < 0)
>                 goto out_fclose;
> 
> 	pthread_cleanup_push(free, alias);
> 	if (fflush(f) != 0) {
> 		condlog(0, "cannot fflush bindings file stream : %s",
> 			strerror(errno));
> 		free(alias);
> 		alias = NULL;
>                 goto out_free;
> 	} else if (can_write && !bindings_read_only && !alias)
> 		alias = allocate_binding(fd, wwid, id, prefix);
> 
> out_free:
>         /* This is necessary to preserve nesting */
> 	pthread_cleanup_pop(0); /* free */
> out_fclose:
> 	pthread_cleanup_pop(0); /* fclose */
>         /* This is necessary because fclose() is a cancellation point */
> 	pthread_cleanup_push(free, alias);
> 	fclose(f);
> 	pthread_cleanup_pop(0); /* free */
> 
>         return alias;
> }
> 
> I hope you concur that this is awfully ugly. Everyone is invited to
> find a solution that doesn't require 3x pthread_cleanup_push()/pop(),
> without completely rewriting the code.

I assume you would do it something like:

static void free_ptr(void *arg)
{
	void *ptr;

	if (!arg)
		return

	ptr = *((void **) arg);
	if (ptr)
		free(ptr);
}

char *get_user_friendly_alias(...)
{
        char *alias = NULL;

	pthread_cleanup_push(free_ptr, &alias);
	...

	/* simplified, we need a wrapper for fclose */
	pthread_cleanup_push(fclose, f);
	...

out:
	pthread_cleanup_pop(1); /* fclose */
	pthread_cleanup_pop(0); /* free_ptr */
	return alias;
}
 
> IMO avoiding the fd leak is more important than avoiding the memory 
> leak of "alias". I'm going to submit a patch that does exactly that.
> 
> In general: I think completely avoiding memory leaks in multithreaded
> code that allows almost arbitrary cancellation is not a worthwhile
> goal. After all, except for the waiter and checker threads,
> all other threads are only cancelled when multipathd exits. Yes, this
> makes it harder to assess potential memory leaks with valgrind, because
> we can't easily distinguish real memory leaks from leaks caused by
> cancellation. But that's about it, and I think the distinction is
> actually not that hard, because the leaks caused by cancellation would
> be sporadic, and wouldn't pile up during longer runs.
> 
> So, I propose not to go further in this direction. IOW, we shouldn't
> write code like bca3729 any more. We don't have to avoid it at all cost
> (for example, it's always good to link allocated memory to some global
> data structure as soon as it makes sense to do so). But I think that
> "pthread_cleanup_push(free, xyz)" is often not worth the code
> uglification it causes. If it conflicts with other cleanup actions, and
> can't be cleanly nested like above, we should definitely not do it.

Sure. For threads that are only cancelled for shutdown, I'm fine with
reasonable effort, balanced against other code considerations.
 
> Moreover, I believe that reacting quickly on cancellation / exit
> requests is more important than avoiding cancellation-caused memory
> leaks. Therefore I plan to resend the "cancel threads early" patch,
> unless you come up with more strong reasons not to do so.

go ahead.

> Another possibility to "fix" cancellation issues and get rid of ugly
> pthread_cleanup_push() calls would be changing our cancellation policy
> to PTHREAD_CANCEL_DISABLE, and check for cancellation only at certain
> points during runtime (basically, before and after blocking / waiting
> on something). But I don't think that's going to work well, for the
> same reason - we'd run high risk to get those multipathd shutdown
> issues back which we've overcome only recently.

I'm not a fan either. I'd rather deal with occasionally seeing fake
memory leaks.

-Ben

> Thoughts?
> 
> Martin
> 
> 
> 

Re: [RFC PATCH 4/6] multipathd: cancel threads early during shutdown

[Martin Wilck]

On Fri, 2020-08-21 at 17:22 -0500, Benjamin Marzinski wrote:
> On Thu, Aug 20, 2020 at 10:39:19PM +0200, Martin Wilck wrote:
> > Hi Ben,
> > 
> > I need to get back to this old discussion. I didn't resend this
> > patch
> > last year, because I tried to figure out how to solve the memory
> > leaks
> > you mentioned.
> > 
> > On Thu, 2019-01-17 at 10:59 +0100, Martin Wilck wrote:
> > > On Wed, 2019-01-16 at 17:40 -0600, Benjamin Marzinski wrote:
> > > > On Fri, Jan 04, 2019 at 06:59:12PM +0100, Martin Wilck wrote:
> > > > > Cancel the other threads before taking vecs->lock. This
> > > > > avoids
> > > > > delays during shutdown caused e.g. by the checker thread
> > > > > holding
> > > > > the vecs lock.
> > > > 
> > > > Before this change, multipathd was guaranteed that once a
> > > > thread
> > > > had
> > > > locked the vecs lock, and checked if it had been cancelled, it
> > > > could
> > > > not
> > > > be cancelled after that until it unlocked the vecs
> > > > lock.  Undoing
> > > > this
> > > > guarantee will likely make it possible for multipathd to leak
> > > > memory
> > > > where it wasn't possible before. 
> > > 
> > > Thanks for pointing that out.
> > > 
> > > I wasn't aware of this guarantee. In my latest valgrind tests,
> > > valgrind
> > > reported no leaks, but multipathd was also not "clean" in the
> > > sense
> > > that every chunk of memory malloc()d had been explicitly free()d
> > > at
> > > exit. IIRC that hadn't been caused by any patches added recently.
> > > I haven't had time to look at that further, I was satisfied with
> > > no
> > > real leaks being reported.
> > > 
> > > We do free the global data structures in "vecs" when we exit. So
> > > any
> > > possible leaks caused by this patch must be cases where temporary
> > > memory is allocated without proper pthread_cleanup_push(), or
> > > where a
> > > thread was cancelled between allocation of memory and setting a
> > > reference to it in the global data structures - e.g. between
> > > allocation
> > > of a path and adding it to vecs->pathvec.
> > > 
> > > I haven't audited either class of leaks. I believe the first
> > > class
> > > should have been eliminated by earlier phthread_cancel audits.
> > > Fixing
> > > the second class would require designing some really clever
> > > helpers,
> > > I
> > > guess. But as argued above, I really don't think it matters much
> > > if
> > > it
> > > concerns only leaks-at-shutdown.
> > > 
> > > I'll put this on my todo list, but not at the highest prio.
> > > For this RFC, we need to decide whether it's more important to be
> > > leak-
> > > free on shutdown, or to react quickly on shutdown requests.
> > 
> > Last year, I had started looking into this, and produced a first
> > patch,
> > bca3729 ("libmultipath: alias.c: prepare for cancel-safe
> > allocation").
> > I've just revisited this. I think what I did back then was broken.
> > 
> >  out:
> > +       pthread_cleanup_push(free, alias);
> >         fclose(f);
> > +       pthread_cleanup_pop(0);
> > 
> > 
> > is confusing and hard to read. When I just saw it, several months
> > after
> > having written it myself, I thought it was a bug. Actually, it *is*
> > broken, as you pointed out already in 
> > https://www.redhat.com/archives/dm-devel/2019-October/msg00211.html
> > ,
> > because we need to avoid leaking the fd, too.
> > 
> > How would code look like that would protect both from the memory
> > and fd
> > leak due to cancellation? Maybe like this:
> > 
> > char *get_user_friendly_alias(...)
> > {
> >         char *alias = NULL;
> >         ...
> > 
> >         /* simplified, we need a wrapper for fclose */
> > 	pthread_cleanup_push(fclose, f);
> > 
> > 	id = lookup_binding(f, wwid, &alias, prefix);
> > 	if (id < 0)
> >                 goto out_fclose;
> > 
> > 	pthread_cleanup_push(free, alias);
> > 	if (fflush(f) != 0) {
> > 		condlog(0, "cannot fflush bindings file stream : %s",
> > 			strerror(errno));
> > 		free(alias);
> > 		alias = NULL;
> >                 goto out_free;
> > 	} else if (can_write && !bindings_read_only && !alias)
> > 		alias = allocate_binding(fd, wwid, id, prefix);
> > 
> > out_free:
> >         /* This is necessary to preserve nesting */
> > 	pthread_cleanup_pop(0); /* free */
> > out_fclose:
> > 	pthread_cleanup_pop(0); /* fclose */
> >         /* This is necessary because fclose() is a cancellation
> > point */
> > 	pthread_cleanup_push(free, alias);
> > 	fclose(f);
> > 	pthread_cleanup_pop(0); /* free */
> > 
> >         return alias;
> > }
> > 
> > I hope you concur that this is awfully ugly. Everyone is invited to
> > find a solution that doesn't require 3x
> > pthread_cleanup_push()/pop(),
> > without completely rewriting the code.
> 
> I assume you would do it something like:
> 
> static void free_ptr(void *arg)
> {
> 	void *ptr;
> 
> 	if (!arg)
> 		return
> 
> 	ptr = *((void **) arg);
> 	if (ptr)
> 		free(ptr);
> }
> 
> char *get_user_friendly_alias(...)
> {
>         char *alias = NULL;
> 
> 	pthread_cleanup_push(free_ptr, &alias);
> 	...
> 
> 	/* simplified, we need a wrapper for fclose */
> 	pthread_cleanup_push(fclose, f);
> 	...
> 
> out:
> 	pthread_cleanup_pop(1); /* fclose */
> 	pthread_cleanup_pop(0); /* free_ptr */
> 	return alias;
> }Regards

Hey, why didn't this occur to me? I literally shifted this code
back and forth for hours :-)

This actually looks acceptible, but...

>  
> > IMO avoiding the fd leak is more important than avoiding the
> > memory 
> > leak of "alias". I'm going to submit a patch that does exactly
> > that.
> > 
> > In general: I think completely avoiding memory leaks in
> > multithreaded
> > code that allows almost arbitrary cancellation is not a worthwhile
> > goal. After all, except for the waiter and checker threads,
> > all other threads are only cancelled when multipathd exits. Yes,
> > this
> > makes it harder to assess potential memory leaks with valgrind,
> > because
> > we can't easily distinguish real memory leaks from leaks caused by
> > cancellation. But that's about it, and I think the distinction is
> > actually not that hard, because the leaks caused by cancellation
> > would
> > be sporadic, and wouldn't pile up during longer runs.
> > 
> > So, I propose not to go further in this direction. IOW, we
> > shouldn't
> > write code like bca3729 any more. We don't have to avoid it at all
> > cost
> > (for example, it's always good to link allocated memory to some
> > global
> > data structure as soon as it makes sense to do so). But I think
> > that
> > "pthread_cleanup_push(free, xyz)" is often not worth the code
> > uglification it causes. If it conflicts with other cleanup actions,
> > and
> > can't be cleanly nested like above, we should definitely not do it.
> 
> Sure. For threads that are only cancelled for shutdown, I'm fine with
> reasonable effort, balanced against other code considerations.

... we seem to be basically on the same boat here. Great!

Thanks,
Martin

>  
> > Moreover, I believe that reacting quickly on cancellation / exit
> > requests is more important than avoiding cancellation-caused memory
> > leaks. Therefore I plan to resend the "cancel threads early" patch,
> > unless you come up with more strong reasons not to do so.
> 
> go ahead.
> 
> > Another possibility to "fix" cancellation issues and get rid of
> > ugly
> > pthread_cleanup_push() calls would be changing our cancellation
> > policy
> > to PTHREAD_CANCEL_DISABLE, and check for cancellation only at
> > certain
> > points during runtime (basically, before and after blocking /
> > waiting
> > on something). But I don't think that's going to work well, for the
> > same reason - we'd run high risk to get those multipathd shutdown
> > issues back which we've overcome only recently.
> 
> I'm not a fan either. I'd rather deal with occasionally seeing fake
> memory leaks.
> 
> -Ben
> 
> > Thoughts?
> > 
> > Martin
> > 
> > 
> >