On Tue, Jan 23, 2024 at 10:45:12AM -0800, Boris Burkov wrote:
> When using squotas, an extent's OWNER_REF can long outlive the subvolume
> that is the owner, since it could pick up a different reference that
> keeps it around, but the subvolume can go away.
>
> Test this case, as originally, it resulted in a read only btrfs.
>
> Since we can blow up the subvolume in the same transaction as the extent
> is written, we can also increment the usage of a non-existent subvolume.
>
> This leaves an OWNER_REF behind with no corresponding incremented usage
> in a qgroup, so if we re-create that qgroup, we can then underflow its
> usage.
>
> Both of these cases are fixed in the kernel by disallowing
> creating subvol qgroups and by disallowing deleting qgroups that still
> have usage.
>
> Signed-off-by: Boris Burkov <boris@xxxxxx>
> ---
> Changelog:
> v2:
> - removed enable quota helper
> - removed unneeded commented cleanup boilerplate
> - change test number 304 -> 310 (based on v2024.01.14)
You don't need to write the number of a test case in commit subject, due to
it might be changed. If you write a new case, the subject can be "btrfs: ...."
or "fstests/btrfs: ..." or others similar you like.
Thanks,
Zorro
>
> tests/btrfs/301 | 14 ++------
> tests/btrfs/310 | 83 +++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/310.out | 6 ++++
> 3 files changed, 91 insertions(+), 12 deletions(-)
> create mode 100755 tests/btrfs/310
> create mode 100644 tests/btrfs/310.out
>
> diff --git a/tests/btrfs/301 b/tests/btrfs/301
> index db4697247..4c1127aa0 100755
> --- a/tests/btrfs/301
> +++ b/tests/btrfs/301
> @@ -157,16 +157,6 @@ do_enospc_falloc()
> do_falloc $file $sz
> }
>
> -enable_quota()
> -{
> - local mode=$1
> -
> - [ $mode == "n" ] && return
> - arg=$([ $mode == "s" ] && echo "--simple")
> -
> - $BTRFS_UTIL_PROG quota enable $arg $SCRATCH_MNT
> -}
> -
> get_subvid()
> {
> _btrfs_get_subvolid $SCRATCH_MNT subv
> @@ -186,7 +176,7 @@ prepare()
> {
> _scratch_mkfs >> $seqres.full
> _scratch_mount
> - enable_quota "s"
> + $BTRFS_UTIL_PROG quota enable --simple $SCRATCH_MNT
> $BTRFS_UTIL_PROG subvolume create $subv >> $seqres.full
> local subvid=$(get_subvid)
> set_subvol_limit $subvid $limit
> @@ -397,7 +387,7 @@ enable_mature()
> # Sync before enabling squotas to reliably *not* count the writes
> # we did before enabling.
> sync
> - enable_quota "s"
> + $BTRFS_UTIL_PROG quota enable --simple $SCRATCH_MNT
> set_subvol_limit $subvid $limit
> _scratch_cycle_mount
> usage=$(get_subvol_usage $subvid)
> diff --git a/tests/btrfs/310 b/tests/btrfs/310
> new file mode 100755
> index 000000000..02714d261
> --- /dev/null
> +++ b/tests/btrfs/310
> @@ -0,0 +1,83 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 310
> +#
> +# Test various race conditions between qgroup deletion and squota writes
> +#
> +. ./common/preamble
> +_begin_fstest auto quick qgroup subvol clone
> +
> +# Import common functions.
> +. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch_reflink
> +_require_cp_reflink
> +_require_scratch_enable_simple_quota
> +_require_no_compress
> +
> +_fixed_by_kernel_commit xxxxxxxxxxxx "btrfs: forbid deleting live subvol qgroup"
> +_fixed_by_kernel_commit xxxxxxxxxxxx "btrfs: forbid creating subvol qgroups"
> +
> +subv1=$SCRATCH_MNT/subv1
> +subv2=$SCRATCH_MNT/subv2
> +
> +prepare()
> +{
> + _scratch_mkfs >> $seqres.full
> + _scratch_mount
> + $BTRFS_UTIL_PROG quota enable --simple $SCRATCH_MNT
> + $BTRFS_UTIL_PROG subvolume create $subv1 >> $seqres.full
> + $BTRFS_UTIL_PROG subvolume create $subv2 >> $seqres.full
> + $XFS_IO_PROG -fc "pwrite -q 0 128K" $subv1/f
> + _cp_reflink $subv1/f $subv2/f
> +}
> +
> +# An extent can long outlive its owner. Test this by deleting the owning
> +# subvolume, committing the transaction, then deleting the reflinked copy.
> +# Deleting the copy will attempt to free space from the missing owner, which
> +# should be a no-op.
> +free_from_deleted_owner()
> +{
> + echo "free from deleted owner"
> + prepare
> + subvid1=$(_btrfs_get_subvolid $SCRATCH_MNT subv1)
> +
> + $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
> + $BTRFS_UTIL_PROG subvolume delete $subv1 >> $seqres.full
> + $BTRFS_UTIL_PROG qgroup destroy 0/$subvid1 $SCRATCH_MNT >> $seqres.full
> + $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
> + rm $subv2/f
> + _scratch_unmount
> +}
> +
> +# A race where we delete the owner in the same transaction as writing the
> +# extent leads to incrementing the squota usage of the missing qgroup.
> +# This leaves behind an owner ref with an owner id that cannot exist, so
> +# freeing the extent now frees from that qgroup, but there has never
> +# been a corresponding usage to free.
> +add_to_deleted_owner()
> +{
> + echo "add to deleted owner"
> + prepare
> + subvid1=$(_btrfs_get_subvolid $SCRATCH_MNT subv1)
> +
> + $BTRFS_UTIL_PROG subvolume delete $subv1 >> $seqres.full
> + $BTRFS_UTIL_PROG qgroup destroy 0/$subvid1 $SCRATCH_MNT >> $seqres.full
> + $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
> + $BTRFS_UTIL_PROG qgroup create 0/$subvid1 $SCRATCH_MNT >> $seqres.full
> + rm $subv2/f
> + _scratch_unmount
> +}
> +
> +free_from_deleted_owner
> +add_to_deleted_owner
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/310.out b/tests/btrfs/310.out
> new file mode 100644
> index 000000000..d7d4bc0ae
> --- /dev/null
> +++ b/tests/btrfs/310.out
> @@ -0,0 +1,6 @@
> +QA output created by 310
> +free from deleted owner
> +ERROR: unable to destroy quota group: Device or resource busy
> +add to deleted owner
> +ERROR: unable to destroy quota group: Device or resource busy
> +ERROR: unable to create quota group: Invalid argument
> --
> 2.43.0
>
>
