On Thu, Sep 28, 2023 at 2:41 AM Boris Burkov <boris@xxxxxx> wrote:
>
> Test some interesting basic and edge cases of simple quotas.
>
> To some extent, this is redundant with the alternate testing strategy of
> using MKFS_OPTIONS to enable simple quotas, running the full suite and
> relying on kernel warnings and fsck to surface issues.
>
> Signed-off-by: Boris Burkov <boris@xxxxxx>
> ---
> tests/btrfs/301 | 418 ++++++++++++++++++++++++++++++++++++++++++++
> tests/btrfs/301.out | 2 +
> 2 files changed, 420 insertions(+)
> create mode 100755 tests/btrfs/301
> create mode 100644 tests/btrfs/301.out
>
> diff --git a/tests/btrfs/301 b/tests/btrfs/301
> new file mode 100755
> index 000000000..527c25230
> --- /dev/null
> +++ b/tests/btrfs/301
> @@ -0,0 +1,418 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 Meta Platforms, Inc. All Rights Reserved.
> +#
> +# FS QA Test 301
> +#
> +# Test common btrfs simple quotas scenarios involving sharing extents and
> +# removing them in various orders.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick qgroup copy_range snapshot
Why the copy_range group? The test doesn't exercise copy_range.
It does exercise reflinking, so it should be in the "clone" group.
Also the following groups are missing: subvol prealloc
> +
> +# Import common functions.
> +# . ./common/filter
Here a:
. ./common/reflink
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs btrfs
> +_require_scratch
Should be: _require_scratch_reflink
> +_require_scratch_enable_simple_quota
Here missing:
_require_cp_reflink
_require_btrfs_command inspect-internal dump-tree
_require_xfs_io_command "falloc"
> +
> +subv=$SCRATCH_MNT/subv
> +nested=$SCRATCH_MNT/subv/nested
> +snap=$SCRATCH_MNT/snap
> +k=1024
> +m=$(($k * $k))
Do we really need to define variables with the size of a KiB and a MiB?
Are there any programmers who don't know what those are? :)
> +nr_fill=1024
> +fill_sz=$((8 * $k))
Just doing ...=$((8 * 1024 ))
Everyone would be aware it's 8 KiB...
> +total_fill=$(($nr_fill * $fill_sz))
> +eb_sz=$((16 * $k))
Same here.
> +ext_sz=$((128 * m))
And here 128 * 1024 * 1024
> +limit_nr=8
> +limit=$(($ext_sz * $limit_nr))
> +
> +get_qgroup_usage()
> +{
> + local qgroupid=$1
> +
> + $BTRFS_UTIL_PROG qgroup show --sync --raw $SCRATCH_MNT | grep "$qgroupid" | $AWK_PROG '{print $3}'
> +}
> +
> +get_subvol_usage()
> +{
> + local subvolid=$1
> + get_qgroup_usage "0/$subvolid"
> +}
> +
> +count_subvol_owned_metadata()
> +{
> + local subvolid=$1
> + # find nodes and leaves owned by the subvol, then get unique offsets
> + # to account for snapshots sharing metadata.
> + count=$($BTRFS_UTIL_PROG inspect-internal dump-tree $SCRATCH_DEV | \
> + grep "owner $subvolid" | awk '{print $2}' | sort | uniq | wc -l)
awk -> $AWK_PROG
> + # output bytes rather than number of metadata blocks
> + echo $(($count * $eb_sz))
So $eb_sz is the extent buffer, leaf/node, size. This is expecting the
default of 16K on x86 (4K pages systems).
Will the test fail if one runs with MKFS_OPTIONS="--nodesize 4K" for
example, or some other different node size?
What about on a 64K pages system, where by default the node size is
64K? Will the test fail?
I would rather make the test flexible to run with any node size, or if
not doable or impractical,
make it require a 4K page size and explicitly pass --nodesize 16K to
mkfs. We have tests where we do this.
> +}
> +
> +check_qgroup_usage()
> +{
> + local qgroupid=$1
> + local expected=$2
> + local actual=$(get_qgroup_usage $qgroupid)
> +
> + [ $expected -eq $actual ] || _fail "qgroup $qgroupid mismatched usage $actual vs $expected"
> +}
> +
> +check_subvol_usage()
> +{
> + local subvolid=$1
> + local expected_data=$2
> + # need to sync to see updated usage numbers.
> + # could probably improve by placing syncs only where they are strictly
> + # needed after actual operations, but it is more error prone.
Can we get proper sentences?
Some are ending with punctuation, others are not. First word should
also be capitalized.
Same applies to everywhere else in the test.
> + sync
I would rather move the sync to the helpers called below, and comment
that it's because they use
the btrfs inspect-internal command, which reads from disk and
therefore everything needs to be
flushed.
> +
> + local expected_meta=$(count_subvol_owned_metadata $subvolid)
> + local actual=$(get_subvol_usage $subvolid)
> + local expected=$(($expected_data + $expected_meta))
> +
> + [ $expected -eq $actual ] || _fail "subvol $subvolid mismatched usage $actual vs $expected (expected data $expected_data expected meta $expected_meta diff $(($actual - $expected)))"
> + echo "OK $subvolid $expected_data $expected_meta $actual" >> $seqres.full
> +}
> +
> +set_subvol_limit()
> +{
> + local subvolid=$1
> + local limit=$2
> +
> + $BTRFS_UTIL_PROG qgroup limit $2 0/$1 $SCRATCH_MNT
> +}
> +
> +sync_check_subvol_usage()
> +{
> + sync
> + check_subvol_usage $@
> +}
Does this helper make any sense? Because check_subvol_usage() is doing
a sync call already.
And it's not called anywhere... so it can be deleted.
> +
> +trigger_cleaner()
> +{
> + $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
> + sleep 1
> + $BTRFS_UTIL_PROG filesystem sync $SCRATCH_MNT
This may be too timing sensitive.
We have other tests that do something like this:
_scratch_remount commit=1
sleep 1.5
At the very least add comments explaining why that guarantees the
cleaner is triggered and
it completes before the function returns, just like we have in
btrfs/276 for example.
> +}
> +
> +cycle_mount_check_subvol_usage()
> +{
> + _scratch_cycle_mount
> + check_subvol_usage $@
> +}
> +
> +
> +do_write()
> +{
> + local file=$1
> + local sz=$2
> +
> + $XFS_IO_PROG -fc "pwrite -q 0 $sz" $file
> + local ret=$?
> + return $ret
Do we need this? Can't the function end just with the $XFS_IO_PROG call?
Wouldn't that be the same?
> +}
> +
> +do_enospc_write()
> +{
> + local file=$1
> + local sz=$2
> +
> + do_write $file $sz 2>/dev/null && _fail "write expected enospc"
So this pattern of calling _fail everywhere, is a bit against what is
generally done in fstests.
Just don't redirect stderr - make it part of the golden output - this
is even better because we can confirm the write failed with -ENOSPC
and not some other error.
> +}
> +
> +do_falloc()
> +{
> + local file=$1
> + local sz=$2
> +
> + $XFS_IO_PROG -fc "falloc 0 $sz" $file
> +}
> +
> +do_enospc_falloc()
> +{
> + local file=$1
> + local sz=$2
> +
> + do_falloc $file $sz 2>/dev/null && _fail "falloc expected enospc"
Same here as before.
> +}
> +
> +enable_quota()
> +{
> + local mode=$1
> +
> + [ $mode == "n" ] && return
> + arg=$([ $mode == "s" ] && echo "--simple")
> +
> + $BTRFS_UTIL_PROG quota enable $arg $SCRATCH_MNT
> +}
> +
> +prepare()
> +{
> + _scratch_mkfs >> $seqres.full
> + _scratch_mount
> + enable_quota "s"
> + $BTRFS_UTIL_PROG subvolume create $subv >> $seqres.full
> + set_subvol_limit 256 $limit
> + check_subvol_usage 256 0
> +
> + # Create a bunch of little filler files to generate several levels in
> + # the btree, to make snapshotting sharing scenarios complex enough.
> + $FIO_PROG --name=filler --directory=$subv --rw=randwrite --nrfiles=$nr_fill --filesize=$fill_sz >/dev/null 2>&1
Using fio, we need a _require_fio somewhere. See other tests like
generic/299 for example.
> + check_subvol_usage 256 $total_fill
> +
> + # Create a single file whose extents we will explicitly share/unshare.
> + do_write $subv/f $ext_sz
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> +}
> +
> +prepare_snapshotted()
> +{
> + prepare
> + $BTRFS_UTIL_PROG subvolume snapshot $subv $snap >> $seqres.full
> + echo "snapshot" >> $seqres.full
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> +}
> +
> +prepare_nested()
> +{
> + prepare
> + $BTRFS_UTIL_PROG qgroup create 1/100 $SCRATCH_MNT
> + $BTRFS_UTIL_PROG qgroup assign 0/256 1/100 $SCRATCH_MNT >> $seqres.full
> + $BTRFS_UTIL_PROG subvolume create $nested >> $seqres.full
> + do_write $nested/f $ext_sz
> + check_subvol_usage 257 $ext_sz
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + local subv_usage=$(get_subvol_usage 256)
> + local nested_usage=$(get_subvol_usage 257)
> + check_qgroup_usage 1/100 $(($subv_usage + $nested_usage))
> +}
> +
> +basic_accounting()
> +{
> + prepare
> + echo "basic" >> $seqres.full
> + echo "delete file" >> $seqres.full
> + rm $subv/f
> + check_subvol_usage 256 $total_fill
> + cycle_mount_check_subvol_usage 256 $total_fill
> + do_write $subv/tmp 512M
> + rm $subv/tmp
> + do_write $subv/tmp 512M
> + rm $subv/tmp
> + do_enospc_falloc $subv/large_falloc 2G
> + do_enospc_write $subv/large 2G
> + _scratch_unmount
> +}
> +
> +reservation_accounting()
> +{
> + prepare
> + for i in $(seq 10); do
> + do_write $subv/tmp 512M
> + rm $subv/tmp
> + done
> + do_enospc_write $subv/large 2G
> + _scratch_unmount
> +}
> +
> +snapshot_accounting()
> +{
> + prepare_snapshotted
> + echo "unshare snapshot metadata" >> $seqres.full
> + touch $snap/f
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + echo "unshare snapshot data" >> $seqres.full
> + do_write $snap/f $ext_sz
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 $ext_sz
> + echo "delete snapshot file" >> $seqres.full
> + rm $snap/f
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + echo "delete original file" >> $seqres.full
> + rm $subv/f
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + cycle_mount_check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + _scratch_unmount
> +}
> +
> +delete_subvol_file()
> +{
> + prepare_snapshotted
> + echo "delete subvol file first" >> $seqres.full
> + rm $subv/f
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + rm $snap/f
> + trigger_cleaner
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + cycle_mount_check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + _scratch_unmount
> +}
> +
> +delete_snapshot_file()
> +{
> + prepare_snapshotted
> + echo "delete snapshot file first" >> $seqres.full
> + rm $snap/f
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + rm $subv/f
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + cycle_mount_check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + _scratch_unmount
> +}
> +
> +delete_subvol()
> +{
> + echo "del sv" > /dev/ksmg
Missing a >> $seqres.full here instead of > /dev/ksmg - which is also
misspelled, should be kmsg.
> + prepare_snapshotted
> + echo "delete subvol first" >> $seqres.full
> + $BTRFS_UTIL_PROG subvolume delete $subv >> $seqres.full
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + rm $snap/f
> + trigger_cleaner
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + $BTRFS_UTIL_PROG subvolume delete $snap >> $seqres.full
> + trigger_cleaner
> + check_subvol_usage 256 0
> + check_subvol_usage 257 0
> + cycle_mount_check_subvol_usage 256 0
> + check_subvol_usage 257 0
> + _scratch_unmount
> +}
> +
> +delete_snapshot()
> +{
> + echo "del snap" > /dev/ksmg
> + prepare_snapshotted
> + echo "delete snapshot first" >> $seqres.full
> + $BTRFS_UTIL_PROG subvolume delete $snap >> $seqres.full
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + check_subvol_usage 257 0
> + $BTRFS_UTIL_PROG subvolume delete $subv >> $seqres.full
> + trigger_cleaner
> + check_subvol_usage 256 0
> + check_subvol_usage 257 0
> + _scratch_unmount
> +}
> +
> +nested_accounting()
> +{
> + echo "nested" > /dev/ksmg
Same here.
> + prepare_nested
> + echo "nested" >> $seqres.full
> + echo "delete file" >> $seqres.full
> + rm $subv/f
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 $ext_sz
> + local subv_usage=$(get_subvol_usage 256)
> + local nested_usage=$(get_subvol_usage 257)
> + check_qgroup_usage 1/100 $(($subv_usage + $nested_usage))
> + rm $nested/f
> + check_subvol_usage 256 $total_fill
> + check_subvol_usage 257 0
> + subv_usage=$(get_subvol_usage 256)
> + nested_usage=$(get_subvol_usage 257)
> + check_qgroup_usage 1/100 $(($subv_usage + $nested_usage))
> + _scratch_unmount
> +}
> +
> +enable_mature()
> +{
> + echo "mature" > /dev/ksmg
Same here.
> + _scratch_mkfs >> $seqres.full
> + _scratch_mount
> + $BTRFS_UTIL_PROG subvolume create $subv >> $seqres.full
> + do_write $subv/f $ext_sz
> + sync
Why is the sync needed? At least add a comment.
> + enable_quota "s"
If enabling quotas requires flushing all delalloc, the kernel should
do it, no need for user space to call sync or anything equivalent.
Would be odd to get incorrect quotas when enabling because there's
pending delalloc.
> + set_subvol_limit 256 $limit
> + _scratch_cycle_mount
> + usage=$(get_subvol_usage 256)
> + [ $usage -lt $ext_sz ] || _fail "captured usage from before enable $usage"
> + do_write $subv/g $ext_sz
> + usage=$(get_subvol_usage 256)
> + [ $usage -lt $ext_sz ] && "failed to capture usage after enable $usage"
> + check_subvol_usage 256 $ext_sz
> + rm $subv/f
> + check_subvol_usage 256 $ext_sz
> + _scratch_cycle_mount
> + rm $subv/g
> + check_subvol_usage 256 0
> + _scratch_unmount
> +}
> +
> +reflink_accounting()
> +{
> + prepare
> + # do more reflinks than would fit
> + for i in $(seq $(($limit_nr * 2))); do
> + cp --reflink=always $subv/f $subv/f.i
Use the helper _cp_reflink instead.
> + done
> + # no additional data usage from reflinks
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + _scratch_unmount
> +}
> +
> +delete_link()
> +{
> + prepare
> + cp --reflink=always $subv/f $subv/f.link
Same here.
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + rm $subv/f.link
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + rm $subv/f
> + check_subvol_usage 256 $(($total_fill))
> + _scratch_unmount
> +}
> +
> +delete_linked()
> +{
> + prepare
> + cp --reflink=always $subv/f $subv/f.link
Same here.
Thanks.
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + rm $subv/f
> + check_subvol_usage 256 $(($total_fill + $ext_sz))
> + rm $subv/f.link
> + check_subvol_usage 256 $(($total_fill))
> + _scratch_unmount
> +}
> +
> +basic_accounting
> +reservation_accounting
> +snapshot_accounting
> +delete_subvol_file
> +delete_snapshot_file
> +delete_subvol
> +delete_snapshot
> +nested_accounting
> +enable_mature
> +reflink_accounting
> +delete_link
> +delete_linked
> +
> +echo "Silence is golden"
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/btrfs/301.out b/tests/btrfs/301.out
> new file mode 100644
> index 000000000..4025504e4
> --- /dev/null
> +++ b/tests/btrfs/301.out
> @@ -0,0 +1,2 @@
> +QA output created by 301
> +Silence is golden
> --
> 2.42.0
>
