[dm-devel] [PATCH 00/14] Rework uevent filtering and merging

[dm-devel] [PATCH 00/14] Rework uevent filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2436 bytes --]

From: Martin Wilck <mwilck@suse.com>

Hi Ben, hi Christophe, hi Tang Junhui,

the bulk of this patch set (3-8) is a rework of the uevent filtering and
merging logic introduced in commit ee8888f ("multipath-tools: improve 
processing efficiency for addition and deletion of multipath devices"),
by Tang Junhui.

The rationale is explained in detail in the commit message in patch 08/14.
TL;DR: The previous approach delayed uevent handling, possibly a lot, which
is often undesirable. The new logic passes events to the dispatcher
immediately, but if they queue up (because the dispatcher can't keep
up with the rate at which events arrive, or is blocked e.g. by the
path checker), the dispatcher will apply filtering and merging
between servicing individual events. This worked well in my own testing,
but I'd appreciate if ZTE could give it a shot in their special test
environment.

Patch 9-14 add some more improvements to the uevent handling code, and
improve logging. The first 2 patches are unrelated fixes.

Comments welcome,

Martin

Martin Wilck (14):
  multipathd: allow adding git rev as version suffix
  multipathd: don't switch to DAEMON_IDLE during startup
  uevent_dispatch(): use while in wait loop
  libmultipath: uevent_dispatch(): process uevents one by one
  libmultipath: uevent_dispatch(): only filter/merge new uevents
  multipathd: reconfigure: disallow changing uid_attrs
  libmultipath: microoptimize uevent filtering and merging
  libmultipath: uevent_listen(): don't delay uevents
  libmultipath: uevent: use struct to pass parameters around
  libmultipath: uevent: log statistics about filtering and merging
  libmultipath: merge_uevq(): filter first, then merge
  libmultipath: uevent_filter(): filter previously merged events
  libmultipath: uevent: improve log messages
  libmultipath: uevent: add debug messages for event queue

 Makefile.inc               |   3 +-
 libmultipath/config.c      |   6 +-
 libmultipath/config.h      |   4 +-
 libmultipath/discovery.c   |   2 +-
 libmultipath/list.h        |  53 +++++
 libmultipath/structs.h     |   2 +-
 libmultipath/uevent.c      | 407 ++++++++++++++++++++++---------------
 libmultipath/uevent.h      |   3 +-
 multipath/multipath.conf.5 |   2 +
 multipathd/main.c          |  59 ++++--
 tests/uevent.c             |   2 +-
 11 files changed, 354 insertions(+), 189 deletions(-)

-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 01/14] multipathd: allow adding git rev as version suffix

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1779 bytes --]

From: Martin Wilck <mwilck@suse.com>

Use the make variable EXTRAVERSION to hold a version suffix,
and print it on multipathd startup. By default, EXTRAVERSION is
the 7-digit git rev in the format "-g1234567" if git is available,
and empty otherwise. Build systems can pass in alternative
EXTRAVERSION strings as they prefer.

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

diff --git a/Makefile.inc b/Makefile.inc
index d24da43..120f671 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -13,6 +13,7 @@
 # SCSI_DH_MODULES_PRELOAD := scsi_dh_alua scsi_dh_rdac
 SCSI_DH_MODULES_PRELOAD :=
 
+EXTRAVERSION := $(shell rev=$$(git rev-parse --short=7 HEAD 2>/dev/null); echo $${rev:+-g$$rev})
 
 PKGCONFIG	?= pkg-config
 
@@ -126,7 +127,7 @@ WARNFLAGS	:= -Werror -Wall -Wextra -Wformat=2 $(WFORMATOVERFLOW) -Werror=implici
 CPPFLAGS	:= -Wp,-D_FORTIFY_SOURCE=2
 CFLAGS		:= --std=gnu99 $(CFLAGS) $(OPTFLAGS) $(WARNFLAGS) -pipe \
 		   -DBIN_DIR=\"$(bindir)\" -DLIB_STRING=\"${LIB}\" -DRUN_DIR=\"${RUN}\" \
-		   -MMD -MP
+		   -DEXTRAVERSION=\"$(EXTRAVERSION)\" -MMD -MP
 BIN_CFLAGS	= -fPIE -DPIE
 LIB_CFLAGS	= -fPIC
 SHARED_FLAGS	= -shared
diff --git a/multipathd/main.c b/multipathd/main.c
index 1406251..3a31154 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3247,7 +3247,8 @@ child (__attribute__((unused)) void *param)
 
 	post_config_state(DAEMON_START);
 
-	condlog(2, "multipathd v%d.%d.%d: start up", MULTIPATH_VERSION(VERSION_CODE));
+	condlog(2, "multipathd v%d.%d.%d%s: start up",
+		MULTIPATH_VERSION(VERSION_CODE), EXTRAVERSION);
 	condlog(3, "read " DEFAULT_CONFIGFILE);
 
 	if (verbosity)
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 02/14] multipathd: don't switch to DAEMON_IDLE during startup

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 991 bytes --]

From: Martin Wilck <mwilck@suse.com>

By switching to DAEMON_IDLE early during startup, we may signal
systemd READY=1 prematurely. Don't do this.

Fixes: c95a739 ("multipathd: avoid sending "READY=1" to systemd on early shutdown")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 3a31154..13b1948 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3307,11 +3307,10 @@ child (__attribute__((unused)) void *param)
 	pthread_cleanup_push(config_cleanup, NULL);
 	pthread_mutex_lock(&config_lock);
 
-	__post_config_state(DAEMON_IDLE);
 	rc = pthread_create(&uxlsnr_thr, &misc_attr, uxlsnrloop, vecs);
 	if (!rc) {
 		/* Wait for uxlsnr startup */
-		while (running_state == DAEMON_IDLE)
+		while (running_state == DAEMON_START)
 			pthread_cond_wait(&config_cond, &config_lock);
 		state = running_state;
 	}
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 03/14] uevent_dispatch(): use while in wait loop

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1098 bytes --]

From: Martin Wilck <mwilck@suse.com>

Callers of pthread_cond_wait() should generally use a while loop
to test the condition. Also, remove the misleading comment.
Condition variables aren't unreliable, they're just not strictly
tied to the condition tested.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 29e2557..fe60ae3 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -443,13 +443,10 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
 		servicing_uev = 0;
-		/*
-		 * Condition signals are unreliable,
-		 * so make sure we only wait if we have to.
-		 */
-		if (list_empty(&uevq)) {
+
+		while (list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
-		}
+
 		servicing_uev = 1;
 		list_splice_init(&uevq, &uevq_tmp);
 		pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 04/14] libmultipath: uevent_dispatch(): process uevents one by one

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 4488 bytes --]

From: Martin Wilck <mwilck@suse.com>

The main rationale for delaying uevents is that the
uevent dispatcher may have to wait for other threads to release the
vecs lock, may the vecs lock for an extended amount of time, and
even sleep occasionally. By delaying them, we have the chance
to accumulate events for the same path device ("filtering") or
WWID ("merging"), thus avoiding duplicate work if we merge these
into one.

A similar effect can be obtained in the uevent dispatcher itself
by looking for new uevents after each dispatched event, and trying
to merge the newly arrived events with those that remained
in the queue.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/list.h   | 29 +++++++++++++++++++++++++++++
 libmultipath/uevent.c | 37 ++++++++++++++++++++-----------------
 2 files changed, 49 insertions(+), 17 deletions(-)

diff --git a/libmultipath/list.h b/libmultipath/list.h
index ced021f..ddea99f 100644
--- a/libmultipath/list.h
+++ b/libmultipath/list.h
@@ -246,6 +246,35 @@ static inline void list_splice_tail_init(struct list_head *list,
 #define list_entry(ptr, type, member) \
 	container_of(ptr, type, member)
 
+
+/**
+ * list_pop - unlink and return the first list element
+ * @head:	the &struct list_head pointer.
+ */
+static inline struct list_head *list_pop(struct list_head *head)
+{
+	struct list_head *tmp;
+
+	if (list_empty(head))
+		return NULL;
+	tmp = head->next;
+	list_del_init(tmp);
+	return tmp;
+}
+
+/**
+ * list_pop_entry - unlink and return the entry of the first list element
+ * @head:	the &struct list_head pointer.
+ * @type:	the type of the struct this is embedded in.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_pop_entry(head, type, member)		\
+({							\
+	struct list_head *__h = list_pop(head);		\
+							\
+	(__h ? container_of(__h, type, member) : NULL);	\
+})
+
 /**
  * list_for_each	-	iterate over a list
  * @pos:	the &struct list_head to use as a loop counter.
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index fe60ae3..602eccb 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -366,6 +366,8 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 				later->action, later->kernel, later->wwid);
 
 			list_move(&earlier->node, &later->merge_node);
+			list_splice_init(&earlier->merge_node,
+					 &later->merge_node);
 		}
 	}
 }
@@ -386,16 +388,15 @@ merge_uevq(struct list_head *tmpq)
 static void
 service_uevq(struct list_head *tmpq)
 {
-	struct uevent *uev, *tmp;
+	struct uevent *uev = list_pop_entry(tmpq, typeof(*uev), node);
 
-	list_for_each_entry_safe(uev, tmp, tmpq, node) {
-		list_del_init(&uev->node);
-
-		pthread_cleanup_push(cleanup_uev, uev);
-		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
-			condlog(0, "uevent trigger error");
-		pthread_cleanup_pop(1);
-	}
+	if (uev == NULL)
+		return;
+	condlog(4, "servicing uevent '%s %s'", uev->action, uev->kernel);
+	pthread_cleanup_push(cleanup_uev, uev);
+	if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
+		condlog(0, "uevent trigger error");
+	pthread_cleanup_pop(1);
 }
 
 static void uevent_cleanup(void *arg)
@@ -432,33 +433,35 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		    void * trigger_data)
 {
+	LIST_HEAD(uevq_work);
+
 	my_uev_trigger = uev_trigger;
 	my_trigger_data = trigger_data;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
+	pthread_cleanup_push(cleanup_uevq, &uevq_work);
 	while (1) {
-		LIST_HEAD(uevq_tmp);
 
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
-		servicing_uev = 0;
 
-		while (list_empty(&uevq))
+		servicing_uev = !list_empty(&uevq_work);
+
+		while (list_empty(&uevq_work) && list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
-		list_splice_init(&uevq, &uevq_tmp);
+		list_splice_tail_init(&uevq, &uevq_work);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		pthread_cleanup_push(cleanup_uevq, &uevq_tmp);
-		merge_uevq(&uevq_tmp);
-		service_uevq(&uevq_tmp);
-		pthread_cleanup_pop(1);
+		merge_uevq(&uevq_work);
+		service_uevq(&uevq_work);
 	}
+	pthread_cleanup_pop(1);
 	condlog(3, "Terminating uev service queue");
 	return 0;
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 05/14] libmultipath: uevent_dispatch(): only filter/merge new uevents

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 7192 bytes --]

From: Martin Wilck <mwilck@suse.com>

When uevq_work is non-empty and we append a list of new events,
we don't need to check the entire list for filterable and mergeable
uevents. uevq_work had been filtered and merged already. So we just
need to check the newly appended events. These must of course be
checked for merges with earlier events, too.

We must deal with some special cases here, like previously merged
uevents being filtered later.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/list.h   | 24 ++++++++++++++
 libmultipath/uevent.c | 77 +++++++++++++++++++++++++++++++++----------
 2 files changed, 83 insertions(+), 18 deletions(-)

diff --git a/libmultipath/list.h b/libmultipath/list.h
index ddea99f..248f72b 100644
--- a/libmultipath/list.h
+++ b/libmultipath/list.h
@@ -363,6 +363,30 @@ static inline struct list_head *list_pop(struct list_head *head)
 	     &pos->member != (head);                                    \
 	     pos = n, n = list_entry(n->member.prev, typeof(*n), member))
 
+/**
+ * list_for_some_entry - iterate list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry(pos, from, to, member)                      \
+	for (pos = list_entry((from)->next, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.next, typeof(*pos), member))
+
+/**
+ * list_for_some_entry_reverse - iterate backwards list from the given begin node to the given end node
+ * @pos:	the type * to use as a loop counter.
+ * @from:	the begin node of the iteration.
+ * @to:		the end node of the iteration.
+ * @member:	the name of the list_struct within the struct.
+ */
+#define list_for_some_entry_reverse(pos, from, to, member)		\
+	for (pos = list_entry((from)->prev, typeof(*pos), member);      \
+	     &pos->member != (to);                                      \
+	     pos = list_entry(pos->member.prev, typeof(*pos), member))
+
 /**
  * list_for_some_entry_safe - iterate list from the given begin node to the given end node safe against removal of list entry
  * @pos:	the type * to use as a loop counter.
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 602eccb..2779703 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -306,17 +306,49 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	return false;
 }
 
+static void uevent_delete_from_list(struct uevent *to_delete,
+				    struct uevent **previous,
+				    struct list_head **old_tail)
+{
+	/*
+	 * "old_tail" is the list_head before the last list element to which
+	 * the caller iterates (the list anchor if the caller iterates over
+	 * the entire list). If this element is removed (which can't happen
+	 * for the anchor), "old_tail" must be moved. It can happen that
+	 * "old_tail" ends up pointing at the anchor.
+	 */
+	if (*old_tail == &to_delete->node)
+		*old_tail = to_delete->node.prev;
+
+	list_del_init(&to_delete->node);
+
+	/*
+	 * The "to_delete" uevent has been merged with other uevents
+	 * previously. Re-insert them into the list, at the point we're
+	 * currently at. This must be done after the list_del_init() above,
+	 * otherwise previous->next would still point to to_delete.
+	 */
+	if (!list_empty(&to_delete->merge_node)) {
+		struct uevent *last = list_entry(to_delete->merge_node.prev,
+						 typeof(*last), node);
+
+		list_splice(&to_delete->merge_node, &(*previous)->node);
+		*previous = last;
+	}
+	if (to_delete->udev)
+		udev_device_unref(to_delete->udev);
+
+	free(to_delete);
+}
+
 static void
-uevent_prepare(struct list_head *tmpq)
+uevent_prepare(struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *uev, *tmp;
 
-	list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) {
+	list_for_some_entry_reverse_safe(uev, tmp, tmpq, *stop, node) {
 		if (uevent_can_discard(uev)) {
-			list_del_init(&uev->node);
-			if (uev->udev)
-				udev_device_unref(uev->udev);
-			free(uev);
+			uevent_delete_from_list(uev, &tmp, stop);
 			continue;
 		}
 
@@ -327,7 +359,7 @@ uevent_prepare(struct list_head *tmpq)
 }
 
 static void
-uevent_filter(struct uevent *later, struct list_head *tmpq)
+uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -341,16 +373,13 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
-			list_del_init(&earlier->node);
-			if (earlier->udev)
-				udev_device_unref(earlier->udev);
-			free(earlier);
+			uevent_delete_from_list(earlier, &tmp, stop);
 		}
 	}
 }
 
 static void
-uevent_merge(struct uevent *later, struct list_head *tmpq)
+uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
 {
 	struct uevent *earlier, *tmp;
 
@@ -365,6 +394,10 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 				earlier->action, earlier->kernel, earlier->wwid,
 				later->action, later->kernel, later->wwid);
 
+			/* See comment in uevent_delete_from_list() */
+			if (&earlier->node == *stop)
+				*stop = earlier->node.prev;
+
 			list_move(&earlier->node, &later->merge_node);
 			list_splice_init(&earlier->merge_node,
 					 &later->merge_node);
@@ -373,15 +406,15 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
 }
 
 static void
-merge_uevq(struct list_head *tmpq)
+merge_uevq(struct list_head *tmpq, struct list_head *stop)
 {
 	struct uevent *later;
 
-	uevent_prepare(tmpq);
-	list_for_each_entry_reverse(later, tmpq, node) {
-		uevent_filter(later, tmpq);
+	uevent_prepare(tmpq, &stop);
+	list_for_some_entry_reverse(later, tmpq, stop, node) {
+		uevent_filter(later, tmpq, &stop);
 		if(uevent_need_merge())
-			uevent_merge(later, tmpq);
+			uevent_merge(later, tmpq, &stop);
 	}
 }
 
@@ -442,6 +475,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 
 	pthread_cleanup_push(cleanup_uevq, &uevq_work);
 	while (1) {
+		struct list_head *stop;
 
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
@@ -452,13 +486,20 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
+		/*
+		 * "stop" is the list element towards which merge_uevq()
+		 * will iterate: the last element of uevq_work before
+		 * appending new uevents. If uveq_is empty, uevq_work.prev
+		 * equals &uevq_work, which is what we need.
+		 */
+		stop = uevq_work.prev;
 		list_splice_tail_init(&uevq, &uevq_work);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		merge_uevq(&uevq_work);
+		merge_uevq(&uevq_work, stop);
 		service_uevq(&uevq_work);
 	}
 	pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 3444 bytes --]

From: Martin Wilck <mwilck@suse.com>

uevent merging by WWID relies on the uid_attrs config option. As we
drop struct config between calls to uevent_merge(), we must be sure
that the WWID is not changed in reconfigure().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/multipath.conf.5 |  2 ++
 multipathd/main.c          | 53 +++++++++++++++++++++++++++++++-------
 2 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 605b46e..a9cd776 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -264,6 +264,8 @@ If this option is configured and matches the device
 node name of a device, it overrides any other configured  methods for
 determining the WWID for this device.
 .PP
+This option cannot be changed during runtime with the multipathd \fBreconfigure\fR command.
+.PP
 The default is: \fB<unset>\fR. To enable uevent merging, set it e.g. to
 \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
 .RE
diff --git a/multipathd/main.c b/multipathd/main.c
index 13b1948..f514b32 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head)
 	free_config(conf);
 }
 
+static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs,
+					const struct _vector *new_attrs)
+{
+	int i;
+	char *old;
+
+	if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
+		return true;
+
+	vector_foreach_slot(old_attrs, old, i) {
+		char *new = VECTOR_SLOT(new_attrs, i);
+
+		if (strcmp(old, new))
+			return true;
+	}
+
+	return false;
+}
+
+static void reconfigure_check(struct config *old, struct config *new)
+{
+	int old_marginal_pathgroups;
+
+	old_marginal_pathgroups = old->marginal_pathgroups;
+	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
+	    (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
+		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
+			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
+			"off" : "on");
+		new->marginal_pathgroups = old_marginal_pathgroups;
+	}
+
+	if (reconfigure_check_uid_attrs(&old->uid_attrs, &new->uid_attrs)) {
+		struct _vector v = new->uid_attrs;
+
+		condlog(1, "multipathd must be restarted to change uid_attrs, keeping old values");
+		new->uid_attrs = old->uid_attrs;
+		vector_reset(&v);
+		old->uid_attrs = v;
+	}
+}
+
 static int
 reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 {
 	struct config * old, *conf;
-	int old_marginal_pathgroups;
 
 	conf = load_config(DEFAULT_CONFIGFILE);
 	if (!conf)
@@ -2870,14 +2911,8 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 	uxsock_timeout = conf->uxsock_timeout;
 
 	old = rcu_dereference(multipath_conf);
-	old_marginal_pathgroups = old->marginal_pathgroups;
-	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
-	    (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
-		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
-			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
-			"off" : "on");
-		conf->marginal_pathgroups = old_marginal_pathgroups;
-	}
+	reconfigure_check(old, conf);
+
 	conf->sequence_nr = old->sequence_nr + 1;
 	rcu_assign_pointer(multipath_conf, conf);
 	call_rcu(&old->rcu, rcu_free_config);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 07/14] libmultipath: microoptimize uevent filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2832 bytes --]

From: Martin Wilck <mwilck@suse.com>

Simplify and microoptimize the filtering and merging tests.
E.g. do the WWID comparison last.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 2779703..0de9c6b 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -220,6 +220,10 @@ static bool
 uevent_can_filter(struct uevent *earlier, struct uevent *later)
 {
 
+	if (!strncmp(later->kernel, "dm-", 3) ||
+	    strcmp(earlier->kernel, later->kernel))
+		return false;
+
 	/*
 	 * filter earlier uvents if path has removed later. Eg:
 	 * "add path1 |chang path1 |add path2 |remove path1"
@@ -227,11 +231,8 @@ uevent_can_filter(struct uevent *earlier, struct uevent *later)
 	 * "add path2 |remove path1"
 	 * uevents "add path1" and "chang path1" are filtered out
 	 */
-	if (!strcmp(earlier->kernel, later->kernel) &&
-		!strcmp(later->action, "remove") &&
-		strncmp(later->kernel, "dm-", 3)) {
+	if (!strcmp(later->action, "remove"))
 		return true;
-	}
 
 	/*
 	 * filter change uvents if add uevents exist. Eg:
@@ -240,12 +241,9 @@ uevent_can_filter(struct uevent *earlier, struct uevent *later)
 	 * "add path1 |add path2"
 	 * uevent "chang path1" is filtered out
 	 */
-	if (!strcmp(earlier->kernel, later->kernel) &&
-		!strcmp(earlier->action, "change") &&
-		!strcmp(later->action, "add") &&
-		strncmp(later->kernel, "dm-", 3)) {
+	if (!strcmp(earlier->action, "change") &&
+	    !strcmp(later->action, "add"))
 		return true;
-	}
 
 	return false;
 }
@@ -278,10 +276,10 @@ merge_need_stop(struct uevent *earlier, struct uevent *later)
 	 * with the same wwid and different action
 	 * it would be better to stop merging.
 	 */
-	if (!strcmp(earlier->wwid, later->wwid) &&
-	    strcmp(earlier->action, later->action) &&
+	if (strcmp(earlier->action, later->action) &&
 	    strcmp(earlier->action, "change") &&
-	    strcmp(later->action, "change"))
+	    strcmp(later->action, "change") &&
+	    !strcmp(earlier->wwid, later->wwid))
 		return true;
 
 	return false;
@@ -296,12 +294,12 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
 	 * and actions are addition or deletion
 	 */
 	if (earlier->wwid && later->wwid &&
-	    !strcmp(earlier->wwid, later->wwid) &&
+	    strncmp(earlier->kernel, "dm-", 3) &&
 	    !strcmp(earlier->action, later->action) &&
-	    strncmp(earlier->action, "change", 6) &&
-	    strncmp(earlier->kernel, "dm-", 3)) {
+	    (!strcmp(earlier->action, "add") ||
+	     !strcmp(earlier->action, "remove")) &&
+	    !strcmp(earlier->wwid, later->wwid))
 		return true;
-	}
 
 	return false;
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 08/14] libmultipath: uevent_listen(): don't delay uevents

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 6620 bytes --]

From: Martin Wilck <mwilck@suse.com>

When multipathd starts up early, basically all devices are added
through uevent processing. This takes much more time than necessary
because of the artificial delays introduced for passing uevents
between the listener and the receiver thread in ee8888f
("multipath-tools: improve processing efficiency for addition and deletion of
multipath devices"). This delay could be up to 30s.

It's generally not a good idea to delay uevent processing in multipathd.
ADD events must normally be handled ASAP in order to avoid maps entering
queueing mode or eventually failing. Handling REMOVE events quickly is
also important to make multipathd aware of deleted devices and keep
kernel and multipathd state in sync.

If uevents arrive quickly, the assumption is that the dispatcher will process
them more slowly than the listener. This was the idea of commit ee8888f,
AFAIU: if a queue of unprocessed events piles up because the dispatcher is
too slow, use filtering and merging to reduce the length of the queue, and
thus the work to be done for the uevent dispatcher, especially the work
that needs to be done while holding the vecs lock. In ee8888f, the
queue was created by allowing uevents to accumulate in the listener.

This patch changes the logic of ee8888f, while keeping the uevent
filtering and discarding features. The idea is that the uevent dispatcher
shouldn't be idle if there are uevents to process. Therefore uevents
are passed to it immediately. But it now checks for new uevents after
processing every individual event, before processing the entire queue,
and it applies filtering and merging to the queue as it grows.

This patch set avoids any delay when the uevent dispatcher is idle or
able to keep up with the rate of incoming uevents, while applying an
increasing amount of filtering and merging as pressure on the uevent
dispatcher increases. It's reasonable to assume that filtering and
merging get more efficient with increasing queue length, because the
probability of finding matching events will increase.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 108 +++++++++++++++---------------------------
 1 file changed, 37 insertions(+), 71 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 0de9c6b..51c912f 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -52,10 +52,6 @@
 #include "blacklist.h"
 #include "devmapper.h"
 
-#define MAX_ACCUMULATION_COUNT 2048
-#define MAX_ACCUMULATION_TIME 30*1000
-#define MIN_BURST_SPEED 10
-
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
 static LIST_HEAD(uevq);
@@ -567,44 +563,43 @@ static struct uevent *uevent_from_udev_device(struct udev_device *dev)
 	return uev;
 }
 
-static bool uevent_burst(struct timeval *start_time, int events)
+#define MAX_UEVENTS 1000
+static int uevent_receive_events(int fd, struct list_head *tmpq,
+				 struct udev_monitor *monitor)
 {
-	struct timeval diff_time, end_time;
-	unsigned long speed;
-	unsigned long eclipse_ms;
+	struct pollfd ev_poll;
+	int n = 0;
 
-	if(events > MAX_ACCUMULATION_COUNT) {
-		condlog(2, "burst got %u uevents, too much uevents, stopped", events);
-		return false;
-	}
+	do {
+		struct uevent *uev;
+		struct udev_device *dev;
 
-	gettimeofday(&end_time, NULL);
-	timersub(&end_time, start_time, &diff_time);
+		dev = udev_monitor_receive_device(monitor);
+		if (!dev) {
+			condlog(0, "failed getting udev device");
+			break;
+		}
+		uev = uevent_from_udev_device(dev);
+		if (!uev)
+			break;
 
-	eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
+		list_add_tail(&uev->node, tmpq);
+		n++;
+		condlog(4, "received uevent \"%s %s\"", uev->action, uev->kernel);
 
-	if (eclipse_ms == 0)
-		return true;
+		ev_poll.fd = fd;
+		ev_poll.events = POLLIN;
 
-	if (eclipse_ms > MAX_ACCUMULATION_TIME) {
-		condlog(2, "burst continued %lu ms, too long time, stopped", eclipse_ms);
-		return false;
-	}
+	} while (n < MAX_UEVENTS && poll(&ev_poll, 1, 0) > 0);
 
-	speed = (events * 1000) / eclipse_ms;
-	if (speed > MIN_BURST_SPEED)
-		return true;
-
-	return false;
+	return n;
 }
 
 int uevent_listen(struct udev *udev)
 {
 	int err = 2;
 	struct udev_monitor *monitor = NULL;
-	int fd, socket_flags, events;
-	struct timeval start_time;
-	int timeout = 30;
+	int fd, socket_flags;
 	LIST_HEAD(uevlisten_tmp);
 
 	/*
@@ -656,59 +651,30 @@ int uevent_listen(struct udev *udev)
 		goto out;
 	}
 
-	events = 0;
-	gettimeofday(&start_time, NULL);
 	pthread_cleanup_push(cleanup_global_uevq, NULL);
 	pthread_cleanup_push(cleanup_uevq, &uevlisten_tmp);
 	while (1) {
-		struct uevent *uev;
-		struct udev_device *dev;
-		struct pollfd ev_poll;
-		int poll_timeout;
-		int fdcount;
+		int fdcount, events;
+		struct pollfd ev_poll = { .fd = fd, .events = POLLIN, };
 
-		memset(&ev_poll, 0, sizeof(struct pollfd));
-		ev_poll.fd = fd;
-		ev_poll.events = POLLIN;
-		poll_timeout = timeout * 1000;
-		errno = 0;
-		fdcount = poll(&ev_poll, 1, poll_timeout);
-		if (fdcount > 0 && ev_poll.revents & POLLIN) {
-			timeout = uevent_burst(&start_time, events + 1) ? 1 : 0;
-			dev = udev_monitor_receive_device(monitor);
-			if (!dev) {
-				condlog(0, "failed getting udev device");
-				continue;
-			}
-			uev = uevent_from_udev_device(dev);
-			if (!uev)
-				continue;
-			list_add_tail(&uev->node, &uevlisten_tmp);
-			events++;
-			continue;
-		}
+		fdcount = poll(&ev_poll, 1, -1);
 		if (fdcount < 0) {
 			if (errno == EINTR)
 				continue;
 
-			condlog(0, "error receiving "
-				"uevent message: %m");
+			condlog(0, "error receiving uevent message: %m");
 			err = -errno;
 			break;
 		}
-		if (!list_empty(&uevlisten_tmp)) {
-			/*
-			 * Queue uevents and poke service pthread.
-			 */
-			condlog(3, "Forwarding %d uevents", events);
-			pthread_mutex_lock(uevq_lockp);
-			list_splice_tail_init(&uevlisten_tmp, &uevq);
-			pthread_cond_signal(uev_condp);
-			pthread_mutex_unlock(uevq_lockp);
-			events = 0;
-		}
-		gettimeofday(&start_time, NULL);
-		timeout = 30;
+		events = uevent_receive_events(fd, &uevlisten_tmp, monitor);
+		if (events <= 0)
+			continue;
+
+		condlog(4, "Forwarding %d uevents", events);
+		pthread_mutex_lock(uevq_lockp);
+		list_splice_tail_init(&uevlisten_tmp, &uevq);
+		pthread_cond_signal(uev_condp);
+		pthread_mutex_unlock(uevq_lockp);
 	}
 	pthread_cleanup_pop(1);
 	pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 09/14] libmultipath: uevent: use struct to pass parameters around

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 11144 bytes --]

From: Martin Wilck <mwilck@suse.com>

libmultipath: uevent_dispatch(): just grab config once

Introduce struct uevent_filter_state to pass parameters around.
This simplifies the function signatures and allows for easy extension
later.

Instead of grabbing multipath config repeatedly, do it just
once per dispatcher iteration, and pass the pointer around in
struct uevent_filter_state. We shouldn't use different configs
for different paths in a single iteration, anyway.

Also, properly constify get_uid_attribute_by_attrs() and
pp->uid_attribute.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/config.c    |   6 +--
 libmultipath/config.h    |   4 +-
 libmultipath/discovery.c |   2 +-
 libmultipath/structs.h   |   2 +-
 libmultipath/uevent.c    | 112 +++++++++++++++++----------------------
 libmultipath/uevent.h    |   3 +-
 tests/uevent.c           |   2 +-
 7 files changed, 59 insertions(+), 72 deletions(-)

diff --git a/libmultipath/config.c b/libmultipath/config.c
index c595e76..7346c37 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -1018,10 +1018,10 @@ out:
 	return 1;
 }
 
-char *get_uid_attribute_by_attrs(struct config *conf,
-				 const char *path_dev)
+const char *get_uid_attribute_by_attrs(const struct config *conf,
+				       const char *path_dev)
 {
-	vector uid_attrs = &conf->uid_attrs;
+	const struct _vector *uid_attrs = &conf->uid_attrs;
 	int j;
 	char *att, *col;
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index c73389b..5351ecd 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -314,7 +314,7 @@ void libmp_put_multipath_config(void *);
 void put_multipath_config(void *);
 
 int parse_uid_attrs(char *uid_attrs, struct config *conf);
-char *get_uid_attribute_by_attrs(struct config *conf,
-				 const char *path_dev);
+const char *get_uid_attribute_by_attrs(const struct config *conf,
+				       const char *path_dev);
 
 #endif
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index b969fba..589eca1 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2044,7 +2044,7 @@ fix_broken_nvme_wwid(struct path *pp, const char *value, size_t size)
 }
 
 static int
-get_udev_uid(struct path * pp, char *uid_attribute, struct udev_device *udev)
+get_udev_uid(struct path * pp, const char *uid_attribute, struct udev_device *udev)
 {
 	ssize_t len;
 	const char *value;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index d94f93a..afd4674 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -327,7 +327,7 @@ struct path {
 	int detect_prio;
 	int detect_checker;
 	int tpgs;
-	char * uid_attribute;
+	const char *uid_attribute;
 	char * getuid;
 	struct prio prio;
 	struct checker checker;
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 51c912f..5f756ce 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -63,6 +63,12 @@ static uev_trigger *my_uev_trigger;
 static void *my_trigger_data;
 static int servicing_uev;
 
+struct uevent_filter_state {
+	struct list_head uevq;
+	struct list_head *old_tail;
+	struct config *conf;
+};
+
 int is_uevent_busy(void)
 {
 	int empty;
@@ -158,40 +164,24 @@ int uevent_get_env_positive_int(const struct uevent *uev,
 }
 
 void
-uevent_get_wwid(struct uevent *uev)
+uevent_get_wwid(struct uevent *uev, const struct config *conf)
 {
-	char *uid_attribute;
+	const char *uid_attribute;
 	const char *val;
-	struct config * conf;
 
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
 	uid_attribute = get_uid_attribute_by_attrs(conf, uev->kernel);
-	pthread_cleanup_pop(1);
-
 	val = uevent_get_env_var(uev, uid_attribute);
 	if (val)
 		uev->wwid = val;
 }
 
-static bool uevent_need_merge(void)
+static bool uevent_need_merge(const struct config *conf)
 {
-	struct config * conf;
-	bool need_merge = false;
-
-	conf = get_multipath_config();
-	if (VECTOR_SIZE(&conf->uid_attrs) > 0)
-		need_merge = true;
-	put_multipath_config(conf);
-
-	return need_merge;
+	return VECTOR_SIZE(&conf->uid_attrs) > 0;
 }
 
-static bool uevent_can_discard(struct uevent *uev)
+static bool uevent_can_discard(struct uevent *uev, const struct config *conf)
 {
-	int invalid = 0;
-	struct config * conf;
-
 	/*
 	 * do not filter dm devices by devnode
 	 */
@@ -200,15 +190,10 @@ static bool uevent_can_discard(struct uevent *uev)
 	/*
 	 * filter paths devices by devnode
 	 */
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
 	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
 			   uev->kernel) > 0)
-		invalid = 1;
-	pthread_cleanup_pop(1);
-
-	if (invalid)
 		return true;
+
 	return false;
 }
 
@@ -335,29 +320,28 @@ static void uevent_delete_from_list(struct uevent *to_delete,
 	free(to_delete);
 }
 
-static void
-uevent_prepare(struct list_head *tmpq, struct list_head **stop)
+static void uevent_prepare(struct uevent_filter_state *st)
 {
 	struct uevent *uev, *tmp;
 
-	list_for_some_entry_reverse_safe(uev, tmp, tmpq, *stop, node) {
-		if (uevent_can_discard(uev)) {
-			uevent_delete_from_list(uev, &tmp, stop);
+	list_for_some_entry_reverse_safe(uev, tmp, &st->uevq, st->old_tail, node) {
+		if (uevent_can_discard(uev, st->conf)) {
+			uevent_delete_from_list(uev, &tmp, &st->old_tail);
 			continue;
 		}
 
 		if (strncmp(uev->kernel, "dm-", 3) &&
-		    uevent_need_merge())
-			uevent_get_wwid(uev);
+		    uevent_need_merge(st->conf))
+			uevent_get_wwid(uev, st->conf);
 	}
 }
 
 static void
-uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
+uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 {
 	struct uevent *earlier, *tmp;
 
-	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, &st->uevq, node) {
 		/*
 		 * filter unnessary earlier uevents
 		 * by the later uevent
@@ -367,17 +351,16 @@ uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **s
 				earlier->kernel, earlier->action,
 				later->kernel, later->action);
 
-			uevent_delete_from_list(earlier, &tmp, stop);
+			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
 		}
 	}
 }
 
-static void
-uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
+static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 {
 	struct uevent *earlier, *tmp;
 
-	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, tmpq, node) {
+	list_for_some_entry_reverse_safe(earlier, tmp, &later->node, &st->uevq, node) {
 		if (merge_need_stop(earlier, later))
 			break;
 		/*
@@ -389,8 +372,8 @@ uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **st
 				later->action, later->kernel, later->wwid);
 
 			/* See comment in uevent_delete_from_list() */
-			if (&earlier->node == *stop)
-				*stop = earlier->node.prev;
+			if (&earlier->node == st->old_tail)
+				st->old_tail = earlier->node.prev;
 
 			list_move(&earlier->node, &later->merge_node);
 			list_splice_init(&earlier->merge_node,
@@ -399,16 +382,15 @@ uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **st
 	}
 }
 
-static void
-merge_uevq(struct list_head *tmpq, struct list_head *stop)
+static void merge_uevq(struct uevent_filter_state *st)
 {
 	struct uevent *later;
 
-	uevent_prepare(tmpq, &stop);
-	list_for_some_entry_reverse(later, tmpq, stop, node) {
-		uevent_filter(later, tmpq, &stop);
-		if(uevent_need_merge())
-			uevent_merge(later, tmpq, &stop);
+	uevent_prepare(st);
+	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node) {
+		uevent_filter(later, st);
+		if(uevent_need_merge(st->conf))
+			uevent_merge(later, st);
 	}
 }
 
@@ -460,41 +442,45 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		    void * trigger_data)
 {
-	LIST_HEAD(uevq_work);
+	struct uevent_filter_state filter_state;
 
+	INIT_LIST_HEAD(&filter_state.uevq);
 	my_uev_trigger = uev_trigger;
 	my_trigger_data = trigger_data;
 
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 
-	pthread_cleanup_push(cleanup_uevq, &uevq_work);
+	pthread_cleanup_push(cleanup_uevq, &filter_state.uevq);
 	while (1) {
-		struct list_head *stop;
-
 		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
 		pthread_mutex_lock(uevq_lockp);
 
-		servicing_uev = !list_empty(&uevq_work);
+		servicing_uev = !list_empty(&filter_state.uevq);
 
-		while (list_empty(&uevq_work) && list_empty(&uevq))
+		while (list_empty(&filter_state.uevq) && list_empty(&uevq))
 			pthread_cond_wait(uev_condp, uevq_lockp);
 
 		servicing_uev = 1;
 		/*
-		 * "stop" is the list element towards which merge_uevq()
-		 * will iterate: the last element of uevq_work before
-		 * appending new uevents. If uveq_is empty, uevq_work.prev
-		 * equals &uevq_work, which is what we need.
+		 * "old_tail" is the list element towards which merge_uevq()
+		 * will iterate: the last element of uevq before
+		 * appending new uevents. If uveq  empty, uevq.prev
+		 * equals &uevq, which is what we need.
 		 */
-		stop = uevq_work.prev;
-		list_splice_tail_init(&uevq, &uevq_work);
+		filter_state.old_tail = filter_state.uevq.prev;
+		list_splice_tail_init(&uevq, &filter_state.uevq);
 		pthread_cleanup_pop(1);
 
 		if (!my_uev_trigger)
 			break;
 
-		merge_uevq(&uevq_work, stop);
-		service_uevq(&uevq_work);
+
+		pthread_cleanup_push(put_multipath_config, filter_state.conf);
+		filter_state.conf = get_multipath_config();
+		merge_uevq(&filter_state);
+		pthread_cleanup_pop(1);
+
+		service_uevq(&filter_state.uevq);
 	}
 	pthread_cleanup_pop(1);
 	condlog(3, "Terminating uev service queue");
diff --git a/libmultipath/uevent.h b/libmultipath/uevent.h
index 61ca1b5..53a7ca2 100644
--- a/libmultipath/uevent.h
+++ b/libmultipath/uevent.h
@@ -10,6 +10,7 @@
 #define OBJECT_SIZE			512
 
 struct udev;
+struct config;
 
 struct uevent {
 	struct list_head node;
@@ -31,7 +32,7 @@ int uevent_listen(struct udev *udev);
 int uevent_dispatch(int (*store_uev)(struct uevent *, void * trigger_data),
 		    void * trigger_data);
 bool uevent_is_mpath(const struct uevent *uev);
-void uevent_get_wwid(struct uevent *uev);
+void uevent_get_wwid(struct uevent *uev, const struct config *conf);
 
 int uevent_get_env_positive_int(const struct uevent *uev,
 				const char *attr);
diff --git a/tests/uevent.c b/tests/uevent.c
index 7523fec..6a010ab 100644
--- a/tests/uevent.c
+++ b/tests/uevent.c
@@ -111,7 +111,7 @@ static void test_uid_attrs(void **state)
 static void test_wwid(void **state)
 {
 	struct uevent *uev = *state;
-	uevent_get_wwid(uev);
+	uevent_get_wwid(uev, &conf);
 
 	assert_string_equal(uev->wwid, WWID);
 }
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 10/14] libmultipath: uevent: log statistics about filtering and merging

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 3458 bytes --]

From: Martin Wilck <mwilck@suse.com>

also, log sleep/wakeup in uevent_dispatch().

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 5f756ce..c49171f 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -67,8 +67,17 @@ struct uevent_filter_state {
 	struct list_head uevq;
 	struct list_head *old_tail;
 	struct config *conf;
+	unsigned long added;
+	unsigned long discarded;
+	unsigned long filtered;
+	unsigned long merged;
 };
 
+static void reset_filter_state(struct uevent_filter_state *st)
+{
+	st->added = st->discarded = st->filtered = st->merged = 0;
+}
+
 int is_uevent_busy(void)
 {
 	int empty;
@@ -311,6 +320,8 @@ static void uevent_delete_from_list(struct uevent *to_delete,
 		struct uevent *last = list_entry(to_delete->merge_node.prev,
 						 typeof(*last), node);
 
+		condlog(3, "%s: deleted uevent \"%s %s\" with merged uevents",
+			__func__, to_delete->action, to_delete->kernel);
 		list_splice(&to_delete->merge_node, &(*previous)->node);
 		*previous = last;
 	}
@@ -325,8 +336,11 @@ static void uevent_prepare(struct uevent_filter_state *st)
 	struct uevent *uev, *tmp;
 
 	list_for_some_entry_reverse_safe(uev, tmp, &st->uevq, st->old_tail, node) {
+
+		st->added++;
 		if (uevent_can_discard(uev, st->conf)) {
 			uevent_delete_from_list(uev, &tmp, &st->old_tail);
+			st->discarded++;
 			continue;
 		}
 
@@ -352,6 +366,7 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 				later->kernel, later->action);
 
 			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
+			st->filtered++;
 		}
 	}
 }
@@ -378,6 +393,7 @@ static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 			list_move(&earlier->node, &later->merge_node);
 			list_splice_init(&earlier->merge_node,
 					 &later->merge_node);
+			st->merged++;
 		}
 	}
 }
@@ -436,6 +452,15 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
 	pthread_mutex_unlock(uevq_lockp);
 }
 
+static void log_filter_state(const struct uevent_filter_state *st)
+{
+	if (st->added == 0 && st->filtered == 0 && st->merged == 0)
+		return;
+
+	condlog(3, "uevents: %lu added, %lu discarded, %lu filtered, %lu merged",
+		st->added, st->discarded, st->filtered, st->merged);
+}
+
 /*
  * Service the uevent queue.
  */
@@ -457,8 +482,11 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 
 		servicing_uev = !list_empty(&filter_state.uevq);
 
-		while (list_empty(&filter_state.uevq) && list_empty(&uevq))
+		while (list_empty(&filter_state.uevq) && list_empty(&uevq)) {
+			condlog(4, "%s: waiting for events", __func__);
 			pthread_cond_wait(uev_condp, uevq_lockp);
+			condlog(4, "%s: waking up", __func__);
+		}
 
 		servicing_uev = 1;
 		/*
@@ -474,11 +502,12 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 		if (!my_uev_trigger)
 			break;
 
-
+		reset_filter_state(&filter_state);
 		pthread_cleanup_push(put_multipath_config, filter_state.conf);
 		filter_state.conf = get_multipath_config();
 		merge_uevq(&filter_state);
 		pthread_cleanup_pop(1);
+		log_filter_state(&filter_state);
 
 		service_uevq(&filter_state.uevq);
 	}
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 11/14] libmultipath: merge_uevq(): filter first, then merge

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1106 bytes --]

From: Martin Wilck <mwilck@suse.com>

Run filtering over the entire list before merging.
Otherwise it can happen that the filter (which is more efficient
at saving work) doesn't find events to drop, because they have been
merged. Also, the the merging logic needs to be run for less events.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index c49171f..eb900ec 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -403,11 +403,13 @@ static void merge_uevq(struct uevent_filter_state *st)
 	struct uevent *later;
 
 	uevent_prepare(st);
-	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node) {
+
+	list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node)
 		uevent_filter(later, st);
-		if(uevent_need_merge(st->conf))
+
+	if(uevent_need_merge(st->conf))
+		list_for_some_entry_reverse(later, &st->uevq, st->old_tail, node)
 			uevent_merge(later, st);
-	}
 }
 
 static void
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 12/14] libmultipath: uevent_filter(): filter previously merged events

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1631 bytes --]

From: Martin Wilck <mwilck@suse.com>

With the new list-appending logic, it can happen that previously
merged events can now be filtered. Do it.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/uevent.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index eb900ec..809c74c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -305,7 +305,7 @@ static void uevent_delete_from_list(struct uevent *to_delete,
 	 * for the anchor), "old_tail" must be moved. It can happen that
 	 * "old_tail" ends up pointing at the anchor.
 	 */
-	if (*old_tail == &to_delete->node)
+	if (old_tail && *old_tail == &to_delete->node)
 		*old_tail = to_delete->node.prev;
 
 	list_del_init(&to_delete->node);
@@ -360,6 +360,20 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 		 * filter unnessary earlier uevents
 		 * by the later uevent
 		 */
+		if (!list_empty(&earlier->merge_node)) {
+			struct uevent *mn, *t;
+
+			list_for_each_entry_reverse_safe(mn, t, &earlier->merge_node, node) {
+				if (uevent_can_filter(mn, later)) {
+					condlog(4, "uevent: \"%s %s\" (merged into \"%s %s\") filtered by \"%s %s\"",
+						mn->action, mn->kernel,
+						earlier->action, earlier->kernel,
+						later->action, later->kernel);
+					uevent_delete_from_list(mn, &t, NULL);
+					st->filtered++;
+				}
+			}
+		}
 		if (uevent_can_filter(earlier, later)) {
 			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
 				earlier->kernel, earlier->action,
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 13/14] libmultipath: uevent: improve log messages

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 1468 bytes --]

From: Martin Wilck <mwilck@suse.com>

Use common format "\add sdl\", and log this detail only at
verbosity 4.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index 809c74c..a5c2f3c 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -375,9 +375,9 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
 			}
 		}
 		if (uevent_can_filter(earlier, later)) {
-			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
-				earlier->kernel, earlier->action,
-				later->kernel, later->action);
+			condlog(4, "uevent: \"%s %s\" filtered by \"%s %s\"",
+				earlier->action, earlier->kernel,
+				later->action, later->kernel);
 
 			uevent_delete_from_list(earlier, &tmp, &st->old_tail);
 			st->filtered++;
@@ -396,8 +396,8 @@ static void uevent_merge(struct uevent *later, struct uevent_filter_state *st)
 		 * merge earlier uevents to the later uevent
 		 */
 		if (uevent_can_merge(earlier, later)) {
-			condlog(3, "merged uevent: %s-%s-%s with uevent: %s-%s-%s",
-				earlier->action, earlier->kernel, earlier->wwid,
+			condlog(4, "uevent: \"%s %s\" merged with \"%s %s\" for WWID %s",
+				earlier->action, earlier->kernel,
 				later->action, later->kernel, later->wwid);
 
 			/* See comment in uevent_delete_from_list() */
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] [PATCH 14/14] libmultipath: uevent: add debug messages for event queue

[mwilck]

[-- Attachment #1: Type: application/octet-stream, Size: 2226 bytes --]

From: Martin Wilck <mwilck@suse.com>

For debugging, it's useful to be able to track the event queue
in detail. Enable this at verbosity level 4.

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

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index a5c2f3c..c276314 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -51,6 +51,7 @@
 #include "config.h"
 #include "blacklist.h"
 #include "devmapper.h"
+#include "strbuf.h"
 
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
@@ -426,6 +427,40 @@ static void merge_uevq(struct uevent_filter_state *st)
 			uevent_merge(later, st);
 }
 
+static void print_uev(struct strbuf *buf, struct uevent *uev)
+{
+	print_strbuf(buf, "\"%s %s\"", uev->action, uev->kernel);
+	if (!list_empty(&uev->merge_node)) {
+		struct uevent *u;
+
+		append_strbuf_str(buf, "[");
+		list_for_each_entry(u, &uev->merge_node, node)
+			print_strbuf(buf, "\"%s %s \"", u->action, u->kernel);
+		append_strbuf_str(buf, "]");
+	}
+	append_strbuf_str(buf, " ");
+}
+
+static void print_uevq(const char *msg, struct list_head *uevq)
+{
+	struct uevent *uev;
+	int i = 0;
+	STRBUF_ON_STACK(buf);
+
+	if (4 > MAX_VERBOSITY || 4 > libmp_verbosity)
+		return;
+
+	if (list_empty(uevq))
+		append_strbuf_str(&buf, "*empty*");
+	else
+		list_for_each_entry(uev, uevq, node) {
+			print_strbuf(&buf, "%d:", i++);
+			print_uev(&buf, uev);
+		}
+
+	condlog(4, "uevent queue (%s): %s", msg, steal_strbuf_str(&buf));
+}
+
 static void
 service_uevq(struct list_head *tmpq)
 {
@@ -520,11 +555,13 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
 
 		reset_filter_state(&filter_state);
 		pthread_cleanup_push(put_multipath_config, filter_state.conf);
+		print_uevq("append", &filter_state.uevq);
 		filter_state.conf = get_multipath_config();
 		merge_uevq(&filter_state);
 		pthread_cleanup_pop(1);
 		log_filter_state(&filter_state);
 
+		print_uevq("merge", &filter_state.uevq);
 		service_uevq(&filter_state.uevq);
 	}
 	pthread_cleanup_pop(1);
-- 
2.35.1


[-- Attachment #2: Type: text/plain, Size: 98 bytes --]

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

[dm-devel] ZTE / Tang Junhui (Re: [PATCH 00/14] Rework uevent filtering and merging)

[Martin Wilck]

On Thu, 2022-03-31 at 00:14 +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Hi Ben, hi Christophe, hi Tang Junhui,

it turns out that Tang Junhui's email address bounces.
Adding ZTE people to this email that used to be on the distribution
list before.

If anyone can point out someone who might have taken Tang Junhui's
responsibilities, please let me know.

Thanks,
Martin

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

Re: [dm-devel] [PATCH 04/14] libmultipath: uevent_dispatch(): process uevents one by one

[Benjamin Marzinski]

> On Thu, Mar 31, 2022 at 12:15:00AM +0200, mwilck@suse.com wrote:
> The main rationale for delaying uevents is that the
> uevent dispatcher may have to wait for other threads to release the
> vecs lock, may the vecs lock for an extended amount of time, and
> even sleep occasionally. By delaying them, we have the chance
> to accumulate events for the same path device ("filtering") or
> WWID ("merging"), thus avoiding duplicate work if we merge these
> into one.
> 
> A similar effect can be obtained in the uevent dispatcher itself
> by looking for new uevents after each dispatched event, and trying
> to merge the newly arrived events with those that remained
> in the queue.

I'm not sure how I feel about this commit existing on its own.
uevent_filter() can leak memory if it deletes a uevent that already has
merged uevents.  I see that you fixed this in your next patch.  The
question is, does the reviewing simplification from splitting the
patches make up for adding an error in this one.  If you want to leave
it in as its own commit, you should probably flag that it creates a
memory leak that will be fixed in a future patch.

-Ben
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/list.h   | 29 +++++++++++++++++++++++++++++
>  libmultipath/uevent.c | 37 ++++++++++++++++++++-----------------
>  2 files changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/libmultipath/list.h b/libmultipath/list.h
> index ced021f..ddea99f 100644
> --- a/libmultipath/list.h
> +++ b/libmultipath/list.h
> @@ -246,6 +246,35 @@ static inline void list_splice_tail_init(struct list_head *list,
>  #define list_entry(ptr, type, member) \
>  	container_of(ptr, type, member)
>  
> +
> +/**
> + * list_pop - unlink and return the first list element
> + * @head:	the &struct list_head pointer.
> + */
> +static inline struct list_head *list_pop(struct list_head *head)
> +{
> +	struct list_head *tmp;
> +
> +	if (list_empty(head))
> +		return NULL;
> +	tmp = head->next;
> +	list_del_init(tmp);
> +	return tmp;
> +}
> +
> +/**
> + * list_pop_entry - unlink and return the entry of the first list element
> + * @head:	the &struct list_head pointer.
> + * @type:	the type of the struct this is embedded in.
> + * @member:	the name of the list_struct within the struct.
> + */
> +#define list_pop_entry(head, type, member)		\
> +({							\
> +	struct list_head *__h = list_pop(head);		\
> +							\
> +	(__h ? container_of(__h, type, member) : NULL);	\
> +})
> +
>  /**
>   * list_for_each	-	iterate over a list
>   * @pos:	the &struct list_head to use as a loop counter.
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index fe60ae3..602eccb 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -366,6 +366,8 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
>  				later->action, later->kernel, later->wwid);
>  
>  			list_move(&earlier->node, &later->merge_node);
> +			list_splice_init(&earlier->merge_node,
> +					 &later->merge_node);
>  		}
>  	}
>  }
> @@ -386,16 +388,15 @@ merge_uevq(struct list_head *tmpq)
>  static void
>  service_uevq(struct list_head *tmpq)
>  {
> -	struct uevent *uev, *tmp;
> +	struct uevent *uev = list_pop_entry(tmpq, typeof(*uev), node);
>  
> -	list_for_each_entry_safe(uev, tmp, tmpq, node) {
> -		list_del_init(&uev->node);
> -
> -		pthread_cleanup_push(cleanup_uev, uev);
> -		if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
> -			condlog(0, "uevent trigger error");
> -		pthread_cleanup_pop(1);
> -	}
> +	if (uev == NULL)
> +		return;
> +	condlog(4, "servicing uevent '%s %s'", uev->action, uev->kernel);
> +	pthread_cleanup_push(cleanup_uev, uev);
> +	if (my_uev_trigger && my_uev_trigger(uev, my_trigger_data))
> +		condlog(0, "uevent trigger error");
> +	pthread_cleanup_pop(1);
>  }
>  
>  static void uevent_cleanup(void *arg)
> @@ -432,33 +433,35 @@ static void cleanup_global_uevq(void *arg __attribute__((unused)))
>  int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>  		    void * trigger_data)
>  {
> +	LIST_HEAD(uevq_work);
> +
>  	my_uev_trigger = uev_trigger;
>  	my_trigger_data = trigger_data;
>  
>  	mlockall(MCL_CURRENT | MCL_FUTURE);
>  
> +	pthread_cleanup_push(cleanup_uevq, &uevq_work);
>  	while (1) {
> -		LIST_HEAD(uevq_tmp);
>  
>  		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
>  		pthread_mutex_lock(uevq_lockp);
> -		servicing_uev = 0;
>  
> -		while (list_empty(&uevq))
> +		servicing_uev = !list_empty(&uevq_work);
> +
> +		while (list_empty(&uevq_work) && list_empty(&uevq))
>  			pthread_cond_wait(uev_condp, uevq_lockp);
>  
>  		servicing_uev = 1;
> -		list_splice_init(&uevq, &uevq_tmp);
> +		list_splice_tail_init(&uevq, &uevq_work);
>  		pthread_cleanup_pop(1);
>  
>  		if (!my_uev_trigger)
>  			break;
>  
> -		pthread_cleanup_push(cleanup_uevq, &uevq_tmp);
> -		merge_uevq(&uevq_tmp);
> -		service_uevq(&uevq_tmp);
> -		pthread_cleanup_pop(1);
> +		merge_uevq(&uevq_work);
> +		service_uevq(&uevq_work);
>  	}
> +	pthread_cleanup_pop(1);
>  	condlog(3, "Terminating uev service queue");
>  	return 0;
>  }
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 05/14] libmultipath: uevent_dispatch(): only filter/merge new uevents

[Benjamin Marzinski]

> On Thu, Mar 31, 2022 at 12:15:01AM +0200, mwilck@suse.com wrote:
> When uevq_work is non-empty and we append a list of new events,
> we don't need to check the entire list for filterable and mergeable
> uevents. uevq_work had been filtered and merged already. So we just
> need to check the newly appended events. These must of course be
> checked for merges with earlier events, too.
> 
> We must deal with some special cases here, like previously merged
> uevents being filtered later.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/list.h   | 24 ++++++++++++++
>  libmultipath/uevent.c | 77 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 83 insertions(+), 18 deletions(-)
> 
> diff --git a/libmultipath/list.h b/libmultipath/list.h
> index ddea99f..248f72b 100644
> --- a/libmultipath/list.h
> +++ b/libmultipath/list.h
> @@ -363,6 +363,30 @@ static inline struct list_head *list_pop(struct list_head *head)
>  	     &pos->member != (head);                                    \
>  	     pos = n, n = list_entry(n->member.prev, typeof(*n), member))
>  
> +/**
> + * list_for_some_entry - iterate list from the given begin node to the given end node
> + * @pos:	the type * to use as a loop counter.
> + * @from:	the begin node of the iteration.
> + * @to:		the end node of the iteration.
> + * @member:	the name of the list_struct within the struct.
> + */
> +#define list_for_some_entry(pos, from, to, member)                      \
> +	for (pos = list_entry((from)->next, typeof(*pos), member);      \
> +	     &pos->member != (to);                                      \
> +	     pos = list_entry(pos->member.next, typeof(*pos), member))
> +
> +/**
> + * list_for_some_entry_reverse - iterate backwards list from the given begin node to the given end node
> + * @pos:	the type * to use as a loop counter.
> + * @from:	the begin node of the iteration.
> + * @to:		the end node of the iteration.
> + * @member:	the name of the list_struct within the struct.
> + */
> +#define list_for_some_entry_reverse(pos, from, to, member)		\
> +	for (pos = list_entry((from)->prev, typeof(*pos), member);      \
> +	     &pos->member != (to);                                      \
> +	     pos = list_entry(pos->member.prev, typeof(*pos), member))
> +
>  /**
>   * list_for_some_entry_safe - iterate list from the given begin node to the given end node safe against removal of list entry
>   * @pos:	the type * to use as a loop counter.
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index 602eccb..2779703 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -306,17 +306,49 @@ uevent_can_merge(struct uevent *earlier, struct uevent *later)
>  	return false;
>  }
>  
> +static void uevent_delete_from_list(struct uevent *to_delete,
> +				    struct uevent **previous,
> +				    struct list_head **old_tail)
> +{
> +	/*
> +	 * "old_tail" is the list_head before the last list element to which
> +	 * the caller iterates (the list anchor if the caller iterates over
> +	 * the entire list). If this element is removed (which can't happen
> +	 * for the anchor), "old_tail" must be moved. It can happen that
> +	 * "old_tail" ends up pointing at the anchor.
> +	 */
> +	if (*old_tail == &to_delete->node)
> +		*old_tail = to_delete->node.prev;
> +
> +	list_del_init(&to_delete->node);
> +
> +	/*
> +	 * The "to_delete" uevent has been merged with other uevents
> +	 * previously. Re-insert them into the list, at the point we're
> +	 * currently at. This must be done after the list_del_init() above,
> +	 * otherwise previous->next would still point to to_delete.
> +	 */
> +	if (!list_empty(&to_delete->merge_node)) {
> +		struct uevent *last = list_entry(to_delete->merge_node.prev,
> +						 typeof(*last), node);
> +
> +		list_splice(&to_delete->merge_node, &(*previous)->node);
> +		*previous = last;
> +	}
> +	if (to_delete->udev)
> +		udev_device_unref(to_delete->udev);
> +
> +	free(to_delete);
> +}
> +
>  static void
> -uevent_prepare(struct list_head *tmpq)
> +uevent_prepare(struct list_head *tmpq, struct list_head **stop)
>  {
>  	struct uevent *uev, *tmp;
>  
> -	list_for_each_entry_reverse_safe(uev, tmp, tmpq, node) {
> +	list_for_some_entry_reverse_safe(uev, tmp, tmpq, *stop, node) {
>  		if (uevent_can_discard(uev)) {
> -			list_del_init(&uev->node);
> -			if (uev->udev)
> -				udev_device_unref(uev->udev);
> -			free(uev);
> +			uevent_delete_from_list(uev, &tmp, stop);

You are only running this on new events, so they can't possibly have any
merged uevents, and can't possibly be equal to stop, so the old, simple,
deleting code works fine here, and you can just pass stop as a regular
pointer, right? Or is this tricker than I understand?

-Ben

>  			continue;
>  		}
>  
> @@ -327,7 +359,7 @@ uevent_prepare(struct list_head *tmpq)
>  }
>  
>  static void
> -uevent_filter(struct uevent *later, struct list_head *tmpq)
> +uevent_filter(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
>  {
>  	struct uevent *earlier, *tmp;
>  
> @@ -341,16 +373,13 @@ uevent_filter(struct uevent *later, struct list_head *tmpq)
>  				earlier->kernel, earlier->action,
>  				later->kernel, later->action);
>  
> -			list_del_init(&earlier->node);
> -			if (earlier->udev)
> -				udev_device_unref(earlier->udev);
> -			free(earlier);
> +			uevent_delete_from_list(earlier, &tmp, stop);
>  		}
>  	}
>  }
>  
>  static void
> -uevent_merge(struct uevent *later, struct list_head *tmpq)
> +uevent_merge(struct uevent *later, struct list_head *tmpq, struct list_head **stop)
>  {
>  	struct uevent *earlier, *tmp;
>  
> @@ -365,6 +394,10 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
>  				earlier->action, earlier->kernel, earlier->wwid,
>  				later->action, later->kernel, later->wwid);
>  
> +			/* See comment in uevent_delete_from_list() */
> +			if (&earlier->node == *stop)
> +				*stop = earlier->node.prev;
> +
>  			list_move(&earlier->node, &later->merge_node);
>  			list_splice_init(&earlier->merge_node,
>  					 &later->merge_node);
> @@ -373,15 +406,15 @@ uevent_merge(struct uevent *later, struct list_head *tmpq)
>  }
>  
>  static void
> -merge_uevq(struct list_head *tmpq)
> +merge_uevq(struct list_head *tmpq, struct list_head *stop)
>  {
>  	struct uevent *later;
>  
> -	uevent_prepare(tmpq);
> -	list_for_each_entry_reverse(later, tmpq, node) {
> -		uevent_filter(later, tmpq);
> +	uevent_prepare(tmpq, &stop);
> +	list_for_some_entry_reverse(later, tmpq, stop, node) {
> +		uevent_filter(later, tmpq, &stop);
>  		if(uevent_need_merge())
> -			uevent_merge(later, tmpq);
> +			uevent_merge(later, tmpq, &stop);
>  	}
>  }
>  
> @@ -442,6 +475,7 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>  
>  	pthread_cleanup_push(cleanup_uevq, &uevq_work);
>  	while (1) {
> +		struct list_head *stop;
>  
>  		pthread_cleanup_push(cleanup_mutex, uevq_lockp);
>  		pthread_mutex_lock(uevq_lockp);
> @@ -452,13 +486,20 @@ int uevent_dispatch(int (*uev_trigger)(struct uevent *, void * trigger_data),
>  			pthread_cond_wait(uev_condp, uevq_lockp);
>  
>  		servicing_uev = 1;
> +		/*
> +		 * "stop" is the list element towards which merge_uevq()
> +		 * will iterate: the last element of uevq_work before
> +		 * appending new uevents. If uveq_is empty, uevq_work.prev
> +		 * equals &uevq_work, which is what we need.
> +		 */
> +		stop = uevq_work.prev;
>  		list_splice_tail_init(&uevq, &uevq_work);
>  		pthread_cleanup_pop(1);
>  
>  		if (!my_uev_trigger)
>  			break;
>  
> -		merge_uevq(&uevq_work);
> +		merge_uevq(&uevq_work, stop);
>  		service_uevq(&uevq_work);
>  	}
>  	pthread_cleanup_pop(1);
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs

[Benjamin Marzinski]

> On Thu, Mar 31, 2022 at 12:15:02AM +0200, mwilck@suse.com wrote:
> uevent merging by WWID relies on the uid_attrs config option. As we
> drop struct config between calls to uevent_merge(), we must be sure
> that the WWID is not changed in reconfigure().
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/multipath.conf.5 |  2 ++
>  multipathd/main.c          | 53 +++++++++++++++++++++++++++++++-------
>  2 files changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 605b46e..a9cd776 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -264,6 +264,8 @@ If this option is configured and matches the device
>  node name of a device, it overrides any other configured  methods for
>  determining the WWID for this device.
>  .PP
> +This option cannot be changed during runtime with the multipathd \fBreconfigure\fR command.
> +.PP
>  The default is: \fB<unset>\fR. To enable uevent merging, set it e.g. to
>  \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
>  .RE
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 13b1948..f514b32 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head)
>  	free_config(conf);
>  }
>  
> +static bool reconfigure_check_uid_attrs(const struct _vector *old_attrs,
> +					const struct _vector *new_attrs)
> +{
> +	int i;
> +	char *old;
> +
> +	if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
> +		return true;
> +
> +	vector_foreach_slot(old_attrs, old, i) {
> +		char *new = VECTOR_SLOT(new_attrs, i);
> +
> +		if (strcmp(old, new))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +static void reconfigure_check(struct config *old, struct config *new)
> +{
> +	int old_marginal_pathgroups;
> +
> +	old_marginal_pathgroups = old->marginal_pathgroups;
> +	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> +	    (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
> +		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
> +			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
> +			"off" : "on");
> +		new->marginal_pathgroups = old_marginal_pathgroups;
> +	}
> +
> +	if (reconfigure_check_uid_attrs(&old->uid_attrs, &new->uid_attrs)) {
> +		struct _vector v = new->uid_attrs;
> +
> +		condlog(1, "multipathd must be restarted to change uid_attrs, keeping old values");
> +		new->uid_attrs = old->uid_attrs;
> +		vector_reset(&v);

This leaks the strings that v points to, right?  This also can happen in
uid_attrs_handler(), I just noticed.

-Ben

> +		old->uid_attrs = v;
> +	}
> +}
> +
>  static int
>  reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
>  {
>  	struct config * old, *conf;
> -	int old_marginal_pathgroups;
>  
>  	conf = load_config(DEFAULT_CONFIGFILE);
>  	if (!conf)
> @@ -2870,14 +2911,8 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
>  	uxsock_timeout = conf->uxsock_timeout;
>  
>  	old = rcu_dereference(multipath_conf);
> -	old_marginal_pathgroups = old->marginal_pathgroups;
> -	if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> -	    (conf->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)) {
> -		condlog(1, "multipathd must be restarted to turn %s fpin marginal paths",
> -			(old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN)?
> -			"off" : "on");
> -		conf->marginal_pathgroups = old_marginal_pathgroups;
> -	}
> +	reconfigure_check(old, conf);
> +
>  	conf->sequence_nr = old->sequence_nr + 1;
>  	rcu_assign_pointer(multipath_conf, conf);
>  	call_rcu(&old->rcu, rcu_free_config);
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 12/14] libmultipath: uevent_filter(): filter previously merged events

[Benjamin Marzinski]

 On Thu, Mar 31, 2022 at 12:15:08AM +0200, mwilck@suse.com wrote:
> With the new list-appending logic, it can happen that previously
> merged events can now be filtered. Do it.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/uevent.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index eb900ec..809c74c 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -305,7 +305,7 @@ static void uevent_delete_from_list(struct uevent *to_delete,
>  	 * for the anchor), "old_tail" must be moved. It can happen that
>  	 * "old_tail" ends up pointing at the anchor.
>  	 */
> -	if (*old_tail == &to_delete->node)
> +	if (old_tail && *old_tail == &to_delete->node)
>  		*old_tail = to_delete->node.prev;
>  
>  	list_del_init(&to_delete->node);
> @@ -360,6 +360,20 @@ uevent_filter(struct uevent *later, struct uevent_filter_state *st)
>  		 * filter unnessary earlier uevents
>  		 * by the later uevent
>  		 */
> +		if (!list_empty(&earlier->merge_node)) {
> +			struct uevent *mn, *t;
> +
> +			list_for_each_entry_reverse_safe(mn, t, &earlier->merge_node, node) {
> +				if (uevent_can_filter(mn, later)) {
> +					condlog(4, "uevent: \"%s %s\" (merged into \"%s %s\") filtered by \"%s %s\"",
> +						mn->action, mn->kernel,
> +						earlier->action, earlier->kernel,
> +						later->action, later->kernel);
> +					uevent_delete_from_list(mn, &t, NULL);

Just like with 05/14, you could just use a much simpler delete function
here, since moving old_tail and merged nodes is unnecessary. I guess I
don't care that much, if you'd rather just have one function, so

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

> +					st->filtered++;
> +				}
> +			}
> +		}
>  		if (uevent_can_filter(earlier, later)) {
>  			condlog(3, "uevent: %s-%s has filtered by uevent: %s-%s",
>  				earlier->kernel, earlier->action,
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 00/14] Rework uevent filtering and merging

[Benjamin Marzinski]

 On Thu, Mar 31, 2022 at 12:14:56AM +0200, mwilck@suse.com wrote:
> Hi Ben, hi Christophe, hi Tang Junhui,
> 
> the bulk of this patch set (3-8) is a rework of the uevent filtering and
> merging logic introduced in commit ee8888f ("multipath-tools: improve 
> processing efficiency for addition and deletion of multipath devices"),
> by Tang Junhui.
> 
> The rationale is explained in detail in the commit message in patch 08/14.
> TL;DR: The previous approach delayed uevent handling, possibly a lot, which
> is often undesirable. The new logic passes events to the dispatcher
> immediately, but if they queue up (because the dispatcher can't keep
> up with the rate at which events arrive, or is blocked e.g. by the
> path checker), the dispatcher will apply filtering and merging
> between servicing individual events. This worked well in my own testing,
> but I'd appreciate if ZTE could give it a shot in their special test
> environment.
> 
> Patch 9-14 add some more improvements to the uevent handling code, and
> improve logging. The first 2 patches are unrelated fixes.
> 
> Comments welcome,
> 
> Martin

For everything except 04, 05, and 06

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

BTW, for some reason, these emails didn't come through a plain test for
me.

> 
> Martin Wilck (14):
>   multipathd: allow adding git rev as version suffix
>   multipathd: don't switch to DAEMON_IDLE during startup
>   uevent_dispatch(): use while in wait loop
>   libmultipath: uevent_dispatch(): process uevents one by one
>   libmultipath: uevent_dispatch(): only filter/merge new uevents
>   multipathd: reconfigure: disallow changing uid_attrs
>   libmultipath: microoptimize uevent filtering and merging
>   libmultipath: uevent_listen(): don't delay uevents
>   libmultipath: uevent: use struct to pass parameters around
>   libmultipath: uevent: log statistics about filtering and merging
>   libmultipath: merge_uevq(): filter first, then merge
>   libmultipath: uevent_filter(): filter previously merged events
>   libmultipath: uevent: improve log messages
>   libmultipath: uevent: add debug messages for event queue
> 
>  Makefile.inc               |   3 +-
>  libmultipath/config.c      |   6 +-
>  libmultipath/config.h      |   4 +-
>  libmultipath/discovery.c   |   2 +-
>  libmultipath/list.h        |  53 +++++
>  libmultipath/structs.h     |   2 +-
>  libmultipath/uevent.c      | 407 ++++++++++++++++++++++---------------
>  libmultipath/uevent.h      |   3 +-
>  multipath/multipath.conf.5 |   2 +
>  multipathd/main.c          |  59 ++++--
>  tests/uevent.c             |   2 +-
>  11 files changed, 354 insertions(+), 189 deletions(-)
> 
> -- 
> 2.35.1
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs

[Martin Wilck]

On Fri, 2022-04-01 at 22:40 -0500, Benjamin Marzinski wrote:
> > On Thu, Mar 31, 2022 at 12:15:02AM +0200, mwilck@suse.com wrote:
> > uevent merging by WWID relies on the uid_attrs config option. As we
> > drop struct config between calls to uevent_merge(), we must be sure
> > that the WWID is not changed in reconfigure().
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/multipath.conf.5 |  2 ++
> >  multipathd/main.c          | 53 +++++++++++++++++++++++++++++++---
> > ----
> >  2 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/multipath/multipath.conf.5
> > b/multipath/multipath.conf.5
> > index 605b46e..a9cd776 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -264,6 +264,8 @@ If this option is configured and matches the
> > device
> >  node name of a device, it overrides any other configured  methods
> > for
> >  determining the WWID for this device.
> >  .PP
> > +This option cannot be changed during runtime with the multipathd
> > \fBreconfigure\fR command.
> > +.PP
> >  The default is: \fB<unset>\fR. To enable uevent merging, set it
> > e.g. to
> >  \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
> >  .RE
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 13b1948..f514b32 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head)
> >         free_config(conf);
> >  }
> >  
> > +static bool reconfigure_check_uid_attrs(const struct _vector
> > *old_attrs,
> > +                                       const struct _vector
> > *new_attrs)
> > +{
> > +       int i;
> > +       char *old;
> > +
> > +       if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
> > +               return true;
> > +
> > +       vector_foreach_slot(old_attrs, old, i) {
> > +               char *new = VECTOR_SLOT(new_attrs, i);
> > +
> > +               if (strcmp(old, new))
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> > +static void reconfigure_check(struct config *old, struct config
> > *new)
> > +{
> > +       int old_marginal_pathgroups;
> > +
> > +       old_marginal_pathgroups = old->marginal_pathgroups;
> > +       if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> > +           (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN))
> > {
> > +               condlog(1, "multipathd must be restarted to turn %s
> > fpin marginal paths",
> > +                       (old_marginal_pathgroups ==
> > MARGINAL_PATHGROUP_FPIN)?
> > +                       "off" : "on");
> > +               new->marginal_pathgroups = old_marginal_pathgroups;
> > +       }
> > +
> > +       if (reconfigure_check_uid_attrs(&old->uid_attrs, &new-
> > >uid_attrs)) {
> > +               struct _vector v = new->uid_attrs;
> > +
> > +               condlog(1, "multipathd must be restarted to change
> > uid_attrs, keeping old values");
> > +               new->uid_attrs = old->uid_attrs;
> > +               vector_reset(&v);
> 
> This leaks the strings that v points to, right?  
> This also can happen in
> uid_attrs_handler(), I just noticed.

I was wondering about the same thing. But vector_reset() calls free()
on every slot. So no, I don't think anything is leaked here. The API
behaves admittedly in a surprising manner.

Martin



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

Re: [dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs

[Martin Wilck]

On Mon, 2022-04-04 at 08:42 +0200, Martin Wilck wrote:
> 
> I was wondering about the same thing. But vector_reset() calls free()
> on every slot. So no, I don't think anything is leaked here. The API
> behaves admittedly in a surprising manner.

Sorry, I was confused. Only v->slot is freed. Forget about this.

Martin

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