DM-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit
@ 2020-10-16 10:44 mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock " mwilck
                   ` (28 more replies)
  0 siblings, 29 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben, hi lixiaokeng,

this series was inspired by lixiaokeng's recent posting "[QUESTION] memory
leak in main (multipath)". It implements my first idea, registering
cleanup handlers with atexit(). However it turned out to be quite
complex. In particular multipathd has a lot of things to clean up.

This series is based on the previous series "multipath-tools: shutdown, 
libdevmapper races, globals" (v3).

While the bulk of the series is the cleanup handling, it also contains
some bug fixes for issues that I found while working on this.

Changes v1 -> v2:

Patches 24..29 have been added; they are logging cleanups, 24/29
and 29/29 are fixes to issues Ben had mentioned during his review.

01/29 "multipathd: uxlsnr: avoid deadlock on exit"
      Fix invalid array element reference (Ben)
07/29 "multipathd: move conf destruction into separate function"
      Don't reset logink when the log thread is stopped (Ben)
      Logging via logsink after shutting down the log thread is racy
      (no worse than before); the race will be fixed in 29/29.
14/29 "libmultipath: add libmp_dm_exit()": Fixed the skip_libmp_dm_init()
      case (Ben).
18/29 "libmultipath: fix log_thread startup and teardown"
      No need to wait before joining the log thread in log_thread_stop() (Ben)
      -> "libmultipath: fix race between log_safe and log_thread_stop()"
23/29: "multipath-tools: mpath-tools.supp: file with valgrind suppressions"
       Remove empty line at end of file (Ben)
24/29 "libmultipath: use libmp_verbosity to track verbosity"
       This fixes 13/21 "libmultipath: provide defaults for {get,put}_multipath_config"
       (from the previous patch series "multipath-tools: shutdown, libdevmapper races, globals";
       control of verbosity during config file loading)
29/29 "libmultipath: fix race between log_safe and log_thread_stop()"
      (see 18/29)

Regards
Martin

Martin Wilck (29):
  multipathd: uxlsnr: avoid deadlock on exit
  multipathd: Fix liburcu memory leak
  multipathd: move handling of io_err_stat_attr into libmultipath
  multipathd: move vecs desctruction into cleanup function
  multipathd: make some globals static
  multipathd: move threads destruction into separate function
  multipathd: move conf destruction into separate function
  multipathd: move pid destruction into separate function
  multipathd: close pidfile on exit
  multipathd: add helper for systemd notification at exit
  multipathd: child(): call cleanups in failure case, too
  multipathd: unwatch_all_dmevents: check if waiter is initialized
  multipathd: print error message if config can't be loaded
  libmultipath: add libmp_dm_exit()
  multipathd: fixup libdm deinitialization
  libmultipath: log_thread_stop(): check if logarea is initialized
  multipathd: add cleanup_child() exit handler
  libmultipath: fix log_thread startup and teardown
  multipathd: move cleanup_{prio,checkers,foreign} to libmultipath_exit
  multipath: use atexit() for cleanup handlers
  mpathpersist: use atexit() for cleanup handlers
  multipath: fix leaks in check_path_valid()
  multipath-tools: mpath-tools.supp: file with valgrind suppressions
  libmultipath: use libmp_verbosity to track verbosity
  libmultipath: introduce symbolic values for logsink
  libmultipath: simplify dlog()
  multipathd: common code for "-k" and command args
  multipathd: sanitize uxsock_listen()
  libmultipath: fix race between log_safe and log_thread_stop()

 libmpathpersist/mpath_persist.c       |   7 +-
 libmultipath/config.c                 |  14 +-
 libmultipath/config.h                 |   2 +
 libmultipath/debug.c                  |  38 +--
 libmultipath/debug.h                  |  27 +-
 libmultipath/devmapper.c              |  23 +-
 libmultipath/devmapper.h              |   1 +
 libmultipath/io_err_stat.c            |   7 +-
 libmultipath/libmultipath.version     |  31 +-
 libmultipath/log_pthread.c            | 114 +++++---
 mpathpersist/main.c                   |   5 +-
 multipath/main.c                      | 119 ++++----
 multipathd/dmevents.c                 |   2 +
 multipathd/main.c                     | 396 ++++++++++++++++----------
 multipathd/uxlsnr.c                   |  80 ++++--
 tests/alias.c                         |   1 +
 tests/blacklist.c                     |   2 +
 tests/globals.c                       |   3 +-
 tests/hwtable.c                       |   2 +-
 tests/test-log.c                      |   4 +-
 tests/test-log.h                      |   3 +-
 third-party/valgrind/mpath-tools.supp |  32 +++
 22 files changed, 585 insertions(+), 328 deletions(-)
 create mode 100644 third-party/valgrind/mpath-tools.supp

-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock on exit
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-20 19:04   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 02/29] multipathd: Fix liburcu memory leak mwilck
                   ` (27 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The uxlsnr wouldn't always release the client lock when cancelled,
causing a deadlock in uxsock_cleanup(). While this hasn't been
caused by commit 3d611a2, the deadlock seems to have become much
more likely after that patch. Solving this means that we have to
treat reallocation failure of the pollfd array differently.
We will now just ignore any clients above the last valid pfd index.
That's a minor problem, as we're in an OOM situation anyway.

Moreover, client_lock is not a "struct lock", but a plain
pthread_mutex_t.

Fixes: 3d611a2 ("multipathd: cancel threads early during shutdown")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index 1c5ce9d..ce2b680 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -35,6 +35,7 @@
 #include "config.h"
 #include "mpath_cmd.h"
 #include "time-util.h"
+#include "util.h"
 
 #include "main.h"
 #include "cli.h"
@@ -116,7 +117,7 @@ static void _dead_client(struct client *c)
 
 static void dead_client(struct client *c)
 {
-	pthread_cleanup_push(cleanup_lock, &client_lock);
+	pthread_cleanup_push(cleanup_mutex, &client_lock);
 	pthread_mutex_lock(&client_lock);
 	_dead_client(c);
 	pthread_cleanup_pop(1);
@@ -302,10 +303,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 	sigdelset(&mask, SIGUSR1);
 	while (1) {
 		struct client *c, *tmp;
-		int i, poll_count, num_clients;
+		int i, n_pfds, poll_count, num_clients;
 
 		/* setup for a poll */
 		pthread_mutex_lock(&client_lock);
+		pthread_cleanup_push(cleanup_mutex, &client_lock);
 		num_clients = 0;
 		list_for_each_entry(c, &clients, node) {
 			num_clients++;
@@ -322,14 +324,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 						sizeof(struct pollfd));
 			}
 			if (!new) {
-				pthread_mutex_unlock(&client_lock);
 				condlog(0, "%s: failed to realloc %d poll fds",
 					"uxsock", 2 + num_clients);
-				sched_yield();
-				continue;
+				num_clients = old_clients;
+			} else {
+				old_clients = num_clients;
+				polls = new;
 			}
-			old_clients = num_clients;
-			polls = new;
 		}
 		polls[0].fd = ux_sock;
 		polls[0].events = POLLIN;
@@ -347,11 +348,14 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 			polls[i].fd = c->fd;
 			polls[i].events = POLLIN;
 			i++;
+			if (i >= 2 + num_clients)
+				break;
 		}
-		pthread_mutex_unlock(&client_lock);
+		n_pfds = i;
+		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, i, &sleep_time, &mask);
+		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -384,7 +388,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		}
 
 		/* see if a client wants to speak to us */
-		for (i = 2; i < num_clients + 2; i++) {
+		for (i = 2; i < n_pfds; i++) {
 			if (polls[i].revents & POLLIN) {
 				struct timespec start_time;
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 02/29] multipathd: Fix liburcu memory leak
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock " mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 03/29] multipathd: move handling of io_err_stat_attr into libmultipath mwilck
                   ` (26 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Fix this leak in multipathd, reported by valgrind, that messes up
multipathd's otherwise clean leak report:

==23823== 336 bytes in 1 blocks are possibly lost in loss record 3 of 3
==23823==    at 0x483AB65: calloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==23823==    by 0x4012F16: _dl_allocate_tls (in /lib64/ld-2.31.so)
==23823==    by 0x493BB8E: pthread_create@@GLIBC_2.2.5 (in /lib64/libpthread-2.31.so)
==23823==    by 0x492A9A9: call_rcu_data_init (urcu-call-rcu-impl.h:437)
==23823==    by 0x492AD2F: UnknownInlinedFun (urcu-call-rcu-impl.h:492)
==23823==    by 0x492AD2F: create_call_rcu_data_memb (urcu-call-rcu-impl.h:504)
==23823==    by 0x1164E3: child.constprop.0.isra.0 (main.c:2915)
==23823==    by 0x10F50C: main (main.c:3335)
==23823==
==23823== LEAK SUMMARY:
==23823==    definitely lost: 0 bytes in 0 blocks
==23823==    indirectly lost: 0 bytes in 0 blocks
==23823==      possibly lost: 336 bytes in 1 blocks

The problem is caused by using liburcu's default RCU call handler,
which liburcu refuses to stop/join. See comments in the code.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index c5c374b..be1b5ae 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2889,6 +2889,47 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+/*
+ * Use a non-default call_rcu_data for child().
+ *
+ * We do this to avoid a memory leak from liburcu.
+ * liburcu never frees the default rcu handler (see comments on
+ * call_rcu_data_free() in urcu-call-rcu-impl.h), its thread
+ * can't be joined with pthread_join(), leaving a memory leak.
+ *
+ * Therefore we create our own, which can be destroyed and joined.
+ */
+static struct call_rcu_data *setup_rcu(void)
+{
+	struct call_rcu_data *crdp;
+
+	rcu_init();
+	rcu_register_thread();
+	crdp = create_call_rcu_data(0UL, -1);
+	if (crdp != NULL)
+		set_thread_call_rcu_data(crdp);
+	return crdp;
+}
+
+static void cleanup_rcu(int dummy __attribute__((unused)), void *arg)
+{
+	struct call_rcu_data *crdp = arg;
+	pthread_t rcu_thread;
+
+	/* Wait for any pending RCU calls */
+	rcu_barrier();
+	if (crdp != NULL) {
+		rcu_thread = get_call_rcu_thread(crdp);
+		/* detach this thread from the RCU thread */
+		set_thread_call_rcu_data(NULL);
+		synchronize_rcu();
+		/* tell RCU thread to exit */
+		call_rcu_data_free(crdp);
+		pthread_join(rcu_thread, NULL);
+	}
+	rcu_unregister_thread();
+}
+
 static int
 child (__attribute__((unused)) void *param)
 {
@@ -2903,10 +2944,12 @@ child (__attribute__((unused)) void *param)
 	char *envp;
 	int queue_without_daemon;
 	enum daemon_status state;
+	struct call_rcu_data *crdp;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
-	rcu_init();
+	crdp = setup_rcu();
+	on_exit(cleanup_rcu, crdp);
 
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 03/29] multipathd: move handling of io_err_stat_attr into libmultipath
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock " mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 02/29] multipathd: Fix liburcu memory leak mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 04/29] multipathd: move vecs desctruction into cleanup function mwilck
                   ` (25 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This thread attribute can be dynamically initialized and destroyed.
No need to carry it along in multipathd. Removal of the symbol
requires to bump the ABI version to 3.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/io_err_stat.c        |  7 +++++--
 libmultipath/libmultipath.version | 23 ++++++++---------------
 multipathd/main.c                 |  2 --
 3 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/libmultipath/io_err_stat.c b/libmultipath/io_err_stat.c
index 58bc1dd..5363049 100644
--- a/libmultipath/io_err_stat.c
+++ b/libmultipath/io_err_stat.c
@@ -34,6 +34,7 @@
 #include "lock.h"
 #include "time-util.h"
 #include "io_err_stat.h"
+#include "util.h"
 
 #define TIMEOUT_NO_IO_NSEC		10000000 /*10ms = 10000000ns*/
 #define FLAKY_PATHFAIL_THRESHOLD	2
@@ -70,8 +71,7 @@ struct io_err_stat_path {
 	int		err_rate_threshold;
 };
 
-pthread_t		io_err_stat_thr;
-pthread_attr_t		io_err_stat_attr;
+static pthread_t	io_err_stat_thr;
 
 static pthread_mutex_t io_err_thread_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t io_err_thread_cond = PTHREAD_COND_INITIALIZER;
@@ -727,6 +727,7 @@ static void *io_err_stat_loop(void *data)
 int start_io_err_stat_thread(void *data)
 {
 	int ret;
+	pthread_attr_t io_err_stat_attr;
 
 	if (uatomic_read(&io_err_thread_running) == 1)
 		return 0;
@@ -739,6 +740,7 @@ int start_io_err_stat_thread(void *data)
 	if (!paths)
 		goto destroy_ctx;
 
+	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0);
 	pthread_mutex_lock(&io_err_thread_lock);
 	pthread_cleanup_push(cleanup_unlock, &io_err_thread_lock);
 
@@ -750,6 +752,7 @@ int start_io_err_stat_thread(void *data)
 				 &io_err_thread_lock) == 0);
 
 	pthread_cleanup_pop(1);
+	pthread_attr_destroy(&io_err_stat_attr);
 
 	if (ret) {
 		io_err_stat_log(0, "cannot create io_error statistic thread");
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 0c300c8..84beb7f 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_2.0.0 {
+LIBMULTIPATH_3.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -121,7 +121,6 @@ global:
 	init_checkers;
 	init_foreign;
 	init_prio;
-	io_err_stat_attr;
 	io_err_stat_handle_pathfail;
 	is_path_valid;
 	is_quote;
@@ -242,30 +241,24 @@ global:
 	free_scandir_result;
 	sysfs_attr_get_value;
 
-local:
-	*;
-};
-
-LIBMULTIPATH_2.1.0 {
-global:
+	/* added in 2.1.0 */
 	libmp_dm_task_run;
 	cleanup_mutex;
-} LIBMULTIPATH_2.0.0;
 
-LIBMULTIPATH_2.2.0 {
-global:
+	/* added in 2.2.0 */
 	libmp_get_multipath_config;
 	get_multipath_config;
 	libmp_put_multipath_config;
 	put_multipath_config;
 	init_config;
 	uninit_config;
-} LIBMULTIPATH_2.1.0;
 
-LIBMULTIPATH_2.3.0 {
-global:
+	/* added in 2.3.0 */
 	udev;
 	logsink;
 	libmultipath_init;
 	libmultipath_exit;
-} LIBMULTIPATH_2.2.0;
+
+local:
+	*;
+};
diff --git a/multipathd/main.c b/multipathd/main.c
index be1b5ae..4d714e8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2954,7 +2954,6 @@ child (__attribute__((unused)) void *param)
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
-	setup_thread_attr(&io_err_stat_attr, 32 * 1024, 0);
 
 	if (logsink == 1) {
 		setup_thread_attr(&log_attr, 64 * 1024, 0);
@@ -3164,7 +3163,6 @@ child (__attribute__((unused)) void *param)
 	rcu_assign_pointer(multipath_conf, NULL);
 	call_rcu(&conf->rcu, rcu_free_config);
 	pthread_attr_destroy(&waiter_attr);
-	pthread_attr_destroy(&io_err_stat_attr);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 04/29] multipathd: move vecs desctruction into cleanup function
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (2 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 03/29] multipathd: move handling of io_err_stat_attr into libmultipath mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 05/29] multipathd: make some globals static mwilck
                   ` (24 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This will make it easer to move the stuff around later.
The only functional change is that map destuction now happens after
joining all threads, which should actually improve robustness.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 64 +++++++++++++++++++++++++++++------------------
 1 file changed, 40 insertions(+), 24 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4d714e8..2642570 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -148,7 +148,7 @@ int should_exit(void)
 /*
  * global copy of vecs for use in sig handlers
  */
-struct vectors * gvecs;
+static struct vectors * gvecs;
 
 struct config *multipath_conf;
 
@@ -2889,6 +2889,44 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_maps(struct vectors *vecs)
+{
+	int queue_without_daemon, i;
+	struct multipath *mpp;
+	struct config *conf;
+
+	conf = get_multipath_config();
+	queue_without_daemon = conf->queue_without_daemon;
+	put_multipath_config(conf);
+	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);
+	vecs->mpvec = NULL;
+}
+
+static void cleanup_paths(struct vectors *vecs)
+{
+	free_pathvec(vecs->pathvec, FREE_PATHS);
+	vecs->pathvec = NULL;
+}
+
+static void cleanup_vecs(void)
+{
+	if (!gvecs)
+		return;
+	/*
+	 * We can't take the vecs lock here, because exit() may
+	 * have been called from the child() thread, holding the lock already.
+	 * Anyway, by the time we get here, all threads that might access
+	 * vecs should have been joined already (in cleanup_threads).
+	 */
+	cleanup_maps(gvecs);
+	cleanup_paths(gvecs);
+	pthread_mutex_destroy(&gvecs->lock.mutex);
+	FREE(gvecs);
+}
+
 /*
  * Use a non-default call_rcu_data for child().
  *
@@ -2936,13 +2974,10 @@ child (__attribute__((unused)) void *param)
 	pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
-	struct multipath * mpp;
-	int i;
 	int rc;
 	int pid_fd = -1;
 	struct config *conf;
 	char *envp;
-	int queue_without_daemon;
 	enum daemon_status state;
 	struct call_rcu_data *crdp;
 
@@ -3108,17 +3143,6 @@ child (__attribute__((unused)) void *param)
 	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_join(check_thr, NULL);
 	pthread_join(uevent_thr, NULL);
 	pthread_join(uxlsnr_thr, NULL);
@@ -3128,15 +3152,7 @@ child (__attribute__((unused)) void *param)
 
 	stop_io_err_stat_thread();
 
-	lock(&vecs->lock);
-	free_pathvec(vecs->pathvec, FREE_PATHS);
-	vecs->pathvec = NULL;
-	unlock(&vecs->lock);
-
-	pthread_mutex_destroy(&vecs->lock.mutex);
-	FREE(vecs);
-	vecs = NULL;
-
+	cleanup_vecs();
 	cleanup_foreign();
 	cleanup_checkers();
 	cleanup_prio();
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 05/29] multipathd: make some globals static
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (3 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 04/29] multipathd: move vecs desctruction into cleanup function mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 06/29] multipathd: move threads destruction into separate function mwilck
                   ` (23 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 2642570..1cdbff1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -116,19 +116,19 @@ struct mpath_event_param
 };
 
 int uxsock_timeout;
-int verbosity;
-int bindings_read_only;
+static int verbosity;
+static int bindings_read_only;
 int ignore_new_devs;
 #ifdef NO_DMEVENTS_POLL
-int poll_dmevents = 0;
+static int poll_dmevents = 0;
 #else
-int poll_dmevents = 1;
+static int poll_dmevents = 1;
 #endif
 /* Don't access this variable without holding config_lock */
-volatile enum daemon_status running_state = DAEMON_INIT;
+static volatile 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 pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t config_cond;
 
 static inline enum daemon_status get_running_state(void)
 {
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 06/29] multipathd: move threads destruction into separate function
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (4 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 05/29] multipathd: make some globals static mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 07/29] multipathd: move conf " mwilck
                   ` (22 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Also, introduce booleans that indicate a certain thread has
been started successfully. Using these booleans, we can avoid
crashing by cancelling threads that have never been started.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 76 +++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 25 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1cdbff1..8b9df55 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -129,6 +129,9 @@ static volatile enum daemon_status running_state = DAEMON_INIT;
 pid_t daemon_pid;
 static pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t config_cond;
+static pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
+static bool check_thr_started, uevent_thr_started, uxlsnr_thr_started,
+	uevq_thr_started, dmevent_thr_started;
 
 static inline enum daemon_status get_running_state(void)
 {
@@ -2927,6 +2930,39 @@ static void cleanup_vecs(void)
 	FREE(gvecs);
 }
 
+static void cleanup_threads(void)
+{
+	stop_io_err_stat_thread();
+
+	if (check_thr_started)
+		pthread_cancel(check_thr);
+	if (uevent_thr_started)
+		pthread_cancel(uevent_thr);
+	if (uxlsnr_thr_started)
+		pthread_cancel(uxlsnr_thr);
+	if (uevq_thr_started)
+		pthread_cancel(uevq_thr);
+	if (dmevent_thr_started)
+		pthread_cancel(dmevent_thr);
+
+	if (check_thr_started)
+		pthread_join(check_thr, NULL);
+	if (uevent_thr_started)
+		pthread_join(uevent_thr, NULL);
+	if (uxlsnr_thr_started)
+		pthread_join(uxlsnr_thr, NULL);
+	if (uevq_thr_started)
+		pthread_join(uevq_thr, NULL);
+	if (dmevent_thr_started)
+		pthread_join(dmevent_thr, NULL);
+
+	/*
+	 * As all threads are joined now, and we're in DAEMON_SHUTDOWN
+	 * state, no new waiter threads will be created any more.
+	 */
+	pthread_attr_destroy(&waiter_attr);
+}
+
 /*
  * Use a non-default call_rcu_data for child().
  *
@@ -2971,7 +3007,6 @@ static void cleanup_rcu(int dummy __attribute__((unused)), void *arg)
 static int
 child (__attribute__((unused)) void *param)
 {
-	pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
 	int rc;
@@ -3070,9 +3105,12 @@ child (__attribute__((unused)) void *param)
 		condlog(0, "failed to create cli listener: %d", rc);
 		goto failed;
 	}
-	else if (state != DAEMON_CONFIGURE) {
-		condlog(0, "cli listener failed to start");
-		goto failed;
+	else {
+		uxlsnr_thr_started = true;
+		if (state != DAEMON_CONFIGURE) {
+			condlog(0, "cli listener failed to start");
+			goto failed;
+		}
 	}
 
 	if (poll_dmevents) {
@@ -3085,7 +3123,8 @@ child (__attribute__((unused)) void *param)
 			condlog(0, "failed to create dmevent waiter thread: %d",
 				rc);
 			goto failed;
-		}
+		} else
+			dmevent_thr_started = true;
 	}
 
 	/*
@@ -3094,7 +3133,8 @@ child (__attribute__((unused)) void *param)
 	if ((rc = pthread_create(&uevent_thr, &uevent_attr, ueventloop, udev))) {
 		condlog(0, "failed to create uevent thread: %d", rc);
 		goto failed;
-	}
+	} else
+		uevent_thr_started = true;
 	pthread_attr_destroy(&uevent_attr);
 
 	/*
@@ -3103,11 +3143,13 @@ child (__attribute__((unused)) void *param)
 	if ((rc = pthread_create(&check_thr, &misc_attr, checkerloop, vecs))) {
 		condlog(0,"failed to create checker loop thread: %d", rc);
 		goto failed;
-	}
+	} else
+		check_thr_started = true;
 	if ((rc = pthread_create(&uevq_thr, &misc_attr, uevqloop, vecs))) {
 		condlog(0, "failed to create uevent dispatcher: %d", rc);
 		goto failed;
-	}
+	} else
+		uevq_thr_started = true;
 	pthread_attr_destroy(&misc_attr);
 
 	while (1) {
@@ -3136,22 +3178,7 @@ child (__attribute__((unused)) void *param)
 		}
 	}
 
-	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);
-	pthread_join(uevq_thr, NULL);
-	if (poll_dmevents)
-		pthread_join(dmevent_thr, NULL);
-
-	stop_io_err_stat_thread();
-
+	cleanup_threads();
 	cleanup_vecs();
 	cleanup_foreign();
 	cleanup_checkers();
@@ -3178,7 +3205,6 @@ child (__attribute__((unused)) void *param)
 	conf = rcu_dereference(multipath_conf);
 	rcu_assign_pointer(multipath_conf, NULL);
 	call_rcu(&conf->rcu, rcu_free_config);
-	pthread_attr_destroy(&waiter_attr);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 07/29] multipathd: move conf destruction into separate function
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (5 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 06/29] multipathd: move threads destruction into separate function mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 18:56   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 08/29] multipathd: move pid " mwilck
                   ` (21 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Also removing the comment about dlog() and dm_write_log().
dlog() can cope with get_multipath_config() returning NULL,
and dm_write_log() hasn't accessed the configuration for a while.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 8b9df55..722ef69 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2892,6 +2892,16 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_conf(void) {
+	struct config *conf;
+
+	conf = rcu_dereference(multipath_conf);
+	if (!conf)
+		return;
+	rcu_assign_pointer(multipath_conf, NULL);
+	call_rcu(&conf->rcu, rcu_free_config);
+}
+
 static void cleanup_maps(struct vectors *vecs)
 {
 	int queue_without_daemon, i;
@@ -3196,15 +3206,7 @@ child (__attribute__((unused)) void *param)
 
 	if (logsink == 1)
 		log_thread_stop();
-
-	/*
-	 * Freeing config must be done after condlog() and dm_lib_exit(),
-	 * because logging functions like dlog() and dm_write_log()
-	 * reference the config.
-	 */
-	conf = rcu_dereference(multipath_conf);
-	rcu_assign_pointer(multipath_conf, NULL);
-	call_rcu(&conf->rcu, rcu_free_config);
+	cleanup_conf();
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 08/29] multipathd: move pid destruction into separate function
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (6 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 07/29] multipathd: move conf " mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 09/29] multipathd: close pidfile on exit mwilck
                   ` (20 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 722ef69..e21ac8a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2892,6 +2892,12 @@ set_oom_adj (void)
 	condlog(0, "couldn't adjust oom score");
 }
 
+static void cleanup_pidfile(void)
+{
+	condlog(3, "unlink pidfile");
+	unlink(DEFAULT_PIDFILE);
+}
+
 static void cleanup_conf(void) {
 	struct config *conf;
 
@@ -3199,9 +3205,7 @@ child (__attribute__((unused)) void *param)
 	dm_lib_exit();
 
 	/* We're done here */
-	condlog(3, "unlink pidfile");
-	unlink(DEFAULT_PIDFILE);
-
+	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
 
 	if (logsink == 1)
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 09/29] multipathd: close pidfile on exit
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (7 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 08/29] multipathd: move pid " mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 10/29] multipathd: add helper for systemd notification at exit mwilck
                   ` (19 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

It seems we've been doing this only in the failure case, for ages.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index e21ac8a..bd8227a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -132,6 +132,7 @@ static pthread_cond_t config_cond;
 static pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr, dmevent_thr;
 static bool check_thr_started, uevent_thr_started, uxlsnr_thr_started,
 	uevq_thr_started, dmevent_thr_started;
+static int pid_fd = -1;
 
 static inline enum daemon_status get_running_state(void)
 {
@@ -2894,6 +2895,8 @@ set_oom_adj (void)
 
 static void cleanup_pidfile(void)
 {
+	if (pid_fd >= 0)
+		close(pid_fd);
 	condlog(3, "unlink pidfile");
 	unlink(DEFAULT_PIDFILE);
 }
@@ -3026,7 +3029,6 @@ child (__attribute__((unused)) void *param)
 	pthread_attr_t log_attr, misc_attr, uevent_attr;
 	struct vectors * vecs;
 	int rc;
-	int pid_fd = -1;
 	struct config *conf;
 	char *envp;
 	enum daemon_status state;
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 10/29] multipathd: add helper for systemd notification at exit
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (8 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 09/29] multipathd: close pidfile on exit mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 11/29] multipathd: child(): call cleanups in failure case, too mwilck
                   ` (18 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Add sd_notify_exit().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index bd8227a..9080a15 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3023,6 +3023,17 @@ static void cleanup_rcu(int dummy __attribute__((unused)), void *arg)
 	rcu_unregister_thread();
 }
 
+static int sd_notify_exit(int err)
+{
+#ifdef USE_SYSTEMD
+	char msg[24];
+
+	snprintf(msg, sizeof(msg), "ERRNO=%d", err);
+	sd_notify(0, msg);
+#endif
+	return err;
+}
+
 static int
 child (__attribute__((unused)) void *param)
 {
@@ -3216,19 +3227,12 @@ child (__attribute__((unused)) void *param)
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-
-#ifdef USE_SYSTEMD
-	sd_notify(0, "ERRNO=0");
-#endif
-	exit(0);
+	exit(sd_notify_exit(0));
 
 failed:
-#ifdef USE_SYSTEMD
-	sd_notify(0, "ERRNO=1");
-#endif
 	if (pid_fd >= 0)
 		close(pid_fd);
-	exit(1);
+	exit(sd_notify_exit(1));
 }
 
 static int
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 11/29] multipathd: child(): call cleanups in failure case, too
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (9 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 10/29] multipathd: add helper for systemd notification at exit mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized mwilck
                   ` (17 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

So far we haven't called any cleanup code if child() failed.
Fix it.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 9080a15..be6d3cd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3044,6 +3044,7 @@ child (__attribute__((unused)) void *param)
 	char *envp;
 	enum daemon_status state;
 	struct call_rcu_data *crdp;
+	int exit_code = 1;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
@@ -3207,6 +3208,8 @@ child (__attribute__((unused)) void *param)
 		}
 	}
 
+	exit_code = 0;
+failed:
 	cleanup_threads();
 	cleanup_vecs();
 	cleanup_foreign();
@@ -3227,12 +3230,7 @@ child (__attribute__((unused)) void *param)
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-	exit(sd_notify_exit(0));
-
-failed:
-	if (pid_fd >= 0)
-		close(pid_fd);
-	exit(sd_notify_exit(1));
+	return sd_notify_exit(exit_code);
 }
 
 static int
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (10 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 11/29] multipathd: child(): call cleanups in failure case, too mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 13/29] multipathd: print error message if config can't be loaded mwilck
                   ` (16 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/dmevents.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index b561cbf..f52f597 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -257,6 +257,8 @@ void unwatch_all_dmevents(void)
 	struct dev_event *dev_evt;
 	int i;
 
+	if (!waiter)
+		return;
 	pthread_mutex_lock(&waiter->events_lock);
 	vector_foreach_slot(waiter->events, dev_evt, i)
 		free(dev_evt);
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 13/29] multipathd: print error message if config can't be loaded
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (11 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit() mwilck
                   ` (15 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index be6d3cd..f3bb272 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3074,8 +3074,10 @@ child (__attribute__((unused)) void *param)
 	condlog(2, "read " DEFAULT_CONFIGFILE);
 
 	conf = load_config(DEFAULT_CONFIGFILE);
-	if (!conf)
+	if (!conf) {
+		condlog(0, "failed to load configuration");
 		goto failed;
+	}
 
 	if (verbosity)
 		conf->verbosity = verbosity;
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit()
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (12 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 13/29] multipathd: print error message if config can't be loaded mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 19:07   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 15/29] multipathd: fixup libdm deinitialization mwilck
                   ` (14 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This function prepares for calling dm_lib_exit() on program exit.
It undoes changes to libdm internals done by libmultipath.
It doesn't call dm_lib_exit(), as the caller may want to keep
libdm active.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c    |  1 +
 libmultipath/config.h    |  2 ++
 libmultipath/devmapper.c | 15 +++++++++++++++
 libmultipath/devmapper.h |  1 +
 4 files changed, 19 insertions(+)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index f74417c..b9cb413 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -60,6 +60,7 @@ int libmultipath_init(void)
 static void _libmultipath_exit(void)
 {
 	libmultipath_exit_called = true;
+	libmp_dm_exit();
 	udev_unref(udev);
 }
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index f478df7..5d46035 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -271,6 +271,8 @@ int libmultipath_init(void);
  *
  * This function un-initializes libmultipath data structures.
  * It is recommended to call this function at program exit.
+ * If the application also calls dm_lib_exit(), it should do so
+ * after libmultipath_exit().
  *
  * Calls to libmultipath_init() after libmultipath_exit() will fail
  * (in other words, libmultipath can't be re-initialized).
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 2e3ae8a..a8e0611 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -333,6 +333,20 @@ void libmp_udev_set_sync_support(int on)
 	libmp_dm_udev_sync = !!on;
 }
 
+static bool libmp_dm_init_called;
+void libmp_dm_exit(void)
+{
+	if (!libmp_dm_init_called)
+		return;
+
+	/* switch back to default libdm logging */
+	dm_log_init(NULL);
+#ifdef LIBDM_API_HOLD_CONTROL
+	/* make sure control fd is closed in dm_lib_release() */
+	dm_hold_control_dev(0);
+#endif
+}
+
 static void libmp_dm_init(void)
 {
 	struct config *conf;
@@ -349,6 +363,7 @@ static void libmp_dm_init(void)
 	dm_hold_control_dev(1);
 #endif
 	dm_udev_set_sync_support(libmp_dm_udev_sync);
+	libmp_dm_init_called = true;
 }
 
 static void _do_skip_libmp_dm_init(void)
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 158057e..ecf2e66 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -35,6 +35,7 @@ enum {
 
 int dm_prereq(unsigned int *v);
 void skip_libmp_dm_init(void);
+void libmp_dm_exit(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
 int dm_simplecmd_flush (int, const char *, uint16_t);
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 15/29] multipathd: fixup libdm deinitialization
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (13 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit() mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 16/29] libmultipath: log_thread_stop(): check if logarea is initialized mwilck
                   ` (13 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

With libmp_dm_exit() in place, we can make sure that the
calls are made in the right order.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f3bb272..45fc3f0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3220,8 +3220,6 @@ failed:
 	if (poll_dmevents)
 		cleanup_dmevent_waiter();
 
-	dm_lib_exit();
-
 	/* We're done here */
 	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
@@ -3318,6 +3316,9 @@ main (int argc, char *argv[])
 
 	pthread_cond_init_mono(&config_cond);
 
+	if (atexit(dm_lib_exit))
+		condlog(3, "failed to register exit handler for libdm");
+
 	libmultipath_init();
 	if (atexit(libmultipath_exit))
 		condlog(3, "failed to register exit handler for libmultipath");
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 16/29] libmultipath: log_thread_stop(): check if logarea is initialized
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (14 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 15/29] multipathd: fixup libdm deinitialization mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 17/29] multipathd: add cleanup_child() exit handler mwilck
                   ` (12 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log_pthread.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 15baef8..0c327ff 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -112,6 +112,9 @@ void log_thread_reset (void)
 
 void log_thread_stop (void)
 {
+	if (!la)
+		return;
+
 	logdbg(stderr,"enter log_thread_stop\n");
 
 	pthread_mutex_lock(&logev_lock);
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 17/29] multipathd: add cleanup_child() exit handler
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (15 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 16/29] libmultipath: log_thread_stop(): check if logarea is initialized mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown mwilck
                   ` (11 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

cleanup_child() calls all cleanups in the right order, in an
exit handler.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 47 +++++++++++++++++++++++++++--------------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 45fc3f0..0578445 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3023,6 +3023,27 @@ static void cleanup_rcu(int dummy __attribute__((unused)), void *arg)
 	rcu_unregister_thread();
 }
 
+static void cleanup_child(void)
+{
+	cleanup_threads();
+	cleanup_vecs();
+	cleanup_foreign();
+	cleanup_checkers();
+	cleanup_prio();
+	if (poll_dmevents)
+		cleanup_dmevent_waiter();
+
+	cleanup_pidfile();
+	if (logsink == 1)
+		log_thread_stop();
+
+	cleanup_conf();
+
+#ifdef _DEBUG_
+	dbg_free_final(NULL);
+#endif
+}
+
 static int sd_notify_exit(int err)
 {
 #ifdef USE_SYSTEMD
@@ -3049,7 +3070,11 @@ child (__attribute__((unused)) void *param)
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	signal_init();
 	crdp = setup_rcu();
-	on_exit(cleanup_rcu, crdp);
+
+	if (on_exit(cleanup_rcu, crdp) ||
+	    atexit(cleanup_child)) {
+		fprintf(stderr, "failed to register cleanup handlers\n");
+	}
 
 	setup_thread_attr(&misc_attr, 64 * 1024, 0);
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
@@ -3063,8 +3088,6 @@ child (__attribute__((unused)) void *param)
 	pid_fd = pidfile_create(DEFAULT_PIDFILE, daemon_pid);
 	if (pid_fd < 0) {
 		condlog(1, "failed to create pidfile");
-		if (logsink == 1)
-			log_thread_stop();
 		exit(1);
 	}
 
@@ -3212,24 +3235,8 @@ child (__attribute__((unused)) void *param)
 
 	exit_code = 0;
 failed:
-	cleanup_threads();
-	cleanup_vecs();
-	cleanup_foreign();
-	cleanup_checkers();
-	cleanup_prio();
-	if (poll_dmevents)
-		cleanup_dmevent_waiter();
-
-	/* We're done here */
-	cleanup_pidfile();
 	condlog(2, "--------shut down-------");
-
-	if (logsink == 1)
-		log_thread_stop();
-	cleanup_conf();
-#ifdef _DEBUG_
-	dbg_free_final(NULL);
-#endif
+	/* All cleanup is done in the cleanup_child() exit handler */
 	return sd_notify_exit(exit_code);
 }
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (16 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 17/29] multipathd: add cleanup_child() exit handler mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 20:00   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit mwilck
                   ` (10 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This fixes several issues with the log_thread. First, the running
flag logq_running should be set by the thread itself, not by
log_thread_start()/_stop(). Second, the thread was both cancelled and
terminated via a flag (again, logq_running). It's sufficient,
and better, to just cancel it and use logq_running as indication for
successful termination. Third, the locking wasn't cancel-safe in some
places. Forth, log_thread_start() and log_thread_stop() didn't wait for
startup/teardown properly. Fifth, using (pthread_t)0 is wrong (pthread_t is
opaque; there's no guarantee that 0 is not a valid pthread_t value). Sixth,
pthread_cancel() was called under logq_lock, which doesn't make sense to
me at all.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log_pthread.c | 72 ++++++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 23 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 0c327ff..3c73941 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -13,6 +13,7 @@
 #include "log_pthread.h"
 #include "log.h"
 #include "lock.h"
+#include "util.h"
 
 static pthread_t log_thr;
 
@@ -56,44 +57,70 @@ static void flush_logqueue (void)
 	} while (empty == 0);
 }
 
+static void cleanup_log_thread(__attribute((unused)) void *arg)
+{
+	logdbg(stderr, "log thread exiting");
+	pthread_mutex_lock(&logev_lock);
+	logq_running = 0;
+	pthread_mutex_unlock(&logev_lock);
+}
+
 static void * log_thread (__attribute__((unused)) void * et)
 {
 	int running;
 
 	pthread_mutex_lock(&logev_lock);
-	logq_running = 1;
+	running = logq_running;
+	if (!running)
+		logq_running = 1;
+	pthread_cond_signal(&logev_cond);
 	pthread_mutex_unlock(&logev_lock);
+	if (running)
+		/* already started */
+		return NULL;
+	pthread_cleanup_push(cleanup_log_thread, NULL);
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	logdbg(stderr,"enter log_thread\n");
 
 	while (1) {
 		pthread_mutex_lock(&logev_lock);
-		if (logq_running && !log_messages_pending)
+		pthread_cleanup_push(cleanup_mutex, &logev_lock);
+		while (!log_messages_pending)
+			/* this is a cancellation point */
 			pthread_cond_wait(&logev_cond, &logev_lock);
 		log_messages_pending = 0;
-		running = logq_running;
-		pthread_mutex_unlock(&logev_lock);
-		if (!running)
-			break;
+		pthread_cleanup_pop(1);
+
 		flush_logqueue();
 	}
+	pthread_cleanup_pop(1);
 	return NULL;
 }
 
 void log_thread_start (pthread_attr_t *attr)
 {
-	logdbg(stderr,"enter log_thread_start\n");
+	bool err = false;
 
-	pthread_mutex_init(&logq_lock, NULL);
-	pthread_mutex_init(&logev_lock, NULL);
-	pthread_cond_init(&logev_cond, NULL);
+	logdbg(stderr,"enter log_thread_start\n");
 
 	if (log_init("multipathd", 0)) {
 		fprintf(stderr,"can't initialize log buffer\n");
 		exit(1);
 	}
+
+	pthread_mutex_lock(&logev_lock);
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
 	if (pthread_create(&log_thr, attr, log_thread, NULL)) {
+		err = true;
+	}
+
+	/* wait for thread startup */
+	while (!logq_running)
+		pthread_cond_wait(&logev_cond, &logev_lock);
+	pthread_cleanup_pop(1);
+
+	if (err) {
 		fprintf(stderr,"can't start log thread\n");
 		exit(1);
 	}
@@ -112,27 +139,26 @@ void log_thread_reset (void)
 
 void log_thread_stop (void)
 {
+	int running;
+
 	if (!la)
 		return;
 
 	logdbg(stderr,"enter log_thread_stop\n");
 
 	pthread_mutex_lock(&logev_lock);
-	logq_running = 0;
-	pthread_cond_signal(&logev_cond);
-	pthread_mutex_unlock(&logev_lock);
-
-	pthread_mutex_lock(&logq_lock);
-	pthread_cancel(log_thr);
-	pthread_mutex_unlock(&logq_lock);
-	pthread_join(log_thr, NULL);
-	log_thr = (pthread_t)0;
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
+	running = logq_running;
+	if (running) {
+		pthread_cancel(log_thr);
+		pthread_cond_signal(&logev_cond);
+	}
+	pthread_cleanup_pop(1);
 
-	flush_logqueue();
+	if (running)
+		pthread_join(log_thr, NULL);
 
-	pthread_mutex_destroy(&logq_lock);
-	pthread_mutex_destroy(&logev_lock);
-	pthread_cond_destroy(&logev_cond);
 
+	flush_logqueue();
 	log_close();
 }
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (17 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 20/29] multipath: use atexit() for cleanup handlers mwilck
                   ` (9 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

This requires another major ABI bump.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c   | 2 --
 libmultipath/config.c             | 4 ++++
 libmultipath/libmultipath.version | 5 +----
 multipathd/main.c                 | 3 ---
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index e1d1cb7..9ebf91d 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -78,8 +78,6 @@ mpath_lib_init (void)
 
 static void libmpathpersist_cleanup(void)
 {
-	cleanup_prio();
-	cleanup_checkers();
 	libmultipath_exit();
 	dm_lib_exit();
 }
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b9cb413..52b1447 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -26,6 +26,7 @@
 #include "devmapper.h"
 #include "mpath_cmd.h"
 #include "propsel.h"
+#include "foreign.h"
 
 /*
  * We don't support re-initialization after
@@ -60,6 +61,9 @@ int libmultipath_init(void)
 static void _libmultipath_exit(void)
 {
 	libmultipath_exit_called = true;
+	cleanup_foreign();
+	cleanup_checkers();
+	cleanup_prio();
 	libmp_dm_exit();
 	udev_unref(udev);
 }
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 84beb7f..800cff2 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_3.0.0 {
+LIBMULTIPATH_4.0.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -51,10 +51,7 @@ global:
 	checker_name;
 	checker_state_name;
 	check_foreign;
-	cleanup_checkers;
-	cleanup_foreign;
 	cleanup_lock;
-	cleanup_prio;
 	close_fd;
 	coalesce_paths;
 	convert_dev;
diff --git a/multipathd/main.c b/multipathd/main.c
index 0578445..33277d5 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3027,9 +3027,6 @@ static void cleanup_child(void)
 {
 	cleanup_threads();
 	cleanup_vecs();
-	cleanup_foreign();
-	cleanup_checkers();
-	cleanup_prio();
 	if (poll_dmevents)
 		cleanup_dmevent_waiter();
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 20/29] multipath: use atexit() for cleanup handlers
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (18 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 21/29] mpathpersist: " mwilck
                   ` (8 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 9ae46ed..049a36f 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -452,13 +452,21 @@ static bool released_to_systemd(void)
 	return ret;
 }
 
+static void cleanup_vecs(__attribute__((unused)) int dummy, void *arg)
+{
+	struct vectors *vecs = arg;
+
+	free_multipathvec(vecs->mpvec, KEEP_PATHS);
+	free_pathvec(vecs->pathvec, FREE_PATHS);
+}
+
 static int
 configure (struct config *conf, enum mpath_cmds cmd,
 	   enum devtypes dev_type, char *devpath)
 {
 	vector curmp = NULL;
 	vector pathvec = NULL;
-	struct vectors vecs;
+	static struct vectors vecs;
 	int r = RTVL_FAIL, rc;
 	int di_flag = 0;
 	char * refwwid = NULL;
@@ -469,6 +477,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	 */
 	curmp = vector_alloc();
 	pathvec = vector_alloc();
+	on_exit(cleanup_vecs, &vecs);
 
 	if (!curmp || !pathvec) {
 		condlog(0, "can not allocate memory");
@@ -580,9 +589,6 @@ out:
 	if (refwwid)
 		FREE(refwwid);
 
-	free_multipathvec(curmp, KEEP_PATHS);
-	free_pathvec(pathvec, FREE_PATHS);
-
 	return r;
 }
 
@@ -808,9 +814,13 @@ main (int argc, char *argv[])
 	bool enable_foreign = false;
 
 	libmultipath_init();
+	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
+		condlog(1, "failed to register cleanup handler for libmultipath: %m");
 	logsink = 0;
 	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
+	if (atexit(uninit_config))
+		condlog(1, "failed to register cleanup handler for config: %m");
 	conf = get_multipath_config();
 	conf->retrigger_tries = 0;
 	conf->force_sync = 1;
@@ -887,7 +897,7 @@ main (int argc, char *argv[])
 			break;
 		case 't':
 			r = dump_config(conf, NULL, NULL) ? RTVL_FAIL : RTVL_OK;
-			goto out_free_config;
+			goto out;
 		case 'T':
 			cmd = CMD_DUMP_CONFIG;
 			break;
@@ -1048,26 +1058,13 @@ main (int argc, char *argv[])
 		condlog(3, "restart multipath configuration process");
 
 out:
-	dm_lib_exit();
-
-	cleanup_foreign();
-	cleanup_prio();
-	cleanup_checkers();
+	put_multipath_config(conf);
+	if (dev)
+		FREE(dev);
 
 	if (dev_type == DEV_UEVENT)
 		closelog();
 
-out_free_config:
-	/*
-	 * Freeing config must be done after dm_lib_exit(), because
-	 * the logging function (dm_write_log()), which is called there,
-	 * references the config.
-	 */
-	put_multipath_config(conf);
-	uninit_config();
-	libmultipath_exit();
-	if (dev)
-		FREE(dev);
 #ifdef _DEBUG_
 	dbg_free_final(NULL);
 #endif
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 21/29] mpathpersist: use atexit() for cleanup handlers
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (19 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 20/29] multipath: use atexit() for cleanup handlers mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid() mwilck
                   ` (7 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 mpathpersist/main.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index 3c2e657..14245cc 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -641,11 +641,10 @@ int main(int argc, char *argv[])
 	if (libmpathpersist_init()) {
 		exit(1);
 	}
+	if (atexit((void(*)(void))libmpathpersist_exit))
+		fprintf(stderr, "failed to register cleanup handler for libmpathpersist: %m");
 
 	ret = handle_args(argc, argv, 0);
-
-	libmpathpersist_exit();
-
 	return (ret >= 0) ? ret : MPATH_PR_OTHER;
 }
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid()
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (20 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 21/29] mpathpersist: " mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions mwilck
                   ` (6 subsequent siblings)
  28 siblings, 0 replies; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

There were two leaks in check_path_valid(): if path status was
successfully determined before calling store_pathvec(), free_path()
wasn't called. Also, if an error exit occured, neither cleanup
function was called.

This patch fixes both, at the cost of using "static" for the pp and
pathvec variables.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/main.c | 55 +++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 43 insertions(+), 12 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 049a36f..9974993 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -93,7 +93,7 @@ void rcu_register_thread_memb(void) {}
 void rcu_unregister_thread_memb(void) {}
 
 static int
-filter_pathvec (vector pathvec, char * refwwid)
+filter_pathvec (vector pathvec, const char *refwwid)
 {
 	int i;
 	struct path * pp;
@@ -592,12 +592,37 @@ out:
 	return r;
 }
 
+static void cleanup_pathvec(__attribute__((unused)) int dummy, void *arg)
+{
+	vector *ppv = arg;
+
+	if (ppv && *ppv) {
+		free_pathvec(*ppv, FREE_PATHS);
+		*ppv = NULL;
+	}
+}
+
+static void cleanup_path(__attribute__((unused)) int dummy, void *arg)
+{
+	struct path **ppp = arg;
+
+	if (ppp && *ppp) {
+		free_path(*ppp);
+		*ppp = NULL;
+	}
+}
+
 static int
 check_path_valid(const char *name, struct config *conf, bool is_uevent)
 {
 	int fd, r = PATH_IS_ERROR;
-	struct path *pp = NULL;
-	vector pathvec = NULL;
+	static struct path *pp = NULL;
+	static vector pathvec = NULL;
+	const char *wwid;
+
+	/* register these as exit handlers in case we exit irregularly */
+	on_exit(cleanup_path, &pp);
+	on_exit(cleanup_pathvec, &pathvec);
 
 	pp = alloc_path();
 	if (!pp)
@@ -667,13 +692,17 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 	if (store_path(pathvec, pp) != 0) {
 		free_path(pp);
 		goto fail;
+	} else {
+		/* make sure path isn't freed twice */
+		wwid = pp->wwid;
+		pp = NULL;
 	}
 
 	/* For find_multipaths = SMART, if there is more than one path
 	 * matching the refwwid, then the path is valid */
 	if (path_discovery(pathvec, DI_SYSFS | DI_WWID) < 0)
 		goto fail;
-	filter_pathvec(pathvec, pp->wwid);
+	filter_pathvec(pathvec, wwid);
 	if (VECTOR_SIZE(pathvec) > 1)
 		r = PATH_IS_VALID;
 	else
@@ -681,21 +710,23 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent)
 
 out:
 	r = print_cmd_valid(r, pathvec, conf);
-	free_pathvec(pathvec, FREE_PATHS);
 	/*
 	 * multipath -u must exit with status 0, otherwise udev won't
 	 * import its output.
 	 */
 	if (!is_uevent && r == PATH_IS_NOT_VALID)
-		return RTVL_FAIL;
-	return RTVL_OK;
+		r = RTVL_FAIL;
+	else
+		r = RTVL_OK;
+	goto cleanup;
 
 fail:
-	if (pathvec)
-		free_pathvec(pathvec, FREE_PATHS);
-	else
-		free_path(pp);
-	return RTVL_FAIL;
+	r = RTVL_FAIL;
+
+cleanup:
+	cleanup_path(0, &pp);
+	cleanup_pathvec(0, &pathvec);
+	return r;
 }
 
 static int
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (21 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid() mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 20:01   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity mwilck
                   ` (5 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

These leaks are caused by other libraries (libsystemd, glibc,
libgcrypt) and should be ignored when debugging with valgrind

Usage example:

valgrind --suppressions=mpath-tools.supp \
    --leak-check=full --show-leak-kinds=all $COMMAND

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 third-party/valgrind/mpath-tools.supp | 32 +++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 third-party/valgrind/mpath-tools.supp

diff --git a/third-party/valgrind/mpath-tools.supp b/third-party/valgrind/mpath-tools.supp
new file mode 100644
index 0000000..0537fd5
--- /dev/null
+++ b/third-party/valgrind/mpath-tools.supp
@@ -0,0 +1,32 @@
+{
+   glibc _dlerror_run leak: https://stackoverflow.com/questions/1542457/memory-leak-reported-by-valgrind-in-dlopen
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:calloc
+   fun:_dlerror_run
+   fun:dlopen*
+}
+
+{
+   systemd mempools are never freed: https://bugzilla.redhat.com/show_bug.cgi?id=1215670
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:malloc
+   fun:mempool_alloc_tile
+   fun:mempool_alloc0_tile
+   fun:hashmap_base_new
+   fun:hashmap_base_ensure_allocated
+}
+
+{
+   libgcrypt library initialization
+   Memcheck:Leak
+   match-leak-kinds: reachable
+   fun:malloc
+   ...
+   fun:_gcry_xmalloc
+   ...
+   fun:global_init.*
+   ...
+   fun:_dl_init
+}
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (22 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 20:38   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink mwilck
                   ` (4 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Introduce a new global variable to set the verbosity of libmultipath.
This avoids accessing the configuration in every dlog() call.
When libmultipath reads its configuration in init_config() or
load_config(), it will use the current value of libmp_verbosity
for logging. Immediately before returning, libmp_verbosity will be
overwritten with the verbosity value from the configuration file,
if it was set there. An application is free to set libmp_verbosity
back to the previous value or not after that, depending on whether
command line options or configuration file settings should take
precedence.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmpathpersist/mpath_persist.c   |  5 +---
 libmultipath/config.c             |  9 +++++--
 libmultipath/debug.c              | 10 ++------
 libmultipath/debug.h              |  1 +
 libmultipath/libmultipath.version |  5 ++++
 multipath/main.c                  | 21 ++++++----------
 multipathd/main.c                 | 40 ++++++++++++++++++-------------
 tests/alias.c                     |  1 +
 tests/blacklist.c                 |  2 ++
 9 files changed, 49 insertions(+), 45 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
index 9ebf91d..79322e8 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -170,10 +170,7 @@ void mpath_persistent_reserve_free_vecs(void)
 
 int mpath_persistent_reserve_init_vecs(int verbose)
 {
-	struct config *conf = get_multipath_config();
-
-	conf->verbosity = verbose;
-	put_multipath_config(conf);
+	libmp_verbosity = verbose;
 
 	if (curmp)
 		return MPATH_PR_SUCCESS;
diff --git a/libmultipath/config.c b/libmultipath/config.c
index 52b1447..49e7fb8 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -828,10 +828,14 @@ int _init_config (const char *file, struct config *conf)
 		conf = &__internal_config;
 
 	/*
-	 * internal defaults
+	 * Processing the config file will overwrite conf->verbosity if set
+	 * When we return, we'll copy the config value back
 	 */
-	conf->verbosity = DEFAULT_VERBOSITY;
+	conf->verbosity = libmp_verbosity;
 
+	/*
+	 * internal defaults
+	 */
 	get_sys_max_fds(&conf->max_fds);
 	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
 	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
@@ -997,6 +1001,7 @@ int _init_config (const char *file, struct config *conf)
 	    !conf->wwids_file || !conf->prkeys_file)
 		goto out;
 
+	libmp_verbosity = conf->verbosity;
 	return 0;
 out:
 	_uninit_config(conf);
diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index b3a1de9..a1713b9 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -16,21 +16,15 @@
 #include "debug.h"
 
 int logsink;
+int libmp_verbosity = DEFAULT_VERBOSITY;
 
 void dlog (int sink, int prio, const char * fmt, ...)
 {
 	va_list ap;
-	int thres;
-	struct config *conf;
 
 	va_start(ap, fmt);
-	conf = get_multipath_config();
-	ANNOTATE_IGNORE_READS_BEGIN();
-	thres = (conf) ? conf->verbosity : DEFAULT_VERBOSITY;
-	ANNOTATE_IGNORE_READS_END();
-	put_multipath_config(conf);
 
-	if (prio <= thres) {
+	if (prio <= libmp_verbosity) {
 		if (sink < 1) {
 			if (sink == 0) {
 				time_t t = time(NULL);
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index c6120c1..1f3bc8b 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -8,6 +8,7 @@ void dlog (int sink, int prio, const char * fmt, ...)
 #include "log_pthread.h"
 
 extern int logsink;
+extern int libmp_verbosity;
 
 #define condlog(prio, fmt, args...) \
 	dlog(logsink, prio, fmt "\n", ##args)
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 800cff2..67a7379 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -259,3 +259,8 @@ global:
 local:
 	*;
 };
+
+LIBMULTIPATH_4.1.0 {
+global:
+	libmp_verbosity;
+} LIBMULTIPATH_4.0.0;
diff --git a/multipath/main.c b/multipath/main.c
index 9974993..9bdc5d2 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -208,22 +208,15 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
 			mpp->bestpg = select_path_group(mpp);
 
 		if (cmd == CMD_LIST_SHORT ||
-		    cmd == CMD_LIST_LONG) {
-			struct config *conf = get_multipath_config();
-			print_multipath_topology(mpp, conf->verbosity);
-			put_multipath_config(conf);
-		}
+		    cmd == CMD_LIST_LONG)
+			print_multipath_topology(mpp, libmp_verbosity);
 
 		if (cmd == CMD_CREATE)
 			reinstate_paths(mpp);
 	}
 
-	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) {
-		struct config *conf = get_multipath_config();
-
-		print_foreign_topology(conf->verbosity);
-		put_multipath_config(conf);
-	}
+	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG)
+		print_foreign_topology(libmp_verbosity);
 
 	return 0;
 }
@@ -554,7 +547,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	if (path_discovery(pathvec, di_flag) < 0)
 		goto out;
 
-	if (conf->verbosity > 2)
+	if (libmp_verbosity > 2)
 		print_all_paths(pathvec, 1);
 
 	get_path_layout(pathvec, 0);
@@ -866,7 +859,7 @@ main (int argc, char *argv[])
 				exit(RTVL_FAIL);
 			}
 
-			conf->verbosity = atoi(optarg);
+			libmp_verbosity = atoi(optarg);
 			break;
 		case 'b':
 			conf->bindings_file = strdup(optarg);
@@ -997,7 +990,7 @@ main (int argc, char *argv[])
 	}
 	if (dev_type == DEV_UEVENT) {
 		openlog("multipath", 0, LOG_DAEMON);
-		setlogmask(LOG_UPTO(conf->verbosity + 3));
+		setlogmask(LOG_UPTO(libmp_verbosity + 3));
 		logsink = 1;
 	}
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 33277d5..fbd354c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -88,10 +88,10 @@
 #define CMDSIZE 160
 #define MSG_SIZE 32
 
-#define LOG_MSG(lvl, verb, pp)					\
+#define LOG_MSG(lvl, pp)					\
 do {								\
 	if (pp->mpp && checker_selected(&pp->checker) &&	\
-	    lvl <= verb) {					\
+	    lvl <= libmp_verbosity) {					\
 		if (pp->offline)				\
 			condlog(lvl, "%s: %s - path offline",	\
 				pp->mpp->alias, pp->dev);	\
@@ -2070,7 +2070,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	int chkr_new_path_up = 0;
 	int disable_reinstate = 0;
 	int oldchkrstate = pp->chkrstate;
-	int retrigger_tries, verbosity;
+	int retrigger_tries;
 	unsigned int checkint, max_checkint;
 	struct config *conf;
 	int marginal_pathgroups, marginal_changed = 0;
@@ -2090,7 +2090,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	retrigger_tries = conf->retrigger_tries;
 	checkint = conf->checkint;
 	max_checkint = conf->max_checkint;
-	verbosity = conf->verbosity;
 	marginal_pathgroups = conf->marginal_pathgroups;
 	put_multipath_config(conf);
 
@@ -2152,7 +2151,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
 		condlog(2, "%s: unusable path (%s) - checker failed",
 			pp->dev, checker_state_name(newstate));
-		LOG_MSG(2, verbosity, pp);
+		LOG_MSG(2, pp);
 		conf = get_multipath_config();
 		pthread_cleanup_push(put_multipath_config, conf);
 		pathinfo(pp, conf, 0);
@@ -2257,7 +2256,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 		int oldstate = pp->state;
 		pp->state = newstate;
 
-		LOG_MSG(1, verbosity, pp);
+		LOG_MSG(1, pp);
 
 		/*
 		 * upon state change, reset the checkint
@@ -2321,7 +2320,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 			/* Clear IO errors */
 			reinstate_path(pp);
 		else {
-			LOG_MSG(4, verbosity, pp);
+			LOG_MSG(4, pp);
 			if (pp->checkint != max_checkint) {
 				/*
 				 * double the next check delay.
@@ -2349,9 +2348,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
 			log_checker_err = conf->log_checker_err;
 			put_multipath_config(conf);
 			if (log_checker_err == LOG_CHKR_ERR_ONCE)
-				LOG_MSG(3, verbosity, pp);
+				LOG_MSG(3, pp);
 			else
-				LOG_MSG(2, verbosity, pp);
+				LOG_MSG(2, pp);
 		}
 	}
 
@@ -2696,6 +2695,10 @@ reconfigure (struct vectors * vecs)
 	if (!conf)
 		return 1;
 
+	if (verbosity)
+		libmp_verbosity = verbosity;
+	setlogmask(LOG_UPTO(libmp_verbosity + 3));
+
 	/*
 	 * free old map and path vectors ... they use old conf state
 	 */
@@ -2710,8 +2713,6 @@ reconfigure (struct vectors * vecs)
 	/* Re-read any timezone changes */
 	tzset();
 
-	if (verbosity)
-		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
 	check_alias_settings(conf);
@@ -3093,14 +3094,18 @@ child (__attribute__((unused)) void *param)
 	condlog(2, "--------start up--------");
 	condlog(2, "read " DEFAULT_CONFIGFILE);
 
+	if (verbosity)
+		libmp_verbosity = verbosity;
 	conf = load_config(DEFAULT_CONFIGFILE);
+	if (verbosity)
+		libmp_verbosity = verbosity;
+	setlogmask(LOG_UPTO(libmp_verbosity + 3));
+
 	if (!conf) {
 		condlog(0, "failed to load configuration");
 		goto failed;
 	}
 
-	if (verbosity)
-		conf->verbosity = verbosity;
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
 	uxsock_timeout = conf->uxsock_timeout;
@@ -3119,7 +3124,6 @@ child (__attribute__((unused)) void *param)
 
 	if (poll_dmevents)
 		poll_dmevents = dmevent_poll_supported();
-	setlogmask(LOG_UPTO(conf->verbosity + 3));
 
 	envp = getenv("LimitNOFILE");
 
@@ -3341,7 +3345,7 @@ main (int argc, char *argv[])
 			    !isdigit(optarg[0]))
 				exit(1);
 
-			verbosity = atoi(optarg);
+			libmp_verbosity = verbosity = atoi(optarg);
 			break;
 		case 's':
 			logsink = -1;
@@ -3352,7 +3356,7 @@ main (int argc, char *argv[])
 			if (!conf)
 				exit(1);
 			if (verbosity)
-				conf->verbosity = verbosity;
+				libmp_verbosity = verbosity;
 			uxsock_timeout = conf->uxsock_timeout;
 			err = uxclnt(optarg, uxsock_timeout + 100);
 			free_config(conf);
@@ -3378,11 +3382,13 @@ main (int argc, char *argv[])
 		char * c = s;
 
 		logsink = 0;
+		if (verbosity)
+			libmp_verbosity = verbosity;
 		conf = load_config(DEFAULT_CONFIGFILE);
 		if (!conf)
 			exit(1);
 		if (verbosity)
-			conf->verbosity = verbosity;
+			libmp_verbosity = verbosity;
 		uxsock_timeout = conf->uxsock_timeout;
 		memset(cmd, 0x0, CMDSIZE);
 		while (optind < argc) {
diff --git a/tests/alias.c b/tests/alias.c
index 5624138..e5e20b2 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -735,6 +735,7 @@ static int test_allocate_binding(void)
 int main(void)
 {
 	int ret = 0;
+	libmp_verbosity = conf.verbosity;
 
 	ret += test_format_devname();
 	ret += test_scan_devname();
diff --git a/tests/blacklist.c b/tests/blacklist.c
index 84a3ba2..0b42e25 100644
--- a/tests/blacklist.c
+++ b/tests/blacklist.c
@@ -22,6 +22,7 @@
 #include "globals.c"
 #include "blacklist.h"
 #include "test-log.h"
+#include "debug.h"
 
 struct udev_device {
 	const char *sysname;
@@ -152,6 +153,7 @@ static int setup(void **state)
 	    store_ble(blist_property_wwn_inv, "!ID_WWN", ORIGIN_CONFIG))
 		return -1;
 
+	libmp_verbosity = conf.verbosity = 4;
 	return 0;
 }
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (23 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-16 20:13   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog() mwilck
                   ` (3 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c     |  4 ++--
 libmultipath/debug.h     |  7 +++++++
 libmultipath/devmapper.c |  4 ++--
 multipath/main.c         |  4 ++--
 multipathd/main.c        | 17 ++++++++---------
 tests/globals.c          |  3 ++-
 tests/hwtable.c          |  2 +-
 7 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index a1713b9..f9b7755 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -25,8 +25,8 @@ void dlog (int sink, int prio, const char * fmt, ...)
 	va_start(ap, fmt);
 
 	if (prio <= libmp_verbosity) {
-		if (sink < 1) {
-			if (sink == 0) {
+		if (sink != LOGSINK_SYSLOG) {
+			if (sink == LOGSINK_STDERR_WITH_TIME) {
 				time_t t = time(NULL);
 				struct tm *tb = localtime(&t);
 				char buff[16];
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index 1f3bc8b..91ba02b 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -12,3 +12,10 @@ extern int libmp_verbosity;
 
 #define condlog(prio, fmt, args...) \
 	dlog(logsink, prio, fmt "\n", ##args)
+
+enum {
+	LOGSINK_STDERR_WITH_TIME = 0,
+	LOGSINK_STDERR_WITHOUT_TIME = -1,
+	LOGSINK_SYSLOG = 1,
+};
+
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index a8e0611..78112a9 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -104,8 +104,8 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 		return;
 
 	va_start(ap, f);
-	if (logsink < 1) {
-		if (logsink == 0) {
+	if (logsink != LOGSINK_SYSLOG) {
+		if (logsink == LOGSINK_STDERR_WITH_TIME) {
 			time_t t = time(NULL);
 			struct tm *tb = localtime(&t);
 			char buff[16];
diff --git a/multipath/main.c b/multipath/main.c
index 9bdc5d2..46cfa17 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -840,7 +840,7 @@ main (int argc, char *argv[])
 	libmultipath_init();
 	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
 		condlog(1, "failed to register cleanup handler for libmultipath: %m");
-	logsink = 0;
+	logsink = LOGSINK_STDERR_WITH_TIME;
 	if (init_config(DEFAULT_CONFIGFILE))
 		exit(RTVL_FAIL);
 	if (atexit(uninit_config))
@@ -991,7 +991,7 @@ main (int argc, char *argv[])
 	if (dev_type == DEV_UEVENT) {
 		openlog("multipath", 0, LOG_DAEMON);
 		setlogmask(LOG_UPTO(libmp_verbosity + 3));
-		logsink = 1;
+		logsink = LOGSINK_SYSLOG;
 	}
 
 	set_max_fds(conf->max_fds);
diff --git a/multipathd/main.c b/multipathd/main.c
index fbd354c..725a10b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2780,7 +2780,7 @@ handle_signals(bool nonfatal)
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
-		if (logsink == 1)
+		if (logsink == LOGSINK_SYSLOG)
 			log_thread_reset();
 	}
 	reconfig_sig = 0;
@@ -3032,7 +3032,7 @@ static void cleanup_child(void)
 		cleanup_dmevent_waiter();
 
 	cleanup_pidfile();
-	if (logsink == 1)
+	if (logsink == LOGSINK_SYSLOG)
 		log_thread_stop();
 
 	cleanup_conf();
@@ -3078,7 +3078,7 @@ child (__attribute__((unused)) void *param)
 	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
 	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
 
-	if (logsink == 1) {
+	if (logsink == LOGSINK_SYSLOG) {
 		setup_thread_attr(&log_attr, 64 * 1024, 0);
 		log_thread_start(&log_attr);
 		pthread_attr_destroy(&log_attr);
@@ -3309,7 +3309,7 @@ main (int argc, char *argv[])
 	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
 		"Suppress complaints about this scalar variable");
 
-	logsink = 1;
+	logsink = LOGSINK_SYSLOG;
 
 	if (getuid() != 0) {
 		fprintf(stderr, "need to be root\n");
@@ -3336,9 +3336,8 @@ main (int argc, char *argv[])
 		switch(arg) {
 		case 'd':
 			foreground = 1;
-			if (logsink > 0)
-				logsink = 0;
-			//debug=1; /* ### comment me out ### */
+			if (logsink == LOGSINK_SYSLOG)
+				logsink = LOGSINK_STDERR_WITH_TIME;
 			break;
 		case 'v':
 			if (sizeof(optarg) > sizeof(char *) ||
@@ -3348,7 +3347,7 @@ main (int argc, char *argv[])
 			libmp_verbosity = verbosity = atoi(optarg);
 			break;
 		case 's':
-			logsink = -1;
+			logsink = LOGSINK_STDERR_WITHOUT_TIME;
 			break;
 		case 'k':
 			logsink = 0;
@@ -3381,7 +3380,7 @@ main (int argc, char *argv[])
 		char * s = cmd;
 		char * c = s;
 
-		logsink = 0;
+		logsink = LOGSINK_STDERR_WITH_TIME;
 		if (verbosity)
 			libmp_verbosity = verbosity;
 		conf = load_config(DEFAULT_CONFIGFILE);
diff --git a/tests/globals.c b/tests/globals.c
index 8add5eb..fc0c07a 100644
--- a/tests/globals.c
+++ b/tests/globals.c
@@ -1,9 +1,10 @@
 #include "structs.h"
 #include "config.h"
+#include "debug.h"
 
 /* Required globals */
 struct udev *udev;
-int logsink = -1;
+int logsink = LOGSINK_STDERR_WITHOUT_TIME;
 struct config conf = {
 	.verbosity = 4,
 };
diff --git a/tests/hwtable.c b/tests/hwtable.c
index 57f832b..4dd0873 100644
--- a/tests/hwtable.c
+++ b/tests/hwtable.c
@@ -53,7 +53,7 @@ struct hwt_state {
 
 static struct config *_conf;
 struct udev *udev;
-int logsink = -1;
+int logsink = LOGSINK_STDERR_WITHOUT_TIME;
 
 struct config *get_multipath_config(void)
 {
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog()
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (24 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 21:07   ` Benjamin Marzinski
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args mwilck
                   ` (2 subsequent siblings)
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

By checking the log level in condlog() directly, we can simplify
dlog(). Also, it's now possible to limit the log level at compile
time by setting MAX_VERBOSITY, enabling the compiler to optimize
away log messages with higher loglevel.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c     | 30 +++++++++++++-----------------
 libmultipath/debug.h     | 19 +++++++++++++++----
 libmultipath/devmapper.c |  4 +++-
 tests/test-log.c         |  4 ++--
 tests/test-log.h         |  3 ++-
 5 files changed, 35 insertions(+), 25 deletions(-)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index f9b7755..429f269 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -18,29 +18,25 @@
 int logsink;
 int libmp_verbosity = DEFAULT_VERBOSITY;
 
-void dlog (int sink, int prio, const char * fmt, ...)
+void dlog(int prio, const char * fmt, ...)
 {
 	va_list ap;
 
 	va_start(ap, fmt);
+	if (logsink != LOGSINK_SYSLOG) {
+		if (logsink == LOGSINK_STDERR_WITH_TIME) {
+			time_t t = time(NULL);
+			struct tm *tb = localtime(&t);
+			char buff[16];
 
-	if (prio <= libmp_verbosity) {
-		if (sink != LOGSINK_SYSLOG) {
-			if (sink == LOGSINK_STDERR_WITH_TIME) {
-				time_t t = time(NULL);
-				struct tm *tb = localtime(&t);
-				char buff[16];
-
-				strftime(buff, sizeof(buff),
-					 "%b %d %H:%M:%S", tb);
-				buff[sizeof(buff)-1] = '\0';
-
-				fprintf(stderr, "%s | ", buff);
-			}
-			vfprintf(stderr, fmt, ap);
+			strftime(buff, sizeof(buff),
+				 "%b %d %H:%M:%S", tb);
+			buff[sizeof(buff)-1] = '\0';
+			fprintf(stderr, "%s | ", buff);
 		}
-		else
-			log_safe(prio + 3, fmt, ap);
+		vfprintf(stderr, fmt, ap);
 	}
+	else
+		log_safe(prio + 3, fmt, ap);
 	va_end(ap);
 }
diff --git a/libmultipath/debug.h b/libmultipath/debug.h
index 91ba02b..705a5d7 100644
--- a/libmultipath/debug.h
+++ b/libmultipath/debug.h
@@ -1,5 +1,7 @@
-void dlog (int sink, int prio, const char * fmt, ...)
-	__attribute__((format(printf, 3, 4)));
+#ifndef _DEBUG_H
+#define _DEBUG_H
+void dlog (int prio, const char *fmt, ...)
+	__attribute__((format(printf, 2, 3)));
 
 
 #include <pthread.h>
@@ -10,8 +12,9 @@ void dlog (int sink, int prio, const char * fmt, ...)
 extern int logsink;
 extern int libmp_verbosity;
 
-#define condlog(prio, fmt, args...) \
-	dlog(logsink, prio, fmt "\n", ##args)
+#ifndef MAX_VERBOSITY
+#define MAX_VERBOSITY 4
+#endif
 
 enum {
 	LOGSINK_STDERR_WITH_TIME = 0,
@@ -19,3 +22,11 @@ enum {
 	LOGSINK_SYSLOG = 1,
 };
 
+#define condlog(prio, fmt, args...)					\
+	do {								\
+		int __p = (prio);					\
+									\
+		if (__p <= MAX_VERBOSITY && __p <= libmp_verbosity)	\
+			dlog(__p, fmt "\n", ##args);			\
+	} while (0)
+#endif /* _DEBUG_H */
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 78112a9..cf2f27c 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -274,7 +274,9 @@ static int dm_tgt_prereq (unsigned int *ver)
 
 static void _init_versions(void)
 {
-	dlog(logsink, 3, VERSION_STRING);
+	/* Can't use condlog here because of how VERSION_STRING is defined */
+	if (3 <= libmp_verbosity)
+		dlog(3, VERSION_STRING);
 	init_dm_library_version();
 	init_dm_drv_version();
 	init_dm_mpath_version();
diff --git a/tests/test-log.c b/tests/test-log.c
index 051491e..42ebad0 100644
--- a/tests/test-log.c
+++ b/tests/test-log.c
@@ -6,8 +6,8 @@
 #include "log.h"
 #include "test-log.h"
 
-__attribute__((format(printf, 3, 0)))
-void __wrap_dlog (int sink, int prio, const char * fmt, ...)
+__attribute__((format(printf, 2, 0)))
+void __wrap_dlog (int prio, const char * fmt, ...)
 {
 	char buff[MAX_MSG_SIZE];
 	va_list ap;
diff --git a/tests/test-log.h b/tests/test-log.h
index 2c878c6..6d22cd2 100644
--- a/tests/test-log.h
+++ b/tests/test-log.h
@@ -1,7 +1,8 @@
 #ifndef _TEST_LOG_H
 #define _TEST_LOG_H
 
-void __wrap_dlog (int sink, int prio, const char * fmt, ...);
+__attribute__((format(printf, 2, 0)))
+void __wrap_dlog (int prio, const char * fmt, ...);
 void expect_condlog(int prio, char *string);
 
 #endif
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (25 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog() mwilck
@ 2020-10-16 10:44 ` mwilck
  2020-10-19 21:51   ` Benjamin Marzinski
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen() mwilck
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop() mwilck
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:44 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

'multipathd -k"cmd"' and 'multipath cmd' are the same thing.
Treat it with common code.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 725a10b..0cf8a26 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3303,6 +3303,8 @@ main (int argc, char *argv[])
 	int err;
 	int foreground = 0;
 	struct config *conf;
+	char *opt_k_arg = NULL;
+	bool opt_k = false;
 
 	ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
 				   "Manipulated through RCU");
@@ -3350,16 +3352,9 @@ main (int argc, char *argv[])
 			logsink = LOGSINK_STDERR_WITHOUT_TIME;
 			break;
 		case 'k':
-			logsink = 0;
-			conf = load_config(DEFAULT_CONFIGFILE);
-			if (!conf)
-				exit(1);
-			if (verbosity)
-				libmp_verbosity = verbosity;
-			uxsock_timeout = conf->uxsock_timeout;
-			err = uxclnt(optarg, uxsock_timeout + 100);
-			free_config(conf);
-			return err;
+			opt_k = true;
+			opt_k_arg = optarg;
+			break;
 		case 'B':
 			bindings_read_only = 1;
 			break;
@@ -3375,7 +3370,7 @@ main (int argc, char *argv[])
 			exit(1);
 		}
 	}
-	if (optind < argc) {
+	if (opt_k || optind < argc) {
 		char cmd[CMDSIZE];
 		char * s = cmd;
 		char * c = s;
@@ -3390,14 +3385,20 @@ main (int argc, char *argv[])
 			libmp_verbosity = verbosity;
 		uxsock_timeout = conf->uxsock_timeout;
 		memset(cmd, 0x0, CMDSIZE);
-		while (optind < argc) {
-			if (strchr(argv[optind], ' '))
-				c += snprintf(c, s + CMDSIZE - c, "\"%s\" ", argv[optind]);
-			else
-				c += snprintf(c, s + CMDSIZE - c, "%s ", argv[optind]);
-			optind++;
+		if (opt_k)
+			s = opt_k_arg;
+		else {
+			while (optind < argc) {
+				if (strchr(argv[optind], ' '))
+					c += snprintf(c, s + CMDSIZE - c,
+						      "\"%s\" ", argv[optind]);
+				else
+					c += snprintf(c, s + CMDSIZE - c,
+						      "%s ", argv[optind]);
+				optind++;
+			}
+			c += snprintf(c, s + CMDSIZE - c, "\n");
 		}
-		c += snprintf(c, s + CMDSIZE - c, "\n");
 		err = uxclnt(s, uxsock_timeout + 100);
 		free_config(conf);
 		return err;
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen()
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (26 preceding siblings ...)
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args mwilck
@ 2020-10-16 10:45 ` mwilck
  2020-10-19 23:33   ` Benjamin Marzinski
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop() mwilck
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:45 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

We were allocating 1025 poll fds, which is not optimal. Fix it, and make this
more easily customizable in general. Use POLLFDS_BASE rather than the
hard-coded "2" for the number of fds we poll besides client connections.
Introduce a maximum number of clients that can connect. When this number is
reached, we simply stop polling the accept socket, so that new connections
aren't accepted any more.  Don't attempt to realloc() the pollfd array if the
number of clients decreases. It's unlikely to ever be more than one or two
pages. Finally, there's no need to wake up every 5s. Our signal handling is
robust. Just sleep forever in ppoll() if nothing happens.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/uxlsnr.c | 70 ++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
index ce2b680..cd462b6 100644
--- a/multipathd/uxlsnr.c
+++ b/multipathd/uxlsnr.c
@@ -41,14 +41,25 @@
 #include "cli.h"
 #include "uxlsnr.h"
 
-static struct timespec sleep_time = {5, 0};
-
 struct client {
 	struct list_head node;
 	int fd;
 };
 
-#define MIN_POLLS 1023
+/* The number of fds we poll on, other than individual client connections */
+#define POLLFDS_BASE 2
+#define POLLFD_CHUNK (4096 / sizeof(struct pollfd))
+/* Minimum mumber of pollfds to reserve for clients */
+#define MIN_POLLS (POLLFD_CHUNK - POLLFDS_BASE)
+/*
+ * Max number of client connections allowed
+ * During coldplug, there may be a large number of "multipath -u"
+ * processes connecting.
+ */
+#define MAX_CLIENTS (16384 - POLLFDS_BASE)
+
+/* Compile-time error if POLLFD_CHUNK is too small */
+static __attribute__((unused)) char ___a[-(MIN_POLLS <= 0)];
 
 static LIST_HEAD(clients);
 static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -282,13 +293,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 	char *inbuf;
 	char *reply;
 	sigset_t mask;
-	int old_clients = MIN_POLLS;
+	int max_pfds = MIN_POLLS + POLLFDS_BASE;
 	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
 	unsigned int sequence_nr = 0;
 	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
 
 	condlog(3, "uxsock: startup listener");
-	polls = (struct pollfd *)MALLOC((MIN_POLLS + 2) * sizeof(struct pollfd));
+	polls = MALLOC(max_pfds * sizeof(*polls));
 	if (!polls) {
 		condlog(0, "uxsock: failed to allocate poll fds");
 		exit_daemon();
@@ -312,28 +323,33 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		list_for_each_entry(c, &clients, node) {
 			num_clients++;
 		}
-		if (num_clients != old_clients) {
+		if (num_clients + POLLFDS_BASE > max_pfds) {
 			struct pollfd *new;
-			if (num_clients <= MIN_POLLS && old_clients > MIN_POLLS) {
-				new = REALLOC(polls, (2 + MIN_POLLS) *
-						sizeof(struct pollfd));
-			} else if (num_clients <= MIN_POLLS && old_clients <= MIN_POLLS) {
-				new = polls;
-			} else {
-				new = REALLOC(polls, (2 + num_clients) *
-						sizeof(struct pollfd));
-			}
-			if (!new) {
-				condlog(0, "%s: failed to realloc %d poll fds",
-					"uxsock", 2 + num_clients);
-				num_clients = old_clients;
-			} else {
-				old_clients = num_clients;
+			int n_new = max_pfds + POLLFD_CHUNK;
+
+			new = REALLOC(polls, n_new * sizeof(*polls));
+			if (new) {
+				max_pfds = n_new;
 				polls = new;
+			} else {
+				condlog(1, "%s: realloc failure, %d clients not served",
+					__func__,
+					num_clients + POLLFDS_BASE - max_pfds);
+				num_clients = max_pfds - POLLFDS_BASE;
 			}
 		}
-		polls[0].fd = ux_sock;
-		polls[0].events = POLLIN;
+		if (num_clients < MAX_CLIENTS) {
+			polls[0].fd = ux_sock;
+			polls[0].events = POLLIN;
+		} else {
+			/*
+			 * New clients can't connect, num_clients won't grow
+			 * to MAX_CLIENTS or higher
+			 */
+			condlog(1, "%s: max client connections reached, pausing polling",
+				__func__);
+			polls[0].fd = -1;
+		}
 
 		reset_watch(notify_fd, &wds, &sequence_nr);
 		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
@@ -343,19 +359,19 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		polls[1].events = POLLIN;
 
 		/* setup the clients */
-		i = 2;
+		i = POLLFDS_BASE;
 		list_for_each_entry(c, &clients, node) {
 			polls[i].fd = c->fd;
 			polls[i].events = POLLIN;
 			i++;
-			if (i >= 2 + num_clients)
+			if (i >= max_pfds)
 				break;
 		}
 		n_pfds = i;
 		pthread_cleanup_pop(1);
 
 		/* most of our life is spent in this call */
-		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
+		poll_count = ppoll(polls, n_pfds, NULL, &mask);
 
 		handle_signals(false);
 		if (poll_count == -1) {
@@ -388,7 +404,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
 		}
 
 		/* see if a client wants to speak to us */
-		for (i = 2; i < n_pfds; i++) {
+		for (i = POLLFDS_BASE; i < n_pfds; i++) {
 			if (polls[i].revents & POLLIN) {
 				struct timespec start_time;
 
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()
  2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
                   ` (27 preceding siblings ...)
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen() mwilck
@ 2020-10-16 10:45 ` mwilck
  2020-10-20  2:20   ` Benjamin Marzinski
  28 siblings, 1 reply; 41+ messages in thread
From: mwilck @ 2020-10-16 10:45 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: lixiaokeng, dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

log_safe() could race with log_thread_stop(); simply
checking the value of log_thr has never been safe. By converting the
mutexes to static initializers, we avoid having to destroy them, and thus
possibly accessing a destroyed mutex in log_safe(). Furthermore, taking
both the logev_lock and the logq_lock makes sure the logarea isn't freed
while we are writing to it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log_pthread.c | 39 ++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 3c73941..91c9c19 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -17,31 +17,42 @@
 
 static pthread_t log_thr;
 
-static pthread_mutex_t logq_lock;
-static pthread_mutex_t logev_lock;
-static pthread_cond_t logev_cond;
+/* logev_lock must not be taken with logq_lock held */
+static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
 
 static int logq_running;
 static int log_messages_pending;
 
 void log_safe (int prio, const char * fmt, va_list ap)
 {
+	bool running;
+
 	if (prio > LOG_DEBUG)
 		prio = LOG_DEBUG;
 
-	if (log_thr == (pthread_t)0) {
-		vsyslog(prio, fmt, ap);
-		return;
-	}
+	/*
+	 * logev_lock protects logq_running. By holding it, we avoid a race
+	 * with log_thread_stop() -> log_close(), which would free the logarea.
+	 */
+	pthread_mutex_lock(&logev_lock);
+	pthread_cleanup_push(cleanup_mutex, &logev_lock);
+	running = logq_running;
 
-	pthread_mutex_lock(&logq_lock);
-	log_enqueue(prio, fmt, ap);
-	pthread_mutex_unlock(&logq_lock);
+	if (running) {
+		pthread_mutex_lock(&logq_lock);
+		pthread_cleanup_push(cleanup_mutex, &logq_lock);
+		log_enqueue(prio, fmt, ap);
+		pthread_cleanup_pop(1);
 
-	pthread_mutex_lock(&logev_lock);
-	log_messages_pending = 1;
-	pthread_cond_signal(&logev_cond);
-	pthread_mutex_unlock(&logev_lock);
+		log_messages_pending = 1;
+		pthread_cond_signal(&logev_cond);
+	}
+	pthread_cleanup_pop(1);
+
+	if (!running)
+		vsyslog(prio, fmt, ap);
 }
 
 static void flush_logqueue (void)
-- 
2.28.0


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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink mwilck
@ 2020-10-16 20:13   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-16 20:13 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:57PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>

Git complains about this patch adding new blank line at EOF
in libmultipath/debug.h

Otherwise, it looks good.
-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/debug.c     |  4 ++--
>  libmultipath/debug.h     |  7 +++++++
>  libmultipath/devmapper.c |  4 ++--
>  multipath/main.c         |  4 ++--
>  multipathd/main.c        | 17 ++++++++---------
>  tests/globals.c          |  3 ++-
>  tests/hwtable.c          |  2 +-
>  7 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index a1713b9..f9b7755 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -25,8 +25,8 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  	va_start(ap, fmt);
>  
>  	if (prio <= libmp_verbosity) {
> -		if (sink < 1) {
> -			if (sink == 0) {
> +		if (sink != LOGSINK_SYSLOG) {
> +			if (sink == LOGSINK_STDERR_WITH_TIME) {
>  				time_t t = time(NULL);
>  				struct tm *tb = localtime(&t);
>  				char buff[16];
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index 1f3bc8b..91ba02b 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -12,3 +12,10 @@ extern int libmp_verbosity;
>  
>  #define condlog(prio, fmt, args...) \
>  	dlog(logsink, prio, fmt "\n", ##args)
> +
> +enum {
> +	LOGSINK_STDERR_WITH_TIME = 0,
> +	LOGSINK_STDERR_WITHOUT_TIME = -1,
> +	LOGSINK_SYSLOG = 1,
> +};
> +
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index a8e0611..78112a9 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -104,8 +104,8 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
>  		return;
>  
>  	va_start(ap, f);
> -	if (logsink < 1) {
> -		if (logsink == 0) {
> +	if (logsink != LOGSINK_SYSLOG) {
> +		if (logsink == LOGSINK_STDERR_WITH_TIME) {
>  			time_t t = time(NULL);
>  			struct tm *tb = localtime(&t);
>  			char buff[16];
> diff --git a/multipath/main.c b/multipath/main.c
> index 9bdc5d2..46cfa17 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -840,7 +840,7 @@ main (int argc, char *argv[])
>  	libmultipath_init();
>  	if (atexit(dm_lib_exit) || atexit(libmultipath_exit))
>  		condlog(1, "failed to register cleanup handler for libmultipath: %m");
> -	logsink = 0;
> +	logsink = LOGSINK_STDERR_WITH_TIME;
>  	if (init_config(DEFAULT_CONFIGFILE))
>  		exit(RTVL_FAIL);
>  	if (atexit(uninit_config))
> @@ -991,7 +991,7 @@ main (int argc, char *argv[])
>  	if (dev_type == DEV_UEVENT) {
>  		openlog("multipath", 0, LOG_DAEMON);
>  		setlogmask(LOG_UPTO(libmp_verbosity + 3));
> -		logsink = 1;
> +		logsink = LOGSINK_SYSLOG;
>  	}
>  
>  	set_max_fds(conf->max_fds);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index fbd354c..725a10b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2780,7 +2780,7 @@ handle_signals(bool nonfatal)
>  	}
>  	if (log_reset_sig) {
>  		condlog(2, "reset log (signal)");
> -		if (logsink == 1)
> +		if (logsink == LOGSINK_SYSLOG)
>  			log_thread_reset();
>  	}
>  	reconfig_sig = 0;
> @@ -3032,7 +3032,7 @@ static void cleanup_child(void)
>  		cleanup_dmevent_waiter();
>  
>  	cleanup_pidfile();
> -	if (logsink == 1)
> +	if (logsink == LOGSINK_SYSLOG)
>  		log_thread_stop();
>  
>  	cleanup_conf();
> @@ -3078,7 +3078,7 @@ child (__attribute__((unused)) void *param)
>  	setup_thread_attr(&uevent_attr, DEFAULT_UEVENT_STACKSIZE * 1024, 0);
>  	setup_thread_attr(&waiter_attr, 32 * 1024, 1);
>  
> -	if (logsink == 1) {
> +	if (logsink == LOGSINK_SYSLOG) {
>  		setup_thread_attr(&log_attr, 64 * 1024, 0);
>  		log_thread_start(&log_attr);
>  		pthread_attr_destroy(&log_attr);
> @@ -3309,7 +3309,7 @@ main (int argc, char *argv[])
>  	ANNOTATE_BENIGN_RACE_SIZED(&uxsock_timeout, sizeof(uxsock_timeout),
>  		"Suppress complaints about this scalar variable");
>  
> -	logsink = 1;
> +	logsink = LOGSINK_SYSLOG;
>  
>  	if (getuid() != 0) {
>  		fprintf(stderr, "need to be root\n");
> @@ -3336,9 +3336,8 @@ main (int argc, char *argv[])
>  		switch(arg) {
>  		case 'd':
>  			foreground = 1;
> -			if (logsink > 0)
> -				logsink = 0;
> -			//debug=1; /* ### comment me out ### */
> +			if (logsink == LOGSINK_SYSLOG)
> +				logsink = LOGSINK_STDERR_WITH_TIME;
>  			break;
>  		case 'v':
>  			if (sizeof(optarg) > sizeof(char *) ||
> @@ -3348,7 +3347,7 @@ main (int argc, char *argv[])
>  			libmp_verbosity = verbosity = atoi(optarg);
>  			break;
>  		case 's':
> -			logsink = -1;
> +			logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  			break;
>  		case 'k':
>  			logsink = 0;
> @@ -3381,7 +3380,7 @@ main (int argc, char *argv[])
>  		char * s = cmd;
>  		char * c = s;
>  
> -		logsink = 0;
> +		logsink = LOGSINK_STDERR_WITH_TIME;
>  		if (verbosity)
>  			libmp_verbosity = verbosity;
>  		conf = load_config(DEFAULT_CONFIGFILE);
> diff --git a/tests/globals.c b/tests/globals.c
> index 8add5eb..fc0c07a 100644
> --- a/tests/globals.c
> +++ b/tests/globals.c
> @@ -1,9 +1,10 @@
>  #include "structs.h"
>  #include "config.h"
> +#include "debug.h"
>  
>  /* Required globals */
>  struct udev *udev;
> -int logsink = -1;
> +int logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  struct config conf = {
>  	.verbosity = 4,
>  };
> diff --git a/tests/hwtable.c b/tests/hwtable.c
> index 57f832b..4dd0873 100644
> --- a/tests/hwtable.c
> +++ b/tests/hwtable.c
> @@ -53,7 +53,7 @@ struct hwt_state {
>  
>  static struct config *_conf;
>  struct udev *udev;
> -int logsink = -1;
> +int logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  
>  struct config *get_multipath_config(void)
>  {
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 07/29] multipathd: move conf destruction into separate function
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 07/29] multipathd: move conf " mwilck
@ 2020-10-19 18:56   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 18:56 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:39PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Also removing the comment about dlog() and dm_write_log().
> dlog() can cope with get_multipath_config() returning NULL,
> and dm_write_log() hasn't accessed the configuration for a while.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 8b9df55..722ef69 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2892,6 +2892,16 @@ set_oom_adj (void)
>  	condlog(0, "couldn't adjust oom score");
>  }
>  
> +static void cleanup_conf(void) {
> +	struct config *conf;
> +
> +	conf = rcu_dereference(multipath_conf);
> +	if (!conf)
> +		return;
> +	rcu_assign_pointer(multipath_conf, NULL);
> +	call_rcu(&conf->rcu, rcu_free_config);
> +}
> +
>  static void cleanup_maps(struct vectors *vecs)
>  {
>  	int queue_without_daemon, i;
> @@ -3196,15 +3206,7 @@ child (__attribute__((unused)) void *param)
>  
>  	if (logsink == 1)
>  		log_thread_stop();
> -
> -	/*
> -	 * Freeing config must be done after condlog() and dm_lib_exit(),
> -	 * because logging functions like dlog() and dm_write_log()
> -	 * reference the config.
> -	 */
> -	conf = rcu_dereference(multipath_conf);
> -	rcu_assign_pointer(multipath_conf, NULL);
> -	call_rcu(&conf->rcu, rcu_free_config);
> +	cleanup_conf();
>  #ifdef _DEBUG_
>  	dbg_free_final(NULL);
>  #endif
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit()
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit() mwilck
@ 2020-10-19 19:07   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 19:07 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:46PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This function prepares for calling dm_lib_exit() on program exit.
> It undoes changes to libdm internals done by libmultipath.
> It doesn't call dm_lib_exit(), as the caller may want to keep
> libdm active.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/config.c    |  1 +
>  libmultipath/config.h    |  2 ++
>  libmultipath/devmapper.c | 15 +++++++++++++++
>  libmultipath/devmapper.h |  1 +
>  4 files changed, 19 insertions(+)
> 
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index f74417c..b9cb413 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -60,6 +60,7 @@ int libmultipath_init(void)
>  static void _libmultipath_exit(void)
>  {
>  	libmultipath_exit_called = true;
> +	libmp_dm_exit();
>  	udev_unref(udev);
>  }
>  
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index f478df7..5d46035 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -271,6 +271,8 @@ int libmultipath_init(void);
>   *
>   * This function un-initializes libmultipath data structures.
>   * It is recommended to call this function at program exit.
> + * If the application also calls dm_lib_exit(), it should do so
> + * after libmultipath_exit().
>   *
>   * Calls to libmultipath_init() after libmultipath_exit() will fail
>   * (in other words, libmultipath can't be re-initialized).
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 2e3ae8a..a8e0611 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -333,6 +333,20 @@ void libmp_udev_set_sync_support(int on)
>  	libmp_dm_udev_sync = !!on;
>  }
>  
> +static bool libmp_dm_init_called;
> +void libmp_dm_exit(void)
> +{
> +	if (!libmp_dm_init_called)
> +		return;
> +
> +	/* switch back to default libdm logging */
> +	dm_log_init(NULL);
> +#ifdef LIBDM_API_HOLD_CONTROL
> +	/* make sure control fd is closed in dm_lib_release() */
> +	dm_hold_control_dev(0);
> +#endif
> +}
> +
>  static void libmp_dm_init(void)
>  {
>  	struct config *conf;
> @@ -349,6 +363,7 @@ static void libmp_dm_init(void)
>  	dm_hold_control_dev(1);
>  #endif
>  	dm_udev_set_sync_support(libmp_dm_udev_sync);
> +	libmp_dm_init_called = true;
>  }
>  
>  static void _do_skip_libmp_dm_init(void)
> diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> index 158057e..ecf2e66 100644
> --- a/libmultipath/devmapper.h
> +++ b/libmultipath/devmapper.h
> @@ -35,6 +35,7 @@ enum {
>  
>  int dm_prereq(unsigned int *v);
>  void skip_libmp_dm_init(void);
> +void libmp_dm_exit(void);
>  void libmp_udev_set_sync_support(int on);
>  struct dm_task *libmp_dm_task_create(int task);
>  int dm_simplecmd_flush (int, const char *, uint16_t);
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown mwilck
@ 2020-10-19 20:00   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 20:00 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:50PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> This fixes several issues with the log_thread. First, the running
> flag logq_running should be set by the thread itself, not by
> log_thread_start()/_stop(). Second, the thread was both cancelled and
> terminated via a flag (again, logq_running). It's sufficient,
> and better, to just cancel it and use logq_running as indication for
> successful termination. Third, the locking wasn't cancel-safe in some
> places. Forth, log_thread_start() and log_thread_stop() didn't wait for
> startup/teardown properly. Fifth, using (pthread_t)0 is wrong (pthread_t is
> opaque; there's no guarantee that 0 is not a valid pthread_t value). Sixth,
> pthread_cancel() was called under logq_lock, which doesn't make sense to
> me at all.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log_pthread.c | 72 ++++++++++++++++++++++++++------------
>  1 file changed, 49 insertions(+), 23 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 0c327ff..3c73941 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -13,6 +13,7 @@
>  #include "log_pthread.h"
>  #include "log.h"
>  #include "lock.h"
> +#include "util.h"
>  
>  static pthread_t log_thr;
>  
> @@ -56,44 +57,70 @@ static void flush_logqueue (void)
>  	} while (empty == 0);
>  }
>  
> +static void cleanup_log_thread(__attribute((unused)) void *arg)
> +{
> +	logdbg(stderr, "log thread exiting");
> +	pthread_mutex_lock(&logev_lock);
> +	logq_running = 0;
> +	pthread_mutex_unlock(&logev_lock);
> +}
> +
>  static void * log_thread (__attribute__((unused)) void * et)
>  {
>  	int running;
>  
>  	pthread_mutex_lock(&logev_lock);
> -	logq_running = 1;
> +	running = logq_running;
> +	if (!running)
> +		logq_running = 1;
> +	pthread_cond_signal(&logev_cond);
>  	pthread_mutex_unlock(&logev_lock);
> +	if (running)
> +		/* already started */
> +		return NULL;
> +	pthread_cleanup_push(cleanup_log_thread, NULL);
>  
>  	mlockall(MCL_CURRENT | MCL_FUTURE);
>  	logdbg(stderr,"enter log_thread\n");
>  
>  	while (1) {
>  		pthread_mutex_lock(&logev_lock);
> -		if (logq_running && !log_messages_pending)
> +		pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +		while (!log_messages_pending)
> +			/* this is a cancellation point */
>  			pthread_cond_wait(&logev_cond, &logev_lock);
>  		log_messages_pending = 0;
> -		running = logq_running;
> -		pthread_mutex_unlock(&logev_lock);
> -		if (!running)
> -			break;
> +		pthread_cleanup_pop(1);
> +
>  		flush_logqueue();
>  	}
> +	pthread_cleanup_pop(1);
>  	return NULL;
>  }
>  
>  void log_thread_start (pthread_attr_t *attr)
>  {
> -	logdbg(stderr,"enter log_thread_start\n");
> +	bool err = false;
>  
> -	pthread_mutex_init(&logq_lock, NULL);
> -	pthread_mutex_init(&logev_lock, NULL);
> -	pthread_cond_init(&logev_cond, NULL);

If you remove these initializers, then these variables are
uninitialized. I realize that patch 29 makes them statically
initialized, so I'm not sure how big of an issue it is.  I guess it
depends on how much we care about the possiblity of things not working
right when bisecting an issue.

-Ben

> +	logdbg(stderr,"enter log_thread_start\n");
>  
>  	if (log_init("multipathd", 0)) {
>  		fprintf(stderr,"can't initialize log buffer\n");
>  		exit(1);
>  	}
> +
> +	pthread_mutex_lock(&logev_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
>  	if (pthread_create(&log_thr, attr, log_thread, NULL)) {
> +		err = true;
> +	}
> +
> +	/* wait for thread startup */
> +	while (!logq_running)
> +		pthread_cond_wait(&logev_cond, &logev_lock);
> +	pthread_cleanup_pop(1);
> +
> +	if (err) {
>  		fprintf(stderr,"can't start log thread\n");
>  		exit(1);
>  	}
> @@ -112,27 +139,26 @@ void log_thread_reset (void)
>  
>  void log_thread_stop (void)
>  {
> +	int running;
> +
>  	if (!la)
>  		return;
>  
>  	logdbg(stderr,"enter log_thread_stop\n");
>  
>  	pthread_mutex_lock(&logev_lock);
> -	logq_running = 0;
> -	pthread_cond_signal(&logev_cond);
> -	pthread_mutex_unlock(&logev_lock);
> -
> -	pthread_mutex_lock(&logq_lock);
> -	pthread_cancel(log_thr);
> -	pthread_mutex_unlock(&logq_lock);
> -	pthread_join(log_thr, NULL);
> -	log_thr = (pthread_t)0;
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +	running = logq_running;
> +	if (running) {
> +		pthread_cancel(log_thr);
> +		pthread_cond_signal(&logev_cond);
> +	}
> +	pthread_cleanup_pop(1);
>  
> -	flush_logqueue();
> +	if (running)
> +		pthread_join(log_thr, NULL);
>  
> -	pthread_mutex_destroy(&logq_lock);
> -	pthread_mutex_destroy(&logev_lock);
> -	pthread_cond_destroy(&logev_cond);
>  
> +	flush_logqueue();
>  	log_close();
>  }
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions mwilck
@ 2020-10-19 20:01   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 20:01 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:55PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> These leaks are caused by other libraries (libsystemd, glibc,
> libgcrypt) and should be ignored when debugging with valgrind
> 
> Usage example:
> 
> valgrind --suppressions=mpath-tools.supp \
>     --leak-check=full --show-leak-kinds=all $COMMAND
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  third-party/valgrind/mpath-tools.supp | 32 +++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 third-party/valgrind/mpath-tools.supp
> 
> diff --git a/third-party/valgrind/mpath-tools.supp b/third-party/valgrind/mpath-tools.supp
> new file mode 100644
> index 0000000..0537fd5
> --- /dev/null
> +++ b/third-party/valgrind/mpath-tools.supp
> @@ -0,0 +1,32 @@
> +{
> +   glibc _dlerror_run leak: https://stackoverflow.com/questions/1542457/memory-leak-reported-by-valgrind-in-dlopen
> +   Memcheck:Leak
> +   match-leak-kinds: reachable
> +   fun:calloc
> +   fun:_dlerror_run
> +   fun:dlopen*
> +}
> +
> +{
> +   systemd mempools are never freed: https://bugzilla.redhat.com/show_bug.cgi?id=1215670
> +   Memcheck:Leak
> +   match-leak-kinds: reachable
> +   fun:malloc
> +   fun:mempool_alloc_tile
> +   fun:mempool_alloc0_tile
> +   fun:hashmap_base_new
> +   fun:hashmap_base_ensure_allocated
> +}
> +
> +{
> +   libgcrypt library initialization
> +   Memcheck:Leak
> +   match-leak-kinds: reachable
> +   fun:malloc
> +   ...
> +   fun:_gcry_xmalloc
> +   ...
> +   fun:global_init.*
> +   ...
> +   fun:_dl_init
> +}
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity mwilck
@ 2020-10-19 20:38   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 20:38 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:56PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Introduce a new global variable to set the verbosity of libmultipath.
> This avoids accessing the configuration in every dlog() call.
> When libmultipath reads its configuration in init_config() or
> load_config(), it will use the current value of libmp_verbosity
> for logging. Immediately before returning, libmp_verbosity will be
> overwritten with the verbosity value from the configuration file,
> if it was set there. An application is free to set libmp_verbosity
> back to the previous value or not after that, depending on whether
> command line options or configuration file settings should take
> precedence.

Is there some reason why domap() and coalesce_paths() call
print_multipath_topology() and libmp_dm_init() calls dm_init() using
conf->verbosity instead of libmp_verbosity?

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmpathpersist/mpath_persist.c   |  5 +---
>  libmultipath/config.c             |  9 +++++--
>  libmultipath/debug.c              | 10 ++------
>  libmultipath/debug.h              |  1 +
>  libmultipath/libmultipath.version |  5 ++++
>  multipath/main.c                  | 21 ++++++----------
>  multipathd/main.c                 | 40 ++++++++++++++++++-------------
>  tests/alias.c                     |  1 +
>  tests/blacklist.c                 |  2 ++
>  9 files changed, 49 insertions(+), 45 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c
> index 9ebf91d..79322e8 100644
> --- a/libmpathpersist/mpath_persist.c
> +++ b/libmpathpersist/mpath_persist.c
> @@ -170,10 +170,7 @@ void mpath_persistent_reserve_free_vecs(void)
>  
>  int mpath_persistent_reserve_init_vecs(int verbose)
>  {
> -	struct config *conf = get_multipath_config();
> -
> -	conf->verbosity = verbose;
> -	put_multipath_config(conf);
> +	libmp_verbosity = verbose;
>  
>  	if (curmp)
>  		return MPATH_PR_SUCCESS;
> diff --git a/libmultipath/config.c b/libmultipath/config.c
> index 52b1447..49e7fb8 100644
> --- a/libmultipath/config.c
> +++ b/libmultipath/config.c
> @@ -828,10 +828,14 @@ int _init_config (const char *file, struct config *conf)
>  		conf = &__internal_config;
>  
>  	/*
> -	 * internal defaults
> +	 * Processing the config file will overwrite conf->verbosity if set
> +	 * When we return, we'll copy the config value back
>  	 */
> -	conf->verbosity = DEFAULT_VERBOSITY;
> +	conf->verbosity = libmp_verbosity;
>  
> +	/*
> +	 * internal defaults
> +	 */
>  	get_sys_max_fds(&conf->max_fds);
>  	conf->bindings_file = set_default(DEFAULT_BINDINGS_FILE);
>  	conf->wwids_file = set_default(DEFAULT_WWIDS_FILE);
> @@ -997,6 +1001,7 @@ int _init_config (const char *file, struct config *conf)
>  	    !conf->wwids_file || !conf->prkeys_file)
>  		goto out;
>  
> +	libmp_verbosity = conf->verbosity;
>  	return 0;
>  out:
>  	_uninit_config(conf);
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index b3a1de9..a1713b9 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -16,21 +16,15 @@
>  #include "debug.h"
>  
>  int logsink;
> +int libmp_verbosity = DEFAULT_VERBOSITY;
>  
>  void dlog (int sink, int prio, const char * fmt, ...)
>  {
>  	va_list ap;
> -	int thres;
> -	struct config *conf;
>  
>  	va_start(ap, fmt);
> -	conf = get_multipath_config();
> -	ANNOTATE_IGNORE_READS_BEGIN();
> -	thres = (conf) ? conf->verbosity : DEFAULT_VERBOSITY;
> -	ANNOTATE_IGNORE_READS_END();
> -	put_multipath_config(conf);
>  
> -	if (prio <= thres) {
> +	if (prio <= libmp_verbosity) {
>  		if (sink < 1) {
>  			if (sink == 0) {
>  				time_t t = time(NULL);
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index c6120c1..1f3bc8b 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -8,6 +8,7 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  #include "log_pthread.h"
>  
>  extern int logsink;
> +extern int libmp_verbosity;
>  
>  #define condlog(prio, fmt, args...) \
>  	dlog(logsink, prio, fmt "\n", ##args)
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 800cff2..67a7379 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -259,3 +259,8 @@ global:
>  local:
>  	*;
>  };
> +
> +LIBMULTIPATH_4.1.0 {
> +global:
> +	libmp_verbosity;
> +} LIBMULTIPATH_4.0.0;
> diff --git a/multipath/main.c b/multipath/main.c
> index 9974993..9bdc5d2 100644
> --- a/multipath/main.c
> +++ b/multipath/main.c
> @@ -208,22 +208,15 @@ get_dm_mpvec (enum mpath_cmds cmd, vector curmp, vector pathvec, char * refwwid)
>  			mpp->bestpg = select_path_group(mpp);
>  
>  		if (cmd == CMD_LIST_SHORT ||
> -		    cmd == CMD_LIST_LONG) {
> -			struct config *conf = get_multipath_config();
> -			print_multipath_topology(mpp, conf->verbosity);
> -			put_multipath_config(conf);
> -		}
> +		    cmd == CMD_LIST_LONG)
> +			print_multipath_topology(mpp, libmp_verbosity);
>  
>  		if (cmd == CMD_CREATE)
>  			reinstate_paths(mpp);
>  	}
>  
> -	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG) {
> -		struct config *conf = get_multipath_config();
> -
> -		print_foreign_topology(conf->verbosity);
> -		put_multipath_config(conf);
> -	}
> +	if (cmd == CMD_LIST_SHORT || cmd == CMD_LIST_LONG)
> +		print_foreign_topology(libmp_verbosity);
>  
>  	return 0;
>  }
> @@ -554,7 +547,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
>  	if (path_discovery(pathvec, di_flag) < 0)
>  		goto out;
>  
> -	if (conf->verbosity > 2)
> +	if (libmp_verbosity > 2)
>  		print_all_paths(pathvec, 1);
>  
>  	get_path_layout(pathvec, 0);
> @@ -866,7 +859,7 @@ main (int argc, char *argv[])
>  				exit(RTVL_FAIL);
>  			}
>  
> -			conf->verbosity = atoi(optarg);
> +			libmp_verbosity = atoi(optarg);
>  			break;
>  		case 'b':
>  			conf->bindings_file = strdup(optarg);
> @@ -997,7 +990,7 @@ main (int argc, char *argv[])
>  	}
>  	if (dev_type == DEV_UEVENT) {
>  		openlog("multipath", 0, LOG_DAEMON);
> -		setlogmask(LOG_UPTO(conf->verbosity + 3));
> +		setlogmask(LOG_UPTO(libmp_verbosity + 3));
>  		logsink = 1;
>  	}
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 33277d5..fbd354c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -88,10 +88,10 @@
>  #define CMDSIZE 160
>  #define MSG_SIZE 32
>  
> -#define LOG_MSG(lvl, verb, pp)					\
> +#define LOG_MSG(lvl, pp)					\
>  do {								\
>  	if (pp->mpp && checker_selected(&pp->checker) &&	\
> -	    lvl <= verb) {					\
> +	    lvl <= libmp_verbosity) {					\
>  		if (pp->offline)				\
>  			condlog(lvl, "%s: %s - path offline",	\
>  				pp->mpp->alias, pp->dev);	\
> @@ -2070,7 +2070,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	int chkr_new_path_up = 0;
>  	int disable_reinstate = 0;
>  	int oldchkrstate = pp->chkrstate;
> -	int retrigger_tries, verbosity;
> +	int retrigger_tries;
>  	unsigned int checkint, max_checkint;
>  	struct config *conf;
>  	int marginal_pathgroups, marginal_changed = 0;
> @@ -2090,7 +2090,6 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	retrigger_tries = conf->retrigger_tries;
>  	checkint = conf->checkint;
>  	max_checkint = conf->max_checkint;
> -	verbosity = conf->verbosity;
>  	marginal_pathgroups = conf->marginal_pathgroups;
>  	put_multipath_config(conf);
>  
> @@ -2152,7 +2151,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  	if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
>  		condlog(2, "%s: unusable path (%s) - checker failed",
>  			pp->dev, checker_state_name(newstate));
> -		LOG_MSG(2, verbosity, pp);
> +		LOG_MSG(2, pp);
>  		conf = get_multipath_config();
>  		pthread_cleanup_push(put_multipath_config, conf);
>  		pathinfo(pp, conf, 0);
> @@ -2257,7 +2256,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  		int oldstate = pp->state;
>  		pp->state = newstate;
>  
> -		LOG_MSG(1, verbosity, pp);
> +		LOG_MSG(1, pp);
>  
>  		/*
>  		 * upon state change, reset the checkint
> @@ -2321,7 +2320,7 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  			/* Clear IO errors */
>  			reinstate_path(pp);
>  		else {
> -			LOG_MSG(4, verbosity, pp);
> +			LOG_MSG(4, pp);
>  			if (pp->checkint != max_checkint) {
>  				/*
>  				 * double the next check delay.
> @@ -2349,9 +2348,9 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks)
>  			log_checker_err = conf->log_checker_err;
>  			put_multipath_config(conf);
>  			if (log_checker_err == LOG_CHKR_ERR_ONCE)
> -				LOG_MSG(3, verbosity, pp);
> +				LOG_MSG(3, pp);
>  			else
> -				LOG_MSG(2, verbosity, pp);
> +				LOG_MSG(2, pp);
>  		}
>  	}
>  
> @@ -2696,6 +2695,10 @@ reconfigure (struct vectors * vecs)
>  	if (!conf)
>  		return 1;
>  
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
> +	setlogmask(LOG_UPTO(libmp_verbosity + 3));
> +
>  	/*
>  	 * free old map and path vectors ... they use old conf state
>  	 */
> @@ -2710,8 +2713,6 @@ reconfigure (struct vectors * vecs)
>  	/* Re-read any timezone changes */
>  	tzset();
>  
> -	if (verbosity)
> -		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
>  	check_alias_settings(conf);
> @@ -3093,14 +3094,18 @@ child (__attribute__((unused)) void *param)
>  	condlog(2, "--------start up--------");
>  	condlog(2, "read " DEFAULT_CONFIGFILE);
>  
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
>  	conf = load_config(DEFAULT_CONFIGFILE);
> +	if (verbosity)
> +		libmp_verbosity = verbosity;
> +	setlogmask(LOG_UPTO(libmp_verbosity + 3));
> +
>  	if (!conf) {
>  		condlog(0, "failed to load configuration");
>  		goto failed;
>  	}
>  
> -	if (verbosity)
> -		conf->verbosity = verbosity;
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
>  	uxsock_timeout = conf->uxsock_timeout;
> @@ -3119,7 +3124,6 @@ child (__attribute__((unused)) void *param)
>  
>  	if (poll_dmevents)
>  		poll_dmevents = dmevent_poll_supported();
> -	setlogmask(LOG_UPTO(conf->verbosity + 3));
>  
>  	envp = getenv("LimitNOFILE");
>  
> @@ -3341,7 +3345,7 @@ main (int argc, char *argv[])
>  			    !isdigit(optarg[0]))
>  				exit(1);
>  
> -			verbosity = atoi(optarg);
> +			libmp_verbosity = verbosity = atoi(optarg);
>  			break;
>  		case 's':
>  			logsink = -1;
> @@ -3352,7 +3356,7 @@ main (int argc, char *argv[])
>  			if (!conf)
>  				exit(1);
>  			if (verbosity)
> -				conf->verbosity = verbosity;
> +				libmp_verbosity = verbosity;
>  			uxsock_timeout = conf->uxsock_timeout;
>  			err = uxclnt(optarg, uxsock_timeout + 100);
>  			free_config(conf);
> @@ -3378,11 +3382,13 @@ main (int argc, char *argv[])
>  		char * c = s;
>  
>  		logsink = 0;
> +		if (verbosity)
> +			libmp_verbosity = verbosity;
>  		conf = load_config(DEFAULT_CONFIGFILE);
>  		if (!conf)
>  			exit(1);
>  		if (verbosity)
> -			conf->verbosity = verbosity;
> +			libmp_verbosity = verbosity;
>  		uxsock_timeout = conf->uxsock_timeout;
>  		memset(cmd, 0x0, CMDSIZE);
>  		while (optind < argc) {
> diff --git a/tests/alias.c b/tests/alias.c
> index 5624138..e5e20b2 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -735,6 +735,7 @@ static int test_allocate_binding(void)
>  int main(void)
>  {
>  	int ret = 0;
> +	libmp_verbosity = conf.verbosity;
>  
>  	ret += test_format_devname();
>  	ret += test_scan_devname();
> diff --git a/tests/blacklist.c b/tests/blacklist.c
> index 84a3ba2..0b42e25 100644
> --- a/tests/blacklist.c
> +++ b/tests/blacklist.c
> @@ -22,6 +22,7 @@
>  #include "globals.c"
>  #include "blacklist.h"
>  #include "test-log.h"
> +#include "debug.h"
>  
>  struct udev_device {
>  	const char *sysname;
> @@ -152,6 +153,7 @@ static int setup(void **state)
>  	    store_ble(blist_property_wwn_inv, "!ID_WWN", ORIGIN_CONFIG))
>  		return -1;
>  
> +	libmp_verbosity = conf.verbosity = 4;
>  	return 0;
>  }
>  
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog()
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog() mwilck
@ 2020-10-19 21:07   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 21:07 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:58PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> By checking the log level in condlog() directly, we can simplify
> dlog(). Also, it's now possible to limit the log level at compile
> time by setting MAX_VERBOSITY, enabling the compiler to optimize
> away log messages with higher loglevel.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/debug.c     | 30 +++++++++++++-----------------
>  libmultipath/debug.h     | 19 +++++++++++++++----
>  libmultipath/devmapper.c |  4 +++-
>  tests/test-log.c         |  4 ++--
>  tests/test-log.h         |  3 ++-
>  5 files changed, 35 insertions(+), 25 deletions(-)
> 
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index f9b7755..429f269 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -18,29 +18,25 @@
>  int logsink;
>  int libmp_verbosity = DEFAULT_VERBOSITY;
>  
> -void dlog (int sink, int prio, const char * fmt, ...)
> +void dlog(int prio, const char * fmt, ...)
>  {
>  	va_list ap;
>  
>  	va_start(ap, fmt);
> +	if (logsink != LOGSINK_SYSLOG) {
> +		if (logsink == LOGSINK_STDERR_WITH_TIME) {
> +			time_t t = time(NULL);
> +			struct tm *tb = localtime(&t);
> +			char buff[16];
>  
> -	if (prio <= libmp_verbosity) {
> -		if (sink != LOGSINK_SYSLOG) {
> -			if (sink == LOGSINK_STDERR_WITH_TIME) {
> -				time_t t = time(NULL);
> -				struct tm *tb = localtime(&t);
> -				char buff[16];
> -
> -				strftime(buff, sizeof(buff),
> -					 "%b %d %H:%M:%S", tb);
> -				buff[sizeof(buff)-1] = '\0';
> -
> -				fprintf(stderr, "%s | ", buff);
> -			}
> -			vfprintf(stderr, fmt, ap);
> +			strftime(buff, sizeof(buff),
> +				 "%b %d %H:%M:%S", tb);
> +			buff[sizeof(buff)-1] = '\0';
> +			fprintf(stderr, "%s | ", buff);
>  		}
> -		else
> -			log_safe(prio + 3, fmt, ap);
> +		vfprintf(stderr, fmt, ap);
>  	}
> +	else
> +		log_safe(prio + 3, fmt, ap);
>  	va_end(ap);
>  }
> diff --git a/libmultipath/debug.h b/libmultipath/debug.h
> index 91ba02b..705a5d7 100644
> --- a/libmultipath/debug.h
> +++ b/libmultipath/debug.h
> @@ -1,5 +1,7 @@
> -void dlog (int sink, int prio, const char * fmt, ...)
> -	__attribute__((format(printf, 3, 4)));
> +#ifndef _DEBUG_H
> +#define _DEBUG_H
> +void dlog (int prio, const char *fmt, ...)
> +	__attribute__((format(printf, 2, 3)));
>  
>  
>  #include <pthread.h>
> @@ -10,8 +12,9 @@ void dlog (int sink, int prio, const char * fmt, ...)
>  extern int logsink;
>  extern int libmp_verbosity;
>  
> -#define condlog(prio, fmt, args...) \
> -	dlog(logsink, prio, fmt "\n", ##args)
> +#ifndef MAX_VERBOSITY
> +#define MAX_VERBOSITY 4
> +#endif
>  
>  enum {
>  	LOGSINK_STDERR_WITH_TIME = 0,
> @@ -19,3 +22,11 @@ enum {
>  	LOGSINK_SYSLOG = 1,
>  };
>  
> +#define condlog(prio, fmt, args...)					\
> +	do {								\
> +		int __p = (prio);					\
> +									\
> +		if (__p <= MAX_VERBOSITY && __p <= libmp_verbosity)	\
> +			dlog(__p, fmt "\n", ##args);			\
> +	} while (0)
> +#endif /* _DEBUG_H */
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 78112a9..cf2f27c 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -274,7 +274,9 @@ static int dm_tgt_prereq (unsigned int *ver)
>  
>  static void _init_versions(void)
>  {
> -	dlog(logsink, 3, VERSION_STRING);
> +	/* Can't use condlog here because of how VERSION_STRING is defined */
> +	if (3 <= libmp_verbosity)
> +		dlog(3, VERSION_STRING);
>  	init_dm_library_version();
>  	init_dm_drv_version();
>  	init_dm_mpath_version();
> diff --git a/tests/test-log.c b/tests/test-log.c
> index 051491e..42ebad0 100644
> --- a/tests/test-log.c
> +++ b/tests/test-log.c
> @@ -6,8 +6,8 @@
>  #include "log.h"
>  #include "test-log.h"
>  
> -__attribute__((format(printf, 3, 0)))
> -void __wrap_dlog (int sink, int prio, const char * fmt, ...)
> +__attribute__((format(printf, 2, 0)))
> +void __wrap_dlog (int prio, const char * fmt, ...)
>  {
>  	char buff[MAX_MSG_SIZE];
>  	va_list ap;
> diff --git a/tests/test-log.h b/tests/test-log.h
> index 2c878c6..6d22cd2 100644
> --- a/tests/test-log.h
> +++ b/tests/test-log.h
> @@ -1,7 +1,8 @@
>  #ifndef _TEST_LOG_H
>  #define _TEST_LOG_H
>  
> -void __wrap_dlog (int sink, int prio, const char * fmt, ...);
> +__attribute__((format(printf, 2, 0)))
> +void __wrap_dlog (int prio, const char * fmt, ...);
>  void expect_condlog(int prio, char *string);
>  
>  #endif
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args mwilck
@ 2020-10-19 21:51   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 21:51 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:59PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> 'multipathd -k"cmd"' and 'multipath cmd' are the same thing.
> Treat it with common code.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 725a10b..0cf8a26 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3303,6 +3303,8 @@ main (int argc, char *argv[])
>  	int err;
>  	int foreground = 0;
>  	struct config *conf;
> +	char *opt_k_arg = NULL;
> +	bool opt_k = false;
>  
>  	ANNOTATE_BENIGN_RACE_SIZED(&multipath_conf, sizeof(multipath_conf),
>  				   "Manipulated through RCU");
> @@ -3350,16 +3352,9 @@ main (int argc, char *argv[])
>  			logsink = LOGSINK_STDERR_WITHOUT_TIME;
>  			break;
>  		case 'k':
> -			logsink = 0;
> -			conf = load_config(DEFAULT_CONFIGFILE);
> -			if (!conf)
> -				exit(1);
> -			if (verbosity)
> -				libmp_verbosity = verbosity;
> -			uxsock_timeout = conf->uxsock_timeout;
> -			err = uxclnt(optarg, uxsock_timeout + 100);
> -			free_config(conf);
> -			return err;
> +			opt_k = true;
> +			opt_k_arg = optarg;
> +			break;
>  		case 'B':
>  			bindings_read_only = 1;
>  			break;
> @@ -3375,7 +3370,7 @@ main (int argc, char *argv[])
>  			exit(1);
>  		}
>  	}
> -	if (optind < argc) {
> +	if (opt_k || optind < argc) {
>  		char cmd[CMDSIZE];
>  		char * s = cmd;
>  		char * c = s;
> @@ -3390,14 +3385,20 @@ main (int argc, char *argv[])
>  			libmp_verbosity = verbosity;
>  		uxsock_timeout = conf->uxsock_timeout;
>  		memset(cmd, 0x0, CMDSIZE);
> -		while (optind < argc) {
> -			if (strchr(argv[optind], ' '))
> -				c += snprintf(c, s + CMDSIZE - c, "\"%s\" ", argv[optind]);
> -			else
> -				c += snprintf(c, s + CMDSIZE - c, "%s ", argv[optind]);
> -			optind++;
> +		if (opt_k)
> +			s = opt_k_arg;
> +		else {
> +			while (optind < argc) {
> +				if (strchr(argv[optind], ' '))
> +					c += snprintf(c, s + CMDSIZE - c,
> +						      "\"%s\" ", argv[optind]);
> +				else
> +					c += snprintf(c, s + CMDSIZE - c,
> +						      "%s ", argv[optind]);
> +				optind++;
> +			}
> +			c += snprintf(c, s + CMDSIZE - c, "\n");

combining these two methods makes is obvious that adding the newline to
the cmd buffer is unnecessary. But since your patch didn't add it, and
it doesn't hurt anything
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

>  		}
> -		c += snprintf(c, s + CMDSIZE - c, "\n");
>  		err = uxclnt(s, uxsock_timeout + 100);
>  		free_config(conf);
>  		return err;
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen()
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen() mwilck
@ 2020-10-19 23:33   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-19 23:33 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:45:00PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We were allocating 1025 poll fds, which is not optimal. Fix it, and make this
> more easily customizable in general. Use POLLFDS_BASE rather than the
> hard-coded "2" for the number of fds we poll besides client connections.
> Introduce a maximum number of clients that can connect. When this number is
> reached, we simply stop polling the accept socket, so that new connections
> aren't accepted any more.  Don't attempt to realloc() the pollfd array if the
> number of clients decreases. It's unlikely to ever be more than one or two
> pages. Finally, there's no need to wake up every 5s. Our signal handling is
> robust. Just sleep forever in ppoll() if nothing happens.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/uxlsnr.c | 70 ++++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 27 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index ce2b680..cd462b6 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -41,14 +41,25 @@
>  #include "cli.h"
>  #include "uxlsnr.h"
>  
> -static struct timespec sleep_time = {5, 0};
> -
>  struct client {
>  	struct list_head node;
>  	int fd;
>  };
>  
> -#define MIN_POLLS 1023
> +/* The number of fds we poll on, other than individual client connections */
> +#define POLLFDS_BASE 2
> +#define POLLFD_CHUNK (4096 / sizeof(struct pollfd))
> +/* Minimum mumber of pollfds to reserve for clients */
> +#define MIN_POLLS (POLLFD_CHUNK - POLLFDS_BASE)

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

I have one nitpick. This code looks like it's pretending to allocate
pages of memory, when it's not. Malloc's bookeeping space means that
this memory chunk will be larger than a page. Even if it was page sized,
unless userspace is specifically asking for page-aligned memory, it most
like won't get it. Since AFAIK there is no benefit to mallocing memory
in a specific size increment, it doesn't seem woirth adding any
complexity to make sure our mallocs do that.

-Ben

> +/*
> + * Max number of client connections allowed
> + * During coldplug, there may be a large number of "multipath -u"
> + * processes connecting.
> + */
> +#define MAX_CLIENTS (16384 - POLLFDS_BASE)
> +
> +/* Compile-time error if POLLFD_CHUNK is too small */
> +static __attribute__((unused)) char ___a[-(MIN_POLLS <= 0)];
>  
>  static LIST_HEAD(clients);
>  static pthread_mutex_t client_lock = PTHREAD_MUTEX_INITIALIZER;
> @@ -282,13 +293,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  	char *inbuf;
>  	char *reply;
>  	sigset_t mask;
> -	int old_clients = MIN_POLLS;
> +	int max_pfds = MIN_POLLS + POLLFDS_BASE;
>  	/* conf->sequence_nr will be 1 when uxsock_listen is first called */
>  	unsigned int sequence_nr = 0;
>  	struct watch_descriptors wds = { .conf_wd = -1, .dir_wd = -1 };
>  
>  	condlog(3, "uxsock: startup listener");
> -	polls = (struct pollfd *)MALLOC((MIN_POLLS + 2) * sizeof(struct pollfd));
> +	polls = MALLOC(max_pfds * sizeof(*polls));
>  	if (!polls) {
>  		condlog(0, "uxsock: failed to allocate poll fds");
>  		exit_daemon();
> @@ -312,28 +323,33 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  		list_for_each_entry(c, &clients, node) {
>  			num_clients++;
>  		}
> -		if (num_clients != old_clients) {
> +		if (num_clients + POLLFDS_BASE > max_pfds) {
>  			struct pollfd *new;
> -			if (num_clients <= MIN_POLLS && old_clients > MIN_POLLS) {
> -				new = REALLOC(polls, (2 + MIN_POLLS) *
> -						sizeof(struct pollfd));
> -			} else if (num_clients <= MIN_POLLS && old_clients <= MIN_POLLS) {
> -				new = polls;
> -			} else {
> -				new = REALLOC(polls, (2 + num_clients) *
> -						sizeof(struct pollfd));
> -			}
> -			if (!new) {
> -				condlog(0, "%s: failed to realloc %d poll fds",
> -					"uxsock", 2 + num_clients);
> -				num_clients = old_clients;
> -			} else {
> -				old_clients = num_clients;
> +			int n_new = max_pfds + POLLFD_CHUNK;
> +
> +			new = REALLOC(polls, n_new * sizeof(*polls));
> +			if (new) {
> +				max_pfds = n_new;
>  				polls = new;
> +			} else {
> +				condlog(1, "%s: realloc failure, %d clients not served",
> +					__func__,
> +					num_clients + POLLFDS_BASE - max_pfds);
> +				num_clients = max_pfds - POLLFDS_BASE;
>  			}
>  		}
> -		polls[0].fd = ux_sock;
> -		polls[0].events = POLLIN;
> +		if (num_clients < MAX_CLIENTS) {
> +			polls[0].fd = ux_sock;
> +			polls[0].events = POLLIN;
> +		} else {
> +			/*
> +			 * New clients can't connect, num_clients won't grow
> +			 * to MAX_CLIENTS or higher
> +			 */
> +			condlog(1, "%s: max client connections reached, pausing polling",
> +				__func__);
> +			polls[0].fd = -1;
> +		}
>  
>  		reset_watch(notify_fd, &wds, &sequence_nr);
>  		if (notify_fd == -1 || (wds.conf_wd == -1 && wds.dir_wd == -1))
> @@ -343,19 +359,19 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  		polls[1].events = POLLIN;
>  
>  		/* setup the clients */
> -		i = 2;
> +		i = POLLFDS_BASE;
>  		list_for_each_entry(c, &clients, node) {
>  			polls[i].fd = c->fd;
>  			polls[i].events = POLLIN;
>  			i++;
> -			if (i >= 2 + num_clients)
> +			if (i >= max_pfds)
>  				break;
>  		}
>  		n_pfds = i;
>  		pthread_cleanup_pop(1);
>  
>  		/* most of our life is spent in this call */
> -		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
> +		poll_count = ppoll(polls, n_pfds, NULL, &mask);
>  
>  		handle_signals(false);
>  		if (poll_count == -1) {
> @@ -388,7 +404,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  		}
>  
>  		/* see if a client wants to speak to us */
> -		for (i = 2; i < n_pfds; i++) {
> +		for (i = POLLFDS_BASE; i < n_pfds; i++) {
>  			if (polls[i].revents & POLLIN) {
>  				struct timespec start_time;
>  
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()
  2020-10-16 10:45 ` [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop() mwilck
@ 2020-10-20  2:20   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-20  2:20 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:45:01PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> log_safe() could race with log_thread_stop(); simply
> checking the value of log_thr has never been safe. By converting the
> mutexes to static initializers, we avoid having to destroy them, and thus
> possibly accessing a destroyed mutex in log_safe(). Furthermore, taking
> both the logev_lock and the logq_lock makes sure the logarea isn't freed
> while we are writing to it.
> 

I don't see any problems with this, but I also don't think it's necssary
to hold the log thread lock (logev_lock), just to add a message to the
queue. It seems like protecting the log queue is the job of logq_lock.
As long as log_safe() enqueues the message before flush_logqueue() is
called in log_thread_stop(), it should be fine. This could be solved by
simply having a static variable in log_pthread.c named something like
log_area_enabled, that always accessed while holding the logq_lock, and
is set to true when the log_area is created, and set to false just
before calling the flush_logqueue() in log_thread_stop(). Then log_safe
just needs to check this before calling log_enqueue(). If it's true,
then logged messages will get flushed, so it can safely enqueue the
message. If it's false, then log_safe should call vsyslog() directly. 

In fairness, if the argument is that we should try to only lock what's
absolutely necessary, then multipath has far worse offenders than
logev_lock. So I don't feel super strongly about this, if there's a
reason why you like your way better.

-Ben

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log_pthread.c | 39 ++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 3c73941..91c9c19 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -17,31 +17,42 @@
>  
>  static pthread_t log_thr;
>  
> -static pthread_mutex_t logq_lock;
> -static pthread_mutex_t logev_lock;
> -static pthread_cond_t logev_cond;
> +/* logev_lock must not be taken with logq_lock held */
> +static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_mutex_t logev_lock = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t logev_cond = PTHREAD_COND_INITIALIZER;
>  
>  static int logq_running;
>  static int log_messages_pending;
>  
>  void log_safe (int prio, const char * fmt, va_list ap)
>  {
> +	bool running;
> +
>  	if (prio > LOG_DEBUG)
>  		prio = LOG_DEBUG;
>  
> -	if (log_thr == (pthread_t)0) {
> -		vsyslog(prio, fmt, ap);
> -		return;
> -	}
> +	/*
> +	 * logev_lock protects logq_running. By holding it, we avoid a race
> +	 * with log_thread_stop() -> log_close(), which would free the logarea.
> +	 */
> +	pthread_mutex_lock(&logev_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logev_lock);
> +	running = logq_running;
>  
> -	pthread_mutex_lock(&logq_lock);
> -	log_enqueue(prio, fmt, ap);
> -	pthread_mutex_unlock(&logq_lock);
> +	if (running) {
> +		pthread_mutex_lock(&logq_lock);
> +		pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +		log_enqueue(prio, fmt, ap);
> +		pthread_cleanup_pop(1);
>  
> -	pthread_mutex_lock(&logev_lock);
> -	log_messages_pending = 1;
> -	pthread_cond_signal(&logev_cond);
> -	pthread_mutex_unlock(&logev_lock);
> +		log_messages_pending = 1;
> +		pthread_cond_signal(&logev_cond);
> +	}
> +	pthread_cleanup_pop(1);
> +
> +	if (!running)
> +		vsyslog(prio, fmt, ap);
>  }
>  
>  static void flush_logqueue (void)
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock on exit
  2020-10-16 10:44 ` [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock " mwilck
@ 2020-10-20 19:04   ` Benjamin Marzinski
  0 siblings, 0 replies; 41+ messages in thread
From: Benjamin Marzinski @ 2020-10-20 19:04 UTC (permalink / raw)
  To: mwilck; +Cc: lixiaokeng, dm-devel

On Fri, Oct 16, 2020 at 12:44:33PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The uxlsnr wouldn't always release the client lock when cancelled,
> causing a deadlock in uxsock_cleanup(). While this hasn't been
> caused by commit 3d611a2, the deadlock seems to have become much
> more likely after that patch. Solving this means that we have to
> treat reallocation failure of the pollfd array differently.
> We will now just ignore any clients above the last valid pfd index.
> That's a minor problem, as we're in an OOM situation anyway.
> 
> Moreover, client_lock is not a "struct lock", but a plain
> pthread_mutex_t.
> 
> Fixes: 3d611a2 ("multipathd: cancel threads early during shutdown")

Oops. Forgot to send this one.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/uxlsnr.c | 24 ++++++++++++++----------
>  1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/multipathd/uxlsnr.c b/multipathd/uxlsnr.c
> index 1c5ce9d..ce2b680 100644
> --- a/multipathd/uxlsnr.c
> +++ b/multipathd/uxlsnr.c
> @@ -35,6 +35,7 @@
>  #include "config.h"
>  #include "mpath_cmd.h"
>  #include "time-util.h"
> +#include "util.h"
>  
>  #include "main.h"
>  #include "cli.h"
> @@ -116,7 +117,7 @@ static void _dead_client(struct client *c)
>  
>  static void dead_client(struct client *c)
>  {
> -	pthread_cleanup_push(cleanup_lock, &client_lock);
> +	pthread_cleanup_push(cleanup_mutex, &client_lock);
>  	pthread_mutex_lock(&client_lock);
>  	_dead_client(c);
>  	pthread_cleanup_pop(1);
> @@ -302,10 +303,11 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  	sigdelset(&mask, SIGUSR1);
>  	while (1) {
>  		struct client *c, *tmp;
> -		int i, poll_count, num_clients;
> +		int i, n_pfds, poll_count, num_clients;
>  
>  		/* setup for a poll */
>  		pthread_mutex_lock(&client_lock);
> +		pthread_cleanup_push(cleanup_mutex, &client_lock);
>  		num_clients = 0;
>  		list_for_each_entry(c, &clients, node) {
>  			num_clients++;
> @@ -322,14 +324,13 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  						sizeof(struct pollfd));
>  			}
>  			if (!new) {
> -				pthread_mutex_unlock(&client_lock);
>  				condlog(0, "%s: failed to realloc %d poll fds",
>  					"uxsock", 2 + num_clients);
> -				sched_yield();
> -				continue;
> +				num_clients = old_clients;
> +			} else {
> +				old_clients = num_clients;
> +				polls = new;
>  			}
> -			old_clients = num_clients;
> -			polls = new;
>  		}
>  		polls[0].fd = ux_sock;
>  		polls[0].events = POLLIN;
> @@ -347,11 +348,14 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  			polls[i].fd = c->fd;
>  			polls[i].events = POLLIN;
>  			i++;
> +			if (i >= 2 + num_clients)
> +				break;
>  		}
> -		pthread_mutex_unlock(&client_lock);
> +		n_pfds = i;
> +		pthread_cleanup_pop(1);
>  
>  		/* most of our life is spent in this call */
> -		poll_count = ppoll(polls, i, &sleep_time, &mask);
> +		poll_count = ppoll(polls, n_pfds, &sleep_time, &mask);
>  
>  		handle_signals(false);
>  		if (poll_count == -1) {
> @@ -384,7 +388,7 @@ void * uxsock_listen(uxsock_trigger_fn uxsock_trigger, long ux_sock,
>  		}
>  
>  		/* see if a client wants to speak to us */
> -		for (i = 2; i < num_clients + 2; i++) {
> +		for (i = 2; i < n_pfds; i++) {
>  			if (polls[i].revents & POLLIN) {
>  				struct timespec start_time;
>  
> -- 
> 2.28.0

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, back to index

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-16 10:44 [dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock " mwilck
2020-10-20 19:04   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 02/29] multipathd: Fix liburcu memory leak mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 03/29] multipathd: move handling of io_err_stat_attr into libmultipath mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 04/29] multipathd: move vecs desctruction into cleanup function mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 05/29] multipathd: make some globals static mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 06/29] multipathd: move threads destruction into separate function mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 07/29] multipathd: move conf " mwilck
2020-10-19 18:56   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 08/29] multipathd: move pid " mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 09/29] multipathd: close pidfile on exit mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 10/29] multipathd: add helper for systemd notification at exit mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 11/29] multipathd: child(): call cleanups in failure case, too mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 13/29] multipathd: print error message if config can't be loaded mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit() mwilck
2020-10-19 19:07   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 15/29] multipathd: fixup libdm deinitialization mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 16/29] libmultipath: log_thread_stop(): check if logarea is initialized mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 17/29] multipathd: add cleanup_child() exit handler mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown mwilck
2020-10-19 20:00   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 20/29] multipath: use atexit() for cleanup handlers mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 21/29] mpathpersist: " mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid() mwilck
2020-10-16 10:44 ` [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions mwilck
2020-10-19 20:01   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity mwilck
2020-10-19 20:38   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink mwilck
2020-10-16 20:13   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog() mwilck
2020-10-19 21:07   ` Benjamin Marzinski
2020-10-16 10:44 ` [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args mwilck
2020-10-19 21:51   ` Benjamin Marzinski
2020-10-16 10:45 ` [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen() mwilck
2020-10-19 23:33   ` Benjamin Marzinski
2020-10-16 10:45 ` [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop() mwilck
2020-10-20  2:20   ` Benjamin Marzinski

DM-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dm-devel/0 dm-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dm-devel dm-devel/ https://lore.kernel.org/dm-devel \
		dm-devel@redhat.com
	public-inbox-index dm-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.dm-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git