Re: [RFC PATCH 2/7] 10-dm.rules: don't deactivate devices for DISK_RO=1

[Peter Rajnoha <prajnoha@redhat.com>]

On 3/4/24 17:09, Martin Wilck wrote:
> On Mon, 2024-03-04 at 11:48 +0100, Peter Rajnoha wrote:
>>
>> Yes, I'd like to get rid of this rule, but, unfortunately, there's
>> one
>> issue during the DM device creation/activation.
>>
>> For example, if I try:
>>
>>   dmsetup create --readonly --table "0 1 zero"
>>
>> Then I get these uevents:
>>
>> 1)
>> ACTION=add
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> SYSTEMD_READY=0
>>
>>
>> 2)
>> ACTION=change
>> DM_UDEV_DISABLE_SUBSYSTEM_RULES_FLAG=1
>> DM_UDEV_DISABLE_DISK_RULES_FLAG=1
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> 3)
>> ACTION=change
>> DM_COOKIE=6335392
>> DM_COOKIE_DISABLE_OTHER_RULES_FLAG=
>>
>>
>> The uevent 3) coming with the DM_COOKIE is the actual event when the
>> device is ready for use (that's the uevent notifying the DM device
>> resume/activation).
>>
>> If we remove the DISK_RO rule, then the
>> DM_UDEV_DISABLE_OTHER_RULES_FLAG
>> is unset for uevent 2), which in turn causes the SYSTEMD_READY=0 to
>> get
>> dropped, which in turn will start all the systemd hooks because the
>> device is considered "ready" for systemd then.
>> But the DM dev is ready only after uevent 3) that comes with the
>> DM_COOKIE. So we still need to cover this scenario.
> 
> As event 2) doesn't have DM_COOKIE, I don't think we need to bother
> about it much. IMO we should treat it like a synthetic "change" event,
> which has almost no effect as far as dm is concerned.

Well, partly yes, but the important thing here is that the other rules
don not consider this as something they should react on. Here, it's like
the (genuine) "add" event for a DM device that has almost zero value to
others (we still need the table to get loaded and the device resumed for
it to be usable).

That's why the DISK_RO uevent was marked with DUDORF flag in the
original 10-dm.rules as anything before the activation mark is simply
considered spurious.

> Event 3) doesn't have DISK_RO=1 set. If any later rules are interested
> in the state of write protection, they need to check the "ro" sysfs
> attribute instead.

The unfortunate thing about the DISK_RO, unlike other events, is that it
is triggered *before* that DM activation (DM activation == table load +
resume). Also, frankly, I don't like this event at all - it's just
notifying about one of the many device attributes. A question then
arises: "Why don't we have uevents for changes in other attributes?",
but that's how it is for now and it's probably for a separate debate.

Till now, we've been marking that DISK_RO uevent as spurious completely.
But yes, we should have probably done that only in case it happens
before the activation, not if the device is already activated, if that's
the case too.

> 
> It would make some sense to be able tell later rules that they don't
> need to bother with a given uevent because (from device mapper PoV)
> nothing relevant has changed. I am not sure if we should use
> DM_UDEV_DISABLE_OTHER_RULES_FLAG=1 for this purpose. "Don't bother,
> nothing has changed" is not the same thing as "don't bother, you can't
> access this device right now", which to my understanding is the meaning
> of DM_UDEV_DISABLE_OTHER_RULES_FLAG=1.

Actually, depending on the perspective, the DISK_RO uevent happening
before the activation is that kind of "you can't access this device
right now" (because it has not been activated fully yet).

> 
> Actually we have DM_ACTIVATION=1 to tell other rules that they do need
> to take action. Later rules only need to rescan a DM device if
> DM_ACTIVATION=1; in all other cases they could just import properties
> from the db. Currently kpartx and lvm are the only rules that check
> DM_ACTIVATION.

Yes, indeed, good point, the DM_ACTIVATION=1 is a little helper so the
rules which need to react *only on the event where the DM device has
just been activated/reactivated* with a new table.

It is set in two cases, IIRC:

  - on genuine "change" event where a new DM table is activated (a
resume after table load)

  - on synthetic "add" event *after* the device has already been
activated before (to cover the coldplug/udevadm trigger --action=add)

If there's a case where other rules do care about reacting only on this
particular event, then they should check DM_ACTIVATION, yes. But is
there such a case for other (non-dm, non-dm-subysystem) rules?

For the DISK_RO, it's about whether it comes before or after the
activation, not whether the DISK_RO is set with the activation at the
same time (which is not, it's a separate event). For this case, we don't
have anything to check right now - we just simply "disable" all the
events coming before the activation.

-- 
Peter