From 1ece068e833e4a6dd364379ecfefd88b9f97d2b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Jun 2021 19:23:15 +0200 Subject: [PATCH 1/3] core: update comment unit_serialize_item() was dropped in d68c645bd3323ae1f0dfcb8fd74ea6b19681db8a. But "cannot be restored from other sources" is also not entirely true: for example for mounts we may be able to figure out most state from /p/s/mountinfo. So let's make the comment more oblique. --- src/core/unit.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/unit.h b/src/core/unit.h index d34d647845..2db40e51e8 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -543,8 +543,8 @@ typedef struct UnitVTable { bool (*can_reload)(Unit *u); - /* Write all data that cannot be restored from other sources - * away using unit_serialize_item() */ + /* Serialize state and file descriptors that should be carried over into the new + * instance after reexecution. */ int (*serialize)(Unit *u, FILE *f, FDSet *fds); /* Restore one item from the serialization */ From cbd4a8df5fb3d69d77451fdb8d177e02e09460d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 4 Jun 2021 14:46:59 +0200 Subject: [PATCH 2/3] core: drop bitfields in VTable object The usual: bitfields make sense as a memory-saving measure when we have many objects of a given type. When the object appears at most in a few copies, the overhead of additional code to access bitfields is more than the savings. --- src/core/unit.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/unit.h b/src/core/unit.h index 2db40e51e8..dddb419613 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -646,7 +646,7 @@ typedef struct UnitVTable { /* Type specific cleanups. */ void (*shutdown)(Manager *m); - /* If this function is set and return false all jobs for units + /* If this function is set and returns false all jobs for units * of this type will immediately fail. */ bool (*supported)(void); @@ -654,25 +654,25 @@ typedef struct UnitVTable { UnitStatusMessageFormats status_message_formats; /* True if transient units of this type are OK */ - bool can_transient:1; + bool can_transient; /* True if cgroup delegation is permissible */ - bool can_delegate:1; + bool can_delegate; /* True if the unit type triggers other units, i.e. can have a UNIT_TRIGGERS dependency */ - bool can_trigger:1; + bool can_trigger; /* True if the unit type knows a failure state, and thus can be source of an OnFailure= dependency */ - bool can_fail:1; + bool can_fail; /* True if units of this type shall be startable only once and then never again */ - bool once_only:1; + bool once_only; /* True if queued jobs of this type should be GC'ed if no other job needs them anymore */ - bool gc_jobs:1; + bool gc_jobs; /* True if systemd-oomd can monitor and act on this unit's recursive children's cgroup(s) */ - bool can_set_managed_oom:1; + bool can_set_managed_oom; } UnitVTable; extern const UnitVTable * const unit_vtable[_UNIT_TYPE_MAX]; From 755021d43448011ef169f20ec3a08d4e92c824af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 3 Jun 2021 19:40:01 +0200 Subject: [PATCH 3/3] core: do not serialize mounts and automounts for switch-root When e.g. tmp.mount is present in the initrd, and we serialize it, switch root, and deserialize, the new systemd is confused because it thinks /tmp is mounted. In general, it doesn't make sense to serialize anything that refers to paths in the old root file system. This fixes two errors for me: 1. tmp.mount was not mounted properly before local-fs.target. It would be mounted as some point (I guess when we re-read /proc/self/mountinfo for some other reason). In effect systemd-tmpfiles-setup.service would see one fs, and some other units started later a different one. In particular gdm.service would fail because the pre-created /tmp/.X11-unix with proper permissions would not exist at time it was started. 2. # systemd[1]: proc-sys-fs-binfmt_misc.automount: Got hangup/error on autofs pipe from kernel. Likely our automount point has been unmounted by someone or something else? # systemd[1]: proc-sys-fs-binfmt_misc.automount: Failed with result 'unmounted'. # systemd[1]: Mounting proc-sys-fs-binfmt_misc.mount... # systemd[1]: Mounted proc-sys-fs-binfmt_misc.mount. # systemd[1]: Starting systemd-binfmt.service... # systemd[1]: Finished systemd-binfmt.service. # systemd[1]: proc-sys-fs-binfmt_misc.automount: Path /proc/sys/fs/binfmt_misc is already a mount point, refusing start. # systemd[1]: Failed to set up automount proc-sys-fs-binfmt_misc.automount. # systemd[1]: proc-sys-fs-binfmt_misc.automount: Path /proc/sys/fs/binfmt_misc is already a mount point, refusing start. # systemd[1]: Failed to set up automount proc-sys-fs-binfmt_misc.automount. # systemd[1]: proc-sys-fs-binfmt_misc.automount: Path /proc/sys/fs/binfmt_misc is already a mount point, refusing start. # systemd[1]: Failed to set up automount proc-sys-fs-binfmt_misc.automount. # systemd[1]: Stopping systemd-binfmt.service... # systemd[1]: systemd-binfmt.service: Deactivated successfully. # systemd[1]: Stopped systemd-binfmt.service. I couldn't understand the error here, but in retrospect the first line is entirely correct: "someone or something else" was the old systemd unmounting the old root. --- src/core/automount.c | 1 + src/core/manager.c | 6 +----- src/core/mount.c | 1 + src/core/unit-serialize.c | 16 ++++++++++++++-- src/core/unit.h | 3 +++ 5 files changed, 20 insertions(+), 7 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index 8d75346d97..57d1265723 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -1087,6 +1087,7 @@ const UnitVTable automount_vtable = { .can_transient = true, .can_fail = true, .can_trigger = true, + .exclude_from_switch_root_serialization = true, .init = automount_init, .load = automount_load, diff --git a/src/core/manager.c b/src/core/manager.c index dd6e7693d0..addd04067f 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -3321,11 +3321,7 @@ int manager_serialize( if (u->id != t) continue; - /* Start marker */ - fputs(u->id, f); - fputc('\n', f); - - r = unit_serialize(u, f, fds, !switching_root); + r = unit_serialize(u, f, fds, switching_root); if (r < 0) return r; } diff --git a/src/core/mount.c b/src/core/mount.c index c1f36ac4f1..8e39cf3ab3 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -2176,6 +2176,7 @@ const UnitVTable mount_vtable = { .can_transient = true, .can_fail = true, + .exclude_from_switch_root_serialization = true, .init = mount_init, .load = mount_load, diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index 7da0d8a3db..4da69769a6 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -89,13 +89,25 @@ static const char *const io_accounting_metric_field_last[_CGROUP_IO_ACCOUNTING_M [CGROUP_IO_WRITE_OPERATIONS] = "io-accounting-write-operations-last", }; -int unit_serialize(Unit *u, FILE *f, FDSet *fds, bool serialize_jobs) { +int unit_serialize(Unit *u, FILE *f, FDSet *fds, bool switching_root) { int r; assert(u); assert(f); assert(fds); + if (switching_root && UNIT_VTABLE(u)->exclude_from_switch_root_serialization) { + /* In the new root, paths for mounts and automounts will be different, so it doesn't make + * much sense to serialize things. API file systems will be moved to the new root, but we + * don't have mount units for those. */ + log_unit_debug(u, "not serializing before switch-root"); + return 0; + } + + /* Start marker */ + fputs(u->id, f); + fputc('\n', f); + if (unit_can_serialize(u)) { r = UNIT_VTABLE(u)->serialize(u, f, fds); if (r < 0) @@ -175,7 +187,7 @@ int unit_serialize(Unit *u, FILE *f, FDSet *fds, bool serialize_jobs) { (void) serialize_item_format(f, ip_accounting_metric_field[m], "%" PRIu64, v); } - if (serialize_jobs) { + if (!switching_root) { if (u->job) { fputs("job\n", f); job_serialize(u->job, f); diff --git a/src/core/unit.h b/src/core/unit.h index dddb419613..8818392731 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -668,6 +668,9 @@ typedef struct UnitVTable { /* True if units of this type shall be startable only once and then never again */ bool once_only; + /* Do not serialize this unit when preparing for root switch */ + bool exclude_from_switch_root_serialization; + /* True if queued jobs of this type should be GC'ed if no other job needs them anymore */ bool gc_jobs;