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

2010-05-21 Thread Pádraig Brady 
On 13/05/10 15:25, jeff.liu wrote:
>
> diff --git a/src/copy.c b/src/copy.c
> index c16cef6..960e5fb 100644
> --- a/src/copy.c
> +++ b/src/copy.c
> @@ -63,6 +63,10 @@
>
>  #include 
>
> +#ifndef HAVE_FIEMAP
> +# include "fiemap.h"
> +#endif

Is HAVE_FIEMAP ever defined anywhere?
In future will we use this to check for  ?

On 20/05/10 20:23, Jim Meyering wrote:
> 
> 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"

There is the unlikely combination of ext4 without fiemap support I think?
If so then that dependency is worth a comment.

> @@ -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

If we don't need any actual data in the files then one could use:
  truncate -s 2TB mnt/sparse

For my reference, I used TB rather than TiB because on ext3
the limit is 0x1FEFF7FC000 (2194719883264)
(0x1FF7FFFD000 (2196875759616) before 2.6.25)

cheers,
Pádraig.

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

2010-05-21 Thread Jim Meyering 
Pádraig Brady wrote:
> On 13/05/10 15:25, jeff.liu wrote:
>>
>> diff --git a/src/copy.c b/src/copy.c
>> index c16cef6..960e5fb 100644
>> --- a/src/copy.c
>> +++ b/src/copy.c
>> @@ -63,6 +63,10 @@
>>
>>  #include 
>>
>> +#ifndef HAVE_FIEMAP
>> +# include "fiemap.h"
>> +#endif
>
> Is HAVE_FIEMAP ever defined anywhere?
> In future will we use this to check for  ?

I'll look.

> On 20/05/10 20:23, Jim Meyering wrote:
>>
>> 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"
>
> There is the unlikely combination of ext4 without fiemap support I think?
> If so then that dependency is worth a comment.

I don't know off hand.
Is there a shell-level way to test for that?

>> @@ -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
>
> If we don't need any actual data in the files then one could use:
>   truncate -s 2TB mnt/sparse
>
> For my reference, I used TB rather than TiB because on ext3
> the limit is 0x1FEFF7FC000 (2194719883264)
> (0x1FF7FFFD000 (2196875759616) before 2.6.25)

Thanks.
For now I'll limit it to 1GiB using dd.
That gives a slightly less uniform input.

  # Create a 1TiB sparse file
  dd if=/dev/zero of=mnt/sparse bs=1k count=1 seek=1G || framework_failure

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

2010-05-21 Thread jeff.liu 
Jim Meyering wrote:
> 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 
>> 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 
>> ---
>>  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.
Thank you to point it out.

> 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.  
Is it make sense if we write a utility in C through FIEMAP to show the extent 
info of a file?
then wrap it in our current test scripts or a new test script to compare the 
non-sparse blocks
offset and length?

filefrag(8) can do such thing(http://e2fsprogs.sourceforge.net/), but maybe we 
can implement a
compacted version focus on furture extent maping related testing only for 
coreutils.

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 tr

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

2010-05-21 Thread Pádraig Brady 
On 21/05/10 13:59, Jim Meyering wrote:
> Pádraig Brady wrote:
>> There is the unlikely combination of ext4 without fiemap support I think?
>> If so then that dependency is worth a comment.
> 
> I don't know off hand.
> Is there a shell-level way to test for that?

One could check if  is available, but that would add
the dependency on kernel-headers being installed.
Alternatively one could skip the test on non linux and linux < 2.6.27
But requiring ext4 is probably good enough to limit this to systems
that actually do have fiemap available. Adding a comment about linux >= 2.6.27
might help us quickly eliminate false positives that are reported.

cheers.
Pádraig.

bug#6236: Bug report in Date Command

2010-05-21 Thread Eric Blake 
On 05/20/2010 08:07 PM, lijian 65631 wrote:
> Dear David,

This is a mailing list frequented by lots of readers.  I'm not David,
but I can reply.

> 
> When I input the comman 'date "+%Y%m%d" -d "1986-05-04 1 day"' to get the
> next day of '1986-05-04', I get the result as below.
> 
> Maybe it is a bug of Date command , Please help to find it, Thanks.

Thanks for the report.  However, it is most likely the only bug here is
your understanding of how date operates around daylight savings events:
http://www.gnu.org/software/coreutils/faq/#The-date-command-is-not-working-right_002e

> This e-mail and its attachments contain confidential information from

It is considered poor netiquette to include employer disclaimers on mail
sent to publicly archived mailing lists, where the disclaimer is
rendered ineffective.  Consider using a personal account instead.

-- 
Eric Blake   ebl...@redhat.com+1-801-349-2682
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

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

2010-05-21 Thread Jim Meyering 
jeff.liu wrote:
...
>> [*] 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.
> Is it make sense if we write a utility in C through FIEMAP to show the extent 
> info of a file?
> then wrap it in our current test scripts or a new test script to compare the 
> non-sparse blocks
> offset and length?

If there were no adequate tool already available, that would be good.

> filefrag(8) can do such thing(http://e2fsprogs.sourceforge.net/), but maybe 
> we can implement a
> compacted version focus on furture extent maping related testing only for 
> coreutils.

Or maybe just use filefrag, when it's available.
On F13, with -v (verbose), it prints this:

$ filefrag -v big
Filesystem type is: ef53
File size of big is 2147483648 (524288 blocks, blocksize 4096)
 ext logical physical expected length flags
   0   0   254527   1
big: 1 extent found


>> ===
>> 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);
> strange, code should break if there is no extent allocated for a file.
>  /* If 0 extents are returned, then more ioctls are not needed.  */
>   if (fiemap->fm_mapped_extents == 0)
> break;

There is one extent, and it is while processing it, with i == 0 that
would trigger the failure when referencing fm_ext[i-1] (aka fm_ext[-1]).

>> the obvious fix is probably to do this instead:
>>
>> fiemap->fm_start = (fm_ext[i].fe_logical + fm_ext[i].fe_length);
> I just found a bug for dealing with the 'fiemap->fm_start', maybe it is the 
> root cause of the
> segment fault.  above line still need to write as 'fm_ext[i-1].fe_logical 
> +' to calculate the
> offset for the next ioctl(2).

"i" can be 0 there, so it sounds like you're saying we need to
reference fm_ext[-1].  If you mean that, you'll have to demonstrate
how we guarantee that i > 0 there.

>> 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);
> I recalled why I initialized this buf before when you ask me the reason, I 
> was intented to
> initialize the 'fiemap->fm_start', so below line 'fiemap->fm_start = 0ULL' 
> should be removed from
> the loop.
>
>>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.
> as what I mentioned above, this line should be removed or remove out of the 
> loop if we do not
> initialize the fiemap buf.

I agree.
Leaving the initialization in the loop would provoke an infinite loop,
for a file with many extents.

This demonstrates it:

$ perl -e 'for (1..100) { sysseek(STDOUT,4096,1)' \
   -e '&& syswrite(STDOUT,"."x4096) or die "$!"}' > j
$ ./cp --sparse=always j j2

^C

With this statement "fiemap->fm_start = 0ULL;" in the do-while loop,
the use of ./cp above would infloop.  Without it, it works properly:

$ env time -f %E ./cp --sparse=always j j2
0:00.01

And we can compare the extents in the two:
(the awk is mainly to exclude the physical block numbers,
which will always differ)

$ diff -u <(filefrag -v j|awk '/^ / {print $1,$2,$NF}') \
  <(filefrag -v j2|awk '/^ / {print $1,$2,$NF}')
$

For reference, here's what filefrag -v output looks like,
given a file with a nontrivial list of extents:

  $ perl -e 'BEGIN{$n=16*1024; *F=*STDOUT}' \
 -e 'for (1..5) { sysseek(*F,$n,1)' \
 -e '&& syswrite *F,"."x$n or die "$!"}' > j
  $ filefrag -v j
  Filesystem type is: ef53
  File size of j is 163840 (40 blocks, blocksize 4096)
   ext logical physical expected length flags
 0   4  6258884   4
 1  12  6258892  6258887  4
 2  20  6258900  6258895  4
 3  28  6258908  6258903  4
 4  36  6258916  6258911  4 eof
  j: 6 extents found

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

2010-05-21 Thread Jim Meyering 
jeff.liu wrote:
...
>>> 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.

BTW, I've just made this additional change to your test,

diff --git a/tests/cp/sparse-fiemap b/tests/cp/sparse-fiemap
index 6312a4c..bdc7ded 100755
--- a/tests/cp/sparse-fiemap
+++ b/tests/cp/sparse-fiemap
@@ -27,6 +27,7 @@ require_root_
 cwd=`pwd`
 cleanup_() { cd /; umount "$cwd/mnt"; }

+skip=0
 # Create an ext4 loopback file system
 dd if=/dev/zero of=blob bs=8192 count=1000 || skip=1
 mkdir mnt

And will push this correction to the one you appear to have used as a model:

>From c9bcbc8f9fc791c97bc85678f5f22458a76689ac Mon Sep 17 00:00:00 2001
From: Jim Meyering 
Date: Fri, 21 May 2010 14:55:36 +0200
Subject: [PATCH] tests: fix cp-a-selinux to skip cleanly upon mkfs failure

* tests/cp/cp-a-selinux: Initialize skip, to avoid a syntax error
in subsequent "test".
---
 tests/cp/cp-a-selinux |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tests/cp/cp-a-selinux b/tests/cp/cp-a-selinux
index b65070a..5b9ff0f 100755
--- a/tests/cp/cp-a-selinux
+++ b/tests/cp/cp-a-selinux
@@ -45,7 +45,7 @@ test -s err && fail=1   #there must be no stderr output for -a
 ls -Z e | grep $ctx || fail=1
 ls -Z f | grep $ctx || fail=1

-
+skip=0
 # Create a file system, then mount it with the context=... option.
 dd if=/dev/zero of=blob bs=8192 count=200 > /dev/null 2>&1 \
  || skip=1
--
1.7.1.262.g5ef3d

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

2010-05-21 Thread jeff.liu 
Jim Meyering wrote:
> jeff.liu wrote:
> ...
 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.
> 
> BTW, I've just made this additional change to your test,
> 
> diff --git a/tests/cp/sparse-fiemap b/tests/cp/sparse-fiemap
> index 6312a4c..bdc7ded 100755
> --- a/tests/cp/sparse-fiemap
> +++ b/tests/cp/sparse-fiemap
> @@ -27,6 +27,7 @@ require_root_
>  cwd=`pwd`
>  cleanup_() { cd /; umount "$cwd/mnt"; }
> 
> +skip=0
>  # Create an ext4 loopback file system
>  dd if=/dev/zero of=blob bs=8192 count=1000 || skip=1
>  mkdir mnt
> 
> And will push this correction to the one you appear to have used as a model:
> 
> From c9bcbc8f9fc791c97bc85678f5f22458a76689ac Mon Sep 17 00:00:00 2001
> From: Jim Meyering 
> Date: Fri, 21 May 2010 14:55:36 +0200
> Subject: [PATCH] tests: fix cp-a-selinux to skip cleanly upon mkfs failure
> 
> * tests/cp/cp-a-selinux: Initialize skip, to avoid a syntax error
> in subsequent "test".
> ---
>  tests/cp/cp-a-selinux |2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tests/cp/cp-a-selinux b/tests/cp/cp-a-selinux
> index b65070a..5b9ff0f 100755
> --- a/tests/cp/cp-a-selinux
> +++ b/tests/cp/cp-a-selinux
> @@ -45,7 +45,7 @@ test -s err && fail=1   #there must be no stderr output for 
> -a
>  ls -Z e | grep $ctx || fail=1
>  ls -Z f | grep $ctx || fail=1
> 
> -
> +skip=0
>  # Create a file system, then mount it with the context=... option.
>  dd if=/dev/zero of=blob bs=8192 count=200 > /dev/null 2>&1 \
>   || skip=1
Hmm, here also has a '2>&1' for `dd'. :)
> --
> 1.7.1.262.g5ef3d


Thanks,
-Jeff
-- 
With Windows 7, Microsoft is asserting legal control over your computer and is 
using this power to
abuse computer users.

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

2010-05-21 Thread Jim Meyering 
jeff.liu wrote:
...
> Sorry for the lack of detailed info for this point, except for removing the 
> fiemap->fm_start from
> the loop, I need to remove "fiemap->fm_start = (fm_ext[i-1].fe_logical + 
> fm_ext[i-1].fe_length);"
> out of the 'for (i = 0; i < fiemap->fm_mapped_extents; i++)" as well.
> So, if there is only one extent, at least 'i == 1' when the loop finished, 
> we'll not hit the
> 'fm_ext[-1]' issue.
>
> my thoughts of the fix looks like below:
>
> memset (fiemap, 0, sizeof fiemap_buf);
> do
>   {
> ioctl (...);
>
> for (i = 0; i < fiemap->fm_mapped_extents; i++)
>   {
> ...
>   }
> fiemap->fm_start = (fm_ext[i-1].fe_logical + fm_ext[i-1].fe_length);
>   } while (! last);

That is better.
Equivalent semantics to my change, but yours avoids unnecessarily
updating fiemap->fm_start for each iteration of the for loop.

...
>> For reference, here's what filefrag -v output looks like,
>> given a file with a nontrivial list of extents:
>>
>>   $ perl -e 'BEGIN{$n=16*1024; *F=*STDOUT}' \
>>  -e 'for (1..5) { sysseek(*F,$n,1)' \
>>  -e '&& syswrite *F,"."x$n or die "$!"}' > j
>>   $ filefrag -v j
>>   Filesystem type is: ef53
>>   File size of j is 163840 (40 blocks, blocksize 4096)
>>ext logical physical expected length flags
>>  0   4  6258884   4
>>  1  12  6258892  6258887  4
>>  2  20  6258900  6258895  4
>>  3  28  6258908  6258903  4
>>  4  36  6258916  6258911  4 eof
>>   j: 6 extents found
> Do we need another test script for this test if we choose `filefrag' to 
> examine the extent info?

Yes, that's why I took the time to do the above.
I've already written most of it.  Will post shortly.

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

2010-05-21 Thread jeff.liu 
Jim Meyering wrote:
> jeff.liu wrote:
> ...
>>> [*] 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.
>> Is it make sense if we write a utility in C through FIEMAP to show the 
>> extent info of a file?
>> then wrap it in our current test scripts or a new test script to compare the 
>> non-sparse blocks
>> offset and length?
> 
> If there were no adequate tool already available, that would be good.
> 
>> filefrag(8) can do such thing(http://e2fsprogs.sourceforge.net/), but maybe 
>> we can implement a
>> compacted version focus on furture extent maping related testing only for 
>> coreutils.
> 
> Or maybe just use filefrag, when it's available.
> On F13, with -v (verbose), it prints this:
> 
> $ filefrag -v big
> Filesystem type is: ef53
> File size of big is 2147483648 (524288 blocks, blocksize 4096)
>  ext logical physical expected length flags
>0   0   254527   1
> big: 1 extent found
> 
> 
>>> ===
>>> 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);
>> strange, code should break if there is no extent allocated for a file.
>>  /* If 0 extents are returned, then more ioctls are not needed.  */
>>   if (fiemap->fm_mapped_extents == 0)
>> break;
> 
> There is one extent, and it is while processing it, with i == 0 that
> would trigger the failure when referencing fm_ext[i-1] (aka fm_ext[-1]).
> 
>>> the obvious fix is probably to do this instead:
>>>
>>> fiemap->fm_start = (fm_ext[i].fe_logical + fm_ext[i].fe_length);
>> I just found a bug for dealing with the 'fiemap->fm_start', maybe it is the 
>> root cause of the
>> segment fault.  above line still need to write as 'fm_ext[i-1].fe_logical 
>> +' to calculate the
>> offset for the next ioctl(2).
> 
> "i" can be 0 there, so it sounds like you're saying we need to
> reference fm_ext[-1].  If you mean that, you'll have to demonstrate
> how we guarantee that i > 0 there.
Sorry for the lack of detailed info for this point, except for removing the 
fiemap->fm_start from
the loop, I need to remove "fiemap->fm_start = (fm_ext[i-1].fe_logical + 
fm_ext[i-1].fe_length);"
out of the 'for (i = 0; i < fiemap->fm_mapped_extents; i++)" as well.
So, if there is only one extent, at least 'i == 1' when the loop finished, 
we'll not hit the
'fm_ext[-1]' issue.

my thoughts of the fix looks like below:

memset (fiemap, 0, sizeof fiemap_buf);
do
  {
ioctl (...);

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

> 
>>> 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);
>> I recalled why I initialized this buf before when you ask me the reason, I 
>> was intented to
>> initialize the 'fiemap->fm_start', so below line 'fiemap->fm_start = 0ULL' 
>> should be removed from
>> the loop.
>>
>>>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.
>> as what I mentioned above, this line should be removed or remove out of the 
>> loop if we do not
>> initialize the fiemap buf.
> 
> I agree.
> Leaving the initialization in the loop would provoke an infinite loop,
> for a file with many extents.
> 
> This demonstrates it:
> 
> $ perl -e 'for (1..100) { sysseek(STDOUT,4096,1)' \
>-e '&& syswrite(STDOUT,"."x4096) or die "$!"}' > j
> $ ./cp --sparse=always j j2
> 
> ^C
> 
> With this statement "fiemap->fm_start = 0ULL;" in the do-while loop,
> the use of ./cp above would infloop.  Without it, it works properly:
> 
> $ env time -f %E ./cp --sparse=always j j2
> 0:00.01
> 
> And we can compare the extents in the two:
> (the awk is mainly to exclude the physical block numbers,
> which will always differ)
> 
> $ diff -u <(filefrag -v j|awk '/^ / {print $1,$2,$NF}') \
>   <(filefrag -v j2|awk '/^ / {print $1,$2,$NF}')
> $
> 
> For reference, here's what filefrag -v output looks like,
> given a file with a nontrivial list of extents:
> 
>   $ perl -e 'BEGIN{$n=16*1024; *F=*STDOUT}' \
>  -e 'for (1..5) { sysseek(*F,$n,1)' \
>  -e '&& syswrite *F,"."x$n or die "$!"}' > j
>   $ filefrag -v j
>   Filesystem type is: ef53
>   File size of j is 163840 (40 blocks, blocksize 4096)
>ext logical physical expected length flags
>  0   4  6258884   4
>  1  12  6258892

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

2010-05-21 Thread jeff.liu 
Hi Jim,

This is the revised version, it fixed the fiemap-start offset calculation 
approach to remove it out
of the 'for (i = 0; i < fiemap->fm_mapped_extents; i++)' loop.

I have not got a 64bits machine for the testing at the moment, at the 
following, the first case only
run againt x86 with valgrind for the non-extent file copy, it works for me, 
could you help verify on
x64?

The second case is to test the non-sparse extents logical offset and length of 
the copied file are
identical to the source file, `ex' is test tool I write in C to show the 
extents info through FIEMAP
ioctl(2), it step through each extent of a file to examine and print out the 
logical offset/extent
length/physical offset.

j...@jeff-laptop:~/opensource_dev/coreutils$ dd if=/dev/null of=/ext4/sp1 bs=1 
seek=2G
j...@jeff-laptop:~/opensource_dev/coreutils$ valgrind --version
valgrind-3.3.0-Debian
j...@jeff-laptop:~/opensource_dev/coreutils$ valgrind ./src/cp --sparse=always 
/ext4/sp1 /ext4/sp2
==13678== Memcheck, a memory error detector.
==13678== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==13678== Using LibVEX rev 1804, a library for dynamic binary translation.
==13678== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==13678== Using valgrind-3.3.0-Debian, a dynamic binary instrumentation 
framework.
==13678== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==13678== For more details, rerun with: -v
==13678==
==13678==
==13678== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 23 from 1)
==13678== malloc/free: in use at exit: 0 bytes in 0 blocks.
==13678== malloc/free: 71 allocs, 71 frees, 10,255 bytes allocated.
==13678== For counts of detected errors, rerun with: -v
==13678== All heap blocks were freed -- no leaks areFrom 
2a2df00acbcc9cdaef723f23efccb65d761d9093
Mon Sep 17 00:00:00 2001

j...@jeff-laptop:~/opensource_dev/coreutils$ ./src/cp --sparse=always 
/ocfs2/sparse_dir/sparse_4
/ext4/sp2
j...@jeff-laptop:~/opensource_dev/coreutils$ ./ex /ext4/sp2
Extents in file "/ext4/sp2":14
Extents returned: 14
Logical: ###[   0]  Ext length: ###[   65536]   Physical: 
###[352321536]
Logical: ###[   98304]  Ext length: ###[   32768]   Physical: 
###[352419840]
Logical: ###[  229376]  Ext length: ###[   32768]   Physical: 
###[352550912]
Logical: ###[  458752]  Ext length: ###[   65536]   Physical: 
###[352780288]
Logical: ###[  950272]  Ext length: ###[   65536]   Physical: 
###[353271808]
Logical: ###[ 1966080]  Ext length: ###[   32768]   Physical: 
###[354287616]
Logical: ###[ 3932160]  Ext length: ###[   65536]   Physical: 
###[356253696]
Logical: ###[ 7897088]  Ext length: ###[   65536]   Physical: 
###[360218624]
Logical: ###[15826944]  Ext length: ###[   65536]   Physical: 
###[384925696]
Logical: ###[31719424]  Ext length: ###[   65536]   Physical: 
###[1004797952]   
Logical: ###[63471616]  Ext length: ###[   65536]   Physical: 
###[1011384320]   
Logical: ###[126976000] Ext length: ###[   65536]   Physical: 
###[1016168448]   
Logical: ###[254017536] Ext length: ###[   65536]   Physical: 
###[1025769472]   
Logical: ###[508100608] Ext length: ###[   32768]   Physical: 
###[1036582912]   
j...@jeff-laptop:~/opensource_dev/coreutils$ ./src/cp --sparse=always /ext4/sp2 
/ext4/sp2_fiemap
j...@jeff-laptop:~/opensource_dev/coreutils$ ./ex /ext4/sp2_fiemap
Extents in file "/ext4/sp2_fiemap":14
Extents returned: 14
Logical: ###[   0]  Ext length: ###[   65536]   Physical: 
###[1040187392]   
Logical: ###[   98304]  Ext length: ###[   32768]   Physical: 
###[1040285696]   
Logical: ###[  229376]  Ext length: ###[   32768]   Physical: 
###[1040416768]   
Logical: ###[  458752]  Ext length: ###[   65536]   Physical: 
###[1040646144]   
Logical: ###[  950272]  Ext length: ###[   65536]   Physical: 
###[1041137664]   
Logical: ###[ 1966080]  Ext length: ###[   32768]   Physical: 
###[1042153472]   
Logical: ###[ 3932160]  Ext length: ###[   65536]   Physical: 
###[1044119552]   
Logical: ###[ 7897088]  Ext length: ###[   65536]   Physical: 
###[1048084480]   
Logical: ###[15826944]  Ext length: ###[   65536]   Physical: 
###[1056014336]   
Logical: ###[31719424]  Ext length: ###[   65536]   Physical: 
###[1063518208]   
Logical: ###[63471616]  Ext length: ###[   65536]   Physical: 
###[1070104576]   
Logical: ###[126976000] Ext length: ###[   65536]   Physical: 
###[1125220352]   
Logical: ###[254017536] Ext length: ###[   65536]   Physical: 
###[1134821376]   
Logical: ###[508100608] Ext length: ###[   32768]   Physical: 
###[1145634816]   


>From 056bb15018466cc2b6b7ae2603fb41b6f61fa084 Mon Sep 17 00:00:00 2001
From: Jie Liu 
Date: Fri, 21 May 2010 22:49:03 +0800
Subject: [PATCH 1/1] cp: Add FIEMAP support for e