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