[PATCH 0/6] multipath-tools: udev rules and service improvements

[PATCH 0/6] multipath-tools: udev rules and service improvements

[Martin Wilck]

The first 3 patches and patch 5 are minor fixes for the udev rules.

Patch 4 fixes an issue that was observed in partner tests. Since we dropped
the dependency on systemd-udev-settle.service, it's more common that
multipath sees paths being added to existing maps during boot and reloads
the maps. If this happens while udev is executing rules for an uevent
related to the map in question (most importantly, a coldplug event),
the rules may see the map as suspended, and will refrain from scanning
the device content.

https://systemd.io/BLOCK_DEVICE_LOCKING/ doesn't help us here. We would need
to take an exclusive lock an lock_multipath() to achieve that, but since
5ec07b3 ("libmultipath: use a shared lock to co-operate with udev") we
don't. We _might_ consider re-introducing exclusive locking, because the main
reason we don't was that udev would discard events for which it couldn't
obtain the lock. Since systemd 250, udev has a retry logic for such events
which would avoid this problem. We would also need to implement similar retry
logic in multipathd, though. For now, and because we need to support systemd
< 250 anyway, I've come up with the workaround in patch 4 (first tests went
well, but more testing is still needed).

Patch 5 is a partial fix for the the problem that multipathd socket activation
starts multipathd also on systems that don't need it. See
https://github.com/opensvc/multipath-tools/issues/76. More far-reaching
approaches have been discussed to avoid that the user needs to enable
multipathd explicitly if multipath hardware is present
(https://github.com/opensvc/multipath-tools/pull/78) but no solid solution
for that has emerged yet.

Reviews and comments welcome.

Martin Wilck (6):
  11-dm-mpath.rules: don't import properties that are already set
  11-dm-mpath.rules: fix list of imported properties
  11-dm-mpath.rules: use import logic like 13-dm-disk.rules
  11-dm-mpath.rules: handle reloads during coldplug events
  multipath: udev rules: use configured $(bindir) in udev rules
  multipathd: don't activate socket activation by default

 .gitignore                                    |  1 +
 Makefile.inc                                  |  2 +-
 ...11-dm-mpath.rules => 11-dm-mpath.rules.in} | 49 ++++++++++++++-----
 multipath/Makefile                            |  2 +-
 multipath/multipath.rules.in                  |  5 +-
 multipathd/multipathd.socket                  |  4 +-
 6 files changed, 45 insertions(+), 18 deletions(-)
 rename multipath/{11-dm-mpath.rules => 11-dm-mpath.rules.in} (73%)

-- 
2.43.0

[PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set

[Martin Wilck]

DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN may be already set
from previous rules, e.g. if the device is suspended. Make sure
we don't overwrite them.

DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY are only
used in this file, and not used in the scan_import code path.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index c339f52..2c4d006 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -2,12 +2,19 @@ ACTION!="add|change", GOTO="mpath_end"
 ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="mpath_end"
 ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
 
-IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
-IMPORT{db}="MPATH_DEVICE_READY"
-
 # If this uevent didn't come from dm, don't try to update the
 # device state
-ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG", IMPORT{db}="DM_NOSCAN", GOTO="scan_import"
+ENV{DM_COOKIE}=="?*", GOTO="check_ready"
+ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
+
+ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
+ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
+GOTO="scan_import"
+
+LABEL="check_ready"
+
+IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
+IMPORT{db}="MPATH_DEVICE_READY"
 
 ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
 
-- 
2.43.0

[PATCH 2/6] 11-dm-mpath.rules: fix list of imported properties

[Martin Wilck]

Make sure we import all properties that are also imported in
13-dm-disk.rules. Keep importing ID_FS_TYPE for now to avoid
breakage, even if 13-dm-disk.rules does not.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 2c4d006..43d227c 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -92,11 +92,13 @@ LABEL="scan_import"
 ENV{DM_NOSCAN}!="1", GOTO="import_end"
 IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
-IMPORT{db}="ID_FS_UUID"
 IMPORT{db}="ID_FS_UUID_ENC"
-IMPORT{db}="ID_FS_LABEL"
 IMPORT{db}="ID_FS_LABEL_ENC"
 IMPORT{db}="ID_FS_VERSION"
+IMPORT{db}="ID_PART_ENTRY_NAME"
+IMPORT{db}="ID_PART_ENTRY_UUID"
+IMPORT{db}="ID_PART_ENTRY_SCHEME"
+IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
 
 LABEL="import_end"
 
-- 
2.43.0

[PATCH 3/6] 11-dm-mpath.rules: use import logic like 13-dm-disk.rules

[Martin Wilck]

We have to import the properties if either DM_NOSCAN or
DM_DISABLE_OTHER_RULES_FLAG is set, because blkid will be skipped
in both cases. Also, if DM_UDEV_PRIMARY_SOURCE_FLAG is not set,
it makes no sense to try and import the properties.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 43d227c..8fc4a6f 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -89,7 +89,8 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0", \
 # not. If symlinks get lost, systemd may auto-unmount file systems.
 
 LABEL="scan_import"
-ENV{DM_NOSCAN}!="1", GOTO="import_end"
+ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_FLAG}!="1", \
+	ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="import_end"
 IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
 IMPORT{db}="ID_FS_UUID_ENC"
-- 
2.43.0

[PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events

[Martin Wilck]

If a map reload happens while udev is processing rules for a coldplug
event, DM_SUSPENDED may be set if the respective test in 10-dm.rules
happens while the device is suspened. This will cause the rules for all
higher block device layers to be skipped. Record this situation in an udev
property. The reload operation will trigger another "change" uevent later,
which would normally be treated as a reload, and be ignored without
rescanning the device. If a previous "coldplug while suspended" situation is
detected, perform a full device rescan instead.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipath/11-dm-mpath.rules | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
index 8fc4a6f..2706809 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules
@@ -9,8 +9,13 @@ ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
 
 ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
 ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
-GOTO="scan_import"
 
+# Coldplug event while device is suspended (e.g. during a reload)
+ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}=="1", \
+	PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.warning \"Coldplug event for suspended device\"", \
+	ENV{DM_COLDPLUG_SUSPENDED}="1"
+
+GOTO="scan_import"
 LABEL="check_ready"
 
 IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
@@ -53,6 +58,16 @@ ENV{DM_ACTION}=="PATH_FAILED", GOTO="mpath_action"
 ENV{MPATH_DEVICE_READY}="1"
 
 LABEL="mpath_action"
+
+# A previous coldplug event occured while the device was suspended.
+# Activation might have been partially skipped. Activate the device now,
+# i.e. disable the MPATH_UNCHANGED logic and set DM_ACTIVATION=1.
+IMPORT{db}="DM_COLDPLUG_SUSPENDED"
+ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{DM_SUSPENDED}!="1", \
+	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0", \
+	PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \
+	GOTO="force_activation"
+
 # DM_SUBSYSTEM_UDEV_FLAG0 is the "RELOAD" flag for multipath subsystem.
 # Drop the DM_ACTIVATION flag here as mpath reloads tables if any of its
 # paths are lost/recovered. For any stack above the mpath device, this is not
@@ -67,6 +82,8 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", \
 ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", \
 	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
 
+LABEL="force_activation"
+
 # Do not initiate scanning if no path is available,
 # otherwise there would be a hang or IO error on access.
 # We'd like to avoid this, especially within udev processing.
@@ -103,6 +120,9 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
 
 LABEL="import_end"
 
+# Reset previous DM_COLDPLUG_SUSPENDED if activation happens now
+ENV{DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}=""
+
 # Multipath maps should take precedence over their members.
 ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
 
-- 
2.43.0

[PATCH 5/6] multipath: udev rules: use configured $(bindir) in udev rules

[Martin Wilck]

This allows us to remove the lumsy MPATH_SBIN_PATH property and
related tests.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 .gitignore                                            | 1 +
 Makefile.inc                                          | 2 +-
 multipath/{11-dm-mpath.rules => 11-dm-mpath.rules.in} | 5 +----
 multipath/Makefile                                    | 2 +-
 multipath/multipath.rules.in                          | 5 +----
 5 files changed, 5 insertions(+), 10 deletions(-)
 rename multipath/{11-dm-mpath.rules => 11-dm-mpath.rules.in} (97%)

diff --git a/.gitignore b/.gitignore
index 6890e4a..049ffe8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -16,6 +16,7 @@ multipath/multipath
 multipath/multipath.8
 multipath/multipath.conf.5
 multipath/multipath.rules
+multipath/11-dm-mpath.rules
 multipath/tmpfiles.conf
 multipathd/multipathd
 multipathd/multipathd.8
diff --git a/Makefile.inc b/Makefile.inc
index 06bdd5e..3bcc7c2 100644
--- a/Makefile.inc
+++ b/Makefile.inc
@@ -148,4 +148,4 @@ NV_VERSION_SCRIPT = $(DEVLIB:%.so=%-nv.version)
 
 %:	%.in
 	@echo creating $@
-	$(Q)sed 's:@CONFIGFILE@:'$(configfile)':g;s:@CONFIGDIR@:'$(configdir)':g;s:@STATE_DIR@:'$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g' $< >$@
+	$(Q)sed 's:@CONFIGFILE@:'$(configfile)':g;s:@CONFIGDIR@:'$(configdir)':g;s:@STATE_DIR@:'$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g;s:@BINDIR@:'$(bindir)':g' $< >$@
diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules.in
similarity index 97%
rename from multipath/11-dm-mpath.rules
rename to multipath/11-dm-mpath.rules.in
index 2706809..38a0132 100644
--- a/multipath/11-dm-mpath.rules
+++ b/multipath/11-dm-mpath.rules.in
@@ -35,16 +35,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG2}=="1", ENV{MPATH_DEVICE_READY}="0", \
 # This may not be reliable, as events aren't necessarily received in order.
 ENV{DM_NR_VALID_PATHS}=="0", ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
 
-ENV{MPATH_SBIN_PATH}="/sbin"
-TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
-
 # Don't run multipath -U during "coldplug" after switching root,
 # because paths are just being added to the udev db.
 ACTION=="add", ENV{.MPATH_DEVICE_READY_OLD}=="1", GOTO="paths_ok"
 
 # Check the map state directly with multipath -U.
 # This doesn't attempt I/O on the device.
-PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -U -v1 %k", GOTO="paths_ok"
+PROGRAM=="@BINDIR@/multipath -U -v1 %k", GOTO="paths_ok"
 ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
 LABEL="paths_ok"
 
diff --git a/multipath/Makefile b/multipath/Makefile
index 0efb9b2..f8c1f5e 100644
--- a/multipath/Makefile
+++ b/multipath/Makefile
@@ -5,7 +5,7 @@ include ../Makefile.inc
 
 EXEC      := multipath
 MANPAGES  := multipath.8 multipath.conf.5
-GENERATED := $(MANPAGES) multipath.rules tmpfiles.conf
+GENERATED := $(MANPAGES) multipath.rules tmpfiles.conf 11-dm-mpath.rules
 
 CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathcmddir)
 CFLAGS += $(BIN_CFLAGS)
diff --git a/multipath/multipath.rules.in b/multipath/multipath.rules.in
index 03fa4d7..780bf85 100644
--- a/multipath/multipath.rules.in
+++ b/multipath/multipath.rules.in
@@ -18,9 +18,6 @@ GOTO="end_mpath"
 
 LABEL="test_dev"
 
-ENV{MPATH_SBIN_PATH}="/sbin"
-TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
-
 # FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
 # epoch).
 IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
@@ -31,7 +28,7 @@ IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
 
 # multipath -u sets DM_MULTIPATH_DEVICE_PATH and,
 # if "find_multipaths smart", also FIND_MULTIPATHS_WAIT_UNTIL.
-IMPORT{program}=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
+IMPORT{program}=="@BINDIR@/multipath -u %k", \
 	ENV{.MPATH_CHECK_PASSED}="1"
 
 # case 1: this is definitely multipath
-- 
2.43.0

[PATCH 6/6] multipathd: don't activate socket activation by default

[Martin Wilck]

Socket activation will start multipathd on systems that don't have multipath
hardware. This is often not desired.  On systems that do have multipath
hardware, OTOH, it is highly recommended to enable multipathd.service directly
rather than have it started via socket activation.  Therefore don't activate
the socket by default.  multipathd still supports socket activation, so users
who find it useful can disable multipathd.service and activate the socket.

Fixes: https://github.com/opensvc/multipath-tools/issues/76
Signed-off-by: Martin Wilck <mwilck@suse.com>
Cc: Sergio Durigan Junior <sergiodj@sergiodj.net>
Cc: Chris Hofstaedtler <zeha@debian.org>
---
 multipathd/multipathd.socket | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/multipathd/multipathd.socket b/multipathd/multipathd.socket
index c777e5e..6a62f5f 100644
--- a/multipathd/multipathd.socket
+++ b/multipathd/multipathd.socket
@@ -10,4 +10,6 @@ Before=sockets.target
 ListenStream=@/org/kernel/linux/storage/multipathd
 
 [Install]
-WantedBy=sockets.target
+# Socket activation for multipathd is disabled by default.
+# Activate it here if you find it useful.
+# WantedBy=sockets.target
-- 
2.43.0

Re: [PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:33PM +0100, Martin Wilck wrote:
> DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN may be already set
> from previous rules, e.g. if the device is suspended. Make sure
> we don't overwrite them.
> 
> DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY are only
> used in this file, and not used in the scan_import code path.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index c339f52..2c4d006 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -2,12 +2,19 @@ ACTION!="add|change", GOTO="mpath_end"
>  ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="mpath_end"
>  ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
>  
> -IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> -IMPORT{db}="MPATH_DEVICE_READY"
> -
>  # If this uevent didn't come from dm, don't try to update the
>  # device state
> -ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG", IMPORT{db}="DM_NOSCAN", GOTO="scan_import"
> +ENV{DM_COOKIE}=="?*", GOTO="check_ready"
> +ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
> +
> +ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
> +ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
> +GOTO="scan_import"
> +

If we do this, don't we forget the values for
DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY whenever we get a
non-dm uevent? If we skip importing them for a uevent, they're dropped
from the database, which means that on the next dm-originated uevent we
won't be able to get the old values. right?

-Ben

> +LABEL="check_ready"
> +
> +IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> +IMPORT{db}="MPATH_DEVICE_READY"
>  
>  ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
>  
> -- 
> 2.43.0

Re: [PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set

[Martin Wilck]

On Mon, 2024-02-05 at 15:44 -0500, Benjamin Marzinski wrote:
> On Mon, Feb 05, 2024 at 01:46:33PM +0100, Martin Wilck wrote:
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN may be already set
> > from previous rules, e.g. if the device is suspended. Make sure
> > we don't overwrite them.
> > 
> > DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY are only
> > used in this file, and not used in the scan_import code path.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/11-dm-mpath.rules | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > mpath.rules
> > index c339f52..2c4d006 100644
> > --- a/multipath/11-dm-mpath.rules
> > +++ b/multipath/11-dm-mpath.rules
> > @@ -2,12 +2,19 @@ ACTION!="add|change", GOTO="mpath_end"
> >  ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="mpath_end"
> >  ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
> >  
> > -IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> > -IMPORT{db}="MPATH_DEVICE_READY"
> > -
> >  # If this uevent didn't come from dm, don't try to update the
> >  # device state
> > -ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*",
> > IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG",
> > IMPORT{db}="DM_NOSCAN", GOTO="scan_import"
> > +ENV{DM_COOKIE}=="?*", GOTO="check_ready"
> > +ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
> > +
> > +ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*",
> > IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
> > +ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
> > +GOTO="scan_import"
> > +
> 
> If we do this, don't we forget the values for
> DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY whenever we
> get a
> non-dm uevent? If we skip importing them for a uevent, they're
> dropped
> from the database, which means that on the next dm-originated uevent
> we
> won't be able to get the old values. right?

Right, I didn't consider that. It makes sense for MPATH_DEVICE_READY.

However, while at it, I wonder about our rationale for saving
DM_UDEV_DISABLE_OTHER_RULES_FLAG in DM_DISABLE_OTHER_RULES_FLAG_OLD.

In 10-dm.rules, DM_DISABLE_OTHER_RULES_FLAG changes its value only
 - in DM_UDEV_PRIMARY_SOURCE_FLAG=1 events, according to the value of
DM_SUSPENDED
 - for DISK_RO=1 events (switches the flag on) 

(11-dm-lvm.rules has some additional logic that doesn't matter for
multipath).

For all other events, the flag is restored from the udev db in 10-
dm.rules. Ignoring DISK_RO, it always has the value that DM_SUSPENDED
had in the last DM_UDEV_PRIMARY_SOURCE_FLAG=1 (aka map reload) event.

The logic in 11-dm-mpath.rules adds a check for unusable arrays
on top, setting DM_DISABLE_OTHER_RULES_FLAG if MPATH_DEVICE_READY
switches from 1 to 0. In this case we save the previous value in
DM_DISABLE_OTHER_RULES_FLAG_OLD, and restore it from there in a later
event if MPATH_DEVICE_READY switches back from 0 to 1.

IMO the following can happen:

 1. an event arrives while DM_SUSPENDED=1, causing 
DM_DISABLE_OTHER_RULES_FLAG=1 to be set
 2. 11-dm-mpath.rules sees no paths and saves
DM_DISABLE_OTHER_RULES_FLAG=1 to DM_DISABLE_OTHER_RULES_FLAG_OLD
 3. an event arrives after the device has resumed, DM_SUSPENDED and
DM_DISABLE_OTHER_RULES_FLAG are cleared in 10-dm.rules
 4. 11-dm-mpath.rules sees MPATH_DEVICE_READY=1 and restores
DM_DISABLE_OTHER_RULES_FLAG, setting it to 1

... and this would be wrong, no?

It seems to me that we should not save DM_DISABLE_OTHER_RULES_FLAG_OLD
between uevents. We must set DM_DISABLE_OTHER_RULES_FLAG=1 to avoid
upper layer probing if there are no paths, but I suppose we should
restore the previous value after udev rule execution, e.g. in 99-dm-
mpath.rules:

ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="?*", \
   ENV{DM_DISABLE_OTHER_RULES_FLAG}=$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}, \
   ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""

Am I confusing stuff here?

Unfortunately, testing of my patch series has shown that it's an
improvement, but it isn't sufficient for completely avoiding races
between multipathd and udev. DM_SUSPENDED=1 can be avoided, but it's
still possible that the device gets suspended after the udev rules test
for supended state and before they run kpartx, blkid, pvscan, or other
similar commands.

I am quite clueless about this right now, and would be grateful for
ideas. Re-implementing LOCK_EX locking would be a possibility for
systemd >= 250, as noted in the cover letter of the series. But for
systemd <= 249, I don't see good options. We can't use LOCK_EX, because
when we release the lock, we have no means to know whether or not a
race with udev occurred (iow, whether udev tried to access the device
while we held the lock, failed, and dropped the event). Thus we'd need
to assume that this was the case, and force a reload after each reload,
which is obvious nonsense. We also have no means to know whether the
full set of rules has ever been run by udev for the device at hand,
because we don't know the set of rules that follow after 11-dm-
mpath.rules.

Thanks,
Martin

Re: [PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set

[Benjamin Marzinski]

On Tue, Feb 06, 2024 at 11:50:46AM +0100, Martin Wilck wrote:
> On Mon, 2024-02-05 at 15:44 -0500, Benjamin Marzinski wrote:
> > On Mon, Feb 05, 2024 at 01:46:33PM +0100, Martin Wilck wrote:
> > > DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN may be already set
> > > from previous rules, e.g. if the device is suspended. Make sure
> > > we don't overwrite them.
> > > 
> > > DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY are only
> > > used in this file, and not used in the scan_import code path.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipath/11-dm-mpath.rules | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > > mpath.rules
> > > index c339f52..2c4d006 100644
> > > --- a/multipath/11-dm-mpath.rules
> > > +++ b/multipath/11-dm-mpath.rules
> > > @@ -2,12 +2,19 @@ ACTION!="add|change", GOTO="mpath_end"
> > >  ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="mpath_end"
> > >  ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
> > >  
> > > -IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> > > -IMPORT{db}="MPATH_DEVICE_READY"
> > > -
> > >  # If this uevent didn't come from dm, don't try to update the
> > >  # device state
> > > -ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*",
> > > IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG",
> > > IMPORT{db}="DM_NOSCAN", GOTO="scan_import"
> > > +ENV{DM_COOKIE}=="?*", GOTO="check_ready"
> > > +ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
> > > +
> > > +ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*",
> > > IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
> > > +ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
> > > +GOTO="scan_import"
> > > +
> > 
> > If we do this, don't we forget the values for
> > DM_DISABLE_OTHER_RULES_FLAG_OLD and MPATH_DEVICE_READY whenever we
> > get a
> > non-dm uevent? If we skip importing them for a uevent, they're
> > dropped
> > from the database, which means that on the next dm-originated uevent
> > we
> > won't be able to get the old values. right?
> 
> Right, I didn't consider that. It makes sense for MPATH_DEVICE_READY.
> 
> However, while at it, I wonder about our rationale for saving
> DM_UDEV_DISABLE_OTHER_RULES_FLAG in DM_DISABLE_OTHER_RULES_FLAG_OLD.
> 
> In 10-dm.rules, DM_DISABLE_OTHER_RULES_FLAG changes its value only
>  - in DM_UDEV_PRIMARY_SOURCE_FLAG=1 events, according to the value of
> DM_SUSPENDED
>  - for DISK_RO=1 events (switches the flag on) 
> 
> (11-dm-lvm.rules has some additional logic that doesn't matter for
> multipath).
> 
> For all other events, the flag is restored from the udev db in 10-
> dm.rules. Ignoring DISK_RO, it always has the value that DM_SUSPENDED
> had in the last DM_UDEV_PRIMARY_SOURCE_FLAG=1 (aka map reload) event.
> 
> The logic in 11-dm-mpath.rules adds a check for unusable arrays
> on top, setting DM_DISABLE_OTHER_RULES_FLAG if MPATH_DEVICE_READY
> switches from 1 to 0. In this case we save the previous value in
> DM_DISABLE_OTHER_RULES_FLAG_OLD, and restore it from there in a later
> event if MPATH_DEVICE_READY switches back from 0 to 1.
> 
> IMO the following can happen:
> 
>  1. an event arrives while DM_SUSPENDED=1, causing 
> DM_DISABLE_OTHER_RULES_FLAG=1 to be set
>  2. 11-dm-mpath.rules sees no paths and saves
> DM_DISABLE_OTHER_RULES_FLAG=1 to DM_DISABLE_OTHER_RULES_FLAG_OLD
>  3. an event arrives after the device has resumed, DM_SUSPENDED and
> DM_DISABLE_OTHER_RULES_FLAG are cleared in 10-dm.rules
>  4. 11-dm-mpath.rules sees MPATH_DEVICE_READY=1 and restores
> DM_DISABLE_OTHER_RULES_FLAG, setting it to 1
> 
> ... and this would be wrong, no?
> 
> It seems to me that we should not save DM_DISABLE_OTHER_RULES_FLAG_OLD
> between uevents. We must set DM_DISABLE_OTHER_RULES_FLAG=1 to avoid
> upper layer probing if there are no paths, but I suppose we should
> restore the previous value after udev rule execution, e.g. in 99-dm-
> mpath.rules:
> 
> ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="?*", \
>    ENV{DM_DISABLE_OTHER_RULES_FLAG}=$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}, \
>    ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=""
> 
> Am I confusing stuff here?

Nope. That makes a lot of sense.
 
> Unfortunately, testing of my patch series has shown that it's an
> improvement, but it isn't sufficient for completely avoiding races
> between multipathd and udev. DM_SUSPENDED=1 can be avoided, but it's
> still possible that the device gets suspended after the udev rules test
> for supended state and before they run kpartx, blkid, pvscan, or other
> similar commands.
> 
> I am quite clueless about this right now, and would be grateful for
> ideas. Re-implementing LOCK_EX locking would be a possibility for
> systemd >= 250, as noted in the cover letter of the series. But for
> systemd <= 249, I don't see good options. We can't use LOCK_EX, because
> when we release the lock, we have no means to know whether or not a
> race with udev occurred (iow, whether udev tried to access the device
> while we held the lock, failed, and dropped the event). Thus we'd need
> to assume that this was the case, and force a reload after each reload,
> which is obvious nonsense. We also have no means to know whether the
> full set of rules has ever been run by udev for the device at hand,
> because we don't know the set of rules that follow after 11-dm-
> mpath.rules.

multipathd already refuses to reload newly created devices before it
sees the creation uevents to try to avoid this.  I assume that the
problem you are seeing is on the coldplug after the pivot root, where
the devices already exist, or am I confused here.  I wonder if we can do
something similar where we would ideally delay all device reloads until
after the coldplug.  The problem is figuring out when it's finished.

-Ben

> Thanks,
> Martin

Re: [PATCH 2/6] 11-dm-mpath.rules: fix list of imported properties

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:34PM +0100, Martin Wilck wrote:
> Make sure we import all properties that are also imported in
> 13-dm-disk.rules. Keep importing ID_FS_TYPE for now to avoid
> breakage, even if 13-dm-disk.rules does not.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 2c4d006..43d227c 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -92,11 +92,13 @@ LABEL="scan_import"
>  ENV{DM_NOSCAN}!="1", GOTO="import_end"
>  IMPORT{db}="ID_FS_TYPE"
>  IMPORT{db}="ID_FS_USAGE"
> -IMPORT{db}="ID_FS_UUID"
>  IMPORT{db}="ID_FS_UUID_ENC"
> -IMPORT{db}="ID_FS_LABEL"
>  IMPORT{db}="ID_FS_LABEL_ENC"
>  IMPORT{db}="ID_FS_VERSION"
> +IMPORT{db}="ID_PART_ENTRY_NAME"
> +IMPORT{db}="ID_PART_ENTRY_UUID"
> +IMPORT{db}="ID_PART_ENTRY_SCHEME"
> +IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
>  
>  LABEL="import_end"
>  
> -- 
> 2.43.0

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

Re: [PATCH 3/6] 11-dm-mpath.rules: use import logic like 13-dm-disk.rules

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:35PM +0100, Martin Wilck wrote:
> We have to import the properties if either DM_NOSCAN or
> DM_DISABLE_OTHER_RULES_FLAG is set, because blkid will be skipped
> in both cases. Also, if DM_UDEV_PRIMARY_SOURCE_FLAG is not set,
> it makes no sense to try and import the properties.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 43d227c..8fc4a6f 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -89,7 +89,8 @@ ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0", \
>  # not. If symlinks get lost, systemd may auto-unmount file systems.
>  
>  LABEL="scan_import"
> -ENV{DM_NOSCAN}!="1", GOTO="import_end"
> +ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_FLAG}!="1", \
> +	ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="import_end"
>  IMPORT{db}="ID_FS_TYPE"
>  IMPORT{db}="ID_FS_USAGE"
>  IMPORT{db}="ID_FS_UUID_ENC"
> -- 
> 2.43.0

Doesn't this mean that we will always import the properties if
ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1"

I think you mean

ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", GOTO="import_end"
ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_FLAG}!="1", GOTO="import_end"

right?

-Ben

Re: [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:36PM +0100, Martin Wilck wrote:
> If a map reload happens while udev is processing rules for a coldplug
> event, DM_SUSPENDED may be set if the respective test in 10-dm.rules
> happens while the device is suspened. This will cause the rules for all
> higher block device layers to be skipped. Record this situation in an udev
> property. The reload operation will trigger another "change" uevent later,
> which would normally be treated as a reload, and be ignored without
> rescanning the device. If a previous "coldplug while suspended" situation is
> detected, perform a full device rescan instead.

For what it's worth, this seems reasonable. But I do see the issue you
pointed out where we can't trust DM_SUSPENDED.  Perhaps we could track
in multipathd if an add event occured for a path between when we issued
a reload, and when we got the change event (unfortunately, change events
can occur for things other than reloads that multipathd triggers, and
the only way I know to accurately associate a uevent with a reload is by
using dm udev cookies. We have those turned off in multipathd and I
think it would be a good bit of work to turn them back on without
causing lots of waiting with locks held in multipathd). 
 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipath/11-dm-mpath.rules | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules
> index 8fc4a6f..2706809 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules
> @@ -9,8 +9,13 @@ ENV{DM_ACTION}=="PATH_*", GOTO="check_ready"
>  
>  ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="?*", IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
>  ENV{DM_NOSCAN}!="?*", IMPORT{db}="DM_NOSCAN"
> -GOTO="scan_import"
>  
> +# Coldplug event while device is suspended (e.g. during a reload)
> +ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}=="1", \
> +	PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.warning \"Coldplug event for suspended device\"", \
> +	ENV{DM_COLDPLUG_SUSPENDED}="1"
> +
> +GOTO="scan_import"
>  LABEL="check_ready"
>  
>  IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> @@ -53,6 +58,16 @@ ENV{DM_ACTION}=="PATH_FAILED", GOTO="mpath_action"
>  ENV{MPATH_DEVICE_READY}="1"
>  
>  LABEL="mpath_action"
> +
> +# A previous coldplug event occured while the device was suspended.
> +# Activation might have been partially skipped. Activate the device now,
> +# i.e. disable the MPATH_UNCHANGED logic and set DM_ACTIVATION=1.
> +IMPORT{db}="DM_COLDPLUG_SUSPENDED"
> +ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{DM_SUSPENDED}!="1", \
> +	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0", \
> +	PROGRAM="/bin/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \
> +	GOTO="force_activation"
> +
>  # DM_SUBSYSTEM_UDEV_FLAG0 is the "RELOAD" flag for multipath subsystem.
>  # Drop the DM_ACTIVATION flag here as mpath reloads tables if any of its
>  # paths are lost/recovered. For any stack above the mpath device, this is not
> @@ -67,6 +82,8 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", \
>  ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", \
>  	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
>  
> +LABEL="force_activation"
> +
>  # Do not initiate scanning if no path is available,
>  # otherwise there would be a hang or IO error on access.
>  # We'd like to avoid this, especially within udev processing.
> @@ -103,6 +120,9 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
>  
>  LABEL="import_end"
>  
> +# Reset previous DM_COLDPLUG_SUSPENDED if activation happens now
> +ENV{DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}=""
> +
>  # Multipath maps should take precedence over their members.
>  ENV{DM_UDEV_LOW_PRIORITY_FLAG}!="1", OPTIONS+="link_priority=50"
>  
> -- 
> 2.43.0

Re: [PATCH 5/6] multipath: udev rules: use configured $(bindir) in udev rules

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:37PM +0100, Martin Wilck wrote:
> This allows us to remove the lumsy MPATH_SBIN_PATH property and
> related tests.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  .gitignore                                            | 1 +
>  Makefile.inc                                          | 2 +-
>  multipath/{11-dm-mpath.rules => 11-dm-mpath.rules.in} | 5 +----
>  multipath/Makefile                                    | 2 +-
>  multipath/multipath.rules.in                          | 5 +----
>  5 files changed, 5 insertions(+), 10 deletions(-)
>  rename multipath/{11-dm-mpath.rules => 11-dm-mpath.rules.in} (97%)
> 
> diff --git a/.gitignore b/.gitignore
> index 6890e4a..049ffe8 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -16,6 +16,7 @@ multipath/multipath
>  multipath/multipath.8
>  multipath/multipath.conf.5
>  multipath/multipath.rules
> +multipath/11-dm-mpath.rules
>  multipath/tmpfiles.conf
>  multipathd/multipathd
>  multipathd/multipathd.8
> diff --git a/Makefile.inc b/Makefile.inc
> index 06bdd5e..3bcc7c2 100644
> --- a/Makefile.inc
> +++ b/Makefile.inc
> @@ -148,4 +148,4 @@ NV_VERSION_SCRIPT = $(DEVLIB:%.so=%-nv.version)
>  
>  %:	%.in
>  	@echo creating $@
> -	$(Q)sed 's:@CONFIGFILE@:'$(configfile)':g;s:@CONFIGDIR@:'$(configdir)':g;s:@STATE_DIR@:'$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g' $< >$@
> +	$(Q)sed 's:@CONFIGFILE@:'$(configfile)':g;s:@CONFIGDIR@:'$(configdir)':g;s:@STATE_DIR@:'$(statedir)':g;s:@RUNTIME_DIR@:'$(runtimedir)':g;s/@MODPROBE_UNIT@/'$(MODPROBE_UNIT)'/g;s:@BINDIR@:'$(bindir)':g' $< >$@
> diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-mpath.rules.in
> similarity index 97%
> rename from multipath/11-dm-mpath.rules
> rename to multipath/11-dm-mpath.rules.in
> index 2706809..38a0132 100644
> --- a/multipath/11-dm-mpath.rules
> +++ b/multipath/11-dm-mpath.rules.in
> @@ -35,16 +35,13 @@ ENV{DM_SUBSYSTEM_UDEV_FLAG2}=="1", ENV{MPATH_DEVICE_READY}="0", \
>  # This may not be reliable, as events aren't necessarily received in order.
>  ENV{DM_NR_VALID_PATHS}=="0", ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
>  
> -ENV{MPATH_SBIN_PATH}="/sbin"
> -TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
> -
>  # Don't run multipath -U during "coldplug" after switching root,
>  # because paths are just being added to the udev db.
>  ACTION=="add", ENV{.MPATH_DEVICE_READY_OLD}=="1", GOTO="paths_ok"
>  
>  # Check the map state directly with multipath -U.
>  # This doesn't attempt I/O on the device.
> -PROGRAM=="$env{MPATH_SBIN_PATH}/multipath -U -v1 %k", GOTO="paths_ok"
> +PROGRAM=="@BINDIR@/multipath -U -v1 %k", GOTO="paths_ok"
>  ENV{MPATH_DEVICE_READY}="0", GOTO="mpath_action"
>  LABEL="paths_ok"
>  
> diff --git a/multipath/Makefile b/multipath/Makefile
> index 0efb9b2..f8c1f5e 100644
> --- a/multipath/Makefile
> +++ b/multipath/Makefile
> @@ -5,7 +5,7 @@ include ../Makefile.inc
>  
>  EXEC      := multipath
>  MANPAGES  := multipath.8 multipath.conf.5
> -GENERATED := $(MANPAGES) multipath.rules tmpfiles.conf
> +GENERATED := $(MANPAGES) multipath.rules tmpfiles.conf 11-dm-mpath.rules
>  
>  CPPFLAGS += -I$(multipathdir) -I$(mpathutildir) -I$(mpathcmddir)
>  CFLAGS += $(BIN_CFLAGS)
> diff --git a/multipath/multipath.rules.in b/multipath/multipath.rules.in
> index 03fa4d7..780bf85 100644
> --- a/multipath/multipath.rules.in
> +++ b/multipath/multipath.rules.in
> @@ -18,9 +18,6 @@ GOTO="end_mpath"
>  
>  LABEL="test_dev"
>  
> -ENV{MPATH_SBIN_PATH}="/sbin"
> -TEST!="$env{MPATH_SBIN_PATH}/multipath", ENV{MPATH_SBIN_PATH}="/usr/sbin"
> -
>  # FIND_MULTIPATHS_WAIT_UNTIL is the timeout (in seconds after the
>  # epoch).
>  IMPORT{db}="FIND_MULTIPATHS_WAIT_UNTIL"
> @@ -31,7 +28,7 @@ IMPORT{db}="DM_MULTIPATH_DEVICE_PATH"
>  
>  # multipath -u sets DM_MULTIPATH_DEVICE_PATH and,
>  # if "find_multipaths smart", also FIND_MULTIPATHS_WAIT_UNTIL.
> -IMPORT{program}=="$env{MPATH_SBIN_PATH}/multipath -u %k", \
> +IMPORT{program}=="@BINDIR@/multipath -u %k", \
>  	ENV{.MPATH_CHECK_PASSED}="1"
>  
>  # case 1: this is definitely multipath
> -- 
> 2.43.0

Re: [PATCH 6/6] multipathd: don't activate socket activation by default

[Benjamin Marzinski]

On Mon, Feb 05, 2024 at 01:46:38PM +0100, Martin Wilck wrote:
> Socket activation will start multipathd on systems that don't have multipath
> hardware. This is often not desired.  On systems that do have multipath
> hardware, OTOH, it is highly recommended to enable multipathd.service directly
> rather than have it started via socket activation.  Therefore don't activate
> the socket by default.  multipathd still supports socket activation, so users
> who find it useful can disable multipathd.service and activate the socket.
> 
> Fixes: https://github.com/opensvc/multipath-tools/issues/76
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> Cc: Sergio Durigan Junior <sergiodj@sergiodj.net>
> Cc: Chris Hofstaedtler <zeha@debian.org>

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

> ---
>  multipathd/multipathd.socket | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/multipathd/multipathd.socket b/multipathd/multipathd.socket
> index c777e5e..6a62f5f 100644
> --- a/multipathd/multipathd.socket
> +++ b/multipathd/multipathd.socket
> @@ -10,4 +10,6 @@ Before=sockets.target
>  ListenStream=@/org/kernel/linux/storage/multipathd
>  
>  [Install]
> -WantedBy=sockets.target
> +# Socket activation for multipathd is disabled by default.
> +# Activate it here if you find it useful.
> +# WantedBy=sockets.target
> -- 
> 2.43.0

Re: [PATCH 1/6] 11-dm-mpath.rules: don't import properties that are already set

[Martin Wilck]

On Thu, 2024-02-08 at 19:30 -0500, Benjamin Marzinski wrote:
> On Tue, Feb 06, 2024 at 11:50:46AM +0100, Martin Wilck wrote:
> > 
> > Unfortunately, testing of my patch series has shown that it's an
> > improvement, but it isn't sufficient for completely avoiding races
> > between multipathd and udev. DM_SUSPENDED=1 can be avoided, but
> > it's
> > still possible that the device gets suspended after the udev rules
> > test
> > for supended state and before they run kpartx, blkid, pvscan, or
> > other
> > similar commands.
> > 
> > I am quite clueless about this right now, and would be grateful for
> > ideas. Re-implementing LOCK_EX locking would be a possibility for
> > systemd >= 250, as noted in the cover letter of the series. But for
> > systemd <= 249, I don't see good options. We can't use LOCK_EX,
> > because
> > when we release the lock, we have no means to know whether or not a
> > race with udev occurred (iow, whether udev tried to access the
> > device
> > while we held the lock, failed, and dropped the event). Thus we'd
> > need
> > to assume that this was the case, and force a reload after each
> > reload,
> > which is obvious nonsense. We also have no means to know whether
> > the
> > full set of rules has ever been run by udev for the device at hand,
> > because we don't know the set of rules that follow after 11-dm-
> > mpath.rules.
> 
> multipathd already refuses to reload newly created devices before it
> sees the creation uevents to try to avoid this.

Right, thanks for reminding me about that.

>   I assume that the
> problem you are seeing is on the coldplug after the pivot root, where
> the devices already exist, or am I confused here.  

No, that's exactly the situation in which the problem is observed.

> I wonder if we can do
> something similar where we would ideally delay all device reloads
> until
> after the coldplug.  The problem is figuring out when it's finished.

The current state of affairs is that patch 4 in my series has avoided
the issue on coldplug events, greatly reducing the frequency of errors
on the test sysstem. There's a variant that needs an additional fix:

 1 map set up in initrd
 2 creation uevent event is observed by multipathd
 3 switch root
 4 path addition, map reload
 5 change event
 6 path addition, another map reload (5-6 can repeat)
 7 DM_SUSPENDED is observed by udev while handling the change event,
   and saved as DM_UDEV_DISABLE_OTHER_RULES_FLAG
 8 Coldplug event, DM_UDEV_DISABLE_OTHER_RULES_FLAG is restored from
   udev db => pvscan is skipped

I've created another patch to fix this situation, and am now waiting
for for test feedback. I'll post about this in more detail later.
In short, I believe 10-dm.rules should differentiate
DM_UDEV_DISABLE_OTHER_RULES_FLAG originating from lvm itself and
DM_SUSPENDED.

Regards,
Martin

Re: [PATCH 3/6] 11-dm-mpath.rules: use import logic like 13-dm-disk.rules

[Martin Wilck]

On Thu, 2024-02-08 at 19:36 -0500, Benjamin Marzinski wrote:
> On Mon, Feb 05, 2024 at 01:46:35PM +0100, Martin Wilck wrote:
> > We have to import the properties if either DM_NOSCAN or
> > DM_DISABLE_OTHER_RULES_FLAG is set, because blkid will be skipped
> > in both cases. Also, if DM_UDEV_PRIMARY_SOURCE_FLAG is not set,
> > it makes no sense to try and import the properties.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipath/11-dm-mpath.rules | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/multipath/11-dm-mpath.rules b/multipath/11-dm-
> > mpath.rules
> > index 43d227c..8fc4a6f 100644
> > --- a/multipath/11-dm-mpath.rules
> > +++ b/multipath/11-dm-mpath.rules
> > @@ -89,7 +89,8 @@ ENV{MPATH_DEVICE_READY}!="0",
> > ENV{.MPATH_DEVICE_READY_OLD}=="0", \
> >  # not. If symlinks get lost, systemd may auto-unmount file
> > systems.
> >  
> >  LABEL="scan_import"
> > -ENV{DM_NOSCAN}!="1", GOTO="import_end"
> > +ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_FLAG}!="1", \
> > +	ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="import_end"
> >  IMPORT{db}="ID_FS_TYPE"
> >  IMPORT{db}="ID_FS_USAGE"
> >  IMPORT{db}="ID_FS_UUID_ENC"
> > -- 
> > 2.43.0
> 
> Doesn't this mean that we will always import the properties if
> ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1"
> 
> I think you mean
> 
> ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", GOTO="import_end"
> ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_FLAG}!="1",
> GOTO="import_end"
> 
> right?

Yes :-/
Thanks for pointing it out.

Martin

Re: [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events

[Martin Wilck]

On Thu, 2024-02-08 at 20:03 -0500, Benjamin Marzinski wrote:
> On Mon, Feb 05, 2024 at 01:46:36PM +0100, Martin Wilck wrote:
> > If a map reload happens while udev is processing rules for a
> > coldplug
> > event, DM_SUSPENDED may be set if the respective test in 10-
> > dm.rules
> > happens while the device is suspened. This will cause the rules for
> > all
> > higher block device layers to be skipped. Record this situation in
> > an udev
> > property. The reload operation will trigger another "change" uevent
> > later,
> > which would normally be treated as a reload, and be ignored without
> > rescanning the device. If a previous "coldplug while suspended"
> > situation is
> > detected, perform a full device rescan instead.
> 
> For what it's worth, this seems reasonable. 

If that's so, I'd appreciate a Reviewed-by: :-)
See below.

> But I do see the issue you
> pointed out where we can't trust DM_SUSPENDED.

That was my suspicion originally, but the actual problem that was
observed was different, see my recent response to 1/6.

Yes, it can happen any time that a device gets suspended between the
DM_SUSPENDED check and an attempt to do actual I/O in later udev rules,
but that is much less likely than the other case. So far I haven't been
able to actually observe it.

Btw am I understanding correctly that you don't like the idea of going
back to exclusive locking?

>   Perhaps we could track
> in multipathd if an add event occured for a path between when we
> issued
> a reload, and when we got the change event (unfortunately, change
> events
> can occur for things other than reloads that multipathd triggers, and
> the only way I know to accurately associate a uevent with a reload is
> by
> using dm udev cookies. We have those turned off in multipathd and I
> think it would be a good bit of work to turn them back on without
> causing lots of waiting with locks held in multipathd). 

IMO doing this right in multipathd will be hard. We must be careful not
to trigger too many additional uevents just because something *might*
have gone wrong during udev handling (most of the time all is well,
even if a reload happens). I believe to do this properly we must listen
for *kernel* uevents, too, and that's something we've been trying to
avoid for good reason.

I had a different idea: to track a "reload count" for a map somehow
(e.g. in a file under /run/multipath) and checking that at the
beginning and end of uevent handling in order to see if a reload
occurred while the uevent was being processed (which would be detected
by seeing a different the reload count change during the uevent).

For now, I hope that this change plus my latest addition will make the
practical issue go away. All else can be discussed later.
We haven't got any reports about this sort of race for years, so it's
safe to assume that happens very rarely.

Regards
Martin

Re: [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events

[Benjamin Marzinski]

On Fri, Feb 09, 2024 at 04:53:50PM +0100, Martin Wilck wrote:
> On Thu, 2024-02-08 at 20:03 -0500, Benjamin Marzinski wrote:
> > On Mon, Feb 05, 2024 at 01:46:36PM +0100, Martin Wilck wrote:
> > > If a map reload happens while udev is processing rules for a
> > > coldplug
> > > event, DM_SUSPENDED may be set if the respective test in 10-
> > > dm.rules
> > > happens while the device is suspened. This will cause the rules for
> > > all
> > > higher block device layers to be skipped. Record this situation in
> > > an udev
> > > property. The reload operation will trigger another "change" uevent
> > > later,
> > > which would normally be treated as a reload, and be ignored without
> > > rescanning the device. If a previous "coldplug while suspended"
> > > situation is
> > > detected, perform a full device rescan instead.
> > 
> > For what it's worth, this seems reasonable. 
> 
> If that's so, I'd appreciate a Reviewed-by: :-)
> See below.

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

> 
> > But I do see the issue you
> > pointed out where we can't trust DM_SUSPENDED.
> 
> That was my suspicion originally, but the actual problem that was
> observed was different, see my recent response to 1/6.
> 
> Yes, it can happen any time that a device gets suspended between the
> DM_SUSPENDED check and an attempt to do actual I/O in later udev rules,
> but that is much less likely than the other case. So far I haven't been
> able to actually observe it.
> 
> Btw am I understanding correctly that you don't like the idea of going
> back to exclusive locking?
> 
> >   Perhaps we could track
> > in multipathd if an add event occured for a path between when we
> > issued
> > a reload, and when we got the change event (unfortunately, change
> > events
> > can occur for things other than reloads that multipathd triggers, and
> > the only way I know to accurately associate a uevent with a reload is
> > by
> > using dm udev cookies. We have those turned off in multipathd and I
> > think it would be a good bit of work to turn them back on without
> > causing lots of waiting with locks held in multipathd). 
> 
> IMO doing this right in multipathd will be hard. We must be careful not
> to trigger too many additional uevents just because something *might*
> have gone wrong during udev handling (most of the time all is well,
> even if a reload happens). I believe to do this properly we must listen
> for *kernel* uevents, too, and that's something we've been trying to
> avoid for good reason.
> 
> I had a different idea: to track a "reload count" for a map somehow
> (e.g. in a file under /run/multipath) and checking that at the
> beginning and end of uevent handling in order to see if a reload
> occurred while the uevent was being processed (which would be detected
> by seeing a different the reload count change during the uevent).

That sounds much easier than messing with udev cookies.

-Ben

> For now, I hope that this change plus my latest addition will make the
> practical issue go away. All else can be discussed later.
> We haven't got any reports about this sort of race for years, so it's
> safe to assume that happens very rarely.
> 
> Regards
> Martin

Re: [PATCH 4/6] 11-dm-mpath.rules: handle reloads during coldplug events

[Benjamin Marzinski]

On Fri, Feb 09, 2024 at 04:53:50PM +0100, Martin Wilck wrote:
> On Thu, 2024-02-08 at 20:03 -0500, Benjamin Marzinski wrote:
> > On Mon, Feb 05, 2024 at 01:46:36PM +0100, Martin Wilck wrote:
> > > If a map reload happens while udev is processing rules for a
> > > coldplug
> > > event, DM_SUSPENDED may be set if the respective test in 10-
> > > dm.rules
> > > happens while the device is suspened. This will cause the rules for
> > > all
> > > higher block device layers to be skipped. Record this situation in
> > > an udev
> > > property. The reload operation will trigger another "change" uevent
> > > later,
> > > which would normally be treated as a reload, and be ignored without
> > > rescanning the device. If a previous "coldplug while suspended"
> > > situation is
> > > detected, perform a full device rescan instead.
> > 
> > For what it's worth, this seems reasonable. 
> 
> If that's so, I'd appreciate a Reviewed-by: :-)
> See below.
> 
> > But I do see the issue you
> > pointed out where we can't trust DM_SUSPENDED.
> 
> That was my suspicion originally, but the actual problem that was
> observed was different, see my recent response to 1/6.
> 
> Yes, it can happen any time that a device gets suspended between the
> DM_SUSPENDED check and an attempt to do actual I/O in later udev rules,
> but that is much less likely than the other case. So far I haven't been
> able to actually observe it.
> 
> Btw am I understanding correctly that you don't like the idea of going
> back to exclusive locking?

Oh, and yeah, I'd rather avoid exclusive locking if possible, just out
of a hunch that it could lead to other unexpected problems.

-Ben

> 
> >   Perhaps we could track
> > in multipathd if an add event occured for a path between when we
> > issued
> > a reload, and when we got the change event (unfortunately, change
> > events
> > can occur for things other than reloads that multipathd triggers, and
> > the only way I know to accurately associate a uevent with a reload is
> > by
> > using dm udev cookies. We have those turned off in multipathd and I
> > think it would be a good bit of work to turn them back on without
> > causing lots of waiting with locks held in multipathd). 
> 
> IMO doing this right in multipathd will be hard. We must be careful not
> to trigger too many additional uevents just because something *might*
> have gone wrong during udev handling (most of the time all is well,
> even if a reload happens). I believe to do this properly we must listen
> for *kernel* uevents, too, and that's something we've been trying to
> avoid for good reason.
> 
> I had a different idea: to track a "reload count" for a map somehow
> (e.g. in a file under /run/multipath) and checking that at the
> beginning and end of uevent handling in order to see if a reload
> occurred while the uevent was being processed (which would be detected
> by seeing a different the reload count change during the uevent).
> 
> For now, I hope that this change plus my latest addition will make the
> practical issue go away. All else can be discussed later.
> We haven't got any reports about this sort of race for years, so it's
> safe to assume that happens very rarely.
> 
> Regards
> Martin