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

[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
>     <INFLOOP!>
>     ^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
Do we need another test script for this test if we choose `filefrag' to examine 
the extent info?
I'd like to handle it.
> 
> 
> 


Thanks,
-Jeff

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