Re: [PATCH 3/5] btrfs: free qgroup pertrans rsv on trans abort

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/12/2 07:30, Boris Burkov wrote:
> If we abort a transaction, we never run the code that frees the pertrans
> qgroup reservation. This results in warnings on unmount as that
> reservation has been leaked. The leak isn't a huge issue since the fs is
> read-only, but it's better to clean it up when we know we can/should. Do
> it during the cleanup_transaction step of aborting.
>
> Signed-off-by: Boris Burkov <boris@bur.io>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/disk-io.c | 28 ++++++++++++++++++++++++++++
>   fs/btrfs/qgroup.c  |  5 +++--
>   2 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 9317606017e2..a1f440cd6d45 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4775,6 +4775,32 @@ void btrfs_cleanup_dirty_bgs(struct btrfs_transaction *cur_trans,
>   	}
>   }
>
> +static void btrfs_free_all_qgroup_pertrans(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_root *gang[8];
> +	int i;
> +	int ret;
> +
> +	spin_lock(&fs_info->fs_roots_radix_lock);
> +	while (1) {
> +		ret = radix_tree_gang_lookup_tag(&fs_info->fs_roots_radix,
> +						 (void **)gang, 0,
> +						 ARRAY_SIZE(gang),
> +						 0); // BTRFS_ROOT_TRANS_TAG
> +		if (ret == 0)
> +			break;
> +		for (i = 0; i < ret; i++) {
> +			struct btrfs_root *root = gang[i];
> +
> +			btrfs_qgroup_free_meta_all_pertrans(root);
> +			radix_tree_tag_clear(&fs_info->fs_roots_radix,
> +					(unsigned long)root->root_key.objectid,
> +					0); // BTRFS_ROOT_TRANS_TAG
> +		}
> +	}
> +	spin_unlock(&fs_info->fs_roots_radix_lock);
> +}
> +
>   void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
>   				   struct btrfs_fs_info *fs_info)
>   {
> @@ -4803,6 +4829,8 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
>   				     EXTENT_DIRTY);
>   	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
>
> +	btrfs_free_all_qgroup_pertrans(fs_info);
> +
>   	cur_trans->state =TRANS_STATE_COMPLETED;
>   	wake_up(&cur_trans->commit_wait);
>   }
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index a953c16c7eb8..daec90342dad 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4337,8 +4337,9 @@ static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
>
>   		qgroup_rsv_release(fs_info, qgroup, num_bytes,
>   				BTRFS_QGROUP_RSV_META_PREALLOC);
> -		qgroup_rsv_add(fs_info, qgroup, num_bytes,
> -				BTRFS_QGROUP_RSV_META_PERTRANS);
> +		if (!sb_rdonly(fs_info->sb))
> +			qgroup_rsv_add(fs_info, qgroup, num_bytes,
> +				       BTRFS_QGROUP_RSV_META_PERTRANS);
>
>   		list_for_each_entry(glist, &qgroup->groups, next_group)
>   			qgroup_iterator_add(&qgroup_list, glist->group);