Re: [PATCH v2 1/3] btrfs: backref: Introduce the skeleton of btrfs_backref_iterator

[Nikolay Borisov <nborisov@suse.com>]

On 14.02.20 г. 11:33 ч., Qu Wenruo wrote:
> 
> 
> On 2020/2/14 下午5:19, Nikolay Borisov wrote:
>>
>>
>> On 14.02.20 г. 10:13 ч., Qu Wenruo wrote:
>>> Due to the complex nature of btrfs extent tree, when we want to iterate
>>> all backrefs of one extent, it involves quite a lot of works, like
>>> search the EXTENT_ITEM/METADATA_ITEM, iteration through inline and keyed
>>> backrefs.
>>>
>>> Normally this would result pretty complex code, something like:
>>>   btrfs_search_slot()
>>>   /* Ensure we are at EXTENT_ITEM/METADATA_ITEM */
>>>   while (1) {	/* Loop for extent tree items */
>>> 	while (ptr < end) { /* Loop for inlined items */
>>> 		/* REAL WORK HERE */
>>> 	}
>>>   next:
>>>   	ret = btrfs_next_item()
>>> 	/* Ensure we're still at keyed item for specified bytenr */
>>>   }
>>>
>>> The idea of btrfs_backref_iterator is to avoid such complex and hard to
>>> read code structure, but something like the following:
>>>
>>>   iterator = btrfs_backref_iterator_alloc();
>>>   ret = btrfs_backref_iterator_start(iterator, bytenr);
>>>   if (ret < 0)
>>> 	goto out;
>>>   for (; ; ret = btrfs_backref_iterator_next(iterator)) {
>>> 	/* REAL WORK HERE */
>>>   }
>>>   out:
>>>   btrfs_backref_iterator_free(iterator);
>>>
>>> This patch is just the skeleton + btrfs_backref_iterator_start() code.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  fs/btrfs/backref.c | 58 +++++++++++++++++++++++++++++++++++
>>>  fs/btrfs/backref.h | 76 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 134 insertions(+)
>>>
>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>>> index e5d85311d5d5..73c4829609c9 100644
>>> --- a/fs/btrfs/backref.c
>>> +++ b/fs/btrfs/backref.c
>>> @@ -2252,3 +2252,61 @@ void free_ipath(struct inode_fs_paths *ipath)
>>>  	kvfree(ipath->fspath);
>>>  	kfree(ipath);
>>>  }
>>> +
>>> +int btrfs_backref_iterator_start(struct btrfs_backref_iterator *iterator,
>>> +				 u64 bytenr)
>>> +{
>>> +	struct btrfs_fs_info *fs_info = iterator->fs_info;
>>> +	struct btrfs_path *path = iterator->path;
>>> +	struct btrfs_extent_item *ei;
>>> +	struct btrfs_key key;
>>> +	int ret;
>>> +
>>> +	key.objectid = bytenr;
>>> +	key.type = BTRFS_METADATA_ITEM_KEY;
>>> +	key.offset = (u64)-1;
>>> +
>>> +	ret = btrfs_search_slot(NULL, fs_info->extent_root, &key, path, 0, 0);
>>> +	if (ret < 0)
>>> +		return ret;
>>> +	if (ret == 0) {
>>> +		ret = -EUCLEAN;
>>> +		goto release;
>>> +	}
>>> +	if (path->slots[0] == 0) {
>>> +		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>>> +		ret = -EUCLEAN;
>>> +		goto release;
>>> +	}
>>> +	path->slots[0]--;
>>> +
>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +	if (!(key.type == BTRFS_EXTENT_ITEM_KEY ||
>>> +	      key.type == BTRFS_METADATA_ITEM_KEY) || key.objectid != bytenr) {
>>> +		ret = -ENOENT;
>>> +		goto release;
>>> +	}
>>> +	memcpy(&iterator->cur_key, &key, sizeof(key));
>>> +	iterator->end_ptr = btrfs_item_end_nr(path->nodes[0], path->slots[0]);
>>> +	iterator->item_ptr = btrfs_item_ptr_offset(path->nodes[0],
>>> +						   path->slots[0]);
>>> +	ei = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>> +			    struct btrfs_extent_item);
>>> +
>>> +	/* Only support iteration on tree backref yet */
>>> +	if (btrfs_extent_flags(path->nodes[0], ei) & BTRFS_EXTENT_FLAG_DATA) {
>>> +		ret = -ENOTTY;
>>> +		goto release;
>>> +	}
>>
>> Isn't this implied bye the fact you are searching for METADATA ITEMS to
>> begin with ? Considering this shouldn't detecting EXTENT_FLAG_DATA in
>> the backrefs of a METADATA_EXTENT be considered a corruption?
> 
> For non skinny-metadata fs, we can hit with EXTENT_ITEM.
> So it's still possible to hit a corruption undetected by tree-checker.
> 
> But you're right, we shouldn't really hit a data extent here, as
> previous loops have excluded all data extents.

Then put a comment saying this is done as an extra precaution.

<split>