bug#6131: [PATCH]: fiemap support for efficient sparse file copy (original) (raw)

[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

From: jeff.liu
Subject: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Fri, 21 May 2010 23:31:53 +0800
User-agent: Thunderbird 2.0.0.14 (X11/20080505)

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->fmstart = (fmext[i-1].felogical + fmext[i-1].felength); > 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->fmmappedextents == 0) > break;

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

>> the obvious fix is probably to do this instead: >> >> fiemap->fmstart = (fmext[i].felogical + fmext[i].felength); > I just found a bug for dealing with the 'fiemap->fmstart', maybe it is the > root cause of the > segment fault. above line still need to write as 'fmext[i-1].felogical > +....' 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 fmext[-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 fiemapbuf array, like this: >> >> + memset (fiemapbuf, 0, sizeof fiemapbuf); > I recalled why I initialized this buf before when you ask me the reason, I > was intented to > initialize the 'fiemap->fmstart', so below line 'fiemap->fmstart = 0ULL' > should be removed from > the loop. > >> do >> { >> fiemap->fmstart = 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->fmstart = 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,1,1,2,$NF}') _ <(filefrag -v j2|awk '/^ / {print 1,1,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.