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

From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 22:59:14 +0200

jeff.liu wrote:

> Jim Meyering wrote:
>> jeff.liu wrote:
>>> Sorry for the delay.
>>>
>>> This is the new patch to isolate the stuff regarding to extents reading to a new module. and teach
>>> cp(1) to make use of it.
>>
>> Jeff,
>>
>> I applied your patch to my rebased fiemap-copy branch.
>> My first step was to run the usual
>>
>>   ./bootstrap && ./configure && make && make check
>>
>> "make check" failed on due to a double free in your new code:
>> (x86_64, Fedora 13, ext4 working directory)
>>
>> To get details, I made this temporary modification:
> Hi Jim,
>
> I am sorry for the fault, it fixed at the patch below.
> Would you please revie at your convenience?

Thanks,

Here are 5 changes on top of yours.
I'll definitely adjust logs and maybe merge one or two before
pushing anything.  Just to be sure people understand, this series
will not be in the upcoming release.

Quick summary:
  - don't let write failure go unreported
  - make "distcheck" pass once again
  - rename functions to start with "extent_scan_"
  - remove unnecessary #include directives (part of "make syntax-check")

None of this is pushed yet, but at least now it passes "make distcheck".

>From ef3c2fe3760b11bc143b36246ee458ec86c484c9 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 11:19:02 +0200
Subject: [PATCH 1/5] rename extent_scan member

* extent-scan.h [struct extent_scan]: Rename member:
s/hit_last_extent/hit_final_extent/.  "final" is clearer,
since "last" can be interpreted as "preceding".
---
 src/copy.c        |    4 ++--
 src/extent-scan.c |    6 +++---
 src/extent-scan.h |    2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 43eeb74..1e1360e 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -208,7 +208,7 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
       bool ok = get_extents_info (&scan);
       if (! ok)
         {
-          if (scan.hit_last_extent)
+          if (scan.hit_final_extent)
             break;

           if (scan.initial_scan_failed)
@@ -302,7 +302,7 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,

       /* Release the space allocated to scan->ext_info.  */
       free_extents_info (&scan);
-    } while (! scan.hit_last_extent);
+    } while (! scan.hit_final_extent);

   /* Do nothing now.  */
   close_extent_scan (&scan);
diff --git a/src/extent-scan.c b/src/extent-scan.c
index f371b87..b0345f5 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -40,7 +40,7 @@ open_extent_scan (int src_fd, struct extent_scan *scan)
   scan->ei_count = 0;
   scan->scan_start = 0;
   scan->initial_scan_failed = false;
-  scan->hit_last_extent = false;
+  scan->hit_final_extent = false;
 }

 #ifdef __linux__
@@ -81,7 +81,7 @@ get_extents_info (struct extent_scan *scan)
   /* If 0 extents are returned, then more get_extent_table() are not needed.  */
   if (fiemap->fm_mapped_extents == 0)
     {
-      scan->hit_last_extent = true;
+      scan->hit_final_extent = true;
       return false;
     }

@@ -100,7 +100,7 @@ get_extents_info (struct extent_scan *scan)
   i--;
   if (scan->ext_info[i].ext_flags & FIEMAP_EXTENT_LAST)
     {
-      scan->hit_last_extent = true;
+      scan->hit_final_extent = true;
       return true;
     }

diff --git a/src/extent-scan.h b/src/extent-scan.h
index 07c2e5b..0c9c199 100644
--- a/src/extent-scan.h
+++ b/src/extent-scan.h
@@ -50,7 +50,7 @@ struct extent_scan
   bool initial_scan_failed;

   /* If ture, the total extent scan per file has been finished.  */
-  bool hit_last_extent;
+  bool hit_final_extent;

   /* Extent information.  */
   struct extent_info *ext_info;
--
1.7.3.1.104.gc752e

>From 7f38fe3ab8d1ee08f8ca4a96457df39da5bd1f70 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 11:44:12 +0200
Subject: [PATCH 2/5] rename extent-scan functions to start with extent_scan_

---
 src/copy.c        |   12 +++++-------
 src/extent-scan.c |   10 +++++-----
 src/extent-scan.h |   22 ++++++++++++----------
 3 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index 1e1360e..a7d10b8 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -201,11 +201,11 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
   uint64_t last_ext_len = 0;
   uint64_t last_read_size = 0;

-  open_extent_scan (src_fd, &scan);
+  extent_scan_init (src_fd, &scan);

   do
     {
-      bool ok = get_extents_info (&scan);
+      bool ok = extent_scan_read (&scan);
       if (! ok)
         {
           if (scan.hit_final_extent)
@@ -213,7 +213,6 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,

           if (scan.initial_scan_failed)
             {
-              close_extent_scan (&scan);
               *require_normal_copy = true;
               return false;
             }
@@ -301,11 +300,10 @@ extent_copy (int src_fd, int dest_fd, size_t buf_size,
         }

       /* Release the space allocated to scan->ext_info.  */
-      free_extents_info (&scan);
-    } while (! scan.hit_final_extent);
+      extent_scan_free (&scan);

-  /* Do nothing now.  */
-  close_extent_scan (&scan);
+    }
+  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
diff --git a/src/extent-scan.c b/src/extent-scan.c
index b0345f5..97bb792 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -32,9 +32,9 @@
 #endif

 /* Allocate space for struct extent_scan, initialize the entries if
-   necessary and return it as the input argument of get_extents_info().  */
+   necessary and return it as the input argument of extent_scan_read().  */
 extern void
-open_extent_scan (int src_fd, struct extent_scan *scan)
+extent_scan_init (int src_fd, struct extent_scan *scan)
 {
   scan->fd = src_fd;
   scan->ei_count = 0;
@@ -50,14 +50,13 @@ open_extent_scan (int src_fd, struct extent_scan *scan)
 /* Call ioctl(2) with FS_IOC_FIEMAP (available in linux 2.6.27) to
    obtain a map of file extents excluding holes.  */
 extern bool
-get_extents_info (struct extent_scan *scan)
+extent_scan_read (struct extent_scan *scan)
 {
   union { struct fiemap f; char c[4096]; } fiemap_buf;
   struct fiemap *fiemap = &fiemap_buf.f;
   struct fiemap_extent *fm_extents = &fiemap->fm_extents[0];
   enum { count = (sizeof fiemap_buf - sizeof *fiemap) / sizeof *fm_extents };
   verify (count != 0);
-  unsigned int i;

   /* This is required at least to initialize fiemap->fm_start,
      but also serves (in mid 2010) to appease valgrind, which
@@ -88,6 +87,7 @@ get_extents_info (struct extent_scan *scan)
   scan->ei_count = fiemap->fm_mapped_extents;
   scan->ext_info = xnmalloc (scan->ei_count, sizeof (struct extent_info));

+  unsigned int i;
   for (i = 0; i < scan->ei_count; i++)
     {
       assert (fm_extents[i].fe_logical <= OFF_T_MAX);
@@ -109,5 +109,5 @@ get_extents_info (struct extent_scan *scan)
   return true;
 }
 #else
-extern bool get_extents_info (ignored) { errno = ENOTSUP; return false; }
+extern bool extent_scan_read (ignored) { errno = ENOTSUP; return false; }
 #endif
diff --git a/src/extent-scan.h b/src/extent-scan.h
index 0c9c199..3119c8d 100644
--- a/src/extent-scan.h
+++ b/src/extent-scan.h
@@ -19,7 +19,7 @@
 #ifndef EXTENT_SCAN_H
 # define EXTENT_SCAN_H

-/* Structure used to reserve information of each extent.  */
+/* Structure used to store information of each extent.  */
 struct extent_info
 {
   /* Logical offset of an extent.  */
@@ -44,25 +44,27 @@ struct extent_scan
   /* How many extent info returned for a scan.  */
   uint32_t ei_count;

-  /* If true, fall back to a normal copy, either
-     set by the failure of ioctl(2) for FIEMAP or
-     lseek(2) with SEEK_DATA.  */
+  /* If true, fall back to a normal copy, either set by the
+     failure of ioctl(2) for FIEMAP or lseek(2) with SEEK_DATA.  */
   bool initial_scan_failed;

-  /* If ture, the total extent scan per file has been finished.  */
+  /* If true, the total extent scan per file has been finished.  */
   bool hit_final_extent;

-  /* Extent information.  */
+  /* Extent information: a malloc'd array of ei_count structs.  */
   struct extent_info *ext_info;
 };

 void
-open_extent_scan (int src_fd, struct extent_scan *scan);
+extent_scan_init (int src_fd, struct extent_scan *scan);

 bool
-get_extents_info (struct extent_scan *scan);
+extent_scan_read (struct extent_scan *scan);

-#define free_extents_info(ext_scan) free ((ext_scan)->ext_info)
-#define close_extent_scan(ext_scan) /* empty */
+static inline void
+extent_scan_free (struct extent_scan *scan)
+{
+  free (scan->ext_info);
+}

 #endif /* EXTENT_SCAN_H */
--
1.7.3.1.104.gc752e

>From e33ec433eb36b1a777f9591a63bcaee1b9e6c1bf Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 11:55:46 +0200
Subject: [PATCH 3/5] distribute extent-scan.h, too

* src/Makefile.am (copy_sources): Also distribute extent-scan.h.
---
 src/Makefile.am |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/Makefile.am b/src/Makefile.am
index 7187596..de4c828 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -450,7 +450,7 @@ uninstall-local:
 	  fi; \
 	fi

-copy_sources = copy.c cp-hash.c extent-scan.c
+copy_sources = copy.c cp-hash.c extent-scan.c extent-scan.h

 # Use `ginstall' in the definition of PROGRAMS and in dependencies to avoid
 # confusion with the `install' target.  The install rule transforms `ginstall'
--
1.7.3.1.104.gc752e

>From b0a1374189800a6e8edc2cfb5154199fe970ccd7 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 11:55:58 +0200
Subject: [PATCH 4/5] formatting

---
 src/extent-scan.c |    7 ++++++-
 src/extent-scan.h |    6 ++----
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/extent-scan.c b/src/extent-scan.c
index 97bb792..5160975 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -109,5 +109,10 @@ extent_scan_read (struct extent_scan *scan)
   return true;
 }
 #else
-extern bool extent_scan_read (ignored) { errno = ENOTSUP; return false; }
+extern bool
+extent_scan_read (struct extent_scan *scan ATTRIBUTE_UNUSED)
+{
+  errno = ENOTSUP;
+  return false;
+}
 #endif
diff --git a/src/extent-scan.h b/src/extent-scan.h
index 3119c8d..ac9e500 100644
--- a/src/extent-scan.h
+++ b/src/extent-scan.h
@@ -55,11 +55,9 @@ struct extent_scan
   struct extent_info *ext_info;
 };

-void
-extent_scan_init (int src_fd, struct extent_scan *scan);
+void extent_scan_init (int src_fd, struct extent_scan *scan);

-bool
-extent_scan_read (struct extent_scan *scan);
+bool extent_scan_read (struct extent_scan *scan);

 static inline void
 extent_scan_free (struct extent_scan *scan)
--
1.7.3.1.104.gc752e

>From f4513c41e656af44859587060ce9658241988cb1 Mon Sep 17 00:00:00 2001
From: Jim Meyering <[email-masked]>
Date: Mon, 11 Oct 2010 12:00:07 +0200
Subject: [PATCH 5/5] extent-scan.c: don't include error.h or quote.h

* src/extent-scan.c: Don't include error.h or quote.h.  Neither is used.
---
 src/extent-scan.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/src/extent-scan.c b/src/extent-scan.c
index 5160975..3bb0d53 100644
--- a/src/extent-scan.c
+++ b/src/extent-scan.c
@@ -24,8 +24,6 @@

 #include "system.h"
 #include "extent-scan.h"
-#include "error.h"
-#include "quote.h"

 #ifndef HAVE_FIEMAP
 # include "fiemap.h"
--
1.7.3.1.104.gc752e