daandemeyer / rpms / systemd

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