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

[Josef Bacik <jbacik@fb.com>]

On 07/20/2016 09:49 PM, Wang Xiaoguang wrote:
> 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.
>

Oh I see, ok that's fine if we're just trying to make it look sane then go for it.

Signed-off-by: Josef Bacik <jbacik@fb.com>

Thanks,

Josef