Re: [PATCH v2 2/3] btrfs: remove free space items when creating free space tree

[Josef Bacik <josef@toxicpanda.com>]

On 9/9/20 5:45 PM, Boris Burkov wrote:
> When the file system transitions from space cache v1 to v2 it removes
> the old cached data, but does not remove the FREE_SPACE items nor the
> free space inodes they point to. This doesn't cause any issues besides
> being a bit inefficient, since these items no longer do anything useful.
> 
> To fix it, as part of populating the free space tree, destroy each block
> group's free space item and free space inode. This code is lifted from
> the existing code for removing them when removing the block group.
> 
> References: https://github.com/btrfs/btrfs-todo/issues/5
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> v2:
> - remove_free_space_inode -> btrfs_remove_free_space_inode
> - undo sinful whitespace change
> 
>   fs/btrfs/block-group.c      | 39 ++----------------------------
>   fs/btrfs/free-space-cache.c | 48 +++++++++++++++++++++++++++++++++++++
>   fs/btrfs/free-space-cache.h |  2 ++
>   fs/btrfs/free-space-tree.c  |  3 +++
>   4 files changed, 55 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 01e8ba1da1d3..e5e5baad88d8 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -892,8 +892,6 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	struct btrfs_path *path;
>   	struct btrfs_block_group *block_group;
>   	struct btrfs_free_cluster *cluster;
> -	struct btrfs_root *tree_root = fs_info->tree_root;
> -	struct btrfs_key key;
>   	struct inode *inode;
>   	struct kobject *kobj = NULL;
>   	int ret;
> @@ -971,42 +969,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans,
>   	spin_unlock(&trans->transaction->dirty_bgs_lock);
>   	mutex_unlock(&trans->transaction->cache_write_mutex);
>   
> -	if (!IS_ERR(inode)) {
> -		ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> -		if (ret) {
> -			btrfs_add_delayed_iput(inode);
> -			goto out;
> -		}
> -		clear_nlink(inode);
> -		/* One for the block groups ref */
> -		spin_lock(&block_group->lock);
> -		if (block_group->iref) {
> -			block_group->iref = 0;
> -			block_group->inode = NULL;
> -			spin_unlock(&block_group->lock);
> -			iput(inode);
> -		} else {
> -			spin_unlock(&block_group->lock);
> -		}
> -		/* One for our lookup ref */
> -		btrfs_add_delayed_iput(inode);
> -	}
> -
> -	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> -	key.type = 0;
> -	key.offset = block_group->start;
> -
> -	ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1);
> -	if (ret < 0)
> +	ret = btrfs_remove_free_space_inode(trans, block_group);
> +	if (ret)
>   		goto out;
> -	if (ret > 0)
> -		btrfs_release_path(path);
> -	if (ret == 0) {
> -		ret = btrfs_del_item(trans, tree_root, path);
> -		if (ret)
> -			goto out;
> -		btrfs_release_path(path);
> -	}
>   
>   	spin_lock(&fs_info->block_group_cache_lock);
>   	rb_erase(&block_group->cache_node,
> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
> index 8759f5a1d6a0..da6d436c58fa 100644
> --- a/fs/btrfs/free-space-cache.c
> +++ b/fs/btrfs/free-space-cache.c
> @@ -207,6 +207,54 @@ int create_free_space_inode(struct btrfs_trans_handle *trans,
>   					 ino, block_group->start);
>   }
>   
> +int btrfs_remove_free_space_inode(struct btrfs_trans_handle *trans,
> +				  struct btrfs_block_group *block_group)
> +{
> +	struct btrfs_path *path;
> +	struct inode *inode;
> +	struct btrfs_key key;
> +	int ret = 0;
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	inode = lookup_free_space_inode(block_group, path);
> +	if (IS_ERR(inode)) {
> +		if (PTR_ERR(inode) != -ENOENT)
> +			ret = PTR_ERR(inode);
> +		goto out;
> +	}
> +	ret = btrfs_orphan_add(trans, BTRFS_I(inode));
> +	if (ret) {
> +		btrfs_add_delayed_iput(inode);
> +		goto out;
> +	}
> +	clear_nlink(inode);
> +	/* One for the block groups ref */
> +	spin_lock(&block_group->lock);
> +	if (block_group->iref) {
> +		block_group->iref = 0;
> +		block_group->inode = NULL;
> +		spin_unlock(&block_group->lock);
> +		iput(inode);
> +	} else {
> +		spin_unlock(&block_group->lock);
> +	}
> +	/* One for our lookup ref */
> +	btrfs_add_delayed_iput(inode);
> +
> +	key.objectid = BTRFS_FREE_SPACE_OBJECTID;
> +	key.type = 0;
> +	key.offset = block_group->start;
> +	ret = btrfs_search_slot(trans, trans->fs_info->tree_root, &key, path, -1, 1);
> +	if (ret != 0)
> +		goto out;

This leaks the ret == 1 if we don't have a free space cache objectid.  This 
needs to be

if (ret) {
	if (ret > 0)
		ret = 0;
	goto out;
}

or something like that.  Thanks,

Josef