Subject: Re: [PATCH]: fiemap support for efficient sparse file copy

From: Jim Meyering <[email-masked]>
Date: Wed, 12 May 2010 10:48:44 +0200

jeff.liu wrote:
> Hello All,
...
> Would you guys please review and consider apply the patches if there is no other issue?

Thanks for yet another iteration, and sorry it's taking so long.

>>>From f8c78794a70f1fb45a2c61c8bbeca344087287ab Mon Sep 17 00:00:00 2001
> From: Jie Liu <[email-masked]>
> Date: Fri, 7 May 2010 20:48:45 +0800
> Subject: [PATCH 1/3] Add fiemap.h for fiemap ioctl(2) support.
>  It does not shipped by default, so I copy it from kernel at the moment.
>  I have update its code style respect to GNU coding style.

Your log message should look like this:
(one-line summary, then ChangeLog-style after a blank line)

add fiemap.h for fiemap ioctl(2) support

* src/fiemap.h: Copied from linux's include/linux/fiemap.h,
with minor formatting changes.

> Signed-off-by: Jie Liu <[email-masked]>
> ---
>  gnulib       |    2 +-

Please omit the gnulib part.
It is irrelevant to your change.

>>>From 8822b8e3f3ee70b49efb8b8aebff373792956422 Mon Sep 17 00:00:00 2001
> From: Jie Liu <[email-masked]>
> Date: Fri, 7 May 2010 21:31:56 +0800
> Subject: [PATCH 3/3] Add test script for cp(1) fiemap copy.
>
> Signed-off-by: Jie Liu <[email-masked]>
> ---
>  tests/cp/sparse-fiemap |   58 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 58 insertions(+), 0 deletions(-)
>  create mode 100755 tests/cp/sparse-fiemap
>
> diff --git a/tests/cp/sparse-fiemap b/tests/cp/sparse-fiemap
> new file mode 100755
> index 0000000..25a8fd6
> --- /dev/null
> +++ b/tests/cp/sparse-fiemap
> @@ -0,0 +1,58 @@
> +#!/bin/sh
> +# Test cp --sparse=always through fiemap copy
> +
> +# Copyright (C) 2006-2010 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/>.
> +
> +if test "$VERBOSE" = yes; then
> +  set -x
> +  cp --version
> +fi
> +
> +. $srcdir/test-lib.sh
> +
> +cp_orig=cp
> +cp_new="$abs_top_builddir/src/cp"
> +
> +test -d "/ext4"              \
> +  && sparse="/ext4/sparse"   \
> +  && normal="/ext4/sparse1"  \
> +  && fiemap="/ext4/sparse2"  \
> +  || skip=1

Be careful about writing to well-known files like these.
That's risky if /ext4 is world-writable.
Work in a temporary directory created by mktemp -d, just in case.

It'd be better still to make the test work when run as root, in which
case you can create and mount a file-backed ext4 partition, and then use
that, rather than relying on the existence of a preexisting, fixed-name
directory that may or may not be of the right FS type.

But don't worry about this part if you're not inclined.
We can make it do that later.

> +test $skip = 1 && skip_test_ "/ext4 does not exists"

s/exists/exist/

> +size=`expr 10 \* 1024`

Here, just use size=10240.

In general, please use $(...), not `...`.
(multiple places)

> +dd if=/dev/zero bs=4k count=1 seek=$size of=$sparse > /dev/null 2>&1 || framework_failure
> +
> +# Using time(1) instead of shell built-in `time' command.
> +# It support "--format" option which is more convinent to calculate
> +# the expense time for different `cp' by combine with bc(1) for
> +# the performance measurement.
> +TIME=`which time` || skip_test_ "time(1) does not exists"

Don't use "which".  It has portability problems.  Instead,

  env time --version | grep GNU > /dev/null \
    || skip_test_ "you lack the GNU time program"

Another alternative: record $(date +%s.%N), before and after.
Elapsed time should be adequate.  Advantage: date is always available,
while time may not be.

> +x=$(echo "1+2" | bc)
> +test $x = 3 || skip_test_ "bc(1) does not exists"
> +
> +t1=$($TIME -f "%U + %S" $cp_orig --sparse=always $sparse $normal 2>&1 | bc) || fail=1
> +t2=$($TIME -f "%U + %S" $cp_new --sparse=always $sparse $fiemap 2>&1 | bc)  || fail=1
> +
> +test $fail = 1 && skip_test_ "at least one sparse file copy failed"
> +
> +# Ensure that the sparse file copied through fiemap has the same size in bytes as the original.
> +test `stat --printf %s $sparse` -eq `stat --printf %s $fiemap` || fail=1

s/-eq/=/

> +echo "$t2 < $t1" | bc || fail=1

At first I wrote this:

    Can't you require something stronger than merely
    that the new code is faster than the old?
    How do the times compare in general?
    Can you construct a worst-case scenario that
    makes the difference as large as possible, yet
    that still completes (with the new code) in say
    5 or 10 seconds on modern equipment?

    Use awk rather than bc for comparison.
    For an example that's similar, I made parted fail a test
    when an efficiency-sensitive operation takes more than a minute here:

    http://git.debian.org/?p=parted/parted.git;a=commitdiff;h=eedc6d77dc4b3488decd4dce9cb8cafaa95755ce

    You can depend on some version of awk being available.
    Invoke it via $AWK, but for that to work in the test script,
    you'll have to add AWK=$(AWK) to tests/Makefile.am's TESTS_ENVIRONMENT,
    as I did here:

    http://git.debian.org/?p=parted/parted.git;a=commitdiff;h=246c953b53c1bd49b1f835f84a1ca29a6d2fbc1c

Then I remembered that here we have timeout(1), so:
you may ignore the above and consider this a suggestion
to use timeout:

But that was in Parted, where we can't guarantee that the timeout
program is available.  Here in coreutils, you're guaranteed to
have timeout(1) (just built), so you might want to use it, too:
Contrive a test that takes a very long time without FIEMAP support
yet that runs in a couple seconds with it.  Then run cp via timeout
with a 10-second limit.  If timeout's exit status is not 0,
then make the test fail.

That has the advantage of letting you use an example that would take
far longer that we typically want to wait for a non-FIEMAP test.
I.e., perform only the FIEMAP-copy and ensure that it's "quick enough".
You don't have to perform a non-FIEMAP one.

Another advantage: if you don't do the old/slow sparse copy,
there's no need for comparison (and bc or awk) at all.