| Message ID | ce4a79cafb6790ef6d1e141d65195f72f469ae4d.1706035378.git.boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs/310: test qgroup deletion | expand |
On Tue, Jan 23, 2024 at 6:44 PM Boris Burkov <boris@bur.io> 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> > --- > Changelog: > v2: > - removed enable quota helper > - removed unneeded commented cleanup boilerplate > - change test number 304 -> 310 (based on v2024.01.14) > > 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) What's the relation of this change with the new test case? This is an independent change that should be in its own separate patch. > 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) Should be made local. > + > + $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) Should be made local as well. Thanks. > + > + $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 > >
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@bur.io> > --- > 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 > >
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
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> --- Changelog: v2: - removed enable quota helper - removed unneeded commented cleanup boilerplate - change test number 304 -> 310 (based on v2024.01.14) 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