Subject: Re: bug#6131: [PATCH]: fiemap support for efficient sparse file copy

From: Jim Meyering <[email-masked]>
Date: Sat, 29 Jan 2011 10:47:53 +0100

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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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 <[email-masked]>
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