Re: [PATCH 1/2] btrfs: include the free space tree in the global rsv minimum calculation

[David Sterba <dsterba@suse.cz>]

On Thu, Dec 02, 2021 at 11:07:51AM +0200, Nikolay Borisov wrote:
> > diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
> > index 21ac60ec19f6..b3086f252ad0 100644
> > --- a/fs/btrfs/block-rsv.c
> > +++ b/fs/btrfs/block-rsv.c
> > @@ -352,25 +352,29 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
> >  {
> >  	struct btrfs_block_rsv *block_rsv = &fs_info->global_block_rsv;
> >  	struct btrfs_space_info *sinfo = block_rsv->space_info;
> > -	struct btrfs_root *extent_root = btrfs_extent_root(fs_info, 0);
> > -	struct btrfs_root *csum_root = btrfs_csum_root(fs_info, 0);
> > -	u64 num_bytes;
> > -	unsigned min_items;
> > +	struct btrfs_root *root, *tmp;
> > +	u64 num_bytes = btrfs_root_used(&fs_info->tree_root->root_item);
> > +	unsigned int min_items = 1;
> >  
> >  	/*
> >  	 * The global block rsv is based on the size of the extent tree, the
> >  	 * checksum tree and the root tree.  If the fs is empty we want to set
> >  	 * it to a minimal amount for safety.
> > +	 *
> > +	 * We also are going to need to modify the minimum of the tree root and
> > +	 * any global roots we could touch.
> >  	 */
> > -	num_bytes = btrfs_root_used(&extent_root->root_item) +
> > -		btrfs_root_used(&csum_root->root_item) +
> > -		btrfs_root_used(&fs_info->tree_root->root_item);
> > -
> > -	/*
> > -	 * We at a minimum are going to modify the csum root, the tree root, and
> > -	 * the extent root.
> > -	 */
> > -	min_items = 3;
> > +	read_lock(&fs_info->global_root_lock);
> > +	rbtree_postorder_for_each_entry_safe(root, tmp, &fs_info->global_root_tree,
> > +					     rb_node) {
> > +		if (root->root_key.objectid == BTRFS_EXTENT_TREE_OBJECTID ||
> > +		    root->root_key.objectid == BTRFS_CSUM_TREE_OBJECTID ||
> > +		    root->root_key.objectid == BTRFS_FREE_SPACE_TREE_OBJECTID) {
> > +			num_bytes += btrfs_root_used(&root->root_item);
> > +			min_items++;
> > +		}
> > +	}
> 
> nit: I think those changes constitute 2 patches - the first does the
> global_root_tree iteration as preparation for global roots. And the
> subsequent patch should also include the freespace tree, no ? Otherwise
> LGTM.

As this is probably a fix we'd like to backport, the oder of the
suggested patches would have to be reversed too, so we first have the
fix and then port to the global roots. But the diff is short and a
backport to non-global roots could be done in one go as well,
referencing this patch.