GNU bug report logs - #6131
[PATCH]: fiemap support for efficient sparse file copy

Previous Next

Full log

Message #29 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: "jeff.liu" <jeff.liu <at> oracle.com>
Cc: Sunil Mushran <sunil.mushran <at> oracle.com>, Tao Ma <tao.ma <at> oracle.com>,
	bug-coreutils <at> gnu.org, Joel Becker <Joel.Becker <at> oracle.com>,
	Chris Mason <chris.mason <at> oracle.com>
Subject: Re: bug#6131: [PATCH]: fiemap support for efficient sparse file copy
Date: Fri, 21 May 2010 16:27:49 +0200

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.