From b862c25716520d9381d5a841dba0f0c14e9c970a Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Mon, 30 Mar 2020 10:49:29 +0200 Subject: [PATCH 1/9] device: drop refuse_after Scheduling devices after a given unit can be useful to start device *jobs* at a specific time in the transaction, see commit 4195077ab4c823c. This (hidden) change was introduced by commit eef85c4a3f8054d2. --- src/core/device.c | 1 - src/core/unit.c | 11 ++++------- src/core/unit.h | 3 --- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/core/device.c b/src/core/device.c index 06bbbf8d2c..45149e7555 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -1064,7 +1064,6 @@ const UnitVTable device_vtable = { "Device\0" "Install\0", - .refuse_after = true, .gc_jobs = true, .init = device_init, diff --git a/src/core/unit.c b/src/core/unit.c index 2816bcef55..115938e6ce 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2957,13 +2957,10 @@ int unit_add_dependency( return 0; } - if (d == UNIT_AFTER && UNIT_VTABLE(u)->refuse_after) { - log_unit_warning(u, "Requested dependency After=%s ignored (%s units cannot be delayed).", other->id, unit_type_to_string(u->type)); - return 0; - } - - if (d == UNIT_BEFORE && UNIT_VTABLE(other)->refuse_after) { - log_unit_warning(u, "Requested dependency Before=%s ignored (%s units cannot be delayed).", other->id, unit_type_to_string(other->type)); + /* Note that ordering a device unit after a unit is permitted since it + * allows to start its job running timeout at a specific time. */ + if (d == UNIT_BEFORE && other->type == UNIT_DEVICE) { + log_unit_warning(u, "Dependency Before=%s ignored (.device units cannot be delayed)", other->id); return 0; } diff --git a/src/core/unit.h b/src/core/unit.h index 2e103f7ab2..93d6f7092b 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -610,9 +610,6 @@ typedef struct UnitVTable { /* True if the unit type knows a failure state, and thus can be source of an OnFailure= dependency */ bool can_fail:1; - /* True if After= dependencies should be refused */ - bool refuse_after:1; - /* True if units of this type shall be startable only once and then never again */ bool once_only:1; From bc9e5a4c67f5fff536d122118e16a53dfb592acd Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Mon, 30 Mar 2020 10:39:21 +0200 Subject: [PATCH 2/9] fstab-util: introduce fstab_is_extrinsic() --- src/core/mount.c | 26 +++++++------------------- src/shared/fstab-util.c | 24 ++++++++++++++++++++++++ src/shared/fstab-util.h | 1 + 3 files changed, 32 insertions(+), 19 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 1c4aefd734..4be4c1781a 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -400,32 +400,20 @@ static bool mount_is_extrinsic(Mount *m) { MountParameters *p; assert(m); - /* Returns true for all units that are "magic" and should be excluded from the usual start-up and shutdown - * dependencies. We call them "extrinsic" here, as they are generally mounted outside of the systemd dependency - * logic. We shouldn't attempt to manage them ourselves but it's fine if the user operates on them with us. */ + /* Returns true for all units that are "magic" and should be excluded from the usual + * start-up and shutdown dependencies. We call them "extrinsic" here, as they are generally + * mounted outside of the systemd dependency logic. We shouldn't attempt to manage them + * ourselves but it's fine if the user operates on them with us. */ - if (!MANAGER_IS_SYSTEM(UNIT(m)->manager)) /* We only automatically manage mounts if we are in system mode */ + /* We only automatically manage mounts if we are in system mode */ + if (!MANAGER_IS_SYSTEM(UNIT(m)->manager)) return true; if (UNIT(m)->perpetual) /* All perpetual units never change state */ return true; - if (PATH_IN_SET(m->where, /* Don't bother with the OS data itself */ - "/", /* (strictly speaking redundant: should already be covered by the perpetual flag check above) */ - "/usr", - "/etc")) - return true; - - if (PATH_STARTSWITH_SET(m->where, - "/run/initramfs", /* This should stay around from before we boot until after we shutdown */ - "/proc", /* All of this is API VFS */ - "/sys", /* … dito … */ - "/dev")) /* … dito … */ - return true; - - /* If this is an initrd mount, and we are not in the initrd, then leave this around forever, too. */ p = get_mount_parameters(m); - if (p && fstab_test_option(p->options, "x-initrd.mount\0") && !in_initrd()) + if (p && fstab_is_extrinsic(m->where, p->options)) return true; return false; diff --git a/src/shared/fstab-util.c b/src/shared/fstab-util.c index 86a57e6b2c..b19127be09 100644 --- a/src/shared/fstab-util.c +++ b/src/shared/fstab-util.c @@ -35,6 +35,30 @@ int fstab_has_fstype(const char *fstype) { return false; } +bool fstab_is_extrinsic(const char *mount, const char *opts) { + + /* Don't bother with the OS data itself */ + if (PATH_IN_SET(mount, + "/", + "/usr", + "/etc")) + return true; + + if (PATH_STARTSWITH_SET(mount, + "/run/initramfs", /* This should stay around from before we boot until after we shutdown */ + "/proc", /* All of this is API VFS */ + "/sys", /* … dito … */ + "/dev")) /* … dito … */ + return true; + + /* If this is an initrd mount, and we are not in the initrd, then leave + * this around forever, too. */ + if (opts && fstab_test_option(opts, "x-initrd.mount\0") && !in_initrd()) + return true; + + return false; +} + int fstab_is_mount_point(const char *mount) { _cleanup_endmntent_ FILE *f = NULL; struct mntent *m; diff --git a/src/shared/fstab-util.h b/src/shared/fstab-util.h index f575ed0bb2..a73575e95c 100644 --- a/src/shared/fstab-util.h +++ b/src/shared/fstab-util.h @@ -6,6 +6,7 @@ #include "macro.h" +bool fstab_is_extrinsic(const char *mount, const char *opts); int fstab_is_mount_point(const char *mount); int fstab_has_fstype(const char *fstype); From ad8f1b0f3601b423b3bad5fe8de667de531ce7c4 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Mon, 30 Mar 2020 10:47:31 +0200 Subject: [PATCH 3/9] generator: don't generate device dependencies for extrinsic mounts Stop generating device dependencies for extrinsic mounts: we already exclude extrinsic mounts from the usual start-up and shutdown dependencies but some extra deps added by generator_write_device_deps() were remaining. --- src/shared/generator.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/shared/generator.c b/src/shared/generator.c index acdd0096f1..04d2f86a4a 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -260,6 +260,9 @@ int generator_write_device_deps( _cleanup_free_ char *node = NULL, *unit = NULL; int r; + if (fstab_is_extrinsic(where, opts)) + return 0; + if (!fstab_test_option(opts, "_netdev\0")) return 0; From 457d65932b3832cc8fd103d09ffb3e7ea924d07c Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Wed, 1 Apr 2020 17:46:42 +0200 Subject: [PATCH 4/9] mount: mount unit activated by automount unit should be only ordered against the automount unit Both fstab-generator and pid1 are duplicating the handling of "Before=local-fs.target" dependency for mount units. fstab-generator is correctly skipping this dep if the mount unit is activated by an automount unit. However the condition used by pid1 was incorrect and missed the case when a mount unit uses "x-systemd.automount" since in this case the mount unit should be (only) ordered against its automount unit counterpart instead. --- src/core/mount.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/core/mount.c b/src/core/mount.c index 4be4c1781a..0b57f89c29 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -66,6 +66,14 @@ static bool MOUNT_STATE_WITH_PROCESS(MountState state) { MOUNT_CLEANING); } +static bool mount_is_automount(const MountParameters *p) { + assert(p); + + return fstab_test_option(p->options, + "comment=systemd.automount\0" + "x-systemd.automount\0"); +} + static bool mount_is_network(const MountParameters *p) { assert(p); @@ -473,7 +481,7 @@ static int mount_add_default_dependencies(Mount *m) { before = SPECIAL_LOCAL_FS_TARGET; } - if (!nofail) { + if (!nofail && !mount_is_automount(p)) { r = unit_add_dependency_by_name(UNIT(m), UNIT_BEFORE, before, true, mask); if (r < 0) return r; From 83cdc870949823b5b9fa04dd76e952d42faab0b1 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 2 Apr 2020 08:29:36 +0200 Subject: [PATCH 5/9] mount: let pid1 alone handle the default dependencies for mount units fstab-generator was also handling the default ordering dependencies for mount units setup in initrd. To do that it was turning the defaults dependencies off completely and ordered the mount unit against either local-fs.target or initrd-fs.target or initrd-root-fs.target itself. But it had the bad side effect to also remove all other default dependencies as well. Thus if an initrd mount was using _netdev, the network dependencies were missing. In general fstab-generator shouldn't use DefaultDependecies=no because it can handle only a small set of the default dependencies the rest are dealt by pid1. So this patch makes pid1 handle all default dependencies. --- src/core/mount.c | 21 ++++++++++++++++----- src/fstab-generator/fstab-generator.c | 9 --------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 0b57f89c29..a70013cecf 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -428,7 +428,7 @@ static bool mount_is_extrinsic(Mount *m) { } static int mount_add_default_dependencies(Mount *m) { - const char *after, *before; + const char *after, *before, *e; UnitDependencyMask mask; MountParameters *p; bool nofail; @@ -452,7 +452,16 @@ static int mount_add_default_dependencies(Mount *m) { mask = m->from_fragment ? UNIT_DEPENDENCY_FILE : UNIT_DEPENDENCY_MOUNTINFO_DEFAULT; nofail = m->from_fragment ? fstab_test_yes_no_option(m->parameters_fragment.options, "nofail\0" "fail\0") : false; - if (mount_is_network(p)) { + e = path_startswith(m->where, "/sysroot"); + if (e && in_initrd()) { + /* All mounts under /sysroot need to happen later, at initrd-fs.target time. IOW, + * it's not technically part of the basic initrd filesystem itself, and so + * shouldn't inherit the default Before=local-fs.target dependency. */ + + after = NULL; + before = isempty(e) ? SPECIAL_INITRD_ROOT_FS_TARGET : SPECIAL_INITRD_FS_TARGET; + + } else if (mount_is_network(p)) { /* We order ourselves after network.target. This is * primarily useful at shutdown: services that take * down the network should order themselves before @@ -487,9 +496,11 @@ static int mount_add_default_dependencies(Mount *m) { return r; } - r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, after, true, mask); - if (r < 0) - return r; + if (after) { + r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, after, true, mask); + if (r < 0) + return r; + } r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_UMOUNT_TARGET, true, mask); if (r < 0) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 08c7b76dba..16be342dbf 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -391,12 +391,6 @@ static int add_mount( "SourcePath=%s\n", source); - /* All mounts under /sysroot need to happen later, at initrd-fs.target time. IOW, it's not - * technically part of the basic initrd filesystem itself, and so shouldn't inherit the default - * Before=local-fs.target dependency. */ - if (in_initrd() && path_startswith(where, "/sysroot")) - fprintf(f, "DefaultDependencies=no\n"); - if (STRPTR_IN_SET(fstype, "nfs", "nfs4") && !(flags & AUTOMOUNT) && fstab_test_yes_no_option(opts, "bg\0" "fg\0")) { /* The default retry timeout that mount.nfs uses for 'bg' mounts @@ -411,9 +405,6 @@ static int add_mount( SET_FLAG(flags, NOFAIL, true); } - if (!(flags & NOFAIL) && !(flags & AUTOMOUNT)) - fprintf(f, "Before=%s\n", post); - if (!(flags & AUTOMOUNT) && opts) { r = write_after(f, opts); if (r < 0) From b3d7aef525dc1620a7948ffdbf3f36bfa3d5b5e8 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 2 Apr 2020 10:52:24 +0200 Subject: [PATCH 6/9] automount: fix handling of default dependencies for automount units First After=local-fs-pre.target wasn't described in the man page although it's part of the default dependencies automatically set by pid1. Secondly, Before=local-fs.target was only set if the automount unit was generated from the fstab-generator because the dep was explicitly generated. It was also not documented as a default dependency. Fix it by managing the dep from pid1 instead. --- man/systemd.automount.xml | 4 ++++ src/core/automount.c | 4 ++++ src/fstab-generator/fstab-generator.c | 2 -- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/man/systemd.automount.xml b/man/systemd.automount.xml index f2ed761021..35690fd22a 100644 --- a/man/systemd.automount.xml +++ b/man/systemd.automount.xml @@ -86,6 +86,10 @@ Automount units acquire automatic Before= and Conflicts= on umount.target in order to be stopped during shutdown. + + Automount units automatically gain an After= dependency + on local-fs-pre.target, and a Before= dependency on + local-fs.target. diff --git a/src/core/automount.c b/src/core/automount.c index 0b3f498bfc..99b4eb2c81 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -152,6 +152,10 @@ static int automount_add_default_dependencies(Automount *a) { if (!MANAGER_IS_SYSTEM(UNIT(a)->manager)) return 0; + r = unit_add_dependency_by_name(UNIT(a), UNIT_BEFORE, SPECIAL_LOCAL_FS_TARGET, true, UNIT_DEPENDENCY_DEFAULT); + if (r < 0) + return r; + r = unit_add_dependency_by_name(UNIT(a), UNIT_AFTER, SPECIAL_LOCAL_FS_PRE_TARGET, true, UNIT_DEPENDENCY_DEFAULT); if (r < 0) return r; diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 16be342dbf..bd022efcf2 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -526,8 +526,6 @@ static int add_mount( "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n", source); - fprintf(f, "Before=%s\n", post); - if (opts) { r = write_after(f, opts); if (r < 0) From 61154cf9533f0bbce674b2de22956f7086604c91 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 9 Apr 2020 15:01:53 +0200 Subject: [PATCH 7/9] mount: introduce mount_add_default_ordering_dependencies() Move the handling of the usual startup/shutdown dependencies in a dedicated funtion. No functional change. --- src/core/mount.c | 51 +++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 18 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index a70013cecf..585bfbde10 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -427,29 +427,17 @@ static bool mount_is_extrinsic(Mount *m) { return false; } -static int mount_add_default_dependencies(Mount *m) { +static int mount_add_default_ordering_dependencies( + Mount *m, + MountParameters *p, + UnitDependencyMask mask) { + const char *after, *before, *e; - UnitDependencyMask mask; - MountParameters *p; bool nofail; int r; assert(m); - if (!UNIT(m)->default_dependencies) - return 0; - - /* We do not add any default dependencies to /, /usr or /run/initramfs/, since they are guaranteed to stay - * mounted the whole time, since our system is on it. Also, don't bother with anything mounted below virtual - * file systems, it's also going to be virtual, and hence not worth the effort. */ - if (mount_is_extrinsic(m)) - return 0; - - p = get_mount_parameters(m); - if (!p) - return 0; - - mask = m->from_fragment ? UNIT_DEPENDENCY_FILE : UNIT_DEPENDENCY_MOUNTINFO_DEFAULT; nofail = m->from_fragment ? fstab_test_yes_no_option(m->parameters_fragment.options, "nofail\0" "fail\0") : false; e = path_startswith(m->where, "/sysroot"); @@ -502,7 +490,34 @@ static int mount_add_default_dependencies(Mount *m) { return r; } - r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_BEFORE, UNIT_CONFLICTS, SPECIAL_UMOUNT_TARGET, true, mask); + return unit_add_two_dependencies_by_name(UNIT(m), UNIT_BEFORE, UNIT_CONFLICTS, + SPECIAL_UMOUNT_TARGET, true, mask); +} + +static int mount_add_default_dependencies(Mount *m) { + UnitDependencyMask mask; + MountParameters *p; + int r; + + assert(m); + + if (!UNIT(m)->default_dependencies) + return 0; + + /* We do not add any default dependencies to /, /usr or /run/initramfs/, since they are + * guaranteed to stay mounted the whole time, since our system is on it. Also, don't + * bother with anything mounted below virtual file systems, it's also going to be virtual, + * and hence not worth the effort. */ + if (mount_is_extrinsic(m)) + return 0; + + p = get_mount_parameters(m); + if (!p) + return 0; + + mask = m->from_fragment ? UNIT_DEPENDENCY_FILE : UNIT_DEPENDENCY_MOUNTINFO_DEFAULT; + + r = mount_add_default_ordering_dependencies(m, p, mask); if (r < 0) return r; From 2ec15c4f8a288d4f2e92ba2b8586736b2a07b9ea Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 2 Apr 2020 08:51:00 +0200 Subject: [PATCH 8/9] mount: default startup dependencies and default network ones are orthogonal Regardless of whether a mount is setup in initrd or int the main system, the network default dependencies _netdev should still be honored. IOW if a mount unit use the following options "x-initrd.mount,_netdev", it should be ordered against initrd-fs.target, network.target, network-online.target. /dev/vdb1 /mnt ext4 x-initrd.mount,_netdev defaults 0 0 Before this patch: Before=umount.target initrd-fs.target After=system.slice sysroot.mount dev-vdb1.device -.mount systemd-journald.socket blockdev@dev-vdb1.target After this patch: Before=initrd-fs.target umount.target After=network-online.target -.mount blockdev@dev-vdb1.target dev-vdb1.device sysroot.mount system.slice network.target systemd-journald.socket --- src/core/mount.c | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 585bfbde10..0f51ac368f 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -450,29 +450,9 @@ static int mount_add_default_ordering_dependencies( before = isempty(e) ? SPECIAL_INITRD_ROOT_FS_TARGET : SPECIAL_INITRD_FS_TARGET; } else if (mount_is_network(p)) { - /* We order ourselves after network.target. This is - * primarily useful at shutdown: services that take - * down the network should order themselves before - * network.target, so that they are shut down only - * after this mount unit is stopped. */ - - r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_NETWORK_TARGET, true, mask); - if (r < 0) - return r; - - /* We pull in network-online.target, and order - * ourselves after it. This is useful at start-up to - * actively pull in tools that want to be started - * before we start mounting network file systems, and - * whose purpose it is to delay this until the network - * is "up". */ - - r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_WANTS, UNIT_AFTER, SPECIAL_NETWORK_ONLINE_TARGET, true, mask); - if (r < 0) - return r; - after = SPECIAL_REMOTE_FS_PRE_TARGET; before = SPECIAL_REMOTE_FS_TARGET; + } else { after = SPECIAL_LOCAL_FS_PRE_TARGET; before = SPECIAL_LOCAL_FS_TARGET; @@ -521,6 +501,26 @@ static int mount_add_default_dependencies(Mount *m) { if (r < 0) return r; + if (mount_is_network(p)) { + /* We order ourselves after network.target. This is primarily useful at shutdown: + * services that take down the network should order themselves before + * network.target, so that they are shut down only after this mount unit is + * stopped. */ + + r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_NETWORK_TARGET, true, mask); + if (r < 0) + return r; + + /* We pull in network-online.target, and order ourselves after it. This is useful + * at start-up to actively pull in tools that want to be started before we start + * mounting network file systems, and whose purpose it is to delay this until the + * network is "up". */ + + r = unit_add_two_dependencies_by_name(UNIT(m), UNIT_WANTS, UNIT_AFTER, SPECIAL_NETWORK_ONLINE_TARGET, true, mask); + if (r < 0) + return r; + } + /* If this is a tmpfs mount then we have to unmount it before we try to deactivate swaps */ if (streq_ptr(p->fstype, "tmpfs")) { r = unit_add_dependency_by_name(UNIT(m), UNIT_AFTER, SPECIAL_SWAP_TARGET, true, mask); From 5a7c4f4f3b3bc8f01fc2fa6ab55ed0b6665508e5 Mon Sep 17 00:00:00 2001 From: Franck Bui Date: Thu, 2 Apr 2020 08:58:31 +0200 Subject: [PATCH 9/9] mount: introduce mount_is_nofail() helper --- src/core/mount.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index 0f51ac368f..38024d1d28 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -86,6 +86,15 @@ static bool mount_is_network(const MountParameters *p) { return false; } +static bool mount_is_nofail(const Mount *m) { + assert(m); + + if (!m->from_fragment) + return false; + + return fstab_test_yes_no_option(m->parameters_fragment.options, "nofail\0" "fail\0"); +} + static bool mount_is_loop(const MountParameters *p) { assert(p); @@ -433,13 +442,10 @@ static int mount_add_default_ordering_dependencies( UnitDependencyMask mask) { const char *after, *before, *e; - bool nofail; int r; assert(m); - nofail = m->from_fragment ? fstab_test_yes_no_option(m->parameters_fragment.options, "nofail\0" "fail\0") : false; - e = path_startswith(m->where, "/sysroot"); if (e && in_initrd()) { /* All mounts under /sysroot need to happen later, at initrd-fs.target time. IOW, @@ -458,7 +464,7 @@ static int mount_add_default_ordering_dependencies( before = SPECIAL_LOCAL_FS_TARGET; } - if (!nofail && !mount_is_automount(p)) { + if (!mount_is_nofail(m) && !mount_is_automount(p)) { r = unit_add_dependency_by_name(UNIT(m), UNIT_BEFORE, before, true, mask); if (r < 0) return r;