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 12:19, Peter Rajnoha wrote:
> On 3/4/24 11:48, Peter Rajnoha wrote:
>> On 3/1/24 23:40, Martin Wilck wrote:
>>> DISK_RO=1 is set in the environment of a block-device uevent if and only if
>>> the device has just been set read-only by a driver by calling set_disk_ro().
>>> It is not synoymous with the "ro" sysfs attribute; the device could very well
>>> be write-protected if DISK_RO is not set. Probing should be possible even if
>>> this flag is set, because blkid and friends usually don't write to the
>>> device. Upper-layer subsystems that do need to write would need to check the
>>> "ro" sysfs attribute rather than DISK_RO.
>>>
>>> Skip the DISK_RO check in 10-dm.rules.
>>>
>>> Signed-off-by: Martin Wilck <mwilck@suse.com>
>>> ---
>>>  udev/10-dm.rules.in | 3 ---
>>>  1 file changed, 3 deletions(-)
>>>
>>> diff --git a/udev/10-dm.rules.in b/udev/10-dm.rules.in
>>> index 4ffd3e2..c08d829 100644
>>> --- a/udev/10-dm.rules.in
>>> +++ b/udev/10-dm.rules.in
>>> @@ -50,9 +50,6 @@ ACTION!="add|change", GOTO="dm_end"
>>>  # kernels >= 2.6.31 only. Cookie is not decoded for remove event.
>>>  ENV{DM_COOKIE}=="?*", IMPORT{program}="(DM_EXEC)/dmsetup udevflags $env{DM_COOKIE}"
>>>  
>>> -# Rule out easy-to-detect inappropriate events first.
>>> -ENV{DISK_RO}=="1", GOTO="dm_disable"
>>> -
>>>  # There is no cookie set nor any flags encoded in events not originating
>>>  # in libdevmapper so we need to detect this and try to behave correctly.
>>>  # For such spurious events, regenerate all flags from current udev database content
>>
>>
>> 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.
>>
> 
> Hmm, this doesn't even work with original rules!
> 
> The 99-systemd.rules has:
> 
> SUBSYSTEM=="block", ACTION=="add",
> ENV{DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1", ENV{SYSTEMD_READY}="0"
> 
> which applies only to "add" uevent, not "change". This means that
> SYSTEMD_READY="0" is even dropped for the DISK_RO="1" uevent even if it
> is marked as DM_UDEV_DISABLE_OTHER_RULES_FLAG="1".
> 
> This is actually a bug. But a minor one, because here we always have the
> change uevent with DM_COOKIE coming right after the DISK_RO change
> uevent. So there's almost no window practically.
> 
> Now, I'm asking myself why we have the 'ACTION="add"' in the systemd
> rule at all. Why is it not just checking  DM_UDEV_DISABLE_OTHER_RULES_FLAG?
> 

Oh! :)

https://github.com/systemd/systemd/commit/35a6750d9e26b26b423fbe815bead7d750210d8d

-- 
Peter