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

[jeff.liu]

Jim Meyering wrote:
> 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".
Thanks for the update, the new functions name looks better than me.


Regards,
-Jeff
> 
> From ef3c2fe3760b11bc143b36246ee458ec86c484c9 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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 <address@hidden>
> 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
> 
> 
>