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

[Jim Meyering]

jeff.liu wrote:
> Hi Jim,
>
> Thanks for your kind advise!
>
> I'd like to adopt the timeout(1) approach for the test work.
>
> My thought is:
> 1. Create and mount a file-backed ext4 partition rather than relying on the 
> HARD CODE path.
> 2. Create a 2gb sparse file without extent allocated for it.
> 3. It take nearly 30 seconds to transfer this file in normal copy, yet less 
> than 1 second through
> FIEMAP-copy, is it a worst-case scenario that makes the difference as large 
> as possible?
> 4. run FIEMAP-copy, use timeout(1) to limit it will complete in 1 second, I 
> hope I understood your
> opinion correctly ;).
>
> The revised patches are shown as following:
>
>>From f18e1801d1dfca9fa278572b8172a5f97da2adc1 Mon Sep 17 00:00:00 2001
> From: Jie Liu <address@hidden>
> Date: Thu, 13 May 2010 22:17:53 +0800
> Subject: [PATCH 1/1] tests: add a new test for FIEMAP-copy
>
> * tests/cp/sparse-fiemap: Add a new test for FIEMAP-copy against a
> loopbacked ext4 partition.
> * tests/Makefile.am (sparse-fiemap): Reference the new test.
>
> Signed-off-by: Jie Liu <address@hidden>
> ---
>  tests/Makefile.am      |    2 +
>  tests/cp/sparse-fiemap |   61 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+), 0 deletions(-)
>  create mode 100644 tests/cp/sparse-fiemap
>
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 46d388a..a76c6a7 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -25,6 +25,7 @@ root_tests =                                        \
>    cp/special-bits                            \
>    cp/cp-mv-enotsup-xattr                     \
>    cp/capability                                      \
> +  cp/sparse-fiemap                              \
>    dd/skip-seek-past-dev                              \
>    install/install-C-root                     \
>    ls/capability                                      \
> @@ -319,6 +320,7 @@ TESTS =                                           \
>    cp/same-file                                       \
>    cp/slink-2-slink                           \
>    cp/sparse                                  \
> +  cp/sparse-fiemap                              \
>    cp/special-f                                       \
>    cp/src-base-dot                            \
>    cp/symlink-slash                           \

I've applied your patches locally and have begun adjusting them.
First, I removed the addition of cp/sparse-fiemap to the TESTS list above.
Adding it to the root_tests is sufficient.

Then, I've made the following changes to your test script:
  - the original size of your test file of 2GiB was too small,
      in that the old (pre-fiemap) cp copied it for me in less than
      1 second when the backing file was on a tmpfs file system.
      I've made the new size be 2TiB.  The fiemap copy is still so
      quick that it completes in < .01 second.[*]
  - no point in discarding stdout/stderr, since it all goes to the log
  - raised timeout to 10 seconds to give more leeway on slow systems
  - remove those "rm -f" uses.  They're not needed, since the test is
      run in its own temp dir, which is removed automatically when done.
  - remove the $? = 124 test -- the preceding test for success is sufficient

[*] I tried to count syscalls with strace but got a segfault.
Using valgrind I get errors, so debugged enough to get a clean
run, but possibly at the expense of correctness.  We'll need more
tests to ensure that the non-sparse blocks in the copy all have
the same offset/length as in the original.  Details below.

diff --git a/tests/cp/sparse-fiemap b/tests/cp/sparse-fiemap
old mode 100644
new mode 100755
index f9d3a94..814d537
--- a/tests/cp/sparse-fiemap
+++ b/tests/cp/sparse-fiemap
@@ -28,8 +28,7 @@ cwd=`pwd`
 cleanup_() { cd /; umount "$cwd/mnt"; }

 # Create an ext4 loopback file system
-dd if=/dev/zero of=blob bs=8192 count=1000 > /dev/null 2>&1 \
-                                               || skip=1
+dd if=/dev/zero of=blob bs=8192 count=1000 || skip=1
 mkdir mnt
 mkfs -t ext4 -F blob ||
   skip_test_ "failed to create ext4 file system"
@@ -42,20 +41,15 @@ test $skip = 1 &&

 rm -f mnt/f

-# Create a 2gb sparse file
-dd if=/dev/zero of=mnt/sparse bs=1k count=1 seek=2096128 > /dev/null 2>&1 || 
framework_failure
+# Create a 2TiB sparse file
+dd if=/dev/zero of=mnt/sparse bs=1k count=1 seek=2G || framework_failure

-# It take more than 20 seconds to transfer the created sparse file
-# through normal copy, by contrast, it take even less than 1 second
-# through FIEMAP-copy.
-timeout 1 cp --sparse=always mnt/sparse mnt/sparse_fiemap || fail=1
-test $? = 124 && fail=1
+# It takes many minutes to copy this sparse file using the old method.
+# By contrast, it takes far less than 1 second using FIEMAP-copy.
+timeout 10 cp --sparse=always mnt/sparse mnt/sparse_fiemap || fail=1

 # Ensure that the sparse file copied through fiemap has the same size
 # in bytes as the original.
 test `stat --printf %s $sparse` = `stat --printf %s $fiemap` || fail=1

-rm -f mnt/sparse
-rm -f mnt/sparse_fiemap
-
 Exit $fail

----------------------------------------
On F13, x86_64, ext4, I did this:

dd if=/dev/null of=big bs=1 seek=2G
valgrind ./cp --sparse=always big big2
==4771== Conditional jump or move depends on uninitialised value(s)
==4771==    at 0x40465A: fiemap_copy_ok (copy.c:205)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Syscall param lseek(offset) contains uninitialised byte(s)
==4771==    at 0x3269CE1540: __lseek_nocancel (syscall-template.S:82)
==4771==    by 0x4046D4: fiemap_copy_ok (copy.c:210)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Syscall param lseek(offset) contains uninitialised byte(s)
==4771==    at 0x3269CE1540: __lseek_nocancel (syscall-template.S:82)
==4771==    by 0x40472D: fiemap_copy_ok (copy.c:216)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Conditional jump or move depends on uninitialised value(s)
==4771==    at 0x404792: fiemap_copy_ok (copy.c:222)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Conditional jump or move depends on uninitialised value(s)
==4771==    at 0x40492B: fiemap_copy_ok (copy.c:229)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Conditional jump or move depends on uninitialised value(s)
==4771==    at 0x4047FA: fiemap_copy_ok (copy.c:235)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Syscall param read(count) contains uninitialised byte(s)
==4771==    at 0x3269CD41B0: __read_nocancel (syscall-template.S:82)
==4771==    by 0x404821: fiemap_copy_ok (copy.c:238)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==
==4771== Invalid read of size 8
==4771==    at 0x404952: fiemap_copy_ok (copy.c:265)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)
==4771==  Address 0x3ffeffdd68 is not stack'd, malloc'd or (recently) free'd
==4771==
==4771==
==4771== Process terminating with default action of signal 11 (SIGSEGV)
==4771==  Access not within mapped region at address 0x3FFEFFDD68
==4771==    at 0x404952: fiemap_copy_ok (copy.c:265)
==4771==    by 0x405B61: copy_reg (copy.c:822)
==4771==    by 0x408713: copy_internal (copy.c:2163)
==4771==    by 0x409237: copy (copy.c:2449)
==4771==    by 0x403AC9: do_copy (cp.c:754)
==4771==    by 0x4041E4: main (cp.c:1154)

===========================================================
The segv just above is due to hitting this line with i==0:

    fiemap->fm_start = (fm_ext[i-1].fe_logical + fm_ext[i-1].fe_length);

the obvious fix is probably to do this instead:

    fiemap->fm_start = (fm_ext[i].fe_logical + fm_ext[i].fe_length);

All of the used-uninitialized errors can be papered over by
clearing the fiemap_buf array, like this:

+  memset (fiemap_buf, 0, sizeof fiemap_buf);
   do
     {
       fiemap->fm_start = 0ULL;

However, if these are all due solely to F13's valgrind not yet knowing the
semantics of the FIEMAP ioctl, then that may be adequate.

Bottom line:
  - you may consider your test-script patch accepted, with the patch above
  - I'd like to see a new version of the copy.c-changing patch,
    including at least a fix for the fm_ext[-1] access bug.

===========================================================
Solely for reference, here's the copy.c patch I used to avoid
the valgrind-spotted problems:

diff --git a/src/copy.c b/src/copy.c
index 960e5fb..e232eaa 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -179,6 +179,7 @@ fiemap_copy_ok (int src_fd, int dest_fd, size_t buf_size,
   uint64_t last_read_size = 0;
   unsigned int i = 0;

+  memset (fiemap_buf, 0, sizeof fiemap_buf);
   do
     {
       fiemap->fm_start = 0ULL;
@@ -187,7 +188,7 @@ fiemap_copy_ok (int src_fd, int dest_fd, size_t buf_size,

       /* When ioctl(2) fails, fall back to the normal copy only if it
          is the first time we met.  */
-      if (ioctl (src_fd, FS_IOC_FIEMAP, (unsigned long) fiemap) < 0)
+      if (ioctl (src_fd, FS_IOC_FIEMAP, fiemap) < 0)
         {
           /* If `i > 0', then at least one ioctl(2) has been performed before. 
 */
           if (i == 0)
@@ -261,7 +262,7 @@ fiemap_copy_ok (int src_fd, int dest_fd, size_t buf_size,
               ext_len -= n_read;
             }

-          fiemap->fm_start = (fm_ext[i-1].fe_logical + fm_ext[i-1].fe_length);
+          fiemap->fm_start = (fm_ext[i].fe_logical + fm_ext[i].fe_length);
         }
     } while (! last);