From 22ed8700c7d70419bc74bf1ce187b950ee7b65fd Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 May 2024 12:02:42 +0200 Subject: [PATCH 1/3] copy: rework how we determine the number of bytes to copy in copy_bytes_full() Let's freshly calculate "m" on each iteration and always start with the maximum size we can. If sendfile() is used we must adhere to its limit of SSIZE_MAX minus the current offset. Otherwise we can copy more, i.e. SSIZE_MAX without any restrictions. Also, if we get too close to having copied SSIZE_MAX, let's turn off sendfile() for the rest. --- src/shared/copy.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index 8389774db75..6d984894f97 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -162,9 +162,9 @@ int copy_bytes_full( void *userdata) { _cleanup_close_ int fdf_opened = -EBADF, fdt_opened = -EBADF; - bool try_cfr = true, try_sendfile = true, try_splice = true, copied_something = false; + bool try_cfr = true, try_sendfile = true, try_splice = true; + uint64_t copied_total = 0; int r, nonblock_pipe = -1; - size_t m = SSIZE_MAX; /* that is the maximum that sendfile and c_f_r accept */ assert(fdf >= 0); assert(fdt >= 0); @@ -264,6 +264,7 @@ int copy_bytes_full( for (;;) { ssize_t n; + size_t m; if (max_bytes <= 0) break; @@ -272,6 +273,14 @@ int copy_bytes_full( if (r < 0) return r; + /* sendfile() accepts at most SSIZE_MAX-offset bytes to copy, hence let's subtract how much + * copied so far from SSIZE_MAX as maximum of what we want to copy. */ + if (try_sendfile) { + assert(copied_total < SSIZE_MAX); + m = (uint64_t) SSIZE_MAX - copied_total; + } else + m = SSIZE_MAX; + if (max_bytes != UINT64_MAX && m > max_bytes) m = max_bytes; @@ -342,7 +351,7 @@ int copy_bytes_full( /* use fallback below */ } else if (n == 0) { /* likely EOF */ - if (copied_something) + if (copied_total > 0) break; /* So, we hit EOF immediately, without having copied a single byte. This @@ -369,7 +378,7 @@ int copy_bytes_full( /* use fallback below */ } else if (n == 0) { /* likely EOF */ - if (copied_something) + if (copied_total > 0) break; try_sendfile = try_splice = false; /* same logic as above for copy_file_range() */ @@ -432,7 +441,7 @@ int copy_bytes_full( /* use fallback below */ } else if (n == 0) { /* likely EOF */ - if (copied_something) + if (copied_total > 0) break; try_splice = false; /* same logic as above for copy_file_range() + sendfile() */ @@ -483,6 +492,12 @@ int copy_bytes_full( } next: + copied_total += n; + + /* Disable sendfile() in case we are getting too close to it's SSIZE_MAX-offset limit */ + if (copied_total > SSIZE_MAX - COPY_BUFFER_SIZE) + try_sendfile = false; + if (progress) { r = progress(n, userdata); if (r < 0) @@ -493,13 +508,6 @@ int copy_bytes_full( assert(max_bytes >= (uint64_t) n); max_bytes -= n; } - - /* sendfile accepts at most SSIZE_MAX-offset bytes to copy, so reduce our maximum by the - * amount we already copied, but don't go below our copy buffer size, unless we are close the - * limit of bytes we are allowed to copy. */ - m = MAX(MIN(COPY_BUFFER_SIZE, max_bytes), m - n); - - copied_something = true; } if (FLAGS_SET(copy_flags, COPY_VERIFY_LINKED)) { From 0de442ac3115ce1719b32374f819d5c4ae4354db Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 May 2024 12:17:00 +0200 Subject: [PATCH 2/3] copy: when a progress callback is provided, never copy more than 1M per iteration Otherwise if we have to fill GB of data we might never call into the callback, hence put some limit on how much to copy per iteration. --- src/shared/copy.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/shared/copy.c b/src/shared/copy.c index 6d984894f97..def8bd1933e 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -41,8 +41,13 @@ #include "user-util.h" #include "xattr-util.h" +/* If we copy via a userspace buffer, size it to 16K */ #define COPY_BUFFER_SIZE (16U*1024U) +/* If a byte progress function is specified during copying, never try to copy more than 1M, so that we can + * reasonably call the progress function still */ +#define PROGRESS_STEP_SIZE (1U*U64_MB) + /* A safety net for descending recursively into file system trees to copy. On Linux PATH_MAX is 4096, which means the * deepest valid path one can build is around 2048, which we hence use as a safety net here, to not spin endlessly in * case of bind mount cycles and suchlike. */ @@ -284,6 +289,9 @@ int copy_bytes_full( if (max_bytes != UINT64_MAX && m > max_bytes) m = max_bytes; + if (progress && m > PROGRESS_STEP_SIZE) + m = PROGRESS_STEP_SIZE; + if (copy_flags & COPY_HOLES) { off_t c, e; From 9b2d9b07a17158b3a2830e1b77ae77534eed3787 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 24 May 2024 12:18:23 +0200 Subject: [PATCH 3/3] copy: increase copy buffer from 16K to 64K In my tests here this tremendously speeds up things when initializing a 1G file from /dev/urandom --- src/shared/copy.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/copy.c b/src/shared/copy.c index def8bd1933e..ac1a9b5933a 100644 --- a/src/shared/copy.c +++ b/src/shared/copy.c @@ -41,8 +41,8 @@ #include "user-util.h" #include "xattr-util.h" -/* If we copy via a userspace buffer, size it to 16K */ -#define COPY_BUFFER_SIZE (16U*1024U) +/* If we copy via a userspace buffer, size it to 64K */ +#define COPY_BUFFER_SIZE (64U*U64_KB) /* If a byte progress function is specified during copying, never try to copy more than 1M, so that we can * reasonably call the progress function still */