Merge pull request #16885 from keszybz/rework-cache-timestamps

Rework cache timestamps

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;

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);

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) {
@@ -1947,19 +1947,22 @@ 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);
 
+        /* Automatic reloading from disk only applies to units which were not found sometime in the past, and
+         * the not-found stub is kept pinned in the unit graph by dependencies. For units that were
+         * previously loaded, we don't do automatic reloading, and daemon-reload is necessary to update. */
         if (u->load_state != UNIT_NOT_FOUND)
                 return false;
 
-        if (u->manager->unit_cache_mtime == 0)
-                return false;
-
-        if (u->manager->unit_cache_mtime > u->fragment_loadtime)
+        /* 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_timestamp_hash != u->fragment_not_found_timestamp_hash)
                 return true;
 
-        return !lookup_paths_mtime_good(&u->manager->lookup_paths, u->manager->unit_cache_mtime);
+        /* The cache needs to be updated because there are modifications on disk. */
+        return !lookup_paths_timestamp_hash_same(&u->manager->lookup_paths, u->manager->unit_cache_timestamp_hash, NULL);
 }
 
 int manager_load_unit_prepare(
@@ -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;

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 */
@@ -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);

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);

src/core/unit.c

@@ -1671,18 +1671,18 @@ 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 :
                                                      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_timestamp_hash = u->manager->unit_cache_timestamp_hash;
 
         unit_add_to_dbus_queue(u);
         unit_add_to_gc_queue(u);

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_timestamp_hash;
         usec_t fragment_mtime;
         usec_t source_mtime;
         usec_t dropin_mtime;

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);

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);