Re: [PATCH v2 4/7] 11-dm-mpath.rules: don't save DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux