Jim Meyering wrote: > Jeff liu wrote: >> Now make check passed against the following combination: >> 1. Refresh installed host in Ubuntu10.0.4, >> filefrag comes from E2fsprogs 1.41.11 && Kernel: 2.6.32-16 >> 2. filefrag in e2fsprogs-1.4.12 && kernel-2.6.36. > [passes] > > Glad to here it passes for you, now. > FYI, I have spent pretty much time on cp over the last > couple days, factoring out the hole-inducing code and > making extent_copy use it. Part of the motivation was > to fix cp --sparse=always, which was broken on the branch. > It would not induce holes when going through extent_copy. > I've added a couple more tests and will post the series as > soon I've cleaned things up a little more.
Here are 9 more patches, just pushed to the fiemap-copy-2 branch: http://git.savannah.gnu.org/cgit/coreutils.git/log/?h=fiemap-copy-2 The first and last add tests, and the others consolidate, clean up, and fix a few bugs. 1/9 tests: ensure that FIEMAP-enabled cp copies a sparse file efficiently Ensure that copying a sparse 1TiB file completes in less than 3 seconds That can only succeed with FIEMAP (or --reflink=, which is off by default) 2/9 fiemap copy: rename some locals The _logical suffix was not useful. Change it to _start 3/9 fiemap copy: simplify post-loop logic; improve comments 4/9 fiemap copy: avoid a performance hit due to very small buffer I didn't measure this, but once you see it, it's an obvious bug. Using an arbitrarily small buffer size is bound to cause trouble. 5/9 fiemap copy: avoid leak-on-error Failing from within the loop, we have to free the extent buffer. 6/9 copy: factor sparse-copying code into its own function, because we're going to have to use it from within extent_copy, too. I realized that cp --sparse=always could no longer create holes in the destination. Factoring this out is the first step. 7/9 copy: remove obsolete comment unrelated to the rest, but hard to pull out since it's in moved code 8/9 copy: make extent_copy use sparse_copy, rather than its own code Now that sparse_copy is separate, and used by copy_reg, adapt it so that it can also be used by extent_copy. 9/9 tests: cp/fiemap: exercise previously-failing parts This is a hole-inducing test that would have failed with previous fiemap-based copying code. I may change or remove the sparse_copy_finalize function, which just calls ftruncate, especially now that it's used from only one place (initially I was using it from each sparse_copy caller, but that didn't work out), and don't particularly like the added lseek call that is performed for each file copied, but keeping track of total written/offset byte counts and inflicting the need to do that on both callers seems like too much added code/complexity to justify avoiding that single lseek call. >From 8e4f0efd3ad17f1dd7a561369da22dfaf43ab3e8 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Fri, 28 Jan 2011 22:31:23 +0100 Subject: [PATCH 1/9] tests: ensure that FIEMAP-enabled cp copies a sparse file efficiently * tests/cp/fiemap-perf: New file. * tests/Makefile.am (TESTS): Add it. --- tests/Makefile.am | 1 + tests/cp/fiemap-perf | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 0 deletions(-) create mode 100755 tests/cp/fiemap-perf diff --git a/tests/Makefile.am b/tests/Makefile.am index 847f181..7855ac5 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -320,6 +320,7 @@ TESTS = \ cp/dir-vs-file \ cp/existing-perm-race \ cp/fail-perm \ + cp/fiemap-perf \ cp/file-perm-race \ cp/into-self \ cp/link \ diff --git a/tests/cp/fiemap-perf b/tests/cp/fiemap-perf new file mode 100755 index 0000000..429e59b --- /dev/null +++ b/tests/cp/fiemap-perf @@ -0,0 +1,32 @@ +#!/bin/sh +# ensure that a sparse file is copied efficiently, by default + +# Copyright (C) 2011 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +print_ver_ cp + +# Require a fiemap-enabled FS. +df -T -t btrfs -t xfs -t ext4 -t ocfs2 . \ + || skip_ "this file system lacks FIEMAP support" + +# Create a large-but-sparse file. +timeout 1 dd bs=1 seek=1T of=f < /dev/null || framework_failure_ + +# Nothing can read (much less write) that many bytes in so little time. +timeout 3 cp f f2 || framework_failure_ + +Exit $fail -- 1.7.3.5.44.g960a >From dd380c3d672f78adb4cb907e8658db6b3962a281 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 18:28:25 +0100 Subject: [PATCH 2/9] fiemap copy: rename some locals (extent_copy): Rename locals: s/*ext_logical/*ext_start/ --- src/copy.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/copy.c b/src/copy.c index c9cc2f7..e164ab7 100644 --- a/src/copy.c +++ b/src/copy.c @@ -200,7 +200,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, bool *require_normal_copy) { struct extent_scan scan; - off_t last_ext_logical = 0; + off_t last_ext_start = 0; uint64_t last_ext_len = 0; uint64_t last_read_size = 0; @@ -228,10 +228,10 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, unsigned int i; for (i = 0; i < scan.ei_count; i++) { - off_t ext_logical = scan.ext_info[i].ext_logical; + off_t ext_start = scan.ext_info[i].ext_logical; uint64_t ext_len = scan.ext_info[i].ext_length; - if (lseek (src_fd, ext_logical, SEEK_SET) < 0) + if (lseek (src_fd, ext_start, SEEK_SET) < 0) { error (0, errno, _("cannot lseek %s"), quote (src_name)); return false; @@ -239,7 +239,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (make_holes) { - if (lseek (dest_fd, ext_logical, SEEK_SET) < 0) + if (lseek (dest_fd, ext_start, SEEK_SET) < 0) { error (0, errno, _("cannot lseek %s"), quote (dst_name)); return false; @@ -249,10 +249,10 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, { /* We're not inducing holes; write zeros to the destination file if there is a hole between the last and current extent. */ - if (last_ext_logical + last_ext_len < ext_logical) + if (last_ext_start + last_ext_len < ext_start) { - uint64_t hole_size = (ext_logical - - last_ext_logical + uint64_t hole_size = (ext_start + - last_ext_start - last_ext_len); if (! write_zeros (dest_fd, hole_size)) { @@ -262,7 +262,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } } - last_ext_logical = ext_logical; + last_ext_start = ext_start; last_ext_len = ext_len; last_read_size = 0; @@ -313,7 +313,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, file. */ if (make_holes) { - if (last_ext_logical + last_read_size < src_total_size) + if (last_ext_start + last_read_size < src_total_size) { if (ftruncate (dest_fd, src_total_size) < 0) { @@ -324,9 +324,9 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } else { - if (last_ext_logical + last_ext_len < src_total_size) + if (last_ext_start + last_ext_len < src_total_size) { - uint64_t holes_len = src_total_size - last_ext_logical - last_ext_len; + uint64_t holes_len = src_total_size - last_ext_start - last_ext_len; if (0 < holes_len) { if (! write_zeros (dest_fd, holes_len)) -- 1.7.3.5.44.g960a >From d1067e37b0e4b945ab901e98d6eedb249fa2a42c Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 19:00:48 +0100 Subject: [PATCH 3/9] fiemap copy: simplify post-loop logic; improve comments * src/copy.c (extent_copy): Avoid duplication in post-loop extend-to-desired-length code. --- src/copy.c | 44 +++++++++++++++----------------------------- 1 files changed, 15 insertions(+), 29 deletions(-) diff --git a/src/copy.c b/src/copy.c index e164ab7..ab18a76 100644 --- a/src/copy.c +++ b/src/copy.c @@ -268,8 +268,8 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, while (ext_len) { - /* Avoid reading into the holes if the left extent - length is shorter than the buffer size. */ + /* Don't read from a following hole if EXT_LEN + is smaller than the buffer size. */ buf_size = MIN (ext_len, buf_size); ssize_t n_read = read (src_fd, buf, buf_size); @@ -285,7 +285,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (n_read == 0) { - /* Record number of bytes read from the previous extent. */ + /* Record number of bytes read from this extent-at-EOF. */ last_read_size = last_ext_len - ext_len; break; } @@ -306,33 +306,19 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } while (! scan.hit_final_extent); - /* If a file ends up with holes, the sum of the last extent logical offset - and the read-returned size or the last extent length will be shorter than - the actual size of the file. Use ftruncate to extend the length of the - destination file if make_holes, or write zeros up to the actual size of the - file. */ - if (make_holes) - { - if (last_ext_start + last_read_size < src_total_size) - { - if (ftruncate (dest_fd, src_total_size) < 0) - { - error (0, errno, _("failed to extend %s"), quote (dst_name)); - return false; - } - } - } - else - { - if (last_ext_start + last_ext_len < src_total_size) - { - uint64_t holes_len = src_total_size - last_ext_start - last_ext_len; - if (0 < holes_len) - { - if (! write_zeros (dest_fd, holes_len)) - return false; - } - } + /* When the source file ends with a hole, the sum of the last extent start + offset and (the read-returned size or the last extent length) is smaller + than the actual size of the file. In that case, extend the destination + file to the required length. When MAKE_HOLES is set, use ftruncate; + otherwise, use write_zeros. */ + uint64_t eof_hole_len = (src_total_size - last_ext_start + - (last_read_size ? last_read_size : last_ext_len)); + if (eof_hole_len && (make_holes + ? ftruncate (dest_fd, src_total_size) + : ! write_zeros (dest_fd, eof_hole_len))) + { + error (0, errno, _("failed to extend %s"), quote (dst_name)); + return false; } return true; -- 1.7.3.5.44.g960a >From 33f4a4a549afb3de94e546091c91586a1ece67ba Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 17:30:08 +0100 Subject: [PATCH 4/9] fiemap copy: avoid a performance hit due to very small buffer * src/copy.c (extent_copy): Don't let what should have been a temporary reduction of buf_size (to handle a short ext_len) become permanent and thus impact the performance of all further iterations. --- src/copy.c | 5 ++--- 1 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/copy.c b/src/copy.c index ab18a76..9a3a8f7 100644 --- a/src/copy.c +++ b/src/copy.c @@ -270,9 +270,8 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, { /* Don't read from a following hole if EXT_LEN is smaller than the buffer size. */ - buf_size = MIN (ext_len, buf_size); - - ssize_t n_read = read (src_fd, buf, buf_size); + size_t b_size = MIN (ext_len, buf_size); + ssize_t n_read = read (src_fd, buf, b_size); if (n_read < 0) { #ifdef EINTR -- 1.7.3.5.44.g960a >From 47c8476ec9629239c82caf50b1c68b7bc58ba2d6 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 17:49:04 +0100 Subject: [PATCH 5/9] fiemap copy: avoid leak-on-error * src/copy.c (extent_copy): Don't leak an extent_scan buffer on failed lseek, read, or write. --- src/copy.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/copy.c b/src/copy.c index 9a3a8f7..208e463 100644 --- a/src/copy.c +++ b/src/copy.c @@ -234,6 +234,8 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (lseek (src_fd, ext_start, SEEK_SET) < 0) { error (0, errno, _("cannot lseek %s"), quote (src_name)); + fail: + extent_scan_free (&scan); return false; } @@ -242,7 +244,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (lseek (dest_fd, ext_start, SEEK_SET) < 0) { error (0, errno, _("cannot lseek %s"), quote (dst_name)); - return false; + goto fail; } } else @@ -257,7 +259,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (! write_zeros (dest_fd, hole_size)) { error (0, errno, _("%s: write failed"), quote (dst_name)); - return false; + goto fail; } } } @@ -279,7 +281,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, continue; #endif error (0, errno, _("reading %s"), quote (src_name)); - return false; + goto fail; } if (n_read == 0) @@ -292,7 +294,7 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, if (full_write (dest_fd, buf, n_read) != n_read) { error (0, errno, _("writing %s"), quote (dst_name)); - return false; + goto fail; } ext_len -= n_read; -- 1.7.3.5.44.g960a >From c0b7bc3864c06ea12c2740056e28623449fb63a7 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 20:57:17 +0100 Subject: [PATCH 6/9] copy: factor sparse-copying code into its own function, because we're going to have to use it from within extent_copy, too. * src/copy.c (sparse_copy): New function, factored out of... (copy_reg): ...here. Remove now-unused locals. --- src/copy.c | 212 ++++++++++++++++++++++++++++++++---------------------------- 1 files changed, 114 insertions(+), 98 deletions(-) diff --git a/src/copy.c b/src/copy.c index 208e463..cc8f68f 100644 --- a/src/copy.c +++ b/src/copy.c @@ -134,6 +134,116 @@ utimens_symlink (char const *file, struct timespec const *timespec) return err; } +/* Copy the regular file open on SRC_FD/SRC_NAME to DST_FD/DST_NAME, + honoring the MAKE_HOLES setting and using the BUF_SIZE-byte buffer + BUF for temporary storage. Return true upon successful completion; + print a diagnostic and return false upon error. */ +static bool +sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, + bool make_holes, + char const *src_name, char const *dst_name) +{ + typedef uintptr_t word; + off_t n_read_total = 0; + bool last_write_made_hole = false; + + while (true) + { + word *wp = NULL; + + ssize_t n_read = read (src_fd, buf, buf_size); + if (n_read < 0) + { +#ifdef EINTR + if (errno == EINTR) + continue; +#endif + error (0, errno, _("reading %s"), quote (src_name)); + return false; + } + if (n_read == 0) + break; + + n_read_total += n_read; + + if (make_holes) + { + char *cp; + + /* Sentinel to stop loop. */ + buf[n_read] = '\1'; +#ifdef lint + /* Usually, buf[n_read] is not the byte just before a "word" + (aka uintptr_t) boundary. In that case, the word-oriented + test below (*wp++ == 0) would read some uninitialized bytes + after the sentinel. To avoid false-positive reports about + this condition (e.g., from a tool like valgrind), set the + remaining bytes -- to any value. */ + memset (buf + n_read + 1, 0, sizeof (word) - 1); +#endif + + /* Find first nonzero *word*, or the word with the sentinel. */ + + wp = (word *) buf; + while (*wp++ == 0) + continue; + + /* Find the first nonzero *byte*, or the sentinel. */ + + cp = (char *) (wp - 1); + while (*cp++ == 0) + continue; + + if (cp <= buf + n_read) + /* Clear to indicate that a normal write is needed. */ + wp = NULL; + else + { + /* We found the sentinel, so the whole input block was zero. + Make a hole. */ + if (lseek (dest_fd, n_read, SEEK_CUR) < 0) + { + error (0, errno, _("cannot lseek %s"), quote (dst_name)); + return false; + } + last_write_made_hole = true; + } + } + + if (!wp) + { + size_t n = n_read; + if (full_write (dest_fd, buf, n) != n) + { + error (0, errno, _("writing %s"), quote (dst_name)); + return false; + } + last_write_made_hole = false; + + /* It is tempting to return early here upon a short read from a + regular file. That would save the final read syscall for each + file. Unfortunately that doesn't work for certain files in + /proc with linux kernels from at least 2.6.9 .. 2.6.29. */ + } + } + + /* If the file ends with a `hole', we need to do something to record + the length of the file. On modern systems, calling ftruncate does + the job. On systems without native ftruncate support, we have to + write a byte at the ending position. Otherwise the kernel would + truncate the file at the end of the last write operation. */ + if (last_write_made_hole) + { + if (ftruncate (dest_fd, n_read_total) < 0) + { + error (0, errno, _("truncating %s"), quote (dst_name)); + return false; + } + } + + return true; +} + /* Perform the O(1) btrfs clone operation, if possible. Upon success, return 0. Otherwise, return -1 and set errno. */ static inline int @@ -824,7 +934,6 @@ copy_reg (char const *src_name, char const *dst_name, if (data_copy_required) { typedef uintptr_t word; - off_t n_read_total = 0; /* Choose a suitable buffer size; it may be adjusted later. */ size_t buf_alignment = lcm (getpagesize (), sizeof (word)); @@ -832,7 +941,6 @@ copy_reg (char const *src_name, char const *dst_name, size_t buf_size = io_blksize (sb); /* Deal with sparse files. */ - bool last_write_made_hole = false; bool make_holes = false; if (S_ISREG (sb.st_mode)) @@ -897,103 +1005,11 @@ copy_reg (char const *src_name, char const *dst_name, goto close_src_and_dst_desc; } - while (true) + if ( ! sparse_copy (source_desc, dest_desc, buf, buf_size, + make_holes, src_name, dst_name)) { - word *wp = NULL; - - ssize_t n_read = read (source_desc, buf, buf_size); - if (n_read < 0) - { -#ifdef EINTR - if (errno == EINTR) - continue; -#endif - error (0, errno, _("reading %s"), quote (src_name)); - return_val = false; - goto close_src_and_dst_desc; - } - if (n_read == 0) - break; - - n_read_total += n_read; - - if (make_holes) - { - char *cp; - - /* Sentinel to stop loop. */ - buf[n_read] = '\1'; -#ifdef lint - /* Usually, buf[n_read] is not the byte just before a "word" - (aka uintptr_t) boundary. In that case, the word-oriented - test below (*wp++ == 0) would read some uninitialized bytes - after the sentinel. To avoid false-positive reports about - this condition (e.g., from a tool like valgrind), set the - remaining bytes -- to any value. */ - memset (buf + n_read + 1, 0, sizeof (word) - 1); -#endif - - /* Find first nonzero *word*, or the word with the sentinel. */ - - wp = (word *) buf; - while (*wp++ == 0) - continue; - - /* Find the first nonzero *byte*, or the sentinel. */ - - cp = (char *) (wp - 1); - while (*cp++ == 0) - continue; - - if (cp <= buf + n_read) - /* Clear to indicate that a normal write is needed. */ - wp = NULL; - else - { - /* We found the sentinel, so the whole input block was zero. - Make a hole. */ - if (lseek (dest_desc, n_read, SEEK_CUR) < 0) - { - error (0, errno, _("cannot lseek %s"), quote (dst_name)); - return_val = false; - goto close_src_and_dst_desc; - } - last_write_made_hole = true; - } - } - - if (!wp) - { - size_t n = n_read; - if (full_write (dest_desc, buf, n) != n) - { - error (0, errno, _("writing %s"), quote (dst_name)); - return_val = false; - goto close_src_and_dst_desc; - } - last_write_made_hole = false; - - /* It is tempting to return early here upon a short read from a - regular file. That would save the final read syscall for each - file. Unfortunately that doesn't work for certain files in - /proc with linux kernels from at least 2.6.9 .. 2.6.29. */ - } - } - - /* If the file ends with a `hole', we need to do something to record - the length of the file. On modern systems, calling ftruncate does - the job. On systems without native ftruncate support, we have to - write a byte at the ending position. Otherwise the kernel would - truncate the file at the end of the last write operation. */ - - if (last_write_made_hole) - { - if (ftruncate (dest_desc, n_read_total) < 0) - { - error (0, errno, _("truncating %s"), quote (dst_name)); - return_val = false; - goto close_src_and_dst_desc; - } + return_val = false; + goto close_src_and_dst_desc; } } -- 1.7.3.5.44.g960a >From 80038c3cba2dee9c6c41ab6a28a1233a538ee2ee Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 21:01:07 +0100 Subject: [PATCH 7/9] copy: remove obsolete comment * src/copy.c (sparse_copy): Remove now-obsolete comment about how we used to work around lack of ftruncate. Combine nested if conditions into one. --- src/copy.c | 21 +++++++++------------ 1 files changed, 9 insertions(+), 12 deletions(-) diff --git a/src/copy.c b/src/copy.c index cc8f68f..4bfdce6 100644 --- a/src/copy.c +++ b/src/copy.c @@ -137,7 +137,10 @@ utimens_symlink (char const *file, struct timespec const *timespec) /* Copy the regular file open on SRC_FD/SRC_NAME to DST_FD/DST_NAME, honoring the MAKE_HOLES setting and using the BUF_SIZE-byte buffer BUF for temporary storage. Return true upon successful completion; - print a diagnostic and return false upon error. */ + print a diagnostic and return false upon error. + Note that for best results, BUF should be "well"-aligned. + BUF must have sizeof(uintptr_t)-1 bytes of additional space + beyond BUF[BUF_SIZE-1]. */ static bool sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, bool make_holes, @@ -227,18 +230,12 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } } - /* If the file ends with a `hole', we need to do something to record - the length of the file. On modern systems, calling ftruncate does - the job. On systems without native ftruncate support, we have to - write a byte at the ending position. Otherwise the kernel would - truncate the file at the end of the last write operation. */ - if (last_write_made_hole) + /* If the file ends with a `hole', we need to do something to record the + length of the file. On modern systems, calling ftruncate does the job. */ + if (last_write_made_hole && ftruncate (dest_fd, n_read_total) < 0) { - if (ftruncate (dest_fd, n_read_total) < 0) - { - error (0, errno, _("truncating %s"), quote (dst_name)); - return false; - } + error (0, errno, _("truncating %s"), quote (dst_name)); + return false; } return true; -- 1.7.3.5.44.g960a >From f161ba3fcd5832d1344224ec41627cace5d73544 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Fri, 28 Jan 2011 21:19:50 +0100 Subject: [PATCH 8/9] copy: make extent_copy use sparse_copy, rather than its own code * src/copy.c (extent_copy): Before this change, extent_copy would fail to create holes, thus breaking --sparse=auto and --sparse=always. I.e., copying a large enough file of all zeros, cp --sparse=always should introduce a hole, but with extent_copy, it would not. --- src/copy.c | 109 +++++++++++++++++++++++++++--------------------------------- 1 files changed, 49 insertions(+), 60 deletions(-) diff --git a/src/copy.c b/src/copy.c index 4bfdce6..96bb35b 100644 --- a/src/copy.c +++ b/src/copy.c @@ -136,25 +136,28 @@ utimens_symlink (char const *file, struct timespec const *timespec) /* Copy the regular file open on SRC_FD/SRC_NAME to DST_FD/DST_NAME, honoring the MAKE_HOLES setting and using the BUF_SIZE-byte buffer - BUF for temporary storage. Return true upon successful completion; + BUF for temporary storage. Copy no more than MAX_N_READ bytes. + Return true upon successful completion; print a diagnostic and return false upon error. Note that for best results, BUF should be "well"-aligned. BUF must have sizeof(uintptr_t)-1 bytes of additional space - beyond BUF[BUF_SIZE-1]. */ + beyond BUF[BUF_SIZE-1]. + Set *LAST_WRITE_MADE_HOLE to true if the final operation on + DEST_FD introduced a hole. */ static bool sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, bool make_holes, - char const *src_name, char const *dst_name) + char const *src_name, char const *dst_name, + uintmax_t max_n_read, bool *last_write_made_hole) { typedef uintptr_t word; - off_t n_read_total = 0; - bool last_write_made_hole = false; + *last_write_made_hole = false; - while (true) + while (max_n_read) { word *wp = NULL; - ssize_t n_read = read (src_fd, buf, buf_size); + ssize_t n_read = read (src_fd, buf, MIN (max_n_read, buf_size)); if (n_read < 0) { #ifdef EINTR @@ -166,8 +169,7 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } if (n_read == 0) break; - - n_read_total += n_read; + max_n_read -= n_read; if (make_holes) { @@ -209,7 +211,7 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, error (0, errno, _("cannot lseek %s"), quote (dst_name)); return false; } - last_write_made_hole = true; + *last_write_made_hole = true; } } @@ -221,7 +223,7 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, error (0, errno, _("writing %s"), quote (dst_name)); return false; } - last_write_made_hole = false; + *last_write_made_hole = false; /* It is tempting to return early here upon a short read from a regular file. That would save the final read syscall for each @@ -230,9 +232,16 @@ sparse_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } } - /* If the file ends with a `hole', we need to do something to record the - length of the file. On modern systems, calling ftruncate does the job. */ - if (last_write_made_hole && ftruncate (dest_fd, n_read_total) < 0) + return true; +} + +/* If the file ends with a `hole' (i.e., if sparse_copy set wrote_hole_at_eof), + call this function to record the length of the output file. */ +static bool +sparse_copy_finalize (int dest_fd, char const *dst_name) +{ + off_t len = lseek (dest_fd, 0, SEEK_CUR); + if (0 <= len && ftruncate (dest_fd, len) < 0) { error (0, errno, _("truncating %s"), quote (dst_name)); return false; @@ -309,10 +318,10 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, struct extent_scan scan; off_t last_ext_start = 0; uint64_t last_ext_len = 0; - uint64_t last_read_size = 0; extent_scan_init (src_fd, &scan); + bool wrote_hole_at_eof = true; do { bool ok = extent_scan_read (&scan); @@ -356,8 +365,9 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } else { - /* We're not inducing holes; write zeros to the destination file - if there is a hole between the last and current extent. */ + /* When not inducing holes and when there is a hole between + the end of the previous extent and the beginning of the + current one, write zeros to the destination file. */ if (last_ext_start + last_ext_len < ext_start) { uint64_t hole_size = (ext_start @@ -373,39 +383,11 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, last_ext_start = ext_start; last_ext_len = ext_len; - last_read_size = 0; - - while (ext_len) - { - /* Don't read from a following hole if EXT_LEN - is smaller than the buffer size. */ - size_t b_size = MIN (ext_len, buf_size); - ssize_t n_read = read (src_fd, buf, b_size); - if (n_read < 0) - { -#ifdef EINTR - if (errno == EINTR) - continue; -#endif - error (0, errno, _("reading %s"), quote (src_name)); - goto fail; - } - - if (n_read == 0) - { - /* Record number of bytes read from this extent-at-EOF. */ - last_read_size = last_ext_len - ext_len; - break; - } - - if (full_write (dest_fd, buf, n_read) != n_read) - { - error (0, errno, _("writing %s"), quote (dst_name)); - goto fail; - } - ext_len -= n_read; - } + if ( ! sparse_copy (src_fd, dest_fd, buf, buf_size, + make_holes, src_name, dst_name, ext_len, + &wrote_hole_at_eof)) + return false; } /* Release the space allocated to scan->ext_info. */ @@ -414,16 +396,19 @@ extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size, } while (! scan.hit_final_extent); - /* When the source file ends with a hole, the sum of the last extent start - offset and (the read-returned size or the last extent length) is smaller - than the actual size of the file. In that case, extend the destination - file to the required length. When MAKE_HOLES is set, use ftruncate; - otherwise, use write_zeros. */ - uint64_t eof_hole_len = (src_total_size - last_ext_start - - (last_read_size ? last_read_size : last_ext_len)); - if (eof_hole_len && (make_holes - ? ftruncate (dest_fd, src_total_size) - : ! write_zeros (dest_fd, eof_hole_len))) + /* When the source file ends with a hole, we have to do a little more work, + since the above copied only up to and including the final extent. + In order to complete the copy, we may have to insert a hole or write + zeros in the destination corresponding to the source file's hole-at-EOF. + + In addition, if the final extent was a block of zeros at EOF and we've + just converted them to a hole in the destination, we must call ftruncate + here in order to record the proper length in the destination. */ + off_t dest_len = lseek (dest_fd, 0, SEEK_CUR); + if ((dest_len < src_total_size || wrote_hole_at_eof) + && (make_holes + ? ftruncate (dest_fd, src_total_size) + : ! write_zeros (dest_fd, src_total_size - dest_len))) { error (0, errno, _("failed to extend %s"), quote (dst_name)); return false; @@ -1002,8 +987,12 @@ copy_reg (char const *src_name, char const *dst_name, goto close_src_and_dst_desc; } + bool wrote_hole_at_eof; if ( ! sparse_copy (source_desc, dest_desc, buf, buf_size, - make_holes, src_name, dst_name)) + make_holes, src_name, dst_name, UINTMAX_MAX, + &wrote_hole_at_eof) + || (wrote_hole_at_eof && + ! sparse_copy_finalize (dest_desc, dst_name))) { return_val = false; goto close_src_and_dst_desc; -- 1.7.3.5.44.g960a >From 7f154dcfc5641c9616921d4c5ac5005bcb2507eb Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyer...@redhat.com> Date: Thu, 27 Jan 2011 15:17:42 +0100 Subject: [PATCH 9/9] tests: cp/fiemap: exercise previously-failing parts * tests/cp/fiemap-2: New test. * tests/Makefile.am (TESTS): Add it. --- tests/Makefile.am | 1 + tests/cp/fiemap-2 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 0 deletions(-) create mode 100755 tests/cp/fiemap-2 diff --git a/tests/Makefile.am b/tests/Makefile.am index 7855ac5..40d35ac 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -321,6 +321,7 @@ TESTS = \ cp/existing-perm-race \ cp/fail-perm \ cp/fiemap-perf \ + cp/fiemap-2 \ cp/file-perm-race \ cp/into-self \ cp/link \ diff --git a/tests/cp/fiemap-2 b/tests/cp/fiemap-2 new file mode 100755 index 0000000..d40505b --- /dev/null +++ b/tests/cp/fiemap-2 @@ -0,0 +1,54 @@ +#!/bin/sh +# Exercise a few more corners of the fiemap-copying code. + +# Copyright (C) 2011 Free Software Foundation, Inc. + +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. + +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +. "${srcdir=.}/init.sh"; path_prepend_ ../src +print_ver_ cp + +# Require a fiemap-enabled FS. +df -T -t btrfs -t xfs -t ext4 -t ocfs2 . \ + || skip_ "this file system lacks FIEMAP support" + +# Exercise the code that handles a file ending in a hole. +printf x > k || framework_failure_ +dd bs=1k seek=128 of=k < /dev/null || framework_failure_ + +# The first time through the outer loop, the input file, K, ends with a hole. +# The second time through, we append a byte so that it does not. +for append in no yes; do + test $append = yes && printf y >> k + for i in always never; do + cp --sparse=$i k k2 || fail=1 + cmp k k2 || fail=1 + done +done + +# Ensure that --sparse=always can restore holes. +rm -f k +# Create a file starting with an "x", followed by 257K-1 0 bytes. +printf x > k || framework_failure_ +dd bs=1k seek=1 of=k count=255 < /dev/zero || framework_failure_ + +# cp should detect the all-zero blocks and convert some of them to holes. +# How many it detects/converts currently depends on io_blksize. +# Currently, on my F14/ext4 desktop, this K starts off with size 256KiB, +# (note that the K in the preceding test starts off with size 4KiB). +# cp from coreutils-8.9 with --sparse=always reduces the size to 32KiB. +cp --sparse=always k k2 || fail=1 +test $(stat -c %b k2) -lt $(stat -c %b k) || fail=1 + +Exit $fail -- 1.7.3.5.44.g960a