On 2020/2/17 下午7:42, Nikolay Borisov wrote:
>
>
> On 17.02.20 г. 13:29 ч., Qu Wenruo wrote:
>>
>>
>> On 2020/2/17 下午6:47, Nikolay Borisov wrote:
>>>
>>>
>>> On 17.02.20 г. 8:31 ч., Qu Wenruo wrote:
>>>> This function will go next inline/keyed backref for
>>>> btrfs_backref_iterator infrastructure.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>>>> ---
>>>> fs/btrfs/backref.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>>>> fs/btrfs/backref.h | 34 +++++++++++++++++++++++++++++++++
>>>> 2 files changed, 81 insertions(+)
>>>>
>>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>>> index 8bd5e067831c..fb0abe344851 100644
>>>> --- a/fs/btrfs/backref.c
>>>> +++ b/fs/btrfs/backref.c
>>>> @@ -2310,3 +2310,50 @@ int btrfs_backref_iterator_start(struct btrfs_backref_iterator *iterator,
>>>> btrfs_backref_iterator_release(iterator);
>>>> return ret;
>>>> }
>>>> +
>>>> +int btrfs_backref_iterator_next(struct btrfs_backref_iterator *iterator)
>>>
>>> Document the return values: 0 in case there are more backerfs for the
>>> given bytenr or 1 in case there are'nt. And a negative value in case of
>>> error.
>>>
>>>> +{
>>>> + struct extent_buffer *eb = btrfs_backref_get_eb(iterator);
>>>> + struct btrfs_path *path = iterator->path;
>>>> + struct btrfs_extent_inline_ref *iref;
>>>> + int ret;
>>>> + u32 size;
>>>> +
>>>> + if (btrfs_backref_iterator_is_inline_ref(iterator)) {
>>>> + /* We're still inside the inline refs */
>>>> + if (btrfs_backref_has_tree_block_info(iterator)) {
>>>> + /* First tree block info */
>>>> + size = sizeof(struct btrfs_tree_block_info);
>>>> + } else {
>>>> + /* Use inline ref type to determine the size */
>>>> + int type;
>>>> +
>>>> + iref = (struct btrfs_extent_inline_ref *)
>>>> + (iterator->cur_ptr);
>>>> + type = btrfs_extent_inline_ref_type(eb, iref);
>>>> +
>>>> + size = btrfs_extent_inline_ref_size(type);
>>>> + }
>>>> + iterator->cur_ptr += size;
>>>> + if (iterator->cur_ptr < iterator->end_ptr)
>>>> + return 0;
>>>> +
>>>> + /* All inline items iterated, fall through */
>>>> + }
>>>
>>> This if could be rewritten as:
>>> if (btrfs_backref_iterator_is_inline_ref(iterator) && iterator->cur_ptr
>>> < iterator->end_ptr)
>>>
>>> what this achieves is:
>>>
>>> 1. Clarity that this whole branch is executed only if we are within the
>>> inline refs limits
>>> 2. It also optimises that function since in the current version, after
>>> the last inline backref has been processed iterator->cur_ptr ==
>>> iterator->end_ptr. On the next call to btrfs_backref_iterator_next you
>>> will execute (needlessly)
>>>
>>> (struct btrfs_extent_inline_ref *) (iterator->cur_ptr);
>>> type = btrfs_extent_inline_ref_type(eb, iref);
>>> size = btrfs_extent_inline_ref_size(type);
>>> iterator->cur_ptr += size;
>>> only to fail "if (iterator->cur_ptr < iterator->end_ptr)" check and
>>> continue processing keyed items.
>>>
>>> As a matter of fact you will be reading past the metadata_item since
>>> cur_ptr will be at the end of them and any deferences will read from the
>>> next item this might not cause a crash but it's still wrong.
>>
>> This shouldn't happen, as we must ensure the cur_ptr < item_end for callers.
>
>
> How are you ensuring this? Before processing the last inline ref
> cur_ptr would be end_ptr - btrfs_extent_inline_ref_size(type);
Firstly, in _start() call, we can easily check if we have any inline refs.
If no, search next item.
If yes, return cur_ptr which points to the current inline extent ref.
Secondly, in _next() call, we keep current check. Increase cur_ptr, then
check against ptr_end.
So that, all backref_iter callers will get a cur_ptr that is always
smaller than ptr_end.
Thanks,
Qu
>
> After it's processed cur_ptr == end_ptr. THen you will do another call
> to btrfs_backref_iterator_next which will do the same calculation? What
> am I missing?
>
>>
>> For the _next() call, the check after increased cur_ptr check it's OK.
>>
>> But it's a problem for _start() call, as we may have a case where an
>> EXTENT_ITEM/METADATA_ITEM has no inlined ref.
>>
>> I'll fix this in next version.
>>
>> Thanks,
>> Qu
>>
>>>
>>>
>>>> + /* We're at keyed items, there is no inline item, just go next item */
>>>> + ret = btrfs_next_item(iterator->fs_info->extent_root, iterator->path);
>>>> + if (ret > 0 || ret < 0)
>>>> + return ret;
>>>
>>> nit: if (ret != 0) return ret;
>>>
>>> <snip>
>>>
