Re: [PATCH v2 01/10] btrfs: backref: Introduce the skeleton of btrfs_backref_iter

[Nikolay Borisov <nborisov@suse.com>]

On 2.03.20 г. 11:45 ч., 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 work, like
> searching 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_iter is to avoid such complex and hard to
> read code structure, but something like the following:
> 
>   iter = btrfs_backref_iter_alloc();
>   ret = btrfs_backref_iter_start(iter, bytenr);
>   if (ret < 0)
> 	goto out;
>   for (; ; ret = btrfs_backref_iter_next(iter)) {
> 	/* REAL WORK HERE */
>   }
>   out:
>   btrfs_backref_iter_free(iter);
> 
> This patch is just the skeleton + btrfs_backref_iter_start() code.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/backref.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/backref.h | 60 ++++++++++++++++++++++++++++++++
>  2 files changed, 147 insertions(+)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 327e4480957b..444cd5d31d87 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -2299,3 +2299,90 @@ void free_ipath(struct inode_fs_paths *ipath)
>  	kvfree(ipath->fspath);
>  	kfree(ipath);
>  }
> +
> +int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr)
> +{
> +	struct btrfs_fs_info *fs_info = iter->fs_info;
> +	struct btrfs_path *path = iter->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;
> +	iter->bytenr = bytenr;
> +
> +	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;
> +	}

nit: Please keep the logic of the checks identical as it makes
comprehending the code easier. What I mean is here you do
!(type == COND1 || type == COND2) which really translates to
type != COND && type != COND2. However, when checking for keyed items
you use the second style. Yes, they are both identical in terms of
semantics but one might wonder "why is this done in 2 different ways for
no apparent reason" ?

> +	memcpy(&iter->cur_key, &key, sizeof(key));
> +	iter->item_ptr = btrfs_item_ptr_offset(path->nodes[0],
> +					       path->slots[0]);
> +	iter->end_ptr = iter->item_ptr + btrfs_item_size_nr(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.
> +	 *
> +	 * This is extra precaustion for non skinny-metadata, where
> +	 * EXTENT_ITEM is also used for tree blocks, that we can only use
> +	 * extent flags to determine if it's a tree block.
> +	 */
> +	if (btrfs_extent_flags(path->nodes[0], ei) & BTRFS_EXTENT_FLAG_DATA) {
> +		ret = -ENOTTY;

why not simply -ENOSUPP

> +		goto release;
> +	}

<snip>