rules: do not "unready" suspended encrypted devices w/o superblock info

[msekletar]

When device is suspended any attempt to use it will block, requesting
process will be put to 'D' scheduling state. We do not scan suspended
device with blkid thus ID_FS_USAGE and ID_PART_TABLE_TYPE are not set.
Hence, it doesn't make sense to try to mark it as not ready because any
attempt by systemd to use it (e.g. unmount FS as a consequence of
SYSTEMD_READY=0) will be blocked. Moreover, after resume the device will
be again fully functional and useable.

[poettering]

hmm, this doesn't look right?

the status quoa ante was that crypt devices without a superblock should not be considered ready.

With your change you change this to allow crypt services lacking a superblock but which are suspended to be considered ready, and that doesn't appear to me is what you are trying to do.

[msekletar]

I want to prevent systemd from unmounting FS on top of suspended devices. This is mostly for case of CHANGE events (i.e. device was there previously and now is suspended and CHANGE was triggered). ADD events are handled on the line above, as suspended devices have DM_UDEV_DISABLE_OTHER_RULES_FLAG}=="1" so they won't be marked as ready.

Btw, with the suspended device you don't know whether it was superblock or not because we won't read it.

[msekletar]

Specifically, I want systemd to not react to bellow sequence of commands,

dmsetup suspend /dev/mapper/disk
udevadm trigger
dmsetup resume /dev/mapper/disk

Now this sequence of step will cause unmount of FS that is on top of /dev/mapper/disk (if this is configured through fstab). This is not wanted and unexpected. After suspend every attempt to use the device will block in kernel anyway. There is no risk of data corruption and no need for systemd to spawn unmount.

[msekletar]

I probably shouldn't have said "...w/o superblock info" in the commit message. We do not have superblock info not because it is not there but because we skip scanning the device because it is suspended.

[poettering]

hmm, oh.

so this patch will address that specific issue, but create a new issue.

I mean, we don't want to access the device at all when it is suspended. Which the existing rule enforced. So the logic so far was "if it's suspended, then it's not really there".

But with your change you want that what is already set up isn#t destroyed if the device is suspended. So the logic you are looking for is "if it's suspended, then it's no really gone".

The fact is that our current model cannot express this concept of "hey, if a device is suspended then don't do anything new with it, but also don't destroy everything old on it".

This kinda suggests that while DM suspend is in effect on a block device we should try to inherit the SYSTEMD_READY state from the previous database, no?

[msekletar]

Hmm, what about actually reacting or rather not reacting to events at all if DM_SUSPENDED key is set to 1. I mean SYSTEMD_READY is about state changes but for suspended devices we probably shouldn't do any state transitions until the device is resumed because we are receiving events with incomplete data.

[poettering]

yeah, but that's precisely what inheriting SYSTEMD_READY from the previous db state will give you

[mvollmer]

What about not running udev rules at all for suspended device mapper devices?

[poettering]

What about not running udev rules at all for suspended device mapper devices?

not running udev rules would mean all props are dropped that normally set by udev rules. including the SYSTEMD_READY prop. hence this might mark devices that previously were marked as unready via SYSTEMD_READY=0 as ready...

[msekletar]

@poettering I've updated the PR. PTAL.

@prajnoha @mvollmer PTAL as well and let me know what do you think.

[poettering]

hmm, i think it would be better to make sure SYSTEMD_READY is always set, and then just blindly import it in case the device is suspended.

i.e.:

SUBSYSTEM=="block", ENV{DM_SUSPENDED}=="1", IMPORT{db}="SYSTEMD_READY", GOTO="systemd_end"

and then something like the this further down as final catchall that ensures SYSTEMD_READY is set if not set by any other rule before it.

# explicitly set SYSTEMD_READY=1 for DM devices that don't have it set yet, so that we always have something to import above
SUBSYSTEM=="block", ENV{DM_UUID}=="?*", ENV{SYSTEMD_READY}=="", ENV{SYSTEMD_READY}="1"

That way, we are pretty systematic and don't ever are tempted to run any fancier rules on suspended devices and don't have to duplicate tests that much.

[msekletar]

Ok, setting it always is something I haven't considered because we (as in our rules) never set it to "1" previously. But I agree this would be probably simpler in the end.

[msekletar]

@poettering updated. And thanks again for review and suggestions, much appreciated!

[poettering]

lgtm