Page MenuHomeSolus
Differential D9534 Diff 22963 files/rework-cache-timestamps.patch

Changeset View

Standalone View

View Options

files/rework-cache-timestamps.patch

  • This file was added.
From 434655bbf9ca131d82b97cb07a3050a62fa623a9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Fri, 28 Aug 2020 10:32:39 +0200
Subject: [PATCH 1/3] core: rename
manager_unit_file_maybe_loadable_from_cache()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The name is misleading, since we aren't really loading the unit from cache — if
this function returns true, we'll try to load the unit from disk, updating the
cache in the process.
---
src/core/manager.c | 9 ++++++---
src/core/manager.h | 2 +-
src/core/transaction.c | 5 +++--
src/core/unit.c | 6 +++---
4 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/core/manager.c b/src/core/manager.c
index 49bf5419d8e..1ccbc0b7f22 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1947,7 +1947,7 @@ unsigned manager_dispatch_load_queue(Manager *m) {
return n;
}
-bool manager_unit_file_maybe_loadable_from_cache(Unit *u) {
+bool manager_unit_cache_should_retry_load(Unit *u) {
assert(u);
if (u->load_state != UNIT_NOT_FOUND)
@@ -1956,9 +1956,12 @@ bool manager_unit_file_maybe_loadable_from_cache(Unit *u) {
if (u->manager->unit_cache_mtime == 0)
return false;
+ /* The cache has been updated since the last time we tried to load the unit. There might be new
+ * fragment paths to read. */
if (u->manager->unit_cache_mtime > u->fragment_loadtime)
return true;
+ /* The cache needs to be updated because there are modifications on disk. */
return !lookup_paths_mtime_good(&u->manager->lookup_paths, u->manager->unit_cache_mtime);
}
@@ -2008,10 +2011,10 @@ int manager_load_unit_prepare(
* first if anything in the usual paths was modified since the last time
* the cache was loaded. Also check if the last time an attempt to load the
* unit was made was before the most recent cache refresh, so that we know
- * we need to try again - even if the cache is current, it might have been
+ * we need to try again — even if the cache is current, it might have been
* updated in a different context before we had a chance to retry loading
* this particular unit. */
- if (manager_unit_file_maybe_loadable_from_cache(ret))
+ if (manager_unit_cache_should_retry_load(ret))
ret->load_state = UNIT_STUB;
else {
*_ret = ret;
diff --git a/src/core/manager.h b/src/core/manager.h
index d507dc1f3b4..760fa917055 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -464,7 +464,7 @@ Unit *manager_get_unit(Manager *m, const char *name);
int manager_get_job_from_dbus_path(Manager *m, const char *s, Job **_j);
-bool manager_unit_file_maybe_loadable_from_cache(Unit *u);
+bool manager_unit_cache_should_retry_load(Unit *u);
int manager_load_unit_prepare(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **_ret);
int manager_load_unit(Manager *m, const char *name, const char *path, sd_bus_error *e, Unit **_ret);
int manager_load_startable_unit_or_warn(Manager *m, const char *name, const char *path, Unit **ret);
diff --git a/src/core/transaction.c b/src/core/transaction.c
index 958243bc94b..0fa419787ef 100644
--- a/src/core/transaction.c
+++ b/src/core/transaction.c
@@ -960,12 +960,13 @@ int transaction_add_job_and_dependencies(
* first if anything in the usual paths was modified since the last time
* the cache was loaded. Also check if the last time an attempt to load the
* unit was made was before the most recent cache refresh, so that we know
- * we need to try again - even if the cache is current, it might have been
+ * we need to try again — even if the cache is current, it might have been
* updated in a different context before we had a chance to retry loading
* this particular unit.
+ *
* Given building up the transaction is a synchronous operation, attempt
* to load the unit immediately. */
- if (r < 0 && manager_unit_file_maybe_loadable_from_cache(unit)) {
+ if (r < 0 && manager_unit_cache_should_retry_load(unit)) {
sd_bus_error_free(e);
unit->load_state = UNIT_STUB;
r = unit_load(unit);
diff --git a/src/core/unit.c b/src/core/unit.c
index e81ef2fd9c9..0124451c78a 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1671,8 +1671,8 @@ int unit_load(Unit *u) {
return 0;
fail:
- /* We convert ENOEXEC errors to the UNIT_BAD_SETTING load state here. Configuration parsing code should hence
- * return ENOEXEC to ensure units are placed in this state after loading */
+ /* We convert ENOEXEC errors to the UNIT_BAD_SETTING load state here. Configuration parsing code
+ * should hence return ENOEXEC to ensure units are placed in this state after loading. */
u->load_state = u->load_state == UNIT_STUB ? UNIT_NOT_FOUND :
r == -ENOEXEC ? UNIT_BAD_SETTING :
@@ -1680,7 +1680,7 @@ int unit_load(Unit *u) {
u->load_error = r;
/* Record the last time we tried to load the unit, so that if the cache gets updated between now
- * and the next time an attempt is made to load this unit, we know we need to check again */
+ * and the next time an attempt is made to load this unit, we know we need to check again. */
if (u->load_state == UNIT_NOT_FOUND)
u->fragment_loadtime = now(CLOCK_REALTIME);
From 7c49d027983d5a2682c3869f7b759cb0dfb97b3e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Fri, 28 Aug 2020 11:19:38 +0200
Subject: [PATCH 2/3] pid1: use the cache mtime not clock to "mark" load
attempts
We really only care if the cache has been reloaded between the time when we
last attempted to load this unit and now. So instead of recording the actual
time we try to load the unit, just store the timestamp of the cache. This has
the advantage that we'll notice if the cache mtime jumps forward or backward.
Also rename fragment_loadtime to fragment_not_found_time. It only gets set when
we failed to load the unit and the old name was suggesting it is always set.
In https://bugzilla.redhat.com/show_bug.cgi?id=1871327
(and most likely https://bugzilla.redhat.com/show_bug.cgi?id=1867930
and most likely https://bugzilla.redhat.com/show_bug.cgi?id=1872068) we try
to load a non-existent unit over and over from transaction_add_job_and_dependencies().
My understanding is that the clock was in the future during inital boot,
so cache_mtime is always in the future (since we don't touch the fs after initial boot),
so no matter how many times we try to load the unit and set
fragment_loadtime / fragment_not_found_time, it is always higher than cache_mtime,
so manager_unit_cache_should_retry_load() always returns true.
---
src/core/manager.c | 2 +-
src/core/unit.c | 6 +++---
src/core/unit.h | 2 +-
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/core/manager.c b/src/core/manager.c
index 1ccbc0b7f22..a4fff9978f6 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -1958,7 +1958,7 @@ bool manager_unit_cache_should_retry_load(Unit *u) {
/* The cache has been updated since the last time we tried to load the unit. There might be new
* fragment paths to read. */
- if (u->manager->unit_cache_mtime > u->fragment_loadtime)
+ if (u->manager->unit_cache_mtime != u->fragment_not_found_time)
return true;
/* The cache needs to be updated because there are modifications on disk. */
diff --git a/src/core/unit.c b/src/core/unit.c
index 0124451c78a..e70831869e2 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1679,10 +1679,10 @@ int unit_load(Unit *u) {
UNIT_ERROR;
u->load_error = r;
- /* Record the last time we tried to load the unit, so that if the cache gets updated between now
- * and the next time an attempt is made to load this unit, we know we need to check again. */
+ /* Record the timestamp on the cache, so that if the cache gets updated between now and the next time
+ * an attempt is made to load this unit, we know we need to check again. */
if (u->load_state == UNIT_NOT_FOUND)
- u->fragment_loadtime = now(CLOCK_REALTIME);
+ u->fragment_not_found_time = u->manager->unit_cache_mtime;
unit_add_to_dbus_queue(u);
unit_add_to_gc_queue(u);
diff --git a/src/core/unit.h b/src/core/unit.h
index e217cec2191..a54d649cd32 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -136,7 +136,7 @@ typedef struct Unit {
char *source_path; /* if converted, the source file */
char **dropin_paths;
- usec_t fragment_loadtime;
+ usec_t fragment_not_found_time;
usec_t fragment_mtime;
usec_t source_mtime;
usec_t dropin_mtime;
From 1b3be327d2f45e5e118f04db1193b70df84a3c8f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>
Date: Fri, 28 Aug 2020 12:21:48 +0200
Subject: [PATCH 3/3] Rework how we cache mtime to figure out if units changed
Instead of assuming that more-recently modified directories have higher mtime,
just look for any mtime changes, up or down. Since we don't want to remember
individual mtimes, hash them to obtain a single value.
This should help us behave properly in the case when the time jumps backwards
during boot: various files might have mtimes that in the future, but we won't
care. This fixes the following scenario:
We have /etc/systemd/system with T1. T1 is initially far in the past.
We have /run/systemd/generator with time T2.
The time is adjusted backwards, so T2 will be always in the future for a while.
Now the user writes new files to /etc/systemd/system, and T1 is updated to T1'.
Nevertheless, T1 < T1' << T2.
We would consider our cache to be up-to-date, falsely.
---
src/basic/siphash24.h | 6 +++++
src/core/load-fragment.c | 2 +-
src/core/manager.c | 8 +++----
src/core/manager.h | 2 +-
src/core/unit.c | 2 +-
src/core/unit.h | 2 +-
src/shared/unit-file.c | 51 ++++++++++++++++++++++------------------
src/shared/unit-file.h | 12 +++++-----
8 files changed, 48 insertions(+), 37 deletions(-)
diff --git a/src/basic/siphash24.h b/src/basic/siphash24.h
index 7f799ede3d1..fe43256882e 100644
--- a/src/basic/siphash24.h
+++ b/src/basic/siphash24.h
@@ -6,6 +6,8 @@
#include <string.h>
#include <sys/types.h>
+#include "time-util.h"
+
struct siphash {
uint64_t v0;
uint64_t v1;
@@ -25,6 +27,10 @@ static inline void siphash24_compress_boolean(bool in, struct siphash *state) {
siphash24_compress(&i, sizeof i, state);
}
+static inline void siphash24_compress_usec_t(usec_t in, struct siphash *state) {
+ siphash24_compress(&in, sizeof in, state);
+}
+
static inline void siphash24_compress_string(const char *in, struct siphash *state) {
if (!in)
return;
diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c
index a8eee285021..6cf3521f811 100644
--- a/src/core/load-fragment.c
+++ b/src/core/load-fragment.c
@@ -5268,7 +5268,7 @@ int unit_load_fragment(Unit *u) {
/* Possibly rebuild the fragment map to catch new units */
r = unit_file_build_name_map(&u->manager->lookup_paths,
- &u->manager->unit_cache_mtime,
+ &u->manager->unit_cache_timestamp,
&u->manager->unit_id_map,
&u->manager->unit_name_map,
&u->manager->unit_path_cache);
diff --git a/src/core/manager.c b/src/core/manager.c
index a4fff9978f6..918a972b19c 100644
--- a/src/core/manager.c
+++ b/src/core/manager.c
@@ -704,7 +704,7 @@ static void manager_free_unit_name_maps(Manager *m) {
m->unit_id_map = hashmap_free(m->unit_id_map);
m->unit_name_map = hashmap_free(m->unit_name_map);
m->unit_path_cache = set_free(m->unit_path_cache);
- m->unit_cache_mtime = 0;
+ m->unit_cache_timestamp = 0;
}
static int manager_setup_run_queue(Manager *m) {
@@ -1953,16 +1953,16 @@ bool manager_unit_cache_should_retry_load(Unit *u) {
if (u->load_state != UNIT_NOT_FOUND)
return false;
- if (u->manager->unit_cache_mtime == 0)
+ if (u->manager->unit_cache_timestamp == 0)
return false;
/* The cache has been updated since the last time we tried to load the unit. There might be new
* fragment paths to read. */
- if (u->manager->unit_cache_mtime != u->fragment_not_found_time)
+ if (u->manager->unit_cache_timestamp != u->fragment_not_found_timestamp)
return true;
/* The cache needs to be updated because there are modifications on disk. */
- return !lookup_paths_mtime_good(&u->manager->lookup_paths, u->manager->unit_cache_mtime);
+ return !lookup_paths_timestamp_good(&u->manager->lookup_paths, u->manager->unit_cache_timestamp, NULL);
}
int manager_load_unit_prepare(
diff --git a/src/core/manager.h b/src/core/manager.h
index 760fa917055..0212016e8b8 100644
--- a/src/core/manager.h
+++ b/src/core/manager.h
@@ -233,7 +233,7 @@ struct Manager {
Hashmap *unit_id_map;
Hashmap *unit_name_map;
Set *unit_path_cache;
- usec_t unit_cache_mtime;
+ uint64_t unit_cache_timestamp;
char **transient_environment; /* The environment, as determined from config files, kernel cmdline and environment generators */
char **client_environment; /* Environment variables created by clients through the bus API */
diff --git a/src/core/unit.c b/src/core/unit.c
index e70831869e2..c3edccf8dc8 100644
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1682,7 +1682,7 @@ int unit_load(Unit *u) {
/* Record the timestamp on the cache, so that if the cache gets updated between now and the next time
* an attempt is made to load this unit, we know we need to check again. */
if (u->load_state == UNIT_NOT_FOUND)
- u->fragment_not_found_time = u->manager->unit_cache_mtime;
+ u->fragment_not_found_timestamp = u->manager->unit_cache_timestamp;
unit_add_to_dbus_queue(u);
unit_add_to_gc_queue(u);
diff --git a/src/core/unit.h b/src/core/unit.h
index a54d649cd32..4e196a90d7b 100644
--- a/src/core/unit.h
+++ b/src/core/unit.h
@@ -136,7 +136,7 @@ typedef struct Unit {
char *source_path; /* if converted, the source file */
char **dropin_paths;
- usec_t fragment_not_found_time;
+ usec_t fragment_not_found_timestamp;
usec_t fragment_mtime;
usec_t source_mtime;
usec_t dropin_mtime;
diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c
index ed4affd6683..57acea80e71 100644
--- a/src/shared/unit-file.c
+++ b/src/shared/unit-file.c
@@ -1,5 +1,7 @@
/* SPDX-License-Identifier: LGPL-2.1+ */
+#include "sd-id128.h"
+
#include "dirent-util.h"
#include "fd-util.h"
#include "fs-util.h"
@@ -199,9 +201,14 @@ static bool lookup_paths_mtime_exclude(const LookupPaths *lp, const char *path)
streq_ptr(path, lp->runtime_control);
}
-bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) {
- char **dir;
+#define HASH_KEY SD_ID128_MAKE(4e,86,1b,e3,39,b3,40,46,98,5d,b8,11,34,8f,c3,c1)
+
+bool lookup_paths_timestamp_good(const LookupPaths *lp, uint64_t timestamp, uint64_t *ret_new) {
+ struct siphash state;
+ siphash24_init(&state, HASH_KEY.bytes);
+
+ char **dir;
STRV_FOREACH(dir, (char**) lp->search_path) {
struct stat st;
@@ -217,18 +224,20 @@ bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) {
continue;
}
- if (timespec_load(&st.st_mtim) > mtime) {
- log_debug_errno(errno, "Unit dir %s has changed, need to update cache.", *dir);
- return false;
- }
+ siphash24_compress_usec_t(timespec_load(&st.st_mtim), &state);
}
- return true;
+ uint64_t new = siphash24_finalize(&state);
+ if (ret_new)
+ *ret_new = new;
+ if (new != timestamp)
+ log_debug_errno(errno, "Modification times have changed, need to update cache.");
+ return new == timestamp;
}
int unit_file_build_name_map(
const LookupPaths *lp,
- usec_t *cache_mtime,
+ uint64_t *cache_timestamp,
Hashmap **unit_ids_map,
Hashmap **unit_names_map,
Set **path_cache) {
@@ -245,14 +254,18 @@ int unit_file_build_name_map(
_cleanup_hashmap_free_ Hashmap *ids = NULL, *names = NULL;
_cleanup_set_free_free_ Set *paths = NULL;
+ uint64_t timestamp;
char **dir;
int r;
- usec_t mtime = 0;
- /* Before doing anything, check if the mtime that was passed is still valid. If
- * yes, do nothing. If *cache_time == 0, always build the cache. */
- if (cache_mtime && *cache_mtime > 0 && lookup_paths_mtime_good(lp, *cache_mtime))
- return 0;
+ /* Before doing anything, check if the timestamp that was passed is still valid.
+ * If yes, do nothing. */
+ if (cache_timestamp &&
+ lookup_paths_timestamp_good(lp, *cache_timestamp, &timestamp))
+ return 0;
+
+ /* The timestamp is now set based on the mtimes from before when we start reading files. If anything
+ * is modified concurrently, we'll consider the cache outdated. */
if (path_cache) {
paths = set_new(&path_hash_ops_free);
@@ -263,7 +276,6 @@ int unit_file_build_name_map(
STRV_FOREACH(dir, (char**) lp->search_path) {
struct dirent *de;
_cleanup_closedir_ DIR *d = NULL;
- struct stat st;
d = opendir(*dir);
if (!d) {
@@ -272,13 +284,6 @@ int unit_file_build_name_map(
continue;
}
- /* Determine the latest lookup path modification time */
- if (fstat(dirfd(d), &st) < 0)
- return log_error_errno(errno, "Failed to fstat %s: %m", *dir);
-
- if (!lookup_paths_mtime_exclude(lp, *dir))
- mtime = MAX(mtime, timespec_load(&st.st_mtim));
-
FOREACH_DIRENT_ALL(de, d, log_warning_errno(errno, "Failed to read \"%s\", ignoring: %m", *dir)) {
char *filename;
_cleanup_free_ char *_filename_free = NULL, *simplified = NULL;
@@ -417,8 +422,8 @@ int unit_file_build_name_map(
basename(dst), src);
}
- if (cache_mtime)
- *cache_mtime = mtime;
+ if (cache_timestamp)
+ *cache_timestamp = timestamp;
hashmap_free_and_replace(*unit_ids_map, ids);
hashmap_free_and_replace(*unit_names_map, names);
diff --git a/src/shared/unit-file.h b/src/shared/unit-file.h
index d6d041d714b..8ec0bd5552d 100644
--- a/src/shared/unit-file.h
+++ b/src/shared/unit-file.h
@@ -43,19 +43,19 @@ bool unit_type_may_template(UnitType type) _const_;
int unit_symlink_name_compatible(const char *symlink, const char *target, bool instance_propagation);
int unit_validate_alias_symlink_and_warn(const char *filename, const char *target);
-bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime);
+bool lookup_paths_timestamp_good(const LookupPaths *lp, uint64_t timestamp, uint64_t *ret_new);
int unit_file_build_name_map(
const LookupPaths *lp,
- usec_t *ret_time,
- Hashmap **ret_unit_ids_map,
- Hashmap **ret_unit_names_map,
- Set **ret_path_cache);
+ uint64_t *cache_timestamp,
+ Hashmap **unit_ids_map,
+ Hashmap **unit_names_map,
+ Set **path_cache);
int unit_file_find_fragment(
Hashmap *unit_ids_map,
Hashmap *unit_name_map,
const char *unit_name,
const char **ret_fragment_path,
- Set **names);
+ Set **ret_names);
const char* runlevel_to_target(const char *rl);
Copyright © 2015-2021 Solus Project. The Solus logo is Copyright © 2016-2021 Solus Project. All Rights Reserved.