From 78ba8cf77037b93df499a72ef4f888112573a15c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 9 Mar 2018 22:44:47 +0100 Subject: [PATCH 1/7] copy: add brief comment to copy_bytes() explaining its return values --- src/basic/copy.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/basic/copy.c b/src/basic/copy.c index b64966c7d1..de71132f8d 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -65,6 +65,10 @@ int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { assert(fdf >= 0); assert(fdt >= 0); + /* Tries to copy bytes from the file descriptor 'fdf' to 'fdt' in the smartest possible way. Copies a maximum + * of 'max_bytes', which may be specified as UINT64_MAX, in which no maximum is applied. Returns negative on + * error, zero if EOF is hit before the bytes limit is hit and positive otherwise. */ + /* Try btrfs reflinks first. */ if ((copy_flags & COPY_REFLINK) && max_bytes == (uint64_t) -1 && From 5de6e1167314f5412285e904ec39299abbd352b5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 16:36:56 +0100 Subject: [PATCH 2/7] copy: tweak reflink logic in copy_bytes() a bit Let's use btrfs_clone_range() if partial copies are desired. And use btrfs_reflink() only for full-file reflinks. --- src/basic/copy.c | 60 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 52 insertions(+), 8 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index de71132f8d..3e6e18f495 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -69,15 +69,59 @@ int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { * of 'max_bytes', which may be specified as UINT64_MAX, in which no maximum is applied. Returns negative on * error, zero if EOF is hit before the bytes limit is hit and positive otherwise. */ - /* Try btrfs reflinks first. */ - if ((copy_flags & COPY_REFLINK) && - max_bytes == (uint64_t) -1 && - lseek(fdf, 0, SEEK_CUR) == 0 && - lseek(fdt, 0, SEEK_CUR) == 0) { + /* Try btrfs reflinks first. This only works on regular, seekable files, hence let's check the file offsets of + * source and destination first. */ + if ((copy_flags & COPY_REFLINK)) { + off_t foffset; - r = btrfs_reflink(fdf, fdt); - if (r >= 0) - return 0; /* we copied the whole thing, hence hit EOF, return 0 */ + foffset = lseek(fdf, 0, SEEK_CUR); + if (foffset >= 0) { + off_t toffset; + + toffset = lseek(fdt, 0, SEEK_CUR); + if (toffset >= 0) { + + if (foffset == 0 && toffset == 0 && max_bytes == UINT64_MAX) + r = btrfs_reflink(fdf, fdt); /* full file reflink */ + else + r = btrfs_clone_range(fdf, foffset, fdt, toffset, max_bytes == UINT64_MAX ? 0 : max_bytes); /* partial reflink */ + if (r >= 0) { + off_t t; + + /* This worked, yay! Now — to be fully correct — let's adjust the file pointers */ + if (max_bytes == UINT64_MAX) { + + /* We cloned to the end of the source file, let's position the read + * pointer there, and query it at the same time. */ + t = lseek(fdf, 0, SEEK_END); + if (t < 0) + return -errno; + if (t < foffset) + return -ESPIPE; + + /* Let's adjust the destination file write pointer by the same number + * of bytes. */ + t = lseek(fdt, toffset + (t - foffset), SEEK_SET); + if (t < 0) + return -errno; + + return 0; /* we copied the whole thing, hence hit EOF, return 0 */ + } else { + t = lseek(fdf, foffset + max_bytes, SEEK_SET); + if (t < 0) + return -errno; + + t = lseek(fdt, toffset + max_bytes, SEEK_SET); + if (t < 0) + return -errno; + + return 1; /* we copied only some number of bytes, which worked, but this means we didn't hit EOF, return 1 */ + } + } + + log_debug_errno(r, "Reflinking didn't work, falling back to non-reflink copying: %m"); + } + } } for (;;) { From 7a23c7fdfe82754bebecf82cdb7e1ecb44d6f23f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 16:41:42 +0100 Subject: [PATCH 3/7] copy: extend copy_bytes() a bit Optionally, when we copy between fds with simple read/write, let's return any remaining data we already read into the buffer if write fails. This is useful to allow callers to use the read data otherwise, perhaps implementing a different fallback for copying. --- src/basic/copy.c | 53 ++++++++++++++++++++++++++++++++++++++++++------ src/basic/copy.h | 5 ++++- 2 files changed, 51 insertions(+), 7 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index 3e6e18f495..15b004e1ae 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -57,7 +57,13 @@ static ssize_t try_copy_file_range(int fd_in, loff_t *off_in, return -errno; } -int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { +int copy_bytes_full( + int fdf, int fdt, + uint64_t max_bytes, + CopyFlags copy_flags, + void **ret_remains, + size_t *ret_remains_size) { + bool try_cfr = true, try_sendfile = true, try_splice = true; int r; size_t m = SSIZE_MAX; /* that is the maximum that sendfile and c_f_r accept */ @@ -67,7 +73,16 @@ int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { /* Tries to copy bytes from the file descriptor 'fdf' to 'fdt' in the smartest possible way. Copies a maximum * of 'max_bytes', which may be specified as UINT64_MAX, in which no maximum is applied. Returns negative on - * error, zero if EOF is hit before the bytes limit is hit and positive otherwise. */ + * error, zero if EOF is hit before the bytes limit is hit and positive otherwise. If the copy fails for some + * reason but we read but didn't yet write some data an ret_remains/ret_remains_size is not NULL, then it will + * be initialized with an allocated buffer containing this "remaining" data. Note that these two parameters are + * initialized with a valid buffer only on failure and only if there's actually data already read. Otherwise + * these parameters if non-NULL are set to NULL. */ + + if (ret_remains) + *ret_remains = NULL; + if (ret_remains_size) + *ret_remains_size = 0; /* Try btrfs reflinks first. This only works on regular, seekable files, hence let's check the file offsets of * source and destination first. */ @@ -185,7 +200,8 @@ int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { /* As a fallback just copy bits by hand */ { - uint8_t buf[MIN(m, COPY_BUFFER_SIZE)]; + uint8_t buf[MIN(m, COPY_BUFFER_SIZE)], *p = buf; + ssize_t z; n = read(fdf, buf, sizeof buf); if (n < 0) @@ -193,9 +209,34 @@ int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { if (n == 0) /* EOF */ break; - r = loop_write(fdt, buf, (size_t) n, false); - if (r < 0) - return r; + z = (size_t) n; + do { + ssize_t k; + + k = write(fdt, p, z); + if (k < 0) { + r = -errno; + + if (ret_remains) { + void *copy; + + copy = memdup(p, z); + if (!copy) + return -ENOMEM; + + *ret_remains = copy; + } + + if (ret_remains_size) + *ret_remains_size = z; + + return r; + } + + assert(k <= z); + z -= k; + p += k; + } while (z > 0); } next: diff --git a/src/basic/copy.h b/src/basic/copy.h index 880bd321cf..9ea1049f83 100644 --- a/src/basic/copy.h +++ b/src/basic/copy.h @@ -25,6 +25,9 @@ int copy_tree(const char *from, const char *to, uid_t override_uid, gid_t overri int copy_tree_at(int fdf, const char *from, int fdt, const char *to, uid_t override_uid, gid_t override_gid, CopyFlags copy_flags); int copy_directory_fd(int dirfd, const char *to, CopyFlags copy_flags); int copy_directory(const char *from, const char *to, CopyFlags copy_flags); -int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags); +int copy_bytes_full(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags, void **ret_remains, size_t *ret_remains_size); +static inline int copy_bytes(int fdf, int fdt, uint64_t max_bytes, CopyFlags copy_flags) { + return copy_bytes_full(fdf, fdt, max_bytes, copy_flags, NULL, NULL); +} int copy_times(int fdf, int fdt); int copy_xattr(int fdf, int fdt); From e0c5c7d8fa07f44c2b694c809703cb8a59f0dfed Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 17:44:15 +0100 Subject: [PATCH 4/7] copy: hide in copy_bytes() the strange way splice() handles O_NONBLOCK splice() ignores O_NONBLOCK on pipes but not on other fds. Let's handle that properly, and query O_ONBLOCK manually in that case, ensuring systematic behaviour in either case. --- src/basic/copy.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 3 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index 15b004e1ae..18a245a92c 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -57,6 +57,31 @@ static ssize_t try_copy_file_range(int fd_in, loff_t *off_in, return -errno; } +enum { + FD_IS_NO_PIPE, + FD_IS_BLOCKING_PIPE, + FD_IS_NONBLOCKING_PIPE, +}; + +static int fd_is_nonblock_pipe(int fd) { + struct stat st; + int flags; + + /* Checks whether the specified file descriptor refers to a pipe, and if so if is has O_NONBLOCK set. */ + + if (fstat(fd, &st) < 0) + return -errno; + + if (!S_ISFIFO(st.st_mode)) + return FD_IS_NO_PIPE; + + flags = fcntl(fd, F_GETFL); + if (flags < 0) + return -errno; + + return (flags & O_NONBLOCK) == O_NONBLOCK ? FD_IS_NONBLOCKING_PIPE : FD_IS_BLOCKING_PIPE; +} + int copy_bytes_full( int fdf, int fdt, uint64_t max_bytes, @@ -65,7 +90,7 @@ int copy_bytes_full( size_t *ret_remains_size) { bool try_cfr = true, try_sendfile = true, try_splice = true; - int r; + int r, nonblock_pipe = -1; size_t m = SSIZE_MAX; /* that is the maximum that sendfile and c_f_r accept */ assert(fdf >= 0); @@ -182,9 +207,51 @@ int copy_bytes_full( goto next; } - /* Then try splice, unless we already tried */ + /* Then try splice, unless we already tried. */ if (try_splice) { - n = splice(fdf, NULL, fdt, NULL, m, 0); + + /* splice()'s asynchronous I/O support is a bit weird. When it encounters a pipe file + * descriptor, then it will ignore its O_NONBLOCK flag and instead only honour the + * SPLICE_F_NONBLOCK flag specified in its flag parameter. Let's hide this behaviour here, and + * check if either of the specified fds are a pipe, and if so, let's pass the flag + * automatically, depending on O_NONBLOCK being set. + * + * Here's a twist though: when we use it to move data between two pipes of which one has + * O_NONBLOCK set and the other has not, then we have no individual control over O_NONBLOCK + * behaviour. Hence in that case we can't use splice() and still guarantee systematic + * O_NONBLOCK behaviour, hence don't. */ + + if (nonblock_pipe < 0) { + int a, b; + + /* Check if either of these fds is a pipe, and if so non-blocking or not */ + a = fd_is_nonblock_pipe(fdf); + if (a < 0) + return a; + + b = fd_is_nonblock_pipe(fdt); + if (b < 0) + return b; + + if ((a == FD_IS_NO_PIPE && b == FD_IS_NO_PIPE) || + (a == FD_IS_BLOCKING_PIPE && b == FD_IS_NONBLOCKING_PIPE) || + (a == FD_IS_NONBLOCKING_PIPE && b == FD_IS_BLOCKING_PIPE)) + + /* splice() only works if one of the fds is a pipe. If neither is, let's skip + * this step right-away. As mentioned above, if one of the two fds refers to a + * blocking pipe and the other to a non-blocking pipe, we can't use splice() + * either, hence don't try either. This hence means we can only use splice() if + * either only one of the two fds is a pipe, or if both are pipes with the same + * nonblocking flag setting. */ + + try_splice = false; + else + nonblock_pipe = a == FD_IS_NONBLOCKING_PIPE || b == FD_IS_NONBLOCKING_PIPE; + } + } + + if (try_splice) { + n = splice(fdf, NULL, fdt, NULL, m, nonblock_pipe ? SPLICE_F_NONBLOCK : 0); if (n < 0) { if (!IN_SET(errno, EINVAL, ENOSYS)) return -errno; From 75036dce9418024a05d20db3c5fb069aeccdec5d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 18:24:07 +0100 Subject: [PATCH 5/7] copy: drop _unlikely_() that isn't obviously the case If a tool only invokes copy_bytes() a single time the _unlikely_() will always be wrong, and is hence not useful. Let's drop it and let the compiler figure our what to do, instead of misleading it. Also, some coding style imprvoements. --- src/basic/copy.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index 18a245a92c..96d059d0e5 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -38,23 +38,25 @@ #define COPY_BUFFER_SIZE (16*1024u) -static ssize_t try_copy_file_range(int fd_in, loff_t *off_in, - int fd_out, loff_t *off_out, - size_t len, - unsigned int flags) { +static ssize_t try_copy_file_range( + int fd_in, loff_t *off_in, + int fd_out, loff_t *off_out, + size_t len, + unsigned int flags) { + static int have = -1; ssize_t r; - if (have == false) + if (have == 0) return -ENOSYS; r = copy_file_range(fd_in, off_in, fd_out, off_out, len, flags); - if (_unlikely_(have < 0)) + if (have < 0) have = r >= 0 || errno != ENOSYS; - if (r >= 0) - return r; - else + if (r < 0) return -errno; + + return r; } enum { From dd641ad1b8c196208aaf74ccfd1f0a0598745484 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 18:26:58 +0100 Subject: [PATCH 6/7] copy: reduce number of checks We check max_bytes twice here, let's simplify that, and reduce one level of indentation. --- src/basic/copy.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/basic/copy.c b/src/basic/copy.c index 96d059d0e5..55394871fd 100644 --- a/src/basic/copy.c +++ b/src/basic/copy.c @@ -169,13 +169,11 @@ int copy_bytes_full( for (;;) { ssize_t n; - if (max_bytes != (uint64_t) -1) { - if (max_bytes <= 0) - return 1; /* return > 0 if we hit the max_bytes limit */ + if (max_bytes <= 0) + return 1; /* return > 0 if we hit the max_bytes limit */ - if (m > max_bytes) - m = max_bytes; - } + if (max_bytes != UINT64_MAX && m > max_bytes) + m = max_bytes; /* First try copy_file_range(), unless we already tried */ if (try_cfr) { From 39c3905646361b5727b01c01401d5a5914aec63f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 23 Mar 2018 18:27:31 +0100 Subject: [PATCH 7/7] copy: rearrange flags field definition Let's use the usual 1U << 0,1,2,3,4 spelling for definiting flags enums. --- src/basic/copy.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/basic/copy.h b/src/basic/copy.h index 9ea1049f83..0c50bcef50 100644 --- a/src/basic/copy.h +++ b/src/basic/copy.h @@ -13,9 +13,9 @@ #include typedef enum CopyFlags { - COPY_REFLINK = 0x1, /* try to reflink */ - COPY_MERGE = 0x2, /* merge existing trees with our new one to copy */ - COPY_REPLACE = 0x4, /* replace an existing file if there's one */ + COPY_REFLINK = 1U << 0, /* Try to reflink */ + COPY_MERGE = 1U << 1, /* Merge existing trees with our new one to copy */ + COPY_REPLACE = 1U << 2, /* Replace an existing file if there's one */ } CopyFlags; int copy_file_fd(const char *from, int to, CopyFlags copy_flags);