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.
