From 519216b71fb3c95c0dfbc9b09a4aa4a67179939e Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Thu, 22 Aug 2024 17:23:58 +0200 Subject: [PATCH 1/5] Revert "tree-wide: Don't explicity disable copy-on-write when copying images" Let's still try to disable COW after copying. It won't do much, but it doesn't hurt either. See https://github.com/systemd/systemd/pull/33825/files#r1727288871. This reverts commit 42e9288180ce37abd86c58774950636ebce35432. --- src/core/exec-invoke.c | 10 ++++++++++ src/nspawn/nspawn.c | 6 ++++-- src/shared/discover-image.c | 3 ++- 3 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index af5552379e..57f96f7eab 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -2891,6 +2891,16 @@ static int setup_ephemeral( if (fd < 0) return log_debug_errno(fd, "Failed to copy image %s to %s: %m", *root_image, new_root); + + /* A root image might be subject to lots of random writes so let's try to disable COW on it + * which tends to not perform well in combination with lots of random writes. + * + * Note: btrfs actually isn't impressed by us setting the flag after making the reflink'ed + * copy, but we at least want to make the intention clear. + */ + r = chattr_fd(fd, FS_NOCOW_FL, FS_NOCOW_FL, NULL); + if (r < 0) + log_debug_errno(r, "Failed to disable copy-on-write for %s, ignoring: %m", new_root); } else { assert(*root_directory); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index 9cd52e06c0..c182069031 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -6072,8 +6072,10 @@ static int run(int argc, char *argv[]) { { BLOCK_SIGNALS(SIGINT); - r = copy_file(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, - COPY_REFLINK|COPY_CRTIME|COPY_SIGINT); + r = copy_file_full(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, + FS_NOCOW_FL, FS_NOCOW_FL, + COPY_REFLINK|COPY_CRTIME|COPY_SIGINT, + NULL, NULL); } if (r == -EINTR) { log_error_errno(r, "Interrupted while copying image file to %s, removed again.", np); diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c index 1386452203..dd78a013aa 100644 --- a/src/shared/discover-image.c +++ b/src/shared/discover-image.c @@ -1205,7 +1205,8 @@ int image_clone(Image *i, const char *new_name, bool read_only) { case IMAGE_RAW: new_path = strjoina("/var/lib/machines/", new_name, ".raw"); - r = copy_file_atomic(i->path, new_path, read_only ? 0444 : 0644, COPY_REFLINK|COPY_CRTIME); + r = copy_file_atomic_full(i->path, new_path, read_only ? 0444 : 0644, FS_NOCOW_FL, FS_NOCOW_FL, + COPY_REFLINK|COPY_CRTIME, NULL, NULL); break; case IMAGE_BLOCK: From 74c9606025c525631c7d9f05512875b92b078375 Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Fri, 23 Aug 2024 12:24:16 +0200 Subject: [PATCH 2/5] Revert "copy: Copy file attributes as well" This reverts commit 2356104efcc599439c887b087da83fbeca1fa9a6. --- src/shared/copy.c | 41 ++++++++--------------------------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index 8e240dc434..abebe2d2f2 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -798,7 +798,6 @@ static int fd_copy_regular( void *userdata) { _cleanup_close_ int fdf = -EBADF, fdt = -EBADF; - unsigned attrs = 0; int r, q; assert(st); @@ -814,10 +813,6 @@ static int fd_copy_regular( if (fdf < 0) return fdf; - r = read_attr_fd(fdf, &attrs); - if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) - return r; - if (copy_flags & COPY_MAC_CREATE) { r = mac_selinux_create_file_prepare_at(dt, to, S_IFREG); if (r < 0) @@ -829,9 +824,6 @@ static int fd_copy_regular( if (fdt < 0) return -errno; - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); - r = copy_bytes_full(fdf, fdt, UINT64_MAX, copy_flags, NULL, NULL, progress, userdata); if (r < 0) goto fail; @@ -847,9 +839,6 @@ static int fd_copy_regular( (void) futimens(fdt, (struct timespec[]) { st->st_atim, st->st_mtim }); (void) copy_xattr(fdf, NULL, fdt, NULL, copy_flags); - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); - if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) { r = fd_verify_linked(fdf); if (r < 0) @@ -1430,7 +1419,6 @@ int copy_file_at_full( _cleanup_close_ int fdf = -EBADF, fdt = -EBADF; struct stat st; - unsigned attrs = 0; int r; assert(dir_fdf >= 0 || dir_fdf == AT_FDCWD); @@ -1448,10 +1436,6 @@ int copy_file_at_full( if (r < 0) return r; - r = read_attr_at(dir_fdf, from, &attrs); - if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) - return r; - WITH_UMASK(0000) { fdt = xopenat_lock_full(dir_fdt, to, flags|O_WRONLY|O_CREAT|O_CLOEXEC|O_NOCTTY, @@ -1468,10 +1452,8 @@ int copy_file_at_full( goto fail; } - attrs = (attrs & ~chattr_mask) | (chattr_flags & chattr_mask); - - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + if (chattr_mask != 0) + (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); r = copy_bytes_full(fdf, fdt, UINT64_MAX, copy_flags & ~COPY_LOCK_BSD, NULL, NULL, progress_bytes, userdata); if (r < 0) @@ -1486,8 +1468,8 @@ int copy_file_at_full( goto fail; } - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + if (chattr_mask != 0) + (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); if (copy_flags & (COPY_FSYNC|COPY_FSYNC_FULL)) { if (fsync(fdt) < 0) { @@ -1532,7 +1514,6 @@ int copy_file_atomic_at_full( _cleanup_(unlink_and_freep) char *t = NULL; _cleanup_close_ int fdt = -EBADF; - unsigned attrs = 0; int r; assert(to); @@ -1549,14 +1530,8 @@ int copy_file_atomic_at_full( if (fdt < 0) return fdt; - r = read_attr_at(dir_fdf, from, &attrs); - if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) - return r; - - attrs = (attrs & ~chattr_mask) | (chattr_flags & chattr_mask); - - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + if (chattr_mask != 0) + (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); r = copy_file_fd_at_full(dir_fdf, from, fdt, copy_flags, progress_bytes, userdata); if (r < 0) @@ -1577,8 +1552,8 @@ int copy_file_atomic_at_full( t = mfree(t); - if (attrs != 0) - (void) chattr_full(fdt, NULL, attrs, ~CHATTR_EARLY_FL, NULL, NULL, CHATTR_FALLBACK_BITWISE); + if (chattr_mask != 0) + (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); r = close_nointr(TAKE_FD(fdt)); /* even if this fails, the fd is now invalidated */ if (r < 0) From 07862c9fc2becdc4bbe112b023591d2ab2dd032b Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Mon, 26 Aug 2024 14:14:07 +0200 Subject: [PATCH 3/5] chattr-util: Optimize read_attr_at() Let's make sure we only reopen O_PATH file descriptors. --- src/basic/chattr-util.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/basic/chattr-util.c b/src/basic/chattr-util.c index 49831a26ce..13d46f26aa 100644 --- a/src/basic/chattr-util.c +++ b/src/basic/chattr-util.c @@ -149,14 +149,23 @@ int read_attr_fd(int fd, unsigned *ret) { } int read_attr_at(int dir_fd, const char *path, unsigned *ret) { - _cleanup_close_ int fd = -EBADF; + _cleanup_close_ int fd_close = -EBADF; + int fd; assert(dir_fd >= 0 || dir_fd == AT_FDCWD); assert(ret); - fd = xopenat(dir_fd, path, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); - if (fd < 0) - return fd; + if (isempty(path)) { + fd = fd_reopen_condition(dir_fd, O_RDONLY|O_CLOEXEC, O_PATH, &fd_close); /* drop O_PATH if it is set */ + if (fd < 0) + return fd; + } else { + fd_close = xopenat(dir_fd, path, O_RDONLY|O_CLOEXEC|O_NOCTTY|O_NOFOLLOW); + if (fd_close < 0) + return fd_close; + + fd = fd_close; + } return read_attr_fd(fd, ret); } From 8af3b12fe031f313d60ce91f61f1685c25b76afc Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Fri, 23 Aug 2024 13:04:33 +0200 Subject: [PATCH 4/5] copy: Copy nocow flag by default Unless otherwise requested, if we're going to copy a nocow file, make the target file nocow as well. Aside from keeping the performance characteristics of the cow or nocow file intact, reflinking also only works from cow to cow or nocow to nocow files. Reflinking from cow to nocow or nocow to cow files does not work and can easily lead to unexpected copies for users, so by keeping the nocow bit intact across copies by default we also make sure reflinks always work. --- src/shared/copy.c | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/src/shared/copy.c b/src/shared/copy.c index abebe2d2f2..8af1cb5ce5 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -765,6 +765,35 @@ static int memorize_hardlink( return 1; } +static int prepare_nocow(int fdf, const char *from, int fdt, unsigned *chattr_mask, unsigned *chattr_flags) { + unsigned attrs = 0; + int r; + + assert(fdf >= 0 || fdf == AT_FDCWD); + assert(fdt >= 0); + assert(!!chattr_mask == !!chattr_flags); + + /* If caller explicitly requested NOCOW to be set or unset, let's not interfere. */ + if (chattr_mask && FLAGS_SET(*chattr_mask, FS_NOCOW_FL)) + return 0; + + r = read_attr_at(fdf, from, &attrs); + if (r < 0 && !ERRNO_IS_NOT_SUPPORTED(r) && r != -ELOOP) /* If the source is a symlink we get ELOOP */ + return r; + + if (FLAGS_SET(attrs, FS_NOCOW_FL)) { + if (chattr_mask && chattr_flags) { + *chattr_mask |= FS_NOCOW_FL; + *chattr_flags |= FS_NOCOW_FL; + } else + /* If the NOCOW flag is set on the source, make the copy NOCOW as well. If the source + * is not NOCOW, don't do anything in particular with the copy. */ + (void) chattr_fd(fdt, FS_NOCOW_FL, FS_NOCOW_FL, /*previous=*/ NULL); + } + + return 0; +} + static int fd_copy_tree_generic( int df, const char *from, @@ -824,6 +853,10 @@ static int fd_copy_regular( if (fdt < 0) return -errno; + r = prepare_nocow(fdf, /*from=*/ NULL, fdt, /*chattr_mask=*/ NULL, /*chattr_flags=*/ NULL); + if (r < 0) + return r; + r = copy_bytes_full(fdf, fdt, UINT64_MAX, copy_flags, NULL, NULL, progress, userdata); if (r < 0) goto fail; @@ -1452,6 +1485,10 @@ int copy_file_at_full( goto fail; } + r = prepare_nocow(fdf, /*from=*/ NULL, fdt, &chattr_mask, &chattr_flags); + if (r < 0) + return r; + if (chattr_mask != 0) (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); @@ -1530,6 +1567,10 @@ int copy_file_atomic_at_full( if (fdt < 0) return fdt; + r = prepare_nocow(dir_fdf, from, fdt, &chattr_mask, &chattr_flags); + if (r < 0) + return r; + if (chattr_mask != 0) (void) chattr_fd(fdt, chattr_flags, chattr_mask & CHATTR_EARLY_FL, NULL); From b1cfa9308061d5aacc65ef6d898ce605edde417d Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Fri, 23 Aug 2024 13:40:40 +0200 Subject: [PATCH 5/5] copy: Introduce COPY_NOCOW_AFTER and use it when copying images When dealing with copying COW images, we have to make a tradeoff: - Either we don't touch the NOCOW bit on the copied file COW and get an instant copy because we're able to reflink, but we might get reduced performance if the source file was COW as COW files and lots of random writes don't play well together. - Or we force NOCOW for the copied file, which means we have to do a full copy as reflinking from COW files to NOCOW files or vice versa is not supported. In exec-invoke.c, we've opted for the first option. In nspawn.c and discover-image.c, we've opted for the second option. In nspawn, this applies to the --ephemeral option to make ephemeral copies. In discover-image.c, this applies to cloning images into /var/lib/machines. Both these features might be used to run many machines of the same original image. We really don't want to force a full copy onto users in these scenarios when they're expecting reflink behavior, leading to them running out of disk space. Instead, degraded performance in their machines is a much less severe issue, which they will discover on their own if it affects them, at which point they can make their original image NOCOW at which point they'll get both the reflinks and better performance. Given the above reasoning, let's switch nspawn.c and discover-image.c to use COPY_NOCOW_AFTER as well instead of enabling NOCOW upfront and forcing a copy if the original source image is COW. --- src/core/exec-invoke.c | 19 ++----------------- src/nspawn/nspawn.c | 7 +++---- src/shared/copy.c | 10 ++++++---- src/shared/copy.h | 8 ++++++++ src/shared/discover-image.c | 4 ++-- 5 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/core/exec-invoke.c b/src/core/exec-invoke.c index 57f96f7eab..1a2595c8bf 100644 --- a/src/core/exec-invoke.c +++ b/src/core/exec-invoke.c @@ -2881,26 +2881,11 @@ static int setup_ephemeral( if (*root_image) { log_debug("Making ephemeral copy of %s to %s", *root_image, new_root); - fd = copy_file(*root_image, - new_root, - O_EXCL, - 0600, - COPY_LOCK_BSD| - COPY_REFLINK| - COPY_CRTIME); + fd = copy_file(*root_image, new_root, O_EXCL, 0600, + COPY_LOCK_BSD|COPY_REFLINK|COPY_CRTIME|COPY_NOCOW_AFTER); if (fd < 0) return log_debug_errno(fd, "Failed to copy image %s to %s: %m", *root_image, new_root); - - /* A root image might be subject to lots of random writes so let's try to disable COW on it - * which tends to not perform well in combination with lots of random writes. - * - * Note: btrfs actually isn't impressed by us setting the flag after making the reflink'ed - * copy, but we at least want to make the intention clear. - */ - r = chattr_fd(fd, FS_NOCOW_FL, FS_NOCOW_FL, NULL); - if (r < 0) - log_debug_errno(r, "Failed to disable copy-on-write for %s, ignoring: %m", new_root); } else { assert(*root_directory); diff --git a/src/nspawn/nspawn.c b/src/nspawn/nspawn.c index c182069031..a27dc8861a 100644 --- a/src/nspawn/nspawn.c +++ b/src/nspawn/nspawn.c @@ -36,6 +36,7 @@ #include "capability-util.h" #include "cgroup-util.h" #include "chase.h" +#include "chattr-util.h" #include "common-signal.h" #include "copy.h" #include "cpu-set-util.h" @@ -6072,10 +6073,8 @@ static int run(int argc, char *argv[]) { { BLOCK_SIGNALS(SIGINT); - r = copy_file_full(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, - FS_NOCOW_FL, FS_NOCOW_FL, - COPY_REFLINK|COPY_CRTIME|COPY_SIGINT, - NULL, NULL); + r = copy_file(arg_image, np, O_EXCL, arg_read_only ? 0400 : 0600, + COPY_REFLINK|COPY_CRTIME|COPY_SIGINT|COPY_NOCOW_AFTER); } if (r == -EINTR) { log_error_errno(r, "Interrupted while copying image file to %s, removed again.", np); diff --git a/src/shared/copy.c b/src/shared/copy.c index 8af1cb5ce5..836753cc75 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -1505,8 +1505,9 @@ int copy_file_at_full( goto fail; } - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); + unsigned nocow = FLAGS_SET(copy_flags, COPY_NOCOW_AFTER) ? FS_NOCOW_FL : 0; + if ((chattr_mask | nocow) != 0) + (void) chattr_fd(fdt, chattr_flags | nocow, (chattr_mask & ~CHATTR_EARLY_FL) | nocow, NULL); if (copy_flags & (COPY_FSYNC|COPY_FSYNC_FULL)) { if (fsync(fdt) < 0) { @@ -1593,8 +1594,9 @@ int copy_file_atomic_at_full( t = mfree(t); - if (chattr_mask != 0) - (void) chattr_fd(fdt, chattr_flags, chattr_mask & ~CHATTR_EARLY_FL, NULL); + unsigned nocow = FLAGS_SET(copy_flags, COPY_NOCOW_AFTER) ? FS_NOCOW_FL : 0; + if ((chattr_mask | nocow) != 0) + (void) chattr_fd(fdt, chattr_flags | nocow, (chattr_mask & ~CHATTR_EARLY_FL) | nocow, NULL); r = close_nointr(TAKE_FD(fdt)); /* even if this fails, the fd is now invalidated */ if (r < 0) diff --git a/src/shared/copy.h b/src/shared/copy.h index db95738b80..4eea560bed 100644 --- a/src/shared/copy.h +++ b/src/shared/copy.h @@ -32,6 +32,14 @@ typedef enum CopyFlags { COPY_LOCK_BSD = 1 << 17, /* Return a BSD exclusively locked file descriptor referring to the copied image/directory. */ COPY_VERIFY_LINKED = 1 << 18, /* Check the source file is still linked after copying. */ COPY_RESTORE_DIRECTORY_TIMESTAMPS = 1 << 19, /* Make sure existing directory timestamps don't change during copying. */ + /* A root image might be subject to lots of random writes so we provide a flag to try to disable COW + * on a copied file which tends to not perform well in combination with lots of random writes. + * + * Note: btrfs actually isn't impressed by us setting the flag after making the copy, but this at + * least makes the intention clear. We don't want to unconditionally set the flag before doing the + * copy because reflinking from COW to NOCOW files is not supported. + */ + COPY_NOCOW_AFTER = 1 << 20, } CopyFlags; typedef enum DenyType { diff --git a/src/shared/discover-image.c b/src/shared/discover-image.c index dd78a013aa..10b2405cc3 100644 --- a/src/shared/discover-image.c +++ b/src/shared/discover-image.c @@ -1205,8 +1205,8 @@ int image_clone(Image *i, const char *new_name, bool read_only) { case IMAGE_RAW: new_path = strjoina("/var/lib/machines/", new_name, ".raw"); - r = copy_file_atomic_full(i->path, new_path, read_only ? 0444 : 0644, FS_NOCOW_FL, FS_NOCOW_FL, - COPY_REFLINK|COPY_CRTIME, NULL, NULL); + r = copy_file_atomic(i->path, new_path, read_only ? 0444 : 0644, + COPY_REFLINK|COPY_CRTIME|COPY_NOCOW_AFTER); break; case IMAGE_BLOCK: