Re: [PATCH v3 2/3] btrfs: backref: Implement btrfs_backref_iterator_next()

[Nikolay Borisov <nborisov@suse.com>]

On 17.02.20 г. 13:45 ч., Qu Wenruo wrote:
> 
> 
> 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@suse.com>
>>>>> ---
>>>>>  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.

Apparently not, btrfs/003 with the following assert:

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index fb0abe344851..403a75f0c99c 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2328,6 +2328,7 @@ int btrfs_backref_iterator_next(struct
btrfs_backref_iterator *iterator)
                        /* Use inline ref type to determine the size */
                        int type;

+                       ASSERT(iterator->cur_ptr < iterator->end_ptr);
                        iref = (struct btrfs_extent_inline_ref *)
                                (iterator->cur_ptr);
                        type = btrfs_extent_inline_ref_type(eb, iref);


Trigger:

[   58.884441] assertion failed: iterator->cur_ptr < iterator->end_ptr,
in fs/btrfs/backref.c:2331


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