daandemeyer / rpms / systemd

Forked from rpms/systemd 2 years ago
Clone
2f7d91
From 9684908410868cb590ad74f4747bdbf9b5790abb Mon Sep 17 00:00:00 2001
2f7d91
From: Lennart Poettering <lennart@poettering.net>
2f7d91
Date: Mon, 13 Nov 2017 15:08:49 +0100
2f7d91
Subject: [PATCH] unit: rework a bit how we keep the service fdstore from being
2f7d91
 destroyed during service restart
2f7d91
2f7d91
When preparing for a restart we quickly go through the DEAD/INACTIVE
2f7d91
service state before entering AUTO_RESTART. When doing this, we need to
2f7d91
make sure we don't destroy the FD store. Previously this was done by
2f7d91
checking the failure state of the unit, and keeping the FD store around
2f7d91
when the unit failed, under the assumption that the restart logic will
2f7d91
then get into action.
2f7d91
2f7d91
This is not entirely correct howver, as there might be failure states
2f7d91
that will no result in restarts.
2f7d91
2f7d91
With this commit we slightly alter the logic: a ref counter for the fd
2f7d91
store is added, that is increased right before we handle the restart
2f7d91
logic, and decreased again right-after.
2f7d91
2f7d91
This should ensure that the fdstore lives exactly as long as it needs.
2f7d91
2f7d91
Follow-up for f0bfbfac43b7faa68ef1bb2ad659c191b9ec85d2.
2f7d91
2f7d91
(cherry picked from commit 7eb2a8a1259043e107ebec94e30ed160a93f40a7)
2f7d91
(cherry picked from commit e2636bde0f07319d0d35262dac6ff2638ba4e598)
2f7d91
Resolves: #1803802
2f7d91
---
2f7d91
 src/core/service.c | 23 ++++++++++++++++++-----
2f7d91
 src/core/service.h |  1 +
2f7d91
 src/core/unit.c    | 12 ++++--------
2f7d91
 src/core/unit.h    |  5 ++---
2f7d91
 4 files changed, 25 insertions(+), 16 deletions(-)
2f7d91
2f7d91
diff --git a/src/core/service.c b/src/core/service.c
2f7d91
index 3c2f69a003..e32cdf4594 100644
2f7d91
--- a/src/core/service.c
2f7d91
+++ b/src/core/service.c
2f7d91
@@ -261,6 +261,9 @@ static void service_fd_store_unlink(ServiceFDStore *fs) {
2f7d91
 static void service_release_fd_store(Service *s) {
2f7d91
         assert(s);
2f7d91
 
2f7d91
+        if (s->n_keep_fd_store > 0)
2f7d91
+                return;
2f7d91
+
2f7d91
         log_unit_debug(UNIT(s)->id, "Releasing all stored fds");
2f7d91
         while (s->fd_store)
2f7d91
                 service_fd_store_unlink(s->fd_store);
2f7d91
@@ -268,7 +271,7 @@ static void service_release_fd_store(Service *s) {
2f7d91
         assert(s->n_fd_store == 0);
2f7d91
 }
2f7d91
 
2f7d91
-static void service_release_resources(Unit *u, bool inactive) {
2f7d91
+static void service_release_resources(Unit *u) {
2f7d91
         Service *s = SERVICE(u);
2f7d91
 
2f7d91
         assert(s);
2f7d91
@@ -278,8 +281,7 @@ static void service_release_resources(Unit *u, bool inactive) {
2f7d91
 
2f7d91
         log_unit_debug(u->id, "Releasing resources.");
2f7d91
 
2f7d91
-        if (inactive)
2f7d91
-                service_release_fd_store(s);
2f7d91
+        service_release_fd_store(s);
2f7d91
 }
2f7d91
 
2f7d91
 static void service_done(Unit *u) {
2f7d91
@@ -327,7 +329,7 @@ static void service_done(Unit *u) {
2f7d91
 
2f7d91
         s->timer_event_source = sd_event_source_unref(s->timer_event_source);
2f7d91
 
2f7d91
-        service_release_resources(u, true);
2f7d91
+        service_release_resources(u);
2f7d91
 }
2f7d91
 
2f7d91
 static int on_fd_store_io(sd_event_source *e, int fd, uint32_t revents, void *userdata) {
2f7d91
@@ -1330,6 +1332,10 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
2f7d91
         if (f != SERVICE_SUCCESS)
2f7d91
                 s->result = f;
2f7d91
 
2f7d91
+        /* Make sure service_release_resources() doesn't destroy our FD store, while we are changing through
2f7d91
+         * SERVICE_FAILED/SERVICE_DEAD before entering into SERVICE_AUTO_RESTART. */
2f7d91
+        s->n_keep_fd_store++;
2f7d91
+
2f7d91
         service_set_state(s, s->result != SERVICE_SUCCESS ? SERVICE_FAILED : SERVICE_DEAD);
2f7d91
 
2f7d91
         if (s->result != SERVICE_SUCCESS) {
2f7d91
@@ -1351,12 +1357,19 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart)
2f7d91
             (!IN_SET(s->main_exec_status.code, CLD_KILLED, CLD_DUMPED) || !set_contains(s->restart_prevent_status.signal, INT_TO_PTR(s->main_exec_status.status)))) {
2f7d91
 
2f7d91
                 r = service_arm_timer(s, s->restart_usec);
2f7d91
-                if (r < 0)
2f7d91
+                if (r < 0) {
2f7d91
+                        s->n_keep_fd_store--;
2f7d91
                         goto fail;
2f7d91
+                }
2f7d91
 
2f7d91
                 service_set_state(s, SERVICE_AUTO_RESTART);
2f7d91
         }
2f7d91
 
2f7d91
+        /* The new state is in effect, let's decrease the fd store ref counter again. Let's also readd us to the GC
2f7d91
+         * queue, so that the fd store is possibly gc'ed again */
2f7d91
+        s->n_keep_fd_store--;
2f7d91
+        unit_add_to_gc_queue(UNIT(s));
2f7d91
+
2f7d91
         s->forbid_restart = false;
2f7d91
 
2f7d91
         /* We want fresh tmpdirs in case service is started again immediately */
2f7d91
diff --git a/src/core/service.h b/src/core/service.h
2f7d91
index 82938a1fc4..e5753f1f4c 100644
2f7d91
--- a/src/core/service.h
2f7d91
+++ b/src/core/service.h
2f7d91
@@ -215,6 +215,7 @@ struct Service {
2f7d91
         ServiceFDStore *fd_store;
2f7d91
         unsigned n_fd_store;
2f7d91
         unsigned n_fd_store_max;
2f7d91
+        unsigned n_keep_fd_store;
2f7d91
 };
2f7d91
 
2f7d91
 extern const UnitVTable service_vtable;
2f7d91
diff --git a/src/core/unit.c b/src/core/unit.c
2f7d91
index 22e31a76b3..eff9fdbe70 100644
2f7d91
--- a/src/core/unit.c
2f7d91
+++ b/src/core/unit.c
2f7d91
@@ -285,7 +285,6 @@ int unit_set_description(Unit *u, const char *description) {
2f7d91
 
2f7d91
 bool unit_may_gc(Unit *u) {
2f7d91
         UnitActiveState state;
2f7d91
-        bool inactive;
2f7d91
         assert(u);
2f7d91
 
2f7d91
         /* Checks whether the unit is ready to be unloaded for garbage collection.
2f7d91
@@ -303,17 +302,14 @@ bool unit_may_gc(Unit *u) {
2f7d91
                 return false;
2f7d91
 
2f7d91
         state = unit_active_state(u);
2f7d91
-        inactive = state == UNIT_INACTIVE;
2f7d91
 
2f7d91
-        /* If the unit is inactive and failed and no job is queued for
2f7d91
-         * it, then release its runtime resources */
2f7d91
+        /* If the unit is inactive and failed and no job is queued for it, then release its runtime resources */
2f7d91
         if (UNIT_IS_INACTIVE_OR_FAILED(state) &&
2f7d91
             UNIT_VTABLE(u)->release_resources)
2f7d91
-                UNIT_VTABLE(u)->release_resources(u, inactive);
2f7d91
+                UNIT_VTABLE(u)->release_resources(u);
2f7d91
 
2f7d91
-        /* But we keep the unit object around for longer when it is
2f7d91
-         * referenced or configured to not be gc'ed */
2f7d91
-        if (!inactive)
2f7d91
+        /* But we keep the unit object around for longer when it is referenced or configured to not be gc'ed */
2f7d91
+        if (state != UNIT_INACTIVE)
2f7d91
                 return false;
2f7d91
 
2f7d91
         if (UNIT_VTABLE(u)->no_gc)
2f7d91
diff --git a/src/core/unit.h b/src/core/unit.h
2f7d91
index 232be8164f..3b0fd8d9df 100644
2f7d91
--- a/src/core/unit.h
2f7d91
+++ b/src/core/unit.h
2f7d91
@@ -358,9 +358,8 @@ struct UnitVTable {
2f7d91
          * even though nothing references it and it isn't active in any way. */
2f7d91
         bool (*may_gc)(Unit *u);
2f7d91
 
2f7d91
-        /* When the unit is not running and no job for it queued we
2f7d91
-         * shall release its runtime resources */
2f7d91
-        void (*release_resources)(Unit *u, bool inactive);
2f7d91
+        /* When the unit is not running and no job for it queued we shall release its runtime resources */
2f7d91
+        void (*release_resources)(Unit *u);
2f7d91
 
2f7d91
         /* Return true when this unit is suitable for snapshotting */
2f7d91
         bool (*check_snapshot)(Unit *u);