Reviewed-by: David Disseldorp <ddiss@xxxxxxx>
A few minor nits below which should be addressed prior to merge...
On Mon, 22 Jan 2024 15:06:28 -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>
> ---
> common/btrfs | 10 +++++
> tests/btrfs/301 | 14 +------
> tests/btrfs/304 | 90 +++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/304.out | 6 +++
btrfs/304 is already taken in fstests v2024.01.14 by
9d812702 ("btrfs: add fstest for stripe-tree metadata with 4k write").
> 4 files changed, 108 insertions(+), 12 deletions(-)
> create mode 100755 tests/btrfs/304
> create mode 100644 tests/btrfs/304.out
>
> diff --git a/common/btrfs b/common/btrfs
> index f91f8dd86..c8593c1f9 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -775,3 +775,13 @@ _has_btrfs_sysfs_feature_attr()
>
> test -e /sys/fs/btrfs/features/$feature_attr
> }
> +
> +_enable_quota()
> +{
> + local mode=$1
> +
> + [ $mode == "n" ] && return
> + arg=$([ $mode == "s" ] && echo "--simple")
> +
> + $BTRFS_UTIL_PROG quota enable $arg $SCRATCH_MNT
It looks as though the "n" mode isn't used, and the "s" -> "--simple"
mapping is confusing. Can we instead just make this:
$BTRFS_UTIL_PROG quota enable $* $SCRATCH_MNT
or drop the helper function altogether?
> +}
> diff --git a/tests/btrfs/301 b/tests/btrfs/301
> index db4697247..b3ee66cd9 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"
> + _enable_quota "s"
...as mentioned, _enable_quota --simple (or inline $BTRFS_UTIL_PROG ...)
> $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"
> + _enable_quota "s"
> set_subvol_limit $subvid $limit
> _scratch_cycle_mount
> usage=$(get_subvol_usage $subvid)
> diff --git a/tests/btrfs/304 b/tests/btrfs/304
> new file mode 100755
> index 000000000..3fce0591c
> --- /dev/null
> +++ b/tests/btrfs/304
> @@ -0,0 +1,90 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 304
> +#
> +# Test various race conditions between qgroup deletion and squota writes
> +#
> +. ./common/preamble
> +_begin_fstest auto quick qgroup subvol clone
> +
> +# Override the default cleanup function.
> +# _cleanup()
> +# {
> +# cd /
> +# rm -r -f $tmp.*
> +# }
nit: doesn't look like there's a need to override the default.
