All of lore.kernel.org
 help / color / mirror / Atom feed
* [dm-devel] [PATCH 0/7] Various multipath-tools patches
@ 2020-12-17 11:00 mwilck
  2020-12-17 11:00 ` [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c mwilck
                   ` (6 more replies)
  0 siblings, 7 replies; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Hi Christophe, hi Ben,

here are a few patches that have accumulated while I hadn't
fixed my previous series "libmultipath: improve cleanup on exit"

The first two are actually related to the log_thread work I did
in that series, but they're additional fixes on top and I didn't
want to delay that series further by adding additional, possibly
controversial stuff.

No 3 is the 2nd attempt to allow unloading of checker DSOs, hopefully
fixing the issues Ben saw with the first one.

No 4 is probably the most important one. It fixes the same issue
that 2b25a9e and cb10d38 targeted, but the previous fixes still
didn't cover all corner cases. This one does, I believe.

No 5 tackles a new failure mode we recently discovered - multipathd
may block trying to access the root FS while this is in queuing mode.
This patch removes one very frequent access pattern.
This is work in progress; there are other file system accesses
from multipathd which might block.

Obviously the wwids, prkeys, and bindings files' contents 
need to be cached. I haven't made up my mind how to handle writing
changes back. Would be sufficient to write back when multipathd exits,
possibly with an external program (e.g. "ExecStartPost")? Or should
we try to write back directly at runtime, using separate threads to
avoid blocking of the main thread? "reconfigure" is another issue,
but perhaps less problematic - if the root FS is blocked, users
couldn't edit /etc/multipath.conf anyway. One might consider moving
everything to tmpfs or even running in a tmpfs-backed chroot, but
it still would beg the question how to make certain changes permanent,
like wwids and bindings.

6 and 7 fix oversights in my library versioning series. The
plugins didn't have symbol versions set on the symbols they referenced,
making the versioning at least partly non-functional.

Regards
Martin

Martin Wilck (7):
  libmultipath: move logq_lock handling to log.c
  libmultipath: protect logarea with logq_lock
  libmultipath: prevent DSO unloading with astray checker threads
  libmultipath: force map reload if udev incomplete
  multipath-tools: avoid access to /etc/localtime
  multipath-tools: make sure plugin DSOs use symbol versions
  libmultipath.version: add missing symbol

 Makefile                           |  1 +
 Makefile.inc                       |  2 +-
 libmpathpersist/Makefile           |  8 ++--
 libmultipath/checkers.c            | 68 +++++++++++++++++++++++++++---
 libmultipath/checkers.h            | 25 +++++++++++
 libmultipath/checkers/Makefile     |  7 ++-
 libmultipath/checkers/tur.c        | 12 +++---
 libmultipath/configure.c           | 45 +++++++++++++-------
 libmultipath/debug.c               | 14 +++---
 libmultipath/devmapper.c           | 12 +++---
 libmultipath/foreign/Makefile      |  4 +-
 libmultipath/libmultipath.version  | 10 +++++
 libmultipath/log.c                 | 59 ++++++++++++++++++++++----
 libmultipath/log.h                 |  1 -
 libmultipath/log_pthread.c         |  9 ----
 libmultipath/prioritizers/Makefile |  7 ++-
 multipathd/main.c                  |  3 --
 17 files changed, 211 insertions(+), 76 deletions(-)

-- 
2.29.0


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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-17 23:56   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock mwilck
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

logq_lock protects internal data structures of log.c, and should
be handled there. This patch doesn't change functionality, except
improving cancel-safety somewhat.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log.c         | 34 ++++++++++++++++++++++++++++++++--
 libmultipath/log_pthread.c |  9 ---------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/libmultipath/log.c b/libmultipath/log.c
index debd36d..7f33787 100644
--- a/libmultipath/log.c
+++ b/libmultipath/log.c
@@ -9,13 +9,16 @@
 #include <string.h>
 #include <syslog.h>
 #include <time.h>
+#include <pthread.h>
 
 #include "memory.h"
 #include "log.h"
+#include "util.h"
 
 #define ALIGN(len, s) (((len)+(s)-1)/(s)*(s))
 
 struct logarea* la;
+static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
 
 #if LOGDBG
 static void dump_logarea (void)
@@ -101,12 +104,17 @@ void log_close (void)
 
 void log_reset (char *program_name)
 {
+	pthread_mutex_lock(&logq_lock);
+	pthread_cleanup_push(cleanup_mutex, &logq_lock);
+
 	closelog();
 	tzset();
 	openlog(program_name, 0, LOG_DAEMON);
+
+	pthread_cleanup_pop(1);
 }
 
-int log_enqueue (int prio, const char * fmt, va_list ap)
+static int _log_enqueue(int prio, const char * fmt, va_list ap)
 {
 	int len, fwd;
 	char buff[MAX_MSG_SIZE];
@@ -165,7 +173,18 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
 	return 0;
 }
 
-int log_dequeue (void * buff)
+int log_enqueue(int prio, const char *fmt, va_list ap)
+{
+	int ret;
+
+	pthread_mutex_lock(&logq_lock);
+	pthread_cleanup_push(cleanup_mutex, &logq_lock);
+	ret = _log_enqueue(prio, fmt, ap);
+	pthread_cleanup_pop(1);
+	return ret;
+}
+
+static int _log_dequeue(void *buff)
 {
 	struct logmsg * src = (struct logmsg *)la->head;
 	struct logmsg * dst = (struct logmsg *)buff;
@@ -194,6 +213,17 @@ int log_dequeue (void * buff)
 	return 0;
 }
 
+int log_dequeue(void *buff)
+{
+	int ret;
+
+	pthread_mutex_lock(&logq_lock);
+	pthread_cleanup_push(cleanup_mutex, &logq_lock);
+	ret = _log_dequeue(buff);
+	pthread_cleanup_pop(1);
+	return ret;
+}
+
 /*
  * this one can block under memory pressure
  */
diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 0d48c52..6599210 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -18,7 +18,6 @@
 static pthread_t log_thr;
 
 /* 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;
 
@@ -41,10 +40,7 @@ void log_safe (int prio, const char * fmt, va_list ap)
 	running = logq_running;
 
 	if (running) {
-		pthread_mutex_lock(&logq_lock);
-		pthread_cleanup_push(cleanup_mutex, &logq_lock);
 		log_enqueue(prio, fmt, ap);
-		pthread_cleanup_pop(1);
 
 		log_messages_pending = 1;
 		pthread_cond_signal(&logev_cond);
@@ -60,9 +56,7 @@ static void flush_logqueue (void)
 	int empty;
 
 	do {
-		pthread_mutex_lock(&logq_lock);
 		empty = log_dequeue(la->buff);
-		pthread_mutex_unlock(&logq_lock);
 		if (!empty)
 			log_syslog(la->buff);
 	} while (empty == 0);
@@ -138,10 +132,7 @@ void log_thread_start (pthread_attr_t *attr)
 void log_thread_reset (void)
 {
 	logdbg(stderr,"resetting log\n");
-
-	pthread_mutex_lock(&logq_lock);
 	log_reset("multipathd");
-	pthread_mutex_unlock(&logq_lock);
 }
 
 void log_thread_stop (void)
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
  2020-12-17 11:00 ` [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18  0:03   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads mwilck
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

Make sure the global logarea (la) is only allocated and freed
hile holding logq_lock. This avoids invalid memory access.

This patch makes free_logarea() static. libmultipath.version
is unchanged, as free_logarea() wasn't exported anyway.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/log.c | 32 +++++++++++++++++++++++---------
 libmultipath/log.h |  1 -
 2 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/libmultipath/log.c b/libmultipath/log.c
index 7f33787..95c8f01 100644
--- a/libmultipath/log.c
+++ b/libmultipath/log.c
@@ -77,16 +77,23 @@ static int logarea_init (int size)
 
 int log_init(char *program_name, int size)
 {
+	int ret = 1;
+
 	logdbg(stderr,"enter log_init\n");
+
+	pthread_mutex_lock(&logq_lock);
+	pthread_cleanup_push(cleanup_mutex, &logq_lock);
+
 	openlog(program_name, 0, LOG_DAEMON);
+	if (!la)
+		ret = logarea_init(size);
 
-	if (logarea_init(size))
-		return 1;
+	pthread_cleanup_pop(1);
 
-	return 0;
+	return ret;
 }
 
-void free_logarea (void)
+static void free_logarea (void)
 {
 	FREE(la->start);
 	FREE(la->buff);
@@ -96,9 +103,14 @@ void free_logarea (void)
 
 void log_close (void)
 {
-	free_logarea();
+	pthread_mutex_lock(&logq_lock);
+	pthread_cleanup_push(cleanup_mutex, &logq_lock);
+
+	if (la)
+		free_logarea();
 	closelog();
 
+	pthread_cleanup_pop(1);
 	return;
 }
 
@@ -175,11 +187,12 @@ static int _log_enqueue(int prio, const char * fmt, va_list ap)
 
 int log_enqueue(int prio, const char *fmt, va_list ap)
 {
-	int ret;
+	int ret = 1;
 
 	pthread_mutex_lock(&logq_lock);
 	pthread_cleanup_push(cleanup_mutex, &logq_lock);
-	ret = _log_enqueue(prio, fmt, ap);
+	if (la)
+		ret = _log_enqueue(prio, fmt, ap);
 	pthread_cleanup_pop(1);
 	return ret;
 }
@@ -215,11 +228,12 @@ static int _log_dequeue(void *buff)
 
 int log_dequeue(void *buff)
 {
-	int ret;
+	int ret = 1;
 
 	pthread_mutex_lock(&logq_lock);
 	pthread_cleanup_push(cleanup_mutex, &logq_lock);
-	ret = _log_dequeue(buff);
+	if (la)
+		ret = _log_dequeue(buff);
 	pthread_cleanup_pop(1);
 	return ret;
 }
diff --git a/libmultipath/log.h b/libmultipath/log.h
index d2448f6..fa224e4 100644
--- a/libmultipath/log.h
+++ b/libmultipath/log.h
@@ -39,6 +39,5 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
 int log_dequeue (void *);
 void log_syslog (void *);
 void dump_logmsg (void *);
-void free_logarea (void);
 
 #endif /* LOG_H */
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
  2020-12-17 11:00 ` [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c mwilck
  2020-12-17 11:00 ` [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18  4:22   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete mwilck
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The multipathd tur checker thread is designed to be able to finish at
any time, even after the tur checker itself has been freed. The
multipathd shutdown code makes sure all the checkers have been freed
before freeing the checker_class and calling dlclose() to unload the
DSO, but this doesn't guarantee that the checker threads have finished.
If one hasn't, the DSO will get unloaded while the thread still running
code from it, causing a segfault.

This patch fixes the issue by further incrementing the DSO's refcount
for every running thread. To avoid race conditions leading to segfaults,
the thread's entrypoint must be in libmultipath, not in the DSO itself.
Therefore we add a new optional checker method, libcheck_thread().
Checkers defining this method may create a detached thread with
entrypoint checker_thread_entry(), which will call the DSO's
libcheck_thread and take care of the refcount handling.

Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/checkers.c           | 68 +++++++++++++++++++++++++++----
 libmultipath/checkers.h           | 25 ++++++++++++
 libmultipath/checkers/tur.c       | 12 +++---
 libmultipath/libmultipath.version |  5 +++
 4 files changed, 97 insertions(+), 13 deletions(-)

diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 18b1f5e..2dd9915 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -3,6 +3,8 @@
 #include <stddef.h>
 #include <dlfcn.h>
 #include <sys/stat.h>
+#include <urcu.h>
+#include <urcu/uatomic.h>
 
 #include "debug.h"
 #include "checkers.h"
@@ -20,6 +22,7 @@ struct checker_class {
 	int (*mp_init)(struct checker *);    /* to allocate the mpcontext */
 	void (*free)(struct checker *);      /* to free the context */
 	void (*reset)(void);		     /* to reset the global variables */
+	void *(*thread)(void *);	     /* async thread entry point */
 	const char **msgtable;
 	short msgtable_size;
 };
@@ -55,19 +58,32 @@ static struct checker_class *alloc_checker_class(void)
 	c = MALLOC(sizeof(struct checker_class));
 	if (c) {
 		INIT_LIST_HEAD(&c->node);
-		c->refcount = 1;
+		uatomic_set(&c->refcount, 1);
 	}
 	return c;
 }
 
+/* Use uatomic_{sub,add}_return() to ensure proper memory barriers */
+static int checker_class_ref(struct checker_class *cls)
+{
+	return uatomic_add_return(&cls->refcount, 1);
+}
+
+static int checker_class_unref(struct checker_class *cls)
+{
+	return uatomic_sub_return(&cls->refcount, 1);
+}
+
 void free_checker_class(struct checker_class *c)
 {
+	int cnt;
+
 	if (!c)
 		return;
-	c->refcount--;
-	if (c->refcount) {
-		condlog(4, "%s checker refcount %d",
-			c->name, c->refcount);
+	cnt = checker_class_unref(c);
+	if (cnt != 0) {
+		condlog(cnt < 0 ? 1 : 4, "%s checker refcount %d",
+			c->name, cnt);
 		return;
 	}
 	condlog(3, "unloading %s checker", c->name);
@@ -161,7 +177,8 @@ static struct checker_class *add_checker_class(const char *multipath_dir,
 
 	c->mp_init = (int (*)(struct checker *)) dlsym(c->handle, "libcheck_mp_init");
 	c->reset = (void (*)(void)) dlsym(c->handle, "libcheck_reset");
-	/* These 2 functions can be NULL. call dlerror() to clear out any
+	c->thread = (void *(*)(void*)) dlsym(c->handle, "libcheck_thread");
+	/* These 3 functions can be NULL. call dlerror() to clear out any
 	 * error string */
 	dlerror();
 
@@ -347,6 +364,43 @@ bad_id:
 	return generic_msg[CHECKER_MSGID_NONE];
 }
 
+static void checker_cleanup_thread(void *arg)
+{
+	struct checker_class *cls = arg;
+
+	(void)checker_class_unref(cls);
+	rcu_unregister_thread();
+}
+
+static void *checker_thread_entry(void *arg)
+{
+	struct checker_context *ctx = arg;
+	void *rv;
+
+	rcu_register_thread();
+	pthread_cleanup_push(checker_cleanup_thread, ctx->cls);
+	rv = ctx->cls->thread(ctx);
+	pthread_cleanup_pop(1);
+	return rv;
+}
+
+int start_checker_thread(pthread_t *thread, const pthread_attr_t *attr,
+			 struct checker_context *ctx)
+{
+	int rv;
+
+	assert(ctx && ctx->cls && ctx->cls->thread);
+	/* Take a ref here, lest the class be freed before the thread starts */
+	(void)checker_class_ref(ctx->cls);
+	rv = pthread_create(thread, attr, checker_thread_entry, ctx);
+	if (rv != 0) {
+		condlog(1, "failed to start checker thread for %s: %m",
+			ctx->cls->name);
+		checker_class_unref(ctx->cls);
+	}
+	return rv;
+}
+
 void checker_clear_message (struct checker *c)
 {
 	if (!c)
@@ -371,7 +425,7 @@ void checker_get(const char *multipath_dir, struct checker *dst,
 	if (!src)
 		return;
 
-	src->refcount++;
+	(void)checker_class_ref(dst->cls);
 }
 
 int init_checkers(const char *multipath_dir)
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index 9d5f90b..2fd1d1c 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -1,6 +1,7 @@
 #ifndef _CHECKERS_H
 #define _CHECKERS_H
 
+#include <pthread.h>
 #include "list.h"
 #include "memory.h"
 #include "defaults.h"
@@ -148,6 +149,28 @@ void checker_set_async (struct checker *);
 void checker_set_fd (struct checker *, int);
 void checker_enable (struct checker *);
 void checker_disable (struct checker *);
+/*
+ * start_checker_thread(): start async path checker thread
+ *
+ * This function provides a wrapper around pthread_create().
+ * The created thread will call the DSO's "libcheck_thread" function with the
+ * checker context as argument.
+ *
+ * Rationale:
+ * Path checkers that do I/O may hang forever. To avoid blocking, some
+ * checkers therefore use asyncronous, detached threads for checking
+ * the paths. These threads may continue hanging if multipathd is stopped.
+ * In this case, we can't unload the checker DSO at exit. In order to
+ * avoid race conditions and crashes, the entry point of the thread
+ * needs to be in libmultipath, not in the DSO itself.
+ *
+ * @param arg: pointer to struct checker_context.
+ */
+struct checker_context {
+	struct checker_class *cls;
+};
+int start_checker_thread (pthread_t *thread, const pthread_attr_t *attr,
+			  struct checker_context *ctx);
 int checker_check (struct checker *, int);
 int checker_is_sync(const struct checker *);
 const char *checker_name (const struct checker *);
@@ -164,6 +187,8 @@ void checker_get(const char *, struct checker *, const char *);
 int libcheck_check(struct checker *);
 int libcheck_init(struct checker *);
 void libcheck_free(struct checker *);
+void *libcheck_thread(struct checker_context *ctx);
+
 /*
  * msgid => message map.
  *
diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
index e886fcf..a4b4a21 100644
--- a/libmultipath/checkers/tur.c
+++ b/libmultipath/checkers/tur.c
@@ -15,7 +15,6 @@
 #include <errno.h>
 #include <sys/time.h>
 #include <pthread.h>
-#include <urcu.h>
 #include <urcu/uatomic.h>
 
 #include "checkers.h"
@@ -55,6 +54,7 @@ struct tur_checker_context {
 	pthread_cond_t active;
 	int holders; /* uatomic access only */
 	int msgid;
+	struct checker_context ctx;
 };
 
 int libcheck_init (struct checker * c)
@@ -74,6 +74,7 @@ int libcheck_init (struct checker * c)
 	pthread_mutex_init(&ct->lock, NULL);
 	if (fstat(c->fd, &sb) == 0)
 		ct->devt = sb.st_rdev;
+	ct->ctx.cls = c->cls;
 	c->context = ct;
 
 	return 0;
@@ -204,7 +205,6 @@ static void cleanup_func(void *data)
 	holders = uatomic_sub_return(&ct->holders, 1);
 	if (!holders)
 		cleanup_context(ct);
-	rcu_unregister_thread();
 }
 
 /*
@@ -251,15 +251,15 @@ static void tur_deep_sleep(const struct tur_checker_context *ct)
 #define tur_deep_sleep(x) do {} while (0)
 #endif /* TUR_TEST_MAJOR */
 
-static void *tur_thread(void *ctx)
+void *libcheck_thread(struct checker_context *ctx)
 {
-	struct tur_checker_context *ct = ctx;
+	struct tur_checker_context *ct =
+		container_of(ctx, struct tur_checker_context, ctx);
 	int state, running;
 	short msgid;
 
 	/* This thread can be canceled, so setup clean up */
 	tur_thread_cleanup_push(ct);
-	rcu_register_thread();
 
 	condlog(4, "%d:%d : tur checker starting up", major(ct->devt),
 		minor(ct->devt));
@@ -394,7 +394,7 @@ int libcheck_check(struct checker * c)
 		uatomic_set(&ct->running, 1);
 		tur_set_async_timeout(c);
 		setup_thread_attr(&attr, 32 * 1024, 1);
-		r = pthread_create(&ct->thread, &attr, tur_thread, ct);
+		r = start_checker_thread(&ct->thread, &attr, &ct->ctx);
 		pthread_attr_destroy(&attr);
 		if (r) {
 			uatomic_sub(&ct->holders, 1);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 2e3583f..751099d 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -270,3 +270,8 @@ global:
 	dm_prereq;
 	skip_libmp_dm_init;
 } LIBMULTIPATH_4.1.0;
+
+LIBMULTIPATH_4.3.0 {
+global:
+	start_checker_thread;
+} LIBMULTIPATH_4.2.0;
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
                   ` (2 preceding siblings ...)
  2020-12-17 11:00 ` [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18  5:48   ` Benjamin Marzinski
  2020-12-18 17:57   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime mwilck
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

We've recently observed various cases of incompletely processed uevents
during initrd processing. Typically, this would leave a dm device in
the state it had after the initial "add" uevent, which is basically unusable,
because udevd had been killed by systemd before processing the subsequent
"change" event. After switching root, the coldplug event would re-read
the db file, which would be in unusable state, and would not do anything.
In such cases, a RELOAD action with force_udev_reload=1 is in order to
make udev re-process the device completely (DM_UDEV_PRIMARY_SOURCE_FLAG=1 and
DM_SUBSYSTEM_UDEV_FLAG0=0).

The previous commits

2b25a9e libmultipath: select_action(): force udev reload for uninitialized maps
cb10d38 multipathd: uev_trigger(): handle incomplete ADD events

addressed the same issue, but incompletely. They would miss cases where the
map was configured correctly but none of the RELOAD criteria were met.
This patch partially reverts 2b25a9e by converting select_reload_action() into
a trivial helper. Instead, we now check for incompletely initialized udev now
before checking any of the other reload criteria.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/configure.c | 45 ++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 3dbc1f1..d64fe88 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -695,12 +695,11 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
 	return err;
 }
 
-static void
-select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
-		     const char *reason)
+static bool is_udev_ready(struct multipath *cmpp)
 {
 	struct udev_device *mpp_ud;
 	const char *env;
+	bool rc;
 
 	/*
 	 * MPATH_DEVICE_READY != 1 can mean two things:
@@ -712,14 +711,20 @@ select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
 	 */
 
 	mpp_ud = get_udev_for_mpp(cmpp);
+	if (!mpp_ud)
+		return true;
 	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
-	if ((!env || strcmp(env, "1")) && count_active_paths(mpp) > 0)
-		mpp->force_udev_reload = 1;
+	rc = (env != NULL && !strcmp(env, "1"));
 	udev_device_unref(mpp_ud);
+	condlog(4, "%s: %s: \"%s\" -> %d\n", __func__, cmpp->alias, env, rc);
+	return rc;
+}
+
+static void
+select_reload_action(struct multipath *mpp, const char *reason)
+{
 	mpp->action = ACT_RELOAD;
-	condlog(3, "%s: set ACT_RELOAD (%s%s)", mpp->alias,
-		mpp->force_udev_reload ? "forced, " : "",
-		reason);
+	condlog(3, "%s: set ACT_RELOAD (%s)", mpp->alias, reason);
 }
 
 void select_action (struct multipath *mpp, const struct _vector *curmp,
@@ -788,10 +793,18 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		return;
 	}
 
+	if (!is_udev_ready(cmpp) && count_active_paths(mpp) > 0) {
+		mpp->force_udev_reload = 1;
+		mpp->action = ACT_RELOAD;
+		condlog(3, "%s: set ACT_RELOAD (udev incomplete)",
+			mpp->alias);
+		return;
+	}
+
 	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
 	    !!strstr(mpp->features, "queue_if_no_path") !=
 	    !!strstr(cmpp->features, "queue_if_no_path")) {
-		select_reload_action(mpp, cmpp, "no_path_retry change");
+		select_reload_action(mpp, "no_path_retry change");
 		return;
 	}
 	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
@@ -799,7 +812,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
 	     strncmp(cmpp->hwhandler, mpp->hwhandler,
 		    strlen(mpp->hwhandler)))) {
-		select_reload_action(mpp, cmpp, "hwhandler change");
+		select_reload_action(mpp, "hwhandler change");
 		return;
 	}
 
@@ -807,7 +820,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
 	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
-		select_reload_action(mpp, cmpp, "retain_hwhandler change");
+		select_reload_action(mpp, "retain_hwhandler change");
 		return;
 	}
 
@@ -819,7 +832,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 		remove_feature(&cmpp_feat, "queue_if_no_path");
 		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
 		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
-			select_reload_action(mpp, cmpp, "features change");
+			select_reload_action(mpp, "features change");
 			FREE(cmpp_feat);
 			FREE(mpp_feat);
 			return;
@@ -830,19 +843,19 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
 
 	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
 		    strlen(mpp->selector))) {
-		select_reload_action(mpp, cmpp, "selector change");
+		select_reload_action(mpp, "selector change");
 		return;
 	}
 	if (cmpp->minio != mpp->minio) {
-		select_reload_action(mpp, cmpp, "minio change");
+		select_reload_action(mpp, "minio change");
 		return;
 	}
 	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
-		select_reload_action(mpp, cmpp, "path group number change");
+		select_reload_action(mpp, "path group number change");
 		return;
 	}
 	if (pgcmp(mpp, cmpp)) {
-		select_reload_action(mpp, cmpp, "path group topology change");
+		select_reload_action(mpp, "path group topology change");
 		return;
 	}
 	if (cmpp->nextpg != mpp->bestpg) {
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
                   ` (3 preceding siblings ...)
  2020-12-17 11:00 ` [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18 18:14   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions mwilck
  2020-12-17 11:00 ` [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol mwilck
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

If the root file system is multipathed, and IO is queued because all paths
are failed, multipathd may block trying to access the root FS, and thus be
unable to reinstate paths. One file that is frequently accessed is
/etc/localtime. Avoid that by printing monotonic timestamps instead.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/debug.c     | 14 ++++++++------
 libmultipath/devmapper.c | 12 ++++++------
 libmultipath/log.c       |  1 -
 multipathd/main.c        |  3 ---
 4 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/libmultipath/debug.c b/libmultipath/debug.c
index 429f269..510e15e 100644
--- a/libmultipath/debug.c
+++ b/libmultipath/debug.c
@@ -14,6 +14,8 @@
 #include "config.h"
 #include "defaults.h"
 #include "debug.h"
+#include "time-util.h"
+#include "util.h"
 
 int logsink;
 int libmp_verbosity = DEFAULT_VERBOSITY;
@@ -25,13 +27,13 @@ void dlog(int prio, const char * fmt, ...)
 	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];
+			struct timespec ts;
+			char buff[32];
 
-			strftime(buff, sizeof(buff),
-				 "%b %d %H:%M:%S", tb);
-			buff[sizeof(buff)-1] = '\0';
+			get_monotonic_time(&ts);
+			safe_sprintf(buff, "%ld.%06ld",
+				     (long)ts.tv_sec,
+				     ts.tv_nsec/1000);
 			fprintf(stderr, "%s | ", buff);
 		}
 		vfprintf(stderr, fmt, ap);
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 4977b31..095cbc0 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -27,6 +27,7 @@
 #include "config.h"
 #include "wwids.h"
 #include "version.h"
+#include "time-util.h"
 
 #include "log_pthread.h"
 #include <sys/types.h>
@@ -106,13 +107,12 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 	va_start(ap, f);
 	if (logsink != LOGSINK_SYSLOG) {
 		if (logsink == 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';
+			struct timespec ts;
+			char buff[32];
 
+			get_monotonic_time(&ts);
+			safe_sprintf(buff, "%ld.%06ld",
+				     (long)ts.tv_sec, ts.tv_nsec/1000);
 			fprintf(stderr, "%s | ", buff);
 		}
 		fprintf(stderr, "libdevmapper: %s(%i): ", file, line);
diff --git a/libmultipath/log.c b/libmultipath/log.c
index 95c8f01..6498c88 100644
--- a/libmultipath/log.c
+++ b/libmultipath/log.c
@@ -120,7 +120,6 @@ void log_reset (char *program_name)
 	pthread_cleanup_push(cleanup_mutex, &logq_lock);
 
 	closelog();
-	tzset();
 	openlog(program_name, 0, LOG_DAEMON);
 
 	pthread_cleanup_pop(1);
diff --git a/multipathd/main.c b/multipathd/main.c
index b6a5f5b..28c147b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2710,9 +2710,6 @@ reconfigure (struct vectors * vecs)
 	delete_all_foreign();
 
 	reset_checker_classes();
-	/* Re-read any timezone changes */
-	tzset();
-
 	if (bindings_read_only)
 		conf->bindings_read_only = bindings_read_only;
 	check_alias_settings(conf);
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
                   ` (4 preceding siblings ...)
  2020-12-17 11:00 ` [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18 18:36   ` Benjamin Marzinski
  2020-12-17 11:00 ` [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol mwilck
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

By adding -Wl,-z,defs, we'll get warnings about unresolved symbols
at the linking stage. This way we make sure our plugins (checkers etc.)
will use versioned symbols from libmultipath, and incompatible plugins
can't be loaded any more. Doing this requires explicitly linking
the plugins with all libraries they use, in particular libmultipath.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 Makefile                           | 1 +
 Makefile.inc                       | 2 +-
 libmpathpersist/Makefile           | 8 ++++----
 libmultipath/checkers/Makefile     | 7 +++----
 libmultipath/foreign/Makefile      | 4 +++-
 libmultipath/prioritizers/Makefile | 7 +++----
 6 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index f127ff9..bddb2bf 100644
--- a/Makefile
+++ b/Makefile
@@ -31,6 +31,7 @@ $(BUILDDIRS):
 
 libmultipath libdmmp: libmpathcmd
 libmpathpersist libmpathvalid multipath multipathd: libmultipath
+libmultipath/prioritizers libmultipath/checkers libmultipath/foreign: libmultipath
 mpathpersist multipathd:  libmpathpersist
 
 libmultipath/checkers.install \
diff --git a/Makefile.inc b/Makefile.inc
index 13587a9..0542930 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -105,7 +105,7 @@ CFLAGS		:= --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
 BIN_CFLAGS	= -fPIE -DPIE
 LIB_CFLAGS	= -fPIC
 SHARED_FLAGS	= -shared
-LDFLAGS		:= $(LDFLAGS) -Wl,-z,relro -Wl,-z,now
+LDFLAGS		:= $(LDFLAGS) -Wl,-z,relro -Wl,-z,now -Wl,-z,defs
 BIN_LDFLAGS	= -pie
 
 # Check whether a function with name $1 has been declared in header file $2.
diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
index 456ce4c..57103e5 100644
--- a/libmpathpersist/Makefile
+++ b/libmpathpersist/Makefile
@@ -6,17 +6,17 @@ LIBS = $(DEVLIB).$(SONAME)
 VERSION_SCRIPT := libmpathpersist.version
 
 CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathpersistdir) -I$(mpathcmddir)
+LDFLAGS += -L$(multipathdir) -L$(mpathcmddir)
 
-LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) -lmultipath \
-	   -L$(mpathcmddir) -lmpathcmd
+LIBDEPS += -lmultipath -lmpathcmd -ldevmapper -lpthread -ldl
 
 OBJS = mpath_persist.o mpath_updatepr.o mpath_pr_ioctl.o
 
 all: $(DEVLIB) man
 
 $(LIBS): $(OBJS) $(VERSION_SCRIPT)
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ \
-		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS)
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ \
+		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS) $(LIBDEPS)
 
 $(DEVLIB): $(LIBS)
 	$(LN) $(LIBS) $@
diff --git a/libmultipath/checkers/Makefile b/libmultipath/checkers/Makefile
index 01c0451..8e0ed5e 100644
--- a/libmultipath/checkers/Makefile
+++ b/libmultipath/checkers/Makefile
@@ -4,6 +4,8 @@
 include ../../Makefile.inc
 
 CFLAGS += $(LIB_CFLAGS) -I..
+LDFLAGS += -L.. -lmultipath
+LIBDEPS = -lmultipath -laio -lpthread -lrt
 
 # If you add or remove a checker also update multipath/multipath.conf.5
 LIBS= \
@@ -17,11 +19,8 @@ LIBS= \
 
 all: $(LIBS)
 
-libcheckdirectio.so: directio.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -laio
-
 libcheck%.so: %.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
 
 install:
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
diff --git a/libmultipath/foreign/Makefile b/libmultipath/foreign/Makefile
index fae58a0..f447a1c 100644
--- a/libmultipath/foreign/Makefile
+++ b/libmultipath/foreign/Makefile
@@ -5,13 +5,15 @@ TOPDIR=../..
 include ../../Makefile.inc
 
 CFLAGS += $(LIB_CFLAGS) -I.. -I$(nvmedir)
+LDFLAGS += -L..
+LIBDEPS = -lmultipath -ludev -lpthread -lrt
 
 LIBS = libforeign-nvme.so
 
 all: $(LIBS)
 
 libforeign-%.so: %.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
 
 install:
 	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
diff --git a/libmultipath/prioritizers/Makefile b/libmultipath/prioritizers/Makefile
index fc6e0e0..8d34ae3 100644
--- a/libmultipath/prioritizers/Makefile
+++ b/libmultipath/prioritizers/Makefile
@@ -4,6 +4,8 @@
 include ../../Makefile.inc
 
 CFLAGS += $(LIB_CFLAGS) -I..
+LDFLAGS += -L..
+LIBDEPS = -lmultipath -lm -lpthread -lrt
 
 # If you add or remove a prioritizer also update multipath/multipath.conf.5
 LIBS = \
@@ -28,11 +30,8 @@ endif
 
 all: $(LIBS)
 
-libpriopath_latency.so: path_latency.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -lm
-
 libprio%.so: %.o
-	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
+	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
 
 install: $(LIBS)
 	$(INSTALL_PROGRAM) -m 755 libprio*.so $(DESTDIR)$(libdir)
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol
  2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
                   ` (5 preceding siblings ...)
  2020-12-17 11:00 ` [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions mwilck
@ 2020-12-17 11:00 ` mwilck
  2020-12-18 18:36   ` Benjamin Marzinski
  6 siblings, 1 reply; 20+ messages in thread
From: mwilck @ 2020-12-17 11:00 UTC (permalink / raw)
  To: Christophe Varoqui, Benjamin Marzinski; +Cc: dm-devel, Martin Wilck

From: Martin Wilck <mwilck@suse.com>

The weightedpath prioritizer uses get_next_string(). I'd overlooked
this before. This was found with the help of the previous patch.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 751099d..2228f4e 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -275,3 +275,8 @@ LIBMULTIPATH_4.3.0 {
 global:
 	start_checker_thread;
 } LIBMULTIPATH_4.2.0;
+
+LIBMULTIPATH_4.4.0 {
+global:
+	get_next_string;
+} LIBMULTIPATH_4.3.0;
-- 
2.29.0


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


^ permalink raw reply related	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c
  2020-12-17 11:00 ` [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c mwilck
@ 2020-12-17 23:56   ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-17 23:56 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:12PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> logq_lock protects internal data structures of log.c, and should
> be handled there. This patch doesn't change functionality, except
> improving cancel-safety somewhat.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log.c         | 34 ++++++++++++++++++++++++++++++++--
>  libmultipath/log_pthread.c |  9 ---------
>  2 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index debd36d..7f33787 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -9,13 +9,16 @@
>  #include <string.h>
>  #include <syslog.h>
>  #include <time.h>
> +#include <pthread.h>
>  
>  #include "memory.h"
>  #include "log.h"
> +#include "util.h"
>  
>  #define ALIGN(len, s) (((len)+(s)-1)/(s)*(s))
>  
>  struct logarea* la;
> +static pthread_mutex_t logq_lock = PTHREAD_MUTEX_INITIALIZER;
>  
>  #if LOGDBG
>  static void dump_logarea (void)
> @@ -101,12 +104,17 @@ void log_close (void)
>  
>  void log_reset (char *program_name)
>  {
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
>  	closelog();
>  	tzset();
>  	openlog(program_name, 0, LOG_DAEMON);
> +
> +	pthread_cleanup_pop(1);
>  }
>  
> -int log_enqueue (int prio, const char * fmt, va_list ap)
> +static int _log_enqueue(int prio, const char * fmt, va_list ap)
>  {
>  	int len, fwd;
>  	char buff[MAX_MSG_SIZE];
> @@ -165,7 +173,18 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
>  	return 0;
>  }
>  
> -int log_dequeue (void * buff)
> +int log_enqueue(int prio, const char *fmt, va_list ap)
> +{
> +	int ret;
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +	ret = _log_enqueue(prio, fmt, ap);
> +	pthread_cleanup_pop(1);
> +	return ret;
> +}
> +
> +static int _log_dequeue(void *buff)
>  {
>  	struct logmsg * src = (struct logmsg *)la->head;
>  	struct logmsg * dst = (struct logmsg *)buff;
> @@ -194,6 +213,17 @@ int log_dequeue (void * buff)
>  	return 0;
>  }
>  
> +int log_dequeue(void *buff)
> +{
> +	int ret;
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +	ret = _log_dequeue(buff);
> +	pthread_cleanup_pop(1);
> +	return ret;
> +}
> +
>  /*
>   * this one can block under memory pressure
>   */
> diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
> index 0d48c52..6599210 100644
> --- a/libmultipath/log_pthread.c
> +++ b/libmultipath/log_pthread.c
> @@ -18,7 +18,6 @@
>  static pthread_t log_thr;
>  
>  /* 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;
>  
> @@ -41,10 +40,7 @@ void log_safe (int prio, const char * fmt, va_list ap)
>  	running = logq_running;
>  
>  	if (running) {
> -		pthread_mutex_lock(&logq_lock);
> -		pthread_cleanup_push(cleanup_mutex, &logq_lock);
>  		log_enqueue(prio, fmt, ap);
> -		pthread_cleanup_pop(1);
>  
>  		log_messages_pending = 1;
>  		pthread_cond_signal(&logev_cond);
> @@ -60,9 +56,7 @@ static void flush_logqueue (void)
>  	int empty;
>  
>  	do {
> -		pthread_mutex_lock(&logq_lock);
>  		empty = log_dequeue(la->buff);
> -		pthread_mutex_unlock(&logq_lock);
>  		if (!empty)
>  			log_syslog(la->buff);
>  	} while (empty == 0);
> @@ -138,10 +132,7 @@ void log_thread_start (pthread_attr_t *attr)
>  void log_thread_reset (void)
>  {
>  	logdbg(stderr,"resetting log\n");
> -
> -	pthread_mutex_lock(&logq_lock);
>  	log_reset("multipathd");
> -	pthread_mutex_unlock(&logq_lock);
>  }
>  
>  void log_thread_stop (void)
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock
  2020-12-17 11:00 ` [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock mwilck
@ 2020-12-18  0:03   ` Benjamin Marzinski
  2020-12-18 16:24     ` Martin Wilck
  0 siblings, 1 reply; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18  0:03 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:13PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Make sure the global logarea (la) is only allocated and freed
> hile holding logq_lock. This avoids invalid memory access.
> 
> This patch makes free_logarea() static. libmultipath.version
> is unchanged, as free_logarea() wasn't exported anyway.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/log.c | 32 +++++++++++++++++++++++---------
>  libmultipath/log.h |  1 -
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index 7f33787..95c8f01 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -77,16 +77,23 @@ static int logarea_init (int size)
>  
>  int log_init(char *program_name, int size)
>  {
> +	int ret = 1;
> +
>  	logdbg(stderr,"enter log_init\n");
> +
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
>  	openlog(program_name, 0, LOG_DAEMON);
> +	if (!la)
> +		ret = logarea_init(size);
>  
> -	if (logarea_init(size))
> -		return 1;
> +	pthread_cleanup_pop(1);
>  
> -	return 0;
> +	return ret;
>  }
>  
> -void free_logarea (void)
> +static void free_logarea (void)
>  {
>  	FREE(la->start);
>  	FREE(la->buff);

I realize that the log area can only be freed by log_close(), which is
called when mulitpathd exits, but it would be nice to have la set to
NULL it's freed, just to make it obvious that that there can't be
double-frees there. However, the code is clearly correct, so

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

> @@ -96,9 +103,14 @@ void free_logarea (void)
>  
>  void log_close (void)
>  {
> -	free_logarea();
> +	pthread_mutex_lock(&logq_lock);
> +	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> +
> +	if (la)
> +		free_logarea();
>  	closelog();
>  
> +	pthread_cleanup_pop(1);
>  	return;
>  }
>  
> @@ -175,11 +187,12 @@ static int _log_enqueue(int prio, const char * fmt, va_list ap)
>  
>  int log_enqueue(int prio, const char *fmt, va_list ap)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	pthread_mutex_lock(&logq_lock);
>  	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> -	ret = _log_enqueue(prio, fmt, ap);
> +	if (la)
> +		ret = _log_enqueue(prio, fmt, ap);
>  	pthread_cleanup_pop(1);
>  	return ret;
>  }
> @@ -215,11 +228,12 @@ static int _log_dequeue(void *buff)
>  
>  int log_dequeue(void *buff)
>  {
> -	int ret;
> +	int ret = 1;
>  
>  	pthread_mutex_lock(&logq_lock);
>  	pthread_cleanup_push(cleanup_mutex, &logq_lock);
> -	ret = _log_dequeue(buff);
> +	if (la)
> +		ret = _log_dequeue(buff);
>  	pthread_cleanup_pop(1);
>  	return ret;
>  }
> diff --git a/libmultipath/log.h b/libmultipath/log.h
> index d2448f6..fa224e4 100644
> --- a/libmultipath/log.h
> +++ b/libmultipath/log.h
> @@ -39,6 +39,5 @@ int log_enqueue (int prio, const char * fmt, va_list ap)
>  int log_dequeue (void *);
>  void log_syslog (void *);
>  void dump_logmsg (void *);
> -void free_logarea (void);
>  
>  #endif /* LOG_H */
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads
  2020-12-17 11:00 ` [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads mwilck
@ 2020-12-18  4:22   ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18  4:22 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:14PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The multipathd tur checker thread is designed to be able to finish at
> any time, even after the tur checker itself has been freed. The
> multipathd shutdown code makes sure all the checkers have been freed
> before freeing the checker_class and calling dlclose() to unload the
> DSO, but this doesn't guarantee that the checker threads have finished.
> If one hasn't, the DSO will get unloaded while the thread still running
> code from it, causing a segfault.
> 
> This patch fixes the issue by further incrementing the DSO's refcount
> for every running thread. To avoid race conditions leading to segfaults,
> the thread's entrypoint must be in libmultipath, not in the DSO itself.
> Therefore we add a new optional checker method, libcheck_thread().
> Checkers defining this method may create a detached thread with
> entrypoint checker_thread_entry(), which will call the DSO's
> libcheck_thread and take care of the refcount handling.
> 
> Reported-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/checkers.c           | 68 +++++++++++++++++++++++++++----
>  libmultipath/checkers.h           | 25 ++++++++++++
>  libmultipath/checkers/tur.c       | 12 +++---
>  libmultipath/libmultipath.version |  5 +++
>  4 files changed, 97 insertions(+), 13 deletions(-)
> 
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 18b1f5e..2dd9915 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -3,6 +3,8 @@
>  #include <stddef.h>
>  #include <dlfcn.h>
>  #include <sys/stat.h>
> +#include <urcu.h>
> +#include <urcu/uatomic.h>
>  
>  #include "debug.h"
>  #include "checkers.h"
> @@ -20,6 +22,7 @@ struct checker_class {
>  	int (*mp_init)(struct checker *);    /* to allocate the mpcontext */
>  	void (*free)(struct checker *);      /* to free the context */
>  	void (*reset)(void);		     /* to reset the global variables */
> +	void *(*thread)(void *);	     /* async thread entry point */
>  	const char **msgtable;
>  	short msgtable_size;
>  };
> @@ -55,19 +58,32 @@ static struct checker_class *alloc_checker_class(void)
>  	c = MALLOC(sizeof(struct checker_class));
>  	if (c) {
>  		INIT_LIST_HEAD(&c->node);
> -		c->refcount = 1;
> +		uatomic_set(&c->refcount, 1);
>  	}
>  	return c;
>  }
>  
> +/* Use uatomic_{sub,add}_return() to ensure proper memory barriers */
> +static int checker_class_ref(struct checker_class *cls)
> +{
> +	return uatomic_add_return(&cls->refcount, 1);
> +}
> +
> +static int checker_class_unref(struct checker_class *cls)
> +{
> +	return uatomic_sub_return(&cls->refcount, 1);
> +}
> +
>  void free_checker_class(struct checker_class *c)
>  {
> +	int cnt;
> +
>  	if (!c)
>  		return;
> -	c->refcount--;
> -	if (c->refcount) {
> -		condlog(4, "%s checker refcount %d",
> -			c->name, c->refcount);
> +	cnt = checker_class_unref(c);
> +	if (cnt != 0) {
> +		condlog(cnt < 0 ? 1 : 4, "%s checker refcount %d",
> +			c->name, cnt);
>  		return;
>  	}
>  	condlog(3, "unloading %s checker", c->name);
> @@ -161,7 +177,8 @@ static struct checker_class *add_checker_class(const char *multipath_dir,
>  
>  	c->mp_init = (int (*)(struct checker *)) dlsym(c->handle, "libcheck_mp_init");
>  	c->reset = (void (*)(void)) dlsym(c->handle, "libcheck_reset");
> -	/* These 2 functions can be NULL. call dlerror() to clear out any
> +	c->thread = (void *(*)(void*)) dlsym(c->handle, "libcheck_thread");
> +	/* These 3 functions can be NULL. call dlerror() to clear out any
>  	 * error string */
>  	dlerror();
>  
> @@ -347,6 +364,43 @@ bad_id:
>  	return generic_msg[CHECKER_MSGID_NONE];
>  }
>  
> +static void checker_cleanup_thread(void *arg)
> +{
> +	struct checker_class *cls = arg;
> +
> +	(void)checker_class_unref(cls);
> +	rcu_unregister_thread();
> +}
> +
> +static void *checker_thread_entry(void *arg)
> +{
> +	struct checker_context *ctx = arg;
> +	void *rv;
> +
> +	rcu_register_thread();
> +	pthread_cleanup_push(checker_cleanup_thread, ctx->cls);
> +	rv = ctx->cls->thread(ctx);
> +	pthread_cleanup_pop(1);
> +	return rv;
> +}
> +
> +int start_checker_thread(pthread_t *thread, const pthread_attr_t *attr,
> +			 struct checker_context *ctx)
> +{
> +	int rv;
> +
> +	assert(ctx && ctx->cls && ctx->cls->thread);
> +	/* Take a ref here, lest the class be freed before the thread starts */
> +	(void)checker_class_ref(ctx->cls);
> +	rv = pthread_create(thread, attr, checker_thread_entry, ctx);
> +	if (rv != 0) {
> +		condlog(1, "failed to start checker thread for %s: %m",
> +			ctx->cls->name);
> +		checker_class_unref(ctx->cls);
> +	}
> +	return rv;
> +}
> +
>  void checker_clear_message (struct checker *c)
>  {
>  	if (!c)
> @@ -371,7 +425,7 @@ void checker_get(const char *multipath_dir, struct checker *dst,
>  	if (!src)
>  		return;
>  
> -	src->refcount++;
> +	(void)checker_class_ref(dst->cls);
>  }
>  
>  int init_checkers(const char *multipath_dir)
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index 9d5f90b..2fd1d1c 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -1,6 +1,7 @@
>  #ifndef _CHECKERS_H
>  #define _CHECKERS_H
>  
> +#include <pthread.h>
>  #include "list.h"
>  #include "memory.h"
>  #include "defaults.h"
> @@ -148,6 +149,28 @@ void checker_set_async (struct checker *);
>  void checker_set_fd (struct checker *, int);
>  void checker_enable (struct checker *);
>  void checker_disable (struct checker *);
> +/*
> + * start_checker_thread(): start async path checker thread
> + *
> + * This function provides a wrapper around pthread_create().
> + * The created thread will call the DSO's "libcheck_thread" function with the
> + * checker context as argument.
> + *
> + * Rationale:
> + * Path checkers that do I/O may hang forever. To avoid blocking, some
> + * checkers therefore use asyncronous, detached threads for checking
> + * the paths. These threads may continue hanging if multipathd is stopped.
> + * In this case, we can't unload the checker DSO at exit. In order to
> + * avoid race conditions and crashes, the entry point of the thread
> + * needs to be in libmultipath, not in the DSO itself.
> + *
> + * @param arg: pointer to struct checker_context.
> + */
> +struct checker_context {
> +	struct checker_class *cls;
> +};
> +int start_checker_thread (pthread_t *thread, const pthread_attr_t *attr,
> +			  struct checker_context *ctx);
>  int checker_check (struct checker *, int);
>  int checker_is_sync(const struct checker *);
>  const char *checker_name (const struct checker *);
> @@ -164,6 +187,8 @@ void checker_get(const char *, struct checker *, const char *);
>  int libcheck_check(struct checker *);
>  int libcheck_init(struct checker *);
>  void libcheck_free(struct checker *);
> +void *libcheck_thread(struct checker_context *ctx);
> +
>  /*
>   * msgid => message map.
>   *
> diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c
> index e886fcf..a4b4a21 100644
> --- a/libmultipath/checkers/tur.c
> +++ b/libmultipath/checkers/tur.c
> @@ -15,7 +15,6 @@
>  #include <errno.h>
>  #include <sys/time.h>
>  #include <pthread.h>
> -#include <urcu.h>
>  #include <urcu/uatomic.h>
>  
>  #include "checkers.h"
> @@ -55,6 +54,7 @@ struct tur_checker_context {
>  	pthread_cond_t active;
>  	int holders; /* uatomic access only */
>  	int msgid;
> +	struct checker_context ctx;
>  };
>  
>  int libcheck_init (struct checker * c)
> @@ -74,6 +74,7 @@ int libcheck_init (struct checker * c)
>  	pthread_mutex_init(&ct->lock, NULL);
>  	if (fstat(c->fd, &sb) == 0)
>  		ct->devt = sb.st_rdev;
> +	ct->ctx.cls = c->cls;
>  	c->context = ct;
>  
>  	return 0;
> @@ -204,7 +205,6 @@ static void cleanup_func(void *data)
>  	holders = uatomic_sub_return(&ct->holders, 1);
>  	if (!holders)
>  		cleanup_context(ct);
> -	rcu_unregister_thread();
>  }
>  
>  /*
> @@ -251,15 +251,15 @@ static void tur_deep_sleep(const struct tur_checker_context *ct)
>  #define tur_deep_sleep(x) do {} while (0)
>  #endif /* TUR_TEST_MAJOR */
>  
> -static void *tur_thread(void *ctx)
> +void *libcheck_thread(struct checker_context *ctx)
>  {
> -	struct tur_checker_context *ct = ctx;
> +	struct tur_checker_context *ct =
> +		container_of(ctx, struct tur_checker_context, ctx);
>  	int state, running;
>  	short msgid;
>  
>  	/* This thread can be canceled, so setup clean up */
>  	tur_thread_cleanup_push(ct);
> -	rcu_register_thread();
>  
>  	condlog(4, "%d:%d : tur checker starting up", major(ct->devt),
>  		minor(ct->devt));
> @@ -394,7 +394,7 @@ int libcheck_check(struct checker * c)
>  		uatomic_set(&ct->running, 1);
>  		tur_set_async_timeout(c);
>  		setup_thread_attr(&attr, 32 * 1024, 1);
> -		r = pthread_create(&ct->thread, &attr, tur_thread, ct);
> +		r = start_checker_thread(&ct->thread, &attr, &ct->ctx);
>  		pthread_attr_destroy(&attr);
>  		if (r) {
>  			uatomic_sub(&ct->holders, 1);
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 2e3583f..751099d 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -270,3 +270,8 @@ global:
>  	dm_prereq;
>  	skip_libmp_dm_init;
>  } LIBMULTIPATH_4.1.0;
> +
> +LIBMULTIPATH_4.3.0 {
> +global:
> +	start_checker_thread;
> +} LIBMULTIPATH_4.2.0;
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete
  2020-12-17 11:00 ` [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete mwilck
@ 2020-12-18  5:48   ` Benjamin Marzinski
  2020-12-18 15:06     ` Martin Wilck
  2020-12-18 17:57   ` Benjamin Marzinski
  1 sibling, 1 reply; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18  5:48 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:15PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We've recently observed various cases of incompletely processed uevents
> during initrd processing. Typically, this would leave a dm device in
> the state it had after the initial "add" uevent, which is basically unusable,
> because udevd had been killed by systemd before processing the subsequent
> "change" event. After switching root, the coldplug event would re-read
> the db file, which would be in unusable state, and would not do anything.
> In such cases, a RELOAD action with force_udev_reload=1 is in order to
> make udev re-process the device completely (DM_UDEV_PRIMARY_SOURCE_FLAG=1 and
> DM_SUBSYSTEM_UDEV_FLAG0=0).
> 
> The previous commits
> 
> 2b25a9e libmultipath: select_action(): force udev reload for uninitialized maps
> cb10d38 multipathd: uev_trigger(): handle incomplete ADD events
> 
> addressed the same issue, but incompletely. They would miss cases where the
> map was configured correctly but none of the RELOAD criteria were met.
> This patch partially reverts 2b25a9e by converting select_reload_action() into
> a trivial helper. Instead, we now check for incompletely initialized udev now
> before checking any of the other reload criteria.

I'll review this patch tomorrow, but are you able to reproduce this?
I've seen something similar, except in the case I saw, multipathd took
too long during the initial configuration, and the systemd shut things
down for the switch-root before multipath could finish creating the
devices. I was thinking of trying to solve cases like this by forcing
some ordering on multipathd stopping in the initramfs, with something
like

Before=initrd-cleanup.service
Conflicts=initrd-cleanup.service

for the multipathd.service file for the initramfs. The goal is to make
sure that things don't get shutdown until multipathd has stopped. This
would keep multipath from creating devices when udev isn't around to
deal with them. Did you try something like this?

-Ben
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 45 ++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 3dbc1f1..d64fe88 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -695,12 +695,11 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
>  	return err;
>  }
>  
> -static void
> -select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
> -		     const char *reason)
> +static bool is_udev_ready(struct multipath *cmpp)
>  {
>  	struct udev_device *mpp_ud;
>  	const char *env;
> +	bool rc;
>  
>  	/*
>  	 * MPATH_DEVICE_READY != 1 can mean two things:
> @@ -712,14 +711,20 @@ select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
>  	 */
>  
>  	mpp_ud = get_udev_for_mpp(cmpp);
> +	if (!mpp_ud)
> +		return true;
>  	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
> -	if ((!env || strcmp(env, "1")) && count_active_paths(mpp) > 0)
> -		mpp->force_udev_reload = 1;
> +	rc = (env != NULL && !strcmp(env, "1"));
>  	udev_device_unref(mpp_ud);
> +	condlog(4, "%s: %s: \"%s\" -> %d\n", __func__, cmpp->alias, env, rc);
> +	return rc;
> +}
> +
> +static void
> +select_reload_action(struct multipath *mpp, const char *reason)
> +{
>  	mpp->action = ACT_RELOAD;
> -	condlog(3, "%s: set ACT_RELOAD (%s%s)", mpp->alias,
> -		mpp->force_udev_reload ? "forced, " : "",
> -		reason);
> +	condlog(3, "%s: set ACT_RELOAD (%s)", mpp->alias, reason);
>  }
>  
>  void select_action (struct multipath *mpp, const struct _vector *curmp,
> @@ -788,10 +793,18 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  		return;
>  	}
>  
> +	if (!is_udev_ready(cmpp) && count_active_paths(mpp) > 0) {
> +		mpp->force_udev_reload = 1;
> +		mpp->action = ACT_RELOAD;
> +		condlog(3, "%s: set ACT_RELOAD (udev incomplete)",
> +			mpp->alias);
> +		return;
> +	}
> +
>  	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
>  	    !!strstr(mpp->features, "queue_if_no_path") !=
>  	    !!strstr(cmpp->features, "queue_if_no_path")) {
> -		select_reload_action(mpp, cmpp, "no_path_retry change");
> +		select_reload_action(mpp, "no_path_retry change");
>  		return;
>  	}
>  	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
> @@ -799,7 +812,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
>  	     strncmp(cmpp->hwhandler, mpp->hwhandler,
>  		    strlen(mpp->hwhandler)))) {
> -		select_reload_action(mpp, cmpp, "hwhandler change");
> +		select_reload_action(mpp, "hwhandler change");
>  		return;
>  	}
>  
> @@ -807,7 +820,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
>  	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
> -		select_reload_action(mpp, cmpp, "retain_hwhandler change");
> +		select_reload_action(mpp, "retain_hwhandler change");
>  		return;
>  	}
>  
> @@ -819,7 +832,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  		remove_feature(&cmpp_feat, "queue_if_no_path");
>  		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
>  		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
> -			select_reload_action(mpp, cmpp, "features change");
> +			select_reload_action(mpp, "features change");
>  			FREE(cmpp_feat);
>  			FREE(mpp_feat);
>  			return;
> @@ -830,19 +843,19 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  
>  	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
>  		    strlen(mpp->selector))) {
> -		select_reload_action(mpp, cmpp, "selector change");
> +		select_reload_action(mpp, "selector change");
>  		return;
>  	}
>  	if (cmpp->minio != mpp->minio) {
> -		select_reload_action(mpp, cmpp, "minio change");
> +		select_reload_action(mpp, "minio change");
>  		return;
>  	}
>  	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
> -		select_reload_action(mpp, cmpp, "path group number change");
> +		select_reload_action(mpp, "path group number change");
>  		return;
>  	}
>  	if (pgcmp(mpp, cmpp)) {
> -		select_reload_action(mpp, cmpp, "path group topology change");
> +		select_reload_action(mpp, "path group topology change");
>  		return;
>  	}
>  	if (cmpp->nextpg != mpp->bestpg) {
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete
  2020-12-18  5:48   ` Benjamin Marzinski
@ 2020-12-18 15:06     ` Martin Wilck
  2020-12-18 15:08       ` Martin Wilck
  0 siblings, 1 reply; 20+ messages in thread
From: Martin Wilck @ 2020-12-18 15:06 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Thu, 2020-12-17 at 23:48 -0600, Benjamin Marzinski wrote:
> On Thu, Dec 17, 2020 at 12:00:15PM +0100, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > We've recently observed various cases of incompletely processed
> > uevents
> > during initrd processing. Typically, this would leave a dm device
> > in
> > the state it had after the initial "add" uevent, which is basically
> > unusable,
> > because udevd had been killed by systemd before processing the
> > subsequent
> > "change" event. After switching root, the coldplug event would re-
> > read
> > the db file, which would be in unusable state, and would not do
> > anything.
> > In such cases, a RELOAD action with force_udev_reload=1 is in order
> > to
> > make udev re-process the device completely
> > (DM_UDEV_PRIMARY_SOURCE_FLAG=1 and
> > DM_SUBSYSTEM_UDEV_FLAG0=0).
> > 
> > The previous commits
> > 
> > 2b25a9e libmultipath: select_action(): force udev reload for
> > uninitialized maps
> > cb10d38 multipathd: uev_trigger(): handle incomplete ADD events
> > 
> > addressed the same issue, but incompletely. They would miss cases
> > where the
> > map was configured correctly but none of the RELOAD criteria were
> > met.
> > This patch partially reverts 2b25a9e by converting
> > select_reload_action() into
> > a trivial helper. Instead, we now check for incompletely
> > initialized udev now
> > before checking any of the other reload criteria.
> 
> I'll review this patch tomorrow, but are you able to reproduce this?

Not me, but multiple customers of ours :-/ Most of them where running
PowperPC, for reasons I can only speculate about. The user-visible
phenomenon is that some upper layers on some maps (kpartx-created
partitions, LVM, ...) are not present after boot, and "multipathd
reload" fixes the situation.

I suppose it should be reproducible if one has multiple multipath maps
with partitions, devices are discovered somewhat slowly / with delays
during intird processing, and the root device is discovered early on.
Then systemd has enough time to mount the root FS and stop services
before all maps are completely set up. The exact behavior would still
depend on timing (but in the in the last case I worked on, it was 100%
reproducible by the customer).

> I've seen something similar, except in the case I saw, multipathd
> took
> too long during the initial configuration, and the systemd shut
> things
> down for the switch-root before multipath could finish creating the
> devices. I was thinking of trying to solve cases like this by forcing
> some ordering on multipathd stopping in the initramfs, with something
> like
> 
> Before=initrd-cleanup.service
> Conflicts=initrd-cleanup.service
> 
> for the multipathd.service file for the initramfs. The goal is to
> make
> sure that things don't get shutdown until multipathd has stopped.
> This
> would keep multipath from creating devices when udev isn't around to
> deal with them. Did you try something like this?

No, I didn't think of that. It's an interesting idea, although it might
slow down booting. IMO it's actually a good thing that the services in
the initrd are stopped quickly when the root FS becomes available. It
can just have some side effects our current code doesn't deal well
with.

AFAICS, multipathd is not the problem, udev is. I can see that
multipathd has cleanly set up the maps, but udev has been stopped
before processing the respective events (in particular, the change
events).

If this happens, the udev db for the affected maps looks more or less
like this:

DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG="1"
DM_UDEV_DISABLE_DISK_RULES_FLAG="1"
DM_UDEV_DISABLE_OTHER_RULES_FLAG="1"

The device-mapper hotplug mechanism doesn't help here, because it tries
to import properties from the db. Triggering uevents in other ways
helps even less. Only a "genuine" change event without the "reload"
flag (DM_SUBSYSTEM_UDEV_FLAG0) set will do the trick.

When multipathd starts after switching root, it sees the maps in
perfect state as far as multipath properties are concerned, and thus
will not set the force_udev_reload flag. This patch changes that.

(My break-through step when I attempted to understand the issue was to
tell customers to tar up /run/udev/data before starting coldplug. That
way I could see the state in which the udev db was left when initrd
finished, and I saw the half-completed entries like above).

I suppose this works differently on Red Hat where you use mpathconf and
set up only a very limited set of maps during initrd processing.
Therefore my guess was you'd not see this at all. I'm still wondering
why we have been seeing it only very recently. *Perhaps* my recent
changes to make multipathd shutdown more quickly are part of the
equation, I'm unsure about that. I am pretty positive that this patch
is effective, though.

Regards,
Martin


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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete
  2020-12-18 15:06     ` Martin Wilck
@ 2020-12-18 15:08       ` Martin Wilck
  0 siblings, 0 replies; 20+ messages in thread
From: Martin Wilck @ 2020-12-18 15:08 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Fri, 2020-12-18 at 16:06 +0100, Martin Wilck wrote:
> 
> If this happens, the udev db for the affected maps looks more or less
> like this:
> 
> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG="1"
> DM_UDEV_DISABLE_DISK_RULES_FLAG="1"
> DM_UDEV_DISABLE_OTHER_RULES_FLAG="1"
> 
> The device-mapper hotplug mechanism doesn't help here, 

I meant "coldplug mechanism". Sorry.

Martin


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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock
  2020-12-18  0:03   ` Benjamin Marzinski
@ 2020-12-18 16:24     ` Martin Wilck
  2020-12-18 16:32       ` Benjamin Marzinski
  0 siblings, 1 reply; 20+ messages in thread
From: Martin Wilck @ 2020-12-18 16:24 UTC (permalink / raw)
  To: Benjamin Marzinski; +Cc: dm-devel

On Thu, 2020-12-17 at 18:03 -0600, Benjamin Marzinski wrote:
> On Thu, Dec 17, 2020 at 12:00:13PM +0100, mwilck@suse.com wrote:
> >  
> > -void free_logarea (void)
> > +static void free_logarea (void)
> >  {
> >  	FREE(la->start);
> >  	FREE(la->buff);
> 
> I realize that the log area can only be freed by log_close(), which
> is
> called when mulitpathd exits, but it would be nice to have la set to
> NULL it's freed, just to make it obvious that that there can't be
> double-frees there. 

There's FREE(la) right after this, and FREE(x) sets the pointer to
NULL. So it should be fine, or am I confused?

> However, the code is clearly correct, so
> 
> Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

Thanks,
Martin


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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock
  2020-12-18 16:24     ` Martin Wilck
@ 2020-12-18 16:32       ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18 16:32 UTC (permalink / raw)
  To: Martin Wilck; +Cc: dm-devel

On Fri, Dec 18, 2020 at 05:24:25PM +0100, Martin Wilck wrote:
> On Thu, 2020-12-17 at 18:03 -0600, Benjamin Marzinski wrote:
> > On Thu, Dec 17, 2020 at 12:00:13PM +0100, mwilck@suse.com wrote:
> > >  
> > > -void free_logarea (void)
> > > +static void free_logarea (void)
> > >  {
> > >  	FREE(la->start);
> > >  	FREE(la->buff);
> > 
> > I realize that the log area can only be freed by log_close(), which
> > is
> > called when mulitpathd exits, but it would be nice to have la set to
> > NULL it's freed, just to make it obvious that that there can't be
> > double-frees there. 
> 
> There's FREE(la) right after this, and FREE(x) sets the pointer to
> NULL. So it should be fine, or am I confused?
> 

Yep it's fine. It was me missing that.

-Ben

> > However, the code is clearly correct, so
> > 
> > Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Thanks,
> Martin
> 

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete
  2020-12-17 11:00 ` [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete mwilck
  2020-12-18  5:48   ` Benjamin Marzinski
@ 2020-12-18 17:57   ` Benjamin Marzinski
  1 sibling, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18 17:57 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:15PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> We've recently observed various cases of incompletely processed uevents
> during initrd processing. Typically, this would leave a dm device in
> the state it had after the initial "add" uevent, which is basically unusable,
> because udevd had been killed by systemd before processing the subsequent
> "change" event. After switching root, the coldplug event would re-read
> the db file, which would be in unusable state, and would not do anything.
> In such cases, a RELOAD action with force_udev_reload=1 is in order to
> make udev re-process the device completely (DM_UDEV_PRIMARY_SOURCE_FLAG=1 and
> DM_SUBSYSTEM_UDEV_FLAG0=0).
> 
> The previous commits
> 
> 2b25a9e libmultipath: select_action(): force udev reload for uninitialized maps
> cb10d38 multipathd: uev_trigger(): handle incomplete ADD events
> 
> addressed the same issue, but incompletely. They would miss cases where the
> map was configured correctly but none of the RELOAD criteria were met.
> This patch partially reverts 2b25a9e by converting select_reload_action() into
> a trivial helper. Instead, we now check for incompletely initialized udev now
> before checking any of the other reload criteria.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/configure.c | 45 ++++++++++++++++++++++++++--------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 3dbc1f1..d64fe88 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -695,12 +695,11 @@ sysfs_set_max_sectors_kb(struct multipath *mpp, int is_reload)
>  	return err;
>  }
>  
> -static void
> -select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
> -		     const char *reason)
> +static bool is_udev_ready(struct multipath *cmpp)
>  {
>  	struct udev_device *mpp_ud;
>  	const char *env;
> +	bool rc;
>  
>  	/*
>  	 * MPATH_DEVICE_READY != 1 can mean two things:
> @@ -712,14 +711,20 @@ select_reload_action(struct multipath *mpp, const struct multipath *cmpp,
>  	 */
>  
>  	mpp_ud = get_udev_for_mpp(cmpp);
> +	if (!mpp_ud)
> +		return true;
>  	env = udev_device_get_property_value(mpp_ud, "MPATH_DEVICE_READY");
> -	if ((!env || strcmp(env, "1")) && count_active_paths(mpp) > 0)
> -		mpp->force_udev_reload = 1;
> +	rc = (env != NULL && !strcmp(env, "1"));
>  	udev_device_unref(mpp_ud);
> +	condlog(4, "%s: %s: \"%s\" -> %d\n", __func__, cmpp->alias, env, rc);
> +	return rc;
> +}
> +
> +static void
> +select_reload_action(struct multipath *mpp, const char *reason)
> +{
>  	mpp->action = ACT_RELOAD;
> -	condlog(3, "%s: set ACT_RELOAD (%s%s)", mpp->alias,
> -		mpp->force_udev_reload ? "forced, " : "",
> -		reason);
> +	condlog(3, "%s: set ACT_RELOAD (%s)", mpp->alias, reason);
>  }
>  
>  void select_action (struct multipath *mpp, const struct _vector *curmp,
> @@ -788,10 +793,18 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  		return;
>  	}
>  
> +	if (!is_udev_ready(cmpp) && count_active_paths(mpp) > 0) {
> +		mpp->force_udev_reload = 1;
> +		mpp->action = ACT_RELOAD;
> +		condlog(3, "%s: set ACT_RELOAD (udev incomplete)",
> +			mpp->alias);
> +		return;
> +	}
> +
>  	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
>  	    !!strstr(mpp->features, "queue_if_no_path") !=
>  	    !!strstr(cmpp->features, "queue_if_no_path")) {
> -		select_reload_action(mpp, cmpp, "no_path_retry change");
> +		select_reload_action(mpp, "no_path_retry change");
>  		return;
>  	}
>  	if ((mpp->retain_hwhandler != RETAIN_HWHANDLER_ON ||
> @@ -799,7 +812,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  	    (strlen(cmpp->hwhandler) != strlen(mpp->hwhandler) ||
>  	     strncmp(cmpp->hwhandler, mpp->hwhandler,
>  		    strlen(mpp->hwhandler)))) {
> -		select_reload_action(mpp, cmpp, "hwhandler change");
> +		select_reload_action(mpp, "hwhandler change");
>  		return;
>  	}
>  
> @@ -807,7 +820,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  	    !!strstr(mpp->features, "retain_attached_hw_handler") !=
>  	    !!strstr(cmpp->features, "retain_attached_hw_handler") &&
>  	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0)) {
> -		select_reload_action(mpp, cmpp, "retain_hwhandler change");
> +		select_reload_action(mpp, "retain_hwhandler change");
>  		return;
>  	}
>  
> @@ -819,7 +832,7 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  		remove_feature(&cmpp_feat, "queue_if_no_path");
>  		remove_feature(&cmpp_feat, "retain_attached_hw_handler");
>  		if (strncmp(mpp_feat, cmpp_feat, PARAMS_SIZE)) {
> -			select_reload_action(mpp, cmpp, "features change");
> +			select_reload_action(mpp, "features change");
>  			FREE(cmpp_feat);
>  			FREE(mpp_feat);
>  			return;
> @@ -830,19 +843,19 @@ void select_action (struct multipath *mpp, const struct _vector *curmp,
>  
>  	if (!cmpp->selector || strncmp(cmpp->selector, mpp->selector,
>  		    strlen(mpp->selector))) {
> -		select_reload_action(mpp, cmpp, "selector change");
> +		select_reload_action(mpp, "selector change");
>  		return;
>  	}
>  	if (cmpp->minio != mpp->minio) {
> -		select_reload_action(mpp, cmpp, "minio change");
> +		select_reload_action(mpp, "minio change");
>  		return;
>  	}
>  	if (!cmpp->pg || VECTOR_SIZE(cmpp->pg) != VECTOR_SIZE(mpp->pg)) {
> -		select_reload_action(mpp, cmpp, "path group number change");
> +		select_reload_action(mpp, "path group number change");
>  		return;
>  	}
>  	if (pgcmp(mpp, cmpp)) {
> -		select_reload_action(mpp, cmpp, "path group topology change");
> +		select_reload_action(mpp, "path group topology change");
>  		return;
>  	}
>  	if (cmpp->nextpg != mpp->bestpg) {
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime
  2020-12-17 11:00 ` [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime mwilck
@ 2020-12-18 18:14   ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18 18:14 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:16PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> If the root file system is multipathed, and IO is queued because all paths
> are failed, multipathd may block trying to access the root FS, and thus be
> unable to reinstate paths. One file that is frequently accessed is
> /etc/localtime. Avoid that by printing monotonic timestamps instead.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/debug.c     | 14 ++++++++------
>  libmultipath/devmapper.c | 12 ++++++------
>  libmultipath/log.c       |  1 -
>  multipathd/main.c        |  3 ---
>  4 files changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/libmultipath/debug.c b/libmultipath/debug.c
> index 429f269..510e15e 100644
> --- a/libmultipath/debug.c
> +++ b/libmultipath/debug.c
> @@ -14,6 +14,8 @@
>  #include "config.h"
>  #include "defaults.h"
>  #include "debug.h"
> +#include "time-util.h"
> +#include "util.h"
>  
>  int logsink;
>  int libmp_verbosity = DEFAULT_VERBOSITY;
> @@ -25,13 +27,13 @@ void dlog(int prio, const char * fmt, ...)
>  	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];
> +			struct timespec ts;
> +			char buff[32];
>  
> -			strftime(buff, sizeof(buff),
> -				 "%b %d %H:%M:%S", tb);
> -			buff[sizeof(buff)-1] = '\0';
> +			get_monotonic_time(&ts);
> +			safe_sprintf(buff, "%ld.%06ld",
> +				     (long)ts.tv_sec,
> +				     ts.tv_nsec/1000);
>  			fprintf(stderr, "%s | ", buff);
>  		}
>  		vfprintf(stderr, fmt, ap);
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 4977b31..095cbc0 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -27,6 +27,7 @@
>  #include "config.h"
>  #include "wwids.h"
>  #include "version.h"
> +#include "time-util.h"
>  
>  #include "log_pthread.h"
>  #include <sys/types.h>
> @@ -106,13 +107,12 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
>  	va_start(ap, f);
>  	if (logsink != LOGSINK_SYSLOG) {
>  		if (logsink == 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';
> +			struct timespec ts;
> +			char buff[32];
>  
> +			get_monotonic_time(&ts);
> +			safe_sprintf(buff, "%ld.%06ld",
> +				     (long)ts.tv_sec, ts.tv_nsec/1000);
>  			fprintf(stderr, "%s | ", buff);
>  		}
>  		fprintf(stderr, "libdevmapper: %s(%i): ", file, line);
> diff --git a/libmultipath/log.c b/libmultipath/log.c
> index 95c8f01..6498c88 100644
> --- a/libmultipath/log.c
> +++ b/libmultipath/log.c
> @@ -120,7 +120,6 @@ void log_reset (char *program_name)
>  	pthread_cleanup_push(cleanup_mutex, &logq_lock);
>  
>  	closelog();
> -	tzset();
>  	openlog(program_name, 0, LOG_DAEMON);
>  
>  	pthread_cleanup_pop(1);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index b6a5f5b..28c147b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2710,9 +2710,6 @@ reconfigure (struct vectors * vecs)
>  	delete_all_foreign();
>  
>  	reset_checker_classes();
> -	/* Re-read any timezone changes */
> -	tzset();
> -
>  	if (bindings_read_only)
>  		conf->bindings_read_only = bindings_read_only;
>  	check_alias_settings(conf);
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions
  2020-12-17 11:00 ` [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions mwilck
@ 2020-12-18 18:36   ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18 18:36 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:17PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> By adding -Wl,-z,defs, we'll get warnings about unresolved symbols
> at the linking stage. This way we make sure our plugins (checkers etc.)
> will use versioned symbols from libmultipath, and incompatible plugins
> can't be loaded any more. Doing this requires explicitly linking
> the plugins with all libraries they use, in particular libmultipath.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  Makefile                           | 1 +
>  Makefile.inc                       | 2 +-
>  libmpathpersist/Makefile           | 8 ++++----
>  libmultipath/checkers/Makefile     | 7 +++----
>  libmultipath/foreign/Makefile      | 4 +++-
>  libmultipath/prioritizers/Makefile | 7 +++----
>  6 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f127ff9..bddb2bf 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -31,6 +31,7 @@ $(BUILDDIRS):
>  
>  libmultipath libdmmp: libmpathcmd
>  libmpathpersist libmpathvalid multipath multipathd: libmultipath
> +libmultipath/prioritizers libmultipath/checkers libmultipath/foreign: libmultipath
>  mpathpersist multipathd:  libmpathpersist
>  
>  libmultipath/checkers.install \
> diff --git a/Makefile.inc b/Makefile.inc
> index 13587a9..0542930 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -105,7 +105,7 @@ CFLAGS		:= --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
>  BIN_CFLAGS	= -fPIE -DPIE
>  LIB_CFLAGS	= -fPIC
>  SHARED_FLAGS	= -shared
> -LDFLAGS		:= $(LDFLAGS) -Wl,-z,relro -Wl,-z,now
> +LDFLAGS		:= $(LDFLAGS) -Wl,-z,relro -Wl,-z,now -Wl,-z,defs
>  BIN_LDFLAGS	= -pie
>  
>  # Check whether a function with name $1 has been declared in header file $2.
> diff --git a/libmpathpersist/Makefile b/libmpathpersist/Makefile
> index 456ce4c..57103e5 100644
> --- a/libmpathpersist/Makefile
> +++ b/libmpathpersist/Makefile
> @@ -6,17 +6,17 @@ LIBS = $(DEVLIB).$(SONAME)
>  VERSION_SCRIPT := libmpathpersist.version
>  
>  CFLAGS += $(LIB_CFLAGS) -I$(multipathdir) -I$(mpathpersistdir) -I$(mpathcmddir)
> +LDFLAGS += -L$(multipathdir) -L$(mpathcmddir)
>  
> -LIBDEPS += -lpthread -ldevmapper -ldl -L$(multipathdir) -lmultipath \
> -	   -L$(mpathcmddir) -lmpathcmd
> +LIBDEPS += -lmultipath -lmpathcmd -ldevmapper -lpthread -ldl
>  
>  OBJS = mpath_persist.o mpath_updatepr.o mpath_pr_ioctl.o
>  
>  all: $(DEVLIB) man
>  
>  $(LIBS): $(OBJS) $(VERSION_SCRIPT)
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) $(LIBDEPS) -Wl,-soname=$@ \
> -		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS)
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -Wl,-soname=$@ \
> +		-Wl,--version-script=$(VERSION_SCRIPT) -o $@ $(OBJS) $(LIBDEPS)
>  
>  $(DEVLIB): $(LIBS)
>  	$(LN) $(LIBS) $@
> diff --git a/libmultipath/checkers/Makefile b/libmultipath/checkers/Makefile
> index 01c0451..8e0ed5e 100644
> --- a/libmultipath/checkers/Makefile
> +++ b/libmultipath/checkers/Makefile
> @@ -4,6 +4,8 @@
>  include ../../Makefile.inc
>  
>  CFLAGS += $(LIB_CFLAGS) -I..
> +LDFLAGS += -L.. -lmultipath
> +LIBDEPS = -lmultipath -laio -lpthread -lrt
>  
>  # If you add or remove a checker also update multipath/multipath.conf.5
>  LIBS= \
> @@ -17,11 +19,8 @@ LIBS= \
>  
>  all: $(LIBS)
>  
> -libcheckdirectio.so: directio.o
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -laio
> -
>  libcheck%.so: %.o
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
>  
>  install:
>  	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
> diff --git a/libmultipath/foreign/Makefile b/libmultipath/foreign/Makefile
> index fae58a0..f447a1c 100644
> --- a/libmultipath/foreign/Makefile
> +++ b/libmultipath/foreign/Makefile
> @@ -5,13 +5,15 @@ TOPDIR=../..
>  include ../../Makefile.inc
>  
>  CFLAGS += $(LIB_CFLAGS) -I.. -I$(nvmedir)
> +LDFLAGS += -L..
> +LIBDEPS = -lmultipath -ludev -lpthread -lrt
>  
>  LIBS = libforeign-nvme.so
>  
>  all: $(LIBS)
>  
>  libforeign-%.so: %.o
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
>  
>  install:
>  	$(INSTALL_PROGRAM) -m 755 $(LIBS) $(DESTDIR)$(libdir)
> diff --git a/libmultipath/prioritizers/Makefile b/libmultipath/prioritizers/Makefile
> index fc6e0e0..8d34ae3 100644
> --- a/libmultipath/prioritizers/Makefile
> +++ b/libmultipath/prioritizers/Makefile
> @@ -4,6 +4,8 @@
>  include ../../Makefile.inc
>  
>  CFLAGS += $(LIB_CFLAGS) -I..
> +LDFLAGS += -L..
> +LIBDEPS = -lmultipath -lm -lpthread -lrt
>  
>  # If you add or remove a prioritizer also update multipath/multipath.conf.5
>  LIBS = \
> @@ -28,11 +30,8 @@ endif
>  
>  all: $(LIBS)
>  
> -libpriopath_latency.so: path_latency.o
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ -lm
> -
>  libprio%.so: %.o
> -	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^
> +	$(CC) $(LDFLAGS) $(SHARED_FLAGS) -o $@ $^ $(LIBDEPS)
>  
>  install: $(LIBS)
>  	$(INSTALL_PROGRAM) -m 755 libprio*.so $(DESTDIR)$(libdir)
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
* Re: [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol
  2020-12-17 11:00 ` [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol mwilck
@ 2020-12-18 18:36   ` Benjamin Marzinski
  0 siblings, 0 replies; 20+ messages in thread
From: Benjamin Marzinski @ 2020-12-18 18:36 UTC (permalink / raw)
  To: mwilck; +Cc: dm-devel

On Thu, Dec 17, 2020 at 12:00:18PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> The weightedpath prioritizer uses get_next_string(). I'd overlooked
> this before. This was found with the help of the previous patch.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/libmultipath.version | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 751099d..2228f4e 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -275,3 +275,8 @@ LIBMULTIPATH_4.3.0 {
>  global:
>  	start_checker_thread;
>  } LIBMULTIPATH_4.2.0;
> +
> +LIBMULTIPATH_4.4.0 {
> +global:
> +	get_next_string;
> +} LIBMULTIPATH_4.3.0;
> -- 
> 2.29.0

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


^ permalink raw reply	[flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-12-18 18:37 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 11:00 [dm-devel] [PATCH 0/7] Various multipath-tools patches mwilck
2020-12-17 11:00 ` [dm-devel] [PATCH 1/7] libmultipath: move logq_lock handling to log.c mwilck
2020-12-17 23:56   ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 2/7] libmultipath: protect logarea with logq_lock mwilck
2020-12-18  0:03   ` Benjamin Marzinski
2020-12-18 16:24     ` Martin Wilck
2020-12-18 16:32       ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 3/7] libmultipath: prevent DSO unloading with astray checker threads mwilck
2020-12-18  4:22   ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 4/7] libmultipath: force map reload if udev incomplete mwilck
2020-12-18  5:48   ` Benjamin Marzinski
2020-12-18 15:06     ` Martin Wilck
2020-12-18 15:08       ` Martin Wilck
2020-12-18 17:57   ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 5/7] multipath-tools: avoid access to /etc/localtime mwilck
2020-12-18 18:14   ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 6/7] multipath-tools: make sure plugin DSOs use symbol versions mwilck
2020-12-18 18:36   ` Benjamin Marzinski
2020-12-17 11:00 ` [dm-devel] [PATCH 7/7] libmultipath.version: add missing symbol mwilck
2020-12-18 18:36   ` Benjamin Marzinski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.