Re: About DM_UDEV_DISABLE_OTHER_RULES_FLAG and DM_NOSCAN

[Martin Wilck <martin.wilck@suse.com>]

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