GNU bug report logs -
#6131
[PATCH]: fiemap support for efficient sparse file copy
Previous Next
Reported by: "jeff.liu" <jeff.liu <at> oracle.com>
Date: Fri, 7 May 2010 14:16:02 UTC
Severity: normal
Tags: patch
Done: Jim Meyering <jim <at> meyering.net>
Bug is archived. No further changes may be made.
Full log
Message #38 received at submit <at> debbugs.gnu.org (full text, mbox):
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.
This bug report was last modified 14 years and 119 days ago.
Previous Next
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.