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

[Pádraig Brady]

On 29/01/11 09:47, Jim Meyering wrote:
> Jim Meyering wrote:
>> Jeff liu wrote:
>>> Now  make check passed against the following combination:
>>> 1. Refresh installed host in Ubuntu10.0.4,
>>> filefrag comes from E2fsprogs 1.41.11 && Kernel: 2.6.32-16
>>> 2. filefrag in e2fsprogs-1.4.12 && kernel-2.6.36.
>> [passes]
>>
>> Glad to here it passes for you, now.
>> FYI, I have spent pretty much time on cp over the last
>> couple days, factoring out the hole-inducing code and
>> making extent_copy use it.  Part of the motivation was
>> to fix cp --sparse=always, which was broken on the branch.
>> It would not induce holes when going through extent_copy.
>> I've added a couple more tests and will post the series as
>> soon I've cleaned things up a little more.
> 
> Here are 9 more patches, just pushed to the fiemap-copy-2 branch:
> 
>   http://git.savannah.gnu.org/cgit/coreutils.git/log/?h=fiemap-copy-2
> 
> The first and last add tests, and the others consolidate,
> clean up, and fix a few bugs.
> 
>   1/9 tests: ensure that FIEMAP-enabled cp copies a sparse file efficiently
>     Ensure that copying a sparse 1TiB file completes in less than 3 seconds
>     That can only succeed with FIEMAP (or --reflink=, which is off by default)
> 
>   2/9 fiemap copy: rename some locals
>     The _logical suffix was not useful.  Change it to _start
> 
>   3/9 fiemap copy: simplify post-loop logic; improve comments
> 
>   4/9 fiemap copy: avoid a performance hit due to very small buffer
>     I didn't measure this, but once you see it, it's an obvious bug.
>     Using an arbitrarily small buffer size is bound to cause trouble.
> 
>   5/9 fiemap copy: avoid leak-on-error
>     Failing from within the loop, we have to free the extent buffer.
> 
>   6/9 copy: factor sparse-copying code into its own function, because
>     we're going to have to use it from within extent_copy, too.
>     I realized that cp --sparse=always could no longer create holes
>     in the destination.  Factoring this out is the first step.
> 
>   7/9 copy: remove obsolete comment
>     unrelated to the rest, but hard to pull out since it's in moved code
> 
>   8/9 copy: make extent_copy use sparse_copy, rather than its own code
>     Now that sparse_copy is separate, and used by copy_reg, adapt it
>     so that it can also be used by extent_copy.
> 
>   9/9 tests: cp/fiemap: exercise previously-failing parts
>     This is a hole-inducing test that would have failed with previous
>     fiemap-based copying code.
> 
> I may change or remove the sparse_copy_finalize function, which just calls
> ftruncate, especially now that it's used from only one place (initially
> I was using it from each sparse_copy caller, but that didn't work out),
> and don't particularly like the added lseek call that is performed for
> each file copied, but keeping track of total written/offset byte counts
> and inflicting the need to do that on both callers seems like too much
> added code/complexity to justify avoiding that single lseek call.
> 
>>From 8e4f0efd3ad17f1dd7a561369da22dfaf43ab3e8 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <address@hidden>
> Date: Fri, 28 Jan 2011 22:31:23 +0100
> Subject: [PATCH 1/9] tests: ensure that FIEMAP-enabled cp copies a sparse 
> file efficiently
> 
> * tests/cp/fiemap-perf: New file.
> * tests/Makefile.am (TESTS): Add it.
> ---
>  tests/Makefile.am    |    1 +
>  tests/cp/fiemap-perf |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+), 0 deletions(-)
>  create mode 100755 tests/cp/fiemap-perf
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 847f181..7855ac5 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -320,6 +320,7 @@ TESTS =                                           \
>    cp/dir-vs-file                             \
>    cp/existing-perm-race                              \
>    cp/fail-perm                                       \
> +  cp/fiemap-perf                                \
>    cp/file-perm-race                          \
>    cp/into-self                                       \
>    cp/link                                    \
> diff --git a/tests/cp/fiemap-perf b/tests/cp/fiemap-perf
> new file mode 100755
> index 0000000..429e59b
> --- /dev/null
> +++ b/tests/cp/fiemap-perf
> @@ -0,0 +1,32 @@
> +#!/bin/sh
> +# ensure that a sparse file is copied efficiently, by default
> +
> +# Copyright (C) 2011 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ cp
> +
> +# Require a fiemap-enabled FS.
> +df -T -t btrfs -t xfs -t ext4 -t ocfs2 . \
> +  || skip_ "this file system lacks FIEMAP support"
> +
> +# Create a large-but-sparse file.
> +timeout 1 dd bs=1 seek=1T of=f < /dev/null || framework_failure_
> +
> +# Nothing can read (much less write) that many bytes in so little time.
> +timeout 3 cp f f2 || framework_failure_

I'm a bit worried with a 1s timeout.
The following will only give false negatives over 100GB/s

timeout 10 truncate -s1T f || framework_failure_
timeout 10 cp f f2 || framework_failure_

I wouldn't worry about filling file systems either,
as we're already limiting to ext4 etc.

>>From 7f154dcfc5641c9616921d4c5ac5005bcb2507eb Mon Sep 17 00:00:00 20: Jim 
>>Meyering <address@hidden>
> Date: Thu, 27 Jan 2011 15:17:42 +0100
> Subject: [PATCH 9/9] tests: cp/fiemap: exercise previously-failing parts
> +# Ensure that --sparse=always can restore holes.
> +rm -f k
> +# Create a file starting with an "x", followed by 257K-1 0 bytes.
> +printf x > k || framework_failure_
> +dd bs=1k seek=1 of=k count=255 < /dev/zero || framework_failure_

S/257/256/ ?