[PATCH v2 0/5] multipath-tools: udev rule fixes

[PATCH v2 0/5] multipath-tools: udev rule fixes

[Martin Wilck]

Here are more fixes for the multipath udev rules. 2/5 is an actual fix,
1/5, 3/5 and 4/5 are minor cleanups / code clarifications.

5/5 implements the fix originally submitted by Ben [1].

Reviews and comments welcome.
Martin

[1] https://lore.kernel.org/dm-devel/20241022233349.247087-1-bmarzins@redhat.com/

Martin Wilck (5):
  11-dm-mpath.rules.in: import DM_COLDPLUG_SUSPENDED only once
  11-dm-mpath.rules.in: handle inactive suspended devices correctly
  11-dm-mpath.rules.in: clarify DM_ACTIVATION logic
  11-dm-mpath-rules.in: skip one .DM_NOSCAN check
  11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

 multipath/11-dm-mpath.rules.in | 44 ++++++++++++++++++++--------------
 1 file changed, 26 insertions(+), 18 deletions(-)

-- 
2.47.0

[PATCH v2 1/5] 11-dm-mpath.rules.in: import DM_COLDPLUG_SUSPENDED only once

[Martin Wilck]

We import DM_COLDPLUG_SUSPENDED in all code flows below mpath_coldplug_end.
Clarify this in the code.

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

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 30647b9..6783826 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -24,12 +24,13 @@ ENV{DM_UDEV_RULES_VSN}=="1|2", ENV{.DM_SUSPENDED}!="1", ENV{DISK_RO}!="1", \
 	ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="", GOTO="scan_import"
 LABEL="mpath_coldplug_end"
 
+IMPORT{db}="DM_COLDPLUG_SUSPENDED"
+
 # If this uevent didn't come from dm, don't try to update the
 # device state
 # Note that .MPATH_DEVICE_READY_OLD=="" here. Thus we won't activate the
 # device below at mpath_is_ready, which is correct.
 ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", \
-	IMPORT{db}="DM_COLDPLUG_SUSPENDED", \
 	GOTO="check_mpath_ready"
 
 ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
@@ -67,7 +68,6 @@ LABEL="check_mpath_unchanged"
 # 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="@SYSDIR_BIN@/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \
-- 
2.47.0

[PATCH v2 2/5] 11-dm-mpath.rules.in: handle inactive suspended devices correctly

[Martin Wilck]

Since b22c273 ("11-dm-mpath.rules: Don't force activation while device is
suspended"), we've handled the case where a device is suspended while
an uevent is processed (e.g. because multipathd is reloading the
map again at the same time). But we were missing the case where
The device had never been initialized before. If .MPATH_DEVICE_READY_OLD
was empty, we'd jump to scan_import without setting MPATH_DEVICE_READY
to 0. This can cause a device not to be fully activated at boot time,
because in follow-up uevents we'd assume that the device had already
been set up.

Treat the case in which an uevent is processed for a previously not
fully set-up, suspended device like other situations where we set
MPATH_DEVICE_READY to 0.

Fixes: b22c273 ("11-dm-mpath.rules: Don't force activation while device is
suspended")
---
 multipath/11-dm-mpath.rules.in | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 6783826..20f8c6a 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -35,6 +35,13 @@ ENV{DM_COOKIE}!="?*", ENV{DM_ACTION}!="PATH_*", \
 
 ENV{.MPATH_DEVICE_READY_OLD}="$env{MPATH_DEVICE_READY}"
 
+# If the device wasn't ready previously and is currently suspended,
+# we have to postpone the activation until the next event.
+# In this case, we have to set MPATH_DEVICE_READY=0; otherwise, the
+# MPATH_UNCHANGED logic will cause later rules to skipped in the next event.
+ENV{.MPATH_DEVICE_READY_OLD}!="1", ENV{.DM_SUSPENDED}=="1", \
+	ENV{MPATH_DEVICE_READY}="0", GOTO="check_mpath_unchanged"
+
 # multipath sets DM_SUBSYSTEM_UDEV_FLAG2 when it reloads a
 # table with no active devices. If this happens, mark the
 # device not ready
@@ -106,14 +113,10 @@ GOTO="scan_import"
 LABEL="mpath_is_ready"
 
 # If the device comes back online, set DM_ACTIVATION so that
-# upper layers do a rescan. If the device is currently suspended,
-# we have to postpone the activation until the next event.
-# In this case, we have to set MPATH_DEVICE_READY=0; otherwise, the
-# MPATH_UNCHANGED logic will cause later rules to skipped in the next event.
-ENV{.MPATH_DEVICE_READY_OLD}!="0", GOTO="scan_import"
-ENV{.DM_SUSPENDED}=="1", ENV{MPATH_DEVICE_READY}="0", GOTO="scan_import"
-
-ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
+# upper layers will do a rescan. Don't do this if .MPATH_DEVICE_READY_OLD
+# is just empty (see comment above the DM_COOKIE test above).
+ENV{.MPATH_DEVICE_READY_OLD}=="0", \
+	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
 
 # The code to check multipath state ends here. We need to set
 # properties and symlinks regardless whether the map is usable or
-- 
2.47.0

[PATCH v2 3/5] 11-dm-mpath.rules.in: clarify DM_ACTIVATION logic

[Martin Wilck]

Our code is always setting MPATH_UNCHANGED and DM_ACTIVATION in
pairs. While DM_ACTIVATION is a global DM property, MPATH_UNCHANGED
is owned by us. Just set MPATH_UNCHANGED, and adapt DM_ACTIVATION
when necessary just in one place.

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

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 20f8c6a..a2655cb 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -74,25 +74,25 @@ LABEL="check_mpath_unchanged"
 
 # 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.
+# i.e. disable the MPATH_UNCHANGED logic.
 ENV{DM_COLDPLUG_SUSPENDED}=="1", ENV{.DM_SUSPENDED}!="1", \
-	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0", \
+	ENV{MPATH_UNCHANGED}="0", \
 	PROGRAM="@SYSDIR_BIN@/logger -t 11-dm-mpath.rules -p daemon.notice \"Forcing activation of previously suspended device\"", \
 	GOTO="check_mpath_ready"
 
 # 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
+# Set the MPATH_UNCHANGED flag here as mpath reloads tables if any of its
 # paths are lost/recovered. For any stack above the mpath device, this is not
 # something that should be reacted upon since it would be useless extra work.
 # It's exactly mpath's job to provide *seamless* device access to any of the
 # paths that are available underneath.
 ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", \
-	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
+	ENV{MPATH_UNCHANGED}="1"
 
-# For path failed or reinstated events, unset DM_ACTIVATION.
+# For path failed or reinstated events, set MPATH_UNCHANGED.
 # This is similar to the DM_SUBSYSTEM_UDEV_FLAG0 case above.
 ENV{DM_ACTION}=="PATH_FAILED|PATH_REINSTATED", \
-	ENV{DM_ACTIVATION}="0", ENV{MPATH_UNCHANGED}="1"
+	ENV{MPATH_UNCHANGED}="1"
 
 LABEL="check_mpath_ready"
 
@@ -112,11 +112,10 @@ GOTO="scan_import"
 
 LABEL="mpath_is_ready"
 
-# If the device comes back online, set DM_ACTIVATION so that
+# If the device comes back online, clear MPATH_UNCHANGED so that
 # upper layers will do a rescan. Don't do this if .MPATH_DEVICE_READY_OLD
 # is just empty (see comment above the DM_COOKIE test above).
-ENV{.MPATH_DEVICE_READY_OLD}=="0", \
-	ENV{DM_ACTIVATION}="1", ENV{MPATH_UNCHANGED}="0"
+ENV{.MPATH_DEVICE_READY_OLD}=="0", ENV{MPATH_UNCHANGED}="0"
 
 # The code to check multipath state ends here. We need to set
 # properties and symlinks regardless whether the map is usable or
@@ -146,6 +145,10 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
 
 LABEL="import_end"
 
+# If MPATH_UNCHANGED is set, adapt DM_ACTIVATION.
+ENV{MPATH_UNCHANGED}=="0", ENV{DM_ACTIVATION}="1"
+ENV{MPATH_UNCHANGED}=="1", ENV{DM_ACTIVATION}="0"
+
 # Reset previous DM_COLDPLUG_SUSPENDED if activation happens now
 ENV{.DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}=""
 
-- 
2.47.0

[PATCH v2 4/5] 11-dm-mpath-rules.in: skip one .DM_NOSCAN check

[Martin Wilck]

We set .DM_NOSCAN above where we set DM_UDEV_DISABLE_OTHER_RULES_FLAG, too. If
the latter isn't set, .DM_NOSCAN can't be set. Skip the redundant test.

Signed-off-by: Martin Wilck <mwilck@suse.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 a2655cb..79227be 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -132,7 +132,7 @@ ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}!="1", GOTO="import_end"
 ENV{DM_UDEV_RULES_VSN}!="1|2", GOTO="import_end"
 
 # Don't import the properties from db if we will run blkid later.
-ENV{.DM_NOSCAN}!="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="1", GOTO="import_end"
+ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}!="1", GOTO="import_end"
 
 IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
-- 
2.47.0

[PATCH v2 5/5] 11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

[Martin Wilck]

When multipath reloads a device or fails or restores a path, the udev
rules disable LVM scanning, but since .DM_NOSCAN isn't set, blkid is
still run on the device. When multipath devices that are set to
queue_if_no_path lose all their paths at close to the same time, udev
workers can hang trying to run blkid. The blkid results shouldn't
change when multipathd is adding, removing, failing or reinstating
paths, aside from avoiding hanging udev processes, we're skipping
unnecessary work.

Hence, set .DM_NOSCAN if MPATH_UNCHANGED is set, to avoid blkid from
being called in 13-dm.rules.

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

diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
index 79227be..a816edb 100644
--- a/multipath/11-dm-mpath.rules.in
+++ b/multipath/11-dm-mpath.rules.in
@@ -145,9 +145,11 @@ IMPORT{db}="ID_PART_GPT_AUTO_ROOT"
 
 LABEL="import_end"
 
-# If MPATH_UNCHANGED is set, adapt DM_ACTIVATION.
+# If MPATH_UNCHANGED is set, adapt DM_ACTIVATION and DM_NOSCAN.
+# .DM_NOSCAN controls whether blkid will be run in 13-dm-disk.rules;
+# we don't want to do that if MPATH_UNCHANGED is 1.
 ENV{MPATH_UNCHANGED}=="0", ENV{DM_ACTIVATION}="1"
-ENV{MPATH_UNCHANGED}=="1", ENV{DM_ACTIVATION}="0"
+ENV{MPATH_UNCHANGED}=="1", ENV{DM_ACTIVATION}="0", ENV{.DM_NOSCAN}="1"
 
 # Reset previous DM_COLDPLUG_SUSPENDED if activation happens now
 ENV{.DM_SUSPENDED}!="1", ENV{DM_ACTIVATION}=="1", ENV{DM_COLDPLUG_SUSPENDED}=""
-- 
2.47.0

Re: [PATCH v2 5/5] 11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

[Zdenek Kabelac]

Dne 03. 11. 24 v 23:43 Martin Wilck napsal(a):
> When multipath reloads a device or fails or restores a path, the udev
> rules disable LVM scanning, but since .DM_NOSCAN isn't set, blkid is
> still run on the device. When multipath devices that are set to
> queue_if_no_path lose all their paths at close to the same time, udev


Hi


There is worth to be mentioned -   blkid has for a long time 'special' rule 
-built-in   for LVM (and some other devices i.e. crypt...)

So whenever lvm2  uses   LVM-uuid-suffix     with the '-suffix' in it's UUID - 
it's a private DM device which is never even opened by blkid. We would like to 
convert all remaining LV activations to use this logic for every 'private' 
device - since this logic is 'persistent' and survive even complete  removal 
od udevdb. However it takes some time and also the backward compatibility is 
complicating things a lot.

But all I say is - the relaing on any logic of  the udev flags is always kind 
of  problematic - and lvm2 does not depend on this fragile flagging too much.


Regards


Zdenk

Re: [PATCH v2 5/5] 11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

[Martin Wilck]

On Mon, 2024-11-04 at 15:00 +0100, Zdenek Kabelac wrote:
> Dne 03. 11. 24 v 23:43 Martin Wilck napsal(a):
> > When multipath reloads a device or fails or restores a path, the
> > udev
> > rules disable LVM scanning, but since .DM_NOSCAN isn't set, blkid
> > is
> > still run on the device. When multipath devices that are set to
> > queue_if_no_path lose all their paths at close to the same time,
> > udev
> 
> 
> Hi
> 
> 
> There is worth to be mentioned -   blkid has for a long time
> 'special' rule 
> -built-in   for LVM (and some other devices i.e. crypt...)
> 
> So whenever lvm2  uses   LVM-uuid-suffix     with the '-suffix' in
> it's UUID - 
> it's a private DM device which is never even opened by blkid. We
> would like to 
> convert all remaining LV activations to use this logic for every
> 'private' 
> device - since this logic is 'persistent' and survive even complete 
> removal 
> od udevdb. However it takes some time and also the backward
> compatibility is 
> complicating things a lot.
> 
> But all I say is - the relaing on any logic of  the udev flags is
> always kind 
> of  problematic - and lvm2 does not depend on this fragile flagging
> too much.

Thanks for the hint. I think you're referring to libblkid's commit
20e1c3d ("libblkid: ignore private LVM devices"), which makes blkid
ignore devices with LVM UUIDs matching "LVM-.*-.*". I actually hadn't
been aware of this.

But note that multipath is overloading the .DM_NOSCAN flag here to tell
higher layers that blkid shouldn't be run in the first place if a
multipath map is in a certain state (we're reusing this flag, which was
originally created for LVM, in a similar but not identical case where
we want the blkid call to be skipped).

Unlike LVM, multipath has no way to set a UUID with special properties,
because multipath doesn't own any meta data on disk. In the case of
multipath, the .DM_NOSCAN property can, and will, change for a map
while its UUID remains constant. That's what this patch is about.

Thanks
Martin

Re: [PATCH v2 5/5] 11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

[Zdenek Kabelac]

Dne 04. 11. 24 v 16:03 Martin Wilck napsal(a):
> On Mon, 2024-11-04 at 15:00 +0100, Zdenek Kabelac wrote:
>> Dne 03. 11. 24 v 23:43 Martin Wilck napsal(a):
>>> When multipath reloads a device or fails or restores a path, the
>>> udev
>>> rules disable LVM scanning, but since .DM_NOSCAN isn't set, blkid
>>> is
>>> still run on the device. When multipath devices that are set to
>>> queue_if_no_path lose all their paths at close to the same time,
>>> udev
> But note that multipath is overloading the .DM_NOSCAN flag here to tell
> higher layers that blkid shouldn't be run in the first place if a
> multipath map is in a certain state (we're reusing this flag, which was
> originally created for LVM, in a similar but not identical case where
> we want the blkid call to be skipped).
> 
> Unlike LVM, multipath has no way to set a UUID with special properties,
> because multipath doesn't own any meta data on disk. In the case of
> multipath, the .DM_NOSCAN property can, and will, change for a map
> while its UUID remains constant. That's what this patch is about.

Hi

It's been more about the idea - that you can try to communicate some
protocol logic for 'libblkid' in some way - that will automatically
make sure blkid will not try to open device in these unwanted cases.

It could be possibly something like:

if DM device is  'mpath' &&  file  /run/mpath/do_not_open_MPATH_UUID exists
the this will be makeing libblkid think it's a device it should not
be opened.

Then mpath tool is just manages present of this file - so this way you
do not rely on works of any udev flag.

Although the case here can be different if the device as such is supposed to 
be a udev visible volume - as the  blkid skipping might make the volume just 
'invisible' as whole.

Regards

Zdenek

Re: [PATCH v2 5/5] 11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set

[Martin Wilck]

On Mon, 2024-11-04 at 17:55 +0100, Zdenek Kabelac wrote:
> Dne 04. 11. 24 v 16:03 Martin Wilck napsal(a):
> > On Mon, 2024-11-04 at 15:00 +0100, Zdenek Kabelac wrote:
> > > Dne 03. 11. 24 v 23:43 Martin Wilck napsal(a):
> > > > When multipath reloads a device or fails or restores a path,
> > > > the
> > > > udev
> > > > rules disable LVM scanning, but since .DM_NOSCAN isn't set,
> > > > blkid
> > > > is
> > > > still run on the device. When multipath devices that are set to
> > > > queue_if_no_path lose all their paths at close to the same
> > > > time,
> > > > udev
> > But note that multipath is overloading the .DM_NOSCAN flag here to
> > tell
> > higher layers that blkid shouldn't be run in the first place if a
> > multipath map is in a certain state (we're reusing this flag, which
> > was
> > originally created for LVM, in a similar but not identical case
> > where
> > we want the blkid call to be skipped).
> > 
> > Unlike LVM, multipath has no way to set a UUID with special
> > properties,
> > because multipath doesn't own any meta data on disk. In the case of
> > multipath, the .DM_NOSCAN property can, and will, change for a map
> > while its UUID remains constant. That's what this patch is about.
> 
> Hi
> 
> It's been more about the idea - that you can try to communicate some
> protocol logic for 'libblkid' in some way - that will automatically
> make sure blkid will not try to open device in these unwanted cases.
> 
> It could be possibly something like:
> 
> if DM device is  'mpath' &&  file  /run/mpath/do_not_open_MPATH_UUID
> exists
> the this will be makeing libblkid think it's a device it should not
> be opened.
> 
> Then mpath tool is just manages present of this file - so this way
> you
> do not rely on works of any udev flag.

We'd still need to manipulate these "flag" files through udev rules,
and we'd have to make sure no stale such file remain in place. I don't
see the advantage of this approach yet (for multipath).

We are are already using this sort of "flag file" mechanism to
communicate between udev rules and multipathd, where environment
variables can't be used. Again, in the LVM case, certain devices are
always "private" to LVM and it makes no sense to run blkid on them, not
even manually. But that isn't the case for multipath.

Martin

Re: [PATCH v2 0/5] multipath-tools: udev rule fixes

[Benjamin Marzinski]

On Sun, Nov 03, 2024 at 11:43:44PM +0100, Martin Wilck wrote:
> Here are more fixes for the multipath udev rules. 2/5 is an actual fix,
> 1/5, 3/5 and 4/5 are minor cleanups / code clarifications.
> 
> 5/5 implements the fix originally submitted by Ben [1].
> 
> Reviews and comments welcome.
> Martin

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

> 
> [1] https://lore.kernel.org/dm-devel/20241022233349.247087-1-bmarzins@redhat.com/
> 
> Martin Wilck (5):
>   11-dm-mpath.rules.in: import DM_COLDPLUG_SUSPENDED only once
>   11-dm-mpath.rules.in: handle inactive suspended devices correctly
>   11-dm-mpath.rules.in: clarify DM_ACTIVATION logic
>   11-dm-mpath-rules.in: skip one .DM_NOSCAN check
>   11-dm-mpath.rules.in: set .DM_NOSCAN if MPATH_UNCHANGED is set
> 
>  multipath/11-dm-mpath.rules.in | 44 ++++++++++++++++++++--------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> -- 
> 2.47.0