On Sat, Feb 10, 2024 at 12:27:32AM +0100, Martin Wilck wrote:
> 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 "z0" for this rule's prefix to make sure it needs to run
> very late (after 99-systemd.rules, for example) while still allowing some
> reasonable space after it.
>
> Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
Is z0-dm-mpath-last.rules the right way to name this? I'm a little
worried that something will expect a two-digit beginning to udev rules
files, and a name like 99-z0-dm-mpath-last.rules would be more
appropriate (if even longer). In a perfect world we'd be able to get all
all the rules to use .DM_UDEV_DISABLE_OTHER_RULES_FLAG which was
initialized to DM_UDEV_DISABLE_OTHER_RULES_FLAG in 10-dm.rules, so we
had a non-saved flag to play with.
But assuming that z0-dm-mpath-last.rules is o.k., everything else looks
fine, so
Reviewed-by: Benjamin Marzinski <bmarzins@xxxxxxxxxx>
> ---
> multipath/11-dm-mpath.rules.in | 19 +++++++++----------
> multipath/Makefile | 2 ++
> multipath/z0-dm-mpath-late.rules | 4 ++++
> 3 files changed, 15 insertions(+), 10 deletions(-)
> create mode 100644 multipath/z0-dm-mpath-late.rules
>
> diff --git a/multipath/11-dm-mpath.rules.in b/multipath/11-dm-mpath.rules.in
> index 54d01a6..e31107b 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)
> @@ -79,16 +78,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/Makefile b/multipath/Makefile
> index f8c1f5e..2f2569c 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 z0-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)/z0-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
> diff --git a/multipath/z0-dm-mpath-late.rules b/multipath/z0-dm-mpath-late.rules
> new file mode 100644
> index 0000000..8574b1a
> --- /dev/null
> +++ b/multipath/z0-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}"
> --
> 2.43.0
