[PATCH v3 0/6] udev rule and CI improvements

[PATCH v3 0/6] udev rule and CI improvements

[Martin Wilck]

Changes v2->v3:

 3/6: make sure DM_COLDPLUG_SUSPENDED is imported from db on spurious uevents
      (Ben Marzinski)
 6/6: fix missing quotes mistake
 7/7: drop, already reviewed and pushed to "queue" branch

I kept the Reviewed-by: trailers in v3.
Thus 3/6 is the only patch missing Ben's Reviewed-by: tag.

Changes v1->v2:
 2/7: This patch becomes trivial, DM_UDEV_DISABLE_OTHER_RULES_FLAG
    is not imported any more, and my previous statement that DM_NOPATH
    might be set in an earlier rule was wrong; the test whether
    it's already set is dropped.
 3/7: looks different now because of the change in 2/6
 4/7: there was a flaw in the previous version, .MPATH_SAVE_DISABLE_OTHER_RULES_FLAG
    must always be set. z0-dm-mpath-late.rules was broken as well.
 5/7: looks different now because of the change in 2/6
 6/7: new, another glitch that I found while working on this code
 7/7: was 6/6.

Except for 1/7, I dropped Ben's "Reviewed-by:" trailer.

Changes wrt "v0" ("multipath-tools: udev rules and service improvements"
posted on Feb 5th)
 1/6: fix logic as pointed out by Ben Marzinski
 2/6: Keep importing MPATH_DEVICE_READY as suggested by Ben Marzinski.
      Don't import DM_UDEV_DISABLE_OTHER_RULES_FLAG any more.

# v1 Cover letter:

Patch 1-3 of this series are actually a v3 of my previous series
"multipath-tools: udev rules and service improvements" posted on Feb 5th.

Patch 4-6 are new. 4/6 implements new logic for setting
DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD, as discussed in the thread about 1/6
of the v1 series. 5/6 fixes a variant of the race condition that 3/6
addresses. Meanwhile I got positive test feedback about these patches; the
partner isn't observing any more errors with these patches applied. Thus
while certain types of race conditions (reload happening between
DM_SUSPENDED test in 10-dm.rules and blkid or pvscan calls in later rules)
are still possible, they seem to be rare enough to be handled in a separate
patch set.

6/6 is an unrelated new patch which is necessary to make CI for arm/v7
environments work.

Martin Wilck (6):
  11-dm-mpath.rules: use import logic like 13-dm-disk.rules
  11-dm-mpath.rules: don't import DM_UDEV_DISABLE_OTHER_RULES_FLAG
  11-dm-mpath.rules: handle reloads during coldplug events
  11-dm-mpath.rules: don't save DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD
  11-dm-mpath.rules: clear DM_DISABLE_OTHER_RULES_FLAG for coldplug
    events
  11-dm-mpath.rules: Don't force activation while device is suspended

 multipath/11-dm-mpath.rules.in     | 69 ++++++++++++++++++++++++------
 multipath/99-z-dm-mpath-late.rules |  4 ++
 multipath/Makefile                 |  2 +
 3 files changed, 61 insertions(+), 14 deletions(-)
 create mode 100644 multipath/99-z-dm-mpath-late.rules

-- 
2.43.0

[PATCH v3 1/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>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/11-dm-mpath.rules.in | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index b3117f9..e7be12d 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -79,7 +79,14 @@ 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"
+
+# If DM_UDEV_PRIMARY_SOURCE_FLAG is not set, the properties below
+# have never been properly set. Don't import them.
+ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", GOTO="import_end"
+
+# Don't import the properties from db if we will run blkid later.
+ENV{DM_NOSCAN}!="1", ENV{DM_DISABLE_OTHER_RULES_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 v3 2/6] 11-dm-mpath.rules: don't import DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

DM_UDEV_DISABLE_OTHER_RULES_FLAG is handled by 10-dm.rules, which imports
it from db if necessary. There is no need to do this again here.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/11-dm-mpath.rules.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index e7be12d..fd8d202 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -7,7 +7,7 @@ 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}!="?*", ENV{DM_ACTION}!="PATH_*", IMPORT{db}="DM_NOSCAN", GOTO="scan_import"
 
 ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
 
-- 
2.43.0

[PATCH v3 3/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.in | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index fd8d202..30d6e78 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -5,9 +5,16 @@ ENV{DM_UUID}!="mpath-?*", GOTO="mpath_end"
 IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
 IMPORT{db}="MPATH_DEVICE_READY"
 
+# 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"
+
 # 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_NOSCAN", GOTO="scan_import"
+ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", \
+	IMPORT{db}="DM_NOSCAN", IMPORT{db}="DM_COLDPLUG_SUSPENDED", \
+	GOTO="scan_import"
 
 ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
 
@@ -43,6 +50,16 @@ ENV{DM_ACTION}=="PATH_FAILED", GOTO="mpath_action"
 ENV{MPATH_DEVICE_READY}="1"
 
 LABEL="mpath_action"
+
+# A previous coldplug event occurred 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
@@ -57,6 +74,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.
@@ -99,6 +118,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 v3 4/6] 11-dm-mpath.rules: don't save DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD

[Martin Wilck]

The current rules overwrite DM_UDEV_DISABLE_OTHER_RULES_FLAG and save its
value in DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD if MPATH_DEVICE_READY changes
from 1 to 0, and restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from
DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD when MPATH_DEVICE_READY changes back from
0 to 1.

This is wrong for multiple reasons. For "spurious" events, 10-dm.rules will
read DM_UDEV_DISABLE_OTHER_RULES_FLAG from the udev db and obtain the value
saved by 11-dm-mpath.rules rather than it's own saved value, which confuses
the logic in 10-dm.rules. 10-dm.rules sets the flag if either DISK_RO==1 or
DM_SUSPENDED==1, and never clears it (it can only be cleared by a new genuine
libdm event that doesn't have DM_UDEV_DISABLE_OTHER_RULES_FLAG set, while the
device is not suspended). lvm commands may set the flag, too, but AFAICS this
is only done for certain types of logical volumes, not for multipath maps.

If a previously suspended device is resumed, DM_UDEV_DISABLE_OTHER_RULES_FLAG
would be cleared, and it would be wrong for 11-dm-mpath.rules to overwrite it
with a previuosly saved value. Likewise, if a uevent arrives for a suspended
map, and MPATH_DEVICE_READY happens to switch from 0 to 1, it would be wrong
to clear DM_UDEV_DISABLE_OTHER_RULES_FLAG by setting it to a previously saved
value.

We need to set DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 to prevent follow-up rules
from attempting I/O on the device. But don't try to save and restore
DM_UDEV_DISABLE_OTHER_RULES_FLAG between uevents. Rather, reset
DM_UDEV_DISABLE_OTHER_RULES_FLAG to the value we got from 10-dm.rules in a
late rule. I chose "99-z-" for this rule's prefix to make sure it runs
after 99-systemd.rules.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/11-dm-mpath.rules.in     | 19 +++++++++----------
 multipath/99-z-dm-mpath-late.rules |  4 ++++
 multipath/Makefile                 |  2 ++
 3 files changed, 15 insertions(+), 10 deletions(-)
 create mode 100644 multipath/99-z-dm-mpath-late.rules

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 30d6e78..cada176 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -2,7 +2,6 @@ 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"
 
 # Coldplug event while device is suspended (e.g. during a reload)
@@ -81,16 +80,16 @@ LABEL="force_activation"
 # We'd like to avoid this, especially within udev processing.
 ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
 
-# Also skip all foreign rules if no path is available.
-# Remember the original value of DM_DISABLE_OTHER_RULES_FLAG
-# and restore it back once we have at least one path available.
-ENV{MPATH_DEVICE_READY}=="0", ENV{.MPATH_DEVICE_READY_OLD}=="1", \
-	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}=="", \
-	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}"
-ENV{MPATH_DEVICE_READY}=="0", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+# Skip all foreign rules if no path is available.
+# Use DM_UDEV_DISABLE_OTHER_RULES_FLAG to communicate this
+# to upper layers. The original value will be restored in a late
+# udev rule.
+ENV{MPATH_DEVICE_READY}=="0", \
+	ENV{.MPATH_SAVE_DISABLE_OTHER_RULES_FLAG}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}", \
+	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+# If the device comes back online, set DM_ACTIVATION so that
+# upper layers do a rescan.
 ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0", \
-	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}", \
-	ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="", \
 	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
 
 # The code to check multipath state ends here. We need to set
diff --git a/multipath/99-z-dm-mpath-late.rules b/multipath/99-z-dm-mpath-late.rules
new file mode 100644
index 0000000..8574b1a
--- /dev/null
+++ b/multipath/99-z-dm-mpath-late.rules
@@ -0,0 +1,4 @@
+# If DM_UDEV_DISABLE_OTHER_RULES_FLAG was modified in 11-dm-mpath.rules,
+# restore it here, lest a temporary value be saved in the udev db
+ACTION=="add|change", ENV{.MPATH_SAVE_DISABLE_OTHER_RULES_FLAG}=="?*", \
+    ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{.MPATH_SAVE_DISABLE_OTHER_RULES_FLAG}"
diff --git a/multipath/Makefile b/multipath/Makefile
index f8c1f5e..67fb5e6 100644
--- a/multipath/Makefile
+++ b/multipath/Makefile
@@ -26,6 +26,7 @@ install:
 	$(Q)$(INSTALL_PROGRAM) -m 755 $(EXEC) $(DESTDIR)$(bindir)/
 	$(Q)$(INSTALL_PROGRAM) -d $(DESTDIR)$(udevrulesdir)
 	$(Q)$(INSTALL_PROGRAM) -m 644 11-dm-mpath.rules $(DESTDIR)$(udevrulesdir)
+	$(Q)$(INSTALL_PROGRAM) -m 644 99-z-dm-mpath-late.rules $(DESTDIR)$(udevrulesdir)
 	$(Q)$(INSTALL_PROGRAM) -m 644 multipath.rules $(DESTDIR)$(udevrulesdir)/56-multipath.rules
 	$(Q)$(INSTALL_PROGRAM) -d $(DESTDIR)$(tmpfilesdir)
 	$(Q)$(INSTALL_PROGRAM) -m 644 tmpfiles.conf $(DESTDIR)$(tmpfilesdir)/multipath.conf
@@ -46,6 +47,7 @@ endif
 uninstall:
 	$(Q)$(RM) $(DESTDIR)$(bindir)/$(EXEC)
 	$(Q)$(RM) $(DESTDIR)$(udevrulesdir)/11-dm-mpath.rules
+	$(Q)$(RM) $(DESTDIR)$(udevrulesdir)/99-z-dm-mpath-late.rules
 	$(Q)$(RM) $(DESTDIR)$(modulesloaddir)/multipath.conf
 	$(Q)$(RM) $(DESTDIR)$(modulesloaddir)/scsi_dh.conf
 	$(Q)$(RM) $(DESTDIR)$(libudevdir)/rules.d/56-multipath.rules
-- 
2.43.0

[PATCH v3 5/6] 11-dm-mpath.rules: clear DM_DISABLE_OTHER_RULES_FLAG for coldplug events

[Martin Wilck]

For all "spurious" events, which includes coldplug events,
DM_DISABLE_OTHER_RULES_FLAG will be read from the udev DB in
10-dm.rules. Thus if a previous event saw the device in suspended
state, the flag will be set even if the device has meanwhile
resumed. Reset the flag if none of the conditions hold that
would cause it to be set in a genuine uevent in 10-dm.rules.

It would be cleaner to do this in 10-dm.rules directly, but it
cannot be done easily, because the flag can also have an origin
inside lvm itself; lvm sets it for various kinds of logical
volumes. For generic (non-LVM) dm devices, the flag is only set
in 10-dm.rules though, so doing this is safe for multipath.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/11-dm-mpath.rules.in | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index cada176..82b0204 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -9,6 +9,13 @@ 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"
 
+# Coldplug event. DM_UDEV_DISABLE_OTHER_RULES_FLAG has been restored
+# from DB in 10-dm.rules. If the device is not suspended, clear the flag.
+# This is safe for multipath where DM_UDEV_DISABLE_OTHER_RULES_FLAG is basically
+# equivalent to DM_SUSPENDED==1 || DISK_RO==1
+ACTION=="add", ENV{DM_ACTIVATION}=="1", ENV{DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \
+	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="", GOTO="scan_import"
+
 # If this uevent didn't come from dm, don't try to update the
 # device state
 ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", \
-- 
2.43.0

[PATCH v3 6/6] 11-dm-mpath.rules: Don't force activation while device is suspended

[Martin Wilck]

If paths become available while the device is suspended, don't
activate. Another uevent will arrive when the device is resumed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/11-dm-mpath.rules.in | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 82b0204..d364f9b 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -94,10 +94,16 @@ ENV{MPATH_DEVICE_READY}=="0", ENV{DM_NOSCAN}="1"
 ENV{MPATH_DEVICE_READY}=="0", \
 	ENV{.MPATH_SAVE_DISABLE_OTHER_RULES_FLAG}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}", \
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+
 # If the device comes back online, set DM_ACTIVATION so that
-# upper layers do a rescan.
-ENV{MPATH_DEVICE_READY}!="0", ENV{.MPATH_DEVICE_READY_OLD}=="0", \
-	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
+# upper layers do a rescan. If the device is currently suspended,
+# we have to postpone the activation until the next event.
+ENV{MPATH_DEVICE_READY}=="0", GOTO="dont_activate"
+ENV{.MPATH_DEVICE_READY_OLD}!="0", GOTO="dont_activate"
+ENV{DM_SUSPENDED}=="1", ENV{MPATH_DEVICE_READY}="0", GOTO="dont_activate"
+
+ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
+LABEL="dont_activate"
 
 # The code to check multipath state ends here. We need to set
 # properties and symlinks regardless whether the map is usable or
-- 
2.43.0