[RFC PATCH 0/7] device mapper udev rules rework

[RFC PATCH 0/7] device mapper udev rules rework

[Martin Wilck]

All,

here is a draft for a rewrite of the device mapper udev rules, as follow up
to the previous thread "About DM_UDEV_DISABLE_OTHER_RULES_FLAG and
DM_NOSCAN" [1]. While I have taken care to minimize the impact on
other udev rule sets, some changes to both the multipath-tools and
systemd rules will be necessary. Patches for these are in preparation,
but I'd like to reach consensus on the dm part first.

As discussed in [1], the idea is to use just one udev property as "API" for
later rules to determine whether the device at hand can be probed or
otherwise accessed. This API remains DM_UDEV_DISABLE_OTHER_RULES_FLAG,
because it is widely used by other rule sets. This means that the flag of
the same name that is set in the DM_COOKIE needs to be saved and restored
via a different property. For this property, I've chosen the name
DM_COOKIE_DISABLE_OTHER_RULES_FLAG, which is hopefully awkward enough that
authors of non-dm rules won't consider using it.

DM_UDEV_DISABLE_OTHER_RULES_FLAG is a logical "OR" of various conditions
that should prevent other rules from accessing a device. This includes the
cookie flag, the "suspended" flag, and the "noscan" flag
(DM_SUBSYSTEM_UDEV_FLAG0=1). dm-multipath has more such conditions.

Because DM_SUSPENDED and DM_NOSCAN are now purely internal and don't need to
be restored from the udev db, they are renamed to .DM_SUSPENDED and
.DM_NOSCAN, respectively.

For follow-up rules, the semantics of DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
is "don't probe, don't attempt IO, and keep existing device properties
(if any) until subsequent uevents are received". It doesn't mean the
device should be completely ignored, and it doesn't mean that the block
layer stack on top of the device should be destroyed. Contrary to what
we discussed in [1], I didn't try to come up with flags carrying these
other meanings. I the LVM layer doesn't have the necessary information
for doing this. The only flag with "destroy" semantics I am aware of
is SYSTEMD_READY="0", and it's up to systemd to deal with that.

The first two patches are minor fixes that I came up with while working
on the rules.

[1] https://lore.kernel.org/linux-lvm/9d50edb0-baa4-4a01-a2f0-136dfdb79937@redhat.com/T/#t

Martin Wilck (7):
  13-dm-disk.rules: import ID_FS_TYPE
  10-dm.rules: don't deactivate devices for DISK_RO=1
  10-dm-rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from db
  11-dm-lvm.rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from
    db
  dm udev rules: don't export and save DM_SUSPENDED
  dm udev rules: don't export and save DM_NOSCAN
  10-dm.rules: bump DM_UDEV_RULES_VSN to 3

 udev/10-dm.rules.in          | 32 +++++++++++++++++++++-----------
 udev/11-dm-lvm.rules.in      | 13 +++++--------
 udev/12-dm-permissions.rules |  2 +-
 udev/13-dm-disk.rules.in     |  9 +++++----
 4 files changed, 32 insertions(+), 24 deletions(-)

-- 
2.43.2

[RFC PATCH 1/7] 13-dm-disk.rules: import ID_FS_TYPE

[Martin Wilck]

ID_FS_TYPE is the most important udev property for most follow-up
rules. It must be imported from the udev db if blkid can't be run.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/13-dm-disk.rules.in | 1 +
 1 file changed, 1 insertion(+)

diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
index dca00bc..eaad972 100644
--- a/udev/13-dm-disk.rules.in
+++ b/udev/13-dm-disk.rules.in
@@ -26,6 +26,7 @@ ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
 GOTO="dm_link"
 
 LABEL="dm_import"
+IMPORT{db}="ID_FS_TYPE"
 IMPORT{db}="ID_FS_USAGE"
 IMPORT{db}="ID_FS_UUID_ENC"
 IMPORT{db}="ID_FS_LABEL_ENC"
-- 
2.43.2

[RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Martin Wilck]

DISK_RO=1 is set in the environment of a block-device uevent if and only if
the device has just been set read-only by a driver by calling set_disk_ro().
It is not synoymous with the "ro" sysfs attribute; the device could very well
be write-protected if DISK_RO is not set. Probing should be possible even if
this flag is set, because blkid and friends usually don't write to the
device. Upper-layer subsystems that do need to write would need to check the
"ro" sysfs attribute rather than DISK_RO.

Skip the DISK_RO check in 10-dm.rules.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/10-dm.rules.in | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
index 4ffd3e2..c08d829 100644
--- a/udev/10-dm.rules.in
+++ b/udev/10-dm.rules.in
@@ -50,9 +50,6 @@ ACTION!="add|change", GOTO="dm_end"
 # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
 ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
 
-# Rule out easy-to-detect inappropriate events first.
-ENV{DISK_RO}=="1", GOTO="dm_disable"
-
 # There is no cookie set nor any flags encoded in events not originating
 # in libdevmapper so we need to detect this and try to behave correctly.
 # For such spurious events, regenerate all flags from current udev database content
-- 
2.43.2

[RFC PATCH 3/7] 10-dm-rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from db

[Martin Wilck]

We use DM_UDEV_DISABLE_OTHER_RULES_FLAG to tell upper non-DM layers
to keep their hands off the device in question, for any reason.
One possible reason is that the device is supended; another is that
the cookie carries the flag of the same name.

DM_SUSPENDED is not restored from the db, but evaluated anew for every
uevent. Therefore DM_UDEV_DISABLE_OTHER_RULES_FLAG shouldn't be
restored, either. Use a new variable DM_COOKIE_DISABLE_OTHER_RULES_FLAG
to save and restore the original value from the cookie.

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

diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
index c08d829..ef36209 100644
--- a/udev/10-dm.rules.in
+++ b/udev/10-dm.rules.in
@@ -48,7 +48,14 @@ ACTION!="add|change", GOTO="dm_end"
 # These flags are encoded in DM_COOKIE variable that was introduced in
 # kernel version 2.6.31. Therefore, we can use this feature with
 # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
-ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
+ENV{DM_COOKIE}!="?*", GOTO="dm_no_cookie"
+IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
+
+# Store the original flag from the cookie as DM_COOKIE_DISABLE_OTHER_RULES_FLAG
+# in the udev db. DM_UDEV_DISABLE_OTHER_RULES_FLAG will be or'd with other
+# conditions for use by upper, non-dm layers.
+ENV{DM_COOKIE_DISABLE_OTHER_RULES_FLAG}="%E{DM_UDEV_DISABLE_OTHER_RULES_FLAG}"
+LABEL="dm_no_cookie"
 
 # There is no cookie set nor any flags encoded in events not originating
 # in libdevmapper so we need to detect this and try to behave correctly.
@@ -58,12 +65,13 @@ ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", ENV{DM_ACTIVATION}="1", GOTO="dm_flags_do
 IMPORT{db}="DM_UDEV_DISABLE_DM_RULES_FLAG"
 IMPORT{db}="DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG"
 IMPORT{db}="DM_UDEV_DISABLE_DISK_RULES_FLAG"
-IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
+IMPORT{db}="DM_COOKIE_DISABLE_OTHER_RULES_FLAG"
 IMPORT{db}="DM_UDEV_LOW_PRIORITY_FLAG"
 IMPORT{db}="DM_UDEV_DISABLE_LIBRARY_FALLBACK_FLAG"
 IMPORT{db}="DM_UDEV_PRIMARY_SOURCE_FLAG"
 IMPORT{db}="DM_UDEV_FLAG7"
 IMPORT{db}="DM_UDEV_RULES_VSN"
+ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="%E{DM_COOKIE_DISABLE_OTHER_RULES_FLAG}"
 LABEL="dm_flags_done"
 
 # Normally, we operate on "change" events. But when coldplugging, there's an
-- 
2.43.2

[RFC PATCH 4/7] 11-dm-lvm.rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from db

[Martin Wilck]

DM_UDEV_DISABLE_OTHER_RULES_FLAG is used as the "output" flag of the
device-mapper rules, to be consumed by non-dm rules. It is a logical OR of
several conditions that might make dm devices inaccessible. 10-dm.rules
calculates it for every uevent, whether it's genuine or spurious.

DM_SUBSYSTEM_UDEV_FLAG0 is just another flag that needs to be or'd in. We
don't need to restore the previous state of DM_UDEV_DISABLE_OTHER_RULES_FLAG.
Actually, doing so is wrong if the flag has previously been set because the
device was suspended, and the device isn't suspended anymore.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/11-dm-lvm.rules.in | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/udev/11-dm-lvm.rules.in b/udev/11-dm-lvm.rules.in
index 7c58994..0b77fe2 100644
--- a/udev/11-dm-lvm.rules.in
+++ b/udev/11-dm-lvm.rules.in
@@ -26,14 +26,11 @@ IMPORT{program}="(DM_EXEC)/dmsetup splitname --nameprefixes --noheadings --rows
 # to zero any stale metadata found within the LV data area. Such stale
 # metadata could cause false claim of the LV device, keeping it open etc.
 #
-# If the NOSCAN flag is present, backup selected existing flags used to
-# disable rules, then set them firmly so those selected rules are surely skipped.
-# Restore these flags once the NOSCAN flag is dropped (which is normally any
-# uevent that follows for this LV, even an artificially generated one).
-ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
-ENV{DM_SUBSYSTEM_UDEV_FLAG0}!="1", IMPORT{db}="DM_NOSCAN", IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
-ENV{DM_SUBSYSTEM_UDEV_FLAG0}!="1", ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}", \
-				   ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="", ENV{DM_NOSCAN}=""
+# If the NOSCAN flag is present, set DM_UDEV_DISABLE_OTHER_RULES_FLAG
+# so those selected rules are surely skipped.
+# We don't need to save and restore the previous of DM_UDEV_DISABLE_OTHER_RULES_FLAG,
+# that's taken care of in 10-dm.rules.
+ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 
 ENV{DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG}=="1", GOTO="lvm_end"
 
-- 
2.43.2

[RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Martin Wilck]

DM_SUSPENDED is a device-mapper internal flag, which is not intended to be
used by other rules, and which is determined by 10-dm.rules from sysfs for
every uevent. Rename it to ".DM_SUSPENDED", so that it won't be saved in the
udev database.

Known consumers of DM_SUSPENDED are 66-kpartx.rules (from multipath-tools) and
99-systemd.rules (from systemd). These will have to be adapted.
11-dm-mpath.rules will be changed to use .DM_SUSPENDED.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/10-dm.rules.in          | 15 +++++++++------
 udev/12-dm-permissions.rules |  2 +-
 udev/13-dm-disk.rules.in     |  4 ++--
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
index ef36209..d30f663 100644
--- a/udev/10-dm.rules.in
+++ b/udev/10-dm.rules.in
@@ -11,7 +11,7 @@
 # for use in later rules:
 #   DM_NAME - actual DM device's name
 #   DM_UUID - UUID set for DM device (blank if not specified)
-#   DM_SUSPENDED - suspended state of DM device (0 or 1)
+#   .DM_SUSPENDED - suspended state of DM device (0 or 1)
 #   DM_UDEV_RULES_VSN - DM udev rules version
 #
 # These rules cover only basic device-mapper functionality in udev.
@@ -114,15 +114,18 @@ LABEL="dm_no_coldplug"
 # The "suspended" item was added even later (kernels >= 2.6.31),
 # so we also have to call dmsetup if the kernel version used
 # is in between these releases.
-TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{DM_SUSPENDED}="$attr{dm/suspended}"
+TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{.DM_SUSPENDED}="$attr{dm/suspended}"
 TEST!="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o name,uuid,suspended"
-ENV{DM_SUSPENDED}!="?*", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended"
 
+ENV{.DM_SUSPENDED}=="?*", GOTO="dm_suspended_set"
+TEST=="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended"
 # dmsetup tool provides suspended state information in textual
 # form with values "Suspended"/"Active". We translate it to
 # 0/1 respectively to be consistent with sysfs values.
-ENV{DM_SUSPENDED}=="Active", ENV{DM_SUSPENDED}="0"
-ENV{DM_SUSPENDED}=="Suspended", ENV{DM_SUSPENDED}="1"
+ENV{DM_SUSPENDED}=="Active", ENV{.DM_SUSPENDED}="0"
+ENV{DM_SUSPENDED}=="Suspended", ENV{.DM_SUSPENDED}="1"
+ENV{DM_SUSPENDED}=""
+LABEL="dm_suspended_set"
 
 # This variable provides a reliable way to check that device-mapper
 # rules were installed. It means that all needed variables are set
@@ -140,7 +143,7 @@ ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/
 # Avoid processing and scanning a DM device in the other (foreign)
 # rules if it is in suspended state. However, we still keep 'disk'
 # and 'DM subsystem' related rules enabled in this case.
-ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 
 GOTO="dm_end"
 
diff --git a/udev/12-dm-permissions.rules b/udev/12-dm-permissions.rules
index a9d4c32..6a69d2f 100644
--- a/udev/12-dm-permissions.rules
+++ b/udev/12-dm-permissions.rules
@@ -14,7 +14,7 @@
 #   DM_UDEV_RULES_VSN - DM udev rules version
 #   DM_NAME - actual DM device's name
 #   DM_UUID - UUID set for DM device (blank if not specified)
-#   DM_SUSPENDED - suspended state of DM device (0 or 1)
+#   .DM_SUSPENDED - suspended state of DM device (0 or 1)
 #   DM_LV_NAME - logical volume name (not set if LVM device not present)
 #   DM_VG_NAME - volume group name (not set if LVM device not present)
 #   DM_LV_LAYER - logical volume layer (not set if LVM device not present)
diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
index eaad972..cb2ce2d 100644
--- a/udev/13-dm-disk.rules.in
+++ b/udev/13-dm-disk.rules.in
@@ -17,9 +17,9 @@ ENV{DM_UDEV_DISABLE_DISK_RULES_FLAG}=="1", GOTO="dm_end"
 SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}"
 ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}"
 
-ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
+ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
 ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
-ENV{DM_SUSPENDED}=="1", GOTO="dm_end"
+ENV{.DM_SUSPENDED}=="1", GOTO="dm_end"
 ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
 
 (BLKID_RULE)
-- 
2.43.2

[RFC PATCH 6/7] dm udev rules: don't export and save DM_NOSCAN

[Martin Wilck]

DM_NOSCAN is not an official API any more and doesn't have to be
restored from the udev db. Rename it to .DM_NOSCAN.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/11-dm-lvm.rules.in  | 2 +-
 udev/13-dm-disk.rules.in | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/udev/11-dm-lvm.rules.in b/udev/11-dm-lvm.rules.in
index 0b77fe2..d0a5637 100644
--- a/udev/11-dm-lvm.rules.in
+++ b/udev/11-dm-lvm.rules.in
@@ -30,7 +30,7 @@ IMPORT{program}="(DM_EXEC)/dmsetup splitname --nameprefixes --noheadings --rows
 # so those selected rules are surely skipped.
 # We don't need to save and restore the previous of DM_UDEV_DISABLE_OTHER_RULES_FLAG,
 # that's taken care of in 10-dm.rules.
-ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
+ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{.DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
 
 ENV{DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG}=="1", GOTO="lvm_end"
 
diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
index cb2ce2d..7989871 100644
--- a/udev/13-dm-disk.rules.in
+++ b/udev/13-dm-disk.rules.in
@@ -18,9 +18,9 @@ SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}"
 ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}"
 
 ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
-ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
+ENV{.DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
 ENV{.DM_SUSPENDED}=="1", GOTO="dm_end"
-ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
+ENV{.DM_NOSCAN}=="1", GOTO="dm_watch"
 
 (BLKID_RULE)
 GOTO="dm_link"
-- 
2.43.2

[RFC PATCH 7/7] 10-dm.rules: bump DM_UDEV_RULES_VSN to 3

[Martin Wilck]

Bump the rules version in order to indicate that upper level rules
should consume DM_UDEV_DISABLE_OTHER_RULES_FLAG rather than DM_NOSCAN
and DM_SUSPENDED.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev/10-dm.rules.in | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
index d30f663..21bbcb0 100644
--- a/udev/10-dm.rules.in
+++ b/udev/10-dm.rules.in
@@ -136,7 +136,9 @@ LABEL="dm_suspended_set"
 # possible future changes.
 # VSN 1 - original rules
 # VSN 2 - add support for synthesized events
-ENV{DM_UDEV_RULES_VSN}="2"
+# VSN 3 - use DM_UDEV_DISABLE_OTHER_RULES_FLAG as the only "API"
+#         to be consumed by non-dm rules.
+ENV{DM_UDEV_RULES_VSN}="3"
 
 ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/$env{DM_NAME}"
 
-- 
2.43.2

Re: [RFC PATCH 1/7] 13-dm-disk.rules: import ID_FS_TYPE

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> ID_FS_TYPE is the most important udev property for most follow-up
> rules. It must be imported from the udev db if blkid can't be run.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/13-dm-disk.rules.in | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
> index dca00bc..eaad972 100644
> --- a/udev/13-dm-disk.rules.in
> +++ b/udev/13-dm-disk.rules.in
> @@ -26,6 +26,7 @@ ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
>  GOTO="dm_link"
>  
>  LABEL="dm_import"
> +IMPORT{db}="ID_FS_TYPE"
>  IMPORT{db}="ID_FS_USAGE"
>  IMPORT{db}="ID_FS_UUID_ENC"
>  IMPORT{db}="ID_FS_LABEL_ENC"

I think this one was left out deliberately. The original intention was
to import only the minimal set of "blkid" variables needed to properly
keep the symlinks based on these values in place (patch 94f77a4). And
the "ID_FS_TYPE" is not actually needed for this.

Though, importing even the "ID_FS_TYPE" together with other blkid
variables should not be an issue because this "import from previous
state" happens only if we have DM_SUSPENDED or DM_NOSCAN set for current
event, in which case any other rules should be also disabled with
DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".

So even if some other rule fires based on ID_FS_TYPE value, the
DM_UDEV_DISABLE_OTHER_RULES_FLAG should stop it anyway.

Then, the question is whether we really need to 'IMPORT{db}="ID_FS_TYPE'?

-- 
Peter

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> DISK_RO=1 is set in the environment of a block-device uevent if and only if
> the device has just been set read-only by a driver by calling set_disk_ro().
> It is not synoymous with the "ro" sysfs attribute; the device could very well
> be write-protected if DISK_RO is not set. Probing should be possible even if
> this flag is set, because blkid and friends usually don't write to the
> device. Upper-layer subsystems that do need to write would need to check the
> "ro" sysfs attribute rather than DISK_RO.
> 
> Skip the DISK_RO check in 10-dm.rules.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/10-dm.rules.in | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
> index 4ffd3e2..c08d829 100644
> --- a/udev/10-dm.rules.in
> +++ b/udev/10-dm.rules.in
> @@ -50,9 +50,6 @@ ACTION!="add|change", GOTO="dm_end"
>  # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
>  ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
>  
> -# Rule out easy-to-detect inappropriate events first.
> -ENV{DISK_RO}=="1", GOTO="dm_disable"
> -
>  # There is no cookie set nor any flags encoded in events not originating
>  # in libdevmapper so we need to detect this and try to behave correctly.
>  # For such spurious events, regenerate all flags from current udev database content


Yes, I'd like to get rid of this rule, but, unfortunately, there's one
issue during the DM device creation/activation.

For example, if I try:

  dmsetup create --readonly --table "0 1 zero"

Then I get these uevents:

1)
ACTION=add
DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
DM_UDEV_DISABLE_DISK_RULES_FLAG=1
SYSTEMD_READY=0


2)
ACTION=change
DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
DM_UDEV_DISABLE_DISK_RULES_FLAG=1
DM_UDEV_DISABLE_OTHER_RULES_FLAG=


3)
ACTION=change
DM_COOKIE=6335392
DM_COOKIE_DISABLE_OTHER_RULES_FLAG=


The uevent 3) coming with the DM_COOKIE is the actual event when the
device is ready for use (that's the uevent notifying the DM device
resume/activation).

If we remove the DISK_RO rule, then the DM_UDEV_DISABLE_OTHER_RULES_FLAG
is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to get
dropped, which in turn will start all the systemd hooks because the
device is considered "ready" for systemd then.

But the DM dev is ready only after uevent 3) that comes with the
DM_COOKIE. So we still need to cover this scenario.

-- 
Peter

Re: [RFC PATCH 3/7] 10-dm-rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from db

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> We use DM_UDEV_DISABLE_OTHER_RULES_FLAG to tell upper non-DM layers
> to keep their hands off the device in question, for any reason.
> One possible reason is that the device is supended; another is that
> the cookie carries the flag of the same name.
> 
> DM_SUSPENDED is not restored from the db, but evaluated anew for every
> uevent. Therefore DM_UDEV_DISABLE_OTHER_RULES_FLAG shouldn't be
> restored, either. Use a new variable DM_COOKIE_DISABLE_OTHER_RULES_FLAG
> to save and restore the original value from the cookie.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/10-dm.rules.in | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
> index c08d829..ef36209 100644
> --- a/udev/10-dm.rules.in
> +++ b/udev/10-dm.rules.in
> @@ -48,7 +48,14 @@ ACTION!="add|change", GOTO="dm_end"
>  # These flags are encoded in DM_COOKIE variable that was introduced in
>  # kernel version 2.6.31. Therefore, we can use this feature with
>  # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
> -ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
> +ENV{DM_COOKIE}!="?*", GOTO="dm_no_cookie"
> +IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
> +
> +# Store the original flag from the cookie as DM_COOKIE_DISABLE_OTHER_RULES_FLAG
> +# in the udev db. DM_UDEV_DISABLE_OTHER_RULES_FLAG will be or'd with other
> +# conditions for use by upper, non-dm layers.
> +ENV{DM_COOKIE_DISABLE_OTHER_RULES_FLAG}="%E{DM_UDEV_DISABLE_OTHER_RULES_FLAG}"
> +LABEL="dm_no_cookie"
>  
>  # There is no cookie set nor any flags encoded in events not originating
>  # in libdevmapper so we need to detect this and try to behave correctly.
> @@ -58,12 +65,13 @@ ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", ENV{DM_ACTIVATION}="1", GOTO="dm_flags_do
>  IMPORT{db}="DM_UDEV_DISABLE_DM_RULES_FLAG"
>  IMPORT{db}="DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG"
>  IMPORT{db}="DM_UDEV_DISABLE_DISK_RULES_FLAG"
> -IMPORT{db}="DM_UDEV_DISABLE_OTHER_RULES_FLAG"
> +IMPORT{db}="DM_COOKIE_DISABLE_OTHER_RULES_FLAG"
>  IMPORT{db}="DM_UDEV_LOW_PRIORITY_FLAG"
>  IMPORT{db}="DM_UDEV_DISABLE_LIBRARY_FALLBACK_FLAG"
>  IMPORT{db}="DM_UDEV_PRIMARY_SOURCE_FLAG"
>  IMPORT{db}="DM_UDEV_FLAG7"
>  IMPORT{db}="DM_UDEV_RULES_VSN"
> +ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="%E{DM_COOKIE_DISABLE_OTHER_RULES_FLAG}"
>  LABEL="dm_flags_done"
>  
>  # Normally, we operate on "change" events. But when coldplugging, there's an

Looks good...

-- 
Peter

Re: [RFC PATCH 4/7] 11-dm-lvm.rules: don't restore DM_UDEV_DISABLE_OTHER_RULES_FLAG from db

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> DM_UDEV_DISABLE_OTHER_RULES_FLAG is used as the "output" flag of the
> device-mapper rules, to be consumed by non-dm rules. It is a logical OR of
> several conditions that might make dm devices inaccessible. 10-dm.rules
> calculates it for every uevent, whether it's genuine or spurious.
> 
> DM_SUBSYSTEM_UDEV_FLAG0 is just another flag that needs to be or'd in. We
> don't need to restore the previous state of DM_UDEV_DISABLE_OTHER_RULES_FLAG.
> Actually, doing so is wrong if the flag has previously been set because the
> device was suspended, and the device isn't suspended anymore.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/11-dm-lvm.rules.in | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/udev/11-dm-lvm.rules.in b/udev/11-dm-lvm.rules.in
> index 7c58994..0b77fe2 100644
> --- a/udev/11-dm-lvm.rules.in
> +++ b/udev/11-dm-lvm.rules.in
> @@ -26,14 +26,11 @@ IMPORT{program}="(DM_EXEC)/dmsetup splitname --nameprefixes --noheadings --rows
>  # to zero any stale metadata found within the LV data area. Such stale
>  # metadata could cause false claim of the LV device, keeping it open etc.
>  #
> -# If the NOSCAN flag is present, backup selected existing flags used to
> -# disable rules, then set them firmly so those selected rules are surely skipped.
> -# Restore these flags once the NOSCAN flag is dropped (which is normally any
> -# uevent that follows for this LV, even an artificially generated one).
> -ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="$env{DM_UDEV_DISABLE_OTHER_RULES_FLAG}", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
> -ENV{DM_SUBSYSTEM_UDEV_FLAG0}!="1", IMPORT{db}="DM_NOSCAN", IMPORT{db}="DM_DISABLE_OTHER_RULES_FLAG_OLD"
> -ENV{DM_SUBSYSTEM_UDEV_FLAG0}!="1", ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="$env{DM_DISABLE_OTHER_RULES_FLAG_OLD}", \
> -				   ENV{DM_DISABLE_OTHER_RULES_FLAG_OLD}="", ENV{DM_NOSCAN}=""
> +# If the NOSCAN flag is present, set DM_UDEV_DISABLE_OTHER_RULES_FLAG
> +# so those selected rules are surely skipped.
> +# We don't need to save and restore the previous of DM_UDEV_DISABLE_OTHER_RULES_FLAG,
> +# that's taken care of in 10-dm.rules.
> +ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
>  
>  ENV{DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG}=="1", GOTO="lvm_end"
>  

Looks good too, I gave it a quick test run and haven't noticed an issue
so far. Nice cleanup!

-- 
Peter

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> DM_SUSPENDED is a device-mapper internal flag, which is not intended to be
> used by other rules, and which is determined by 10-dm.rules from sysfs for
> every uevent. Rename it to ".DM_SUSPENDED", so that it won't be saved in the
> udev database.
> 
> Known consumers of DM_SUSPENDED are 66-kpartx.rules (from multipath-tools) and
> 99-systemd.rules (from systemd). These will have to be adapted.
> 11-dm-mpath.rules will be changed to use .DM_SUSPENDED.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/10-dm.rules.in          | 15 +++++++++------
>  udev/12-dm-permissions.rules |  2 +-
>  udev/13-dm-disk.rules.in     |  4 ++--
>  3 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
> index ef36209..d30f663 100644
> --- a/udev/10-dm.rules.in
> +++ b/udev/10-dm.rules.in
> @@ -11,7 +11,7 @@
>  # for use in later rules:
>  #   DM_NAME - actual DM device's name
>  #   DM_UUID - UUID set for DM device (blank if not specified)
> -#   DM_SUSPENDED - suspended state of DM device (0 or 1)
> +#   .DM_SUSPENDED - suspended state of DM device (0 or 1)
>  #   DM_UDEV_RULES_VSN - DM udev rules version
>  #
>  # These rules cover only basic device-mapper functionality in udev.
> @@ -114,15 +114,18 @@ LABEL="dm_no_coldplug"
>  # The "suspended" item was added even later (kernels >= 2.6.31),
>  # so we also have to call dmsetup if the kernel version used
>  # is in between these releases.
> -TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{DM_SUSPENDED}="$attr{dm/suspended}"
> +TEST=="dm", ENV{DM_NAME}="$attr{dm/name}", ENV{DM_UUID}="$attr{dm/uuid}", ENV{.DM_SUSPENDED}="$attr{dm/suspended}"
>  TEST!="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o name,uuid,suspended"
> -ENV{DM_SUSPENDED}!="?*", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended"
>  
> +ENV{.DM_SUSPENDED}=="?*", GOTO="dm_suspended_set"
> +TEST=="dm", IMPORT{program}="(DM_EXEC)/dmsetup info -j %M -m %m -c --nameprefixes --noheadings --rows -o suspended"
>  # dmsetup tool provides suspended state information in textual
>  # form with values "Suspended"/"Active". We translate it to
>  # 0/1 respectively to be consistent with sysfs values.
> -ENV{DM_SUSPENDED}=="Active", ENV{DM_SUSPENDED}="0"
> -ENV{DM_SUSPENDED}=="Suspended", ENV{DM_SUSPENDED}="1"
> +ENV{DM_SUSPENDED}=="Active", ENV{.DM_SUSPENDED}="0"
> +ENV{DM_SUSPENDED}=="Suspended", ENV{.DM_SUSPENDED}="1"
> +ENV{DM_SUSPENDED}=""
> +LABEL="dm_suspended_set"
>  
>  # This variable provides a reliable way to check that device-mapper
>  # rules were installed. It means that all needed variables are set
> @@ -140,7 +143,7 @@ ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/
>  # Avoid processing and scanning a DM device in the other (foreign)
>  # rules if it is in suspended state. However, we still keep 'disk'
>  # and 'DM subsystem' related rules enabled in this case.
> -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
> +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
>  
>  GOTO="dm_end"
>  
> diff --git a/udev/12-dm-permissions.rules b/udev/12-dm-permissions.rules
> index a9d4c32..6a69d2f 100644
> --- a/udev/12-dm-permissions.rules
> +++ b/udev/12-dm-permissions.rules
> @@ -14,7 +14,7 @@
>  #   DM_UDEV_RULES_VSN - DM udev rules version
>  #   DM_NAME - actual DM device's name
>  #   DM_UUID - UUID set for DM device (blank if not specified)
> -#   DM_SUSPENDED - suspended state of DM device (0 or 1)
> +#   .DM_SUSPENDED - suspended state of DM device (0 or 1)
>  #   DM_LV_NAME - logical volume name (not set if LVM device not present)
>  #   DM_VG_NAME - volume group name (not set if LVM device not present)
>  #   DM_LV_LAYER - logical volume layer (not set if LVM device not present)
> diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
> index eaad972..cb2ce2d 100644
> --- a/udev/13-dm-disk.rules.in
> +++ b/udev/13-dm-disk.rules.in
> @@ -17,9 +17,9 @@ ENV{DM_UDEV_DISABLE_DISK_RULES_FLAG}=="1", GOTO="dm_end"
>  SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}"
>  ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}"
>  
> -ENV{DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
> +ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
>  ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
> -ENV{DM_SUSPENDED}=="1", GOTO="dm_end"
> +ENV{.DM_SUSPENDED}=="1", GOTO="dm_end"
>  ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
>  
>  (BLKID_RULE)

OK, makes sense. But I haven't looked at what implications this might
have for 99-systemd.rules yet, but we surely need to have that covered
somehow.

Maybe, now, I would probably even remove the mention about DM_SUSPENDED
in 12-dm-permissions.rules, it looks superfluous there. We normally set
perms based on names, not on DM_SUSPENDED state. I'm not sure why we
mentioned it there before.

Do mpath rules still need to look at DM_SUSPENDED?

-- 
Peter

Re: [RFC PATCH 6/7] dm udev rules: don't export and save DM_NOSCAN

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> DM_NOSCAN is not an official API any more and doesn't have to be
> restored from the udev db. Rename it to .DM_NOSCAN.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/11-dm-lvm.rules.in  | 2 +-
>  udev/13-dm-disk.rules.in | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/udev/11-dm-lvm.rules.in b/udev/11-dm-lvm.rules.in
> index 0b77fe2..d0a5637 100644
> --- a/udev/11-dm-lvm.rules.in
> +++ b/udev/11-dm-lvm.rules.in
> @@ -30,7 +30,7 @@ IMPORT{program}="(DM_EXEC)/dmsetup splitname --nameprefixes --noheadings --rows
>  # so those selected rules are surely skipped.
>  # We don't need to save and restore the previous of DM_UDEV_DISABLE_OTHER_RULES_FLAG,
>  # that's taken care of in 10-dm.rules.
> -ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
> +ENV{DM_SUBSYSTEM_UDEV_FLAG0}=="1", ENV{.DM_NOSCAN}="1", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}="1"
>  
>  ENV{DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG}=="1", GOTO="lvm_end"
>  
> diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
> index cb2ce2d..7989871 100644
> --- a/udev/13-dm-disk.rules.in
> +++ b/udev/13-dm-disk.rules.in
> @@ -18,9 +18,9 @@ SYMLINK+="disk/by-id/dm-name-$env{DM_NAME}"
>  ENV{DM_UUID}=="?*", SYMLINK+="disk/by-id/dm-uuid-$env{DM_UUID}"
>  
>  ENV{.DM_SUSPENDED}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
> -ENV{DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
> +ENV{.DM_NOSCAN}=="1", ENV{DM_UDEV_PRIMARY_SOURCE_FLAG}=="1", GOTO="dm_import"
>  ENV{.DM_SUSPENDED}=="1", GOTO="dm_end"
> -ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
> +ENV{.DM_NOSCAN}=="1", GOTO="dm_watch"
>  
>  (BLKID_RULE)
>  GOTO="dm_link"

Yup, the DM_NOSCAN is internal and we don't need to import that for
subsequent events.

(Well, it's still used in 13-dm-disk.rules, but that's at least in our
hands.)

-- 
Peter

Re: [RFC PATCH 7/7] 10-dm.rules: bump DM_UDEV_RULES_VSN to 3

[Peter Rajnoha]

On 3/1/24 23:40, Martin Wilck wrote:
> Bump the rules version in order to indicate that upper level rules
> should consume DM_UDEV_DISABLE_OTHER_RULES_FLAG rather than DM_NOSCAN
> and DM_SUSPENDED.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev/10-dm.rules.in | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
> index d30f663..21bbcb0 100644
> --- a/udev/10-dm.rules.in
> +++ b/udev/10-dm.rules.in
> @@ -136,7 +136,9 @@ LABEL="dm_suspended_set"
>  # possible future changes.
>  # VSN 1 - original rules
>  # VSN 2 - add support for synthesized events
> -ENV{DM_UDEV_RULES_VSN}="2"
> +# VSN 3 - use DM_UDEV_DISABLE_OTHER_RULES_FLAG as the only "API"
> +#         to be consumed by non-dm rules.
> +ENV{DM_UDEV_RULES_VSN}="3"
>  
>  ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*", SYMLINK+="(DM_DIR)/$env{DM_NAME}"
>  

One thing that comes to my mind here is cooperation between the rules
from initrd/initramfs and rootfs - the initrd/initramfs can have
different versions of the rules installed. This was actually the
original reason for introducing such versioning so we can still try to
do our best even if the rules are mixed (to not cause a hang at boot
just because a proper symlink was not found undev /dev).

I haven't tested this with the new rules yet...

-- 
Peter

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Peter Rajnoha]

On 3/4/24 11:48, Peter Rajnoha wrote:
> On 3/1/24 23:40, Martin Wilck wrote:
>> DISK_RO=1 is set in the environment of a block-device uevent if and only if
>> the device has just been set read-only by a driver by calling set_disk_ro().
>> It is not synoymous with the "ro" sysfs attribute; the device could very well
>> be write-protected if DISK_RO is not set. Probing should be possible even if
>> this flag is set, because blkid and friends usually don't write to the
>> device. Upper-layer subsystems that do need to write would need to check the
>> "ro" sysfs attribute rather than DISK_RO.
>>
>> Skip the DISK_RO check in 10-dm.rules.
>>
>> Signed-off-by: Martin Wilck <mwilck@suse.com>
>> ---
>>  udev/10-dm.rules.in | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
>> index 4ffd3e2..c08d829 100644
>> --- a/udev/10-dm.rules.in
>> +++ b/udev/10-dm.rules.in
>> @@ -50,9 +50,6 @@ ACTION!="add|change", GOTO="dm_end"
>>  # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
>>  ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
>>  
>> -# Rule out easy-to-detect inappropriate events first.
>> -ENV{DISK_RO}=="1", GOTO="dm_disable"
>> -
>>  # There is no cookie set nor any flags encoded in events not originating
>>  # in libdevmapper so we need to detect this and try to behave correctly.
>>  # For such spurious events, regenerate all flags from current udev database content
> 
> 
> Yes, I'd like to get rid of this rule, but, unfortunately, there's one
> issue during the DM device creation/activation.
> 
> For example, if I try:
> 
>   dmsetup create --readonly --table "0 1 zero"
> 
> Then I get these uevents:
> 
> 1)
> ACTION=add
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
> SYSTEMD_READY=0
> 
> 
> 2)
> ACTION=change
> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=
> 
> 
> 3)
> ACTION=change
> DM_COOKIE=6335392
> DM_COOKIE_DISABLE_OTHER_RULES_FLAG=
> 
> 
> The uevent 3) coming with the DM_COOKIE is the actual event when the
> device is ready for use (that's the uevent notifying the DM device
> resume/activation).
> 
> If we remove the DISK_RO rule, then the DM_UDEV_DISABLE_OTHER_RULES_FLAG
> is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to get
> dropped, which in turn will start all the systemd hooks because the
> device is considered "ready" for systemd then.
> 
> But the DM dev is ready only after uevent 3) that comes with the
> DM_COOKIE. So we still need to cover this scenario.
> 

Hmm, this doesn't even work with original rules!

The 99-systemd.rules has:

SUBSYSTEM=="block", ACTION=="add",
ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", ENV{SYSTEMD_READY}="0"

which applies only to "add" uevent, not "change". This means that
SYSTEMD_READY="0" is even dropped for the DISK_RO="1" uevent even if it
is marked as DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".

This is actually a bug. But a minor one, because here we always have the
change uevent with DM_COOKIE coming right after the DISK_RO change
uevent. So there's almost no window practically.

Now, I'm asking myself why we have the 'ACTION="add"' in the systemd
rule at all. Why is it not just checking  DM_UDEV_DISABLE_OTHER_RULES_FLAG?

-- 
Peter

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Peter Rajnoha]

On 3/4/24 12:19, Peter Rajnoha wrote:
> On 3/4/24 11:48, Peter Rajnoha wrote:
>> On 3/1/24 23:40, Martin Wilck wrote:
>>> DISK_RO=1 is set in the environment of a block-device uevent if and only if
>>> the device has just been set read-only by a driver by calling set_disk_ro().
>>> It is not synoymous with the "ro" sysfs attribute; the device could very well
>>> be write-protected if DISK_RO is not set. Probing should be possible even if
>>> this flag is set, because blkid and friends usually don't write to the
>>> device. Upper-layer subsystems that do need to write would need to check the
>>> "ro" sysfs attribute rather than DISK_RO.
>>>
>>> Skip the DISK_RO check in 10-dm.rules.
>>>
>>> Signed-off-by: Martin Wilck <mwilck@suse.com>
>>> ---
>>>  udev/10-dm.rules.in | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
>>> index 4ffd3e2..c08d829 100644
>>> --- a/udev/10-dm.rules.in
>>> +++ b/udev/10-dm.rules.in
>>> @@ -50,9 +50,6 @@ ACTION!="add|change", GOTO="dm_end"
>>>  # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
>>>  ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
>>>  
>>> -# Rule out easy-to-detect inappropriate events first.
>>> -ENV{DISK_RO}=="1", GOTO="dm_disable"
>>> -
>>>  # There is no cookie set nor any flags encoded in events not originating
>>>  # in libdevmapper so we need to detect this and try to behave correctly.
>>>  # For such spurious events, regenerate all flags from current udev database content
>>
>>
>> Yes, I'd like to get rid of this rule, but, unfortunately, there's one
>> issue during the DM device creation/activation.
>>
>> For example, if I try:
>>
>>   dmsetup create --readonly --table "0 1 zero"
>>
>> Then I get these uevents:
>>
>> 1)
>> ACTION=add
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> SYSTEMD_READY=0
>>
>>
>> 2)
>> ACTION=change
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> 3)
>> ACTION=change
>> DM_COOKIE=6335392
>> DM_COOKIE_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> The uevent 3) coming with the DM_COOKIE is the actual event when the
>> device is ready for use (that's the uevent notifying the DM device
>> resume/activation).
>>
>> If we remove the DISK_RO rule, then the DM_UDEV_DISABLE_OTHER_RULES_FLAG
>> is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to get
>> dropped, which in turn will start all the systemd hooks because the
>> device is considered "ready" for systemd then.
>>
>> But the DM dev is ready only after uevent 3) that comes with the
>> DM_COOKIE. So we still need to cover this scenario.
>>
> 
> Hmm, this doesn't even work with original rules!
> 
> The 99-systemd.rules has:
> 
> SUBSYSTEM=="block", ACTION=="add",
> ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", ENV{SYSTEMD_READY}="0"
> 
> which applies only to "add" uevent, not "change". This means that
> SYSTEMD_READY="0" is even dropped for the DISK_RO="1" uevent even if it
> is marked as DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".
> 
> This is actually a bug. But a minor one, because here we always have the
> change uevent with DM_COOKIE coming right after the DISK_RO change
> uevent. So there's almost no window practically.
> 
> Now, I'm asking myself why we have the 'ACTION="add"' in the systemd
> rule at all. Why is it not just checking  DM_UDEV_DISABLE_OTHER_RULES_FLAG?
> 

Oh! :)

https://github.com/systemd/systemd/commit/35a6750d9e26b26b423fbe815bead7d750210d8d

-- 
Peter

Re: [RFC PATCH 1/7] 13-dm-disk.rules: import ID_FS_TYPE

[Martin Wilck]

On Mon, 2024-03-04 at 11:37 +0100, Peter Rajnoha wrote:
> On 3/1/24 23:40, Martin Wilck wrote:
> > ID_FS_TYPE is the most important udev property for most follow-up
> > rules. It must be imported from the udev db if blkid can't be run.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  udev/13-dm-disk.rules.in | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
> > index dca00bc..eaad972 100644
> > --- a/udev/13-dm-disk.rules.in
> > +++ b/udev/13-dm-disk.rules.in
> > @@ -26,6 +26,7 @@ ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
> >  GOTO="dm_link"
> >  
> >  LABEL="dm_import"
> > +IMPORT{db}="ID_FS_TYPE"
> >  IMPORT{db}="ID_FS_USAGE"
> >  IMPORT{db}="ID_FS_UUID_ENC"
> >  IMPORT{db}="ID_FS_LABEL_ENC"
> 
> I think this one was left out deliberately. The original intention
> was
> to import only the minimal set of "blkid" variables needed to
> properly
> keep the symlinks based on these values in place (patch 94f77a4). And
> the "ID_FS_TYPE" is not actually needed for this.

Why would we want to maintain the symlinks, but not other properties?
ID_FS_TYPE is the blkid property with the highest number of consumers
in a standard rule set. It is interpreted by most rule sets which build
additional layers on top of dm devices.

> Though, importing even the "ID_FS_TYPE" together with other blkid
> variables should not be an issue because this "import from previous
> state" happens only if we have DM_SUSPENDED or DM_NOSCAN set for
> current
> event, in which case any other rules should be also disabled with
> DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".
> 
> So even if some other rule fires based on ID_FS_TYPE value, the
> DM_UDEV_DISABLE_OTHER_RULES_FLAG should stop it anyway.

Exactly.

> Then, the question is whether we really need to
> 'IMPORT{db}="ID_FS_TYPE'?

I believe the question to ask rather "why should we not?". What harm do
you think could be done by importing ID_FS_TYPE from the db?

If we don't import ID_FS_TYPE, a later rule set could justly assume
that the fstype had changed, and in the worst case, might even take
some destructive action in response. AFAICS no rule set currently does,
but there's no guarantee for that. We should provide the follow-up
rules as much information as we can about the device, and if a device
is suspended, or the NOSCAN flag is set, there is no reason to assume
that the device has changed it fstype, or is about to change it.

The multipath rules have imported ID_FS_TYPE since 2014, and we've seen
no issues with it.

Regards
Martin

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Martin Wilck]

On Mon, 2024-03-04 at 12:27 +0100, Peter Rajnoha wrote:
> On 3/4/24 12:19, Peter Rajnoha wrote:
> > 
> > Now, I'm asking myself why we have the 'ACTION="add"' in the
> > systemd
> > rule at all. Why is it not just checking 
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG?
> > 
> 
> Oh! :)
> 
> https://github.com/systemd/systemd/commit/35a6750d9e26b26b423fbe815bead7d750210d8d
> 

This systemd rule is wrong, and I will to post a PR for it, but as
I said in the cover letter, I want to address these issues one by one.

If DM_UDEV_DISABLE_OTHER_RULES_FLAG=1, systemd should import
SYSTEMD_READY from the db rather than set it to 0.

Martin

Re: [RFC PATCH 1/7] 13-dm-disk.rules: import ID_FS_TYPE

[Peter Rajnoha]

On 3/4/24 16:17, Martin Wilck wrote:
> On Mon, 2024-03-04 at 11:37 +0100, Peter Rajnoha wrote:
>> On 3/1/24 23:40, Martin Wilck wrote:
>>> ID_FS_TYPE is the most important udev property for most follow-up
>>> rules. It must be imported from the udev db if blkid can't be run.
>>>
>>> Signed-off-by: Martin Wilck <mwilck@suse.com>
>>> ---
>>>  udev/13-dm-disk.rules.in | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/udev/13-dm-disk.rules.in b/udev/13-dm-disk.rules.in
>>> index dca00bc..eaad972 100644
>>> --- a/udev/13-dm-disk.rules.in
>>> +++ b/udev/13-dm-disk.rules.in
>>> @@ -26,6 +26,7 @@ ENV{DM_NOSCAN}=="1", GOTO="dm_watch"
>>>  GOTO="dm_link"
>>>  
>>>  LABEL="dm_import"
>>> +IMPORT{db}="ID_FS_TYPE"
>>>  IMPORT{db}="ID_FS_USAGE"
>>>  IMPORT{db}="ID_FS_UUID_ENC"
>>>  IMPORT{db}="ID_FS_LABEL_ENC"
>>
>> I think this one was left out deliberately. The original intention
>> was
>> to import only the minimal set of "blkid" variables needed to
>> properly
>> keep the symlinks based on these values in place (patch 94f77a4). And
>> the "ID_FS_TYPE" is not actually needed for this.
> 
> Why would we want to maintain the symlinks, but not other properties?
> ID_FS_TYPE is the blkid property with the highest number of consumers
> in a standard rule set. It is interpreted by most rule sets which build
> additional layers on top of dm devices.
> 
>> Though, importing even the "ID_FS_TYPE" together with other blkid
>> variables should not be an issue because this "import from previous
>> state" happens only if we have DM_SUSPENDED or DM_NOSCAN set for
>> current
>> event, in which case any other rules should be also disabled with
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".
>>
>> So even if some other rule fires based on ID_FS_TYPE value, the
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG should stop it anyway.
> 
> Exactly.
> 
>> Then, the question is whether we really need to
>> 'IMPORT{db}="ID_FS_TYPE'?
> 
> I believe the question to ask rather "why should we not?". What harm do
> you think could be done by importing ID_FS_TYPE from the db?
> 
> If we don't import ID_FS_TYPE, a later rule set could justly assume
> that the fstype had changed, and in the worst case, might even take
> some destructive action in response. AFAICS no rule set currently does,
> but there's no guarantee for that. We should provide the follow-up
> rules as much information as we can about the device, and if a device
> is suspended, or the NOSCAN flag is set, there is no reason to assume
> that the device has changed it fstype, or is about to change it.
> 

I agree.

The only issue I can identify here is that someone may fire its rule
based on the ID_FS_TYPE existence, but that would also mean they are
ignoring the DM_UDEV_DISABLE_OTHER_RULES_FLAG, which is much bigger
issue. I was just curious whether it makes sense in that case to import
it if nobody is reading it, but it's true some rule can still compare
previous and current state of ID_FS_TYPE and then act on a state change.
So yes, better to have that in.

-- 
Peter

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Martin Wilck]

On Mon, 2024-03-04 at 11:48 +0100, Peter Rajnoha wrote:
> 
> Yes, I'd like to get rid of this rule, but, unfortunately, there's
> one
> issue during the DM device creation/activation.
> 
> For example, if I try:
> 
>   dmsetup create --readonly --table "0 1 zero"
> 
> Then I get these uevents:
> 
> 1)
> ACTION=add
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
> SYSTEMD_READY=0
> 
> 
> 2)
> ACTION=change
> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=
> 
> 
> 3)
> ACTION=change
> DM_COOKIE=6335392
> DM_COOKIE_DISABLE_OTHER_RULES_FLAG=
> 
> 
> The uevent 3) coming with the DM_COOKIE is the actual event when the
> device is ready for use (that's the uevent notifying the DM device
> resume/activation).
> 
> If we remove the DISK_RO rule, then the
> DM_UDEV_DISABLE_OTHER_RULES_FLAG
> is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to
> get
> dropped, which in turn will start all the systemd hooks because the
> device is considered "ready" for systemd then.
> But the DM dev is ready only after uevent 3) that comes with the
> DM_COOKIE. So we still need to cover this scenario.

As event 2) doesn't have DM_COOKIE, I don't think we need to bother
about it much. IMO we should treat it like a synthetic "change" event,
which has almost no effect as far as dm is concerned.
Event 3) doesn't have DISK_RO=1 set. If any later rules are interested
in the state of write protection, they need to check the "ro" sysfs
attribute instead.

It would make some sense to be able tell later rules that they don't
need to bother with a given uevent because (from device mapper PoV)
nothing relevant has changed. I am not sure if we should use
DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 for this purpose. "Don't bother,
nothing has changed" is not the same thing as "don't bother, you can't
access this device right now", which to my understanding is the meaning
of DM_UDEV_DISABLE_OTHER_RULES_FLAG=1.

Actually we have DM_ACTIVATION=1 to tell other rules that they do need
to take action. Later rules only need to rescan a DM device if
DM_ACTIVATION=1; in all other cases they could just import properties
from the db. Currently kpartx and lvm are the only rules that check
DM_ACTIVATION.

Regards
Martin

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Martin Wilck]

On Mon, 2024-03-04 at 12:00 +0100, Peter Rajnoha wrote:
> 
> OK, makes sense. But I haven't looked at what implications this might
> have for 99-systemd.rules yet, but we surely need to have that
> covered
> somehow.
> 
> Maybe, now, I would probably even remove the mention about
> DM_SUSPENDED
> in 12-dm-permissions.rules, it looks superfluous there. We normally
> set
> perms based on names, not on DM_SUSPENDED state. I'm not sure why we
> mentioned it there before.
> 
> Do mpath rules still need to look at DM_SUSPENDED?

No, and yes :-)

If we remove the DISK_RO clause, DM_SUSPENDED and
DM_UDEV_DISABLE_OTHER_RULES_FLAG (as input from 10-dm.rules) are
equivalent for multipath. We'd be able to modify 11-dm-mpath.rules such
that DM_SUSPENDED isn't used any more. The downside is that 11-dm-
mpath.rules needs to modify DM_UDEV_DISABLE_OTHER_RULES_FLAG under
certain conditions, and that DM_SUSPENDED is shorter and expresses the
actual situation more intuitively. Therefore I don't love the idea to 
replace use of DM_SUSPENDED with "DUDORF" in 11-dm-mpath.rules.

My personal take on this is that 11-dm-mpath.rules actually belongs to
device-mapper (being executed before 13-dm-disk.rules), even though
it's not maintained in the lvm2 repository. As such, it should be
allowed to access dm-internal flags like DM_SUSPENDED. 
Not that's not a problem with this patch; the multipath rules can just
access .DM_SUSPENDED instead of DM_SUSPENDED.

Regards
Martin

Re: [RFC PATCH 7/7] 10-dm.rules: bump DM_UDEV_RULES_VSN to 3

[Martin Wilck]

On Mon, 2024-03-04 at 12:09 +0100, Peter Rajnoha wrote:
> On 3/1/24 23:40, Martin Wilck wrote:
> > Bump the rules version in order to indicate that upper level rules
> > should consume DM_UDEV_DISABLE_OTHER_RULES_FLAG rather than
> > DM_NOSCAN
> > and DM_SUSPENDED.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  udev/10-dm.rules.in | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
> > index d30f663..21bbcb0 100644
> > --- a/udev/10-dm.rules.in
> > +++ b/udev/10-dm.rules.in
> > @@ -136,7 +136,9 @@ LABEL="dm_suspended_set"
> >  # possible future changes.
> >  # VSN 1 - original rules
> >  # VSN 2 - add support for synthesized events
> > -ENV{DM_UDEV_RULES_VSN}="2"
> > +# VSN 3 - use DM_UDEV_DISABLE_OTHER_RULES_FLAG as the only "API"
> > +#         to be consumed by non-dm rules.
> > +ENV{DM_UDEV_RULES_VSN}="3"
> >  
> >  ENV{DM_UDEV_DISABLE_DM_RULES_FLAG}!="1", ENV{DM_NAME}=="?*",
> > SYMLINK+="(DM_DIR)/$env{DM_NAME}"
> >  
> 
> One thing that comes to my mind here is cooperation between the rules
> from initrd/initramfs and rootfs - the initrd/initramfs can have
> different versions of the rules installed.

Yes, that's a source of pain. Are there current initramfs tools that
user DM_UDEV_RULES_VSN!=2? I think "2" should be the standard today,
given that it has existed since 2009. dracut just installs the upstream
rules it finds, at least for dm, AFAICT.

I've reviewed other rule sets I'm aware of, and the only one in which I
needed to check DM_UDEV_RULES_VSN was 11-dm-mpath.rules. I didn't have
a close look at the rule sets that dracut ships yet, let alone other
tools for initramfs maintenance.

Regardless, my patch set changes the availability and semantics of the 
device-mapper udev properties, and thus we should bump the version, no?

Thanks,
Martin

Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Peter Rajnoha]

On 3/4/24 17:09, Martin Wilck wrote:
> On Mon, 2024-03-04 at 11:48 +0100, Peter Rajnoha wrote:
>>
>> Yes, I'd like to get rid of this rule, but, unfortunately, there's
>> one
>> issue during the DM device creation/activation.
>>
>> For example, if I try:
>>
>>   dmsetup create --readonly --table "0 1 zero"
>>
>> Then I get these uevents:
>>
>> 1)
>> ACTION=add
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> SYSTEMD_READY=0
>>
>>
>> 2)
>> ACTION=change
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> 3)
>> ACTION=change
>> DM_COOKIE=6335392
>> DM_COOKIE_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> The uevent 3) coming with the DM_COOKIE is the actual event when the
>> device is ready for use (that's the uevent notifying the DM device
>> resume/activation).
>>
>> If we remove the DISK_RO rule, then the
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG
>> is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to
>> get
>> dropped, which in turn will start all the systemd hooks because the
>> device is considered "ready" for systemd then.
>> But the DM dev is ready only after uevent 3) that comes with the
>> DM_COOKIE. So we still need to cover this scenario.
> 
> As event 2) doesn't have DM_COOKIE, I don't think we need to bother
> about it much. IMO we should treat it like a synthetic "change" event,
> which has almost no effect as far as dm is concerned.

Well, partly yes, but the important thing here is that the other rules
don not consider this as something they should react on. Here, it's like
the (genuine) "add" event for a DM device that has almost zero value to
others (we still need the table to get loaded and the device resumed for
it to be usable).

That's why the DISK_RO uevent was marked with DUDORF flag in the
original 10-dm.rules as anything before the activation mark is simply
considered spurious.

> Event 3) doesn't have DISK_RO=1 set. If any later rules are interested
> in the state of write protection, they need to check the "ro" sysfs
> attribute instead.

The unfortunate thing about the DISK_RO, unlike other events, is that it
is triggered *before* that DM activation (DM activation == table load +
resume). Also, frankly, I don't like this event at all - it's just
notifying about one of the many device attributes. A question then
arises: "Why don't we have uevents for changes in other attributes?",
but that's how it is for now and it's probably for a separate debate.

Till now, we've been marking that DISK_RO uevent as spurious completely.
But yes, we should have probably done that only in case it happens
before the activation, not if the device is already activated, if that's
the case too.

> 
> It would make some sense to be able tell later rules that they don't
> need to bother with a given uevent because (from device mapper PoV)
> nothing relevant has changed. I am not sure if we should use
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 for this purpose. "Don't bother,
> nothing has changed" is not the same thing as "don't bother, you can't
> access this device right now", which to my understanding is the meaning
> of DM_UDEV_DISABLE_OTHER_RULES_FLAG=1.

Actually, depending on the perspective, the DISK_RO uevent happening
before the activation is that kind of "you can't access this device
right now" (because it has not been activated fully yet).

> 
> Actually we have DM_ACTIVATION=1 to tell other rules that they do need
> to take action. Later rules only need to rescan a DM device if
> DM_ACTIVATION=1; in all other cases they could just import properties
> from the db. Currently kpartx and lvm are the only rules that check
> DM_ACTIVATION.

Yes, indeed, good point, the DM_ACTIVATION=1 is a little helper so the
rules which need to react *only on the event where the DM device has
just been activated/reactivated* with a new table.

It is set in two cases, IIRC:

  - on genuine "change" event where a new DM table is activated (a
resume after table load)

  - on synthetic "add" event *after* the device has already been
activated before (to cover the coldplug/udevadm trigger --action=add)

If there's a case where other rules do care about reacting only on this
particular event, then they should check DM_ACTIVATION, yes. But is
there such a case for other (non-dm, non-dm-subysystem) rules?

For the DISK_RO, it's about whether it comes before or after the
activation, not whether the DISK_RO is set with the activation at the
same time (which is not, it's a separate event). For this case, we don't
have anything to check right now - we just simply "disable" all the
events coming before the activation.

-- 
Peter

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Peter Rajnoha]

On 3/4/24 17:21, Martin Wilck wrote:
> On Mon, 2024-03-04 at 12:00 +0100, Peter Rajnoha wrote:
>>
>> OK, makes sense. But I haven't looked at what implications this might
>> have for 99-systemd.rules yet, but we surely need to have that
>> covered
>> somehow.
>>
>> Maybe, now, I would probably even remove the mention about
>> DM_SUSPENDED
>> in 12-dm-permissions.rules, it looks superfluous there. We normally
>> set
>> perms based on names, not on DM_SUSPENDED state. I'm not sure why we
>> mentioned it there before.
>>
>> Do mpath rules still need to look at DM_SUSPENDED?
> 
> No, and yes :-)
> 
> If we remove the DISK_RO clause, DM_SUSPENDED and
> DM_UDEV_DISABLE_OTHER_RULES_FLAG (as input from 10-dm.rules) are
> equivalent for multipath. We'd be able to modify 11-dm-mpath.rules such
> that DM_SUSPENDED isn't used any more. The downside is that 11-dm-
> mpath.rules needs to modify DM_UDEV_DISABLE_OTHER_RULES_FLAG under
> certain conditions, and that DM_SUSPENDED is shorter and expresses the
> actual situation more intuitively. Therefore I don't love the idea to 
> replace use of DM_SUSPENDED with "DUDORF" in 11-dm-mpath.rules.
> 
> My personal take on this is that 11-dm-mpath.rules actually belongs to
> device-mapper (being executed before 13-dm-disk.rules), even though
> it's not maintained in the lvm2 repository. As such, it should be
> allowed to access dm-internal flags like DM_SUSPENDED. 
> Not that's not a problem with this patch; the multipath rules can just
> access .DM_SUSPENDED instead of DM_SUSPENDED.

Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if needed.

We should just hide it from all the "other" rules so they don't need to
bother. For them (right now), it's either "usable" or "unusable" device
for whatever reason behind and we (DM+DM-subystem) should reimport
whatever is needed for the state/set of variables that others may use,
to stay sane. Of course, we can do this only for the state that we own.

As we discussed before, this can be extended to making a difference
among "usable", "temporarily unusable" (so reimport the state/variables
needed) and "completely unusable" state for others.

-- 
Peter

Re: [RFC PATCH 7/7] 10-dm.rules: bump DM_UDEV_RULES_VSN to 3

[Peter Rajnoha]

On 3/4/24 17:46, Martin Wilck wrote:
> On Mon, 2024-03-04 at 12:09 +0100, Peter Rajnoha wrote:
>> One thing that comes to my mind here is cooperation between the rules
>> from initrd/initramfs and rootfs - the initrd/initramfs can have
>> different versions of the rules installed.
> 
> Yes, that's a source of pain. Are there current initramfs tools that
> user DM_UDEV_RULES_VSN!=2? I think "2" should be the standard today,
> given that it has existed since 2009. dracut just installs the upstream
> rules it finds, at least for dm, AFAICT.
> 
> I've reviewed other rule sets I'm aware of, and the only one in which I
> needed to check DM_UDEV_RULES_VSN was 11-dm-mpath.rules. I didn't have
> a close look at the rule sets that dracut ships yet, let alone other
> tools for initramfs maintenance.
> 
> Regardless, my patch set changes the availability and semantics of the 
> device-mapper udev properties, and thus we should bump the version, no?

Sure, that is expected if we do such changes to rules.

I just meant to be cautious about a situation where we have initramfs
running a different version of rules, then keeping the udev database
over the pivot-to-rootfs (which happens with dracut, not sure about
others). Then, on coldplug running from rootfs, refreshing the state
with the other version of rules.

Important here is that the rules running from rootfs do not get mislead
with the state that was taken over from the initrams.

-- 
Peter

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Martin Wilck]

On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote:
> On 3/4/24 17:21, Martin Wilck wrote:
> > 
> > My personal take on this is that 11-dm-mpath.rules actually belongs
> > to
> > device-mapper (being executed before 13-dm-disk.rules), even though
> > it's not maintained in the lvm2 repository. As such, it should be
> > allowed to access dm-internal flags like DM_SUSPENDED. 
> > Not that's not a problem with this patch; the multipath rules can
> > just
> > access .DM_SUSPENDED instead of DM_SUSPENDED.
> 
> Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if
> needed.

I gather that you agree that 11-dm-mpath.rules represents a "DM
subsystem" rule set?

> 
> We should just hide it from all the "other" rules so they don't need
> to
> bother. For them (right now), it's either "usable" or "unusable"
> device
> for whatever reason behind and we (DM+DM-subystem) should reimport
> whatever is needed for the state/set of variables that others may
> use,
> to stay sane. Of course, we can do this only for the state that we
> own.
> 
> As we discussed before, this can be extended to making a difference
> among "usable", "temporarily unusable" (so reimport the
> state/variables
> needed) and "completely unusable" state for others.

Yeah, but that's future work, and I doubt that it makes sense to invest
much effort into it. I definitely wouldn't want to tie this to the
current patch set.

As mentioned previously, it might make sense to introduce a flag that
expresses something like "you can access this device, but you don't
need to" (DISK_RO={0,1} case, for example). But then, we already have
DM_ACTIVATION to express the opposite ("you must have a look at this
device, its properties have changed"). I wonder if you consider
DM_ACTIVATION a dm-internal property?

Thanks
Martin

Re: [RFC PATCH 7/7] 10-dm.rules: bump DM_UDEV_RULES_VSN to 3

[Martin Wilck]

On Tue, 2024-03-05 at 09:26 +0100, Peter Rajnoha wrote:
> On 3/4/24 17:46, Martin Wilck wrote:
> > On Mon, 2024-03-04 at 12:09 +0100, Peter Rajnoha wrote:
> > > One thing that comes to my mind here is cooperation between the
> > > rules
> > > from initrd/initramfs and rootfs - the initrd/initramfs can have
> > > different versions of the rules installed.
> > 
> > Yes, that's a source of pain. Are there current initramfs tools
> > that
> > user DM_UDEV_RULES_VSN!=2? I think "2" should be the standard
> > today,
> > given that it has existed since 2009. dracut just installs the
> > upstream
> > rules it finds, at least for dm, AFAICT.
> > 
> > I've reviewed other rule sets I'm aware of, and the only one in
> > which I
> > needed to check DM_UDEV_RULES_VSN was 11-dm-mpath.rules. I didn't
> > have
> > a close look at the rule sets that dracut ships yet, let alone
> > other
> > tools for initramfs maintenance.
> > 
> > Regardless, my patch set changes the availability and semantics of
> > the 
> > device-mapper udev properties, and thus we should bump the version,
> > no?
> 
> Sure, that is expected if we do such changes to rules.
> 
> I just meant to be cautious about a situation where we have initramfs
> running a different version of rules, then keeping the udev database
> over the pivot-to-rootfs (which happens with dracut, not sure about
> others). Then, on coldplug running from rootfs, refreshing the state
> with the other version of rules.
> 
> Important here is that the rules running from rootfs do not get
> mislead
> with the state that was taken over from the initrams.

For udev rules, that can't happen. If any udev rules were running after
switching root, they'd be running in the context of a new uevent, 
which means that the device-mapper rules from the root FS would already
be in place. We don't import DM_SUSPENDED from the db, so this property
wouldn't survive switching root, even if it had been set pre-pivot.

Other (non-udev) system components might be confused if they read
properties directly from udev data base using
udev_device_get_property_value(). But we can't do anything about this.
For multipathd, which is one of the prime suspects in this area, I can
confirm that it doesn't use libudev to access any device–mapper
internal properties.

In general, I can't conceive any danger arising from an older dm
ruleset (e.g. in the initramfs) that still sets DM_SUSPENDED or
DM_NOSCAN. In the worst case, some rule would be triggered that's also
triggered with today's rule set. That's not optimal, but it can hardly
be fatal.

FTR, summarizing the effect of my patch set for follow-up rules:

- it slightly changes the meaning of DM_UDEV_DISABLE_OTHER_RULES FLAG,
- it renames DM_SUSPENDED to .DM_SUSPENDED,
- it renames DM_NOSCAN to .DM_NOSCAN.

Technically, the renames just have the effect that these variables
aren't saved in the udev db. "Psychologically", the intention is that
people realize they are meant to be "dm internal", knowing that unless
we add more rules to unset these properties, we can't prevent later
rules from reading and using them.

With this patch set, later rules that depend on DM_SUSPENDED or
DM_NOSCAN won't trigger any more. The only rule outside of dm and
multipath that I am aware of is the broken rule in 99-systemd.rules.

Regards
Martin

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Peter Rajnoha]

On 3/5/24 09:47, Martin Wilck wrote:
> On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote:
>> Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if
>> needed.
> 
> I gather that you agree that 11-dm-mpath.rules represents a "DM
> subsystem" rule set?
> 

Sure, of course.

>>
>> We should just hide it from all the "other" rules so they don't need
>> to
>> bother. For them (right now), it's either "usable" or "unusable"
>> device
>> for whatever reason behind and we (DM+DM-subystem) should reimport
>> whatever is needed for the state/set of variables that others may
>> use,
>> to stay sane. Of course, we can do this only for the state that we
>> own.
>>
>> As we discussed before, this can be extended to making a difference
>> among "usable", "temporarily unusable" (so reimport the
>> state/variables
>> needed) and "completely unusable" state for others.
> 
> Yeah, but that's future work, and I doubt that it makes sense to invest
> much effort into it. I definitely wouldn't want to tie this to the
> current patch set.
> 

I agree.

> As mentioned previously, it might make sense to introduce a flag that
> expresses something like "you can access this device, but you don't
> need to" (DISK_RO={0,1} case, for example). But then, we already have
> DM_ACTIVATION to express the opposite ("you must have a look at this
> device, its properties have changed"). I wonder if you consider
> DM_ACTIVATION a dm-internal property?

Well, we added DM_ACTIVATION as a helper primarily for DM and DM-subsys
rules to have a way to identify when the actual (re)activation happens,
or the "add" trigger on coldplug.

I think it was 69-dm-lvm.rules (or 69-dm-lvm-metad.rules at that time)
where we needed to run pvscan only right after the DM dev is activated
and hence avoiding running costly pvscan uselessly where it doesn't matter.

If there's anyone else out there with similar use case, I think that
checking DM_ACTIVATION might be useful. But it's true it is not
advertised and shouted out somehow publicly yet.

Usually, all the other rules are interested in rescanning all the other
"foreign" state and attributes that is out of DM's hands, which means
they know exactly when to do the scan or not or any other actions, it
depends on what attributes are they watching for.

DM_ACTIVATION is very useful to know when stacking devices on top of DM
though, so a time when to activate the layer above. So yes, this
variable might be useful for other to look for too.

-- 
Peter

Re: [RFC PATCH 5/7] dm udev rules: don't export and save DM_SUSPENDED

[Martin Wilck]

On Tue, 2024-03-05 at 10:10 +0100, Peter Rajnoha wrote:
> On 3/5/24 09:47, Martin Wilck wrote:
> > On Tue, 2024-03-05 at 09:19 +0100, Peter Rajnoha wrote:
> > > Within DM and DM-subsystem rules, it's OK to use DM_SUSPENDED, if
> > > needed.
> > 
> > I gather that you agree that 11-dm-mpath.rules represents a "DM
> > subsystem" rule set?
> > 
> 
> Sure, of course.
> 
> > > 
> > > We should just hide it from all the "other" rules so they don't
> > > need
> > > to
> > > bother. For them (right now), it's either "usable" or "unusable"
> > > device
> > > for whatever reason behind and we (DM+DM-subystem) should
> > > reimport
> > > whatever is needed for the state/set of variables that others may
> > > use,
> > > to stay sane. Of course, we can do this only for the state that
> > > we
> > > own.
> > > 
> > > As we discussed before, this can be extended to making a
> > > difference
> > > among "usable", "temporarily unusable" (so reimport the
> > > state/variables
> > > needed) and "completely unusable" state for others.
> > 
> > Yeah, but that's future work, and I doubt that it makes sense to
> > invest
> > much effort into it. I definitely wouldn't want to tie this to the
> > current patch set.
> > 
> 
> I agree.
> 
> > As mentioned previously, it might make sense to introduce a flag
> > that
> > expresses something like "you can access this device, but you don't
> > need to" (DISK_RO={0,1} case, for example). But then, we already
> > have
> > DM_ACTIVATION to express the opposite ("you must have a look at
> > this
> > device, its properties have changed"). I wonder if you consider
> > DM_ACTIVATION a dm-internal property?
> 
> Well, we added DM_ACTIVATION as a helper primarily for DM and DM-
> subsys
> rules to have a way to identify when the actual (re)activation
> happens,
> or the "add" trigger on coldplug.
> 
> I think it was 69-dm-lvm.rules (or 69-dm-lvm-metad.rules at that
> time)
> where we needed to run pvscan only right after the DM dev is
> activated
> and hence avoiding running costly pvscan uselessly where it doesn't
> matter.
> 
> If there's anyone else out there with similar use case, I think that
> checking DM_ACTIVATION might be useful. But it's true it is not
> advertised and shouted out somehow publicly yet.
> 
> Usually, all the other rules are interested in rescanning all the
> other
> "foreign" state and attributes that is out of DM's hands, which means
> they know exactly when to do the scan or not or any other actions, it
> depends on what attributes are they watching for.
> 
> DM_ACTIVATION is very useful to know when stacking devices on top of
> DM
> though, so a time when to activate the layer above. So yes, this
> variable might be useful for other to look for too.


Thanks. I agree, but it's future work and it would mostly be a thing
for authors of stacked subsystems to look into.

I think are on the same page except perhaps for the DISK_RO part of the
set. I will give that some more thought, and submit a non-RFC patch
set.

Regards
Martin