Re: [PATCH v2 2/2] btrfs: reserve extra space for the free space tree

[Filipe Manana <fdmanana@kernel.org>]

On Thu, Dec 02, 2021 at 03:34:32PM -0500, Josef Bacik wrote:
> Filipe reported a problem where sometimes he'd get an ENOSPC abort when
> running delayed refs with generic/619 and the free space tree enabled.
> This is partly because we do not reserve space for modifying the free
> space tree, nor do we have a block rsv associated with that tree.
> 
> The delayed_refs_rsv tracks the amount of space required to run delayed
> refs.  This means 1 modification means 1 change to the extent root.
> With the free space tree this turns into 2 changes, because modifying 1
> extent means updating the extent tree and potentially updating the free
> space tree to either remove that entry or add the free space.  Thus if
> we have the FST enabled, simply double the reservation size for our
> modification.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/block-rsv.c   |  1 +
>  fs/btrfs/delayed-ref.c | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
> index b3086f252ad0..b3ee49b0b1e8 100644
> --- a/fs/btrfs/block-rsv.c
> +++ b/fs/btrfs/block-rsv.c
> @@ -426,6 +426,7 @@ void btrfs_init_root_block_rsv(struct btrfs_root *root)
>  	switch (root->root_key.objectid) {
>  	case BTRFS_CSUM_TREE_OBJECTID:
>  	case BTRFS_EXTENT_TREE_OBJECTID:
> +	case BTRFS_FREE_SPACE_TREE_OBJECTID:
>  		root->block_rsv = &fs_info->delayed_refs_rsv;
>  		break;
>  	case BTRFS_ROOT_TREE_OBJECTID:
> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
> index da9d20813147..533521be8fdf 100644
> --- a/fs/btrfs/delayed-ref.c
> +++ b/fs/btrfs/delayed-ref.c
> @@ -84,6 +84,17 @@ void btrfs_delayed_refs_rsv_release(struct btrfs_fs_info *fs_info, int nr)
>  	u64 num_bytes = btrfs_calc_insert_metadata_size(fs_info, nr);
>  	u64 released = 0;
>  
> +	/*
> +	 * We have to check the mount option here because we could be enabling
> +	 * the free space tree for the first time and don't have the compat_ro
> +	 * option set yet.
> +	 *
> +	 * We need extra reservations if we have the free space tree because
> +	 * we'll have to modify that tree as well.
> +	 */
> +	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE))
> +		num_bytes <<= 1;
> +
>  	released = btrfs_block_rsv_release(fs_info, block_rsv, num_bytes, NULL);
>  	if (released)
>  		trace_btrfs_space_reservation(fs_info, "delayed_refs_rsv",
> @@ -108,6 +119,17 @@ void btrfs_update_delayed_refs_rsv(struct btrfs_trans_handle *trans)
>  
>  	num_bytes = btrfs_calc_insert_metadata_size(fs_info,
>  						    trans->delayed_ref_updates);
> +	/*
> +	 * We have to check the mount option here because we could be enabling
> +	 * the free space tree for the first time and don't have the compat_ro
> +	 * option set yet.
> +	 *
> +	 * We need extra reservations if we have the free space tree because
> +	 * we'll have to modify that tree as well.
> +	 */
> +	if (btrfs_test_opt(fs_info, FREE_SPACE_TREE))
> +		num_bytes <<= 1;

Don't we need to bump the minimum (limit variable) number of bytes at
btrfs_delayed_refs_rsv_refill() as well?

I don't see why not.

Thanks.

> +
>  	spin_lock(&delayed_rsv->lock);
>  	delayed_rsv->size += num_bytes;
>  	delayed_rsv->full = 0;
> -- 
> 2.26.3
>