Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Benjamin Marzinski <bmarzins@redhat.com>]

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 :-)