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

[Josef Bacik <josef@toxicpanda.com>]

On Thu, Dec 02, 2021 at 04:14:51PM +0200, Nikolay Borisov wrote:
> 
> 
> On 1.12.21 г. 18:45, 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.  Fix
> > this by making sure any free space tree defaults to using the
> > delayed_refs_rsv, and make sure we reserve the space for those
> > allocations.
> > 
> > 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;
> 
> That num_bytes * 2 is heuristically derived, right? If so I'd like this
> to be mentioned explicitly in the changelog.

No, the delayed refs is number of items we're modifying in the extent tree.  If
we have the free space tree we have to modify the same number of items in the
free space tree, so it's 2x the space reservations required.  Thanks,

Josef