From b1b657c48fad087d6451ae022a2f246a07b05c59 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Nov 2020 15:10:52 +0100 Subject: [PATCH 1/3] copy: teach copy_file() that a mode=-1 call means "take mode from original file" --- src/basic/copy.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index 6a9c3a396f..aa805bb8e2 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -1047,18 +1047,29 @@ int copy_file_full( copy_progress_bytes_t progress_bytes, void *userdata) { + _cleanup_close_ int fdf = -1; + struct stat st; int fdt = -1, r; assert(from); assert(to); + fdf = open(from, O_RDONLY|O_CLOEXEC|O_NOCTTY); + if (fdf < 0) + return -errno; + + if (mode == (mode_t) -1) + if (fstat(fdf, &st) < 0) + return -errno; + RUN_WITH_UMASK(0000) { if (copy_flags & COPY_MAC_CREATE) { r = mac_selinux_create_file_prepare(to, S_IFREG); if (r < 0) return r; } - fdt = open(to, flags|O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY, mode); + fdt = open(to, flags|O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY, + mode != (mode_t) -1 ? mode : st.st_mode); if (copy_flags & COPY_MAC_CREATE) mac_selinux_create_file_clear(); if (fdt < 0) @@ -1068,13 +1079,16 @@ int copy_file_full( if (chattr_mask != 0) (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); - r = copy_file_fd_full(from, fdt, copy_flags, progress_bytes, userdata); + r = copy_bytes_full(fdf, fdt, (uint64_t) -1, copy_flags, NULL, NULL, progress_bytes, userdata); if (r < 0) { close(fdt); (void) unlink(to); return r; } + (void) copy_times(fdf, fdt, copy_flags); + (void) copy_xattr(fdf, fdt); + if (chattr_mask != 0) (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); From 1098142436f46b889f6b7bcc87af54bc5b95d560 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Nov 2020 15:11:43 +0100 Subject: [PATCH 2/3] fs-util: add conservative_rename() that suppresses unnecessary renames if the source and destination file match in contents and basic file attributes, don#t rename, but just remove source. This is a simple way to suppress inotify events + mtime changes when atomically updating files. --- src/basic/fs-util.c | 77 +++++++++++++++++++++++++++++++++++++++++ src/basic/fs-util.h | 2 ++ src/test/test-fs-util.c | 48 +++++++++++++++++++++++++ 3 files changed, 127 insertions(+) diff --git a/src/basic/fs-util.c b/src/basic/fs-util.c index 6924f5dfb1..f240f84322 100644 --- a/src/basic/fs-util.c +++ b/src/basic/fs-util.c @@ -1613,3 +1613,80 @@ int path_is_encrypted(const char *path) { return blockdev_is_encrypted(p, 10 /* safety net: maximum recursion depth */); } + +int conservative_rename( + int olddirfd, const char *oldpath, + int newdirfd, const char *newpath) { + + _cleanup_close_ int old_fd = -1, new_fd = -1; + struct stat old_stat, new_stat; + + /* Renames the old path to thew new path, much like renameat() — except if both are regular files and + * have the exact same contents and basic file attributes already. In that case remove the new file + * instead. This call is useful for reducing inotify wakeups on files that are updated but don't + * actually change. This function is written in a style that we rather rename too often than suppress + * too much. i.e. whenever we are in doubt we rather rename than fail. After all reducing inotify + * events is an optimization only, not more. */ + + old_fd = openat(olddirfd, oldpath, O_CLOEXEC|O_RDONLY|O_NOCTTY|O_NOFOLLOW); + if (old_fd < 0) + goto do_rename; + + new_fd = openat(newdirfd, newpath, O_CLOEXEC|O_RDONLY|O_NOCTTY|O_NOFOLLOW); + if (new_fd < 0) + goto do_rename; + + if (fstat(old_fd, &old_stat) < 0) + goto do_rename; + + if (!S_ISREG(old_stat.st_mode)) + goto do_rename; + + if (fstat(new_fd, &new_stat) < 0) + goto do_rename; + + if (new_stat.st_ino == old_stat.st_ino && + new_stat.st_dev == old_stat.st_dev) + goto is_same; + + if (old_stat.st_mode != new_stat.st_mode || + old_stat.st_size != new_stat.st_size || + old_stat.st_uid != new_stat.st_uid || + old_stat.st_gid != new_stat.st_gid) + goto do_rename; + + for (;;) { + char buf1[16*1024]; + char buf2[sizeof(buf1) + 1]; + ssize_t l1, l2; + + l1 = read(old_fd, buf1, sizeof(buf1)); + if (l1 < 0) + goto do_rename; + + l2 = read(new_fd, buf2, l1 + 1); + if (l1 != l2) + goto do_rename; + + if (l1 == 0) /* EOF on both! And everything's the same so far, yay! */ + break; + + if (memcmp(buf1, buf2, l1) != 0) + goto do_rename; + } + +is_same: + /* Everything matches? Then don't rename, instead remove the source file, and leave the existing + * destination in place */ + + if (unlinkat(olddirfd, oldpath, 0) < 0) + goto do_rename; + + return 0; + +do_rename: + if (renameat(olddirfd, oldpath, newdirfd, newpath) < 0) + return -errno; + + return 1; +} diff --git a/src/basic/fs-util.h b/src/basic/fs-util.h index 5dc8853eac..9a39473567 100644 --- a/src/basic/fs-util.h +++ b/src/basic/fs-util.h @@ -132,3 +132,5 @@ int syncfs_path(int atfd, const char *path); int open_parent(const char *path, int flags, mode_t mode); int path_is_encrypted(const char *path); + +int conservative_rename(int olddirfd, const char *oldpath, int newdirfd, const char *newpath); diff --git a/src/test/test-fs-util.c b/src/test/test-fs-util.c index d1f9252521..e0ef8257bd 100644 --- a/src/test/test-fs-util.c +++ b/src/test/test-fs-util.c @@ -3,7 +3,9 @@ #include #include "alloc-util.h" +#include "copy.h" #include "fd-util.h" +#include "fileio.h" #include "fs-util.h" #include "id128-util.h" #include "macro.h" @@ -834,6 +836,51 @@ static void test_path_is_encrypted(void) { test_path_is_encrypted_one("/dev", booted > 0 ? false : -1); } +static void test_conservative_rename(void) { + _cleanup_(unlink_and_freep) char *p = NULL; + _cleanup_free_ char *q = NULL; + + assert_se(tempfn_random_child(NULL, NULL, &p) >= 0); + assert_se(write_string_file(p, "this is a test", WRITE_STRING_FILE_CREATE) >= 0); + + assert_se(tempfn_random_child(NULL, NULL, &q) >= 0); + + /* Check that the hardlinked "copy" is detected */ + assert_se(link(p, q) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) == 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Check that a manual copy is detected */ + assert_se(copy_file(p, q, 0, (mode_t) -1, 0, 0, COPY_REFLINK) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) == 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Check that a manual new writeout is also detected */ + assert_se(write_string_file(q, "this is a test", WRITE_STRING_FILE_CREATE) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) == 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Check that a minimally changed version is detected */ + assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) > 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Check that this really is new updated version */ + assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) == 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Make sure we detect extended files */ + assert_se(write_string_file(q, "this is a_testx", WRITE_STRING_FILE_CREATE) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) > 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); + + /* Make sure we detect truncated files */ + assert_se(write_string_file(q, "this is a_test", WRITE_STRING_FILE_CREATE) >= 0); + assert_se(conservative_rename(AT_FDCWD, q, AT_FDCWD, p) > 0); + assert_se(access(q, F_OK) < 0 && errno == ENOENT); +} + int main(int argc, char *argv[]) { test_setup_logging(LOG_INFO); @@ -852,6 +899,7 @@ int main(int argc, char *argv[]) { test_rename_noreplace(); test_chmod_and_chown(); test_path_is_encrypted(); + test_conservative_rename(); return 0; } From f3e1f00d03445911ee73729219cea88c8a70c612 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 18 Nov 2020 15:12:44 +0100 Subject: [PATCH 3/3] resolved: don't update resolv.conf snippets unnecessarily Fixes: #17577 --- src/resolve/resolved-resolv-conf.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-resolv-conf.c b/src/resolve/resolved-resolv-conf.c index 6fd9959daf..33fc435a9a 100644 --- a/src/resolve/resolved-resolv-conf.c +++ b/src/resolve/resolved-resolv-conf.c @@ -359,8 +359,9 @@ int manager_write_resolv_conf(Manager *m) { goto fail; } - if (rename(temp_path_stub, PRIVATE_STUB_RESOLV_CONF) < 0) - r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); + r = conservative_rename(AT_FDCWD, temp_path_stub, AT_FDCWD, PRIVATE_STUB_RESOLV_CONF); + if (r < 0) + log_error_errno(r, "Failed to move new %s into place: %m", PRIVATE_STUB_RESOLV_CONF); } else { r = symlink_atomic(basename(PRIVATE_UPLINK_RESOLV_CONF), PRIVATE_STUB_RESOLV_CONF); @@ -368,8 +369,9 @@ int manager_write_resolv_conf(Manager *m) { log_error_errno(r, "Failed to symlink %s: %m", PRIVATE_STUB_RESOLV_CONF); } - if (rename(temp_path_uplink, PRIVATE_UPLINK_RESOLV_CONF) < 0) - r = log_error_errno(errno, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); + r = conservative_rename(AT_FDCWD, temp_path_uplink, AT_FDCWD, PRIVATE_UPLINK_RESOLV_CONF); + if (r < 0) + log_error_errno(r, "Failed to move new %s into place: %m", PRIVATE_UPLINK_RESOLV_CONF); fail: if (r < 0) {