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.

diff --git a/src/basic/siphash24.h b/src/basic/siphash24.h
index 7f799ede3d19628261ac85912c6001334f8b329c..fe43256882eccf86fe112310c11de5887247f9e1 100644 (file)
--- 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 a8eee285021cd35ec0aebccd5cfa6aeb39f79065..a93f12b27c45a1e8dad1c5e14c2425ef52590419 100644 (file)
--- 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_hash,
                                      &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 b2878c236d9cfd86ea2bbe1eac916c0df37d3f9c..bd02337fafaf82e53e3de4f0db07617bdbe0d3eb 100644 (file)
--- 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_hash = 0;
 }
 
 static int manager_setup_run_queue(Manager *m) {
@@ -1958,11 +1958,11 @@ 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_not_found_time)
+        if (u->manager->unit_cache_timestamp_hash != u->fragment_not_found_timestamp_hash)
                 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_hash_same(&u->manager->lookup_paths, u->manager->unit_cache_timestamp_hash, NULL);
 }
 
 int manager_load_unit_prepare(

diff --git a/src/core/manager.h b/src/core/manager.h
index 760fa917055f311174708a1b589911da3bb522e7..9e98b31c4bd1085d55e37a5cdb1e60f2498a00bf 100644 (file)
--- 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_hash;
 
         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 e70831869e2d4a14b2f8cffd749d07aea5724d27..518a07f619f2166fdea2cc0fc615906293325990 100644 (file)
--- a/src/core/unit.c
+++ b/src/core/unit.c
@@ -1682,7 +1682,7 @@ fail:
         /* 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_hash = u->manager->unit_cache_timestamp_hash;
 
         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 a54d649cd32f0236fc25e18c38dbdf376c86c05e..5a75136e3d02ead489d08096e21695246a9349ba 100644 (file)
--- 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_hash;
         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 ed4affd6683543ea5c2d1f97f63344fe99233e92..0f030ae4313f17303b85f107afcdbb500634f804 100644 (file)
--- 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_hash_same(const LookupPaths *lp, uint64_t timestamp_hash, 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 updated = siphash24_finalize(&state);
+        if (ret_new)
+                *ret_new = updated;
+        if (updated != timestamp_hash)
+                log_debug("Modification times have changed, need to update cache.");
+        return updated == timestamp_hash;
 }
 
 int unit_file_build_name_map(
                 const LookupPaths *lp,
-                usec_t *cache_mtime,
+                uint64_t *cache_timestamp_hash,
                 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_hash;
         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 hash that was passed is still valid.
+         * If yes, do nothing. */
+        if (cache_timestamp_hash &&
+            lookup_paths_timestamp_hash_same(lp, *cache_timestamp_hash, &timestamp_hash))
+                        return 0;
+
+        /* The timestamp hash 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_hash)
+                *cache_timestamp_hash = timestamp_hash;
 
         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 d6d041d714bc60f443ebc45aa90c82d0533bc72d..d36bb07cc275109b9a5f097647b1230b0b89df86 100644 (file)
--- 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_hash_same(const LookupPaths *lp, uint64_t timestamp_hash, 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_hash,
+                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);