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

Commit Message

robbieko March 7, 2018, 8:20 a.m. UTC

From: Robbie Ko <[e-mail]>

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

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

[FIX]
Use correct disko.

Signed-off-by: Robbie Ko <[e-mail]>
---
 fs/btrfs/extent_io.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

--
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Nikolay Borisov March 7, 2018, 10:19 a.m. UTC | #1

On  7.03.2018 10:20, robbieko wrote:
> From: Robbie Ko <[e-mail]>
> 
>  # 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.


> 
> [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 <[e-mail]>
> ---
>  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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

robbieko March 7, 2018, 10:27 a.m. UTC | #2

Nikolay Borisov 於 2018-03-07 18:19 寫到:
> On  7.03.2018 10:20, robbieko wrote:
>> From: Robbie Ko <[e-mail]>
>> 
>>  # 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.
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 <[e-mail]>
>> ---
>>  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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nikolay Borisov March 7, 2018, 11:15 a.m. UTC | #3

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 <[e-mail]>
>>>
>>>  # 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.

> 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 <[e-mail]>
>>> ---
>>>  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 [e-mail]
>>> 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 [e-mail]
> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

robbieko March 9, 2018, 9:01 a.m. UTC | #4

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 <[e-mail]>
>>>> 
>>>>  # 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.

[REASON]
When fm_extent_count is 0, disko is not initialized correctly,
The value is 0 in this case, not the right bytenr.
It will cause the fiemap merge mechanism to fail.

[FIX]
Use correct disko.

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 <[e-mail]>
>>>> ---
>>>>  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 [e-mail]
>>>> 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 [e-mail]
>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nikolay Borisov March 9, 2018, 9:18 a.m. UTC | #5

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 <[e-mail]>
>>>>>
>>>>>  # 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 <[e-mail]>
>>>>> ---
>>>>>  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 [e-mail]
>>>>> 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 [e-mail]
>>> 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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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