Re: [PATCH 1/2] Btrfs: fiemap: pass correct bytenr when fm_extent_count is zero

[Nikolay Borisov <nborisov@suse.com>]

On  9.03.2018 11:01, robbieko wrote:
> Nikolay Borisov 於 2018-03-07 19:15 寫到:
>> On  7.03.2018 12:27, robbieko wrote:
>>> Nikolay Borisov 於 2018-03-07 18:19 寫到:
>>>> On  7.03.2018 10:20, robbieko wrote:
>>>>> From: Robbie Ko <robbieko@synology.com>
>>>>>
>>>>>  # mount /dev/vdb5 /mnt/btrfs
>>>>>  # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file
>>>>>  # xfs_io -c "fiemap -v" /mnt/btrfs/file
>>>>>  /mnt/btrfs/file:
>>>>>  EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>>>>>    0: [0..127]:        25088..25215       128   0x1
>>>>>
>>>>> Run fiemap with fm_extent_count set to 0, we'll get wrong value 4
>>>>> instead of 1.
>>>>
>>>> Wrong value 4 instead of 1 for which exact column, the flags? State
>>>> this
>>>> explicitly.
>>>>
>>>> Also this seems a bit bogus since fiemap's documentation states:
>>>>
>>>> If fm_extent_count is zero, then the fm_extents[] array is ignored (no
>>>> extents will be returned), and the fm_mapped_extents count will hold
>>>> the
>>>> number of extents needed in fm_extents[] to hold the file's current
>>>> mapping.
>>>>
>>>> So when fm_extent_count we shouldn't really be returning anything from
>>>> kernel.
>>>>
>>>
>>> Sorry I did not explain clearly.
>>> The value is fm_mapped_extents.
>>
>> But fm_mapped_extents is tagged as an OUT member, meaning the user has
>> no job writing to it.
>>
> 
> 
> [BUG]
> fm_mapped_extents is not correct when fm_extent_count is 0
> Like:
>   # mount /dev/vdb5 /mnt/btrfs
>   # dd if=/dev/zero bs=16K count=4 oflag=dsync of=/mnt/btrfs/file
>   # xfs_io -c "fiemap -v" /mnt/btrfs/file
>   /mnt/btrfs/file:
>   EXT: FILE-OFFSET      BLOCK-RANGE      TOTAL FLAGS
>     0: [0..127]:        25088..25215       128   0x1
> 
> When user space wants to get the number of file extents,
> set fm_extent_count to 0 to run fiemap and then read fm_mapped_extents.
> 
> In the above example, fiemap will return with fm_mapped_extents set to 4 ,
> but it should be 1 since there's only one entry in the output.

This is slightly better.

> 
> [REASON]
> When fm_extent_count is 0, disko is not initialized correctly,
> The value is 0 in this case, not the right bytenr.

The value of what, be more explicit.

"
Also the problem seems to be that disko is only set if
fieinfo->fi_extents_max is set. And this member is initialized, in the
generic ioctl_fiemap function, to the value of used-passed
fm_extent_count. So when the user passes 0 then fi_extent_max is also
set to zero and this causes btrfs to not initialize disko at all.
Eventually this leads emit_fiemap_extent being called with a bogus
'phys' argument preventing proper fiemap entries merging.
"


You see how complicated in fact the issue is, and your 2 line
explanation is not really making it clear.

> It will cause the fiemap merge mechanism to fail.


> 
> [FIX]
> Use correct disko.

This is as uninformative as it gets... What about something like :

"Move the disko initialization earlier in extent_fiemap making it
independent of user-passed arguments, allowing emit_fiemap_extent to
properly handle consecutive extent entries."

> 
> Thanks.
> Robbie Ko
> 
>>> If fm_extent_count  is zero, the fm_mapped_extents count will hold the
>>> number of extents needed.
>>>
>>>
>>>>
>>>>>
>>>>> [REASON]
>>>>> When fm_extent_count is 0, disko is not initialized correctly,
>>>>> The value is 0 in this case, not the right bytenr.
>>>>
>>>> This is too sparse, be more explicit i.e. that disko=0 is passed to
>>>> emit_fiemap_extent which then leads to issues.
>>>>
>>>>>
>>>>> [FIX]
>>>>> Use correct disko.
>>>>>
>>>>> Signed-off-by: Robbie Ko <robbieko@synology.com>
>>>>> ---
>>>>>  fs/btrfs/extent_io.c | 4 +---
>>>>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>> index 012d638..066b6df 100644
>>>>> --- a/fs/btrfs/extent_io.c
>>>>> +++ b/fs/btrfs/extent_io.c
>>>>> @@ -4567,7 +4567,7 @@ int extent_fiemap(struct inode *inode, struct
>>>>> fiemap_extent_info *fieinfo,
>>>>>              offset_in_extent = em_start - em->start;
>>>>>          em_end = extent_map_end(em);
>>>>>          em_len = em_end - em_start;
>>>>> -        disko = 0;
>>>>> +        disko = em->block_start + offset_in_extent;
>>>>>          flags = 0;
>>>>>
>>>>>          /*
>>>>> @@ -4590,8 +4590,6 @@ int extent_fiemap(struct inode *inode, struct
>>>>> fiemap_extent_info *fieinfo,
>>>>>              u64 bytenr = em->block_start -
>>>>>                  (em->start - em->orig_start);
>>>>>
>>>>> -            disko = em->block_start + offset_in_extent;
>>>>> -
>>>>>              /*
>>>>>               * As btrfs supports shared space, this information
>>>>>               * can be exported to userspace tools via
>>>>> -- 
>>>>> 1.9.1
>>>>>
>>>>> -- 
>>>>> To unsubscribe from this list: send the line "unsubscribe
>>>>> linux-btrfs" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
> 
>