diff --git a/files/rework-cache-timestamps.patch b/files/rework-cache-timestamps.patch new file mode 100644 --- /dev/null +++ b/files/rework-cache-timestamps.patch @@ -0,0 +1,472 @@ +From 434655bbf9ca131d82b97cb07a3050a62fa623a9 Mon Sep 17 00:00:00 2001 +From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= +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?= +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?= +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 + #include + ++#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, ×tamp)) ++ 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); diff --git a/files/series b/files/series --- a/files/series +++ b/files/series @@ -5,3 +5,4 @@ 0001-sysusers-Explicitly-set-9-as-lp-ID.patch clr/0037-Make-timesyncd-a-simple-service.patch 0001-login-Allow-more-active-permissions-to-prevent-permi.patch +rework-cache-timestamps.patch diff --git a/package.yml b/package.yml --- a/package.yml +++ b/package.yml @@ -1,6 +1,6 @@ name : systemd version : '246.3' -release : 88 +release : 89 source : - https://github.com/systemd/systemd-stable/archive/v246.3.tar.gz : d33619cee7f283c1f2ef78ed31024eca641a0886bbb51d63300e816714637384 license : diff --git a/pspec_x86_64.xml b/pspec_x86_64.xml --- a/pspec_x86_64.xml +++ b/pspec_x86_64.xml @@ -2,8 +2,8 @@ systemd - Joshua Strobl - joshua@streambits.io + Thomas Staudinger + Staudi.Kaos@gmail.com LGPL-2.1-or-later GPL-2.0-or-later @@ -935,7 +935,7 @@ emul32 - systemd + systemd /usr/lib32/libnss_myhostname.so.2 @@ -957,8 +957,8 @@ programming.devel - systemd-devel - systemd-32bit + systemd-devel + systemd-32bit /usr/lib32/pkgconfig/libsystemd.pc @@ -972,7 +972,7 @@ system.devel - systemd + systemd /usr/include/libudev.h @@ -1695,12 +1695,12 @@ - - 2020-08-27 + + 2020-08-28 246.3 Packaging update - Joshua Strobl - joshua@streambits.io + Thomas Staudinger + Staudi.Kaos@gmail.com \ No newline at end of file