[PATCH 00/12] multipath: new and rebased patches

[PATCH 00/12] multipath: new and rebased patches

[Benjamin Marzinski]

Patches 1-5 are minor bug fixes and a unit test for basenamecpy.

Patch 6 is revised version of my nanosleep patch, that only changes
the checkerloop code, since that is the only time when multipathd
actually sets an alarm without locking.

Patches 7-11 are a rebase of my "alternate dmevents waiter method"
patchset.  The only code that has changed outside of rebasing them is in
patch 11.  It adds a function, remove_map_by_alias(), and uses it in
dmevent_loop(). This simply moves the code the gets the multipath device
out of dmevents.c, so it can work exclusively in device names.  It also
changes the names of the functions the initialize and cleanup the
dmevents polling code to match similar multipathd functions and adds
a Makefile define to disable using the polling code, both suggested
by Martin Wilck.

Patch 12 is unit testing code for the new code in dmevents.c

Here is the description from my initial posting of the alternate dmevents
waiter method" patchset:

This patchset implements a new method of getting dmevents for
multipathd.

With the existing wait code, multipathd needs to create a waiter thread
for every multipath device. This can become very wasteful in setups with
large numbers of multipath devices. These duplicate threads all are
serialized to update the multipath devices, so they don't actually speed
up dmevent handling.

The new method uses the new dmevent polling ability introduced in the
4.37.0 device-mapper kernel module.  The original method has been
retained for backwards compatablility, and it is possible to force
multipathd to use the orignal method on newer kernels. The benefit of
this new method is that there is only one thread necessary to wait on
dmevents, which can be started when device-mapper starts, and stopped
during shutdown, just like the other main threads.

These patches use device-mapper features that don't have a libdevmapper
API.  They will switch over as soon as support is available in
libdevmapper.

Benjamin Marzinski (12):
  Unit tests for basenamecpy
  libmultipath: fix basenamecpy
  libmultipath: set dm_conf_verbosity
  multipathd: log thread cleanup
  libmultipath: fix log_pthread processing
  multipathd: use nanosleep for strict timing
  libmultipath: move remove_map waiter code to multipathd
  move waiter code from libmultipath to multipathd
  call start_waiter_thread() before setup_multipath()
  libmultipath: add helper functions
  multipathd: add new polling dmevents waiter thread
  multipath: add unit tests for dmevents code

 Makefile.inc               |   3 +
 libmultipath/Makefile      |   2 +-
 libmultipath/devmapper.c   |  29 +-
 libmultipath/devmapper.h   |   3 +-
 libmultipath/log_pthread.c |  40 +--
 libmultipath/log_pthread.h |  10 +-
 libmultipath/structs_vec.c | 140 +-------
 libmultipath/structs_vec.h |   8 +-
 libmultipath/util.c        |  25 +-
 libmultipath/util.h        |   2 +-
 libmultipath/vector.c      |  16 +-
 libmultipath/vector.h      |   1 +
 libmultipath/waiter.c      | 215 ------------
 libmultipath/waiter.h      |  17 -
 multipathd/Makefile        |   6 +-
 multipathd/dmevents.c      | 392 +++++++++++++++++++++
 multipathd/dmevents.h      |  13 +
 multipathd/main.c          | 230 ++++++++++--
 multipathd/waiter.c        | 215 ++++++++++++
 multipathd/waiter.h        |  17 +
 tests/Makefile             |   9 +-
 tests/dmevents.c           | 847 +++++++++++++++++++++++++++++++++++++++++++++
 tests/util.c               | 167 +++++++++
 23 files changed, 1944 insertions(+), 463 deletions(-)
 delete mode 100644 libmultipath/waiter.c
 delete mode 100644 libmultipath/waiter.h
 create mode 100644 multipathd/dmevents.c
 create mode 100644 multipathd/dmevents.h
 create mode 100644 multipathd/waiter.c
 create mode 100644 multipathd/waiter.h
 create mode 100644 tests/dmevents.c
 create mode 100644 tests/util.c

-- 
2.7.4

[PATCH 01/12] Unit tests for basenamecpy

[Benjamin Marzinski]

The current implementation of basenamecpy is broken, so some of these
tests currently fail. Fixes to follow.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/Makefile |   2 +-
 tests/util.c   | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 168 insertions(+), 1 deletion(-)
 create mode 100644 tests/util.c

diff --git a/tests/Makefile b/tests/Makefile
index 81f5518..3450b14 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,7 +3,7 @@ include ../Makefile.inc
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
 LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
 
-TESTS := uevent parser
+TESTS := uevent parser util
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
diff --git a/tests/util.c b/tests/util.c
new file mode 100644
index 0000000..e9a004f
--- /dev/null
+++ b/tests/util.c
@@ -0,0 +1,167 @@
+/*
+ * Copyright (c) 2018 Benjamin Marzinski, Redhat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ *
+ */
+
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+#include "util.h"
+
+#include "globals.c"
+
+static void test_basenamecpy_good0(void **state)
+{
+	char dst[10];
+
+	assert_int_equal(basenamecpy("foobar", dst, sizeof(dst)), 6);
+	assert_string_equal(dst, "foobar");
+}
+
+static void test_basenamecpy_good1(void **state)
+{
+	char dst[10];
+
+	assert_int_equal(basenamecpy("foo/bar", dst, sizeof(dst)), 3);
+	assert_string_equal(dst, "bar");
+}
+
+static void test_basenamecpy_good2(void **state)
+{
+	char dst[10];
+
+	assert_int_equal(basenamecpy("/thud/blat", dst, sizeof(dst)), 4);
+	assert_string_equal(dst, "blat");
+}
+
+static void test_basenamecpy_good3(void **state)
+{
+	char dst[4];
+
+	assert_int_equal(basenamecpy("foo/bar", dst, sizeof(dst)), 3);
+	assert_string_equal(dst, "bar");
+}
+
+static void test_basenamecpy_good4(void **state)
+{
+	char dst[10];
+
+	assert_int_equal(basenamecpy("/xyzzy", dst, sizeof(dst)), 5);
+	assert_string_equal(dst, "xyzzy");
+}
+
+static void test_basenamecpy_good5(void **state)
+{
+	char dst[4];
+
+	assert_int_equal(basenamecpy("/foo/bar\n", dst, sizeof(dst)), 3);
+	assert_string_equal(dst, "bar");
+}
+
+static void test_basenamecpy_good6(void **state)
+{
+        char dst[6];
+
+        assert_int_equal(basenamecpy("/xyzzy/plugh   ", dst, sizeof(dst)), 5);
+        assert_string_equal(dst, "plugh");
+}
+
+static void test_basenamecpy_good7(void **state)
+{
+	char src[] = "/foo/bar";
+	char dst[10];
+
+	assert_int_equal(basenamecpy(src, dst, sizeof(dst)), 3);
+
+	strcpy(src, "badbadno");
+	assert_string_equal(dst, "bar");
+}
+
+/* buffer too small */
+static void test_basenamecpy_bad0(void **state)
+{
+        char dst[3];
+
+        assert_int_equal(basenamecpy("baz", dst, sizeof(dst)), 0);
+}
+
+/* ends in slash */
+static void test_basenamecpy_bad1(void **state)
+{
+        char dst[10];
+
+        assert_int_equal(basenamecpy("foo/bar/", dst, sizeof(dst)), 0);
+}
+
+static void test_basenamecpy_bad2(void **state)
+{
+        char dst[10];
+
+        assert_int_equal(basenamecpy(NULL, dst, sizeof(dst)), 0);
+}
+
+static void test_basenamecpy_bad3(void **state)
+{
+        char dst[10];
+
+        assert_int_equal(basenamecpy("", dst, sizeof(dst)), 0);
+}
+
+static void test_basenamecpy_bad4(void **state)
+{
+        char dst[10];
+
+        assert_int_equal(basenamecpy("/", dst, sizeof(dst)), 0);
+}
+
+static void test_basenamecpy_bad5(void **state)
+{
+        char dst[10];
+
+        assert_int_equal(basenamecpy("baz/qux", NULL, sizeof(dst)), 0);
+}
+
+int test_basenamecpy(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_basenamecpy_good0),
+		cmocka_unit_test(test_basenamecpy_good1),
+		cmocka_unit_test(test_basenamecpy_good2),
+		cmocka_unit_test(test_basenamecpy_good3),
+		cmocka_unit_test(test_basenamecpy_good4),
+		cmocka_unit_test(test_basenamecpy_good5),
+		cmocka_unit_test(test_basenamecpy_good6),
+		cmocka_unit_test(test_basenamecpy_good7),
+		cmocka_unit_test(test_basenamecpy_bad0),
+		cmocka_unit_test(test_basenamecpy_bad1),
+		cmocka_unit_test(test_basenamecpy_bad2),
+		cmocka_unit_test(test_basenamecpy_bad3),
+		cmocka_unit_test(test_basenamecpy_bad4),
+		cmocka_unit_test(test_basenamecpy_bad5),
+	};
+	return cmocka_run_group_tests(tests, NULL, NULL);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	ret += test_basenamecpy();
+	return ret;
+}
-- 
2.7.4

Re: [PATCH 01/12] Unit tests for basenamecpy

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> The current implementation of basenamecpy is broken, so some of these
> tests currently fail. Fixes to follow.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  tests/Makefile |   2 +-
>  tests/util.c   | 167
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 168 insertions(+), 1 deletion(-)
>  create mode 100644 tests/util.c
> 

> +static void test_basenamecpy_good6(void **state)
> +{
> +        char dst[6];
> +
> +        assert_int_equal(basenamecpy("/xyzzy/plugh   ", dst,
> sizeof(dst)), 5);
> +        assert_string_equal(dst, "plugh");
> +}

This deserves explanation. "basename" wouldn't normally strip trailing
whitespace.

> +/* ends in slash */
> +static void test_basenamecpy_bad1(void **state)
> +{
> +        char dst[10];
> +
> +        assert_int_equal(basenamecpy("foo/bar/", dst, sizeof(dst)),
> 0);

This, too, deviates from standard "basename" behavior and should be
explained ("basename /usr/" yields "usr", so does "basename /usr///").

> +}
> +
> +static void test_basenamecpy_bad2(void **state)
> +{
> +        char dst[10];
> +
> +        assert_int_equal(basenamecpy(NULL, dst, sizeof(dst)), 0);
> +}
> +
> +static void test_basenamecpy_bad3(void **state)
> +{
> +        char dst[10];
> +
> +        assert_int_equal(basenamecpy("", dst, sizeof(dst)), 0);
> +}
> +
> +static void test_basenamecpy_bad4(void **state)
> +{
> +        char dst[10];
> +
> +        assert_int_equal(basenamecpy("/", dst, sizeof(dst)), 0);
> +}

Another one: "basename /" yields "/" in the shell, so does 
"basename ///").

I don't insist that basenamecpy behaves 100% identical as "basename",
but I reckon if expectations are different, we should explain why.

Regards,
Martin

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

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

Re: [PATCH 01/12] Unit tests for basenamecpy

[Martin Wilck]

On Mon, 2018-03-19 at 10:49 +0100, Martin Wilck wrote:
> On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> > The current implementation of basenamecpy is broken, so some of
> > these
> > tests currently fail. Fixes to follow.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  tests/Makefile |   2 +-
> >  tests/util.c   | 167
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 168 insertions(+), 1 deletion(-)
> >  create mode 100644 tests/util.c
> > 
> > +static void test_basenamecpy_good6(void **state)
> > +{
> > +        char dst[6];
> > +
> > +        assert_int_equal(basenamecpy("/xyzzy/plugh   ", dst,
> > sizeof(dst)), 5);
> > +        assert_string_equal(dst, "plugh");
> > +}
> 
> This deserves explanation. "basename" wouldn't normally strip
> trailing
> whitespace.
> 
> > +/* ends in slash */
> > +static void test_basenamecpy_bad1(void **state)
> > +{
> > +        char dst[10];
> > +
> > +        assert_int_equal(basenamecpy("foo/bar/", dst,
> > sizeof(dst)),
> > 0);
> 
> This, too, deviates from standard "basename" behavior and should be
> explained ("basename /usr/" yields "usr", so does "basename
> /usr///").

I didn't realize the difference between POSIX "basename" and GNU
"basename", and was mislead by the shell behavior. So this and the
following nitpick about "/" can be disregarded. But please add a
comment for the whitespace stripping.

Apart from that, ACK.

Martin

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

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

[PATCH 02/12] libmultipath: fix basenamecpy

[Benjamin Marzinski]

basenamecpy was returning the wrong answer in multiple cases, as
shown by the unit tests for it. Now it will properly find the
basename (as defined by GNU basename, which works well for all of
multipath's uses) and return a copy, if the basename can fit in
provided buffer.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/util.c | 25 ++++++++-----------------
 libmultipath/util.h |  2 +-
 2 files changed, 9 insertions(+), 18 deletions(-)

diff --git a/libmultipath/util.c b/libmultipath/util.c
index d3dd3eb..7251ad0 100644
--- a/libmultipath/util.c
+++ b/libmultipath/util.c
@@ -30,30 +30,21 @@ strchop(char *str)
 }
 
 int
-basenamecpy (const char * str1, char * str2, int str2len)
+basenamecpy (const char *src, char *dst, size_t size)
 {
-	const char *p;
+	const char *p, *e;
 
-	if (!str1 || !strlen(str1))
+	if (!src || !dst || !strlen(src))
 		return 0;
 
-	if (strlen(str1) >= str2len)
-		return 0;
+	p = basename(src);
 
-	if (!str2)
+	for (e = p + strlen(p) - 1; e >= p && isspace(*e); --e) ;
+	if (e < p || e - p > size - 2)
 		return 0;
 
-	p = str1 + (strlen(str1) - 1);
-
-	while (*--p != '/' && p != str1)
-		continue;
-
-	if (p != str1)
-		p++;
-
-	strncpy(str2, p, str2len);
-	str2[str2len - 1] = '\0';
-	return strchop(str2);
+	strlcpy(dst, p, e - p + 2);
+	return strlen(dst);
 }
 
 int
diff --git a/libmultipath/util.h b/libmultipath/util.h
index 51a6d54..a3ab894 100644
--- a/libmultipath/util.h
+++ b/libmultipath/util.h
@@ -5,7 +5,7 @@
 #include <inttypes.h>
 
 size_t strchop(char *);
-int basenamecpy (const char * src, char * dst, int);
+int basenamecpy (const char *src, char *dst, size_t size);
 int filepresent (char * run);
 char *get_next_string(char **temp, char *split_char);
 int get_word (char * sentence, char ** word);
-- 
2.7.4

[PATCH 03/12] libmultipath: set dm_conf_verbosity

[Benjamin Marzinski]

dm_conf_verbosity was created to keep dm_write_log from needing
access to the multipath config. However it never was set.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 767d87c..ef1fd2b 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -95,6 +95,7 @@ dm_write_log (int level, const char *file, int line, const char *f, ...)
 
 void dm_init(int v)
 {
+	dm_conf_verbosity = v;
 	dm_log_init(&dm_write_log);
 	dm_log_init_verbose(v + 3);
 }
-- 
2.7.4

Re: [PATCH 03/12] libmultipath: set dm_conf_verbosity

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> dm_conf_verbosity was created to keep dm_write_log from needing
> access to the multipath config. However it never was set.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

I'm curious how much verbosity this will add to multipathd output. But
ut's activated at v >= 4 only, so it should be fine.

Back in my head, I've been thinking about more fine-grained logging
control with log layer + log level for a while...

> ---
>  libmultipath/devmapper.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 767d87c..ef1fd2b 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -95,6 +95,7 @@ dm_write_log (int level, const char *file, int
> line, const char *f, ...)
>  
>  void dm_init(int v)
>  {
> +	dm_conf_verbosity = v;
>  	dm_log_init(&dm_write_log);
>  	dm_log_init_verbose(v + 3);
>  }

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

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

[PATCH 04/12] multipathd: log thread cleanup

[Benjamin Marzinski]

The function log_thread_flush() is an exact copy of flush_lgoqueue(), so
both clearly don't need to exist. Also, There is no reason to make all
of the log thread variables global.  The only time any of them were
being used outside of log_thread.c, was to reset the log.  This code
should never be run if the log_thread isn't running, so it makes more
sense to live inside of log_thread.c

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/log_pthread.c | 34 +++++++++++++++-------------------
 libmultipath/log_pthread.h | 10 +---------
 multipathd/main.c          |  5 ++---
 3 files changed, 18 insertions(+), 31 deletions(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index 035ae5d..f491386 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -14,13 +14,13 @@
 #include "log.h"
 #include "lock.h"
 
-pthread_t log_thr;
+static pthread_t log_thr;
 
-pthread_mutex_t logq_lock;
-pthread_mutex_t logev_lock;
-pthread_cond_t logev_cond;
+static pthread_mutex_t logq_lock;
+static pthread_mutex_t logev_lock;
+static pthread_cond_t logev_cond;
 
-int logq_running;
+static int logq_running;
 
 void log_safe (int prio, const char * fmt, va_list ap)
 {
@@ -38,19 +38,6 @@ void log_safe (int prio, const char * fmt, va_list ap)
 	pthread_mutex_unlock(&logev_lock);
 }
 
-void log_thread_flush (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);
-}
-
 static void flush_logqueue (void)
 {
 	int empty;
@@ -82,7 +69,7 @@ static void * log_thread (void * et)
 		pthread_mutex_unlock(&logev_lock);
 		if (!running)
 			break;
-		log_thread_flush();
+		flush_logqueue();
 	}
 	return NULL;
 }
@@ -107,6 +94,15 @@ void log_thread_start (pthread_attr_t *attr)
 	return;
 }
 
+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)
 {
 	logdbg(stderr,"enter log_thread_stop\n");
diff --git a/libmultipath/log_pthread.h b/libmultipath/log_pthread.h
index e5c6499..7e138a0 100644
--- a/libmultipath/log_pthread.h
+++ b/libmultipath/log_pthread.h
@@ -3,17 +3,9 @@
 
 #include <pthread.h>
 
-extern pthread_t log_thr;
-
-extern pthread_mutex_t logq_lock;
-extern pthread_mutex_t logev_lock;
-extern pthread_cond_t logev_cond;
-
-extern int logq_running;
-
 void log_safe(int prio, const char * fmt, va_list ap);
 void log_thread_start(pthread_attr_t *attr);
+void log_thread_reset (void);
 void log_thread_stop(void);
-void log_thread_flush(void);
 
 #endif /* _LOG_PTHREAD_H */
diff --git a/multipathd/main.c b/multipathd/main.c
index a03ba20..6ba6131 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2219,9 +2219,8 @@ handle_signals(bool nonfatal)
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
-		pthread_mutex_lock(&logq_lock);
-		log_reset("multipathd");
-		pthread_mutex_unlock(&logq_lock);
+		if (logsink == 1)
+			log_thread_reset();
 	}
 	reconfig_sig = 0;
 	log_reset_sig = 0;
-- 
2.7.4

Re: [PATCH 04/12] multipathd: log thread cleanup

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> The function log_thread_flush() is an exact copy of flush_lgoqueue(),
> so
> both clearly don't need to exist. Also, There is no reason to make
> all
> of the log thread variables global.  The only time any of them were
> being used outside of log_thread.c, was to reset the log.  This code
> should never be run if the log_thread isn't running, so it makes more
> sense to live inside of log_thread.c
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---

Reviewed-by: Martin Wilck <mwilck@suse.com>


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

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

[PATCH 05/12] libmultipath: fix log_pthread processing

[Benjamin Marzinski]

log_pthread() was waiting for notification on logev_cond, without
checking if it had already happened.  This means it could end up
waiting, while there is work that it should be doing.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/log_pthread.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/libmultipath/log_pthread.c b/libmultipath/log_pthread.c
index f491386..bb35dfc 100644
--- a/libmultipath/log_pthread.c
+++ b/libmultipath/log_pthread.c
@@ -21,6 +21,7 @@ static pthread_mutex_t logev_lock;
 static pthread_cond_t logev_cond;
 
 static int logq_running;
+static int log_messages_pending;
 
 void log_safe (int prio, const char * fmt, va_list ap)
 {
@@ -34,6 +35,7 @@ void log_safe (int prio, const char * fmt, va_list ap)
 	pthread_mutex_unlock(&logq_lock);
 
 	pthread_mutex_lock(&logev_lock);
+	log_messages_pending = 1;
 	pthread_cond_signal(&logev_cond);
 	pthread_mutex_unlock(&logev_lock);
 }
@@ -64,7 +66,9 @@ static void * log_thread (void * et)
 
 	while (1) {
 		pthread_mutex_lock(&logev_lock);
-		pthread_cond_wait(&logev_cond, &logev_lock);
+		if (logq_running && !log_messages_pending)
+			pthread_cond_wait(&logev_cond, &logev_lock);
+		log_messages_pending = 0;
 		running = logq_running;
 		pthread_mutex_unlock(&logev_lock);
 		if (!running)
-- 
2.7.4

Re: [PATCH 05/12] libmultipath: fix log_pthread processing

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> log_pthread() was waiting for notification on logev_cond, without
> checking if it had already happened.  This means it could end up
> waiting, while there is work that it should be doing.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

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

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

[PATCH 06/12] multipathd: use nanosleep for strict timing

[Benjamin Marzinski]

In order to safely use SIGALRM in a multi-threaded program, only one
thread can schedule and wait on SIGALRM at a time. All other threads
must have SIGALRM blocked, and be unable to schedule an alarm. The
strict_timing code in checkerloop was unblocking SIGALRM, and calling
setitimer(), without any locking.  Instead, it should use nanosleep()
to sleep for the correct length of time, since that doesn't depend or
interfere with signals.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 6ba6131..ce914ab 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1855,7 +1855,6 @@ checkerloop (void *ap)
 	struct path *pp;
 	int count = 0;
 	unsigned int i;
-	struct itimerval timer_tick_it;
 	struct timespec last_time;
 	struct config *conf;
 
@@ -1873,8 +1872,7 @@ checkerloop (void *ap)
 
 	while (1) {
 		struct timespec diff_time, start_time, end_time;
-		int num_paths = 0, ticks = 0, signo, strict_timing, rc = 0;
-		sigset_t mask;
+		int num_paths = 0, ticks = 0, strict_timing, rc = 0;
 
 		if (clock_gettime(CLOCK_MONOTONIC, &start_time) != 0)
 			start_time.tv_sec = 0;
@@ -1962,25 +1960,18 @@ checkerloop (void *ap)
 		if (!strict_timing)
 			sleep(1);
 		else {
-			timer_tick_it.it_interval.tv_sec = 0;
-			timer_tick_it.it_interval.tv_usec = 0;
 			if (diff_time.tv_nsec) {
-				timer_tick_it.it_value.tv_sec = 0;
-				timer_tick_it.it_value.tv_usec =
+				diff_time.tv_sec = 0;
+				diff_time.tv_nsec =
 				     1000UL * 1000 * 1000 - diff_time.tv_nsec;
-			} else {
-				timer_tick_it.it_value.tv_sec = 1;
-				timer_tick_it.it_value.tv_usec = 0;
-			}
-			setitimer(ITIMER_REAL, &timer_tick_it, NULL);
+			} else
+				diff_time.tv_sec = 1;
 
-			sigemptyset(&mask);
-			sigaddset(&mask, SIGALRM);
 			condlog(3, "waiting for %lu.%06lu secs",
-				timer_tick_it.it_value.tv_sec,
-				timer_tick_it.it_value.tv_usec);
-			if (sigwait(&mask, &signo) != 0) {
-				condlog(3, "sigwait failed with error %d",
+				diff_time.tv_sec,
+				diff_time.tv_nsec / 1000);
+			if (nanosleep(&diff_time, NULL) != 0) {
+				condlog(3, "nanosleep failed with error %d",
 					errno);
 				conf = get_multipath_config();
 				conf->strict_timing = 0;
-- 
2.7.4

Re: [PATCH 06/12] multipathd: use nanosleep for strict timing

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> In order to safely use SIGALRM in a multi-threaded program, only one
> thread can schedule and wait on SIGALRM at a time. All other threads
> must have SIGALRM blocked, and be unable to schedule an alarm. The
> strict_timing code in checkerloop was unblocking SIGALRM, and calling
> setitimer(), without any locking.  Instead, it should use nanosleep()
> to sleep for the correct length of time, since that doesn't depend or
> interfere with signals.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 6ba6131..ce914ab 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> 

>  			condlog(3, "waiting for %lu.%06lu secs",
> -				timer_tick_it.it_value.tv_sec,
> -				timer_tick_it.it_value.tv_usec);
> -			if (sigwait(&mask, &signo) != 0) {
> -				condlog(3, "sigwait failed with
> error %d",
> +				diff_time.tv_sec,
> +				diff_time.tv_nsec / 1000);
> +			if (nanosleep(&diff_time, NULL) != 0) {
> +				condlog(3, "nanosleep failed with
> error %d",
>  					errno);
>  				conf = get_multipath_config();
>  				conf->strict_timing = 0;

Nitpick: the only realistic error code for nanosleep is EINTR, in which
case we IMO don't need the log message, because just means one of the
expected signals arrived.

As stated earlier, I'd prefer a kernel interval timer for
strict_timing. I'm unsure why it hasn't been done that way in the first
place. Anyway, that can be discussed later, therefore:

Reviewed-by: Martin Wilck <mwilck@suse.com> 

... with the nit above.

Martin

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

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

[PATCH 07/12] libmultipath: move remove_map waiter code to multipathd

[Benjamin Marzinski]

Only multipathd needs to worry about the multipath waiter code. There is
no point in having remove_map_and_stop_waiter() or
remove_maps_and_stop_waiters() in libmultipath, since they should never
be use outside of multipathd.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 40 +++++-----------------------------------
 libmultipath/structs_vec.h |  2 --
 multipathd/main.c          | 23 +++++++++++++++++++++++
 3 files changed, 28 insertions(+), 37 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index f9dc8a8..41e5d88 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -117,25 +117,16 @@ set_multipath_wwid (struct multipath * mpp)
 	dm_get_uuid(mpp->alias, mpp->wwid);
 }
 
-#define KEEP_WAITER 0
-#define STOP_WAITER 1
 #define PURGE_VEC 1
 
-static void
-_remove_map (struct multipath * mpp, struct vectors * vecs,
-	    int stop_waiter, int purge_vec)
+void
+remove_map(struct multipath * mpp, struct vectors * vecs, int purge_vec)
 {
 	int i;
 
 	condlog(4, "%s: remove multipath map", mpp->alias);
 
 	/*
-	 * stop the DM event waiter thread
-	 */
-	if (stop_waiter)
-		stop_waiter_thread(mpp, vecs);
-
-	/*
 	 * clear references to this map
 	 */
 	orphan_paths(vecs->pathvec, mpp);
@@ -150,19 +141,8 @@ _remove_map (struct multipath * mpp, struct vectors * vecs,
 	free_multipath(mpp, KEEP_PATHS);
 }
 
-void remove_map(struct multipath *mpp, struct vectors *vecs, int purge_vec)
-{
-	_remove_map(mpp, vecs, KEEP_WAITER, purge_vec);
-}
-
-void remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs,
-				int purge_vec)
-{
-	_remove_map(mpp, vecs, STOP_WAITER, purge_vec);
-}
-
-static void
-_remove_maps (struct vectors * vecs, int stop_waiter)
+void
+remove_maps(struct vectors * vecs)
 {
 	int i;
 	struct multipath * mpp;
@@ -171,7 +151,7 @@ _remove_maps (struct vectors * vecs, int stop_waiter)
 		return;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i) {
-		_remove_map(mpp, vecs, stop_waiter, 1);
+		remove_map(mpp, vecs, 1);
 		i--;
 	}
 
@@ -179,16 +159,6 @@ _remove_maps (struct vectors * vecs, int stop_waiter)
 	vecs->mpvec = NULL;
 }
 
-void remove_maps(struct vectors *vecs)
-{
-	_remove_maps(vecs, KEEP_WAITER);
-}
-
-void remove_maps_and_stop_waiters(struct vectors *vecs)
-{
-	_remove_maps(vecs, STOP_WAITER);
-}
-
 void
 extract_hwe_from_path(struct multipath * mpp)
 {
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 3749eb6..2a0be93 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -27,9 +27,7 @@ int update_multipath_strings (struct multipath *mpp, vector pathvec,
 void extract_hwe_from_path(struct multipath * mpp);
 
 void remove_map (struct multipath * mpp, struct vectors * vecs, int purge_vec);
-void remove_map_and_stop_waiter (struct multipath * mpp, struct vectors * vecs, int purge_vec);
 void remove_maps (struct vectors * vecs);
-void remove_maps_and_stop_waiters (struct vectors * vecs);
 
 void sync_map_state (struct multipath *);
 int update_map (struct multipath *mpp, struct vectors *vecs);
diff --git a/multipathd/main.c b/multipathd/main.c
index ce914ab..32cd6fe 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -293,6 +293,29 @@ switch_pathgroup (struct multipath * mpp)
 		 mpp->alias, mpp->bestpg);
 }
 
+static void
+remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs,
+			   int purge_vec)
+{
+	stop_waiter_thread(mpp, vecs);
+	remove_map(mpp, vecs, purge_vec);
+}
+
+static void
+remove_maps_and_stop_waiters(struct vectors *vecs)
+{
+	int i;
+	struct multipath * mpp;
+
+	if (!vecs)
+		return;
+
+	vector_foreach_slot(vecs->mpvec, mpp, i)
+		stop_waiter_thread(mpp, vecs);
+
+	remove_maps(vecs);
+}
+
 static int
 coalesce_maps(struct vectors *vecs, vector nmpv)
 {
-- 
2.7.4

Re: [PATCH 07/12] libmultipath: move remove_map waiter code to multipathd

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> Only multipathd needs to worry about the multipath waiter code. There
> is
> no point in having remove_map_and_stop_waiter() or
> remove_maps_and_stop_waiters() in libmultipath, since they should
> never
> be use outside of multipathd.
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>



> +static void
> +remove_map_and_stop_waiter(struct multipath *mpp, struct vectors
> *vecs,
> +			   int purge_vec)
> +{
> +	stop_waiter_thread(mpp, vecs);
> +	remove_map(mpp, vecs, purge_vec);
> +}

AFAICS remove_map_and_stop_waiter is always called with purge_vec == 1,
so this could be further simplified. Could be a separate patch though.



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

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

[PATCH 08/12] move waiter code from libmultipath to multipathd

[Benjamin Marzinski]

Only multipathd uses the code in waiter.[ch] and the functions that call
it directly, so they should all live in the multipathd directory.  This
patch is simply moving the waiter.[ch] files and the functions in
structs_vec that use them. None of the moved code has been changed.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/Makefile      |   2 +-
 libmultipath/structs_vec.c |  98 ---------------------
 libmultipath/structs_vec.h |   4 +-
 libmultipath/waiter.c      | 215 ---------------------------------------------
 libmultipath/waiter.h      |  17 ----
 multipathd/Makefile        |   2 +-
 multipathd/main.c          |  96 ++++++++++++++++++++
 multipathd/waiter.c        | 215 +++++++++++++++++++++++++++++++++++++++++++++
 multipathd/waiter.h        |  17 ++++
 9 files changed, 332 insertions(+), 334 deletions(-)
 delete mode 100644 libmultipath/waiter.c
 delete mode 100644 libmultipath/waiter.h
 create mode 100644 multipathd/waiter.c
 create mode 100644 multipathd/waiter.h

diff --git a/libmultipath/Makefile b/libmultipath/Makefile
index 806aaa2..f51786d 100644
--- a/libmultipath/Makefile
+++ b/libmultipath/Makefile
@@ -42,7 +42,7 @@ OBJS = memory.o parser.o vector.o devmapper.o callout.o \
 	pgpolicies.o debug.o defaults.o uevent.o time-util.o \
 	switchgroup.o uxsock.o print.o alias.o log_pthread.o \
 	log.o configure.o structs_vec.o sysfs.o prio.o checkers.o \
-	lock.o waiter.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \
+	lock.o file.o wwids.o prioritizers/alua_rtpg.o prkey.o \
 	io_err_stat.o dm-generic.o generic.o foreign.o
 
 all: $(LIBS)
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 41e5d88..9a01631 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -10,7 +10,6 @@
 #include "structs.h"
 #include "structs_vec.h"
 #include "sysfs.h"
-#include "waiter.h"
 #include "devmapper.h"
 #include "dmparser.h"
 #include "propsel.h"
@@ -108,17 +107,6 @@ void orphan_paths(vector pathvec, struct multipath *mpp)
 	}
 }
 
-static void
-set_multipath_wwid (struct multipath * mpp)
-{
-	if (strlen(mpp->wwid))
-		return;
-
-	dm_get_uuid(mpp->alias, mpp->wwid);
-}
-
-#define PURGE_VEC 1
-
 void
 remove_map(struct multipath * mpp, struct vectors * vecs, int purge_vec)
 {
@@ -380,92 +368,6 @@ sync_map_state(struct multipath *mpp)
 	}
 }
 
-int
-update_map (struct multipath *mpp, struct vectors *vecs)
-{
-	int retries = 3;
-	char params[PARAMS_SIZE] = {0};
-
-retry:
-	condlog(4, "%s: updating new map", mpp->alias);
-	if (adopt_paths(vecs->pathvec, mpp)) {
-		condlog(0, "%s: failed to adopt paths for new map update",
-			mpp->alias);
-		retries = -1;
-		goto fail;
-	}
-	verify_paths(mpp, vecs);
-	mpp->action = ACT_RELOAD;
-
-	extract_hwe_from_path(mpp);
-	if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
-		condlog(0, "%s: failed to setup new map in update", mpp->alias);
-		retries = -1;
-		goto fail;
-	}
-	if (domap(mpp, params, 1) <= 0 && retries-- > 0) {
-		condlog(0, "%s: map_udate sleep", mpp->alias);
-		sleep(1);
-		goto retry;
-	}
-	dm_lib_release();
-
-fail:
-	if (setup_multipath(vecs, mpp))
-		return 1;
-
-	sync_map_state(mpp);
-
-	if (retries < 0)
-		condlog(0, "%s: failed reload in new map update", mpp->alias);
-	return 0;
-}
-
-struct multipath *add_map_without_path (struct vectors *vecs, const char *alias)
-{
-	struct multipath * mpp = alloc_multipath();
-	struct config *conf;
-
-	if (!mpp)
-		return NULL;
-	if (!alias) {
-		FREE(mpp);
-		return NULL;
-	}
-
-	mpp->alias = STRDUP(alias);
-
-	if (dm_get_info(mpp->alias, &mpp->dmi)) {
-		condlog(3, "%s: cannot access table", mpp->alias);
-		goto out;
-	}
-	set_multipath_wwid(mpp);
-	conf = get_multipath_config();
-	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
-	put_multipath_config(conf);
-
-	if (update_multipath_table(mpp, vecs->pathvec, 1))
-		goto out;
-	if (update_multipath_status(mpp))
-		goto out;
-
-	if (!vector_alloc_slot(vecs->mpvec))
-		goto out;
-
-	vector_set_slot(vecs->mpvec, mpp);
-
-	if (update_map(mpp, vecs) != 0) /* map removed */
-		return NULL;
-
-	if (start_waiter_thread(mpp, vecs))
-		goto out;
-
-	return mpp;
-out:
-	remove_map(mpp, vecs, PURGE_VEC);
-	return NULL;
-}
-
 static void
 find_existing_alias (struct multipath * mpp,
 		     struct vectors *vecs)
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index 2a0be93..ceab6d9 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -26,12 +26,12 @@ int update_multipath_strings (struct multipath *mpp, vector pathvec,
 			      int is_daemon);
 void extract_hwe_from_path(struct multipath * mpp);
 
+#define PURGE_VEC 1
+
 void remove_map (struct multipath * mpp, struct vectors * vecs, int purge_vec);
 void remove_maps (struct vectors * vecs);
 
 void sync_map_state (struct multipath *);
-int update_map (struct multipath *mpp, struct vectors *vecs);
-struct multipath * add_map_without_path (struct vectors * vecs, const char * alias);
 struct multipath * add_map_with_path (struct vectors * vecs,
 				struct path * pp, int add_vec);
 int update_multipath (struct vectors *vecs, char *mapname, int reset);
diff --git a/libmultipath/waiter.c b/libmultipath/waiter.c
deleted file mode 100644
index cb9708b..0000000
--- a/libmultipath/waiter.c
+++ /dev/null
@@ -1,215 +0,0 @@
-/*
- * Copyright (c) 2004, 2005 Christophe Varoqui
- * Copyright (c) 2005 Kiyoshi Ueda, NEC
- * Copyright (c) 2005 Benjamin Marzinski, Redhat
- * Copyright (c) 2005 Edward Goggin, EMC
- */
-#include <unistd.h>
-#include <libdevmapper.h>
-#include <sys/mman.h>
-#include <pthread.h>
-#include <signal.h>
-#include <urcu.h>
-
-#include "vector.h"
-#include "memory.h"
-#include "checkers.h"
-#include "config.h"
-#include "structs.h"
-#include "structs_vec.h"
-#include "devmapper.h"
-#include "debug.h"
-#include "lock.h"
-#include "waiter.h"
-
-pthread_attr_t waiter_attr;
-
-static struct event_thread *alloc_waiter (void)
-{
-
-	struct event_thread *wp;
-
-	wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
-	memset(wp, 0, sizeof(struct event_thread));
-
-	return wp;
-}
-
-static void free_waiter (void *data)
-{
-	struct event_thread *wp = (struct event_thread *)data;
-
-	if (wp->dmt)
-		dm_task_destroy(wp->dmt);
-
-	rcu_unregister_thread();
-	FREE(wp);
-}
-
-void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
-{
-	pthread_t thread;
-
-	if (mpp->waiter == (pthread_t)0) {
-		condlog(3, "%s: event checker thread already stopped",
-			mpp->alias);
-		return;
-	}
-	condlog(2, "%s: stop event checker thread (%lu)", mpp->alias,
-		mpp->waiter);
-	thread = mpp->waiter;
-	mpp->waiter = (pthread_t)0;
-	pthread_cancel(thread);
-	pthread_kill(thread, SIGUSR2);
-}
-
-/*
- * returns the reschedule delay
- * negative means *stop*
- */
-static int waiteventloop (struct event_thread *waiter)
-{
-	sigset_t set, oldset;
-	int event_nr;
-	int r;
-
-	if (!waiter->event_nr)
-		waiter->event_nr = dm_geteventnr(waiter->mapname);
-
-	if (!(waiter->dmt = libmp_dm_task_create(DM_DEVICE_WAITEVENT))) {
-		condlog(0, "%s: devmap event #%i dm_task_create error",
-				waiter->mapname, waiter->event_nr);
-		return 1;
-	}
-
-	if (!dm_task_set_name(waiter->dmt, waiter->mapname)) {
-		condlog(0, "%s: devmap event #%i dm_task_set_name error",
-				waiter->mapname, waiter->event_nr);
-		dm_task_destroy(waiter->dmt);
-		waiter->dmt = NULL;
-		return 1;
-	}
-
-	if (waiter->event_nr && !dm_task_set_event_nr(waiter->dmt,
-						      waiter->event_nr)) {
-		condlog(0, "%s: devmap event #%i dm_task_set_event_nr error",
-				waiter->mapname, waiter->event_nr);
-		dm_task_destroy(waiter->dmt);
-		waiter->dmt = NULL;
-		return 1;
-	}
-
-	dm_task_no_open_count(waiter->dmt);
-
-	/* wait */
-	sigemptyset(&set);
-	sigaddset(&set, SIGUSR2);
-	pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
-
-	pthread_testcancel();
-	r = dm_task_run(waiter->dmt);
-	pthread_testcancel();
-
-	pthread_sigmask(SIG_SETMASK, &oldset, NULL);
-	dm_task_destroy(waiter->dmt);
-	waiter->dmt = NULL;
-
-	if (!r)	/* wait interrupted by signal */
-		return -1;
-
-	waiter->event_nr++;
-
-	/*
-	 * upon event ...
-	 */
-	while (1) {
-		condlog(3, "%s: devmap event #%i",
-				waiter->mapname, waiter->event_nr);
-
-		/*
-		 * event might be :
-		 *
-		 * 1) a table reload, which means our mpp structure is
-		 *    obsolete : refresh it through update_multipath()
-		 * 2) a path failed by DM : mark as such through
-		 *    update_multipath()
-		 * 3) map has gone away : stop the thread.
-		 * 4) a path reinstate : nothing to do
-		 * 5) a switch group : nothing to do
-		 */
-		pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
-		lock(&waiter->vecs->lock);
-		pthread_testcancel();
-		r = update_multipath(waiter->vecs, waiter->mapname, 1);
-		lock_cleanup_pop(waiter->vecs->lock);
-
-		if (r) {
-			condlog(2, "%s: event checker exit",
-				waiter->mapname);
-			return -1; /* stop the thread */
-		}
-
-		event_nr = dm_geteventnr(waiter->mapname);
-
-		if (waiter->event_nr == event_nr)
-			return 1; /* upon problem reschedule 1s later */
-
-		waiter->event_nr = event_nr;
-	}
-	return -1; /* never reach there */
-}
-
-static void *waitevent (void *et)
-{
-	int r;
-	struct event_thread *waiter;
-
-	mlockall(MCL_CURRENT | MCL_FUTURE);
-
-	waiter = (struct event_thread *)et;
-	pthread_cleanup_push(free_waiter, et);
-
-	rcu_register_thread();
-	while (1) {
-		r = waiteventloop(waiter);
-
-		if (r < 0)
-			break;
-
-		sleep(r);
-	}
-
-	pthread_cleanup_pop(1);
-	return NULL;
-}
-
-int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
-{
-	struct event_thread *wp;
-
-	if (!mpp)
-		return 0;
-
-	wp = alloc_waiter();
-
-	if (!wp)
-		goto out;
-
-	strncpy(wp->mapname, mpp->alias, WWID_SIZE - 1);
-	wp->vecs = vecs;
-
-	if (pthread_create(&wp->thread, &waiter_attr, waitevent, wp)) {
-		condlog(0, "%s: cannot create event checker", wp->mapname);
-		goto out1;
-	}
-	mpp->waiter = wp->thread;
-	condlog(2, "%s: event checker started", wp->mapname);
-
-	return 0;
-out1:
-	free_waiter(wp);
-	mpp->waiter = (pthread_t)0;
-out:
-	condlog(0, "failed to start waiter thread");
-	return 1;
-}
diff --git a/libmultipath/waiter.h b/libmultipath/waiter.h
deleted file mode 100644
index 0cfae46..0000000
--- a/libmultipath/waiter.h
+++ /dev/null
@@ -1,17 +0,0 @@
-#ifndef _WAITER_H
-#define _WAITER_H
-
-extern pthread_attr_t waiter_attr;
-
-struct event_thread {
-	struct dm_task *dmt;
-	pthread_t thread;
-	int event_nr;
-	char mapname[WWID_SIZE];
-	struct vectors *vecs;
-};
-
-void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs);
-int start_waiter_thread (struct multipath *mpp, struct vectors *vecs);
-
-#endif /* _WAITER_H */
diff --git a/multipathd/Makefile b/multipathd/Makefile
index 4c9d296..7800e47 100644
--- a/multipathd/Makefile
+++ b/multipathd/Makefile
@@ -22,7 +22,7 @@ ifdef SYSTEMD
 	endif
 endif
 
-OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o
+OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o waiter.o
 
 EXEC = multipathd
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 32cd6fe..2924d53 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -316,6 +316,102 @@ remove_maps_and_stop_waiters(struct vectors *vecs)
 	remove_maps(vecs);
 }
 
+static void
+set_multipath_wwid (struct multipath * mpp)
+{
+	if (strlen(mpp->wwid))
+		return;
+
+	dm_get_uuid(mpp->alias, mpp->wwid);
+}
+
+static int
+update_map (struct multipath *mpp, struct vectors *vecs)
+{
+	int retries = 3;
+	char params[PARAMS_SIZE] = {0};
+
+retry:
+	condlog(4, "%s: updating new map", mpp->alias);
+	if (adopt_paths(vecs->pathvec, mpp)) {
+		condlog(0, "%s: failed to adopt paths for new map update",
+			mpp->alias);
+		retries = -1;
+		goto fail;
+	}
+	verify_paths(mpp, vecs);
+	mpp->action = ACT_RELOAD;
+
+	extract_hwe_from_path(mpp);
+	if (setup_map(mpp, params, PARAMS_SIZE, vecs)) {
+		condlog(0, "%s: failed to setup new map in update", mpp->alias);
+		retries = -1;
+		goto fail;
+	}
+	if (domap(mpp, params, 1) <= 0 && retries-- > 0) {
+		condlog(0, "%s: map_udate sleep", mpp->alias);
+		sleep(1);
+		goto retry;
+	}
+	dm_lib_release();
+
+fail:
+	if (setup_multipath(vecs, mpp))
+		return 1;
+
+	sync_map_state(mpp);
+
+	if (retries < 0)
+		condlog(0, "%s: failed reload in new map update", mpp->alias);
+	return 0;
+}
+
+static struct multipath *
+add_map_without_path (struct vectors *vecs, const char *alias)
+{
+	struct multipath * mpp = alloc_multipath();
+	struct config *conf;
+
+	if (!mpp)
+		return NULL;
+	if (!alias) {
+		FREE(mpp);
+		return NULL;
+	}
+
+	mpp->alias = STRDUP(alias);
+
+	if (dm_get_info(mpp->alias, &mpp->dmi)) {
+		condlog(3, "%s: cannot access table", mpp->alias);
+		goto out;
+	}
+	set_multipath_wwid(mpp);
+	conf = get_multipath_config();
+	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
+	put_multipath_config(conf);
+
+	if (update_multipath_table(mpp, vecs->pathvec, 1))
+		goto out;
+	if (update_multipath_status(mpp))
+		goto out;
+
+	if (!vector_alloc_slot(vecs->mpvec))
+		goto out;
+
+	vector_set_slot(vecs->mpvec, mpp);
+
+	if (update_map(mpp, vecs) != 0) /* map removed */
+		return NULL;
+
+	if (start_waiter_thread(mpp, vecs))
+		goto out;
+
+	return mpp;
+out:
+	remove_map(mpp, vecs, PURGE_VEC);
+	return NULL;
+}
+
 static int
 coalesce_maps(struct vectors *vecs, vector nmpv)
 {
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
new file mode 100644
index 0000000..cb9708b
--- /dev/null
+++ b/multipathd/waiter.c
@@ -0,0 +1,215 @@
+/*
+ * Copyright (c) 2004, 2005 Christophe Varoqui
+ * Copyright (c) 2005 Kiyoshi Ueda, NEC
+ * Copyright (c) 2005 Benjamin Marzinski, Redhat
+ * Copyright (c) 2005 Edward Goggin, EMC
+ */
+#include <unistd.h>
+#include <libdevmapper.h>
+#include <sys/mman.h>
+#include <pthread.h>
+#include <signal.h>
+#include <urcu.h>
+
+#include "vector.h"
+#include "memory.h"
+#include "checkers.h"
+#include "config.h"
+#include "structs.h"
+#include "structs_vec.h"
+#include "devmapper.h"
+#include "debug.h"
+#include "lock.h"
+#include "waiter.h"
+
+pthread_attr_t waiter_attr;
+
+static struct event_thread *alloc_waiter (void)
+{
+
+	struct event_thread *wp;
+
+	wp = (struct event_thread *)MALLOC(sizeof(struct event_thread));
+	memset(wp, 0, sizeof(struct event_thread));
+
+	return wp;
+}
+
+static void free_waiter (void *data)
+{
+	struct event_thread *wp = (struct event_thread *)data;
+
+	if (wp->dmt)
+		dm_task_destroy(wp->dmt);
+
+	rcu_unregister_thread();
+	FREE(wp);
+}
+
+void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs)
+{
+	pthread_t thread;
+
+	if (mpp->waiter == (pthread_t)0) {
+		condlog(3, "%s: event checker thread already stopped",
+			mpp->alias);
+		return;
+	}
+	condlog(2, "%s: stop event checker thread (%lu)", mpp->alias,
+		mpp->waiter);
+	thread = mpp->waiter;
+	mpp->waiter = (pthread_t)0;
+	pthread_cancel(thread);
+	pthread_kill(thread, SIGUSR2);
+}
+
+/*
+ * returns the reschedule delay
+ * negative means *stop*
+ */
+static int waiteventloop (struct event_thread *waiter)
+{
+	sigset_t set, oldset;
+	int event_nr;
+	int r;
+
+	if (!waiter->event_nr)
+		waiter->event_nr = dm_geteventnr(waiter->mapname);
+
+	if (!(waiter->dmt = libmp_dm_task_create(DM_DEVICE_WAITEVENT))) {
+		condlog(0, "%s: devmap event #%i dm_task_create error",
+				waiter->mapname, waiter->event_nr);
+		return 1;
+	}
+
+	if (!dm_task_set_name(waiter->dmt, waiter->mapname)) {
+		condlog(0, "%s: devmap event #%i dm_task_set_name error",
+				waiter->mapname, waiter->event_nr);
+		dm_task_destroy(waiter->dmt);
+		waiter->dmt = NULL;
+		return 1;
+	}
+
+	if (waiter->event_nr && !dm_task_set_event_nr(waiter->dmt,
+						      waiter->event_nr)) {
+		condlog(0, "%s: devmap event #%i dm_task_set_event_nr error",
+				waiter->mapname, waiter->event_nr);
+		dm_task_destroy(waiter->dmt);
+		waiter->dmt = NULL;
+		return 1;
+	}
+
+	dm_task_no_open_count(waiter->dmt);
+
+	/* wait */
+	sigemptyset(&set);
+	sigaddset(&set, SIGUSR2);
+	pthread_sigmask(SIG_UNBLOCK, &set, &oldset);
+
+	pthread_testcancel();
+	r = dm_task_run(waiter->dmt);
+	pthread_testcancel();
+
+	pthread_sigmask(SIG_SETMASK, &oldset, NULL);
+	dm_task_destroy(waiter->dmt);
+	waiter->dmt = NULL;
+
+	if (!r)	/* wait interrupted by signal */
+		return -1;
+
+	waiter->event_nr++;
+
+	/*
+	 * upon event ...
+	 */
+	while (1) {
+		condlog(3, "%s: devmap event #%i",
+				waiter->mapname, waiter->event_nr);
+
+		/*
+		 * event might be :
+		 *
+		 * 1) a table reload, which means our mpp structure is
+		 *    obsolete : refresh it through update_multipath()
+		 * 2) a path failed by DM : mark as such through
+		 *    update_multipath()
+		 * 3) map has gone away : stop the thread.
+		 * 4) a path reinstate : nothing to do
+		 * 5) a switch group : nothing to do
+		 */
+		pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
+		lock(&waiter->vecs->lock);
+		pthread_testcancel();
+		r = update_multipath(waiter->vecs, waiter->mapname, 1);
+		lock_cleanup_pop(waiter->vecs->lock);
+
+		if (r) {
+			condlog(2, "%s: event checker exit",
+				waiter->mapname);
+			return -1; /* stop the thread */
+		}
+
+		event_nr = dm_geteventnr(waiter->mapname);
+
+		if (waiter->event_nr == event_nr)
+			return 1; /* upon problem reschedule 1s later */
+
+		waiter->event_nr = event_nr;
+	}
+	return -1; /* never reach there */
+}
+
+static void *waitevent (void *et)
+{
+	int r;
+	struct event_thread *waiter;
+
+	mlockall(MCL_CURRENT | MCL_FUTURE);
+
+	waiter = (struct event_thread *)et;
+	pthread_cleanup_push(free_waiter, et);
+
+	rcu_register_thread();
+	while (1) {
+		r = waiteventloop(waiter);
+
+		if (r < 0)
+			break;
+
+		sleep(r);
+	}
+
+	pthread_cleanup_pop(1);
+	return NULL;
+}
+
+int start_waiter_thread (struct multipath *mpp, struct vectors *vecs)
+{
+	struct event_thread *wp;
+
+	if (!mpp)
+		return 0;
+
+	wp = alloc_waiter();
+
+	if (!wp)
+		goto out;
+
+	strncpy(wp->mapname, mpp->alias, WWID_SIZE - 1);
+	wp->vecs = vecs;
+
+	if (pthread_create(&wp->thread, &waiter_attr, waitevent, wp)) {
+		condlog(0, "%s: cannot create event checker", wp->mapname);
+		goto out1;
+	}
+	mpp->waiter = wp->thread;
+	condlog(2, "%s: event checker started", wp->mapname);
+
+	return 0;
+out1:
+	free_waiter(wp);
+	mpp->waiter = (pthread_t)0;
+out:
+	condlog(0, "failed to start waiter thread");
+	return 1;
+}
diff --git a/multipathd/waiter.h b/multipathd/waiter.h
new file mode 100644
index 0000000..0cfae46
--- /dev/null
+++ b/multipathd/waiter.h
@@ -0,0 +1,17 @@
+#ifndef _WAITER_H
+#define _WAITER_H
+
+extern pthread_attr_t waiter_attr;
+
+struct event_thread {
+	struct dm_task *dmt;
+	pthread_t thread;
+	int event_nr;
+	char mapname[WWID_SIZE];
+	struct vectors *vecs;
+};
+
+void stop_waiter_thread (struct multipath *mpp, struct vectors *vecs);
+int start_waiter_thread (struct multipath *mpp, struct vectors *vecs);
+
+#endif /* _WAITER_H */
-- 
2.7.4

[PATCH 09/12] call start_waiter_thread() before setup_multipath()

[Benjamin Marzinski]

If setup_multipath() is called before the waiter thread has started,
there is a window where a dm event can occur between when
setup_multipath() updates the device state and when the waiter thread
starts waiting for new events, causing the new event to be missed and
the multipath device to not get updated.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 2924d53..f116c74 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -326,7 +326,7 @@ set_multipath_wwid (struct multipath * mpp)
 }
 
 static int
-update_map (struct multipath *mpp, struct vectors *vecs)
+update_map (struct multipath *mpp, struct vectors *vecs, int new_map)
 {
 	int retries = 3;
 	char params[PARAMS_SIZE] = {0};
@@ -356,6 +356,12 @@ retry:
 	dm_lib_release();
 
 fail:
+	if (new_map && (retries < 0 || start_waiter_thread(mpp, vecs))) {
+		condlog(0, "%s: failed to create new map", mpp->alias);
+		remove_map(mpp, vecs, 1);
+		return 1;
+	}
+
 	if (setup_multipath(vecs, mpp))
 		return 1;
 
@@ -400,12 +406,9 @@ add_map_without_path (struct vectors *vecs, const char *alias)
 
 	vector_set_slot(vecs->mpvec, mpp);
 
-	if (update_map(mpp, vecs) != 0) /* map removed */
+	if (update_map(mpp, vecs, 1) != 0) /* map removed */
 		return NULL;
 
-	if (start_waiter_thread(mpp, vecs))
-		goto out;
-
 	return mpp;
 out:
 	remove_map(mpp, vecs, PURGE_VEC);
@@ -559,7 +562,7 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 		if (mpp->wait_for_udev > 1) {
 			condlog(2, "%s: performing delayed actions",
 				mpp->alias);
-			if (update_map(mpp, vecs))
+			if (update_map(mpp, vecs, 0))
 				/* setup multipathd removed the map */
 				return 1;
 		}
@@ -870,6 +873,11 @@ retry:
 	}
 	dm_lib_release();
 
+	if ((mpp->action == ACT_CREATE ||
+	     (mpp->action == ACT_NOTHING && start_waiter && !mpp->waiter)) &&
+	    start_waiter_thread(mpp, vecs))
+			goto fail_map;
+
 	/*
 	 * update our state from kernel regardless of create or reload
 	 */
@@ -878,11 +886,6 @@ retry:
 
 	sync_map_state(mpp);
 
-	if ((mpp->action == ACT_CREATE ||
-	     (mpp->action == ACT_NOTHING && start_waiter && !mpp->waiter)) &&
-	    start_waiter_thread(mpp, vecs))
-			goto fail_map;
-
 	if (retries >= 0) {
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
@@ -1518,7 +1521,8 @@ missing_uev_wait_tick(struct vectors *vecs)
 		if (mpp->wait_for_udev && --mpp->uev_wait_tick <= 0) {
 			timed_out = 1;
 			condlog(0, "%s: timeout waiting on creation uevent. enabling reloads", mpp->alias);
-			if (mpp->wait_for_udev > 1 && update_map(mpp, vecs)) {
+			if (mpp->wait_for_udev > 1 &&
+			    update_map(mpp, vecs, 0)) {
 				/* update_map removed map */
 				i--;
 				continue;
@@ -1550,7 +1554,7 @@ ghost_delay_tick(struct vectors *vecs)
 			condlog(0, "%s: timed out waiting for active path",
 				mpp->alias);
 			mpp->force_udev_reload = 1;
-			if (update_map(mpp, vecs) != 0) {
+			if (update_map(mpp, vecs, 0) != 0) {
 				/* update_map removed map */
 				i--;
 				continue;
@@ -2199,14 +2203,13 @@ configure (struct vectors * vecs)
 	 * start dm event waiter threads for these new maps
 	 */
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (setup_multipath(vecs, mpp)) {
-			i--;
-			continue;
-		}
 		if (start_waiter_thread(mpp, vecs)) {
 			remove_map(mpp, vecs, 1);
 			i--;
+			continue;
 		}
+		if (setup_multipath(vecs, mpp))
+			i--;
 	}
 	return 0;
 }
-- 
2.7.4

[PATCH 10/12] libmultipath: add helper functions

[Benjamin Marzinski]

Add the ability to reset a vector without completely freeing it, and to
check the version of the device-mapper module.  The existing version
checking code checks the version of a specific device mapper target, and
has been renamed for clarity's sake. These functions will be used in a
later patch.

Reviewed-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/devmapper.c | 28 ++++++++++++++++++++++++----
 libmultipath/devmapper.h |  3 ++-
 libmultipath/vector.c    | 16 ++++++++++++----
 libmultipath/vector.h    |  1 +
 multipathd/main.c        |  2 +-
 5 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index ef1fd2b..9bafbc6 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -130,7 +130,27 @@ dm_lib_prereq (void)
 }
 
 int
-dm_drv_version (unsigned int * version, char * str)
+dm_drv_version(unsigned int *v)
+{
+	char buff[64];
+
+	v[0] = 0;
+	v[1] = 0;
+	v[2] = 0;
+
+	if (!dm_driver_version(buff, sizeof(buff))) {
+		condlog(0, "cannot get kernel dm version");
+		return 1;
+	}
+	if (sscanf(buff, "%u.%u.%u ", &v[0], &v[1], &v[2]) != 3) {
+		condlog(0, "invalid kernel dm version '%s'", buff);
+		return 1;
+	}
+	return 0;
+}
+
+int
+dm_tgt_version (unsigned int * version, char * str)
 {
 	int r = 2;
 	struct dm_task *dmt;
@@ -177,13 +197,13 @@ out:
 }
 
 static int
-dm_drv_prereq (unsigned int *ver)
+dm_tgt_prereq (unsigned int *ver)
 {
 	unsigned int minv[3] = {1, 0, 3};
 	unsigned int version[3] = {0, 0, 0};
 	unsigned int * v = version;
 
-	if (dm_drv_version(v, TGT_MPATH)) {
+	if (dm_tgt_version(v, TGT_MPATH)) {
 		/* in doubt return not capable */
 		return 1;
 	}
@@ -208,7 +228,7 @@ static int dm_prereq(unsigned int *v)
 {
 	if (dm_lib_prereq())
 		return 1;
-	return dm_drv_prereq(v);
+	return dm_tgt_prereq(v);
 }
 
 static int libmp_dm_udev_sync = 0;
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 8c8ea6c..db75526 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -31,7 +31,8 @@ void dm_init(int verbosity);
 void libmp_dm_init(void);
 void libmp_udev_set_sync_support(int on);
 struct dm_task *libmp_dm_task_create(int task);
-int dm_drv_version (unsigned int * version, char * str);
+int dm_drv_version (unsigned int * version);
+int dm_tgt_version (unsigned int * version, char * str);
 int dm_simplecmd_flush (int, const char *, uint16_t);
 int dm_simplecmd_noflush (int, const char *, uint16_t);
 int dm_addmap_create (struct multipath *mpp, char *params);
diff --git a/libmultipath/vector.c b/libmultipath/vector.c
index 6266e0a..f741ae0 100644
--- a/libmultipath/vector.c
+++ b/libmultipath/vector.c
@@ -145,18 +145,26 @@ vector_repack(vector v)
 			vector_del_slot(v, i--);
 }
 
-/* Free memory vector allocation */
-void
-vector_free(vector v)
+vector
+vector_reset(vector v)
 {
 	if (!v)
-		return;
+		return NULL;
 
 	if (v->slot)
 		FREE(v->slot);
 
 	v->allocated = 0;
 	v->slot = NULL;
+	return v;
+}
+
+/* Free memory vector allocation */
+void
+vector_free(vector v)
+{
+	if (!vector_reset(v))
+		return;
 	FREE(v);
 }
 
diff --git a/libmultipath/vector.h b/libmultipath/vector.h
index 6018b57..b9450ac 100644
--- a/libmultipath/vector.h
+++ b/libmultipath/vector.h
@@ -74,6 +74,7 @@ typedef struct _vector *vector;
 /* Prototypes */
 extern vector vector_alloc(void);
 extern void *vector_alloc_slot(vector v);
+vector vector_reset(vector v);
 extern void vector_free(vector v);
 #define vector_free_const(x) vector_free((vector)(long)(x))
 extern void free_strvec(vector strvec);
diff --git a/multipathd/main.c b/multipathd/main.c
index f116c74..aa86785 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2259,7 +2259,7 @@ reconfigure (struct vectors * vecs)
 	/* Re-read any timezone changes */
 	tzset();
 
-	dm_drv_version(conf->version, TGT_MPATH);
+	dm_tgt_version(conf->version, TGT_MPATH);
 	if (verbosity)
 		conf->verbosity = verbosity;
 	if (bindings_read_only)
-- 
2.7.4

[PATCH 11/12] multipathd: add new polling dmevents waiter thread

[Benjamin Marzinski]

The current method of waiting for dmevents on multipath devices involves
creating a seperate thread for each device. This can become very
wasteful when there are large numbers of multipath devices. Also, since
multipathd needs to grab the vecs lock to update the devices, the
additional threads don't actually provide much parallelism.

The patch adds a new method of updating multipath devices on dmevents,
which uses the new device-mapper event polling interface. This means
that there is only one dmevent waiting thread which will wait for events
on all of the multipath devices.  Currently the code to get the event
number from the list of device names and to re-arm the polling interface
is not in libdevmapper, so the patch does that work. Obviously, these
bits need to go into libdevmapper, so that multipathd can use a standard
interface.

I haven't touched any of the existing event waiting code, since event
polling was only added to device-mapper in version 4.37.0.  multipathd
checks this version, and defaults to using the polling code if
device-mapper supports it. This can be overridden by running multipathd
with "-w", to force it to use the old event waiting code.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 Makefile.inc               |   3 +
 libmultipath/structs_vec.c |   8 +
 libmultipath/structs_vec.h |   2 +
 multipathd/Makefile        |   6 +-
 multipathd/dmevents.c      | 392 +++++++++++++++++++++++++++++++++++++++++++++
 multipathd/dmevents.h      |  13 ++
 multipathd/main.c          |  62 ++++++-
 7 files changed, 477 insertions(+), 9 deletions(-)
 create mode 100644 multipathd/dmevents.c
 create mode 100644 multipathd/dmevents.h

diff --git a/Makefile.inc b/Makefile.inc
index 5d6123d..57a1835 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -14,6 +14,9 @@
 #
 # Uncomment to disable libdmmp support
 # ENABLE_LIBDMMP = 0
+#
+# Uncomment to disable dmevents polling support
+# ENABLE_DMEVENTS_POLL = 0
 
 ifeq ($(TOPDIR),)
 	TOPDIR	= ..
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 9a01631..e9a0274 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -130,6 +130,14 @@ remove_map(struct multipath * mpp, struct vectors * vecs, int purge_vec)
 }
 
 void
+remove_map_by_alias(const char *alias, struct vectors * vecs, int purge_vec)
+{
+	struct multipath * mpp = find_mp_by_alias(vecs->mpvec, alias);
+	if (mpp)
+		remove_map(mpp, vecs, purge_vec);
+}
+
+void
 remove_maps(struct vectors * vecs)
 {
 	int i;
diff --git a/libmultipath/structs_vec.h b/libmultipath/structs_vec.h
index ceab6d9..0adba17 100644
--- a/libmultipath/structs_vec.h
+++ b/libmultipath/structs_vec.h
@@ -29,6 +29,8 @@ void extract_hwe_from_path(struct multipath * mpp);
 #define PURGE_VEC 1
 
 void remove_map (struct multipath * mpp, struct vectors * vecs, int purge_vec);
+void remove_map_by_alias(const char *alias, struct vectors * vecs,
+			 int purge_vec);
 void remove_maps (struct vectors * vecs);
 
 void sync_map_state (struct multipath *);
diff --git a/multipathd/Makefile b/multipathd/Makefile
index 7800e47..d1a9863 100644
--- a/multipathd/Makefile
+++ b/multipathd/Makefile
@@ -21,8 +21,12 @@ ifdef SYSTEMD
 		LIBDEPS += -lsystemd-daemon
 	endif
 endif
+ifeq ($(ENABLE_DMEVENTS_POLL),0)
+	CFLAGS += -DNO_DMEVENTS_POLL
+endif
 
-OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o waiter.o
+OBJS = main.o pidfile.o uxlsnr.o uxclnt.o cli.o cli_handlers.o waiter.o \
+       dmevents.o
 
 EXEC = multipathd
 
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
new file mode 100644
index 0000000..1ef811e
--- /dev/null
+++ b/multipathd/dmevents.c
@@ -0,0 +1,392 @@
+/*
+ * Copyright (c) 2004, 2005 Christophe Varoqui
+ * Copyright (c) 2005 Kiyoshi Ueda, NEC
+ * Copyright (c) 2005 Edward Goggin, EMC
+ * Copyright (c) 2005, 2018 Benjamin Marzinski, Redhat
+ */
+#include <unistd.h>
+#include <libdevmapper.h>
+#include <sys/mman.h>
+#include <pthread.h>
+#include <urcu.h>
+#include <poll.h>
+#include <sys/ioctl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <linux/dm-ioctl.h>
+#include <errno.h>
+
+#include "vector.h"
+#include "structs.h"
+#include "structs_vec.h"
+#include "devmapper.h"
+#include "debug.h"
+#include "dmevents.h"
+
+#ifndef DM_DEV_ARM_POLL
+#define DM_DEV_ARM_POLL _IOWR(DM_IOCTL, DM_DEV_SET_GEOMETRY_CMD + 1, struct dm_ioctl)
+#endif
+
+enum event_actions {
+	EVENT_NOTHING,
+	EVENT_REMOVE,
+	EVENT_UPDATE,
+};
+
+struct dev_event {
+	char name[WWID_SIZE];
+	uint32_t evt_nr;
+	enum event_actions action;
+};
+
+struct dmevent_waiter {
+	int fd;
+	struct vectors *vecs;
+	vector events;
+	pthread_mutex_t events_lock;
+};
+
+static struct dmevent_waiter *waiter;
+
+int dmevent_poll_supported(void)
+{
+	unsigned int minv[3] = {4, 37, 0};
+	unsigned int v[3];
+
+	if (dm_drv_version(v))
+		return 0;
+
+	if (VERSION_GE(v, minv))
+		return 1;
+	return 0;
+}
+
+
+int init_dmevent_waiter(struct vectors *vecs)
+{
+	if (!vecs) {
+		condlog(0, "can't create waiter structure. invalid vectors");
+		goto fail;
+	}
+	waiter = (struct dmevent_waiter *)malloc(sizeof(struct dmevent_waiter));
+	if (!waiter) {
+		condlog(0, "failed to allocate waiter structure");
+		goto fail;
+	}
+	memset(waiter, 0, sizeof(struct dmevent_waiter));
+	waiter->events = vector_alloc();
+	if (!waiter->events) {
+		condlog(0, "failed to allocate waiter events vector");
+		goto fail_waiter;
+	}
+	waiter->fd = open("/dev/mapper/control", O_RDWR);
+	if (waiter->fd < 0) {
+		condlog(0, "failed to open /dev/mapper/control for waiter");
+		goto fail_events;
+	}
+	pthread_mutex_init(&waiter->events_lock, NULL);
+	waiter->vecs = vecs;
+
+	return 0;
+fail_events:
+	vector_free(waiter->events);
+fail_waiter:
+	free(waiter);
+fail:
+	waiter = NULL;
+	return -1;
+}
+
+void cleanup_dmevent_waiter(void)
+{
+	struct dev_event *dev_evt;
+	int i;
+
+	if (!waiter)
+		return;
+	pthread_mutex_destroy(&waiter->events_lock);
+	close(waiter->fd);
+	vector_foreach_slot(waiter->events, dev_evt, i)
+		free(dev_evt);
+	vector_free(waiter->events);
+	free(waiter);
+	waiter = NULL;
+}
+
+static int arm_dm_event_poll(int fd)
+{
+	struct dm_ioctl dmi;
+	memset(&dmi, 0, sizeof(dmi));
+	dmi.version[0] = DM_VERSION_MAJOR;
+	dmi.version[1] = DM_VERSION_MINOR;
+	dmi.version[2] = DM_VERSION_PATCHLEVEL;
+	dmi.flags = 0x4;
+	dmi.data_start = offsetof(struct dm_ioctl, data);
+	dmi.data_size = sizeof(dmi);
+	return ioctl(fd, DM_DEV_ARM_POLL, &dmi);
+}
+
+/*
+ * As of version 4.37.0 device-mapper stores the event number in the
+ * dm_names structure after the name, when DM_DEVICE_LIST is called
+ */
+static uint32_t dm_event_nr(struct dm_names *n)
+{
+	return *(uint32_t *)(((uintptr_t)(strchr(n->name, 0) + 1) + 7) & ~7);
+}
+
+static int dm_get_events(void)
+{
+	struct dm_task *dmt;
+	struct dm_names *names;
+	struct dev_event *dev_evt;
+	int i;
+
+	if (!(dmt = libmp_dm_task_create(DM_DEVICE_LIST)))
+		return -1;
+
+	dm_task_no_open_count(dmt);
+
+	if (!dm_task_run(dmt))
+		goto fail;
+
+	if (!(names = dm_task_get_names(dmt)))
+		goto fail;
+
+	pthread_mutex_lock(&waiter->events_lock);
+	vector_foreach_slot(waiter->events, dev_evt, i)
+		dev_evt->action = EVENT_REMOVE;
+	while (names->dev) {
+		uint32_t event_nr;
+
+		if (!dm_is_mpath(names->name))
+			goto next;
+
+		event_nr = dm_event_nr(names);
+		vector_foreach_slot(waiter->events, dev_evt, i) {
+			if (!strcmp(dev_evt->name, names->name)) {
+				if (event_nr != dev_evt->evt_nr) {
+					dev_evt->evt_nr = event_nr;
+					dev_evt->action = EVENT_UPDATE;
+				} else
+					dev_evt->action = EVENT_NOTHING;
+				break;
+			}
+		}
+next:
+		if (!names->next)
+			break;
+		names = (void *)names + names->next;
+	}
+	pthread_mutex_unlock(&waiter->events_lock);
+	dm_task_destroy(dmt);
+	return 0;
+
+fail:
+	dm_task_destroy(dmt);
+	return -1;
+}
+
+/* You must call update_multipath() after calling this function, to
+ * deal with any events that came in before the device was added */
+int watch_dmevents(char *name)
+{
+	int event_nr;
+	struct dev_event *dev_evt, *old_dev_evt;
+	int i;
+
+	if (!dm_is_mpath(name)) {
+		condlog(0, "%s: not a multipath device. can't watch events",
+			name);
+		return -1;
+	}
+
+	if ((event_nr = dm_geteventnr(name)) < 0)
+		return -1;
+
+	dev_evt = (struct dev_event *)malloc(sizeof(struct dev_event));
+	if (!dev_evt) {
+		condlog(0, "%s: can't allocate event waiter structure", name);
+		return -1;
+	}
+
+	strncpy(dev_evt->name, name, WWID_SIZE);
+	dev_evt->name[WWID_SIZE - 1] = 0;
+	dev_evt->evt_nr = event_nr;
+	dev_evt->action = EVENT_NOTHING;
+
+	pthread_mutex_lock(&waiter->events_lock);
+	vector_foreach_slot(waiter->events, old_dev_evt, i){
+		if (!strcmp(dev_evt->name, old_dev_evt->name)) {
+			/* caller will be updating this device */
+			old_dev_evt->evt_nr = event_nr;
+			old_dev_evt->action = EVENT_NOTHING;
+			pthread_mutex_unlock(&waiter->events_lock);
+			condlog(2, "%s: already waiting for events on device",
+				name);
+			free(dev_evt);
+			return 0;
+		}
+	}
+	if (!vector_alloc_slot(waiter->events)) {
+		pthread_mutex_unlock(&waiter->events_lock);
+		free(dev_evt);
+		return -1;
+	}
+	vector_set_slot(waiter->events, dev_evt);
+	pthread_mutex_unlock(&waiter->events_lock);
+	return 0;
+}
+
+void unwatch_all_dmevents(void)
+{
+	struct dev_event *dev_evt;
+	int i;
+
+	pthread_mutex_lock(&waiter->events_lock);
+	vector_foreach_slot(waiter->events, dev_evt, i)
+		free(dev_evt);
+	vector_reset(waiter->events);
+	pthread_mutex_unlock(&waiter->events_lock);
+}
+
+static void unwatch_dmevents(char *name)
+{
+	struct dev_event *dev_evt;
+	int i;
+
+	pthread_mutex_lock(&waiter->events_lock);
+	vector_foreach_slot(waiter->events, dev_evt, i) {
+		if (!strcmp(dev_evt->name, name)) {
+			vector_del_slot(waiter->events, i);
+			free(dev_evt);
+			break;
+		}
+	}
+	pthread_mutex_unlock(&waiter->events_lock);
+}
+
+/*
+ * returns the reschedule delay
+ * negative means *stop*
+ */
+
+/* poll, arm, update, return */
+static int dmevent_loop (void)
+{
+	int r, i = 0;
+	struct pollfd pfd;
+	struct dev_event *dev_evt;
+
+	pfd.fd = waiter->fd;
+	pfd.events = POLLIN;
+	r = poll(&pfd, 1, -1);
+	if (r <= 0) {
+		condlog(0, "failed polling for dm events: %s", strerror(errno));
+		/* sleep 1s and hope things get better */
+		return 1;
+	}
+
+	if (arm_dm_event_poll(waiter->fd) != 0) {
+		condlog(0, "Cannot re-arm event polling: %s", strerror(errno));
+		/* sleep 1s and hope things get better */
+		return 1;
+	}
+
+	if (dm_get_events() != 0) {
+		condlog(0, "failed getting dm events: %s", strerror(errno));
+		/* sleep 1s and hope things get better */
+		return 1;
+	}
+
+	/*
+	 * upon event ...
+	 */
+
+	while (1) {
+		int done = 1;
+		struct dev_event curr_dev;
+
+		pthread_mutex_lock(&waiter->events_lock);
+		vector_foreach_slot(waiter->events, dev_evt, i) {
+			if (dev_evt->action != EVENT_NOTHING) {
+				curr_dev = *dev_evt;
+				if (dev_evt->action == EVENT_REMOVE) {
+					vector_del_slot(waiter->events, i);
+					free(dev_evt);
+				} else
+					dev_evt->action = EVENT_NOTHING;
+				done = 0;
+				break;
+			}
+		}
+		pthread_mutex_unlock(&waiter->events_lock);
+		if (done)
+			return 1;
+
+		condlog(3, "%s: devmap event #%i", curr_dev.name,
+			curr_dev.evt_nr);
+
+		/*
+		 * event might be :
+		 *
+		 * 1) a table reload, which means our mpp structure is
+		 *    obsolete : refresh it through update_multipath()
+		 * 2) a path failed by DM : mark as such through
+		 *    update_multipath()
+		 * 3) map has gone away : stop the thread.
+		 * 4) a path reinstate : nothing to do
+		 * 5) a switch group : nothing to do
+		 */
+		pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
+		lock(&waiter->vecs->lock);
+		pthread_testcancel();
+		r = 0;
+		if (curr_dev.action == EVENT_REMOVE)
+			remove_map_by_alias(curr_dev.name, waiter->vecs, 1);
+		else
+			r = update_multipath(waiter->vecs, curr_dev.name, 1);
+		lock_cleanup_pop(&waiter->vecs->lock);
+
+		if (r) {
+			condlog(2, "%s: stopped watching dmevents",
+				curr_dev.name);
+			unwatch_dmevents(curr_dev.name);
+		}
+	}
+	condlog(0, "dmevent waiter thread unexpectedly quit");
+	return -1; /* never reach there */
+}
+
+static void rcu_unregister(void *param)
+{
+	rcu_unregister_thread();
+}
+
+void *wait_dmevents (void *unused)
+{
+	int r;
+
+
+	if (!waiter) {
+		condlog(0, "dmevents waiter not intialized");
+		return NULL;
+	}
+
+	pthread_cleanup_push(rcu_unregister, NULL);
+	rcu_register_thread();
+	mlockall(MCL_CURRENT | MCL_FUTURE);
+
+	while (1) {
+		r = dmevent_loop();
+
+		if (r < 0)
+			break;
+
+		sleep(r);
+	}
+
+	pthread_cleanup_pop(1);
+	return NULL;
+}
diff --git a/multipathd/dmevents.h b/multipathd/dmevents.h
new file mode 100644
index 0000000..012fbad
--- /dev/null
+++ b/multipathd/dmevents.h
@@ -0,0 +1,13 @@
+#ifndef _DMEVENTS_H
+#define _DMEVENTS_H
+
+#include "structs_vec.h"
+
+int dmevent_poll_supported(void);
+int init_dmevent_waiter(struct vectors *vecs);
+void cleanup_dmevent_waiter(void);
+int watch_dmevents(char *name);
+void unwatch_all_dmevents(void);
+void *wait_dmevents (void *unused);
+
+#endif /* _DMEVENTS_H */
diff --git a/multipathd/main.c b/multipathd/main.c
index aa86785..e35231e 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -82,6 +82,7 @@ static int use_watchdog;
 #include "cli_handlers.h"
 #include "lock.h"
 #include "waiter.h"
+#include "dmevents.h"
 #include "io_err_stat.h"
 #include "wwids.h"
 #include "foreign.h"
@@ -109,6 +110,11 @@ int uxsock_timeout;
 int verbosity;
 int bindings_read_only;
 int ignore_new_devs;
+#ifdef NO_DMEVENTS_POLL
+int poll_dmevents = 0;
+#else
+int poll_dmevents = 1;
+#endif
 enum daemon_status running_state = DAEMON_INIT;
 pid_t daemon_pid;
 pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -293,11 +299,23 @@ switch_pathgroup (struct multipath * mpp)
 		 mpp->alias, mpp->bestpg);
 }
 
+static int
+wait_for_events(struct multipath *mpp, struct vectors *vecs)
+{
+	if (poll_dmevents)
+		return watch_dmevents(mpp->alias);
+	else
+		return start_waiter_thread(mpp, vecs);
+}
+
 static void
 remove_map_and_stop_waiter(struct multipath *mpp, struct vectors *vecs,
 			   int purge_vec)
 {
-	stop_waiter_thread(mpp, vecs);
+	/* devices are automatically removed by the dmevent polling code,
+	 * so they don't need to be manually removed here */
+	if (!poll_dmevents)
+		stop_waiter_thread(mpp, vecs);
 	remove_map(mpp, vecs, purge_vec);
 }
 
@@ -310,8 +328,12 @@ remove_maps_and_stop_waiters(struct vectors *vecs)
 	if (!vecs)
 		return;
 
-	vector_foreach_slot(vecs->mpvec, mpp, i)
-		stop_waiter_thread(mpp, vecs);
+	if (!poll_dmevents) {
+		vector_foreach_slot(vecs->mpvec, mpp, i)
+			stop_waiter_thread(mpp, vecs);
+	}
+	else
+		unwatch_all_dmevents();
 
 	remove_maps(vecs);
 }
@@ -356,7 +378,7 @@ retry:
 	dm_lib_release();
 
 fail:
-	if (new_map && (retries < 0 || start_waiter_thread(mpp, vecs))) {
+	if (new_map && (retries < 0 || wait_for_events(mpp, vecs))) {
 		condlog(0, "%s: failed to create new map", mpp->alias);
 		remove_map(mpp, vecs, 1);
 		return 1;
@@ -875,7 +897,7 @@ retry:
 
 	if ((mpp->action == ACT_CREATE ||
 	     (mpp->action == ACT_NOTHING && start_waiter && !mpp->waiter)) &&
-	    start_waiter_thread(mpp, vecs))
+	    wait_for_events(mpp, vecs))
 			goto fail_map;
 
 	/*
@@ -2203,7 +2225,7 @@ configure (struct vectors * vecs)
 	 * start dm event waiter threads for these new maps
 	 */
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (start_waiter_thread(mpp, vecs)) {
+		if (wait_for_events(mpp, vecs)) {
 			remove_map(mpp, vecs, 1);
 			i--;
 			continue;
@@ -2449,7 +2471,7 @@ set_oom_adj (void)
 static int
 child (void * param)
 {
-	pthread_t check_thr, uevent_thr, uxlsnr_thr, uevq_thr;
+	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;
@@ -2514,6 +2536,8 @@ child (void * param)
 
 	init_foreign(conf->multipath_dir);
 
+	if (poll_dmevents)
+		poll_dmevents = dmevent_poll_supported();
 	setlogmask(LOG_UPTO(conf->verbosity + 3));
 
 	envp = getenv("LimitNOFILE");
@@ -2580,6 +2604,19 @@ child (void * param)
 
 	init_path_check_interval(vecs);
 
+	if (poll_dmevents) {
+		if (init_dmevent_waiter(vecs)) {
+			condlog(0, "failed to allocate dmevents waiter info");
+			goto failed;
+		}
+		if ((rc = pthread_create(&dmevent_thr, &misc_attr,
+					 wait_dmevents, NULL))) {
+			condlog(0, "failed to create dmevent waiter thread: %d",
+				rc);
+			goto failed;
+		}
+	}
+
 	/*
 	 * Start uevent listener early to catch events
 	 */
@@ -2649,11 +2686,15 @@ child (void * param)
 	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();
 
@@ -2669,6 +2710,8 @@ child (void * param)
 	cleanup_foreign();
 	cleanup_checkers();
 	cleanup_prio();
+	if (poll_dmevents)
+		cleanup_dmevent_waiter();
 
 	dm_lib_release();
 	dm_lib_exit();
@@ -2800,7 +2843,7 @@ main (int argc, char *argv[])
 	udev = udev_new();
 	libmp_udev_set_sync_support(0);
 
-	while ((arg = getopt(argc, argv, ":dsv:k::Bn")) != EOF ) {
+	while ((arg = getopt(argc, argv, ":dsv:k::Bniw")) != EOF ) {
 		switch(arg) {
 		case 'd':
 			foreground = 1;
@@ -2834,6 +2877,9 @@ main (int argc, char *argv[])
 		case 'n':
 			ignore_new_devs = 1;
 			break;
+		case 'w':
+			poll_dmevents = 0;
+			break;
 		default:
 			fprintf(stderr, "Invalid argument '-%c'\n",
 				optopt);
-- 
2.7.4

Re: [PATCH 11/12] multipathd: add new polling dmevents waiter thread

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> The current method of waiting for dmevents on multipath devices
> involves
> creating a seperate thread for each device. This can become very
> wasteful when there are large numbers of multipath devices. Also,
> since
> multipathd needs to grab the vecs lock to update the devices, the
> additional threads don't actually provide much parallelism.
> 
> The patch adds a new method of updating multipath devices on
> dmevents,
> which uses the new device-mapper event polling interface. This means
> that there is only one dmevent waiting thread which will wait for
> events
> on all of the multipath devices.  Currently the code to get the event
> number from the list of device names and to re-arm the polling
> interface
> is not in libdevmapper, so the patch does that work. Obviously, these
> bits need to go into libdevmapper, so that multipathd can use a
> standard
> interface.
> 
> I haven't touched any of the existing event waiting code, since event
> polling was only added to device-mapper in version
> 4.37.0.  multipathd
> checks this version, and defaults to using the polling code if
> device-mapper supports it. This can be overridden by running
> multipathd
> with "-w", to force it to use the old event waiting code.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

... with some minor nitpicks, see below.

> ---
>  Makefile.inc               |   3 +
>  libmultipath/structs_vec.c |   8 +
>  libmultipath/structs_vec.h |   2 +
>  multipathd/Makefile        |   6 +-
>  multipathd/dmevents.c      | 392
> +++++++++++++++++++++++++++++++++++++++++++++
>  multipathd/dmevents.h      |  13 ++
>  multipathd/main.c          |  62 ++++++-
>  7 files changed, 477 insertions(+), 9 deletions(-)
>  create mode 100644 multipathd/dmevents.c
>  create mode 100644 multipathd/dmevents.h
> 
> 

> +static int arm_dm_event_poll(int fd)
> +{
> +	struct dm_ioctl dmi;
> +	memset(&dmi, 0, sizeof(dmi));
> +	dmi.version[0] = DM_VERSION_MAJOR;
> +	dmi.version[1] = DM_VERSION_MINOR;
> +	dmi.version[2] = DM_VERSION_PATCHLEVEL;
> +	dmi.flags = 0x4;

We've had this before, I'd appreciate a short comment explaining this
flag as Alasdair did in his post from Feb 13th.

> +
> +/* You must call update_multipath() after calling this function, to
> + * deal with any events that came in before the device was added */

This comment is slightly irritating, because you don't call
update_multipath() anywhere where you call watch_dmevents (rather, you
call __setup_multipath). I understand the comment is about the order of
calls, not about having to call update_multipath(), but that's not
immediately obvious.

> +int watch_dmevents(char *name)
> +{
> +	int event_nr;
> +	struct dev_event *dev_evt, *old_dev_evt;
> +	int i;
> +
> +	if (!dm_is_mpath(name)) {
> +		condlog(0, "%s: not a multipath device. can't watch
> events",
> +			name);
> +		return -1;
> +	}
> +
> +	if ((event_nr = dm_geteventnr(name)) < 0)
> +		return -1;
> +
> +	dev_evt = (struct dev_event *)malloc(sizeof(struct
> dev_event));
> +	if (!dev_evt) {
> +		condlog(0, "%s: can't allocate event waiter
> structure", name);
> +		return -1;
> +	}
> +
> +	strncpy(dev_evt->name, name, WWID_SIZE);
> +	dev_evt->name[WWID_SIZE - 1] = 0;

Nitpick: using strlcpy() in new code would simplify code and review.

> +	dev_evt->evt_nr = event_nr;
> +	dev_evt->action = EVENT_NOTHING;
> +
> +	pthread_mutex_lock(&waiter->events_lock);
> +	vector_foreach_slot(waiter->events, old_dev_evt, i){
> +		if (!strcmp(dev_evt->name, old_dev_evt->name)) {
> +			/* caller will be updating this device */
> +			old_dev_evt->evt_nr = event_nr;
> +			old_dev_evt->action = EVENT_NOTHING;
> +			pthread_mutex_unlock(&waiter->events_lock);
> +			condlog(2, "%s: already waiting for events
> on device",
> +				name);

What about using condlog(3) here? Is it something admins need to care
about in regular operation?

> +			free(dev_evt);
> +			return 0;
> +		}
> +	}
> +	if (!vector_alloc_slot(waiter->events)) {
> +		pthread_mutex_unlock(&waiter->events_lock);
> +		free(dev_evt);
> +		return -1;
> +	}
> +	vector_set_slot(waiter->events, dev_evt);
> +	pthread_mutex_unlock(&waiter->events_lock);
> +	return 0;
> +}

> +/*
> + * returns the reschedule delay
> + * negative means *stop*
> + */
> +
> +/* poll, arm, update, return */
> +static int dmevent_loop (void)
> +{
> +	int r, i = 0;
> +	struct pollfd pfd;
> +	struct dev_event *dev_evt;
> +
> +	pfd.fd = waiter->fd;
> +	pfd.events = POLLIN;
> +	r = poll(&pfd, 1, -1);
> +	if (r <= 0) {
> +		condlog(0, "failed polling for dm events: %s",
> strerror(errno));
> +		/* sleep 1s and hope things get better */
> +		return 1;
> +	}
> +
> +	if (arm_dm_event_poll(waiter->fd) != 0) {
> +		condlog(0, "Cannot re-arm event polling: %s",
> strerror(errno));
> +		/* sleep 1s and hope things get better */
> +		return 1;
> +	}
> +
> +	if (dm_get_events() != 0) {
> +		condlog(0, "failed getting dm events: %s",
> strerror(errno));
> +		/* sleep 1s and hope things get better */
> +		return 1;
> +	}
> +
> +	/*
> +	 * upon event ...
> +	 */
> +
> +	while (1) {
> +		int done = 1;
> +		struct dev_event curr_dev;
> +
> +		pthread_mutex_lock(&waiter->events_lock);
> +		vector_foreach_slot(waiter->events, dev_evt, i) {
> +			if (dev_evt->action != EVENT_NOTHING) {
> +				curr_dev = *dev_evt;
> +				if (dev_evt->action == EVENT_REMOVE)
> {
> +					vector_del_slot(waiter-
> >events, i);
> +					free(dev_evt);
> +				} else
> +					dev_evt->action =
> EVENT_NOTHING;
> +				done = 0;
> +				break;
> +			}
> +		}
> +		pthread_mutex_unlock(&waiter->events_lock);
> +		if (done)
> +			return 1;
> +
> +		condlog(3, "%s: devmap event #%i", curr_dev.name,
> +			curr_dev.evt_nr);
> +
> +		/*
> +		 * event might be :
> +		 *
> +		 * 1) a table reload, which means our mpp structure
> is
> +		 *    obsolete : refresh it through
> update_multipath()
> +		 * 2) a path failed by DM : mark as such through
> +		 *    update_multipath()
> +		 * 3) map has gone away : stop the thread.
> +		 * 4) a path reinstate : nothing to do
> +		 * 5) a switch group : nothing to do
> +		 */
> +		pthread_cleanup_push(cleanup_lock, &waiter->vecs-
> >lock);
> +		lock(&waiter->vecs->lock);
> +		pthread_testcancel();
> +		r = 0;
> +		if (curr_dev.action == EVENT_REMOVE)
> +			remove_map_by_alias(curr_dev.name, waiter-
> >vecs, 1);
> +		else
> +			r = update_multipath(waiter->vecs,
> curr_dev.name, 1);
> +		lock_cleanup_pop(&waiter->vecs->lock);

I dislike lock_cleanup_pop(). pthread_cleanup_pop(1) would be so much
more obvious.

Regards,
Martin

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

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

[PATCH 12/12] multipath: add unit tests for dmevents code

[Benjamin Marzinski]

These unit tests do not get complete code coverage. Also, they don't
check for memory errors. To do this through unit tests, instead of
using valgrid, would require adding unit test specific compilation
defines to the code, and compiling a seperate unit-test version.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/Makefile   |   9 +-
 tests/dmevents.c | 847 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 853 insertions(+), 3 deletions(-)
 create mode 100644 tests/dmevents.c

diff --git a/tests/Makefile b/tests/Makefile
index 3450b14..1f36411 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -3,11 +3,16 @@ include ../Makefile.inc
 CFLAGS += $(BIN_CFLAGS) -I$(multipathdir) -I$(mpathcmddir)
 LIBDEPS += -L$(multipathdir) -lmultipath -lcmocka
 
-TESTS := uevent parser util
+TESTS := uevent parser util dmevents
 
 .SILENT: $(TESTS:%=%.o)
 .PRECIOUS: $(TESTS:%=%-test)
 
+all:	$(TESTS:%=%.out)
+
+dmevents-test: dmevents.o ../multipathd/dmevents.c globals.c $(multipathdir)/libmultipath.so
+	@$(CC) -o $@ $< $(LDFLAGS) $(LIBDEPS) -lpthread -ldevmapper -lurcu -Wl,--wrap=open -Wl,--wrap=close -Wl,--wrap=dm_is_mpath -Wl,--wrap=dm_geteventnr -Wl,--wrap=ioctl -Wl,--wrap=libmp_dm_task_create -Wl,--wrap=dm_task_no_open_count -Wl,--wrap=dm_task_run -Wl,--wrap=dm_task_get_names -Wl,--wrap=dm_task_destroy -Wl,--wrap=poll -Wl,--wrap=remove_map_by_alias -Wl,--wrap=update_multipath
+
 %-test:	%.o globals.c $(multipathdir)/libmultipath.so
 	@$(CC) -o $@ $< $(LDFLAGS) $(LIBDEPS)
 
@@ -15,8 +20,6 @@ TESTS := uevent parser util
 	@echo == running $< ==
 	@LD_LIBRARY_PATH=$(multipathdir):$(mpathcmddir) ./$< >$@
 
-all:	$(TESTS:%=%.out)
-
 clean: dep_clean
 	rm -f $(TESTS:%=%-test) $(TESTS:%=%.out) $(TESTS:%=%.o)
 
diff --git a/tests/dmevents.c b/tests/dmevents.c
new file mode 100644
index 0000000..4442fc2
--- /dev/null
+++ b/tests/dmevents.c
@@ -0,0 +1,847 @@
+/*
+ * Copyright (c) 2018 Benjamin Marzinski, Redhat
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ *
+ */
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <stdarg.h>
+#include <stddef.h>
+#include <setjmp.h>
+#include <stdlib.h>
+#include <cmocka.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include "structs.h"
+#include "structs_vec.h"
+
+#include "globals.c"
+/* I have to do this to get at the static variables */
+#include "../multipathd/dmevents.c"
+
+struct dm_device {
+	char name[WWID_SIZE];
+	int is_mpath;
+	uint32_t evt_nr;
+	uint32_t update_nr;
+};
+
+struct test_data {
+	struct vectors vecs;
+	vector dm_devices;
+	struct dm_names *names;
+};
+
+struct test_data data;
+
+int add_dm_device_event(char *name, int is_mpath, uint32_t evt_nr)
+{
+	struct dm_device *dev;
+	int i;
+
+	vector_foreach_slot(data.dm_devices, dev, i) {
+		if (strcmp(name, dev->name) == 0) {
+			dev->evt_nr = evt_nr;
+			return 0;
+		}
+	}
+	dev = (struct dm_device *)malloc(sizeof(struct dm_device));
+	if (!dev){
+		condlog(0, "Testing error mallocing dm_device");
+		return -1;
+	}
+	strncpy(dev->name, name, WWID_SIZE);
+	dev->name[WWID_SIZE - 1] = 0;
+	dev->is_mpath = is_mpath;
+	dev->evt_nr = evt_nr;
+	if (!vector_alloc_slot(data.dm_devices)) {
+		condlog(0, "Testing error setting dm_devices slot");
+		free(dev);
+		return -1;
+	}
+	vector_set_slot(data.dm_devices, dev);
+	return 0;
+}
+
+struct dm_device *find_dm_device(const char *name)
+{
+	struct dm_device *dev;
+	int i;
+
+	vector_foreach_slot(data.dm_devices, dev, i)
+		if (strcmp(name, dev->name) == 0)
+			return dev;
+	return NULL;
+}
+
+int remove_dm_device_event(const char *name)
+{
+	struct dm_device *dev;
+	int i;
+
+	vector_foreach_slot(data.dm_devices, dev, i) {
+		if (strcmp(name, dev->name) == 0) {
+			vector_del_slot(data.dm_devices, i);
+				free(dev);
+			return 0;
+		}
+	}
+	return -1;
+}
+
+void remove_all_dm_device_events(void)
+{
+	struct dm_device *dev;
+	int i;
+
+	vector_foreach_slot(data.dm_devices, dev, i)
+		free(dev);
+	vector_reset(data.dm_devices);
+}
+
+static inline size_t align_val(size_t val)
+{
+        return (val + 7) & ~7;
+}
+static inline void *align_ptr(void *ptr)
+{
+	return (void *)align_val((size_t)ptr);
+}
+
+/* copied off of list_devices in dm-ioctl.c */
+struct dm_names *build_dm_names(void)
+{
+	struct dm_names *names, *np, *old_np = NULL;
+	uint32_t *event_nr;
+	struct dm_device *dev;
+	int i, size = 0;
+
+	if (VECTOR_SIZE(data.dm_devices) == 0) {
+		names = (struct dm_names *)malloc(sizeof(struct dm_names));
+		if (!names) {
+			condlog(0, "Testing error allocating empty dm_names");
+			return NULL;
+		}
+		names->dev = 0;
+		names->next = 0;
+		return names;
+	}
+	vector_foreach_slot(data.dm_devices, dev, i) {
+		size += align_val(offsetof(struct dm_names, name) +
+				  strlen(dev->name) + 1);
+		size += align_val(sizeof(uint32_t));
+	}
+	names = (struct dm_names *)malloc(size);
+	if (!names) {
+		condlog(0, "Testing error allocating dm_names");
+		return NULL;
+	}
+	np = names;
+	vector_foreach_slot(data.dm_devices, dev, i) {
+		if (old_np)
+			old_np->next = (uint32_t) ((uintptr_t) np -
+						   (uintptr_t) old_np);
+		np->dev = 1;
+		np->next = 0;
+		strcpy(np->name, dev->name);
+
+		old_np = np;
+		event_nr = align_ptr(np->name + strlen(dev->name) + 1);
+		*event_nr = dev->evt_nr;
+		np = align_ptr(event_nr + 1);
+	}
+	assert_int_equal((char *)np - (char *)names, size);
+	return names;
+}
+
+static int setup(void **state)
+{
+	if (dmevent_poll_supported()) {
+		data.dm_devices = vector_alloc();
+		*state = &data;
+	} else
+		*state = NULL;
+	return 0;
+}
+
+static int teardown(void **state)
+{
+	struct dm_device *dev;
+	int i;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		return 0;
+	vector_foreach_slot(datap->dm_devices, dev, i)
+		free(dev);
+	vector_free(datap->dm_devices);
+	datap = NULL;
+	return 0;
+}
+
+int __wrap_open(const char *pathname, int flags)
+{
+	assert_ptr_equal(pathname, "/dev/mapper/control");
+	assert_int_equal(flags, O_RDWR);
+	return mock_type(int);
+}
+
+int __wrap_close(int fd)
+{
+	assert_int_equal(fd, waiter->fd);
+	return 0;
+}
+
+int __wrap_dm_is_mpath(const char *name)
+{
+	struct dm_device *dev;
+	int i;
+
+	vector_foreach_slot(data.dm_devices, dev, i)
+		if (strcmp(name, dev->name) == 0)
+			return dev->is_mpath;
+	return 0;
+}
+
+int __wrap_dm_geteventnr(const char *name)
+{
+	struct dm_device *dev;
+	int fail = mock_type(int);
+
+	if (fail)
+		return -1;
+	dev = find_dm_device(name);
+	if (dev) {
+		/* simulate updating device state after adding it */
+		dev->update_nr = dev->evt_nr;
+		return dev->evt_nr;
+	}
+	return -1;
+}
+
+int __wrap_ioctl(int fd, unsigned long request, void *argp)
+{
+	assert_int_equal(fd, waiter->fd);
+	assert_int_equal(request, DM_DEV_ARM_POLL);
+	return mock_type(int);
+}
+
+struct dm_task *__wrap_libmp_dm_task_create(int task)
+{
+	assert_int_equal(task, DM_DEVICE_LIST);
+	return mock_type(struct dm_task *);
+}
+
+int __wrap_dm_task_no_open_count(struct dm_task *dmt)
+{
+	assert_ptr_equal((struct test_data *)dmt, &data);
+	return mock_type(int);
+}
+
+int __wrap_dm_task_run(struct dm_task *dmt)
+{
+	assert_ptr_equal((struct test_data *)dmt, &data);
+	return mock_type(int);
+}
+
+struct dm_names * __wrap_dm_task_get_names(struct dm_task *dmt)
+{
+	int good = mock_type(int);
+	assert_ptr_equal((struct test_data *)dmt, &data);
+
+	if (data.names) {
+		condlog(0, "Testing error. data.names already allocated");
+		return NULL;
+	}
+	if (!good)
+		return NULL;
+	data.names = build_dm_names();
+	return data.names;
+}
+
+void __wrap_dm_task_destroy(struct dm_task *dmt)
+{
+	assert_ptr_equal((struct test_data *)dmt, &data);
+
+	if (data.names) {
+		free(data.names);
+		data.names = NULL;
+	}
+}
+
+int __wrap_poll(struct pollfd *fds, nfds_t nfds, int timeout)
+{
+	assert_int_equal(nfds, 1);
+	assert_int_equal(timeout, -1);
+	assert_int_equal(fds->fd, waiter->fd);
+	assert_int_equal(fds->events, POLLIN);
+	return mock_type(int);
+}
+
+void __wrap_remove_map_by_alias(const char *alias, struct vectors * vecs,
+				int purge_vec)
+{
+	check_expected(alias);
+	assert_ptr_equal(vecs, waiter->vecs);
+	assert_int_equal(purge_vec, 1);
+}
+
+int __wrap_update_multipath(struct vectors *vecs, char *mapname, int reset)
+{
+	int fail;
+
+	check_expected(mapname);
+	assert_ptr_equal(vecs, waiter->vecs);
+	assert_int_equal(reset, 1);
+	fail = mock_type(int);
+	if (fail) {
+		assert_int_equal(remove_dm_device_event(mapname), 0);
+		return fail;
+	} else {
+		struct dm_device *dev;
+		int i;
+
+		vector_foreach_slot(data.dm_devices, dev, i) {
+			if (strcmp(mapname, dev->name) == 0) {
+				dev->update_nr = dev->evt_nr;
+				return 0;
+			}
+		}
+		fail();
+	}
+	return fail;
+}
+
+struct dev_event *find_dmevents(const char *name)
+{
+	struct dev_event *dev_evt;
+	int i;
+
+	vector_foreach_slot(waiter->events, dev_evt, i)
+		if (!strcmp(dev_evt->name, name))
+			return dev_evt;
+	return NULL;
+}
+
+static void test_init_waiter_bad0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	assert_int_equal(init_dmevent_waiter(NULL), -1);
+}
+
+static void test_init_waiter_bad1(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	will_return(__wrap_open, -1);
+	assert_int_equal(init_dmevent_waiter(&datap->vecs), -1);
+	assert_ptr_equal(waiter, NULL);
+}
+
+static void test_init_waiter_good0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	will_return(__wrap_open, 2);
+	assert_int_equal(init_dmevent_waiter(&datap->vecs), 0);
+	assert_ptr_not_equal(waiter, NULL);
+}
+
+static void test_watch_dmevents_bad0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	assert_int_equal(watch_dmevents("foo"), -1);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+}
+
+static void test_watch_dmevents_bad1(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	assert_int_equal(add_dm_device_event("foo", 0, 5), 0);
+	assert_int_equal(watch_dmevents("foo"), -1);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+}
+
+static void test_watch_dmevents_bad2(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	will_return(__wrap_dm_geteventnr, -1);
+	assert_int_equal(watch_dmevents("foo"), -1);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+}
+static void test_watch_dmevents_good0(void **state)
+{
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 1);
+	unwatch_dmevents("foo");
+	assert_int_equal(VECTOR_SIZE(waiter->events), 0);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+}
+
+static void test_watch_dmevents_good1(void **state)
+{
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);	
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	assert_int_equal(add_dm_device_event("foo", 1, 6), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 6);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 1);
+	unwatch_dmevents("foo");
+	assert_int_equal(VECTOR_SIZE(waiter->events), 0);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+}
+
+static void test_watch_dmevents_good2(void **state)
+{
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	unwatch_all_dmevents();
+	remove_all_dm_device_events();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	assert_int_equal(add_dm_device_event("bar", 1, 7), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	assert_ptr_equal(find_dmevents("bar"), NULL);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("bar"), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev_evt = find_dmevents("bar");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 7);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 2);
+	unwatch_all_dmevents();
+	assert_int_equal(VECTOR_SIZE(waiter->events), 0);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+	assert_ptr_equal(find_dmevents("bar"), NULL);
+}
+
+static void test_get_events_bad0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	unwatch_all_dmevents();
+	remove_all_dm_device_events();
+
+	will_return(__wrap_libmp_dm_task_create, NULL);
+	assert_int_equal(dm_get_events(), -1);
+}
+
+static void test_get_events_bad1(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 0);
+	assert_int_equal(dm_get_events(), -1);
+}
+
+static void test_get_events_bad2(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 0);
+	assert_int_equal(dm_get_events(), -1);
+}
+
+static void test_get_events_good0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	assert_int_equal(dm_get_events(), 0);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 0);
+}
+
+static void test_get_events_good1(void **state)
+{
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	assert_int_equal(add_dm_device_event("bar", 1, 7), 0);
+	assert_int_equal(add_dm_device_event("baz", 1, 12), 0);
+	assert_int_equal(add_dm_device_event("qux", 0, 4), 0);
+	assert_int_equal(add_dm_device_event("xyzzy", 1, 8), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("bar"), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("xyzzy"), 0);
+	assert_int_equal(add_dm_device_event("foo", 1, 6), 0);
+	assert_int_equal(remove_dm_device_event("xyzzy"), 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	assert_int_equal(dm_get_events(), 0);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 6);
+	assert_int_equal(dev_evt->action, EVENT_UPDATE);
+	dev_evt = find_dmevents("bar");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 7);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev_evt = find_dmevents("xyzzy");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 8);
+	assert_int_equal(dev_evt->action, EVENT_REMOVE);
+	assert_ptr_equal(find_dmevents("baz"), NULL);
+	assert_ptr_equal(find_dmevents("qux"), NULL);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 3);
+}
+
+static void test_dmevent_loop_bad0(void **state)
+{
+	struct dm_device *dev;
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	unwatch_all_dmevents();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	assert_int_equal(add_dm_device_event("foo", 1, 6), 0);
+	will_return(__wrap_poll, 0);
+	assert_int_equal(dmevent_loop(), 1);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("foo");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 6);
+	assert_int_equal(dev->update_nr, 5);
+}
+
+static void test_dmevent_loop_bad1(void **state)
+{
+	struct dm_device *dev;
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, -1);
+	assert_int_equal(dmevent_loop(), 1);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("foo");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 6);
+	assert_int_equal(dev->update_nr, 5);
+}
+
+static void test_dmevent_loop_bad2(void **state)
+{
+	struct dm_device *dev;
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, 0);
+	will_return(__wrap_libmp_dm_task_create, NULL);
+	assert_int_equal(dmevent_loop(), 1);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 5);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("foo");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 6);
+	assert_int_equal(dev->update_nr, 5);
+}
+
+static void test_dmevent_loop_good0(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	unwatch_all_dmevents();
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	assert_int_equal(dmevent_loop(), 1);
+}
+
+static void test_dmevent_loop_good1(void **state)
+{
+	struct dm_device *dev;
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	remove_all_dm_device_events();
+	unwatch_all_dmevents();
+	assert_int_equal(add_dm_device_event("foo", 1, 5), 0);
+	assert_int_equal(add_dm_device_event("bar", 1, 7), 0);
+	assert_int_equal(add_dm_device_event("baz", 1, 12), 0);
+	assert_int_equal(add_dm_device_event("xyzzy", 1, 8), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("foo"), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("bar"), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("xyzzy"), 0);
+	assert_int_equal(add_dm_device_event("foo", 1, 6), 0);
+	assert_int_equal(remove_dm_device_event("xyzzy"), 0);
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	expect_string(__wrap_update_multipath, mapname, "foo");
+	will_return(__wrap_update_multipath, 0);
+	expect_string(__wrap_remove_map_by_alias, alias, "xyzzy");
+	assert_int_equal(dmevent_loop(), 1);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 2);
+	assert_int_equal(VECTOR_SIZE(data.dm_devices), 3);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 6);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("foo");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 6);
+	assert_int_equal(dev->update_nr, 6);
+	dev_evt = find_dmevents("bar");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 7);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("bar");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 7);
+	assert_int_equal(dev->update_nr, 7);
+	assert_ptr_equal(find_dmevents("xyzzy"), NULL);
+	assert_ptr_equal(find_dm_device("xyzzy"), NULL);
+}
+
+static void test_dmevent_loop_good2(void **state)
+{
+	struct dm_device *dev;
+	struct dev_event *dev_evt;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	assert_int_equal(add_dm_device_event("bar", 1, 9), 0);
+	will_return(__wrap_dm_geteventnr, 0);
+	assert_int_equal(watch_dmevents("baz"), 0);
+	assert_int_equal(add_dm_device_event("baz", 1, 14), 0);
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	expect_string(__wrap_update_multipath, mapname, "bar");
+	will_return(__wrap_update_multipath, 0);
+	expect_string(__wrap_update_multipath, mapname, "baz");
+	will_return(__wrap_update_multipath, 1);
+	assert_int_equal(dmevent_loop(), 1);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 2);
+	assert_int_equal(VECTOR_SIZE(data.dm_devices), 2);
+	dev_evt = find_dmevents("foo");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 6);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("foo");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 6);
+	assert_int_equal(dev->update_nr, 6);
+	dev_evt = find_dmevents("bar");
+	assert_ptr_not_equal(dev_evt, NULL);
+	assert_int_equal(dev_evt->evt_nr, 9);
+	assert_int_equal(dev_evt->action, EVENT_NOTHING);
+	dev = find_dm_device("bar");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 9);
+	assert_int_equal(dev->update_nr, 9);
+	assert_ptr_equal(find_dmevents("baz"), NULL);
+	assert_ptr_equal(find_dm_device("baz"), NULL);
+}
+
+static void test_dmevent_loop_good3(void **state)
+{
+	struct dm_device *dev;
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+
+	assert_int_equal(remove_dm_device_event("foo"), 0);
+	unwatch_dmevents("bar");
+	will_return(__wrap_poll, 1);
+	will_return(__wrap_ioctl, 0);
+	will_return(__wrap_libmp_dm_task_create, &data);
+	will_return(__wrap_dm_task_no_open_count, 1);
+	will_return(__wrap_dm_task_run, 1);
+	will_return(__wrap_dm_task_get_names, 1);
+	expect_string(__wrap_remove_map_by_alias, alias, "foo");
+	assert_int_equal(dmevent_loop(), 1);
+	assert_int_equal(VECTOR_SIZE(waiter->events), 0);
+	assert_int_equal(VECTOR_SIZE(data.dm_devices), 1);
+	dev = find_dm_device("bar");
+	assert_ptr_not_equal(dev, NULL);
+	assert_int_equal(dev->evt_nr, 9);
+	assert_int_equal(dev->update_nr, 9);
+	assert_ptr_equal(find_dmevents("foo"), NULL);
+	assert_ptr_equal(find_dmevents("bar"), NULL);
+	assert_ptr_equal(find_dm_device("foo"), NULL);
+}
+
+static void test_arm_poll(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	will_return(__wrap_ioctl, 0);
+	assert_int_equal(arm_dm_event_poll(waiter->fd), 0);
+}
+
+static void test_cleanup_waiter(void **state)
+{
+	struct test_data *datap = (struct test_data *)(*state);
+	if (datap == NULL)
+		skip();
+	cleanup_dmevent_waiter();
+	assert_ptr_equal(waiter, NULL);
+}
+
+int test_dmevents(void)
+{
+	const struct CMUnitTest tests[] = {
+		cmocka_unit_test(test_init_waiter_bad0),
+		cmocka_unit_test(test_init_waiter_bad1),
+		cmocka_unit_test(test_init_waiter_good0),
+		cmocka_unit_test(test_watch_dmevents_bad0),
+		cmocka_unit_test(test_watch_dmevents_bad1),
+		cmocka_unit_test(test_watch_dmevents_bad2),
+		cmocka_unit_test(test_watch_dmevents_good0),
+		cmocka_unit_test(test_watch_dmevents_good1),
+		cmocka_unit_test(test_watch_dmevents_good2),
+		cmocka_unit_test(test_get_events_bad0),
+		cmocka_unit_test(test_get_events_bad1),
+		cmocka_unit_test(test_get_events_bad2),
+		cmocka_unit_test(test_get_events_good0),
+		cmocka_unit_test(test_get_events_good1),
+		cmocka_unit_test(test_arm_poll),
+		cmocka_unit_test(test_dmevent_loop_bad0),
+		cmocka_unit_test(test_dmevent_loop_bad1),
+		cmocka_unit_test(test_dmevent_loop_bad2),
+		cmocka_unit_test(test_dmevent_loop_good0),
+		cmocka_unit_test(test_dmevent_loop_good1),
+		cmocka_unit_test(test_dmevent_loop_good2),
+		cmocka_unit_test(test_dmevent_loop_good3),
+		cmocka_unit_test(test_cleanup_waiter),
+	};
+	return cmocka_run_group_tests(tests, setup, teardown);
+}
+
+int main(void)
+{
+	int ret = 0;
+
+	ret += test_dmevents();
+	return ret;
+}
-- 
2.7.4

Re: [PATCH 12/12] multipath: add unit tests for dmevents code

[Martin Wilck]

On Wed, 2018-03-14 at 12:46 -0500, Benjamin Marzinski wrote:
> These unit tests do not get complete code coverage. Also, they don't
> check for memory errors. To do this through unit tests, instead of
> using valgrid, would require adding unit test specific compilation
> defines to the code, and compiling a seperate unit-test version.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

I'm impressed, you went much further with cmocka than I did so far.
Nice work.

Reviewed-by: Martin Wilck <mwilck@suse.com>

... but this is pretty dense material. I'd appreciate a follow-up patch
adding some comments about the more complex test cases to make them
easier to understand.

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

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

Re: [PATCH 00/12] multipath: new and rebased patches

[Benjamin Marzinski]

Ooops. I accidentally used your old email for this patchset, Christophe.

-Ben

On Wed, Mar 14, 2018 at 12:46:33PM -0500, Benjamin Marzinski wrote:
> Patches 1-5 are minor bug fixes and a unit test for basenamecpy.
> 
> Patch 6 is revised version of my nanosleep patch, that only changes
> the checkerloop code, since that is the only time when multipathd
> actually sets an alarm without locking.
> 
> Patches 7-11 are a rebase of my "alternate dmevents waiter method"
> patchset.  The only code that has changed outside of rebasing them is in
> patch 11.  It adds a function, remove_map_by_alias(), and uses it in
> dmevent_loop(). This simply moves the code the gets the multipath device
> out of dmevents.c, so it can work exclusively in device names.  It also
> changes the names of the functions the initialize and cleanup the
> dmevents polling code to match similar multipathd functions and adds
> a Makefile define to disable using the polling code, both suggested
> by Martin Wilck.
> 
> Patch 12 is unit testing code for the new code in dmevents.c
> 
> Here is the description from my initial posting of the alternate dmevents
> waiter method" patchset:
> 
> This patchset implements a new method of getting dmevents for
> multipathd.
> 
> With the existing wait code, multipathd needs to create a waiter thread
> for every multipath device. This can become very wasteful in setups with
> large numbers of multipath devices. These duplicate threads all are
> serialized to update the multipath devices, so they don't actually speed
> up dmevent handling.
> 
> The new method uses the new dmevent polling ability introduced in the
> 4.37.0 device-mapper kernel module.  The original method has been
> retained for backwards compatablility, and it is possible to force
> multipathd to use the orignal method on newer kernels. The benefit of
> this new method is that there is only one thread necessary to wait on
> dmevents, which can be started when device-mapper starts, and stopped
> during shutdown, just like the other main threads.
> 
> These patches use device-mapper features that don't have a libdevmapper
> API.  They will switch over as soon as support is available in
> libdevmapper.
> 
> Benjamin Marzinski (12):
>   Unit tests for basenamecpy
>   libmultipath: fix basenamecpy
>   libmultipath: set dm_conf_verbosity
>   multipathd: log thread cleanup
>   libmultipath: fix log_pthread processing
>   multipathd: use nanosleep for strict timing
>   libmultipath: move remove_map waiter code to multipathd
>   move waiter code from libmultipath to multipathd
>   call start_waiter_thread() before setup_multipath()
>   libmultipath: add helper functions
>   multipathd: add new polling dmevents waiter thread
>   multipath: add unit tests for dmevents code
> 
>  Makefile.inc               |   3 +
>  libmultipath/Makefile      |   2 +-
>  libmultipath/devmapper.c   |  29 +-
>  libmultipath/devmapper.h   |   3 +-
>  libmultipath/log_pthread.c |  40 +--
>  libmultipath/log_pthread.h |  10 +-
>  libmultipath/structs_vec.c | 140 +-------
>  libmultipath/structs_vec.h |   8 +-
>  libmultipath/util.c        |  25 +-
>  libmultipath/util.h        |   2 +-
>  libmultipath/vector.c      |  16 +-
>  libmultipath/vector.h      |   1 +
>  libmultipath/waiter.c      | 215 ------------
>  libmultipath/waiter.h      |  17 -
>  multipathd/Makefile        |   6 +-
>  multipathd/dmevents.c      | 392 +++++++++++++++++++++
>  multipathd/dmevents.h      |  13 +
>  multipathd/main.c          | 230 ++++++++++--
>  multipathd/waiter.c        | 215 ++++++++++++
>  multipathd/waiter.h        |  17 +
>  tests/Makefile             |   9 +-
>  tests/dmevents.c           | 847 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/util.c               | 167 +++++++++
>  23 files changed, 1944 insertions(+), 463 deletions(-)
>  delete mode 100644 libmultipath/waiter.c
>  delete mode 100644 libmultipath/waiter.h
>  create mode 100644 multipathd/dmevents.c
>  create mode 100644 multipathd/dmevents.h
>  create mode 100644 multipathd/waiter.c
>  create mode 100644 multipathd/waiter.h
>  create mode 100644 tests/dmevents.c
>  create mode 100644 tests/util.c
> 
> -- 
> 2.7.4
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel