[PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[mwilck]

From: Martin Wilck <mwilck@suse.com>

device-mapper sets the flag DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1 for
devices which are unusable. They may be no set up yet, suspended, or
otherwise unusable (e.g. multipath maps without usable path). This
flag does not necessarily imply SYSTEMD_READY=0 and must therefore
be tested separately.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 udev-md-raid-assembly.rules | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/udev-md-raid-assembly.rules b/udev-md-raid-assembly.rules
index d668cdd..4568b01 100644
--- a/udev-md-raid-assembly.rules
+++ b/udev-md-raid-assembly.rules
@@ -21,6 +21,11 @@ IMPORT{cmdline}="noiswmd"
 IMPORT{cmdline}="nodmraid"
 
 ENV{nodmraid}=="?*", GOTO="md_inc_end"
+
+# device mapper sets DM_UDEV_DISABLE_OTHER_RULES_FLAG for devices which
+# aren't ready to use
+KERNEL=="dm-*", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", GOTO="md_inc_end"
+
 ENV{ID_FS_TYPE}=="ddf_raid_member", GOTO="md_inc"
 ENV{noiswmd}=="?*", GOTO="md_inc_end"
 ENV{ID_FS_TYPE}=="isw_raid_member", ACTION!="change", GOTO="md_inc"
-- 
2.35.1

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[NeilBrown]

On Thu, 17 Feb 2022, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> device-mapper sets the flag DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1 for
> devices which are unusable. They may be no set up yet, suspended, or
> otherwise unusable (e.g. multipath maps without usable path). This
> flag does not necessarily imply SYSTEMD_READY=0 and must therefore
> be tested separately.

I really don't like this - looks like a hack.  A Kludge.

Can you provide a reference to a detailed discussion that explains why
SYSTEMD_READY=0 cannot be used?

Thanks,
NeilBrown


> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  udev-md-raid-assembly.rules | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/udev-md-raid-assembly.rules b/udev-md-raid-assembly.rules
> index d668cdd..4568b01 100644
> --- a/udev-md-raid-assembly.rules
> +++ b/udev-md-raid-assembly.rules
> @@ -21,6 +21,11 @@ IMPORT{cmdline}="noiswmd"
>  IMPORT{cmdline}="nodmraid"
>  
>  ENV{nodmraid}=="?*", GOTO="md_inc_end"
> +
> +# device mapper sets DM_UDEV_DISABLE_OTHER_RULES_FLAG for devices which
> +# aren't ready to use
> +KERNEL=="dm-*", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", GOTO="md_inc_end"
> +
>  ENV{ID_FS_TYPE}=="ddf_raid_member", GOTO="md_inc"
>  ENV{noiswmd}=="?*", GOTO="md_inc_end"
>  ENV{ID_FS_TYPE}=="isw_raid_member", ACTION!="change", GOTO="md_inc"
> -- 
> 2.35.1
> 
> 

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

On Thu, 2022-02-17 at 09:09 +1100, NeilBrown wrote:
> On Thu, 17 Feb 2022, mwilck@suse.com wrote:
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > device-mapper sets the flag DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1
> > for
> > devices which are unusable. They may be no set up yet, suspended,
> > or
> > otherwise unusable (e.g. multipath maps without usable path). This
> > flag does not necessarily imply SYSTEMD_READY=0 and must therefore
> > be tested separately.
> 
> I really don't like this - looks like a hack.  A Kludge.

These are strong words. You didn't go into detail, so I'm assuming that
your reasoning is that DM_UDEV_DISABLE_OTHER_RULES_FLAG is an internal
flag of the device-mapper subsystem. Still, you can see that is's used
both internally by dm, and by other subsystems:

https://github.com/lvmteam/lvm2/blob/8dccc2314e2482370bc6e5cf007eb210994abdef/udev/13-dm-disk.rules.in#L15
https://github.com/g2p/bcache-tools/blob/a73679b22c333763597d39c72112ef5a53f55419/69-bcache.rules#L6
https://github.com/opensvc/multipath-tools/blob/d9d7ae9e2125116b465b4ff4d98ce65fe0eac3cc/kpartx/kpartx.rules#L10

Would you call these others "hacks", too?

> Can you provide a reference to a detailed discussion that explains
> why
> SYSTEMD_READY=0 cannot be used?

The main reason is that SYSTEMD_READY=0 is set too late, in 99-systemd-
rules, and only on "add" events:
https://github.com/systemd/systemd/blob/bfae960e53f6986f1c4d234ea82681d0acad57df/rules.d/99-systemd.rules.in#L18

I guess the device-mapper rules themselves could be setting
SYSTEMD_READY="0". @Peter Rajnoha, do you want to comment on that? My
concern wrt such a change would be possible side effects. Setting
SYSTEMD_READY=0 on "change" events could actually be wrong, see below.

I the case I was observing, there was a multipath device without valid
paths, which had switched to queueing mode [*]. If this happens for
whatever reason (and it could be something else, like a suspended DM
device), IO on such a device hangs. IO that may hang must not be
attempted from an udev rule. Therefore it makes sense that layers
stacked on top of DM try to avoid it, and checking udev properties set
by DM is a reasonable way to do that.

The core of the problem is that there is no well-defined "API"
specifying how different udev rule sets can communicate, iow which udev
properties are well-defined enough to be consumed outside of the
subsystem that defines them. SYSTEMD_READY is about the only "global"
property. IMO it's somewhat overloaded: The actual semantics of
SYSTEMD_READY=0 is "systemd shouldn't activate the associated device
unit". Various udev rule sets use it with similar but not 100%
identical semantics like "don't touch this" or "don't probe this". 

In the case I was looking at, the device had already been activated by
systemd. Later, the device had lost all active paths and thus became
unusable. We can't easily set SYSTEMD_READY=0 on such a device. Doing
so would actually be dangerous, because systemd might remove the
device. Moreover, while processing the udev rule, we just don't know if
the problem is temporary or permanent. 

Other properties, like those set by the DM subsystem, are less well-
defined. There's no official spec defining their names and semantics,
and there are multiple flags which aren't easly differentiated
(DM_UDEV_DISABLE_DISK_RULES_FLAG, DM_UDEV_DISABLE_OTHER_RULES_FLAG,
DM_NOSCAN, DM_SUSPENDED, MPATH_DEVICE_READY). OTOH, most of these flags
have been around for many years without changing, and thus have
acquired the status of a semi-official API, which is actually used in
other rule sets. In particular DM_UDEV_DISABLE_OTHER_RULES_FLAG has a
few users, see above. I believe this is for good reason, and therefore
I don't consider my patch a "hack".

Regards
Martin

[*] How that came to pass is subtle by itself, and admittedly not fully
understood.

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Peter Rajnoha]

On Thu 17 Feb 2022 11:58, Martin Wilck wrote:
> On Thu, 2022-02-17 at 09:09 +1100, NeilBrown wrote:
> > On Thu, 17 Feb 2022, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > device-mapper sets the flag DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1
> > > for
> > > devices which are unusable. They may be no set up yet, suspended,
> > > or
> > > otherwise unusable (e.g. multipath maps without usable path). This
> > > flag does not necessarily imply SYSTEMD_READY=0 and must therefore
> > > be tested separately.
> > 
> > I really don't like this - looks like a hack.  A Kludge.
> 
> These are strong words. You didn't go into detail, so I'm assuming that
> your reasoning is that DM_UDEV_DISABLE_OTHER_RULES_FLAG is an internal
> flag of the device-mapper subsystem. Still, you can see that is's used
> both internally by dm, and by other subsystems:
> 
> https://github.com/lvmteam/lvm2/blob/8dccc2314e2482370bc6e5cf007eb210994abdef/udev/13-dm-disk.rules.in#L15
> https://github.com/g2p/bcache-tools/blob/a73679b22c333763597d39c72112ef5a53f55419/69-bcache.rules#L6
> https://github.com/opensvc/multipath-tools/blob/d9d7ae9e2125116b465b4ff4d98ce65fe0eac3cc/kpartx/kpartx.rules#L10
> 

The flags that DM use for udev were introduced before systemd project
even existed. We needed to introduce the DM_UDEV_DISABLE_OTHER_RULES_FLAG
to have a possibility for all the "other" (non-dm) udev rules to check
for if there's another subsystem stacking its own devices on top of DM ones.

The flag is used to communicate the other rules a condition when a DM
device underneath is either not yet set up completely or it's not ready
to be read/scanned for a reason (e.g. the device is suspended, not yet
loaded with a table...).

The reason we needed to introduce such a flag is simple - there's
limited amount of event types and DM devices are not ready on the usual
ADD event. It's after the CHANGE event that originates from the DM
device having a table loaded and resumed. At the same time, a CHANGE
event can be generated for various different reasons. So checking a
single flag that we set based in out own udev rules based on our best
knowledge and let other other rules to check for this single flag
seemed to be the best option to solve this.

> Would you call these others "hacks", too?
> 
> > Can you provide a reference to a detailed discussion that explains
> > why
> > SYSTEMD_READY=0 cannot be used?
> 
> The main reason is that SYSTEMD_READY=0 is set too late, in 99-systemd-
> rules, and only on "add" events:
> https://github.com/systemd/systemd/blob/bfae960e53f6986f1c4d234ea82681d0acad57df/rules.d/99-systemd.rules.in#L18
> 
> I guess the device-mapper rules themselves could be setting
> SYSTEMD_READY="0". @Peter Rajnoha, do you want to comment on that? My
> concern wrt such a change would be possible side effects. Setting
> SYSTEMD_READY=0 on "change" events could actually be wrong, see below.
> 

First of all, as already mentioned, DM udev rules with all the flags
pre-date the systemd project itself.

When systemd was introduced, we communicated the flag use with systemd
right away and so this line was added to 99-systemd.rules:

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

At that early time, the SYSTEMD_READY flag was used solely for systemd
purpose of setting its own device units properly. Just later, other subsystems
started (mis)using this flag for notifying about device readiness and so
the very original intention of the SYSTEMD_READY flag has diverted this
way a little bit.

Last but not the least, systemd is just one of the init systems/service
managers around so it's not any standard for block devices to set the
SYSTEMD_READY flag to notify about device readiness. Yes, it's true
that systemd is widespread now, but still not a single standard...

> I the case I was observing, there was a multipath device without valid
> paths, which had switched to queueing mode [*]. If this happens for
> whatever reason (and it could be something else, like a suspended DM
> device), IO on such a device hangs. IO that may hang must not be
> attempted from an udev rule. Therefore it makes sense that layers
> stacked on top of DM try to avoid it, and checking udev properties set
> by DM is a reasonable way to do that.
> 
> The core of the problem is that there is no well-defined "API"
> specifying how different udev rule sets can communicate, iow which udev
> properties are well-defined enough to be consumed outside of the
> subsystem that defines them. SYSTEMD_READY is about the only "global"
> property. IMO it's somewhat overloaded: The actual semantics of
> SYSTEMD_READY=0 is "systemd shouldn't activate the associated device
> unit". Various udev rule sets use it with similar but not 100%
> identical semantics like "don't touch this" or "don't probe this". 
> 

Exactly!

The SID - Storage Instantiation Daemon, which is still in development,
is trying to cover exactly this part, among other things.

> In the case I was looking at, the device had already been activated by
> systemd. Later, the device had lost all active paths and thus became
> unusable. We can't easily set SYSTEMD_READY=0 on such a device. Doing
> so would actually be dangerous, because systemd might remove the
> device. Moreover, while processing the udev rule, we just don't know if
> the problem is temporary or permanent. 
> 
> Other properties, like those set by the DM subsystem, are less well-
> defined. There's no official spec defining their names and semantics,
> and there are multiple flags which aren't easly differentiated
> (DM_UDEV_DISABLE_DISK_RULES_FLAG, DM_UDEV_DISABLE_OTHER_RULES_FLAG,
> DM_NOSCAN, DM_SUSPENDED, MPATH_DEVICE_READY). OTOH, most of these flags
> have been around for many years without changing, and thus have
> acquired the status of a semi-official API, which is actually used in
> other rule sets. In particular DM_UDEV_DISABLE_OTHER_RULES_FLAG has a
> few users, see above. I believe this is for good reason, and therefore
> I don't consider my patch a "hack".
> 

Maybe we (DM) should have documented this better, more clearly, but the
DM_UDEV_DISABLE_OTHER_RULES_FLAG is really designed to be checked by
"other" foreign subsystems to notify them whether they can process their
udev rules on such a DM device.

Full documentation for the generic DM udev flags is here:

https://sourceware.org/git/?p=lvm2.git;a=blob;f=libdm/libdevmapper.h;=e9412da7d33fc7534cd1eccd88c21b75c6c221b1;hb=HEAD#l3644

In summary, the meaning of the flags:

  DM_UDEV_DISABLE_DISK_RULES_FLAG is controlling 13-dm-disk.rules (where
blkid is called for DM devices and /dev/disk/by-* symlinks are set)

  DM_UDEV_DISABLE_DM_RULES_FLAG is controlling 10-dm.rules

  DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG is controlling DM subsystem (LVM,
multipath, crypt, ...)

  DM_NOSCAN is just a helper DM-internal flag in udev to help inside
DM's own rules and/or its subsystem rules

  DM_SUSPENDED is something that is set and can be checked, but foreign
(non-DM) udev rules don't need to bother about this at all. DM udev
rules already set DM_UDEV_DISABLE_OTHER_RULES_FLAG to notify other rules
if the DM device becomes unreadable.

  DM_NAME, DM_UUID - normally, other rules don't need to bother about
DM name or UUID - they're set mainly to hook custom permission rules on
(for which DM has a template 12-dm-permissions.rules).

So the only flag a non-DM rule should be concerned about is exactly the
single DM_UDEV_DISABLE_OTHER_RULES_FLAG. That's its exact purpose it
was designed for within DM block devices and uevent processing.

Definitely not a hack!

(I'm just a bit surprised that we haven't sent a patch to MD yet.
Wasn't there a check for this flag anytime before? I thought all
major block subsystems have already been covered.)

-- 
Peter

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[NeilBrown]

> The flags that DM use for udev were introduced before systemd project
> even existed. We needed to introduce the DM_UDEV_DISABLE_OTHER_RULES_FLAG
> to have a possibility for all the "other" (non-dm) udev rules to check
> for if there's another subsystem stacking its own devices on top of DM
> ones.

If this is an established API that DM uses, then presumably it is
documented somewhere.  If a link to that documentation were provided, it
would look a a whole lot less like a hack.

Thanks,
NeilBrown

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

Neil,

On Tue, 2022-02-22 at 10:36 +1100, NeilBrown wrote:
> 
> > The flags that DM use for udev were introduced before systemd
> > project
> > even existed. We needed to introduce the
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG
> > to have a possibility for all the "other" (non-dm) udev rules to
> > check
> > for if there's another subsystem stacking its own devices on top of
> > DM
> > ones.
> 
> If this is an established API that DM uses, then presumably it is
> documented somewhere.  If a link to that documentation were provided,
> it
> would look a a whole lot less like a hack.

Peter has provided a link to libdevmapper.h in his previous post in
this thread. Is this a request for me to include that link in the patch
description?

Regards,
Martin

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[NeilBrown]

On Wed, 23 Feb 2022, Martin Wilck wrote:
> Neil,
> 
> On Tue, 2022-02-22 at 10:36 +1100, NeilBrown wrote:
> > 
> > > The flags that DM use for udev were introduced before systemd
> > > project
> > > even existed. We needed to introduce the
> > > DM_UDEV_DISABLE_OTHER_RULES_FLAG
> > > to have a possibility for all the "other" (non-dm) udev rules to
> > > check
> > > for if there's another subsystem stacking its own devices on top of
> > > DM
> > > ones.
> > 
> > If this is an established API that DM uses, then presumably it is
> > documented somewhere.  If a link to that documentation were provided,
> > it
> > would look a a whole lot less like a hack.
> 
> Peter has provided a link to libdevmapper.h in his previous post in
> this thread. Is this a request for me to include that link in the patch
> description?

If libdevmapper.h is the best documentation there is for this variable,
then hopefully you can see why it feels to an outsider like a hack.

It isn't even immediately clear that the text there is talking about
environment variables visible in udev.
If there is an expectation that tools outside of lvm2 will use these,
then they really should be documented properly.  SYSTEMD_READY is
documented in "man systemd.device".  How hard would it be to write a
"dm-udev" man page which explains all this.

But if libdevmapper.h is the best you have, then maybe it'll have to do.
It is up to Jes what he accepts of course.

Thanks,
NeilBrown

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

Hello Neil,

On Wed, 2022-02-23 at 09:49 +1100, NeilBrown wrote:
> > 
> > Peter has provided a link to libdevmapper.h in his previous post in
> > this thread. Is this a request for me to include that link in the
> > patch
> > description?
> 
> If libdevmapper.h is the best documentation there is for this
> variable,
> then hopefully you can see why it feels to an outsider like a hack.
> 

There's also some documentation in the form of comments in the device-
mapper rules files themselves:

https://github.com/lvmteam/lvm2/blob/8dccc2314e2482370bc6e5cf007eb210994abdef/udev/10-dm.rules.in#L137

In general, 10-dm.rules is one of the most extensively commented rule
files. I've always found these comments quite helpful.

> It isn't even immediately clear that the text there is talking about
> environment variables visible in udev.
> If there is an expectation that tools outside of lvm2 will use these,
> then they really should be documented properly.  SYSTEMD_READY is
> documented in "man systemd.device".  How hard would it be to write a
> "dm-udev" man page which explains all this.

I agree that the documentation could be improved. OTOH, the text in
systemd.device(5) only explains how systemd itself interprets
SYSTEMD_READY. It doesn't say a word about how other udev rules are
supposed to deal with the device. IOW, SYSTEMD_READY is part of an API
between udev rules and systemd, and not between different udev rule
sets.

In particular the "don't probe this iff SYSTEMD_READY==0" semantics
that are frequently used in rules files can't be inferred from this
documentation. It makes sense most of the time, but there are some
cases where it doesn't. Here, we have a case where probing should be
skipped, even though SYSTEMD_READY is not 0.

Regards,
Martin

PS: The big issue remains that there is no "official" API in which udev
rule sets can transport information between each other. I guess the
original idea was that every rule set would be self-contained, which
isn't the case any more. If anyone makes an effort redesign udev, they
should consider creating such an API somehow.

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

On Wed, 2022-02-23 at 09:49 +1100, NeilBrown wrote:
> 
> But if libdevmapper.h is the best you have, then maybe it'll have to
> do.
> It is up to Jes what he accepts of course.

Jes, have you made up your mind on this patch yet?
Please contact me if you have remarks or questions.

Note that the patch that started this thread is broken; the v2 is
correct. With the v2, I was able to reach 1000 consecutive reboots on
a system with MD on top of multipath that would otherwise hang during
boot about once every 50-100 reboots.

Regards
Martin

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Martin Wilck]

Jes,

On Mon, 2022-02-28 at 09:48 +0100, Martin Wilck wrote:
> On Wed, 2022-02-23 at 09:49 +1100, NeilBrown wrote:
> > 
> > But if libdevmapper.h is the best you have, then maybe it'll have
> > to
> > do.
> > It is up to Jes what he accepts of course.
> 
> Jes, have you made up your mind on this patch yet?
> Please contact me if you have remarks or questions.

Gentle reminder ...

Regards
Martin

Re: [dm-devel] [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Xiao Ni]

On Thu, Feb 17, 2022 at 9:10 PM Peter Rajnoha <prajnoha@redhat.com> wrote:
>
> On Thu 17 Feb 2022 11:58, Martin Wilck wrote:
> > On Thu, 2022-02-17 at 09:09 +1100, NeilBrown wrote:
> > > On Thu, 17 Feb 2022, mwilck@suse.com wrote:
> > > > From: Martin Wilck <mwilck@suse.com>
> > > >
> > > > device-mapper sets the flag DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1
> > > > for
> > > > devices which are unusable. They may be no set up yet, suspended,
> > > > or
> > > > otherwise unusable (e.g. multipath maps without usable path). This
> > > > flag does not necessarily imply SYSTEMD_READY=0 and must therefore
> > > > be tested separately.
> > >
> > > I really don't like this - looks like a hack.  A Kludge.
> >
> > These are strong words. You didn't go into detail, so I'm assuming that
> > your reasoning is that DM_UDEV_DISABLE_OTHER_RULES_FLAG is an internal
> > flag of the device-mapper subsystem. Still, you can see that is's used
> > both internally by dm, and by other subsystems:
> >
> > https://github.com/lvmteam/lvm2/blob/8dccc2314e2482370bc6e5cf007eb210994abdef/udev/13-dm-disk.rules.in#L15
> > https://github.com/g2p/bcache-tools/blob/a73679b22c333763597d39c72112ef5a53f55419/69-bcache.rules#L6
> > https://github.com/opensvc/multipath-tools/blob/d9d7ae9e2125116b465b4ff4d98ce65fe0eac3cc/kpartx/kpartx.rules#L10
> >
>
> The flags that DM use for udev were introduced before systemd project
> even existed. We needed to introduce the DM_UDEV_DISABLE_OTHER_RULES_FLAG
> to have a possibility for all the "other" (non-dm) udev rules to check
> for if there's another subsystem stacking its own devices on top of DM ones.
>
> The flag is used to communicate the other rules a condition when a DM
> device underneath is either not yet set up completely or it's not ready
> to be read/scanned for a reason (e.g. the device is suspended, not yet
> loaded with a table...).
>
> The reason we needed to introduce such a flag is simple - there's
> limited amount of event types and DM devices are not ready on the usual
> ADD event. It's after the CHANGE event that originates from the DM
> device having a table loaded and resumed. At the same time, a CHANGE
> event can be generated for various different reasons. So checking a
> single flag that we set based in out own udev rules based on our best
> knowledge and let other other rules to check for this single flag
> seemed to be the best option to solve this.
>
> > Would you call these others "hacks", too?
> >
> > > Can you provide a reference to a detailed discussion that explains
> > > why
> > > SYSTEMD_READY=0 cannot be used?
> >
> > The main reason is that SYSTEMD_READY=0 is set too late, in 99-systemd-
> > rules, and only on "add" events:
> > https://github.com/systemd/systemd/blob/bfae960e53f6986f1c4d234ea82681d0acad57df/rules.d/99-systemd.rules.in#L18
> >
> > I guess the device-mapper rules themselves could be setting
> > SYSTEMD_READY="0". @Peter Rajnoha, do you want to comment on that? My
> > concern wrt such a change would be possible side effects. Setting
> > SYSTEMD_READY=0 on "change" events could actually be wrong, see below.
> >
>
> First of all, as already mentioned, DM udev rules with all the flags
> pre-date the systemd project itself.
>
> When systemd was introduced, we communicated the flag use with systemd
> right away and so this line was added to 99-systemd.rules:
>
>   SUBSYSTEM=="block", ACTION=="add", ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", ENV{SYSTEMD_READY}="0"
>
> At that early time, the SYSTEMD_READY flag was used solely for systemd
> purpose of setting its own device units properly. Just later, other subsystems
> started (mis)using this flag for notifying about device readiness and so
> the very original intention of the SYSTEMD_READY flag has diverted this
> way a little bit.
>
> Last but not the least, systemd is just one of the init systems/service
> managers around so it's not any standard for block devices to set the
> SYSTEMD_READY flag to notify about device readiness. Yes, it's true
> that systemd is widespread now, but still not a single standard...
>
> > I the case I was observing, there was a multipath device without valid
> > paths, which had switched to queueing mode [*]. If this happens for
> > whatever reason (and it could be something else, like a suspended DM
> > device), IO on such a device hangs. IO that may hang must not be
> > attempted from an udev rule. Therefore it makes sense that layers
> > stacked on top of DM try to avoid it, and checking udev properties set
> > by DM is a reasonable way to do that.
> >
> > The core of the problem is that there is no well-defined "API"
> > specifying how different udev rule sets can communicate, iow which udev
> > properties are well-defined enough to be consumed outside of the
> > subsystem that defines them. SYSTEMD_READY is about the only "global"
> > property. IMO it's somewhat overloaded: The actual semantics of
> > SYSTEMD_READY=0 is "systemd shouldn't activate the associated device
> > unit". Various udev rule sets use it with similar but not 100%
> > identical semantics like "don't touch this" or "don't probe this".
> >
>
> Exactly!
>
> The SID - Storage Instantiation Daemon, which is still in development,
> is trying to cover exactly this part, among other things.
>
> > In the case I was looking at, the device had already been activated by
> > systemd. Later, the device had lost all active paths and thus became
> > unusable. We can't easily set SYSTEMD_READY=0 on such a device. Doing
> > so would actually be dangerous, because systemd might remove the
> > device. Moreover, while processing the udev rule, we just don't know if
> > the problem is temporary or permanent.
> >
> > Other properties, like those set by the DM subsystem, are less well-
> > defined. There's no official spec defining their names and semantics,
> > and there are multiple flags which aren't easly differentiated
> > (DM_UDEV_DISABLE_DISK_RULES_FLAG, DM_UDEV_DISABLE_OTHER_RULES_FLAG,
> > DM_NOSCAN, DM_SUSPENDED, MPATH_DEVICE_READY). OTOH, most of these flags
> > have been around for many years without changing, and thus have
> > acquired the status of a semi-official API, which is actually used in
> > other rule sets. In particular DM_UDEV_DISABLE_OTHER_RULES_FLAG has a
> > few users, see above. I believe this is for good reason, and therefore
> > I don't consider my patch a "hack".
> >
>
> Maybe we (DM) should have documented this better, more clearly, but the
> DM_UDEV_DISABLE_OTHER_RULES_FLAG is really designed to be checked by
> "other" foreign subsystems to notify them whether they can process their
> udev rules on such a DM device.
>
> Full documentation for the generic DM udev flags is here:
>
> https://sourceware.org/git/?p=lvm2.git;a=blob;f=libdm/libdevmapper.h;=e9412da7d33fc7534cd1eccd88c21b75c6c221b1;hb=HEAD#l3644
>
> In summary, the meaning of the flags:
>
>   DM_UDEV_DISABLE_DISK_RULES_FLAG is controlling 13-dm-disk.rules (where
> blkid is called for DM devices and /dev/disk/by-* symlinks are set)
>
>   DM_UDEV_DISABLE_DM_RULES_FLAG is controlling 10-dm.rules
>
>   DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG is controlling DM subsystem (LVM,
> multipath, crypt, ...)
>
>   DM_NOSCAN is just a helper DM-internal flag in udev to help inside
> DM's own rules and/or its subsystem rules
>
>   DM_SUSPENDED is something that is set and can be checked, but foreign
> (non-DM) udev rules don't need to bother about this at all. DM udev
> rules already set DM_UDEV_DISABLE_OTHER_RULES_FLAG to notify other rules
> if the DM device becomes unreadable.
>
>   DM_NAME, DM_UUID - normally, other rules don't need to bother about
> DM name or UUID - they're set mainly to hook custom permission rules on
> (for which DM has a template 12-dm-permissions.rules).
>
> So the only flag a non-DM rule should be concerned about is exactly the
> single DM_UDEV_DISABLE_OTHER_RULES_FLAG. That's its exact purpose it
> was designed for within DM block devices and uevent processing.
>
> Definitely not a hack!
>
> (I'm just a bit surprised that we haven't sent a patch to MD yet.
> Wasn't there a check for this flag anytime before? I thought all
> major block subsystems have already been covered.)
>

Hi Peter

In rhel, we have a rhel only udev rule that checks
DM_UDEV_DISABLE_OTHER_RULES_FLAG. Maybe it's the reason why you don't
notice this. Besides DM_UDEV_DISABLE_OTHER_RULES_FLAG, it still checks
other flags.

# Next make sure that this isn't a dm device we should skip for some reason
ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="dm_change_end"
ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", GOTO="dm_change_end"
ENV{DM_SUSPENDED}=="1", GOTO="dm_change_end"
KERNEL=="dm-*", SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="linux_raid_member", \
        ACTION=="change", RUN+="/sbin/mdadm -I $env{DEVNAME}"
LABEL="dm_change_end"

In 10-dm.rules, if DM_SUSPENDED is 1, it sets
DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1. So we don't need the check of
DM_SUSPENDED. But how DM_UDEV_RULES_VSN? Do we need to check it?

Best Regards
Xiao

Re: [dm-devel] [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Peter Rajnoha]

Hi!

On Mon 28 Feb 2022 23:28, Xiao Ni wrote:
> Hi Peter
> 
> In rhel, we have a rhel only udev rule that checks
> DM_UDEV_DISABLE_OTHER_RULES_FLAG. Maybe it's the reason why you don't
> notice this. Besides DM_UDEV_DISABLE_OTHER_RULES_FLAG, it still checks
> other flags.
> 

Ah yes, that's it! I've been still recalling this to be patched once.
So looks like it just didn't get propagated upstream :-/

> # Next make sure that this isn't a dm device we should skip for some reason
> ENV{DM_UDEV_RULES_VSN}!="?*", GOTO="dm_change_end"
> ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", GOTO="dm_change_end"
> ENV{DM_SUSPENDED}=="1", GOTO="dm_change_end"
> KERNEL=="dm-*", SUBSYSTEM=="block", ENV{ID_FS_TYPE}=="linux_raid_member", \
>         ACTION=="change", RUN+="/sbin/mdadm -I $env{DEVNAME}"
> LABEL="dm_change_end"
> 
> In 10-dm.rules, if DM_SUSPENDED is 1, it sets
> DM_UDEV_DISABLE_OTHER_RULES_FLAG to 1. So we don't need the check of
> DM_SUSPENDED. But how DM_UDEV_RULES_VSN? Do we need to check it?
> 

Yes, right, the check for DM_SUSPENDED is superfluous here so we don't
actually need that one. The single check for DM_UDEV_DISABLE_OTHER_RULES_FLAG
covers it.

The DM_UDEV_RULES_VSN - this was meant for future changes in case a new
set of DM udev variables is used or existing set changed so the other rules
know what exact variable set is available for checking. But I think the rules
are settled down for a few years now and I don't expect any more radical
changes here, so we can remove that check for DM_UDEV_RULES_VSN as well.

-- 
Peter

Re: [PATCH] udev-md-raid-assembly.rules: skip if DM_UDEV_DISABLE_OTHER_RULES_FLAG

[Peter Rajnoha]

On Thu 17 Feb 2022 11:58, Martin Wilck wrote:
> The main reason is that SYSTEMD_READY=0 is set too late, in 99-systemd-
> rules, and only on "add" events:
> https://github.com/systemd/systemd/blob/bfae960e53f6986f1c4d234ea82681d0acad57df/rules.d/99-systemd.rules.in#L18
> 
> I guess the device-mapper rules themselves could be setting
> SYSTEMD_READY="0". @Peter Rajnoha, do you want to comment on that? My
> concern wrt such a change would be possible side effects. Setting
> SYSTEMD_READY=0 on "change" events could actually be wrong, see below.

We need to be very careful with SYSTEMD_READY as switching that from 1
to 0 would cause the systemd device unit to stop. And services can be
bound to device existence (that is, systemd device unit). If we just
temporarily suspend a DM device and not completely removing it, then
we might get out-of-sync easily when it comes to notion of device
presence in various parts of the system.

One thing is device presence, the other are various sub-states that
a single SYSTEMD_READY is not covering, like the suspended state...

-- 
Peter