diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 908c0cd03f..9bc4920998 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -117,14 +117,33 @@ static const char* const unit_active_state_table[_UNIT_ACTIVE_STATE_MAX] = { DEFINE_STRING_TABLE_LOOKUP(unit_active_state, UnitActiveState); static const char* const freezer_state_table[_FREEZER_STATE_MAX] = { - [FREEZER_RUNNING] = "running", - [FREEZER_FREEZING] = "freezing", - [FREEZER_FROZEN] = "frozen", - [FREEZER_THAWING] = "thawing", + [FREEZER_RUNNING] = "running", + [FREEZER_FREEZING] = "freezing", + [FREEZER_FREEZING_BY_PARENT] = "freezing-by-parent", + [FREEZER_FROZEN] = "frozen", + [FREEZER_FROZEN_BY_PARENT] = "frozen-by-parent", + [FREEZER_THAWING] = "thawing", }; DEFINE_STRING_TABLE_LOOKUP(freezer_state, FreezerState); +/* Maps in-progress freezer states to the corresponding finished state */ +static const FreezerState freezer_state_finish_table[_FREEZER_STATE_MAX] = { + [FREEZER_FREEZING] = FREEZER_FROZEN, + [FREEZER_FREEZING_BY_PARENT] = FREEZER_FROZEN_BY_PARENT, + [FREEZER_THAWING] = FREEZER_RUNNING, + + /* Finished states trivially map to themselves */ + [FREEZER_RUNNING] = FREEZER_RUNNING, + [FREEZER_FROZEN] = FREEZER_FROZEN, + [FREEZER_FROZEN_BY_PARENT] = FREEZER_FROZEN_BY_PARENT, +}; + +FreezerState freezer_state_finish(FreezerState state) { + assert(state >= 0 && state < _FREEZER_STATE_MAX); + return freezer_state_finish_table[state]; +} + static const char* const unit_marker_table[_UNIT_MARKER_MAX] = { [UNIT_MARKER_NEEDS_RELOAD] = "needs-reload", [UNIT_MARKER_NEEDS_RESTART] = "needs-restart", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 6627da5614..8e73e286da 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -53,8 +53,10 @@ typedef enum UnitActiveState { typedef enum FreezerState { FREEZER_RUNNING, - FREEZER_FREEZING, + FREEZER_FREEZING, /* freezing due to user request */ FREEZER_FROZEN, + FREEZER_FREEZING_BY_PARENT, /* freezing as a result of parent slice freezing */ + FREEZER_FROZEN_BY_PARENT, FREEZER_THAWING, _FREEZER_STATE_MAX, _FREEZER_STATE_INVALID = -EINVAL, @@ -297,6 +299,7 @@ UnitActiveState unit_active_state_from_string(const char *s) _pure_; const char *freezer_state_to_string(FreezerState i) _const_; FreezerState freezer_state_from_string(const char *s) _pure_; +FreezerState freezer_state_finish(FreezerState i) _const_; const char *unit_marker_to_string(UnitMarker m) _const_; UnitMarker unit_marker_from_string(const char *s) _pure_; diff --git a/src/core/cgroup.c b/src/core/cgroup.c index bc820b7d14..597bf12f47 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -3897,8 +3897,10 @@ static int unit_check_cgroup_events(Unit *u) { unit_add_to_cgroup_empty_queue(u); } - /* Disregard freezer state changes due to operations not initiated by us */ - if (values[1] && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING)) { + /* Disregard freezer state changes due to operations not initiated by us. + * See: https://github.com/systemd/systemd/pull/13512/files#r416469963 and + * https://github.com/systemd/systemd/pull/13512#issuecomment-573007207 */ + if (values[1] && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT, FREEZER_THAWING)) { if (streq(values[1], "0")) unit_thawed(u); else @@ -4891,66 +4893,87 @@ void manager_invalidate_startup_units(Manager *m) { unit_invalidate_cgroup(u, CGROUP_MASK_CPU|CGROUP_MASK_IO|CGROUP_MASK_BLKIO|CGROUP_MASK_CPUSET); } -int unit_cgroup_freezer_action(Unit *u, FreezerAction action) { - _cleanup_free_ char *path = NULL; - FreezerState target, kernel = _FREEZER_STATE_INVALID; - int r, ret; +static int unit_cgroup_freezer_kernel_state(Unit *u, FreezerState *ret) { + _cleanup_free_ char *val = NULL; + FreezerState s; + int r; assert(u); - assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); + assert(ret); - if (!cg_freezer_supported()) - return 0; + r = cg_get_keyed_attribute(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.events", + STRV_MAKE("frozen"), &val); + if (IN_SET(r, -ENOENT, -ENXIO)) + return -ENODATA; + if (r < 0) + return r; - /* Ignore all requests to thaw init.scope or -.slice and reject all requests to freeze them */ - if (unit_has_name(u, SPECIAL_ROOT_SLICE) || unit_has_name(u, SPECIAL_INIT_SCOPE)) - return action == FREEZER_FREEZE ? -EPERM : 0; - - if (!u->cgroup_realized) - return -EBUSY; - - if (action == FREEZER_THAW) { - Unit *slice = UNIT_GET_SLICE(u); - - if (slice) { - r = unit_cgroup_freezer_action(slice, FREEZER_THAW); - if (r < 0) - return log_unit_error_errno(u, r, "Failed to thaw slice %s of unit: %m", slice->id); - } + if (streq(val, "0")) + s = FREEZER_RUNNING; + else if (streq(val, "1")) + s = FREEZER_FROZEN; + else { + log_unit_debug_errno(u, SYNTHETIC_ERRNO(EINVAL), "Unexpected cgroup frozen state: %s", val); + s = _FREEZER_STATE_INVALID; } - target = action == FREEZER_FREEZE ? FREEZER_FROZEN : FREEZER_RUNNING; + *ret = s; + return 0; +} - r = unit_freezer_state_kernel(u, &kernel); +int unit_cgroup_freezer_action(Unit *u, FreezerAction action) { + _cleanup_free_ char *path = NULL; + FreezerState target, current, next; + int r; + + assert(u); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); + + if (!cg_freezer_supported() || !u->cgroup_realized) + return 0; + + unit_next_freezer_state(u, action, &next, &target); + + r = unit_cgroup_freezer_kernel_state(u, ¤t); if (r < 0) - log_unit_debug_errno(u, r, "Failed to obtain cgroup freezer state: %m"); + return r; - if (target == kernel) { - u->freezer_state = target; - if (action == FREEZER_FREEZE) - return 0; - ret = 0; - } else - ret = 1; + if (current == target) + next = freezer_state_finish(next); + else if (IN_SET(next, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT, FREEZER_RUNNING)) { + /* We're transitioning into a finished state, which implies that the cgroup's + * current state already matches the target and thus we'd return 0. But, reality + * shows otherwise. This indicates that our freezer_state tracking has diverged + * from the real state of the cgroup, which can happen if someone meddles with the + * cgroup from underneath us. This really shouldn't happen during normal operation, + * though. So, let's warn about it and fix up the state to be valid */ + + log_unit_warning(u, "Unit wants to transition to %s freezer state but cgroup is unexpectedly %s, fixing up.", + freezer_state_to_string(next), freezer_state_to_string(current) ?: "(invalid)"); + + if (next == FREEZER_FROZEN) + next = FREEZER_FREEZING; + else if (next == FREEZER_FROZEN_BY_PARENT) + next = FREEZER_FREEZING_BY_PARENT; + else if (next == FREEZER_RUNNING) + next = FREEZER_THAWING; + } r = cg_get_path(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.freeze", &path); if (r < 0) return r; - log_unit_debug(u, "%s unit.", action == FREEZER_FREEZE ? "Freezing" : "Thawing"); + log_unit_debug(u, "Unit freezer state was %s, now %s.", + freezer_state_to_string(u->freezer_state), + freezer_state_to_string(next)); - if (target != kernel) { - if (action == FREEZER_FREEZE) - u->freezer_state = FREEZER_FREEZING; - else - u->freezer_state = FREEZER_THAWING; - } - - r = write_string_file(path, one_zero(action == FREEZER_FREEZE), WRITE_STRING_FILE_DISABLE_BUFFER); + r = write_string_file(path, one_zero(target == FREEZER_FROZEN), WRITE_STRING_FILE_DISABLE_BUFFER); if (r < 0) return r; - return ret; + u->freezer_state = next; + return target != current; } int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name) { @@ -4989,17 +5012,10 @@ static const char* const cgroup_device_policy_table[_CGROUP_DEVICE_POLICY_MAX] = DEFINE_STRING_TABLE_LOOKUP(cgroup_device_policy, CGroupDevicePolicy); -static const char* const freezer_action_table[_FREEZER_ACTION_MAX] = { - [FREEZER_FREEZE] = "freeze", - [FREEZER_THAW] = "thaw", -}; - -DEFINE_STRING_TABLE_LOOKUP(freezer_action, FreezerAction); - static const char* const cgroup_pressure_watch_table[_CGROUP_PRESSURE_WATCH_MAX] = { - [CGROUP_PRESSURE_WATCH_OFF] = "off", + [CGROUP_PRESSURE_WATCH_OFF] = "off", [CGROUP_PRESSURE_WATCH_AUTO] = "auto", - [CGROUP_PRESSURE_WATCH_ON] = "on", + [CGROUP_PRESSURE_WATCH_ON] = "on", [CGROUP_PRESSURE_WATCH_SKIP] = "skip", }; diff --git a/src/core/cgroup.h b/src/core/cgroup.h index d834cb8bbd..3ea3879bbc 100644 --- a/src/core/cgroup.h +++ b/src/core/cgroup.h @@ -53,7 +53,9 @@ typedef enum CGroupDevicePolicy { typedef enum FreezerAction { FREEZER_FREEZE, + FREEZER_PARENT_FREEZE, FREEZER_THAW, + FREEZER_PARENT_THAW, _FREEZER_ACTION_MAX, _FREEZER_ACTION_INVALID = -EINVAL, @@ -433,9 +435,6 @@ bool unit_cgroup_delegate(Unit *u); int unit_get_cpuset(Unit *u, CPUSet *cpus, const char *name); int unit_cgroup_freezer_action(Unit *u, FreezerAction action); -const char* freezer_action_to_string(FreezerAction a) _const_; -FreezerAction freezer_action_from_string(const char *s) _pure_; - const char* cgroup_pressure_watch_to_string(CGroupPressureWatch a) _const_; CGroupPressureWatch cgroup_pressure_watch_from_string(const char *s) _pure_; diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 4b017bff48..11a52a2535 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -724,7 +724,6 @@ int bus_unit_method_clean(sd_bus_message *message, void *userdata, sd_bus_error static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userdata, sd_bus_error *error, FreezerAction action) { const char* perm; - int (*method)(Unit*); Unit *u = ASSERT_PTR(userdata); bool reply_no_delay = false; int r; @@ -732,13 +731,7 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda assert(message); assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); - if (action == FREEZER_FREEZE) { - perm = "stop"; - method = unit_freeze; - } else { - perm = "start"; - method = unit_thaw; - } + perm = action == FREEZER_FREEZE ? "stop" : "start"; r = mac_selinux_unit_access_check(u, message, perm, error); if (r < 0) @@ -755,15 +748,17 @@ static int bus_unit_method_freezer_generic(sd_bus_message *message, void *userda if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = method(u); + r = unit_freezer_action(u, action); if (r == -EOPNOTSUPP) - return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit '%s' does not support freezing.", u->id); + return sd_bus_error_setf(error, SD_BUS_ERROR_NOT_SUPPORTED, "Unit does not support freeze/thaw."); if (r == -EBUSY) return sd_bus_error_set(error, BUS_ERROR_UNIT_BUSY, "Unit has a pending job."); if (r == -EHOSTDOWN) return sd_bus_error_set(error, BUS_ERROR_UNIT_INACTIVE, "Unit is inactive."); if (r == -EALREADY) - return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Previously requested freezer operation for unit '%s' is still in progress.", u->id); + return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Previously requested freezer operation for unit is still in progress."); + if (r == -ECHILD) + return sd_bus_error_setf(error, SD_BUS_ERROR_FAILED, "Unit is frozen by a parent slice."); if (r < 0) return r; if (r == 0) diff --git a/src/core/job.c b/src/core/job.c index e78c2a70db..595fbce93a 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -633,16 +633,19 @@ static const char* job_done_message_format(Unit *u, JobType t, JobResult result) [JOB_UNSUPPORTED] = "Starting of %s unsupported.", [JOB_COLLECTED] = "Unnecessary job was removed for %s.", [JOB_ONCE] = "Unit %s has been started before and cannot be started again.", + [JOB_FROZEN] = "Cannot start frozen unit %s.", }; static const char* const generic_finished_stop_job[_JOB_RESULT_MAX] = { [JOB_DONE] = "Stopped %s.", [JOB_FAILED] = "Stopped %s with error.", [JOB_TIMEOUT] = "Timed out stopping %s.", + [JOB_FROZEN] = "Cannot stop frozen unit %s.", }; static const char* const generic_finished_reload_job[_JOB_RESULT_MAX] = { [JOB_DONE] = "Reloaded %s.", [JOB_FAILED] = "Reload failed for %s.", [JOB_TIMEOUT] = "Timed out reloading %s.", + [JOB_FROZEN] = "Cannot reload frozen unit %s.", }; /* When verify-active detects the unit is inactive, report it. * Most likely a DEPEND warning from a requisiting unit will @@ -704,6 +707,7 @@ static const struct { [JOB_UNSUPPORTED] = { LOG_WARNING, ANSI_HIGHLIGHT_YELLOW, "UNSUPP" }, [JOB_COLLECTED] = { LOG_INFO, }, [JOB_ONCE] = { LOG_ERR, ANSI_HIGHLIGHT_RED, " ONCE " }, + [JOB_FROZEN] = { LOG_ERR, ANSI_HIGHLIGHT_RED, "FROZEN" }, }; static const char* job_done_mid(JobType type, JobResult result) { @@ -954,6 +958,8 @@ int job_run_and_invalidate(Job *j) { r = job_finish_and_invalidate(j, JOB_DEPENDENCY, true, false); else if (r == -ESTALE) r = job_finish_and_invalidate(j, JOB_ONCE, true, false); + else if (r == -EDEADLK) + r = job_finish_and_invalidate(j, JOB_FROZEN, true, false); else if (r < 0) r = job_finish_and_invalidate(j, JOB_FAILED, true, false); } @@ -1011,7 +1017,7 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr goto finish; } - if (IN_SET(result, JOB_FAILED, JOB_INVALID)) + if (IN_SET(result, JOB_FAILED, JOB_INVALID, JOB_FROZEN)) j->manager->n_failed_jobs++; job_uninstall(j); @@ -1645,6 +1651,7 @@ static const char* const job_result_table[_JOB_RESULT_MAX] = { [JOB_UNSUPPORTED] = "unsupported", [JOB_COLLECTED] = "collected", [JOB_ONCE] = "once", + [JOB_FROZEN] = "frozen", }; DEFINE_STRING_TABLE_LOOKUP(job_result, JobResult); diff --git a/src/core/job.h b/src/core/job.h index 891d87a79c..8318b524b4 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -96,6 +96,7 @@ enum JobResult { JOB_UNSUPPORTED, /* Couldn't start a unit, because the unit type is not supported on the system */ JOB_COLLECTED, /* Job was garbage collected, since nothing needed it anymore */ JOB_ONCE, /* Unit was started before, and hence can't be started again */ + JOB_FROZEN, /* Unit is currently frozen, so we can't safely operate on it */ _JOB_RESULT_MAX, _JOB_RESULT_INVALID = -EINVAL, }; diff --git a/src/core/scope.c b/src/core/scope.c index 8cbbf25801..3f950e0c9e 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -800,8 +800,7 @@ const UnitVTable scope_vtable = { .start = scope_start, .stop = scope_stop, - .freeze = unit_freeze_vtable_common, - .thaw = unit_thaw_vtable_common, + .freezer_action = unit_cgroup_freezer_action, .get_timeout = scope_get_timeout, diff --git a/src/core/service.c b/src/core/service.c index 13a9275f3a..2d5c43bca5 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -5146,8 +5146,7 @@ const UnitVTable service_vtable = { .clean = service_clean, .can_clean = service_can_clean, - .freeze = unit_freeze_vtable_common, - .thaw = unit_thaw_vtable_common, + .freezer_action = unit_cgroup_freezer_action, .serialize = service_serialize, .deserialize_item = service_deserialize_item, diff --git a/src/core/slice.c b/src/core/slice.c index fb4f23c106..e9b2683912 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -347,46 +347,47 @@ static void slice_enumerate_perpetual(Manager *m) { (void) slice_make_perpetual(m, SPECIAL_SYSTEM_SLICE, NULL); } -static bool slice_freezer_action_supported_by_children(Unit *s) { +static bool slice_can_freeze(Unit *s) { Unit *member; assert(s); - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { - - if (member->type == UNIT_SLICE && - !slice_freezer_action_supported_by_children(member)) + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) + if (!unit_can_freeze(member)) return false; - - if (!UNIT_VTABLE(member)->freeze) - return false; - } - return true; } static int slice_freezer_action(Unit *s, FreezerAction action) { + FreezerAction child_action; Unit *member; int r; assert(s); - assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); - if (action == FREEZER_FREEZE && !slice_freezer_action_supported_by_children(s)) { + if (action == FREEZER_FREEZE && !slice_can_freeze(s)) { + /* We're intentionally only checking for FREEZER_FREEZE here and ignoring the + * _BY_PARENT variant. If we're being frozen by parent, that means someone has + * already checked if we can be frozen further up the call stack. No point to + * redo that work */ log_unit_warning(s, "Requested freezer operation is not supported by all children of the slice"); return 0; } - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { - if (!member->cgroup_realized) - continue; + if (action == FREEZER_FREEZE) + child_action = FREEZER_PARENT_FREEZE; + else if (action == FREEZER_THAW) + child_action = FREEZER_PARENT_THAW; + else + child_action = action; - if (action == FREEZER_FREEZE) - r = UNIT_VTABLE(member)->freeze(member); - else if (UNIT_VTABLE(member)->thaw) - r = UNIT_VTABLE(member)->thaw(member); + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { + if (UNIT_VTABLE(member)->freezer_action) + r = UNIT_VTABLE(member)->freezer_action(member, child_action); else - /* Thawing is requested but no corresponding method is available, ignore. */ + /* Only thawing will reach here, since freezing checks for a method in can_freeze */ r = 0; if (r < 0) return r; @@ -395,24 +396,6 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { return unit_cgroup_freezer_action(s, action); } -static int slice_freeze(Unit *s) { - assert(s); - - return slice_freezer_action(s, FREEZER_FREEZE); -} - -static int slice_thaw(Unit *s) { - assert(s); - - return slice_freezer_action(s, FREEZER_THAW); -} - -static bool slice_can_freeze(Unit *s) { - assert(s); - - return slice_freezer_action_supported_by_children(s); -} - const UnitVTable slice_vtable = { .object_size = sizeof(Slice), .cgroup_context_offset = offsetof(Slice, cgroup_context), @@ -436,8 +419,7 @@ const UnitVTable slice_vtable = { .start = slice_start, .stop = slice_stop, - .freeze = slice_freeze, - .thaw = slice_thaw, + .freezer_action = slice_freezer_action, .can_freeze = slice_can_freeze, .serialize = slice_serialize, diff --git a/src/core/unit.c b/src/core/unit.c index 85021da85a..2998ed9ba5 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -905,32 +905,6 @@ FreezerState unit_freezer_state(Unit *u) { return u->freezer_state; } -int unit_freezer_state_kernel(Unit *u, FreezerState *ret) { - char *values[1] = {}; - int r; - - assert(u); - - r = cg_get_keyed_attribute(SYSTEMD_CGROUP_CONTROLLER, u->cgroup_path, "cgroup.events", - STRV_MAKE("frozen"), values); - if (r < 0) - return r; - - r = _FREEZER_STATE_INVALID; - - if (values[0]) { - if (streq(values[0], "0")) - r = FREEZER_RUNNING; - else if (streq(values[0], "1")) - r = FREEZER_FROZEN; - } - - free(values[0]); - *ret = r; - - return 0; -} - UnitActiveState unit_active_state(Unit *u) { assert(u); @@ -1982,6 +1956,10 @@ int unit_start(Unit *u, ActivationDetails *details) { return unit_start(following, details); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + /* Check our ability to start early so that failure conditions don't cause us to enter a busy loop. */ if (UNIT_VTABLE(u)->can_start) { r = UNIT_VTABLE(u)->can_start(u); @@ -1998,7 +1976,6 @@ int unit_start(Unit *u, ActivationDetails *details) { * waits for a holdoff timer to elapse before it will start again. */ unit_add_to_dbus_queue(u); - unit_cgroup_freezer_action(u, FREEZER_THAW); if (!u->activation_details) /* Older details object wins */ u->activation_details = activation_details_ref(details); @@ -2033,6 +2010,7 @@ bool unit_can_isolate(Unit *u) { * -EBADR: This unit type does not support stopping. * -EALREADY: Unit is already stopped. * -EAGAIN: An operation is already in progress. Retry later. + * -EDEADLK: Unit is frozen */ int unit_stop(Unit *u) { UnitActiveState state; @@ -2050,11 +2028,14 @@ int unit_stop(Unit *u) { return unit_stop(following); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + if (!UNIT_VTABLE(u)->stop) return -EBADR; unit_add_to_dbus_queue(u); - unit_cgroup_freezer_action(u, FREEZER_THAW); return UNIT_VTABLE(u)->stop(u); } @@ -2079,6 +2060,7 @@ bool unit_can_stop(Unit *u) { * -EBADR: This unit type does not support reloading. * -ENOEXEC: Unit is not started. * -EAGAIN: An operation is already in progress. Retry later. + * -EDEADLK: Unit is frozen. */ int unit_reload(Unit *u) { UnitActiveState state; @@ -2105,6 +2087,10 @@ int unit_reload(Unit *u) { return unit_reload(following); } + /* Check to make sure the unit isn't frozen */ + if (u->freezer_state != FREEZER_RUNNING) + return -EDEADLK; + unit_add_to_dbus_queue(u); if (!UNIT_VTABLE(u)->reload) { @@ -2113,8 +2099,6 @@ int unit_reload(Unit *u) { return 0; } - unit_cgroup_freezer_action(u, FREEZER_THAW); - return UNIT_VTABLE(u)->reload(u); } @@ -6155,19 +6139,98 @@ bool unit_can_isolate_refuse_manual(Unit *u) { return unit_can_isolate(u) && !u->refuse_manual_start; } +void unit_next_freezer_state(Unit *u, FreezerAction action, FreezerState *ret, FreezerState *ret_target) { + Unit *slice; + FreezerState curr, parent, next, tgt; + + assert(u); + assert(IN_SET(action, FREEZER_FREEZE, FREEZER_PARENT_FREEZE, + FREEZER_THAW, FREEZER_PARENT_THAW)); + assert(ret); + assert(ret_target); + + /* This function determintes the correct freezer state transitions for a unit + * given the action being requested. It returns the next state, and also the "target", + * which is either FREEZER_FROZEN or FREEZER_RUNNING, depending on what actual state we + * ultimately want to achieve. */ + + curr = u->freezer_state; + slice = UNIT_GET_SLICE(u); + if (slice) + parent = slice->freezer_state; + else + parent = FREEZER_RUNNING; + + if (action == FREEZER_FREEZE) { + /* We always "promote" a freeze initiated by parent into a normal freeze */ + if (IN_SET(curr, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT)) + next = FREEZER_FROZEN; + else + next = FREEZER_FREEZING; + } else if (action == FREEZER_THAW) { + /* Thawing is the most complicated operation here, because we can't thaw a unit + * if its parent is frozen. So we instead "demote" a normal freeze into a freeze + * initiated by parent if the parent is frozen */ + if (IN_SET(curr, FREEZER_RUNNING, FREEZER_THAWING, FREEZER_FREEZING_BY_PARENT, FREEZER_FROZEN_BY_PARENT)) + next = curr; + else if (curr == FREEZER_FREEZING) { + if (IN_SET(parent, FREEZER_RUNNING, FREEZER_THAWING)) + next = FREEZER_THAWING; + else + next = FREEZER_FREEZING_BY_PARENT; + } else { + assert(curr == FREEZER_FROZEN); + if (IN_SET(parent, FREEZER_RUNNING, FREEZER_THAWING)) + next = FREEZER_THAWING; + else + next = FREEZER_FROZEN_BY_PARENT; + } + } else if (action == FREEZER_PARENT_FREEZE) { + /* We need to avoid accidentally demoting units frozen manually */ + if (IN_SET(curr, FREEZER_FREEZING, FREEZER_FROZEN, FREEZER_FROZEN_BY_PARENT)) + next = curr; + else + next = FREEZER_FREEZING_BY_PARENT; + } else { + assert(action == FREEZER_PARENT_THAW); + + /* We don't want to thaw units from a parent if they were frozen + * manually, so for such units this action is a no-op */ + if (IN_SET(curr, FREEZER_RUNNING, FREEZER_FREEZING, FREEZER_FROZEN)) + next = curr; + else + next = FREEZER_THAWING; + } + + tgt = freezer_state_finish(next); + if (tgt == FREEZER_FROZEN_BY_PARENT) + tgt = FREEZER_FROZEN; + assert(IN_SET(tgt, FREEZER_RUNNING, FREEZER_FROZEN)); + + *ret = next; + *ret_target = tgt; +} + bool unit_can_freeze(Unit *u) { assert(u); + if (unit_has_name(u, SPECIAL_ROOT_SLICE) || unit_has_name(u, SPECIAL_INIT_SCOPE)) + return false; + if (UNIT_VTABLE(u)->can_freeze) return UNIT_VTABLE(u)->can_freeze(u); - return UNIT_VTABLE(u)->freeze; + return UNIT_VTABLE(u)->freezer_action; } void unit_frozen(Unit *u) { assert(u); - u->freezer_state = FREEZER_FROZEN; + u->freezer_state = u->freezer_state == FREEZER_FREEZING_BY_PARENT + ? FREEZER_FROZEN_BY_PARENT + : FREEZER_FROZEN; + + log_unit_debug(u, "Unit now %s.", freezer_state_to_string(u->freezer_state)); bus_unit_send_pending_freezer_message(u, false); } @@ -6177,19 +6240,19 @@ void unit_thawed(Unit *u) { u->freezer_state = FREEZER_RUNNING; + log_unit_debug(u, "Unit thawed."); + bus_unit_send_pending_freezer_message(u, false); } -static int unit_freezer_action(Unit *u, FreezerAction action) { +int unit_freezer_action(Unit *u, FreezerAction action) { UnitActiveState s; - int (*method)(Unit*); int r; assert(u); assert(IN_SET(action, FREEZER_FREEZE, FREEZER_THAW)); - method = action == FREEZER_FREEZE ? UNIT_VTABLE(u)->freeze : UNIT_VTABLE(u)->thaw; - if (!method || !cg_freezer_supported()) + if (!cg_freezer_supported() || !unit_can_freeze(u)) return -EOPNOTSUPP; if (u->job) @@ -6202,36 +6265,21 @@ static int unit_freezer_action(Unit *u, FreezerAction action) { if (s != UNIT_ACTIVE) return -EHOSTDOWN; - if ((IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING) && action == FREEZER_FREEZE) || - (u->freezer_state == FREEZER_THAWING && action == FREEZER_THAW)) + if (action == FREEZER_FREEZE && IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT)) return -EALREADY; + if (action == FREEZER_THAW && u->freezer_state == FREEZER_THAWING) + return -EALREADY; + if (action == FREEZER_THAW && IN_SET(u->freezer_state, FREEZER_FREEZING_BY_PARENT, FREEZER_FROZEN_BY_PARENT)) + return -ECHILD; - r = method(u); + r = UNIT_VTABLE(u)->freezer_action(u, action); if (r <= 0) return r; - assert(IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_THAWING)); - + assert(IN_SET(u->freezer_state, FREEZER_FREEZING, FREEZER_FREEZING_BY_PARENT, FREEZER_THAWING)); return 1; } -int unit_freeze(Unit *u) { - return unit_freezer_action(u, FREEZER_FREEZE); -} - -int unit_thaw(Unit *u) { - return unit_freezer_action(u, FREEZER_THAW); -} - -/* Wrappers around low-level cgroup freezer operations common for service and scope units */ -int unit_freeze_vtable_common(Unit *u) { - return unit_cgroup_freezer_action(u, FREEZER_FREEZE); -} - -int unit_thaw_vtable_common(Unit *u) { - return unit_cgroup_freezer_action(u, FREEZER_THAW); -} - Condition *unit_find_failed_condition(Unit *u) { Condition *failed_trigger = NULL; bool has_succeeded_trigger = false; diff --git a/src/core/unit.h b/src/core/unit.h index 372791f77e..15103efb64 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -642,9 +642,9 @@ typedef struct UnitVTable { /* Clear out the various runtime/state/cache/logs/configuration data */ int (*clean)(Unit *u, ExecCleanMask m); - /* Freeze the unit */ - int (*freeze)(Unit *u); - int (*thaw)(Unit *u); + /* Freeze or thaw the unit. Returns > 0 to indicate that the request will be handled asynchronously; unit_frozen + * or unit_thawed should be called once the operation is done. Returns 0 if done successfully, or < 0 on error. */ + int (*freezer_action)(Unit *u, FreezerAction a); bool (*can_freeze)(Unit *u); /* Return which kind of data can be cleaned */ @@ -912,7 +912,6 @@ bool unit_has_name(const Unit *u, const char *name); UnitActiveState unit_active_state(Unit *u); FreezerState unit_freezer_state(Unit *u); -int unit_freezer_state_kernel(Unit *u, FreezerState *ret); const char* unit_sub_state_to_string(Unit *u); @@ -1099,15 +1098,11 @@ bool unit_can_stop_refuse_manual(Unit *u); bool unit_can_isolate_refuse_manual(Unit *u); bool unit_can_freeze(Unit *u); -int unit_freeze(Unit *u); +int unit_freezer_action(Unit *u, FreezerAction action); +void unit_next_freezer_state(Unit *u, FreezerAction a, FreezerState *ret, FreezerState *ret_tgt); void unit_frozen(Unit *u); - -int unit_thaw(Unit *u); void unit_thawed(Unit *u); -int unit_freeze_vtable_common(Unit *u); -int unit_thaw_vtable_common(Unit *u); - Condition *unit_find_failed_condition(Unit *u); int unit_arm_timer(Unit *u, sd_event_source **source, bool relative, usec_t usec, sd_event_time_handler_t handler); diff --git a/src/shared/bus-wait-for-jobs.c b/src/shared/bus-wait-for-jobs.c index 70f2043347..2930552be4 100644 --- a/src/shared/bus-wait-for-jobs.c +++ b/src/shared/bus-wait-for-jobs.c @@ -248,6 +248,8 @@ static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const cha log_error("Queued job for %s was garbage collected.", d->name); else if (streq(d->result, "once")) log_error("Unit %s was started already once and can't be started again.", d->name); + else if (streq(d->result, "frozen")) + log_error("Cannot perform operation on frozen unit %s.", d->name); else if (endswith(d->name, ".service")) { /* Job result is unknown. For services, let's also try Result property. */ _cleanup_free_ char *result = NULL; @@ -276,6 +278,8 @@ static int check_wait_response(BusWaitForJobs *d, WaitJobsFlags flags, const cha return -EOPNOTSUPP; else if (streq(d->result, "once")) return -ESTALE; + else if (streq(d->result, "frozen")) + return -EDEADLK; return log_debug_errno(SYNTHETIC_ERRNO(ENOMEDIUM), "Unexpected job result '%s' for unit '%s', assuming server side newer than us.", diff --git a/test/units/testsuite-38.sh b/test/units/testsuite-38.sh index 5fc87fca7b..e557fa1dbf 100755 --- a/test/units/testsuite-38.sh +++ b/test/units/testsuite-38.sh @@ -105,7 +105,14 @@ check_freezer_state() { } check_cgroup_state() { - grep -q "frozen $2" /sys/fs/cgroup/system.slice/"$1"/cgroup.events + # foo.unit -> /system.slice/foo.unit/ + # foo.slice/ -> /foo.slice/./ + # foo.slice/foo.unit -> /foo.slice/foo.unit/ + local slice unit + unit="${1##*/}" + slice="${1%"$unit"}" + slice="${slice%/}" + grep -q "frozen $2" /sys/fs/cgroup/"${slice:-system.slice}"/"${unit:-.}"/cgroup.events } testcase_dbus_api() { @@ -142,31 +149,6 @@ testcase_dbus_api() { echo } -testcase_jobs() { - local pid_before= - local pid_after= - echo "Test that it is possible to apply jobs on frozen units:" - - systemctl start "${unit}" - dbus_freeze "${unit}" - check_freezer_state "${unit}" "frozen" - - echo -n " - restart: " - pid_before=$(systemctl show -p MainPID "${unit}" --value) - systemctl restart "${unit}" - pid_after=$(systemctl show -p MainPID "${unit}" --value) - [ "$pid_before" != "$pid_after" ] && echo "[ OK ]" - - dbus_freeze "${unit}" - check_freezer_state "${unit}" "frozen" - - echo -n " - stop: " - timeout 5s systemctl stop "${unit}" - echo "[ OK ]" - - echo -} - testcase_systemctl() { echo "Test that systemctl freeze/thaw verbs:" @@ -224,22 +206,101 @@ testcase_recursive() { echo "Test recursive freezing:" - echo -n " - freeze: " + echo -n " - freeze/thaw parent: " systemctl freeze "$slice" - check_freezer_state "${slice}" "frozen" - check_freezer_state "${unit}" "frozen" - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 echo "[ OK ]" - echo -n " - thaw: " + echo -n " - child freeze/thaw during frozen parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 systemctl thaw "$slice" - check_freezer_state "${unit}" "running" - check_freezer_state "${slice}" "running" - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 echo "[ OK ]" + echo -n " - pre-frozen child not thawed by parent: " + systemctl freeze "$unit" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 + echo "[ OK ]" + + echo -n " - pre-frozen child demoted and thawed by parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 + echo "[ OK ]" + + echo -n " - child promoted and not thawed by parent: " + systemctl freeze "$slice" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen-by-parent" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl freeze "$unit" + check_freezer_state "$slice" "frozen" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 + systemctl thaw "$slice" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 1 + echo "[ OK ]" + + echo -n " - can't stop a frozen unit: " + (! systemctl -q stop "$unit" ) + echo "[ OK ]" + systemctl thaw "$unit" + systemctl stop "$unit" systemctl stop "$slice" @@ -255,38 +316,39 @@ testcase_preserve_state() { echo "Test that freezer state is preserved when recursive freezing is initiated from outside (e.g. by manager up the tree):" echo -n " - freeze from outside: " - echo 1 >/sys/fs/cgroup/"${slice}"/cgroup.freeze + echo 1 >/sys/fs/cgroup/"$slice"/cgroup.freeze # Give kernel some time to freeze the slice sleep 1 # Our state should not be affected - check_freezer_state "${slice}" "running" - check_freezer_state "${unit}" "running" + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "running" # However actual kernel state should be frozen - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 1" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_cgroup_state "$slice/" 1 + check_cgroup_state "$slice/$unit" 1 echo "[ OK ]" echo -n " - thaw from outside: " - echo 0 >/sys/fs/cgroup/"${slice}"/cgroup.freeze + echo 0 >/sys/fs/cgroup/"$slice"/cgroup.freeze sleep 1 - check_freezer_state "${unit}" "running" - check_freezer_state "${slice}" "running" - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/cgroup.events - grep -q "frozen 0" /sys/fs/cgroup/"${slice}"/"${unit}"/cgroup.events + check_freezer_state "$unit" "running" + check_freezer_state "$slice" "running" + check_cgroup_state "$slice/" 0 + check_cgroup_state "$slice/$unit" 0 echo "[ OK ]" echo -n " - thaw from outside while inner service is frozen: " systemctl freeze "$unit" - check_freezer_state "${unit}" "frozen" - echo 1 >/sys/fs/cgroup/"${slice}"/cgroup.freeze - echo 0 >/sys/fs/cgroup/"${slice}"/cgroup.freeze - check_freezer_state "${slice}" "running" - check_freezer_state "${unit}" "frozen" + check_freezer_state "$unit" "frozen" + echo 1 >/sys/fs/cgroup/"$slice"/cgroup.freeze + echo 0 >/sys/fs/cgroup/"$slice"/cgroup.freeze + check_freezer_state "$slice" "running" + check_freezer_state "$unit" "frozen" echo "[ OK ]" + systemctl thaw "$unit" systemctl stop "$unit" systemctl stop "$slice"