Re: [PATCH] btrfs/304: test qgroup deletion

[David Disseldorp <ddiss@suse.de>]

Reviewed-by: David Disseldorp <ddiss@suse.de>

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@bur.io>
> ---
>  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.