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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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 <meyer...@redhat.com> 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