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