2f7d91
From c7c8a7648623a25c00dc585bd42b9037a20396c8 Mon Sep 17 00:00:00 2001
2f7d91
From: Lennart Poettering <lennart@poettering.net>
2f7d91
Date: Thu, 9 Aug 2018 16:26:27 +0200
2f7d91
Subject: [PATCH] core: rework StopWhenUnneeded= logic
2f7d91
2f7d91
Previously, we'd act immediately on StopWhenUnneeded= when a unit state
2f7d91
changes. With this rework we'll maintain a queue instead: whenever
2f7d91
there's the chance that StopWhenUneeded= might have an effect we enqueue
2f7d91
the unit, and process it later when we have nothing better to do.
2f7d91
2f7d91
This should make the implementation a bit more reliable, as the unit notify event
2f7d91
cannot immediately enqueue tons of side-effect jobs that might
2f7d91
contradict each other, but we do so only in a strictly ordered fashion,
2f7d91
from the main event loop.
2f7d91
2f7d91
This slightly changes the check when to consider a unit "unneeded".
2f7d91
Previously, we'd assume that a unit in "deactivating" state could also
2f7d91
be cleaned up. With this new logic we'll only consider units unneeded
2f7d91
that are fully up and have no job queued. This means that whenever
2f7d91
there's something pending for a unit we won't clean it up.
2f7d91
2f7d91
(cherry picked from commit a3c1168ac293f16d9343d248795bb4c246aaff4a)
2f7d91
(cherry picked from commit 65ebf5e453846e29ab5894670a83b5d8b942c858)
2f7d91
2f7d91
Resolves: #1810576
2f7d91
---
2f7d91
 src/core/manager.c |  43 +++++++++++++++
2f7d91
 src/core/manager.h |   3 ++
2f7d91
 src/core/unit.c    | 132 ++++++++++++++++++++++++---------------------
2f7d91
 src/core/unit.h    |   7 +++
2f7d91
 4 files changed, 124 insertions(+), 61 deletions(-)
2f7d91
2f7d91
diff --git a/src/core/manager.c b/src/core/manager.c
2f7d91
index 4c87ad8a2f..f0553b4df9 100644
2f7d91
--- a/src/core/manager.c
2f7d91
+++ b/src/core/manager.c
2f7d91
@@ -961,6 +961,45 @@ static unsigned manager_dispatch_gc_queue(Manager *m) {
2f7d91
         return n;
2f7d91
 }
2f7d91
 
2f7d91
+static unsigned manager_dispatch_stop_when_unneeded_queue(Manager *m) {
2f7d91
+        unsigned n = 0;
2f7d91
+        Unit *u;
2f7d91
+        int r;
2f7d91
+
2f7d91
+        assert(m);
2f7d91
+
2f7d91
+        while ((u = m->stop_when_unneeded_queue)) {
2f7d91
+                _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
2f7d91
+                assert(m->stop_when_unneeded_queue);
2f7d91
+
2f7d91
+                assert(u->in_stop_when_unneeded_queue);
2f7d91
+                LIST_REMOVE(stop_when_unneeded_queue, m->stop_when_unneeded_queue, u);
2f7d91
+                u->in_stop_when_unneeded_queue = false;
2f7d91
+
2f7d91
+                n++;
2f7d91
+
2f7d91
+                if (!unit_is_unneeded(u))
2f7d91
+                        continue;
2f7d91
+
2f7d91
+                log_unit_debug(u->id, "Unit is not needed anymore.");
2f7d91
+
2f7d91
+                /* If stopping a unit fails continuously we might enter a stop loop here, hence stop acting on the
2f7d91
+                 * service being unnecessary after a while. */
2f7d91
+
2f7d91
+                if (!ratelimit_test(&u->check_unneeded_ratelimit)) {
2f7d91
+                        log_unit_warning(u->id, "Unit not needed anymore, but not stopping since we tried this too often recently.");
2f7d91
+                        continue;
2f7d91
+                }
2f7d91
+
2f7d91
+                /* Ok, nobody needs us anymore. Sniff. Then let's commit suicide */
2f7d91
+                r = manager_add_job(u->manager, JOB_STOP, u, JOB_FAIL, true, &error, NULL);
2f7d91
+                if (r < 0)
2f7d91
+                        log_unit_warning_errno(u->id, r, "Failed to enqueue stop job, ignoring: %s", bus_error_message(&error, r));
2f7d91
+        }
2f7d91
+
2f7d91
+        return n;
2f7d91
+}
2f7d91
+
2f7d91
 static void manager_clear_jobs_and_units(Manager *m) {
2f7d91
         Unit *u;
2f7d91
 
2f7d91
@@ -977,6 +1016,7 @@ static void manager_clear_jobs_and_units(Manager *m) {
2f7d91
         assert(!m->dbus_job_queue);
2f7d91
         assert(!m->cleanup_queue);
2f7d91
         assert(!m->gc_queue);
2f7d91
+        assert(!m->stop_when_unneeded_queue);
2f7d91
 
2f7d91
         assert(hashmap_isempty(m->jobs));
2f7d91
         assert(hashmap_isempty(m->units));
2f7d91
@@ -2259,6 +2299,9 @@ int manager_loop(Manager *m) {
2f7d91
                 if (manager_dispatch_cgroup_queue(m) > 0)
2f7d91
                         continue;
2f7d91
 
2f7d91
+                if (manager_dispatch_stop_when_unneeded_queue(m) > 0)
2f7d91
+                        continue;
2f7d91
+
2f7d91
                 if (manager_dispatch_dbus_queue(m) > 0)
2f7d91
                         continue;
2f7d91
 
2f7d91
diff --git a/src/core/manager.h b/src/core/manager.h
2f7d91
index cfc564dfb6..f9280956e9 100644
2f7d91
--- a/src/core/manager.h
2f7d91
+++ b/src/core/manager.h
2f7d91
@@ -117,6 +117,9 @@ struct Manager {
2f7d91
         /* Target units whose default target dependencies haven't been set yet */
2f7d91
         LIST_HEAD(Unit, target_deps_queue);
2f7d91
 
2f7d91
+        /* Units that might be subject to StopWhenUnneeded= clean-up */
2f7d91
+        LIST_HEAD(Unit, stop_when_unneeded_queue);
2f7d91
+
2f7d91
         sd_event *event;
2f7d91
 
2f7d91
         /* We use two hash tables here, since the same PID might be
2f7d91
diff --git a/src/core/unit.c b/src/core/unit.c
2f7d91
index 583b9fae28..dc2df4c89c 100644
2f7d91
--- a/src/core/unit.c
2f7d91
+++ b/src/core/unit.c
2f7d91
@@ -380,6 +380,22 @@ void unit_add_to_dbus_queue(Unit *u) {
2f7d91
         u->in_dbus_queue = true;
2f7d91
 }
2f7d91
 
2f7d91
+void unit_add_to_stop_when_unneeded_queue(Unit *u) {
2f7d91
+        assert(u);
2f7d91
+
2f7d91
+        if (u->in_stop_when_unneeded_queue)
2f7d91
+                return;
2f7d91
+
2f7d91
+        if (!u->stop_when_unneeded)
2f7d91
+                return;
2f7d91
+
2f7d91
+        if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u)))
2f7d91
+                return;
2f7d91
+
2f7d91
+        LIST_PREPEND(stop_when_unneeded_queue, u->manager->stop_when_unneeded_queue, u);
2f7d91
+        u->in_stop_when_unneeded_queue = true;
2f7d91
+}
2f7d91
+
2f7d91
 static void bidi_set_free(Unit *u, Set *s) {
2f7d91
         Iterator i;
2f7d91
         Unit *other;
2f7d91
@@ -544,6 +560,9 @@ void unit_free(Unit *u) {
2f7d91
                 u->manager->n_in_gc_queue--;
2f7d91
         }
2f7d91
 
2f7d91
+        if (u->in_stop_when_unneeded_queue)
2f7d91
+                LIST_REMOVE(stop_when_unneeded_queue, u->manager->stop_when_unneeded_queue, u);
2f7d91
+
2f7d91
         if (u->on_console)
2f7d91
                 manager_unref_console(u->manager);
2f7d91
 
2f7d91
@@ -1565,49 +1584,68 @@ bool unit_can_reload(Unit *u) {
2f7d91
         return UNIT_VTABLE(u)->can_reload(u);
2f7d91
 }
2f7d91
 
2f7d91
-static void unit_check_unneeded(Unit *u) {
2f7d91
-        Iterator i;
2f7d91
-        Unit *other;
2f7d91
+bool unit_is_unneeded(Unit *u) {
2f7d91
+        static const UnitDependency deps[] = {
2f7d91
+                UNIT_REQUIRED_BY,
2f7d91
+                UNIT_REQUIRED_BY_OVERRIDABLE,
2f7d91
+                UNIT_WANTED_BY,
2f7d91
+                UNIT_BOUND_BY,
2f7d91
+        };
2f7d91
+        size_t j;
2f7d91
 
2f7d91
         assert(u);
2f7d91
 
2f7d91
-        /* If this service shall be shut down when unneeded then do
2f7d91
-         * so. */
2f7d91
-
2f7d91
         if (!u->stop_when_unneeded)
2f7d91
-                return;
2f7d91
+                return false;
2f7d91
 
2f7d91
-        if (!UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(u)))
2f7d91
-                return;
2f7d91
+        /* Don't clean up while the unit is transitioning or is even inactive. */
2f7d91
+        if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u)))
2f7d91
+                return false;
2f7d91
+        if (u->job)
2f7d91
+                return false;
2f7d91
 
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUIRED_BY], i)
2f7d91
-                if (unit_active_or_pending(other))
2f7d91
-                        return;
2f7d91
+        for (j = 0; j < ELEMENTSOF(deps); j++) {
2f7d91
+                Unit *other;
2f7d91
+                Iterator i;
2f7d91
 
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUIRED_BY_OVERRIDABLE], i)
2f7d91
-                if (unit_active_or_pending(other))
2f7d91
-                        return;
2f7d91
+                /* If a dependending unit has a job queued, or is active (or in transitioning), or is marked for
2f7d91
+                 * restart, then don't clean this one up. */
2f7d91
 
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_WANTED_BY], i)
2f7d91
-                if (unit_active_or_pending(other))
2f7d91
-                        return;
2f7d91
+                SET_FOREACH(other, u->dependencies[deps[j]], i) {
2f7d91
+                        if (u->job)
2f7d91
+                                return false;
2f7d91
 
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_BOUND_BY], i)
2f7d91
-                if (unit_active_or_pending(other))
2f7d91
-                        return;
2f7d91
-
2f7d91
-        /* If stopping a unit fails continously we might enter a stop
2f7d91
-         * loop here, hence stop acting on the service being
2f7d91
-         * unnecessary after a while. */
2f7d91
-        if (!ratelimit_test(&u->check_unneeded_ratelimit)) {
2f7d91
-                log_unit_warning(u->id, "Unit not needed anymore, but not stopping since we tried this too often recently.");
2f7d91
-                return;
2f7d91
+                        if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other)))
2f7d91
+                                return false;
2f7d91
+                }
2f7d91
         }
2f7d91
 
2f7d91
-        log_unit_info(u->id, "Unit %s is not needed anymore. Stopping.", u->id);
2f7d91
+        return true;
2f7d91
+}
2f7d91
 
2f7d91
-        /* Ok, nobody needs us anymore. Sniff. Then let's commit suicide */
2f7d91
-        manager_add_job(u->manager, JOB_STOP, u, JOB_FAIL, true, NULL, NULL);
2f7d91
+static void check_unneeded_dependencies(Unit *u) {
2f7d91
+
2f7d91
+        static const UnitDependency deps[] = {
2f7d91
+                UNIT_REQUIRES,
2f7d91
+                UNIT_REQUIRES_OVERRIDABLE,
2f7d91
+                UNIT_REQUISITE,
2f7d91
+                UNIT_REQUISITE_OVERRIDABLE,
2f7d91
+                UNIT_WANTS,
2f7d91
+                UNIT_BINDS_TO,
2f7d91
+        };
2f7d91
+        size_t j;
2f7d91
+
2f7d91
+        assert(u);
2f7d91
+
2f7d91
+        /* Add all units this unit depends on to the queue that processes StopWhenUnneeded= behaviour. */
2f7d91
+
2f7d91
+        for (j = 0; j < ELEMENTSOF(deps); j++) {
2f7d91
+                Unit *other;
2f7d91
+                Iterator i;
2f7d91
+
2f7d91
+                SET_FOREACH(other, u->dependencies[deps[j]], i)
2f7d91
+                        unit_add_to_stop_when_unneeded_queue(other);
2f7d91
+        }
2f7d91
 }
2f7d91
 
2f7d91
 static void unit_check_binds_to(Unit *u) {
2f7d91
@@ -1693,34 +1731,6 @@ static void retroactively_stop_dependencies(Unit *u) {
2f7d91
                         manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, true, NULL, NULL);
2f7d91
 }
2f7d91
 
2f7d91
-static void check_unneeded_dependencies(Unit *u) {
2f7d91
-        Iterator i;
2f7d91
-        Unit *other;
2f7d91
-
2f7d91
-        assert(u);
2f7d91
-        assert(UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(u)));
2f7d91
-
2f7d91
-        /* Garbage collect services that might not be needed anymore, if enabled */
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUIRES], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUIRES_OVERRIDABLE], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_WANTS], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUISITE], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_REQUISITE_OVERRIDABLE], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-        SET_FOREACH(other, u->dependencies[UNIT_BINDS_TO], i)
2f7d91
-                if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other)))
2f7d91
-                        unit_check_unneeded(other);
2f7d91
-}
2f7d91
-
2f7d91
 void unit_start_on_failure(Unit *u) {
2f7d91
         Unit *other;
2f7d91
         Iterator i;
2f7d91
@@ -1898,7 +1908,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
2f7d91
                 }
2f7d91
 
2f7d91
                 /* stop unneeded units regardless if going down was expected or not */
2f7d91
-                if (UNIT_IS_INACTIVE_OR_DEACTIVATING(ns))
2f7d91
+                if (UNIT_IS_INACTIVE_OR_FAILED(ns))
2f7d91
                         check_unneeded_dependencies(u);
2f7d91
 
2f7d91
                 if (ns != os && ns == UNIT_FAILED) {
2f7d91
@@ -1959,7 +1969,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su
2f7d91
         if (u->manager->n_reloading <= 0) {
2f7d91
                 /* Maybe we finished startup and are now ready for
2f7d91
                  * being stopped because unneeded? */
2f7d91
-                unit_check_unneeded(u);
2f7d91
+                unit_add_to_stop_when_unneeded_queue(u);
2f7d91
 
2f7d91
                 /* Maybe we finished startup, but something we needed
2f7d91
                  * has vanished? Let's die then. (This happens when
2f7d91
diff --git a/src/core/unit.h b/src/core/unit.h
2f7d91
index 29353ea81c..38c97397ee 100644
2f7d91
--- a/src/core/unit.h
2f7d91
+++ b/src/core/unit.h
2f7d91
@@ -165,6 +165,9 @@ struct Unit {
2f7d91
         /* Target dependencies queue */
2f7d91
         LIST_FIELDS(Unit, target_deps_queue);
2f7d91
 
2f7d91
+        /* Queue of units with StopWhenUnneeded set that shell be checked for clean-up. */
2f7d91
+        LIST_FIELDS(Unit, stop_when_unneeded_queue);
2f7d91
+
2f7d91
         /* PIDs we keep an eye on. Note that a unit might have many
2f7d91
          * more, but these are the ones we care enough about to
2f7d91
          * process SIGCHLD for */
2f7d91
@@ -235,6 +238,7 @@ struct Unit {
2f7d91
         bool in_gc_queue:1;
2f7d91
         bool in_cgroup_queue:1;
2f7d91
         bool in_target_deps_queue:1;
2f7d91
+        bool in_stop_when_unneeded_queue:1;
2f7d91
 
2f7d91
         bool sent_dbus_new_signal:1;
2f7d91
 
2f7d91
@@ -508,6 +512,7 @@ void unit_add_to_dbus_queue(Unit *u);
2f7d91
 void unit_add_to_cleanup_queue(Unit *u);
2f7d91
 void unit_add_to_gc_queue(Unit *u);
2f7d91
 void unit_add_to_target_deps_queue(Unit *u);
2f7d91
+void unit_add_to_stop_when_unneeded_queue(Unit *u);
2f7d91
 
2f7d91
 int unit_merge(Unit *u, Unit *other);
2f7d91
 int unit_merge_by_name(Unit *u, const char *other);
2f7d91
@@ -627,6 +632,8 @@ int unit_make_transient(Unit *u);
2f7d91
 
2f7d91
 int unit_require_mounts_for(Unit *u, const char *path);
2f7d91
 
2f7d91
+bool unit_is_unneeded(Unit *u);
2f7d91
+
2f7d91
 pid_t unit_control_pid(Unit *u);
2f7d91
 pid_t unit_main_pid(Unit *u);
2f7d91