[dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit

[dm-devel] [PATCH v2 00/29] libmultipath: improve cleanup on exit

[mwilck]

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

[dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock on exit

[mwilck]

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

Re: [dm-devel] [PATCH v2 01/29] multipathd: uxlsnr: avoid deadlock on exit

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 02/29] multipathd: Fix liburcu memory leak

[mwilck]

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

[dm-devel] [PATCH v2 03/29] multipathd: move handling of io_err_stat_attr into libmultipath

[mwilck]

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

[dm-devel] [PATCH v2 04/29] multipathd: move vecs desctruction into cleanup function

[mwilck]

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

[dm-devel] [PATCH v2 05/29] multipathd: make some globals static

[mwilck]

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

[dm-devel] [PATCH v2 06/29] multipathd: move threads destruction into separate function

[mwilck]

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

[dm-devel] [PATCH v2 07/29] multipathd: move conf destruction into separate function

[mwilck]

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

Re: [dm-devel] [PATCH v2 07/29] multipathd: move conf destruction into separate function

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 08/29] multipathd: move pid destruction into separate function

[mwilck]

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

[dm-devel] [PATCH v2 09/29] multipathd: close pidfile on exit

[mwilck]

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

[dm-devel] [PATCH v2 10/29] multipathd: add helper for systemd notification at exit

[mwilck]

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

[dm-devel] [PATCH v2 11/29] multipathd: child(): call cleanups in failure case, too

[mwilck]

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

[dm-devel] [PATCH v2 12/29] multipathd: unwatch_all_dmevents: check if waiter is initialized

[mwilck]

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

[dm-devel] [PATCH v2 13/29] multipathd: print error message if config can't be loaded

[mwilck]

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

[dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit()

[mwilck]

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

Re: [dm-devel] [PATCH v2 14/29] libmultipath: add libmp_dm_exit()

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 15/29] multipathd: fixup libdm deinitialization

[mwilck]

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

[dm-devel] [PATCH v2 16/29] libmultipath: log_thread_stop(): check if logarea is initialized

[mwilck]

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

[dm-devel] [PATCH v2 17/29] multipathd: add cleanup_child() exit handler

[mwilck]

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

[dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown

[mwilck]

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

Re: [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown

[Benjamin Marzinski]

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

Re: [dm-devel] [PATCH v2 18/29] libmultipath: fix log_thread startup and teardown

[Martin Wilck]

On Mon, 2020-10-19 at 15:00 -0500, Benjamin Marzinski wrote:
>  
> >  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.
> 

Thanks for pointing this out. It was a rebasing mistake. I'll fix it.

Martin


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

[dm-devel] [PATCH v2 19/29] multipathd: move cleanup_{prio, checkers, foreign} to libmultipath_exit

[mwilck]

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

[dm-devel] [PATCH v2 20/29] multipath: use atexit() for cleanup handlers

[mwilck]

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

[dm-devel] [PATCH v2 21/29] mpathpersist: use atexit() for cleanup handlers

[mwilck]

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

[dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid()

[mwilck]

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

Re: [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid()

[Martin Wilck]

On Fri, 2020-10-16 at 12:44 +0200, mwilck@suse.com wrote:
> 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.

Looking at this again after 2 months, I think the on_exit() part of
this patch is wrong. First, we can't use on_exit() on every platform,
as e.g. musl libc doesn't have it. To replace this by the more portable
atexit(), we'd need to declare the two variables "pp" and "pathvec"
with file scope, which is very ugly. But more importantly, using static
variables here causes check_path_valid() to be non-reentrant. While it
doesn't have to be, it's still a coding pattern we haven't been using
anywhere else, just to avoid a "memory leak" for an irregular exit,
which isn't a real memory leak, actually.

One day we should remove the exit() calls somewhere deep down in our
libraries, and deal with the respective errors cleanly.

@lixiaokeng, I hope this is ok for you, as you brought the issue up
originally ("[QUESTION] memory leak in main (multipath)").

Regards
Martin

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


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

Re: [dm-devel] [PATCH v2 22/29] multipath: fix leaks in check_path_valid()

[Benjamin Marzinski]

On Wed, Dec 16, 2020 at 06:34:05PM +0100, Martin Wilck wrote:
> On Fri, 2020-10-16 at 12:44 +0200, mwilck@suse.com wrote:
> > 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.
> 
> Looking at this again after 2 months, I think the on_exit() part of
> this patch is wrong. First, we can't use on_exit() on every platform,
> as e.g. musl libc doesn't have it. To replace this by the more portable
> atexit(), we'd need to declare the two variables "pp" and "pathvec"
> with file scope, which is very ugly. But more importantly, using static
> variables here causes check_path_valid() to be non-reentrant. While it
> doesn't have to be, it's still a coding pattern we haven't been using
> anywhere else, just to avoid a "memory leak" for an irregular exit,
> which isn't a real memory leak, actually.

A while ago, we had a conversation where we talked about not adding too
much complexity to deal with issues on shutdown, that will just go away
when multipathd stops. I still think that we shouldn't worry too much
about making sure we always free everything when it will automatically
get freed by the system anyway.

-Ben

> 
> One day we should remove the exit() calls somewhere deep down in our
> libraries, and deal with the respective errors cleanly.
> 
> @lixiaokeng, I hope this is ok for you, as you brought the issue up
> originally ("[QUESTION] memory leak in main (multipath)").
> 
> Regards
> Martin
> 
> > 
> > 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
> 

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

[dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions

[mwilck]

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

Re: [dm-devel] [PATCH v2 23/29] multipath-tools: mpath-tools.supp: file with valgrind suppressions

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity

[mwilck]

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

Re: [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity

[Benjamin Marzinski]

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

Re: [dm-devel] [PATCH v2 24/29] libmultipath: use libmp_verbosity to track verbosity

[Martin Wilck]

On Mon, 2020-10-19 at 15:38 -0500, Benjamin Marzinski wrote:
> 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?

No, just an oversight. I realized this in the meantime, too.
Will fix it.

Martin


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

[dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink

[mwilck]

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

Re: [dm-devel] [PATCH v2 25/29] libmultipath: introduce symbolic values for logsink

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog()

[mwilck]

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

Re: [dm-devel] [PATCH v2 26/29] libmultipath: simplify dlog()

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args

[mwilck]

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

Re: [dm-devel] [PATCH v2 27/29] multipathd: common code for "-k" and command args

[Benjamin Marzinski]

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

[dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen()

[mwilck]

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

Re: [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen()

[Benjamin Marzinski]

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

Re: [dm-devel] [PATCH v2 28/29] multipathd: sanitize uxsock_listen()

[Martin Wilck]

On Mon, 2020-10-19 at 18:33 -0500, Benjamin Marzinski wrote:
> 
> 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.

I agree about the bookkeeping space, and about the "pretending", too.
This was not about alignment. It's just a habit to use powers of 2 for
array sizes.

The point of increasing the memory area in chunks was simply to call
realloc() less often. I believe that's a good thing.
I plan to do this for our vector implementation, too, some day.

Thanks,
Martin



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

[dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[mwilck]

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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Benjamin Marzinski]

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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Martin Wilck]

On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> 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().

If we do this, we might as well use the variable "la" itself for that,
and make sure it's only accessed under the lock. It'd be fine, because
la is used if and only if the log thread is active, and spare us
another variable. I had actually considered that, thought it was too
invasive for the already big series. If you prefer this way, I can do
it. 

Martin


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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Martin Wilck]

On Mon, 2020-10-26 at 17:22 +0100, Martin Wilck wrote:
> On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> > 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().
> 
> If we do this, we might as well use the variable "la" itself for
> that,
> and make sure it's only accessed under the lock. It'd be fine,
> because
> la is used if and only if the log thread is active, and spare us
> another variable. I had actually considered that, thought it was too
> invasive for the already big series. If you prefer this way, I can do
> it. 

OTOH, we take logev_lock in log_safe() anyway (to set
log_messages_pending). I doubt that it makes a big difference if we
take the two locks sequentially, or nested. The previous code actually
took the logev_lock twice, before and after logq_lock. Assuming that
contention is rather rare, I believe my code might actually perform
better than before. 

In your previous review 
https://www.redhat.com/archives/dm-devel/2020-September/msg00631.html
you pointed out that you considered it important that log_safe() works
even after the thread was stopped. Making this work implies that
log_safe() needs to check if the thread is up. So we either have to
take logev_lock twice, or take logq_lock while holding logev_lock.

Bottom line: I think my patch is correct. We could add another patch on
top that moves logq_lock() into log.c, protecting the "la" variable,
but the nesting would still need to be the same.

Does this make sense?
     
Regards,
Martin



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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Benjamin Marzinski]

On Mon, Oct 26, 2020 at 06:24:57PM +0100, Martin Wilck wrote:
> On Mon, 2020-10-26 at 17:22 +0100, Martin Wilck wrote:
> > On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> > > 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().
> > 
> > If we do this, we might as well use the variable "la" itself for
> > that,
> > and make sure it's only accessed under the lock. It'd be fine,
> > because
> > la is used if and only if the log thread is active, and spare us
> > another variable. I had actually considered that, thought it was too
> > invasive for the already big series. If you prefer this way, I can do
> > it. 
> 
> OTOH, we take logev_lock in log_safe() anyway (to set
> log_messages_pending). I doubt that it makes a big difference if we
> take the two locks sequentially, or nested. The previous code actually
> took the logev_lock twice, before and after logq_lock. Assuming that
> contention is rather rare, I believe my code might actually perform
> better than before. 
> 
> In your previous review 
> https://www.redhat.com/archives/dm-devel/2020-September/msg00631.html
> you pointed out that you considered it important that log_safe() works
> even after the thread was stopped. Making this work implies that
> log_safe() needs to check if the thread is up. So we either have to
> take logev_lock twice, or take logq_lock while holding logev_lock.
> 
> Bottom line: I think my patch is correct. We could add another patch on
> top that moves logq_lock() into log.c, protecting the "la" variable,
> but the nesting would still need to be the same.
> 
> Does this make sense?

No. Maybe I'm just being dumb, but couldn't log safe:

- grab the logq_lock
- check if the log_area is usable. We can use la for this.
	- If not, unlock, write to syslog and return
	- If so, you know that flush_logqueue() hasn't been run by
	  log_thread_stop() yet, meaning anything you add to the log
	  will get flushed by flush_logqueue, so enqueue the message
-unlock logq_lock and lock logev_lock
-signal that there are messages pending. If nobody is listening on the
 the other side, who cares, because log_thread_stop() will still flush
 the enqueued message
-unlock logev_lock

Am I missing something?

-Ben

>      
> Regards,
> Martin
> 
> 

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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Martin Wilck]

On Mon, 2020-11-02 at 18:11 -0600, Benjamin Marzinski wrote:
> On Mon, Oct 26, 2020 at 06:24:57PM +0100, Martin Wilck wrote:
> > On Mon, 2020-10-26 at 17:22 +0100, Martin Wilck wrote:
> > > On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> > > > 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().
> > > 
> > > If we do this, we might as well use the variable "la" itself for
> > > that,
> > > and make sure it's only accessed under the lock. It'd be fine,
> > > because
> > > la is used if and only if the log thread is active, and spare us
> > > another variable. I had actually considered that, thought it was
> > > too
> > > invasive for the already big series. If you prefer this way, I
> > > can do
> > > it. 
> > 
> > OTOH, we take logev_lock in log_safe() anyway (to set
> > log_messages_pending). I doubt that it makes a big difference if we
> > take the two locks sequentially, or nested. The previous code
> > actually
> > took the logev_lock twice, before and after logq_lock. Assuming
> > that
> > contention is rather rare, I believe my code might actually perform
> > better than before. 
> > 
> > In your previous review 
> > https://www.redhat.com/archives/dm-devel/2020-September/msg00631.html
> > you pointed out that you considered it important that log_safe()
> > works
> > even after the thread was stopped. Making this work implies that
> > log_safe() needs to check if the thread is up. So we either have to
> > take logev_lock twice, or take logq_lock while holding logev_lock.
> > 
> > Bottom line: I think my patch is correct. We could add another
> > patch on
> > top that moves logq_lock() into log.c, protecting the "la"
> > variable,
> > but the nesting would still need to be the same.
> > 
> > Does this make sense?
> 
> No. Maybe I'm just being dumb, but couldn't log safe:
> 
> - grab the logq_lock
> - check if the log_area is usable. We can use la for this.
> 	- If not, unlock, write to syslog and return
> 	- If so, you know that flush_logqueue() hasn't been run by
> 	  log_thread_stop() yet,

How do I know that? flush_logqueue() could be running, or could just
have finished. Neither free_logarea() nor log_close() take the
logq_lock (in the current code), so the following would be possible:

  thread A                       thread B               log thread
  --------                       --------               ----------
                                 
     log_thread_stop()           log_safe()                                
                                   under logev_lock:
                                     observe logq_running==1
         under logev_lock:
           pthread_cancel
           signal logev_cond
                                                        <observe logev_cond/cancel>
                                                        under logev_log:
                                                          logq_running=0
                                                        exit()
         pthread_join()
         flush_logqueue()
            <return>
         log_close()               under logq_lock:
                                     test la -> ok           
            free_logarea()
                FREE(la)             log_enqueue()   
            closelog()                 access la
                                       -> *bummer*
 
Even if it doesn't crash because thread B wins the race against FREE(),
the messages written after flush_logqueue() returns will be lost.
AFAICS, the latter would still hold if we did grab logq_lock in
free_logarea(), but at least then we couldn't crash any more.
(I doubt that loosing messages in this corner case really matters).

>  meaning anything you add to the log
> 	  will get flushed by flush_logqueue, so enqueue the message
> -unlock logq_lock and lock logev_lock
> -signal that there are messages pending.
> If nobody is listening on the
>  the other side, who cares, because log_thread_stop() will still
> flush
>  the enqueued message
> -unlock logev_lock
> 
> Am I missing something?

See above. Be invited to prove that I'm wrong :-)

What can we do about it? Of course you're right, if we keep logev_lock
held in log_safe(), we hold this log for a longer time, and effectively
prevent synchronous queueing and dequeuing of messages, so it's not
ideal either.

By taking logq_log in all functions accessing "la" in log.c, we would
avoid crashing. We might still loose some messages. Next, we could
switch to direct logging if log_enqueue() failed because "la" has been
freed (I'm not sure if we should also switch to direct logging if
log_enqueue() fails for other reasons, e.g. because the ring buffer is
full - I suppose we shouldn't).

What about that?

Regards,
Martin

PS: One reason for the awkwardness here is the use of
log_messages_pending under the logev_lock. I believe that
log_messages_pending is redundant; it should be replaced
by something like (la->empty) or (la->tail != la->head), to be tested under logq_lock. But this is subtle, I need to study the code more deeply to get it right. I see it rather as a long-term improvement.


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

Re: [dm-devel] [PATCH v2 29/29] libmultipath: fix race between log_safe and log_thread_stop()

[Benjamin Marzinski]

On Wed, Nov 04, 2020 at 01:36:02PM +0100, Martin Wilck wrote:
> On Mon, 2020-11-02 at 18:11 -0600, Benjamin Marzinski wrote:
> > On Mon, Oct 26, 2020 at 06:24:57PM +0100, Martin Wilck wrote:
> > > On Mon, 2020-10-26 at 17:22 +0100, Martin Wilck wrote:
> > > > On Mon, 2020-10-19 at 21:20 -0500, Benjamin Marzinski wrote:
> > > > > 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().
> > > > 
> > > > If we do this, we might as well use the variable "la" itself for
> > > > that,
> > > > and make sure it's only accessed under the lock. It'd be fine,
> > > > because
> > > > la is used if and only if the log thread is active, and spare us
> > > > another variable. I had actually considered that, thought it was
> > > > too
> > > > invasive for the already big series. If you prefer this way, I
> > > > can do
> > > > it. 
> > > 
> > > OTOH, we take logev_lock in log_safe() anyway (to set
> > > log_messages_pending). I doubt that it makes a big difference if we
> > > take the two locks sequentially, or nested. The previous code
> > > actually
> > > took the logev_lock twice, before and after logq_lock. Assuming
> > > that
> > > contention is rather rare, I believe my code might actually perform
> > > better than before. 
> > > 
> > > In your previous review 
> > > https://www.redhat.com/archives/dm-devel/2020-September/msg00631.html
> > > you pointed out that you considered it important that log_safe()
> > > works
> > > even after the thread was stopped. Making this work implies that
> > > log_safe() needs to check if the thread is up. So we either have to
> > > take logev_lock twice, or take logq_lock while holding logev_lock.
> > > 
> > > Bottom line: I think my patch is correct. We could add another
> > > patch on
> > > top that moves logq_lock() into log.c, protecting the "la"
> > > variable,
> > > but the nesting would still need to be the same.
> > > 
> > > Does this make sense?
> > 
> > No. Maybe I'm just being dumb, but couldn't log safe:
> > 
> > - grab the logq_lock
> > - check if the log_area is usable. We can use la for this.
> > 	- If not, unlock, write to syslog and return
> > 	- If so, you know that flush_logqueue() hasn't been run by
> > 	  log_thread_stop() yet,
> 
> How do I know that?

oops. When I talked above about using a seperate variable,
log_area_enabled, I mentioned that you would need to set this to false
before calling flush_logqueue() in log_thread_stop(). This means that
you can't just use la for this, since you need la around for
flush_logqueue. Sorry. On the other hand, while I not a big fan of
nesting locks that don't need to be, it's not a big deal. So I'm o.k.
with your version.

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

> flush_logqueue() could be running, or could just
> have finished. Neither free_logarea() nor log_close() take the
> logq_lock (in the current code), so the following would be possible:
> 
>   thread A                       thread B               log thread
>   --------                       --------               ----------
>                                  
>      log_thread_stop()           log_safe()                                
>                                    under logev_lock:
>                                      observe logq_running==1
>          under logev_lock:
>            pthread_cancel
>            signal logev_cond
>                                                         <observe logev_cond/cancel>
>                                                         under logev_log:
>                                                           logq_running=0
>                                                         exit()
>          pthread_join()
>          flush_logqueue()
>             <return>
>          log_close()               under logq_lock:
>                                      test la -> ok           
>             free_logarea()
>                 FREE(la)             log_enqueue()   
>             closelog()                 access la
>                                        -> *bummer*
>  
> Even if it doesn't crash because thread B wins the race against FREE(),
> the messages written after flush_logqueue() returns will be lost.
> AFAICS, the latter would still hold if we did grab logq_lock in
> free_logarea(), but at least then we couldn't crash any more.
> (I doubt that loosing messages in this corner case really matters).
> 
> >  meaning anything you add to the log
> > 	  will get flushed by flush_logqueue, so enqueue the message
> > -unlock logq_lock and lock logev_lock
> > -signal that there are messages pending.
> > If nobody is listening on the
> >  the other side, who cares, because log_thread_stop() will still
> > flush
> >  the enqueued message
> > -unlock logev_lock
> > 
> > Am I missing something?
> 
> See above. Be invited to prove that I'm wrong :-)
> 
> What can we do about it? Of course you're right, if we keep logev_lock
> held in log_safe(), we hold this log for a longer time, and effectively
> prevent synchronous queueing and dequeuing of messages, so it's not
> ideal either.
> 
> By taking logq_log in all functions accessing "la" in log.c, we would
> avoid crashing. We might still loose some messages. Next, we could
> switch to direct logging if log_enqueue() failed because "la" has been
> freed (I'm not sure if we should also switch to direct logging if
> log_enqueue() fails for other reasons, e.g. because the ring buffer is
> full - I suppose we shouldn't).
> 
> What about that?
> 
> Regards,
> Martin
> 
> PS: One reason for the awkwardness here is the use of
> log_messages_pending under the logev_lock. I believe that
> log_messages_pending is redundant; it should be replaced
> by something like (la->empty) or (la->tail != la->head), to be tested under logq_lock. But this is subtle, I need to study the code more deeply to get it right. I see it rather as a long-term improvement.
> 

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