About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Martin Wilck]

Hi Zdenek, Peter, David, Ben,

I have been pondering the device-mapper udev rules a lot lately, and I
believe I have found glitches in the logic of the device mapper udev
flags that I'd like to bring to your attention.

TL;DR: change I propose:

* use DM_DISABLE_OTHER_RULES_FLAG consistently as a flag meaning
  "upper layers should leave this device alone until told otherwise",
saved between uevents in the udev db
* use DM_SUSPENDED consistently as a flag meaning "upper layers
  should keep their hands off this device temporarily", not saved
  in the udev db.
* don't use any other flags in upper layers, including 13-dm-
disk.rules.

This implies:

* stop setting DM_DISABLE_OTHER_RULES_FLAG from DM_SUSPENDED
  in 10-dm.rules
* check DM_DISABLE_OTHER_RULES_FLAG instead of DM_NOSCAN in
  13-dm-disk.rules
* check DM_SUSPENDED in 69-dm-lvm.rules, 80-udisks2.rules
* stop using DM_NOSCAN in 11-dm-mpath.rules and 66-kpartx.rules

Full story, I apologize for the lengthy post:

# DM_DISABLE_OTHER_RULES_FLAG 

This flag serves multiple purposes:

 1) it is set by LVM itself directly for certain types of logical
volumes, such as thin pools, and passed through to the udev rules using
DM_COOKIE. 
 2) it is set by 10-dm.rules if the device in question is found
suspended (DM_SUSPENDED=1), or if DISK_RO is set.
 3) For spurious events, 10-dm.rules restores it from the udev db.
 4) it set by 11-lvm.rules if LVM's internal "noscan" was set
(DM_SUBSYSTEM_UDEV_FLAG0). This applies only for actual LVs, not
generic DM devices. On the first subsequent event that has
DM_SUBSYSTEM_UDEV_FLAG0 cleared (usually right the next event),
DM_DISABLE_OTHER_RULES_FLAG is restored from
DM_UDEV_DISABLE_OTHER_RULES_FLAG_OLD.
 5) It's used by the 11-dm-mpath rules in a similar way. 
MPATH_DEVICE_READY=0 implies DM_DISABLE_OTHER_RULES_FLAG=1, but not
vice-versa. My latest multipath patch series changes the treatment
of the DM_DISABLE_OTHER_RULES_FLAG by not saving the override in the db
any more.
 6) It is consumed by later rules as sort a generic "don't touch this
device" flag. 

IMO the fact that the same flag is used for various different
conditions is problematic, in particular because the value of the
flag is saved in the udev db between uevents. The flag can only be
cleared if a genuine libdm event arrives that has this flag cleared,
while the device is not suspended (and DISK_RO is not set, which is
never the case for genuine libdm events, AFAICT).

Saving the flag is probably correct for cases in which the flag has
been set via the cookie (I'm not LVM expert enough to tell with
certainty). But saving it if it has been set from DM_SUSPENDED seems
wrong, because DM_SUSPENDED is tested anew for every new uevent, but
DM_UDEV_DISABLE_OTHER_RULES_FLAG is not cleared if the device is not
found suspended. Therefore I think the two cases 1) and 2) above must
be differentiated.

DISK_RO is yet different; I have to say I am unsure under what
circumstances this flag is set at all. It doesn't seem to
be set for a read-only LVM LV, for example. The kernel sets it only
when the disk's "ro" attribute is _toggled_, and no udev rule imports
it from the db, causing it to be temporary at best.

# DM_NOSCAN

This flag is also used in different ways.

 - In 11-dm-lvm.rules, it doesn't seem to be meant for later rules
   to consume. Rather, it is used to remember the fact that the
   previous uevent had DM_SUBSYSTEM_UDEV_FLAG0 (aka LVM "noscan")
   flag set. It's only important from DB if DM_SUBSYSTEM_UDEV_FLAG0
   is clear, and then cleared immediately. In later rules, the
   semantics of DM_NOSCAN is "DM_SUBSYSTEM_UDEV_FLAG0 was set in
   this event", and not "this device is not accessible, don't 
   attempt IO". 11-dm-lvm.rules sets DM_DISABLE_OTHER_RULES_FLAG
   to indicate non-accessibility.
 - The modification of DM_DISABLE_OTHER_RULES_FLAG seems wrong if
   DM_DISABLE_OTHER_RULES_FLAG is set from DM_SUSPENDED, but if we
   drop that behavior as described above, I think we can leave at
   that.
 - But that would mean that later rules should actually evaluate
   DM_DISABLE_OTHER_RULES_FLAG and not DM_NOSCAN, unless they 
   want the precise semantics described above (which doesn't make
   much sense for layers further up the stack IMO)
 - 11-dm-mpath.rules sets different semantics for DM_NOSCAN. 
   It basically sets DM_NOSCAN=1 equivalent to MPATH_DEVICE_READY=0;
   moreover, MPATH_DEVICE_READY=0 implies DM_DISABLE_OTHER_RULES_FLAG=1
 - The consumers of DM_NOSCAN assume "don't attempt IO" semantics.
   
# Usage on upper layers

The only condition that is relevant to layers above LVM/multipath is 
whether they are allowed to attempt I/O on the device and read meta
data to derive further properties, create symlinks, activate device
etc. We currently have 3 properties with vaguely these semantics:

 - DM_DISABLE_OTHER_RULES_FLAG (consumed in 66-kpartx.rules,
   69-dm-lvm.rules, 80-udisks2.rules, 99-systemd.rules)
 - DM_NOSCAN (consumed in 13-dm-disk.rules, 66-kpartx.rules)
 - DM_SUSPENDED (13-dm-disk.rules, 66-kpartx.rules, 99-systemd.rules).

These properties differ in the way they are remembered between uevents,
but to upper layers they have a similar meaning. However, they are not
equivalent.

AFAICS, DM_NOSCAN=1 implies DM_DISABLE_OTHER_RULES_FLAG=1.
I can't think of a situation where it would be beneficial to attempt IO
with DM_DISABLE_OTHER_RULES_FLAG=1 and DM_NOSCAN!=1.
So basically upper layer should simply ignore DM_NOSCAN. That also
means that 11-dm-mpath.rules doesn't need to touch DM_NOSCAN any more.

DM_SUSPENDED is different, because it's temporary condition that is re-
evaluated on every uevent. Also, it may have different
implications; see e.g. 99-systemd.rules.

Upper layers should use the condition DM_DISABLE_OTHER_RULES_FLAG=1 ||
DM_SUSPENDED=1 to check if IO is impossible on a device.
That means that we should not set DM_DISABLE_OTHER_RULES_FLAG=1 from
DM_SUSPENDED=1; we should treat these two as independent conditions.

I'd find it somewhat cleaner if we didn't mess with
DM_DISABLE_OTHER_RULES_FLAG in 11-dm-lvm.rules and 11-dm-mpath.rules at
all. This way the flag could preserve exactly the meaning it had been
given by the sender of the last cookie. But that would mean adding new
properties and making appropriate changes in upper layers, which we
don't control. I am not sure if we want to do that. I observe that in
the lvm2 C code, DM_DISABLE_OTHER_RULES_FLAG is equivalent to
DM_DISABLE_DISK_RULES_FLAG, so the latter flag may give us the cookie
value, if we agree to keep this behavior in the future.

Regards
Martin

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Peter Rajnoha]

On 2/9/24 22:58, Martin Wilck wrote:
> Hi Zdenek, Peter, David, Ben,
> 
> I have been pondering the device-mapper udev rules a lot lately, and I
> believe I have found glitches in the logic of the device mapper udev
> flags that I'd like to bring to your attention.
> 
> TL;DR: change I propose:
> 
> * use DM_DISABLE_OTHER_RULES_FLAG consistently as a flag meaning
>   "upper layers should leave this device alone until told otherwise",
> saved between uevents in the udev db
> * use DM_SUSPENDED consistently as a flag meaning "upper layers
>   should keep their hands off this device temporarily", not saved
>   in the udev db.
> * don't use any other flags in upper layers, including 13-dm-
> disk.rules.
> 
> This implies:
> 
> * stop setting DM_DISABLE_OTHER_RULES_FLAG from DM_SUSPENDED
>   in 10-dm.rules
> * check DM_DISABLE_OTHER_RULES_FLAG instead of DM_NOSCAN in
>   13-dm-disk.rules
> * check DM_SUSPENDED in 69-dm-lvm.rules, 80-udisks2.rules
> * stop using DM_NOSCAN in 11-dm-mpath.rules and 66-kpartx.rules

Hi Martin!

It is surely possible we could do some improvements and optimizations
here - all the rules are a result of long evolution where we were fixing
various issues on the way. So if we could shoot down some complexities,
that would be great...

On the other hand, we need to be really careful, because even a tiny
change here can cause troubles if we omit some case.

I'll surely go through your suggestions, thanks for diving deep into
that! Just please give me some time so I'll remap all the paths through
the rules in my head :)

Note that we're working with 3 levels of logical rule separation here:

  1) DM base (10-dm.rules, 13-dm-disk.rules, 95-dm-notify.rules)
  2) DM subsystem (11-dm-lvm.rules, the mpath rules, ...)
  3) all the "other"

As for the very original intentions of the
DM_UDEV_DISABLE_OTHER_RULES_FLAG - this one was supposed to be the *only
one* to check for in "other" rules so they don't need to bother about
checking other states/variables ("other" here means other than DM and
any DM subsystem). One simple variable to check for others - an ideal -
otherwise, we would be having a hard time to persuade others why they
need to add complex checks in their rules/applications.

Yes, there's the unlucky exception in the 99-systemd.rules which needs
to check the DM_SUSPENDED to import the "SYSTEMD_READY" from udev db,
otherwise we had been running into an issue with stopping systemd device
units prematurely (actually, this was an issue spotted much much later
on after years of using the rules without this rule). And we haven't
found a better way to check for this condition. This is mainly because
systemd is also a "device" manager/tracker in some way through its
"device" units, besides udev itself. I simply gave up there and admitted
the check for DM_SUSPENDED case.

The DM_NOSCAN was actually just a helper and a more human readable name
for "DM_SUSBYSTEM_UDEV_FLAG0" within LVM subsystem *only* at first.
It is used during LV initialization - the wiping/zeroing of the LV
before it is pronounced as usable - that's why, when it is set, we
signal to "others" the DM_UDEV_DISABLE_OTHER_RULES_FLAG based on this
flag. However, since we have the 13-dm-disk.rules which manages the
blkid call for DM devices (and which is ours - owned by DM), we need to
signal these rules to avoid calling blkid (as it could see
uninitiliazed/stale data). In this case, we import any previous scan
results from udev db. It's not like "completely ignore and skip rules
for this event". Other DM subsystems may get into exactly the same
situation with initialization as LVM, so that's why it ended up as
generic "DM_NOSCAN" so any 11-dm-<subsystem>.rules can use it to signal
13-dm-disk.rules to avoid the blkid scan this way (the "SCAN" here
actually means "blkid scan" so the better name would be
"DM_NO_BLKID_SCAN" probably). The "DM_NOSCAN" is not meant to be
consumed by "others" at all.

The other way round, we also don't want (ideally) to check for
DM_UDEV_DISABLE_OTHER_RULES flag in DM and DM subysystem rules. This
flag is meant for "others" to check. So we have a clear separation of
what is signalled withing "DM world" and "the other world".

The DM_SUSPENDED flag - well, this was supposed to be just internal to
DM and DM subystem rules. All the "others" have the
DM_UDEV_DISABLE_OTHER_RULES_FLAG - they simply don't need to bother for
what exact reason the device is not usable, whether it is a suspended
case or whatever else...

The DM udev rules are designed in a way that all the DM_UDEV_DISABLE_*
can still be changed in DM/DM-subsystem rules based on further findings
and processing of the state. So the original flags from the DM_COOKIE is
just a starting point here.

-- 
Peter

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Martin Wilck]

On Mon, 2024-02-12 at 10:51 +0100, Peter Rajnoha wrote:
> On 2/9/24 22:58, Martin Wilck wrote:
> > Hi Zdenek, Peter, David, Ben,
> > 
> > I have been pondering the device-mapper udev rules a lot lately,
> > and I
> > believe I have found glitches in the logic of the device mapper
> > udev
> > flags that I'd like to bring to your attention.
> > 
> > TL;DR: change I propose:
> > 
> > * use DM_DISABLE_OTHER_RULES_FLAG consistently as a flag meaning
> >   "upper layers should leave this device alone until told
> > otherwise",
> > saved between uevents in the udev db
> > * use DM_SUSPENDED consistently as a flag meaning "upper layers
> >   should keep their hands off this device temporarily", not saved
> >   in the udev db.
> > * don't use any other flags in upper layers, including 13-dm-
> > disk.rules.
> > 
> > This implies:
> > 
> > * stop setting DM_DISABLE_OTHER_RULES_FLAG from DM_SUSPENDED
> >   in 10-dm.rules
> > * check DM_DISABLE_OTHER_RULES_FLAG instead of DM_NOSCAN in
> >   13-dm-disk.rules
> > * check DM_SUSPENDED in 69-dm-lvm.rules, 80-udisks2.rules
> > * stop using DM_NOSCAN in 11-dm-mpath.rules and 66-kpartx.rules
> 
> Hi Martin!
> 
> It is surely possible we could do some improvements and optimizations
> here - all the rules are a result of long evolution where we were
> fixing
> various issues on the way. So if we could shoot down some
> complexities,
> that would be great...
> 
> On the other hand, we need to be really careful, because even a tiny
> change here can cause troubles if we omit some case.
> 
> I'll surely go through your suggestions, thanks for diving deep into
> that! Just please give me some time so I'll remap all the paths
> through
> the rules in my head :)
> 
> Note that we're working with 3 levels of logical rule separation
> here:
> 
>   1) DM base (10-dm.rules, 13-dm-disk.rules, 95-dm-notify.rules)
>   2) DM subsystem (11-dm-lvm.rules, the mpath rules, ...)
>   3) all the "other"
> 
> As for the very original intentions of the
> DM_UDEV_DISABLE_OTHER_RULES_FLAG - this one was supposed to be the
> *only
> one* to check for in "other" rules so they don't need to bother about
> checking other states/variables ("other" here means other than DM and
> any DM subsystem). One simple variable to check for others - an ideal
> -
> otherwise, we would be having a hard time to persuade others why they
> need to add complex checks in their rules/applications.

Yes, I understand that, and it makes a lot of sense. 

The problem arises by saving and restoring to/from the udev db. We've
got different possible *reasons* (inputs) for a device becoming
unusable. The value we communicate to upper layers should arguably be a
"logical or" of all these. But then we can't use a single variable to
save and restore the state; we must determine the value of all "reason"
flags (either directly or by restoring from the db, as appropriate for
the flag in question) and "or"-them into a single "dontuse" flag.

Example: Currently, we set DM_UDEV_DISABLE_OTHER_RULES_FLAG if
DM_SUSPENDED is set. If the next spurious uevent arrives, we see
DM_UDEV_DISABLE_OTHER_RULES_FLAG set, but we don't know if the origin
was DM_SUSPENDED or a genuine udevflag. In the former case, the device
would now be usable, while in the latter case it most probably
wouldn't. For now, I've posted a patch to fix this for the multipath
rules [1], but I'd like to see it fixed in the general DM rules.

> Yes, there's the unlucky exception in the 99-systemd.rules which
> needs
> to check the DM_SUSPENDED to import the "SYSTEMD_READY" from udev db,
> otherwise we had been running into an issue with stopping systemd
> device
> units prematurely (actually, this was an issue spotted much much
> later
> on after years of using the rules without this rule). 
> And we haven't
> found a better way to check for this condition. This is mainly
> because
> systemd is also a "device" manager/tracker in some way through its
> "device" units, besides udev itself. I simply gave up there and
> admitted
> the check for DM_SUSPENDED case.

Well, IIUC the main reason that systemd couldn't use
DM_UDEV_DISABLE_OTHER_RULES_FLAG was at least in part due to the fact
that the multipath rules used it in a special way that was inconsistent
with the rest of DM ;-)

I think there are 3 variants of "unusable":

a) temporarily unusable (just for this event), ignore this uevent and
restore previous properties from db.
b) unusable, avoid IO, don't scan, don't activate (this is how we use
DM_UDEV_DISABLE_OTHER_RULES_FLAG). Upper layers will usually load saved
properties from udev db in this case, too.
c) like b), but also try to unmount / unconfigure if already used. This
is SYSTEMD_READY=0. I don't think DM has a flag with these semantics at
this time. I can imagine such a flag being set if a device was reloaded
with an incompatible table, but that's rather a corner case.

It's an honorable goal to condense everything into a single variable
for consumer rules, but I think it doesn't work if we want the upper
layers to be able to distinguish these. We can merge a) and b) I think,
because their meaning for upper layers is practically the same, if we
get the saving and restoring right.

> The DM_NOSCAN was actually just a helper and a more human readable
> name
> for "DM_SUSBYSTEM_UDEV_FLAG0" within LVM subsystem *only* at first.
> It is used during LV initialization - the wiping/zeroing of the LV
> before it is pronounced as usable - that's why, when it is set, we
> signal to "others" the DM_UDEV_DISABLE_OTHER_RULES_FLAG based on this
> flag. However, since we have the 13-dm-disk.rules which manages the
> blkid call for DM devices (and which is ours - owned by DM), we need
> to
> signal these rules to avoid calling blkid (as it could see
> uninitiliazed/stale data). In this case, we import any previous scan
> results from udev db. It's not like "completely ignore and skip rules
> for this event".

I'm not sure if I understand what you mean with "completely ignore and
skip". Upper-level rules usually can't "completely ignore" an uevent if
they need to preserve any udev properties across the current event. If
the device is unusable in the weaker "don't try IO" sense, the upper
rules must import existing properties from the db, just like 13-dm-
disk.rules, lest the properties be forgotten by udev. IMO this weaker
semantics (corresponding to b) above) is what matters most for upper
level rules.

>  Other DM subsystems may get into exactly the same
> situation with initialization as LVM, so that's why it ended up as
> generic "DM_NOSCAN" so any 11-dm-<subsystem>.rules can use it to
> signal
> 13-dm-disk.rules to avoid the blkid scan this way (the "SCAN" here
> actually means "blkid scan" so the better name would be
> "DM_NO_BLKID_SCAN" probably). The "DM_NOSCAN" is not meant to be
> consumed by "others" at all.

If blkid is likely to fail or retrieve stale data, other tools that
read content from the device are likely to fail, too, and that's why
they currently consume this variable. I agree that they shouldn't, but
that works only if DM_UDEV_DISABLE_OTHER_RULES_FLAG has "don't do IO"
semantics, as opposed to "completely ignore and skip".

> The other way round, we also don't want (ideally) to check for
> DM_UDEV_DISABLE_OTHER_RULES flag in DM and DM subysystem rules. This
> flag is meant for "others" to check. So we have a clear separation of
> what is signalled withing "DM world" and "the other world".
> 
> The DM_SUSPENDED flag - well, this was supposed to be just internal
> to
> DM and DM subystem rules. All the "others" have the
> DM_UDEV_DISABLE_OTHER_RULES_FLAG - they simply don't need to bother
> for
> what exact reason the device is not usable, whether it is a suspended
> case or whatever else...

I understand. I agree that non-dm rules shouldn't use DM_SUSPENDED and
DM_NOSCAN. But this requires clean handling of save/restore, and that's
what we currently don't do right, IMO.

Thanks,
Martin

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Peter Rajnoha]

On 2/12/24 12:09, Martin Wilck wrote:
> On Mon, 2024-02-12 at 10:51 +0100, Peter Rajnoha wrote:
>> On 2/9/24 22:58, Martin Wilck wrote:
>>> Hi Zdenek, Peter, David, Ben,
>>>
>>> I have been pondering the device-mapper udev rules a lot lately,
>>> and I
>>> believe I have found glitches in the logic of the device mapper
>>> udev
>>> flags that I'd like to bring to your attention.
>>>
>>> TL;DR: change I propose:
>>>
>>> * use DM_DISABLE_OTHER_RULES_FLAG consistently as a flag meaning
>>>   "upper layers should leave this device alone until told
>>> otherwise",
>>> saved between uevents in the udev db
>>> * use DM_SUSPENDED consistently as a flag meaning "upper layers
>>>   should keep their hands off this device temporarily", not saved
>>>   in the udev db.
>>> * don't use any other flags in upper layers, including 13-dm-
>>> disk.rules.
>>>
>>> This implies:
>>>
>>> * stop setting DM_DISABLE_OTHER_RULES_FLAG from DM_SUSPENDED
>>>   in 10-dm.rules
>>> * check DM_DISABLE_OTHER_RULES_FLAG instead of DM_NOSCAN in
>>>   13-dm-disk.rules
>>> * check DM_SUSPENDED in 69-dm-lvm.rules, 80-udisks2.rules
>>> * stop using DM_NOSCAN in 11-dm-mpath.rules and 66-kpartx.rules
>>
>> Hi Martin!
>>
>> It is surely possible we could do some improvements and optimizations
>> here - all the rules are a result of long evolution where we were
>> fixing
>> various issues on the way. So if we could shoot down some
>> complexities,
>> that would be great...
>>
>> On the other hand, we need to be really careful, because even a tiny
>> change here can cause troubles if we omit some case.
>>
>> I'll surely go through your suggestions, thanks for diving deep into
>> that! Just please give me some time so I'll remap all the paths
>> through
>> the rules in my head :)
>>
>> Note that we're working with 3 levels of logical rule separation
>> here:
>>
>>   1) DM base (10-dm.rules, 13-dm-disk.rules, 95-dm-notify.rules)
>>   2) DM subsystem (11-dm-lvm.rules, the mpath rules, ...)
>>   3) all the "other"
>>
>> As for the very original intentions of the
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG - this one was supposed to be the
>> *only
>> one* to check for in "other" rules so they don't need to bother about
>> checking other states/variables ("other" here means other than DM and
>> any DM subsystem). One simple variable to check for others - an ideal
>> -
>> otherwise, we would be having a hard time to persuade others why they
>> need to add complex checks in their rules/applications.
> 
> Yes, I understand that, and it makes a lot of sense. 
> 
> The problem arises by saving and restoring to/from the udev db. We've
> got different possible *reasons* (inputs) for a device becoming
> unusable. The value we communicate to upper layers should arguably be a
> "logical or" of all these. But then we can't use a single variable to
> save and restore the state; we must determine the value of all "reason"
> flags (either directly or by restoring from the db, as appropriate for
> the flag in question) and "or"-them into a single "dontuse" flag.
> 
> Example: Currently, we set DM_UDEV_DISABLE_OTHER_RULES_FLAG if
> DM_SUSPENDED is set. If the next spurious uevent arrives, we see
> DM_UDEV_DISABLE_OTHER_RULES_FLAG set, but we don't know if the origin
> was DM_SUSPENDED or a genuine udevflag. In the former case, the device
> would now be usable, while in the latter case it most probably
> wouldn't. For now, I've posted a patch to fix this for the multipath
> rules [1], but I'd like to see it fixed in the general DM rules.
> 

Right, underneath within DM/DM-subystem, we should be able to keep and
restore those reasons for why it has been flagged with
DM_UDEV_DISABLE_OTHER_RULES_FLAG till now and when the state is good
enough that we can drop it, we would do it transparently for higher
(non-dm and non-dm-subsystem) layers. So if there's a case that is not
currently handled by 10-dm.rules or 11-dm-<subsystem>.rules, we can fix
that there. If it's a generic rule that applies to all DM, not just
subystem, then yes, we can move that to 10-dm.rules (will have a look at
your patch [1]).

>> Yes, there's the unlucky exception in the 99-systemd.rules which
>> needs
>> to check the DM_SUSPENDED to import the "SYSTEMD_READY" from udev db,
>> otherwise we had been running into an issue with stopping systemd
>> device
>> units prematurely (actually, this was an issue spotted much much
>> later
>> on after years of using the rules without this rule). 
>> And we haven't
>> found a better way to check for this condition. This is mainly
>> because
>> systemd is also a "device" manager/tracker in some way through its
>> "device" units, besides udev itself. I simply gave up there and
>> admitted
>> the check for DM_SUSPENDED case.
> 
> Well, IIUC the main reason that systemd couldn't use
> DM_UDEV_DISABLE_OTHER_RULES_FLAG was at least in part due to the fact
> that the multipath rules used it in a special way that was inconsistent
> with the rest of DM ;-)


Aha! Well, honestly, I don't remember the exact details and context of
that fix, but I know we haven't found a better way...

> 
> I think there are 3 variants of "unusable":
> 
> a) temporarily unusable (just for this event), ignore this uevent and
> restore previous properties from db.
> b) unusable, avoid IO, don't scan, don't activate (this is how we use
> DM_UDEV_DISABLE_OTHER_RULES_FLAG). Upper layers will usually load saved
> properties from udev db in this case, too.
> c) like b), but also try to unmount / unconfigure if already used. This
> is SYSTEMD_READY=0. I don't think DM has a flag with these semantics at
> this time. I can imagine such a flag being set if a device was reloaded
> with an incompatible table, but that's rather a corner case.
> 
> It's an honorable goal to condense everything into a single variable
> for consumer rules, but I think it doesn't work if we want the upper
> layers to be able to distinguish these. We can merge a) and b) I think,
> because their meaning for upper layers is practically the same, if we
> get the saving and restoring right.
> 

The c) case - well, it's questionable what should be done in that case,
because that means we have literally cut off the underlying device while
it was still in use. Any further IO from higher layers will return IO
errors, will queue IOs or, in the worse case, issue IOs to a device that
we don't want to anymore.

Anyway, if I understand correctly, we simply need to signal higher
layers either:

  A) device is unusable, forget it and clear all your current extra
records you have about this device (including removing any custom
symlinks for udev). That would also map to SYSTEMD_READY=0.

  B) device is unusable temporarily, restore any records you need, wait
for the DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 to drop to 0 (or being unset).

What do you think about keeping a single
DM_UDEV_DISABLE_OTHER_RULES_FLAG for this, just having a different
value, say "2" to denote the B case? Otherwise, we need 2 distinct
variables (which is harder for others to accept I bet).

>> The DM_NOSCAN was actually just a helper and a more human readable
>> name
>> for "DM_SUSBYSTEM_UDEV_FLAG0" within LVM subsystem *only* at first.
>> It is used during LV initialization - the wiping/zeroing of the LV
>> before it is pronounced as usable - that's why, when it is set, we
>> signal to "others" the DM_UDEV_DISABLE_OTHER_RULES_FLAG based on this
>> flag. However, since we have the 13-dm-disk.rules which manages the
>> blkid call for DM devices (and which is ours - owned by DM), we need
>> to
>> signal these rules to avoid calling blkid (as it could see
>> uninitiliazed/stale data). In this case, we import any previous scan
>> results from udev db. It's not like "completely ignore and skip rules
>> for this event".
> 
> I'm not sure if I understand what you mean with "completely ignore and
> skip". Upper-level rules usually can't "completely ignore" an uevent if
> they need to preserve any udev properties across the current event. If
> the device is unusable in the weaker "don't try IO" sense, the upper
> rules must import existing properties from the db, just like 13-dm-
> disk.rules, lest the properties be forgotten by udev. IMO this weaker
> semantics (corresponding to b) above) is what matters most for upper
> level rules.

I didn't consider that there might be extra records to keep around for
anything else than blkid (13-dm-disk.rules) and DM/DM subsys (10-dm and
11-dm-subsystem.rules). But you're probably right here - differentiating
the A) and B) from above could be beneficial for some layers above. If
nothing else, then at least the systemd's SYSTEMD_READY case in
99-systemd.rules.

If we could still keep the single DM_UDEV_DISABLE_OTHER_RULES_FLAG for
others to check, I'd really like to have only that one, not adding more.

> 
>>  Other DM subsystems may get into exactly the same
>> situation with initialization as LVM, so that's why it ended up as
>> generic "DM_NOSCAN" so any 11-dm-<subsystem>.rules can use it to
>> signal
>> 13-dm-disk.rules to avoid the blkid scan this way (the "SCAN" here
>> actually means "blkid scan" so the better name would be
>> "DM_NO_BLKID_SCAN" probably). The "DM_NOSCAN" is not meant to be
>> consumed by "others" at all.
> 
> If blkid is likely to fail or retrieve stale data, other tools that
> read content from the device are likely to fail, too, and that's why
> they currently consume this variable. I agree that they shouldn't, but
> that works only if DM_UDEV_DISABLE_OTHER_RULES_FLAG has "don't do IO"
> semantics, as opposed to "completely ignore and skip".
> 
>> The other way round, we also don't want (ideally) to check for
>> DM_UDEV_DISABLE_OTHER_RULES flag in DM and DM subysystem rules. This
>> flag is meant for "others" to check. So we have a clear separation of
>> what is signalled withing "DM world" and "the other world".
>>
>> The DM_SUSPENDED flag - well, this was supposed to be just internal
>> to
>> DM and DM subystem rules. All the "others" have the
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG - they simply don't need to bother
>> for
>> what exact reason the device is not usable, whether it is a suspended
>> case or whatever else...
> 
> I understand. I agree that non-dm rules shouldn't use DM_SUSPENDED and
> DM_NOSCAN. But this requires clean handling of save/restore, and that's
> what we currently don't do right, IMO.
> 
> Thanks,
> Martin
> 

-- 
Peter

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Martin Wilck]

On Mon, 2024-02-12 at 13:32 +0100, Peter Rajnoha wrote:
> On 2/12/24 12:09, Martin Wilck wrote:
> 
> 
> Right, underneath within DM/DM-subystem, we should be able to keep
> and
> restore those reasons for why it has been flagged with
> DM_UDEV_DISABLE_OTHER_RULES_FLAG till now and when the state is good
> enough that we can drop it, we would do it transparently for higher
> (non-dm and non-dm-subsystem) layers. So if there's a case that is
> not
> currently handled by 10-dm.rules or 11-dm-<subsystem>.rules, we can
> fix
> that there. If it's a generic rule that applies to all DM, not just
> subystem, then yes, we can move that to 10-dm.rules (will have a look
> at
> your patch [1]).

I don't think that patch can be used as-is for DM. For multipath, I'm
not aware of any situation where DM_UDEV_DISABLE_OTHER_RULES_FLAG would
be set in the udev cookie, therefore DM_UDEV_DISABLE_OTHER_RULES_FLAG
is essentially an alias for DM_SUSPENDED before 11-dm-mpath.rules
changes it. That's not the case for other targets, in particular LVM.

> > 
> > Well, IIUC the main reason that systemd couldn't use
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG was at least in part due to the
> > fact
> > that the multipath rules used it in a special way that was
> > inconsistent
> > with the rest of DM ;-)
> 
> 
> Aha! Well, honestly, I don't remember the exact details and context
> of
> that fix, but I know we haven't found a better way...
> 
> > 
> > I think there are 3 variants of "unusable":
> > 
> > a) temporarily unusable (just for this event), ignore this uevent
> > and
> > restore previous properties from db.
> > b) unusable, avoid IO, don't scan, don't activate (this is how we
> > use
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG). Upper layers will usually load
> > saved
> > properties from udev db in this case, too.
> > c) like b), but also try to unmount / unconfigure if already used.
> > This
> > is SYSTEMD_READY=0. I don't think DM has a flag with these
> > semantics at
> > this time. I can imagine such a flag being set if a device was
> > reloaded
> > with an incompatible table, but that's rather a corner case.
> > 
> > It's an honorable goal to condense everything into a single
> > variable
> > for consumer rules, but I think it doesn't work if we want the
> > upper
> > layers to be able to distinguish these. We can merge a) and b) I
> > think,
> > because their meaning for upper layers is practically the same, if
> > we
> > get the saving and restoring right.
> > 
> 
> The c) case - well, it's questionable what should be done in that
> case,
> because that means we have literally cut off the underlying device
> while
> it was still in use. Any further IO from higher layers will return IO
> errors, will queue IOs or, in the worse case, issue IOs to a device
> that
> we don't want to anymore.
> 
> Anyway, if I understand correctly, we simply need to signal higher
> layers either:
> 
>   A) device is unusable, forget it and clear all your current extra
> records you have about this device (including removing any custom
> symlinks for udev). That would also map to SYSTEMD_READY=0.
> 
>   B) device is unusable temporarily, restore any records you need,
> wait
> for the DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 to drop to 0 (or being
> unset).
> 
> What do you think about keeping a single
> DM_UDEV_DISABLE_OTHER_RULES_FLAG for this, just having a different
> value, say "2" to denote the B case? Otherwise, we need 2 distinct
> variables (which is harder for others to accept I bet).

Yes, that could work, if the save / restore is implemented cleanly.


> 
> > > The DM_NOSCAN was actually just a helper and a more human
> > > readable
> > > name
> > > for "DM_SUSBYSTEM_UDEV_FLAG0" within LVM subsystem *only* at
> > > first.
> > > It is used during LV initialization - the wiping/zeroing of the
> > > LV
> > > before it is pronounced as usable - that's why, when it is set,
> > > we
> > > signal to "others" the DM_UDEV_DISABLE_OTHER_RULES_FLAG based on
> > > this
> > > flag. However, since we have the 13-dm-disk.rules which manages
> > > the
> > > blkid call for DM devices (and which is ours - owned by DM), we
> > > need
> > > to
> > > signal these rules to avoid calling blkid (as it could see
> > > uninitiliazed/stale data). In this case, we import any previous
> > > scan
> > > results from udev db. It's not like "completely ignore and skip
> > > rules
> > > for this event".
> > 
> > I'm not sure if I understand what you mean with "completely ignore
> > and
> > skip". Upper-level rules usually can't "completely ignore" an
> > uevent if
> > they need to preserve any udev properties across the current event.
> > If
> > the device is unusable in the weaker "don't try IO" sense, the
> > upper
> > rules must import existing properties from the db, just like 13-dm-
> > disk.rules, lest the properties be forgotten by udev. IMO this
> > weaker
> > semantics (corresponding to b) above) is what matters most for
> > upper
> > level rules.
> 
> I didn't consider that there might be extra records to keep around
> for
> anything else than blkid (13-dm-disk.rules) and DM/DM subsys (10-dm
> and
> 11-dm-subsystem.rules). But you're probably right here -
> differentiating
> the A) and B) from above could be beneficial for some layers above.
> If
> nothing else, then at least the systemd's SYSTEMD_READY case in
> 99-systemd.rules.
> 
> If we could still keep the single DM_UDEV_DISABLE_OTHER_RULES_FLAG
> for
> others to check, I'd really like to have only that one, not adding
> more.

I'm fine with that, but changes to higher-level rules will be necessary
either way, be it only that they refrain from accessing those variables
that we don't want them to access.

I the longer term, I suggest that properties that we don't want higher
layers to access be marked as such [*]. For example, DM_NOSCAN could be
replaced by __DM_NOSCAN, following the widely respected convention that
symbols starting with underscores should be left alone. DM_SUSPENDED
could actually be replaced by .DM_SUSPENDED, which would correctly
reflect the fact that it's never restored from the udev db (as
properties starting with "." aren't saved to the db).

Thanks,
Martin

[*] I think the suggestive names of DM_NOSCAN and DM_SUSPENDED have
have contibuted to their adoption by 3rd parties. Sometimes it's better
to use non-intuitive variable names like DM_UDEV_SUBSYSTEM_FLAG0 :-)

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Benjamin Marzinski]

On Mon, Feb 12, 2024 at 03:16:27PM +0100, Martin Wilck wrote:
> On Mon, 2024-02-12 at 13:32 +0100, Peter Rajnoha wrote:
> > On 2/12/24 12:09, Martin Wilck wrote:
> > 
> > 
> > Right, underneath within DM/DM-subystem, we should be able to keep
> > and
> > restore those reasons for why it has been flagged with
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG till now and when the state is good
> > enough that we can drop it, we would do it transparently for higher
> > (non-dm and non-dm-subsystem) layers. So if there's a case that is
> > not
> > currently handled by 10-dm.rules or 11-dm-<subsystem>.rules, we can
> > fix
> > that there. If it's a generic rule that applies to all DM, not just
> > subystem, then yes, we can move that to 10-dm.rules (will have a look
> > at
> > your patch [1]).
> 
> I don't think that patch can be used as-is for DM. For multipath, I'm
> not aware of any situation where DM_UDEV_DISABLE_OTHER_RULES_FLAG would
> be set in the udev cookie, therefore DM_UDEV_DISABLE_OTHER_RULES_FLAG
> is essentially an alias for DM_SUSPENDED before 11-dm-mpath.rules
> changes it. That's not the case for other targets, in particular LVM.
> 
> > > 
> > > Well, IIUC the main reason that systemd couldn't use
> > > DM_UDEV_DISABLE_OTHER_RULES_FLAG was at least in part due to the
> > > fact
> > > that the multipath rules used it in a special way that was
> > > inconsistent
> > > with the rest of DM ;-)
> > 
> > 
> > Aha! Well, honestly, I don't remember the exact details and context
> > of
> > that fix, but I know we haven't found a better way...
> > 
> > > 
> > > I think there are 3 variants of "unusable":
> > > 
> > > a) temporarily unusable (just for this event), ignore this uevent
> > > and
> > > restore previous properties from db.
> > > b) unusable, avoid IO, don't scan, don't activate (this is how we
> > > use
> > > DM_UDEV_DISABLE_OTHER_RULES_FLAG). Upper layers will usually load
> > > saved
> > > properties from udev db in this case, too.
> > > c) like b), but also try to unmount / unconfigure if already used.
> > > This
> > > is SYSTEMD_READY=0. I don't think DM has a flag with these
> > > semantics at
> > > this time. I can imagine such a flag being set if a device was
> > > reloaded
> > > with an incompatible table, but that's rather a corner case.
> > > 
> > > It's an honorable goal to condense everything into a single
> > > variable
> > > for consumer rules, but I think it doesn't work if we want the
> > > upper
> > > layers to be able to distinguish these. We can merge a) and b) I
> > > think,
> > > because their meaning for upper layers is practically the same, if
> > > we
> > > get the saving and restoring right.
> > > 
> > 
> > The c) case - well, it's questionable what should be done in that
> > case,
> > because that means we have literally cut off the underlying device
> > while
> > it was still in use. Any further IO from higher layers will return IO
> > errors, will queue IOs or, in the worse case, issue IOs to a device
> > that
> > we don't want to anymore.
> > 
> > Anyway, if I understand correctly, we simply need to signal higher
> > layers either:
> > 
> >   A) device is unusable, forget it and clear all your current extra
> > records you have about this device (including removing any custom
> > symlinks for udev). That would also map to SYSTEMD_READY=0.
> > 
> >   B) device is unusable temporarily, restore any records you need,
> > wait
> > for the DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 to drop to 0 (or being
> > unset).
> > 
> > What do you think about keeping a single
> > DM_UDEV_DISABLE_OTHER_RULES_FLAG for this, just having a different
> > value, say "2" to denote the B case? Otherwise, we need 2 distinct
> > variables (which is harder for others to accept I bet).
> 
> Yes, that could work, if the save / restore is implemented cleanly.

What if we never read DM_UDEV_DISABLE_OTHER_RULES_FLAG from the
database. Instead how about, if DM_UDEV_DISABLE_OTHER_RULES_FLAG is set
by "dmsetup udevflags", we save it as something like DM_IGNORE_DEVICE.
Otherwise, if it's a spurious event, we read DM_IGNORE_DEVICE from the
database. After "dm_flags_done", if DM_IGNORE_DEVICE is set, we set
DM_UDEV_DISABLE_OTHER_RULES_FLAG. This leaves the other rules free to
mess with DM_UDEV_DISABLE_OTHER_RULES_FLAG all they want.

> > 
> > > > The DM_NOSCAN was actually just a helper and a more human
> > > > readable
> > > > name
> > > > for "DM_SUSBYSTEM_UDEV_FLAG0" within LVM subsystem *only* at
> > > > first.
> > > > It is used during LV initialization - the wiping/zeroing of the
> > > > LV
> > > > before it is pronounced as usable - that's why, when it is set,
> > > > we
> > > > signal to "others" the DM_UDEV_DISABLE_OTHER_RULES_FLAG based on
> > > > this
> > > > flag. However, since we have the 13-dm-disk.rules which manages
> > > > the
> > > > blkid call for DM devices (and which is ours - owned by DM), we
> > > > need
> > > > to
> > > > signal these rules to avoid calling blkid (as it could see
> > > > uninitiliazed/stale data). In this case, we import any previous
> > > > scan
> > > > results from udev db. It's not like "completely ignore and skip
> > > > rules
> > > > for this event".
> > > 
> > > I'm not sure if I understand what you mean with "completely ignore
> > > and
> > > skip". Upper-level rules usually can't "completely ignore" an
> > > uevent if
> > > they need to preserve any udev properties across the current event.
> > > If
> > > the device is unusable in the weaker "don't try IO" sense, the
> > > upper
> > > rules must import existing properties from the db, just like 13-dm-
> > > disk.rules, lest the properties be forgotten by udev. IMO this
> > > weaker
> > > semantics (corresponding to b) above) is what matters most for
> > > upper
> > > level rules.
> > 
> > I didn't consider that there might be extra records to keep around
> > for
> > anything else than blkid (13-dm-disk.rules) and DM/DM subsys (10-dm
> > and
> > 11-dm-subsystem.rules). But you're probably right here -
> > differentiating
> > the A) and B) from above could be beneficial for some layers above.
> > If
> > nothing else, then at least the systemd's SYSTEMD_READY case in
> > 99-systemd.rules.
> > 
> > If we could still keep the single DM_UDEV_DISABLE_OTHER_RULES_FLAG
> > for
> > others to check, I'd really like to have only that one, not adding
> > more.
> 
> I'm fine with that, but changes to higher-level rules will be necessary
> either way, be it only that they refrain from accessing those variables
> that we don't want them to access.
> 
> I the longer term, I suggest that properties that we don't want higher
> layers to access be marked as such [*]. For example, DM_NOSCAN could be
> replaced by __DM_NOSCAN, following the widely respected convention that
> symbols starting with underscores should be left alone. DM_SUSPENDED
> could actually be replaced by .DM_SUSPENDED, which would correctly
> reflect the fact that it's never restored from the udev db (as
> properties starting with "." aren't saved to the db).
> 
> Thanks,
> Martin
> 
> [*] I think the suggestive names of DM_NOSCAN and DM_SUSPENDED have
> have contibuted to their adoption by 3rd parties. Sometimes it's better
> to use non-intuitive variable names like DM_UDEV_SUBSYSTEM_FLAG0 :-)

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Martin Wilck]

On Thu, 2024-02-15 at 17:45 -0500, Benjamin Marzinski wrote:
> On Mon, Feb 12, 2024 at 03:16:27PM +0100, Martin Wilck wrote:
> > On Mon, 2024-02-12 at 13:32 +0100, Peter Rajnoha wrote:
> > 
> > > 
> > > What do you think about keeping a single
> > > DM_UDEV_DISABLE_OTHER_RULES_FLAG for this, just having a
> > > different
> > > value, say "2" to denote the B case? Otherwise, we need 2
> > > distinct
> > > variables (which is harder for others to accept I bet).
> > 
> > Yes, that could work, if the save / restore is implemented cleanly.
> 
> What if we never read DM_UDEV_DISABLE_OTHER_RULES_FLAG from the
> database. Instead how about, if DM_UDEV_DISABLE_OTHER_RULES_FLAG is
> set
> by "dmsetup udevflags", we save it as something like
> DM_IGNORE_DEVICE.
> Otherwise, if it's a spurious event, we read DM_IGNORE_DEVICE from
> the
> database. After "dm_flags_done", if DM_IGNORE_DEVICE is set, we set
> DM_UDEV_DISABLE_OTHER_RULES_FLAG. This leaves the other rules free to
> mess with DM_UDEV_DISABLE_OTHER_RULES_FLAG all they want.

That sounds good and aligns with what I'd thought by myself. But we
should use a less suggestive name. DM_IGNORE_DEVICE would again make
users think that they should consume this variable, like DM_NOSCAN.

Martin

Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Peter Rajnoha]

On 2/16/24 15:29, Martin Wilck wrote:
> On Thu, 2024-02-15 at 17:45 -0500, Benjamin Marzinski wrote:
>> On Mon, Feb 12, 2024 at 03:16:27PM +0100, Martin Wilck wrote:
>>> On Mon, 2024-02-12 at 13:32 +0100, Peter Rajnoha wrote:
>>>
>>>>
>>>> What do you think about keeping a single
>>>> DM_UDEV_DISABLE_OTHER_RULES_FLAG for this, just having a
>>>> different
>>>> value, say "2" to denote the B case? Otherwise, we need 2
>>>> distinct
>>>> variables (which is harder for others to accept I bet).
>>>
>>> Yes, that could work, if the save / restore is implemented cleanly.
>>
>> What if we never read DM_UDEV_DISABLE_OTHER_RULES_FLAG from the
>> database. Instead how about, if DM_UDEV_DISABLE_OTHER_RULES_FLAG is
>> set
>> by "dmsetup udevflags", we save it as something like
>> DM_IGNORE_DEVICE.
>> Otherwise, if it's a spurious event, we read DM_IGNORE_DEVICE from
>> the
>> database. After "dm_flags_done", if DM_IGNORE_DEVICE is set, we set
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG. This leaves the other rules free to
>> mess with DM_UDEV_DISABLE_OTHER_RULES_FLAG all they want.
> 
> That sounds good and aligns with what I'd thought by myself. But we
> should use a less suggestive name. DM_IGNORE_DEVICE would again make
> users think that they should consume this variable, like DM_NOSCAN.
> 

Yup, that looks reasonable and clean.

Though, the DM_UDEV_DISABLE_OTHER_RULES_FLAG is only one flag from the
whole set. To avoid handling only a single selected flag in a special
way and to avoid issues raising from that, we might as well apply that
logic to all the other flags decoded by `dmsetup udevflags`.

-- 
Peter