Re: [PATCH v2 05/16] btrfs: don't add metadata items for the remap tree to the extent tree

[Boris Burkov <boris@bur.io>]

On Wed, Aug 13, 2025 at 03:34:47PM +0100, Mark Harmstone wrote:
> There is the following potential problem with the remap tree and delayed refs:
> 
> * Remapped extent freed in a delayed ref, which removes an entry from the
>   remap tree
> * Remap tree now small enough to fit in a single leaf
> * Corruption as we now have a level-0 block with a level-1 metadata item
>   in the extent tree
> 
> One solution to this would be to rework the remap tree code so that it operates
> via delayed refs. But as we're hoping to remove cow-only metadata items in the
> future anyway, change things so that the remap tree doesn't have any entries in
> the extent tree. This also has the benefit of reducing write amplification.
> 
> We also make it so that the clear_cache mount option is a no-op, as with the
> extent tree v2, as the free-space tree can no longer be recreated from the
> extent tree.
> 
> Finally disable relocating the remap tree itself, which is added back in
> a later patch. As it is we would get corruption as the traditional
> relocation method walks the extent tree, and we're removing its metadata
> items.
> 
Reviewed-by: Boris Burkov <boris@bur.io>
> Signed-off-by: Mark Harmstone <mark@harmstone.com>
> ---
>  fs/btrfs/disk-io.c     |  3 +++
>  fs/btrfs/extent-tree.c | 31 ++++++++++++++++++++++++++++++-
>  fs/btrfs/volumes.c     |  3 +++
>  3 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 7e60097b2a96..8e9520119d4f 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3049,6 +3049,9 @@ int btrfs_start_pre_rw_mount(struct btrfs_fs_info *fs_info)
>  		if (btrfs_fs_incompat(fs_info, EXTENT_TREE_V2))
>  			btrfs_warn(fs_info,
>  				   "'clear_cache' option is ignored with extent tree v2");
> +		else if (btrfs_fs_incompat(fs_info, REMAP_TREE))
> +			btrfs_warn(fs_info,
> +				   "'clear_cache' option is ignored with remap tree");
>  		else
>  			rebuild_free_space_tree = true;
>  	} else if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE) &&
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 682d21a73a67..5e038ae1a93f 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1552,6 +1552,28 @@ static void free_head_ref_squota_rsv(struct btrfs_fs_info *fs_info,
>  				  BTRFS_QGROUP_RSV_DATA);
>  }
>  
> +static int drop_remap_tree_ref(struct btrfs_trans_handle *trans,
> +			       const struct btrfs_delayed_ref_node *node)
> +{
> +	u64 bytenr = node->bytenr;
> +	u64 num_bytes = node->num_bytes;
> +	int ret;
> +
> +	ret = btrfs_add_to_free_space_tree(trans, bytenr, num_bytes);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	ret = btrfs_update_block_group(trans, bytenr, num_bytes, false);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int run_delayed_data_ref(struct btrfs_trans_handle *trans,
>  				struct btrfs_delayed_ref_head *href,
>  				const struct btrfs_delayed_ref_node *node,
> @@ -1746,7 +1768,10 @@ static int run_delayed_tree_ref(struct btrfs_trans_handle *trans,
>  	} else if (node->action == BTRFS_ADD_DELAYED_REF) {
>  		ret = __btrfs_inc_extent_ref(trans, node, extent_op);
>  	} else if (node->action == BTRFS_DROP_DELAYED_REF) {
> -		ret = __btrfs_free_extent(trans, href, node, extent_op);
> +		if (node->ref_root == BTRFS_REMAP_TREE_OBJECTID)
> +			ret = drop_remap_tree_ref(trans, node);
> +		else
> +			ret = __btrfs_free_extent(trans, href, node, extent_op);
>  	} else {
>  		BUG();
>  	}
> @@ -4896,6 +4921,9 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
>  	int level = btrfs_delayed_ref_owner(node);
>  	bool skinny_metadata = btrfs_fs_incompat(fs_info, SKINNY_METADATA);
>  
> +	if (unlikely(node->ref_root == BTRFS_REMAP_TREE_OBJECTID))
> +		goto skip;
> +
>  	extent_key.objectid = node->bytenr;
>  	if (skinny_metadata) {
>  		/* The owner of a tree block is the level. */
> @@ -4948,6 +4976,7 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
>  
>  	btrfs_free_path(path);
>  
> +skip:
>  	return alloc_reserved_extent(trans, node->bytenr, fs_info->nodesize);
>  }
>  
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index c95f83305c82..678e5d4cd780 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3993,6 +3993,9 @@ static bool should_balance_chunk(struct extent_buffer *leaf, struct btrfs_chunk
>  	struct btrfs_balance_args *bargs = NULL;
>  	u64 chunk_type = btrfs_chunk_type(leaf, chunk);
>  
> +	if (chunk_type & BTRFS_BLOCK_GROUP_REMAP)
> +		return false;
> +
>  	/* type filter */
>  	if (!((chunk_type & BTRFS_BLOCK_GROUP_TYPE_MASK) &
>  	      (bctl->flags & BTRFS_BALANCE_TYPE_MASK))) {
> -- 
> 2.49.1
>