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