device: make sure to always retroactively start device dependencies #8675

fbuihuu · 2018-04-06T16:26:37Z

PID1 updates the state of device units upon 2 different events:

when it processes an event sent by udev and in this case the device deps are
started if the device enters in the "plugged" state.
when it enumerates all devices during its startup or when it is asked to
reload its configuration data but in this case the device deps (if any) are
not retroactively started.

Whe udev processes a new "add" kernel event, it first registers the new device
in its database then sends a new event to systemd.

If for any reason, systemd is asked to reload its configuration (via
"daemon-reload" command for example) between the previous 2 steps, it might see
for the first time the new device when enumerating all devices via udev DB
first and process the event sent by udev in a second step.

In this peculiar case the device deps wont be started (even though the device
is first seen by PID1).

Indeed while "daemon-reload", PID1 will put the device unit in the "plugged"
state but without starting the device deps. Thereafter PID1 will get the event
from udev for the new device but the device unit will be in "plugged" state
already therefore it won't see any need to start the device dependencies.

Rather than assuming that during the reloading of systemd manager configuration
all devices listed in udev DB have been already processed and should be put in
the "plugged" state (done by device_coldplug()), this patch does that only for
devices which have been processed via an udev event (device_dispatch_io())
previously. In this case we set "d->found" to "DEVICE_FOUND_UDEV" and we make
also sure to no more initialize "d->found" while enumerating devices. Instead
this field is now saved/restored while devices are serialized.

fbuihuu · 2018-04-06T16:27:38Z

Please double check as I'm not really sure about this one.

poettering

I think this makes a lot of sense, but I'd prefer if we'd not turn the raw bit values into API, see comments...

poettering · 2018-04-12T17:55:16Z

src/core/device.c

@@ -165,6 +165,7 @@ static int device_serialize(Unit *u, FILE *f, FDSet *fds) {
        assert(fds);

        unit_serialize_item(u, f, "state", device_state_to_string(d->state));
+        unit_serialize_item_format(u, f, "found", "%i", d->found);


i feel a bit nervous about serializing the binary value of this flag field, as it turns something into an API that is not necessarily obvious as one... Hence, I'd rather see three individual bool fields serialized for the three relevant bits in the flags field...

good point, I'll update the PR accordingly, thanks for the review.

fbuihuu · 2018-04-16T16:22:58Z

@poettering I just realize that it will probably wont work when we'll upgrade systemd from a version that doesn't serialize "d->found" to a new one that does...

Indeed when reexec systemd, the old version won't serialize "d->found" and the new one expects to find the info to correctly initialize this field.

But that's actually a more general issue which should happen every times a new field is serialized, or am I missing something ?

fbuihuu · 2018-04-18T07:54:44Z

New version forced pushed:

the serialization of "found" does not use the raw bit format anymore
I added an additional fix in a separate commit (see commit message for details)

keszybz

With this patch, after daemon-reexec from previous version, all devices will be in not-found state. Not good.

I think it's necessary to provide an upgrade mechanism:

add a tri-state field in the device structure, with values "not seen during deserialization", "seen during deserialization", "found state set during serialization"
during deserialization set state to "seen" when the device is encountered, and to "found state set" if "found=" is encountered
after deserialization is complete, flip all "seen" to DEVICE_FOUND_UDEV.

That way this upgrade from previous versions will behave like upgrades did previously, and upgrade from this version to another will make use of the serialized state.

keszybz · 2018-04-20T10:05:02Z

src/core/device.c

@@ -179,6 +179,16 @@ static int device_deserialize_item(Unit *u, const char *key, const char *value,
        assert(value);
        assert(fds);

+        /* The device was known at the time units were serialized but it's not
+         * anymore at the time units are deserialized. This happens when PID1 is
+         * reexec after having switched to the new rootfs. When devices are


reexec → reexecuted

keszybz · 2018-04-20T10:06:37Z

src/core/device.c

+         * reexec after having switched to the new rootfs. When devices are
+         * enumerated, udevd isn't running yet and therefore the list of devices
+         * (handled by systemd) to init was empty. We ignore completely such
+         * case and wait for the device events to be re-triggered by udev so


Hmm... "we completely ignore such case" — I guess you mean the opposite, we handle this case (by doing nothing now and waiting for the device to be re-triggered by udev and initialized properly).

yes that's what I tried to mean, will reword this part. Thanks.

fbuihuu · 2018-04-20T12:54:04Z

@keszybz you're right it's currently broken when PID is reexecuted.

That was actually the point I raised in my previous comment #8675 (comment). And I wondered how it was supposed to be handled since all new introductions of field serialization have the same issue...

keszybz · 2018-04-20T13:55:57Z

That was actually the point I raised in my previous comment #8675 (comment)

I was responding to that ;)

And I wondered how it was supposed to be handled since all new introductions of field serialization have the same issue...

Right. We always need to do something backwards compatible, as far as reasonable. Sometimes it's not possible, vide e8a565c.

PID1 updates the state of device units upon 2 different events: - when it processes an event sent by udev and in this case the device deps are started if the device enters in the "plugged" state. - when it enumerates all devices during its startup or when it is asked to reload its configuration data but in this case the device deps (if any) are not retroactively started. When udev processes a new "add" kernel event, it first registers the new device in its databases then sends an event to systemd. If for any reason, systemd is asked to reload its configuration between the previous 2 steps, it might see for the first time the new device while scanning /sys for all devices. Only during a second step, udev will send the event for the new device. In this peculiar case the device deps wont be started (even though the device is first seen by PID1). Indeed when reloading its configurations, PID1 will put the device unit in the "plugged" state but without starting the device deps. Thereafter PID1 will get the event from udev for the new device but the device unit will be in "plugged" state already therefore it won't see any need to start the device dependencies. Rather than assuming that during the reloading of systemd manager configuration all devices listed in udev DBs have been already processed and should be put in the "plugged" state (done by device_coldplug()), this patch does that only for devices which have been processed via an udev event (device_dispatch_io()) previously. In this case we set "d->found" to "DEVICE_FOUND_UDEV" and we make also sure to no more initialize "d->found" while enumerating devices. Instead this field is now saved/restored while devices are serialized.

Do not try to party initialize a device during deserialization if it's not known by udev (anymore) and therefore hasn't been seen during device enumeration. The device unit in this case has not been initialized properly and setting it in the "plugged" state can be confusing. Actually this happens during every boots when PID switches to the new rootfs: PID is reexecuted and enumerates devices but since udev is not running, the list of enumerated devices is empty.

fbuihuu · 2018-04-20T15:52:55Z

@keszybz how about something like this ?

keszybz · 2018-04-23T07:13:57Z

KVM: entry failed, hardware error 0x0

keszybz · 2018-04-23T07:14:27Z

Looks good.

This reworks how device units are "powered on". This makes sure that any device changes that might have happened while we were restarting/reloading will be noticed properly. For that we'll now properly serialize/deserialize both the device unit state and the device "found" flags, and restore these initially in the "coldplug" phase of the manager deserialization. While enumerating the udev devices during startup we'll put together a new "found" flags mask, which we'll the switch to in the "catchup" phase of the manager deserialization, which follows the "coldplug" phase. Note that during the "coldplug" phase no unit state change events are generated, which is different for the "catchall" phase which will do that. Thus we correctly make sure that the deserialized state won't pull in new deps, but any device's change while we were reloading would. Fixes: systemd#8832 Replaces: systemd#8675

fbuihuu added the pid1 label Apr 6, 2018

poettering requested changes Apr 12, 2018

View reviewed changes

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Apr 12, 2018

fbuihuu force-pushed the make-sure-device-enter-in-plugged-state branch from d56f183 to df96af5 Compare April 18, 2018 07:52

fbuihuu removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Apr 18, 2018

keszybz requested changes Apr 20, 2018

View reviewed changes

fbuihuu added 2 commits April 20, 2018 17:49

fbuihuu force-pushed the make-sure-device-enter-in-plugged-state branch from df96af5 to 036d2ee Compare April 20, 2018 15:52

keszybz added the ci-failure-appears-unrelated label Apr 23, 2018

keszybz approved these changes Apr 23, 2018

View reviewed changes

keszybz merged commit d2bc13b into systemd:master Apr 23, 2018

fbuihuu deleted the make-sure-device-enter-in-plugged-state branch April 23, 2018 07:15

yuwata mentioned this pull request Apr 24, 2018

device: fix serialization and deserialization of DeviceFound #8798

Merged

poettering mentioned this pull request Apr 26, 2018

device flags logic is borked #8832

Closed

mwilck mentioned this pull request Apr 27, 2022

Boot to emergency mode if devices are switched to SYSTEMD_READY=0 (multipath scenario) #23208

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

device: make sure to always retroactively start device dependencies #8675

device: make sure to always retroactively start device dependencies #8675

fbuihuu commented Apr 6, 2018

fbuihuu commented Apr 6, 2018

poettering left a comment

poettering Apr 12, 2018

fbuihuu Apr 13, 2018

fbuihuu commented Apr 16, 2018

fbuihuu commented Apr 18, 2018

keszybz left a comment •

edited

keszybz Apr 20, 2018

keszybz Apr 20, 2018

fbuihuu Apr 20, 2018

fbuihuu commented Apr 20, 2018

keszybz commented Apr 20, 2018

fbuihuu commented Apr 20, 2018

keszybz commented Apr 23, 2018

keszybz commented Apr 23, 2018

device: make sure to always retroactively start device dependencies #8675

device: make sure to always retroactively start device dependencies #8675

Conversation

fbuihuu commented Apr 6, 2018

fbuihuu commented Apr 6, 2018

poettering left a comment

Choose a reason for hiding this comment

poettering Apr 12, 2018

Choose a reason for hiding this comment

fbuihuu Apr 13, 2018

Choose a reason for hiding this comment

fbuihuu commented Apr 16, 2018

fbuihuu commented Apr 18, 2018

keszybz left a comment • edited

Choose a reason for hiding this comment

keszybz Apr 20, 2018

Choose a reason for hiding this comment

keszybz Apr 20, 2018

Choose a reason for hiding this comment

fbuihuu Apr 20, 2018

Choose a reason for hiding this comment

fbuihuu commented Apr 20, 2018

keszybz commented Apr 20, 2018

fbuihuu commented Apr 20, 2018

keszybz commented Apr 23, 2018

keszybz commented Apr 23, 2018

keszybz left a comment •

edited