Re: [PATCH 1/4] btrfs: use correct offset for reloc_inode in prealloc_file_extent_cluster()

[Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>]

hello,

On 07/20/2016 09:18 PM, Josef Bacik wrote:
> On 07/20/2016 01:56 AM, Wang Xiaoguang wrote:
>> In prealloc_file_extent_cluster(), btrfs_check_data_free_space() uses
>> wrong file offset for reloc_inode, it uses cluster->start and 
>> cluster->end,
>> which indeed are extent's bytenr. The correct value should be
>> cluster->[start|end] minus block group's start bytenr.
>>
>> start bytenr   cluster->start
>> |              |     extent      |   extent   | ...| extent |
>> |----------------------------------------------------------------|
>> |                block group reloc_inode |
>>
>> Signed-off-by: Wang Xiaoguang <wangxg.fnst@cn.fujitsu.com>
>> ---
>>  fs/btrfs/relocation.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 0477dca..a0de885 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -3030,12 +3030,14 @@ int prealloc_file_extent_cluster(struct inode 
>> *inode,
>>      u64 num_bytes;
>>      int nr = 0;
>>      int ret = 0;
>> +    u64 prealloc_start = cluster->start - offset;
>> +    u64 prealloc_end = cluster->end - offset;
>>
>>      BUG_ON(cluster->start != cluster->boundary[0]);
>>      inode_lock(inode);
>>
>> -    ret = btrfs_check_data_free_space(inode, cluster->start,
>> -                      cluster->end + 1 - cluster->start);
>> +    ret = btrfs_check_data_free_space(inode, prealloc_start,
>> +                      prealloc_end + 1 - prealloc_start);
>>      if (ret)
>>          goto out;
>>
>> @@ -3056,8 +3058,8 @@ int prealloc_file_extent_cluster(struct inode 
>> *inode,
>>              break;
>>          nr++;
>>      }
>> -    btrfs_free_reserved_data_space(inode, cluster->start,
>> -                       cluster->end + 1 - cluster->start);
>> +    btrfs_free_reserved_data_space(inode, prealloc_start,
>> +                       prealloc_end + 1 - prealloc_start);
>>  out:
>>      inode_unlock(inode);
>>      return ret;
>>
>
> This ends up being the same amount.  Consider this scenario
>
> bg bytenr = 4096
> cluster->start = 8192
> cluster->end = 12287
>
> cluster->end + 1 - cluster->start = 4096
>
> prealloc_start = cluster->start - offset = 0
> prealloc_end = cluster->end - offset = 8191
>
> prealloc_end + 1 - prealloc_start = 4096
>
> You shift both by the same amount, which gives you the same answer.  
> Thanks,
Thanks for reviewing.
Yes, I know the amount of preallocated data space is the same, this patch
does not fix any bugs :)

For every block group to be balanced, we create a corresponding inode.
For this inode, the initial offset should be 0. In your above example,
before this patch, it's btrfs_free_reserved_data_space(inode, 8192, 4096);
with this patch, it's btrfs_free_reserved_data_space(inode, 4096, 4096).

I just want to make btrfs_free_reserved_data_space()'s 'start' argument
be offset inside block group, not offset inside whole fs byternr space. 
I'm not
a  English native, hope that I have expressed what I want to :)

But yes, I'm also OK with removing this patch.

Regards,
Xiaoguang Wang

>
> Josef
>
>