Re: [PATCH v8 10/17] btrfs: handle deletions from remapped block group

[Chris Mason <clm@meta.com>]

Mark Harmstone <mark@harmstone.com> wrote:
> Handle the case where we free an extent from a block group that has the
> REMAPPED flag set. Because the remap tree is orthogonal to the extent
> tree, for data this may be within any number of identity remaps or
> actual remaps. If we're freeing a metadata node, this will be wholly
> inside one or the other.
> 
> btrfs_remove_extent_from_remap_tree() searches the remap tree for the
> remaps that cover the range in question, then calls
> remove_range_from_remap_tree() for each one, to punch a hole in the
> remap and adjust the free-space tree.
> 
> For an identity remap, remove_range_from_remap_tree() will adjust the
> block group's `identity_remap_count` if this changes. If it reaches
> zero we mark the block group as fully remapped.
> 
> For an identity remap, remove_range_from_remap_tree() will adjust the
> block group's `identity_remap_count` if this changes. If it reaches
> zero we mark the block group as fully remapped.
> 
> Fully remapped block groups have their chunk stripes removed and their
> device extents freed, which makes the disk space available again to the
> chunk allocator. This happens asynchronously: in the cleaner thread for
> sync discard and nodiscard, and (in a later patch) in the discard worker
> for async discard.

Hi everyone,

Another AI review from my recent linux-next run:

> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c

[ ... ]

> +int btrfs_last_identity_remap_gone(struct btrfs_chunk_map *chunk_map,
> +				   struct btrfs_block_group *bg)
> +{
> +	struct btrfs_fs_info *fs_info = bg->fs_info;
> +	struct btrfs_trans_handle *trans;
> +	int ret;
> +	unsigned int num_items;
> +	BTRFS_PATH_AUTO_FREE(path);
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	/*
> +	 * One item for each entry we're removing in the dev extents tree, and
> +	 * another for each device. DUP chunks are all on one device,
> +	 * everything else has one device per stripe.
> +	 */
> +	if (bg->flags & BTRFS_BLOCK_GROUP_DUP)
> +		num_items = chunk_map->num_stripes + 1;
> +	else
> +		num_items = 2 * chunk_map->num_stripes;
> +
> +	trans = btrfs_start_transaction_fallback_global_rsv(fs_info->tree_root, num_items);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +
> +	ret = btrfs_remove_dev_extents(trans, chunk_map);
> +	if (unlikely(ret)) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}

Can this leak the transaction handle? btrfs_abort_transaction() marks
the transaction as aborted but does not end it. __btrfs_abort_transaction()
has a comment explaining cleanup happens in btrfs_end_transaction().

> +
> +	mutex_lock(&trans->fs_info->chunk_mutex);
> +	for (unsigned int i = 0; i < chunk_map->num_stripes; i++) {
> +		ret = btrfs_update_device(trans, chunk_map->stripes[i].dev);
> +		if (unlikely(ret)) {
> +			mutex_unlock(&trans->fs_info->chunk_mutex);
> +			btrfs_abort_transaction(trans, ret);
> +			return ret;
> +		}

Same question here about leaking the transaction handle.

> +	}
> +	mutex_unlock(&trans->fs_info->chunk_mutex);
> +
> +	write_lock(&trans->fs_info->mapping_tree_lock);
> +	btrfs_chunk_map_device_clear_bits(chunk_map, CHUNK_ALLOCATED);
> +	write_unlock(&trans->fs_info->mapping_tree_lock);
> +
> +	btrfs_remove_bg_from_sinfo(bg);
> +
> +	ret = remove_chunk_stripes(trans, chunk_map, path);
> +	if (unlikely(ret)) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}

And here as well - does this return path need btrfs_end_transaction()?

> +
> +	ret = btrfs_commit_transaction(trans);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

[ ... ]

> +static int remove_range_from_remap_tree(struct btrfs_trans_handle *trans,
> +					struct btrfs_path *path,
> +					struct btrfs_block_group *bg,
> +					u64 hole_start, u64 hole_length)
> +{

[ ... ]

> +	if (!is_identity_remap) {
> +		struct btrfs_block_group *dest_bg;
> +
> +		dest_bg = btrfs_lookup_block_group(fs_info, new_addr);
> +		adjust_block_group_remap_bytes(trans, dest_bg, -overlap_length);
> +		btrfs_put_block_group(dest_bg);

btrfs_lookup_block_group() can return NULL if no block group is found
for the address. Every other caller checks for NULL before using the
result. Is a NULL check needed here before passing dest_bg to
adjust_block_group_remap_bytes()? That function does spin_lock(&bg->lock)
which would crash on a NULL pointer.

> +		ret = btrfs_add_to_free_space_tree(trans,
> +						   hole_start - remap_start + new_addr,
> +						   overlap_length);
> +		if (ret)
> +			return ret;
> +	}

[ ... ]

This is not a bug, but the commit message appears to have a duplicated
paragraph about identity remaps:

    For an identity remap, remove_range_from_remap_tree() will adjust the
    block group's `identity_remap_count` if this changes. If it reaches
    zero we mark the block group as fully remapped.

    For an identity remap, remove_range_from_remap_tree() will adjust the
    block group's `identity_remap_count` if this changes. If it reaches
    zero we mark the block group as fully remapped.