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

[jeff.liu]

Hi Jim,

Thanks for your time to help consolidating the code!

Is this patchset acceptable to merge into the next official release?
AFAICS, the tests passed on all filesystems except ext4, but the result is ok 
by comparing the file
contents, can we take this risk?

Another thing is to add solaris SEEK_DATA support to extent_scan.c as we 
discussed before, not sure
if anyone working on this now. If not, I will take some time to follow up but 
have to delay about 2
weeks since I will on vacation for the chinese new year start from next week.

Btw, do you have plan to post extent_scan module to gnulib upstream? so that 
other file archive
projects(like tar(1)) can benefit from it.

Any thing I can do for this patchset please just let me know. :)


Regards,
-Jeff

Jim Meyering wrote:
> 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
> 
> 
>