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

[Josef Bacik <josef@toxicpanda.com>]

On Mon, Dec 06, 2021 at 10:44:51AM +0000, Filipe Manana wrote:
> 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.
> 

Because refill is about adding more space to keep up with usage.  We're not
adding space at that point.  These things here are to make sure ->size is
correct.  Refill is about making sure ->reserved == ->size.  In this case we're
just trying to add the smallest unit possible, min(1 items worth of
modificaitons, ->size - >reserved).  Thanks,

Josef