[PATCH] btrfs: qgroup: always free reserved space for extent records

[PATCH] btrfs: qgroup: always free reserved space for extent records

[Qu Wenruo]

[BUG]
If qgroup is marked inconsistent (e.g. caused by operations needing full
subtree rescan, like creating a snapshot and assign to a higher level
qgroup), btrfs would immediately start leaking its data reserved space.

The following script can easily reproduce it:

 mkfs.btrfs -O quota -f $dev
 mount $dev $mnt
 btrfs subv create $mnt/subv1
 btrfs qgroup create 1/0 $mnt

 # This snapshot creation would mark qgroup inconsistent,
 # as the ownership involves different higher level qgroup, thus
 # we have to rescan both source and snapshot, which can be very
 # time consuming, thus here btrfs just choose to mark qgroup
 # inconsistent, and let users to determine when to do the rescan.
 btrfs subv snapshot -i 1/0 $mnt/subv1 $mnt/snap1

 # Now this write would lead to qgroup rsv leak.
 xfs_io -f -c "pwrite 0 64k" $mnt/file1

 # And at unmount time, btrfs would report 64K DATA rsv space leaked.
 umount $mnt

And we would have the following dmesg output for the unmount:

 BTRFS info (device dm-1): last unmount of filesystem 14a3d84e-f47b-4f72-b053-a8a36eef74d3
 BTRFS warning (device dm-1): qgroup 0/5 has unreleased space, type 0 rsv 65536

[CAUSE]
Since commit e15e9f43c7ca ("btrfs: introduce
BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting"),
we introduce a mode for btrfs qgroup to skip the timing consuming
backref walk, if the qgroup is already inconsistent.

But this skip also covered the data reserved freeing, thus the qgroup
reserved space for each newly created data extent would not be freed,
thus cause the leakage.

[FIX]
Make the data extent reserved space freeing mandatory.

The qgroup reserved space handling is way cheaper compared to the
backref walking part, and we always have the super sensitive leak
detector, thus it's definitely worthy to always free the qgroup
reserved data space.

Fixes: e15e9f43c7ca ("btrfs: introduce BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting")
Reported-by: Fabian Vogt <fvogt@suse.com>
Link: https://bugzilla.suse.com/show_bug.cgi?id=1216196
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/qgroup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 3846433d83d9..b3bf08fc2a39 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2957,11 +2957,6 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans)
 				ctx.roots = NULL;
 			}
 
-			/* Free the reserved data space */
-			btrfs_qgroup_free_refroot(fs_info,
-					record->data_rsv_refroot,
-					record->data_rsv,
-					BTRFS_QGROUP_RSV_DATA);
 			/*
 			 * Use BTRFS_SEQ_LAST as time_seq to do special search,
 			 * which doesn't lock tree or delayed_refs and search
@@ -2985,6 +2980,11 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans)
 			record->old_roots = NULL;
 			new_roots = NULL;
 		}
+		/* Free the reserved data space */
+		btrfs_qgroup_free_refroot(fs_info,
+				record->data_rsv_refroot,
+				record->data_rsv,
+				BTRFS_QGROUP_RSV_DATA);
 cleanup:
 		ulist_free(record->old_roots);
 		ulist_free(new_roots);
-- 
2.43.2

Re: [PATCH] btrfs: qgroup: always free reserved space for extent records

[Filipe Manana]

On Fri, Feb 23, 2024 at 7:44 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> If qgroup is marked inconsistent (e.g. caused by operations needing full
> subtree rescan, like creating a snapshot and assign to a higher level
> qgroup), btrfs would immediately start leaking its data reserved space.
>
> The following script can easily reproduce it:
>
>  mkfs.btrfs -O quota -f $dev
>  mount $dev $mnt
>  btrfs subv create $mnt/subv1
>  btrfs qgroup create 1/0 $mnt
>
>  # This snapshot creation would mark qgroup inconsistent,
>  # as the ownership involves different higher level qgroup, thus
>  # we have to rescan both source and snapshot, which can be very
>  # time consuming, thus here btrfs just choose to mark qgroup
>  # inconsistent, and let users to determine when to do the rescan.
>  btrfs subv snapshot -i 1/0 $mnt/subv1 $mnt/snap1
>
>  # Now this write would lead to qgroup rsv leak.
>  xfs_io -f -c "pwrite 0 64k" $mnt/file1
>
>  # And at unmount time, btrfs would report 64K DATA rsv space leaked.
>  umount $mnt
>
> And we would have the following dmesg output for the unmount:
>
>  BTRFS info (device dm-1): last unmount of filesystem 14a3d84e-f47b-4f72-b053-a8a36eef74d3
>  BTRFS warning (device dm-1): qgroup 0/5 has unreleased space, type 0 rsv 65536
>
> [CAUSE]
> Since commit e15e9f43c7ca ("btrfs: introduce
> BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting"),
> we introduce a mode for btrfs qgroup to skip the timing consuming
> backref walk, if the qgroup is already inconsistent.
>
> But this skip also covered the data reserved freeing, thus the qgroup
> reserved space for each newly created data extent would not be freed,
> thus cause the leakage.
>
> [FIX]
> Make the data extent reserved space freeing mandatory.
>
> The qgroup reserved space handling is way cheaper compared to the
> backref walking part, and we always have the super sensitive leak
> detector, thus it's definitely worthy to always free the qgroup
> reserved data space.
>
> Fixes: e15e9f43c7ca ("btrfs: introduce BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting")
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1216196
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/qgroup.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 3846433d83d9..b3bf08fc2a39 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2957,11 +2957,6 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans)
>                                 ctx.roots = NULL;
>                         }
>
> -                       /* Free the reserved data space */
> -                       btrfs_qgroup_free_refroot(fs_info,
> -                                       record->data_rsv_refroot,
> -                                       record->data_rsv,
> -                                       BTRFS_QGROUP_RSV_DATA);
>                         /*
>                          * Use BTRFS_SEQ_LAST as time_seq to do special search,
>                          * which doesn't lock tree or delayed_refs and search
> @@ -2985,6 +2980,11 @@ int btrfs_qgroup_account_extents(struct btrfs_trans_handle *trans)
>                         record->old_roots = NULL;
>                         new_roots = NULL;
>                 }
> +               /* Free the reserved data space */
> +               btrfs_qgroup_free_refroot(fs_info,
> +                               record->data_rsv_refroot,
> +                               record->data_rsv,
> +                               BTRFS_QGROUP_RSV_DATA);
>  cleanup:
>                 ulist_free(record->old_roots);
>                 ulist_free(new_roots);
> --
> 2.43.2
>
>

Re: [PATCH] btrfs: qgroup: always free reserved space for extent records

[David Sterba]

On Fri, Feb 23, 2024 at 06:13:38PM +1030, Qu Wenruo wrote:
> [BUG]
> If qgroup is marked inconsistent (e.g. caused by operations needing full
> subtree rescan, like creating a snapshot and assign to a higher level
> qgroup), btrfs would immediately start leaking its data reserved space.
> 
> The following script can easily reproduce it:
> 
>  mkfs.btrfs -O quota -f $dev
>  mount $dev $mnt
>  btrfs subv create $mnt/subv1
>  btrfs qgroup create 1/0 $mnt
> 
>  # This snapshot creation would mark qgroup inconsistent,
>  # as the ownership involves different higher level qgroup, thus
>  # we have to rescan both source and snapshot, which can be very
>  # time consuming, thus here btrfs just choose to mark qgroup
>  # inconsistent, and let users to determine when to do the rescan.
>  btrfs subv snapshot -i 1/0 $mnt/subv1 $mnt/snap1
> 
>  # Now this write would lead to qgroup rsv leak.
>  xfs_io -f -c "pwrite 0 64k" $mnt/file1
> 
>  # And at unmount time, btrfs would report 64K DATA rsv space leaked.
>  umount $mnt
> 
> And we would have the following dmesg output for the unmount:
> 
>  BTRFS info (device dm-1): last unmount of filesystem 14a3d84e-f47b-4f72-b053-a8a36eef74d3
>  BTRFS warning (device dm-1): qgroup 0/5 has unreleased space, type 0 rsv 65536
> 
> [CAUSE]
> Since commit e15e9f43c7ca ("btrfs: introduce
> BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting"),
> we introduce a mode for btrfs qgroup to skip the timing consuming
> backref walk, if the qgroup is already inconsistent.
> 
> But this skip also covered the data reserved freeing, thus the qgroup
> reserved space for each newly created data extent would not be freed,
> thus cause the leakage.
> 
> [FIX]
> Make the data extent reserved space freeing mandatory.
> 
> The qgroup reserved space handling is way cheaper compared to the
> backref walking part, and we always have the super sensitive leak
> detector, thus it's definitely worthy to always free the qgroup
> reserved data space.
> 
> Fixes: e15e9f43c7ca ("btrfs: introduce BTRFS_QGROUP_RUNTIME_FLAG_NO_ACCOUNTING to skip qgroup accounting")
> Reported-by: Fabian Vogt <fvogt@suse.com>
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1216196
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to for-next, thanks.