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

[Jim Meyering]

jeff.liu wrote:
> Hi Jim and All,
>
> Do you have any comments for the current implementation?

There have been several releases since we last talked about this,
but now is a good time to revive it.

I've rebased the fiemap-copy branch and made a few changes:
(somewhat sloppy 2nd log entry with the "*")

      copy.c: shorten a comment to fit in 80 columns
      * src/copy.c (copy_reg): Remove useless else-after-goto.
      copy: call extent_copy also when make_holes is false, ...
      copy: tweak variable name; improve a comment
      copy: don't allocate a separate buffer just for extent-based copy

I pushed the result as the new fiemap-copy-2 branch:

    http://git.savannah.gnu.org/cgit/coreutils.git/log/?h=fiemap-copy-2

Here are the five most recent commits.
The last one is the most interesting.
Here's its full log entry:

    copy: don't allocate a separate buffer just for extent-based copy
    * src/copy.c (copy_reg): Move use of extent_scan to just *after*
    we allocate the main copying buffer, so we can...
    (extent_scan): Take a new parameter, BUF, and use that rather
    than allocating a private buffer.  Update caller.


>From ffd02ad91ac22b18c0a07c433e7e9983aed81542 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 11 Jan 2011 22:49:34 +0100
Subject: [PATCH 1/5] copy.c: shorten a comment to fit in 80 columns

---
 src/copy.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 30c1b56..270009b 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -287,7 +287,7 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,

               if (n_read == 0)
                 {
-                  /* Figure out how many bytes read from the previous extent.  
*/
+                  /* Record number of bytes read from the previous extent.  */
                   last_read_size = last_ext_len - ext_len;
                   break;
                 }
--
1.7.3.5.38.gb312b


>From f880d4e43c47fa0b08757d911e00c69de07296ab Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 22 Jan 2011 12:30:21 +0100
Subject: [PATCH 2/5] * src/copy.c (copy_reg): Remove useless else-after-goto.

---
 src/copy.c |   10 ++++------
 1 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 270009b..71da00d 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -879,13 +879,11 @@ copy_reg (char const *src_name, char const *dst_name,
                            src_open_sb.st_size, make_holes,
                            src_name, dst_name, &require_normal_copy))
             goto preserve_metadata;
-          else
+
+          if (! require_normal_copy)
             {
-              if (! require_normal_copy)
-                {
-                  return_val = false;
-                  goto close_src_and_dst_desc;
-                }
+              return_val = false;
+              goto close_src_and_dst_desc;
             }
         }

--
1.7.3.5.38.gb312b


>From 237c2325b3d11e1b1a576978b884df3423a075b1 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 22 Jan 2011 12:36:03 +0100
Subject: [PATCH 3/5] copy: call extent_copy also when make_holes is false, ...

so that we benefit from using extents also when reading a sparse
input file with --sparse=never.
* src/copy.c (copy_reg): Remove erroneous test of "make_holes"
so that we call extent_copy also when make_holes is false.
Otherwise, what's the point of that parameter?
---
 src/copy.c |   29 +++++++++++++----------------
 1 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 71da00d..be7fdba 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -868,23 +868,20 @@ copy_reg (char const *src_name, char const *dst_name,
 #endif
         }

-      if (make_holes)
+      bool require_normal_copy;
+      /* Perform efficient extent copy for sparse file, fall back to the
+         standard copy only if the initial extent scan fails.  If the
+         '--sparse=never' option was specified, we writing all data but
+         use extent copy if available to efficiently read.  */
+      if (extent_copy (source_desc, dest_desc, buf_size,
+                       src_open_sb.st_size, make_holes,
+                       src_name, dst_name, &require_normal_copy))
+        goto preserve_metadata;
+
+      if (! require_normal_copy)
         {
-          bool require_normal_copy;
-          /* Perform efficient extent copy for sparse file, fall back to the
-             standard copy only if the initial extent scan fails.  If the
-             '--sparse=never' option was specified, we writing all data but
-             use extent copy if available to efficiently read.  */
-          if (extent_copy (source_desc, dest_desc, buf_size,
-                           src_open_sb.st_size, make_holes,
-                           src_name, dst_name, &require_normal_copy))
-            goto preserve_metadata;
-
-          if (! require_normal_copy)
-            {
-              return_val = false;
-              goto close_src_and_dst_desc;
-            }
+          return_val = false;
+          goto close_src_and_dst_desc;
         }

       /* If not making a sparse file, try to use a more-efficient
--
1.7.3.5.38.gb312b


>From b3dfab326ad8d917ac1eaba10e0852bf695f93ae Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 22 Jan 2011 12:55:58 +0100
Subject: [PATCH 4/5] copy: tweak variable name; improve a comment

* src/copy.c (copy_reg): Rename a variable to make more sense from
caller's perspective: s/require_normal_copy/normal_copy_required/.
This is an output-only variable, and the original name could make
it look like an input (or i&o) variable.
---
 src/copy.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index be7fdba..fae8dbe 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -868,17 +868,17 @@ copy_reg (char const *src_name, char const *dst_name,
 #endif
         }

-      bool require_normal_copy;
-      /* Perform efficient extent copy for sparse file, fall back to the
+      bool normal_copy_required;
+      /* Perform an efficient extent-based copy, falling back to the
          standard copy only if the initial extent scan fails.  If the
-         '--sparse=never' option was specified, we writing all data but
-         use extent copy if available to efficiently read.  */
+         '--sparse=never' option is specified, write all data but use
+         any extents to read more efficiently.  */
       if (extent_copy (source_desc, dest_desc, buf_size,
                        src_open_sb.st_size, make_holes,
-                       src_name, dst_name, &require_normal_copy))
+                       src_name, dst_name, &normal_copy_required))
         goto preserve_metadata;

-      if (! require_normal_copy)
+      if (! normal_copy_required)
         {
           return_val = false;
           goto close_src_and_dst_desc;
--
1.7.3.5.38.gb312b


>From bdf7c351a37ed6eeaa6bce98cb82902073bcc6c3 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Sat, 22 Jan 2011 13:09:08 +0100
Subject: [PATCH 5/5] copy: don't allocate a separate buffer just for 
extent-based copy

* src/copy.c (copy_reg): Move use of extent_scan to just *after*
we allocate the main copying buffer, so we can...
(extent_scan): Take a new parameter, BUF, and use that rather
than allocating a private buffer.  Update caller.
---
 src/copy.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index fae8dbe..c9cc2f7 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -194,7 +194,7 @@ write_zeros (int fd, uint64_t n_bytes)
    Upon any other failure, set *NORMAL_COPY_REQUIRED to false and
    return false.  */
 static bool
-extent_copy (int src_fd, int dest_fd, size_t buf_size,
+extent_copy (int src_fd, int dest_fd, char *buf, size_t buf_size,
              off_t src_total_size, bool make_holes,
              char const *src_name, char const *dst_name,
              bool *require_normal_copy)
@@ -268,8 +268,6 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,

           while (ext_len)
             {
-              char buf[buf_size];
-
               /* Avoid reading into the holes if the left extent
                  length is shorter than the buffer size.  */
               buf_size = MIN (ext_len, buf_size);
@@ -868,22 +866,6 @@ copy_reg (char const *src_name, char const *dst_name,
 #endif
         }

-      bool normal_copy_required;
-      /* Perform an efficient extent-based copy, falling back to the
-         standard copy only if the initial extent scan fails.  If the
-         '--sparse=never' option is specified, write all data but use
-         any extents to read more efficiently.  */
-      if (extent_copy (source_desc, dest_desc, buf_size,
-                       src_open_sb.st_size, make_holes,
-                       src_name, dst_name, &normal_copy_required))
-        goto preserve_metadata;
-
-      if (! normal_copy_required)
-        {
-          return_val = false;
-          goto close_src_and_dst_desc;
-        }
-
       /* If not making a sparse file, try to use a more-efficient
          buffer size.  */
       if (! make_holes)
@@ -912,6 +894,22 @@ copy_reg (char const *src_name, char const *dst_name,
       buf_alloc = xmalloc (buf_size + buf_alignment_slop);
       buf = ptr_align (buf_alloc, buf_alignment);

+      bool normal_copy_required;
+      /* Perform an efficient extent-based copy, falling back to the
+         standard copy only if the initial extent scan fails.  If the
+         '--sparse=never' option is specified, write all data but use
+         any extents to read more efficiently.  */
+      if (extent_copy (source_desc, dest_desc, buf, buf_size,
+                       src_open_sb.st_size, make_holes,
+                       src_name, dst_name, &normal_copy_required))
+        goto preserve_metadata;
+
+      if (! normal_copy_required)
+        {
+          return_val = false;
+          goto close_src_and_dst_desc;
+        }
+
       while (true)
         {
           word *wp = NULL;
--
1.7.3.5.38.gb312b