From 3a4d0f4684f4653338c4756c8a1abfc6b5738467 Mon Sep 17 00:00:00 2001 From: Colin Walters <walters@verbum.org> Date: Thu, 27 Apr 2017 13:51:31 -0400 Subject: [PATCH] fdio: Expose glnx_regfile_copy_bytes(), rewrite: GNU style, POSIX errno NOTE: This changes the error handling API of `glnx_loop_write()` to be "old school POSIX" instead of "systemd". In ostree in a few places we use `g_output_stream_splice()`. I thought this would use `splice()`, but actually it doesn't today. They also, if a cancellable is provided, end up dropping into `poll()` for every read and write. (In addition to copying data to/from userspace). My opinion on this is - for *local files* that's dumb. In the big picture, you really only need cancellation when copying gigabytes. Down the line, we could perhaps add a `glnx_copy_bytes_cancellable()` that only did that check e.g. every gigabyte of copied data. And when we do that we should use `g_cancellable_set_error_if_cancelled()` rather than a `poll()` with the regular file FD, since regular files are *always* readable and writable. For my use case with rpm-ostree though, we don't have gigabyte sized files, and seeing all of the `poll()` calls in strace is annoying. So let's have the non-cancellable file copying API that's modern and uses both reflink and `sendfile()` if available, in that order. My plan at some point once this is tested more is to migrate this code into GLib. Note that in order to keep our APIs consistent, I switched the systemd-imported code to "old school POSIX" error conventions. Otherwise we'd have *3* (POSIX, systemd, and GError) and particularly given the first two are easily confused, it'd be a recipe for bugs. --- glnx-fdio.c | 202 +++++++++++++++++++++++++++++----------------------- glnx-fdio.h | 3 + 2 files changed, 114 insertions(+), 91 deletions(-) diff --git a/glnx-fdio.c b/glnx-fdio.c index cda627a0a..df4c8518e 100644 --- a/glnx-fdio.c +++ b/glnx-fdio.c @@ -554,117 +554,141 @@ copy_symlink_at (int src_dfd, #define COPY_BUFFER_SIZE (16*1024) -/* From systemd */ +/* Most of the code below is from systemd, but has been reindented to GNU style, + * and changed to use POSIX error conventions (return -1, set errno) to more + * conveniently fit in with the rest of libglnx. + */ static int btrfs_reflink(int infd, int outfd) { - int r; - g_return_val_if_fail(infd >= 0, -1); g_return_val_if_fail(outfd >= 0, -1); - r = ioctl(outfd, BTRFS_IOC_CLONE, infd); - if (r < 0) - return -errno; - - return 0; + return ioctl (outfd, BTRFS_IOC_CLONE, infd); } -int glnx_loop_write(int fd, const void *buf, size_t nbytes) { - const uint8_t *p = buf; - - g_return_val_if_fail(fd >= 0, -1); - g_return_val_if_fail(buf, -1); +/* Like write(), but loop until @nbytes are written, or an error + * occurs. + * + * On error, -1 is returned an @errno is set. NOTE: This is an + * API change from previous versions of this function. + */ +int +glnx_loop_write(int fd, const void *buf, size_t nbytes) +{ + const uint8_t *p = buf; - errno = 0; + g_return_val_if_fail(fd >= 0, -1); + g_return_val_if_fail(buf, -1); - while (nbytes > 0) { - ssize_t k; + errno = 0; - k = write(fd, p, nbytes); - if (k < 0) { - if (errno == EINTR) - continue; + while (nbytes > 0) + { + ssize_t k; - return -errno; - } + k = write(fd, p, nbytes); + if (k < 0) + { + if (errno == EINTR) + continue; - if (k == 0) /* Can't really happen */ - return -EIO; + return -1; + } - p += k; - nbytes -= k; + if (k == 0) /* Can't really happen */ + { + errno = EIO; + return -1; } - return 0; -} + p += k; + nbytes -= k; + } -static int copy_bytes(int fdf, int fdt, off_t max_bytes, bool try_reflink) { - bool try_sendfile = true; - int r; + return 0; +} - g_return_val_if_fail (fdf >= 0, -1); - g_return_val_if_fail (fdt >= 0, -1); +/* Read from @fdf until EOF, writing to @fdt. If @try_reflink is %TRUE, + * attempt to use any "reflink" functionality; see e.g. https://lwn.net/Articles/331808/ + * + * The file descriptor @fdf must refer to a regular file. + * + * If provided, @max_bytes specifies the maximum number of bytes to read from @fdf. + * On error, this function returns `-1` and @errno will be set. + */ +int +glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes, gboolean try_reflink) +{ + bool try_sendfile = true; + int r; - /* Try btrfs reflinks first. */ - if (try_reflink && max_bytes == (off_t) -1) { - r = btrfs_reflink(fdf, fdt); - if (r >= 0) - return r; - } + g_return_val_if_fail (fdf >= 0, -1); + g_return_val_if_fail (fdt >= 0, -1); + g_return_val_if_fail (max_bytes >= -1, -1); - for (;;) { - size_t m = COPY_BUFFER_SIZE; - ssize_t n; + /* Try btrfs reflinks first. */ + if (try_reflink && max_bytes == (off_t) -1) + { + r = btrfs_reflink(fdf, fdt); + if (r >= 0) + return 0; + /* Fall through */ + } - if (max_bytes != (off_t) -1) { + while (TRUE) + { + size_t m = COPY_BUFFER_SIZE; + ssize_t n; - if (max_bytes <= 0) - return -EFBIG; + if (max_bytes != (off_t) -1) + { + if ((off_t) m > max_bytes) + m = (size_t) max_bytes; + } - if ((off_t) m > max_bytes) - m = (size_t) max_bytes; - } + /* First try sendfile(), unless we already tried */ + if (try_sendfile) + { + n = sendfile (fdt, fdf, NULL, m); + if (n < 0) + { + if (errno != EINVAL && errno != ENOSYS) + return -1; - /* First try sendfile(), unless we already tried */ - if (try_sendfile) { - - n = sendfile(fdt, fdf, NULL, m); - if (n < 0) { - if (errno != EINVAL && errno != ENOSYS) - return -errno; - - try_sendfile = false; - /* use fallback below */ - } else if (n == 0) /* EOF */ - break; - else if (n > 0) - /* Succcess! */ - goto next; - } + try_sendfile = false; + /* use fallback below */ + } + else if (n == 0) /* EOF */ + break; + else if (n > 0) + /* Succcess! */ + goto next; + } - /* As a fallback just copy bits by hand */ - { - char buf[m]; + /* As a fallback just copy bits by hand */ + { char buf[m]; - n = read(fdf, buf, m); - if (n < 0) - return -errno; - if (n == 0) /* EOF */ - break; + n = read (fdf, buf, m); + if (n < 0) + return -1; + if (n == 0) /* EOF */ + break; - r = glnx_loop_write(fdt, buf, (size_t) n); - if (r < 0) - return r; - } + if (glnx_loop_write (fdt, buf, (size_t) n) < 0) + return -1; + } - next: - if (max_bytes != (off_t) -1) { - g_assert(max_bytes >= n); - max_bytes -= n; - } + next: + if (max_bytes != (off_t) -1) + { + g_assert(max_bytes >= n); + max_bytes -= n; + if (max_bytes == 0) + break; } + } - return 0; + return 0; } /** @@ -752,10 +776,9 @@ glnx_file_copy_at (int src_dfd, goto out; } - r = copy_bytes (src_fd, dest_fd, (off_t) -1, TRUE); + r = glnx_regfile_copy_bytes (src_fd, dest_fd, (off_t) -1, TRUE); if (r < 0) { - errno = -r; glnx_set_error_from_errno (error); goto out; } @@ -905,17 +928,14 @@ glnx_file_replace_contents_with_perms_at (int dfd, } } - if ((r = glnx_loop_write (fd, buf, len)) != 0) - { - errno = -r; - return glnx_throw_errno (error); - } - + if (glnx_loop_write (fd, buf, len) < 0) + return glnx_throw_errno (error); + if (!(flags & GLNX_FILE_REPLACE_NODATASYNC)) { struct stat stbuf; gboolean do_sync; - + if (fstatat (dfd, subpath, &stbuf, AT_SYMLINK_NOFOLLOW) != 0) { if (errno != ENOENT) diff --git a/glnx-fdio.h b/glnx-fdio.h index 56d0b78ed..deff22b2c 100644 --- a/glnx-fdio.h +++ b/glnx-fdio.h @@ -131,6 +131,9 @@ glnx_readlinkat_malloc (int dfd, int glnx_loop_write (int fd, const void *buf, size_t nbytes); +int +glnx_regfile_copy_bytes (int fdf, int fdt, off_t max_bytes, gboolean try_reflink); + typedef enum { GLNX_FILE_COPY_OVERWRITE = (1 << 0), GLNX_FILE_COPY_NOXATTRS = (1 << 1), -- GitLab