Re: fiemap bugs on sparse files.

[Eric Sandeen <sandeen@redhat.com>]

On 2/23/11 3:34 AM, Yongqiang Yang wrote:
> On Wed, Feb 23, 2011 at 4:59 PM, Yongqiang Yang <xiaoqiangnk@gmail.com> wrote:
>> On Wed, Feb 23, 2011 at 5:18 AM, Chris Mason <chris.mason@oracle.com> wrote:
>>> [ resend, sorry if this is a dup ]
>>>
>>> Hi everyone,
>>>
>>> We've had reports on btrfs that cp is giving us files full of zeros
>>> instead of actually copying them.  It was tracked down to a bug with
>>> the btrfs fiemap implementation where it was returning holes for
>>> delalloc ranges.
>>>
>>> Newer versions of cp are trusting fiemap to tell it where the holes
>>> are, which does seem like a pretty neat trick.
>>>
>>> I decided to give xfs and ext4 a shot with a few tests cases too, xfs
>>> passed with all the ones btrfs was getting wrong, and ext4 got the basic
>>> delalloc case right.
>>>
>>> # mkfs.ext4 /dev/xxx
>>> # mount /dev/xxx /mnt
>>> # dd if=/dev/zero of=/mnt/foo bs=1M count=1
>>> # fiemap-test foo
>>> ext:   0 logical: [       0..     255] phys:        0..     255 flags: 0x007 tot: 256
>>>
>>> Horray!  But once we throw a hole in, things go bad:
>>>
>>> # mkfs.ext4 /dev/xxx
>>> # mount /dev/xxx /mnt
>>> # dd if=/dev/zero of=/mnt/foo bs=1M count=1 seek=1
>>> # fiemap-test foo
>>> < no output >
>> Actually, there is no extent in extent tree now, so
>> ext4_ext_walk_space() will pass ext4_ext_fiemap_cb() a variable of
>> struct ext4_ext_cache with the requested length. But in
>> ext4_ext_fiemap_cb() just the paging contains start block is got
>> via find_get_page(), if find_get_page() return null,
>> ext4_ext_fiemap_cb()  thinks the whole request range is empty and it
>> returns request range.
>>
>> In 1st case, find_get_page() will succeed.
>>
>> It seems that we should get no. of pages in page cache if
>> find_get_page() fails, and correct the range to be returned.
> We can call find_get_pages() with nr_pages=1 instead. And we can regulate
> the range with page->index if it is not the the paging contains start block.
> 
> 
>>
>> Right?
>>
>> If right I will send a patch.

Your analysis is correct, the way it's working now is pretty broken (my fault I'm afraid)

Right now we only look at the first page in a "gap" to see if it's delalloc; we need to search through any dirty pages in the gap, since the first page may be a hole, with delalloc ranges coming later.

We need some variant of page cache search, yes.

-Eric