Re: [PATCH v14 05/42] btrfs: release path before calling into btrfs_load_block_group_zone_info

From: Anand Jain <anand.jain@oracle.com>
To: Naohiro Aota <naohiro.aota@wdc.com>,
	linux-btrfs@vger.kernel.org, dsterba@suse.com
Cc: hare@suse.com, linux-fsdevel@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Johannes Thumshirn <johannes.thumshirn@wdc.com>,
	Josef Bacik <josef@toxicpanda.com>
Subject: Re: [PATCH v14 05/42] btrfs: release path before calling into btrfs_load_block_group_zone_info
Date: Sun, 31 Jan 2021 07:21:38 +0800	[thread overview]
Message-ID: <d7e18cf7-ad33-7052-15c7-3dff3fa8f635@oracle.com> (raw)
In-Reply-To: <d931b0c588dda740d140a1d8e87eb5b138869fc5.1611627788.git.naohiro.aota@wdc.com>

On 1/26/2021 10:24 AM, Naohiro Aota wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Since we have no write pointer in conventional zones, we cannot determine
> the allocation offset from it. Instead, we set the allocation offset after
> the highest addressed extent. This is done by reading the extent tree in
> btrfs_load_block_group_zone_info().
> 
> However, this function is called from btrfs_read_block_groups(), so the
> read lock for the tree node can recursively taken.
> 
> To avoid this unsafe locking scenario, release the path before reading the
> extent tree to get the allocation offset.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>

Looks good to me.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.

> ---
>   fs/btrfs/block-group.c | 39 ++++++++++++++++++---------------------
>   1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 763a3671b7af..bdd20af69dde 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1805,24 +1805,8 @@ static int check_chunk_block_group_mappings(struct btrfs_fs_info *fs_info)
>   	return ret;
>   }
>   
> -static void read_block_group_item(struct btrfs_block_group *cache,
> -				 struct btrfs_path *path,
> -				 const struct btrfs_key *key)
> -{
> -	struct extent_buffer *leaf = path->nodes[0];
> -	struct btrfs_block_group_item bgi;
> -	int slot = path->slots[0];
> -
> -	cache->length = key->offset;
> -
> -	read_extent_buffer(leaf, &bgi, btrfs_item_ptr_offset(leaf, slot),
> -			   sizeof(bgi));
> -	cache->used = btrfs_stack_block_group_used(&bgi);
> -	cache->flags = btrfs_stack_block_group_flags(&bgi);
> -}
> -
>   static int read_one_block_group(struct btrfs_fs_info *info,
> -				struct btrfs_path *path,
> +				struct btrfs_block_group_item *bgi,
>   				const struct btrfs_key *key,
>   				int need_clear)
>   {
> @@ -1837,7 +1821,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>   	if (!cache)
>   		return -ENOMEM;
>   
> -	read_block_group_item(cache, path, key);
> +	cache->length = key->offset;
> +	cache->used = btrfs_stack_block_group_used(bgi);
> +	cache->flags = btrfs_stack_block_group_flags(bgi);
>   
>   	set_free_space_tree_thresholds(cache);
>   
> @@ -1996,19 +1982,30 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>   		need_clear = 1;
>   
>   	while (1) {
> +		struct btrfs_block_group_item bgi;
> +		struct extent_buffer *leaf;
> +		int slot;
> +
>   		ret = find_first_block_group(info, path, &key);
>   		if (ret > 0)
>   			break;
>   		if (ret != 0)
>   			goto error;
>   
> -		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> -		ret = read_one_block_group(info, path, &key, need_clear);
> +		leaf = path->nodes[0];
> +		slot = path->slots[0];
> +
> +		read_extent_buffer(leaf, &bgi,
> +				   btrfs_item_ptr_offset(leaf, slot),
> +				   sizeof(bgi));
> +
> +		btrfs_item_key_to_cpu(leaf, &key, slot);
> +		btrfs_release_path(path);
> +		ret = read_one_block_group(info, &bgi, &key, need_clear);
>   		if (ret < 0)
>   			goto error;
>   		key.objectid += key.offset;
>   		key.offset = 0;
> -		btrfs_release_path(path);
>   	}
>   	btrfs_release_path(path);
>   
> 