[PATCH 0/5] Btrfs fstests fixups and updates

[PATCH 0/5] Btrfs fstests fixups and updates

[David Sterba]

Hi,

a few more fixup from our repository.  They have been in our CI branch
and consider them tested and working for our needs. All of them are
btrfs-specific and will not affect other filesystems.

Please apply, thanks.

David Sterba (2):
  common/rc: use proper temporary file path in _repair_test_fs()
  generic/733: disable for btrfs

Josef Bacik (3):
  common/verity: use the correct options for btrfs-corrupt-block
  btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature
    in btrfs
  btrfs/330: add test to validate ro/rw subvol mounting

 common/btrfs        | 10 +++++++++
 common/rc           |  4 ++--
 common/verity       |  2 +-
 tests/btrfs/131     |  2 ++
 tests/btrfs/172     |  3 +++
 tests/btrfs/206     |  3 +++
 tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/330.out |  6 +++++
 tests/generic/733   |  2 +-
 9 files changed, 82 insertions(+), 4 deletions(-)
 create mode 100755 tests/btrfs/330
 create mode 100644 tests/btrfs/330.out

-- 
2.42.1

[PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block

[David Sterba]

From: Josef Bacik <josef@toxicpanda.com>

A long time ago we changed the short options to long options in
btrfs-corrupt-block, so adjust the helper to use the correct options so
the verity tests pass properly.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 common/verity | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/verity b/common/verity
index 03d175ce1b7a18..0e5f0d75e746a8 100644
--- a/common/verity
+++ b/common/verity
@@ -402,7 +402,7 @@ _fsv_scratch_corrupt_merkle_tree()
 			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
 			# $offset (-o $offset) with the ascii representation of the byte we read
 			# (-v $ascii)
-			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
+			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value $ascii --offset $offset -b 1 $SCRATCH_DEV
 			(( offset += 1 ))
 		done
 		_scratch_mount
-- 
2.42.1

[PATCH 2/5] btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature in btrfs

[David Sterba]

From: Josef Bacik <josef@toxicpanda.com>

A new disk format option will make the no-holes option a requirement, so
add a helper to make sure that we aren't creating a fs with
BLOCK_GROUP_TREE by default, and skip the tests that require turning off
no-holes.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 common/btrfs    | 10 ++++++++++
 tests/btrfs/131 |  2 ++
 tests/btrfs/172 |  3 +++
 tests/btrfs/206 |  3 +++
 4 files changed, 18 insertions(+)

diff --git a/common/btrfs b/common/btrfs
index aa344706cd5f81..c1c3c4683a6fe6 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -148,6 +148,16 @@ _require_btrfs_free_space_tree()
 	fi
 }
 
+_require_btrfs_no_block_group_tree()
+{
+	_scratch_mkfs > /dev/null 2>&1
+	if $BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | \
+		grep -q "BLOCK_GROUP_TREE"
+	then
+		_notrun "This test requires no block-group-tree"
+	fi
+}
+
 _check_btrfs_filesystem()
 {
 	device=$1
diff --git a/tests/btrfs/131 b/tests/btrfs/131
index 529ee3e80f87eb..d34fc8d53801b3 100755
--- a/tests/btrfs/131
+++ b/tests/btrfs/131
@@ -20,6 +20,8 @@ _require_btrfs_command inspect-internal dump-super
 _require_btrfs_fs_feature free_space_tree
 # Zoned btrfs does not support space_cache(v1)
 _require_non_zoned_device "${SCRATCH_DEV}"
+# Block group tree does not support space_cache(v1)
+_require_btrfs_no_block_group_tree
 
 _scratch_mkfs >/dev/null 2>&1
 [ "$(_get_page_size)" -gt "$(_scratch_btrfs_sectorsize)" ] && \
diff --git a/tests/btrfs/172 b/tests/btrfs/172
index f2997c047effb0..964251b449de20 100755
--- a/tests/btrfs/172
+++ b/tests/btrfs/172
@@ -32,6 +32,9 @@ _require_scratch
 _require_log_writes
 _require_xfs_io_command "sync_range"
 
+# block-group-tree requires no-holes
+_require_btrfs_no_block_group_tree
+
 _log_writes_init $SCRATCH_DEV
 _log_writes_mkfs "-O ^no-holes" >> $seqres.full 2>&1
 
diff --git a/tests/btrfs/206 b/tests/btrfs/206
index 6ac3d27bbde375..02a5082046f4a5 100755
--- a/tests/btrfs/206
+++ b/tests/btrfs/206
@@ -33,6 +33,9 @@ _require_log_writes
 _require_xfs_io_command "falloc" "-k"
 _require_xfs_io_command "fpunch"
 
+# block-group-tree requires no-holes
+_require_btrfs_no_block_group_tree
+
 _log_writes_init $SCRATCH_DEV
 _log_writes_mkfs "-O ^no-holes" >> $seqres.full 2>&1
 
-- 
2.42.1

[PATCH 3/5] btrfs/330: add test to validate ro/rw subvol mounting

[David Sterba]

From: Josef Bacik <josef@toxicpanda.com>

Btrfs has had the ability for almost a decade to allow ro and rw
mounting of subvols.  This behavior specifically

mount -o subvol=foo,ro /some/dir
mount -o subvol=bar,rw /some/other/dir

This seems simple, but because of the limitations of how we did mounting
in ye olde days we would mark the super block as RO and the mount if we
mounted RO first.  In the case above /some/dir would instantiate the
super block as read only and the mount point.  So the second mount
command under the covers would convert the super block to RW, and then
allow the mount to continue.

The results were still consistent, /some/dir was still read only because
the mount was marked read only, but /some/other/dir could be written to.

This is a test to make sure we maintain this behavior, as I almost
regressed this behavior while converting us to the new mount API.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
 tests/btrfs/330.out |  6 +++++
 2 files changed, 60 insertions(+)
 create mode 100755 tests/btrfs/330
 create mode 100644 tests/btrfs/330.out

diff --git a/tests/btrfs/330 b/tests/btrfs/330
new file mode 100755
index 00000000000000..3ce9840e76d028
--- /dev/null
+++ b/tests/btrfs/330
@@ -0,0 +1,54 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2023 Meta Platforms, Inc.  All Rights Reserved.
+#
+# FS QA Test No. btrfs/330
+#
+# Test mounting one subvolume as ro and another as rw
+#
+. ./common/preamble
+_begin_fstest auto quick subvol
+
+_cleanup()
+{
+	rm -rf $TEST_DIR/$seq
+}
+
+# Import common functions.
+. ./common/filter
+. ./common/filter.btrfs
+
+# real QA test starts here
+_supported_fs btrfs
+_require_scratch
+
+$MOUNT_PROG -V | grep -q 'fd-based-mount'
+[ "$?" -eq 0 ] && _notrun "mount uses the new mount api"
+
+_scratch_mkfs > /dev/null 2>&1
+_scratch_mount
+
+# Create our subvolumes to mount
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/foo | _filter_scratch
+$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/bar | _filter_scratch
+
+_scratch_unmount
+
+mkdir -p $TEST_DIR/$seq/foo
+mkdir -p $TEST_DIR/$seq/bar
+
+_mount -t btrfs -o subvol=foo,ro $SCRATCH_DEV $TEST_DIR/$seq/foo
+_mount -t btrfs -o subvol=bar,rw $SCRATCH_DEV $TEST_DIR/$seq/bar
+
+echo "making sure foo is read only"
+touch $TEST_DIR/$seq/foo/baz > /dev/null 2&>1
+ls $TEST_DIR/$seq/foo
+
+echo "making sure bar allows writes"
+touch $TEST_DIR/$seq/bar/qux
+ls $TEST_DIR/$seq/bar
+
+$UMOUNT_PROG $TEST_DIR/$seq/foo
+$UMOUNT_PROG $TEST_DIR/$seq/bar
+
+status=0 ; exit
diff --git a/tests/btrfs/330.out b/tests/btrfs/330.out
new file mode 100644
index 00000000000000..4795a2ccc8cb62
--- /dev/null
+++ b/tests/btrfs/330.out
@@ -0,0 +1,6 @@
+QA output created by 330
+Create subvolume 'SCRATCH_MNT/foo'
+Create subvolume 'SCRATCH_MNT/bar'
+making sure foo is read only
+making sure bar allows writes
+qux
-- 
2.42.1

[PATCH 4/5] common/rc: use proper temporary file path in _repair_test_fs()

[David Sterba]

The path /tmp.repair would be on the system root that could not be
writable, the temporary files are available at $tmp .

Signed-off-by: David Sterba <dsterba@suse.com>
---
 common/rc | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/common/rc b/common/rc
index 37d26bf21b7c5f..ff2949bd05353d 100644
--- a/common/rc
+++ b/common/rc
@@ -1301,9 +1301,9 @@ _repair_test_fs()
 		;;
 	btrfs)
 	echo 'yes|$BTRFS_UTIL_PROG check --repair --force "$TEST_DEV"' > \
-								/tmp.repair 2>&1
+								$tmp.repair 2>&1
 	yes | $BTRFS_UTIL_PROG check --repair --force "$TEST_DEV" >> \
-								/tmp.repair 2>&1
+								$tmp.repair 2>&1
 		res=$?
 		;;
 	*)
-- 
2.42.1

[PATCH 5/5] generic/733: disable for btrfs

[David Sterba]

This tests if a clone source can be read but in btrfs there's an
exclusive lock and the test always fails. The functionality might be
implemented in btrfs in the future but for now disable the test.

CC: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
---
 tests/generic/733 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/733 b/tests/generic/733
index d88d92a4705add..adb141e190b7b0 100755
--- a/tests/generic/733
+++ b/tests/generic/733
@@ -18,7 +18,7 @@ _begin_fstest auto clone punch
 . ./common/reflink
 
 # real QA test starts here
-_supported_fs generic
+_supported_fs ^btrfs
 _require_scratch_reflink
 _require_cp_reflink
 _require_xfs_io_command "fpunch"
-- 
2.42.1

Re: [PATCH 5/5] generic/733: disable for btrfs

[Christoph Hellwig]

On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote:
> This tests if a clone source can be read but in btrfs there's an
> exclusive lock and the test always fails. The functionality might be
> implemented in btrfs in the future but for now disable the test.

Well, this sounds like btrf sis broken and it should fail?

>  # real QA test starts here
> -_supported_fs generic
> +_supported_fs ^btrfs


and just throwing random not supported in generic testes is just a mine
field.  You;d better explain very well why something is not supported,
and do that by adding a _require_.  If you can't explain the
_require this probably actually is a bug and not just an exclude..

Re: [PATCH 5/5] generic/733: disable for btrfs

[Darrick J. Wong]

On Tue, Mar 19, 2024 at 02:01:00PM -0700, Christoph Hellwig wrote:
> On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote:
> > This tests if a clone source can be read but in btrfs there's an
> > exclusive lock and the test always fails. The functionality might be
> > implemented in btrfs in the future but for now disable the test.
> 
> Well, this sounds like btrf sis broken and it should fail?
> 
> >  # real QA test starts here
> > -_supported_fs generic
> > +_supported_fs ^btrfs
> 
> 
> and just throwing random not supported in generic testes is just a mine
> field.  You;d better explain very well why something is not supported,
> and do that by adding a _require_.  If you can't explain the
> _require this probably actually is a bug and not just an exclude..

I don't think there's a good way to _require-test for holding i_rwsem in
exclusive or shared mode during an FICLONE operation.  But that ought to
be a comment somewhere at least.

--D

Re: [PATCH 5/5] generic/733: disable for btrfs

[Christoph Hellwig]

On Tue, Mar 19, 2024 at 02:10:50PM -0700, Darrick J. Wong wrote:
> I don't think there's a good way to _require-test for holding i_rwsem in
> exclusive or shared mode during an FICLONE operation.  But that ought to
> be a comment somewhere at least.

Throwing random per-fs hacks into generic tests simply does not
scale.  We'll need a way to describe and opt into these behaviors
in common helpers.

Re: [PATCH 0/5] Btrfs fstests fixups and updates

[Anand Jain]

On 3/19/24 23:41, David Sterba wrote:
> Hi,
> 
> a few more fixup from our repository.  They have been in our CI branch
> and consider them tested and working for our needs. All of them are
> btrfs-specific and will not affect other filesystems.
> 
> Please apply, thanks.
> 
> David Sterba (2):
>    common/rc: use proper temporary file path in _repair_test_fs()
>    generic/733: disable for btrfs
> 
> Josef Bacik (3):
>    common/verity: use the correct options for btrfs-corrupt-block
>    btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature
>      in btrfs
>    btrfs/330: add test to validate ro/rw subvol mounting
> 

Which repo base is this from? Fails to apply without conflicts.

Thanks, Anand


>   common/btrfs        | 10 +++++++++
>   common/rc           |  4 ++--
>   common/verity       |  2 +-
>   tests/btrfs/131     |  2 ++
>   tests/btrfs/172     |  3 +++
>   tests/btrfs/206     |  3 +++
>   tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/btrfs/330.out |  6 +++++
>   tests/generic/733   |  2 +-
>   9 files changed, 82 insertions(+), 4 deletions(-)
>   create mode 100755 tests/btrfs/330
>   create mode 100644 tests/btrfs/330.out
> 

Re: [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block

[Anand Jain]

On 3/19/24 23:42, David Sterba wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> A long time ago we changed the short options to long options in
> btrfs-corrupt-block, so adjust the helper to use the correct options so
> the verity tests pass properly.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>   common/verity | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/verity b/common/verity
> index 03d175ce1b7a18..0e5f0d75e746a8 100644
> --- a/common/verity
> +++ b/common/verity
> @@ -402,7 +402,7 @@ _fsv_scratch_corrupt_merkle_tree()
>   			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
>   			# $offset (-o $offset) with the ascii representation of the byte we read
>   			# (-v $ascii)

> -			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
> +			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value $ascii --offset $offset -b 1 $SCRATCH_DEV


Can we still make it work with the older btrfs-corrupt-block?

How about..

corrupt_block_value_opt()
{
	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q -e "--value"
	if [ $? == 0 ]; then
		echo "--value"
	else
		echo "-v"
	fi
}

And to use,

$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 $(corrupt_block_value_opt) 
$ascii --offset $offset -b 1 $SCRATCH_DEV


I will make this change before submitting the PR if no objection.

Thanks, Anand

>   			(( offset += 1 ))
>   		done
>   		_scratch_mount

Re: [PATCH 2/5] btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature in btrfs

[Anand Jain]

On 3/19/24 23:42, David Sterba wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> A new disk format option will make the no-holes option a requirement, so
> add a helper to make sure that we aren't creating a fs with
> BLOCK_GROUP_TREE by default, and skip the tests that require turning off
> no-holes.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks.


> ---
>   common/btrfs    | 10 ++++++++++
>   tests/btrfs/131 |  2 ++
>   tests/btrfs/172 |  3 +++
>   tests/btrfs/206 |  3 +++
>   4 files changed, 18 insertions(+)
> 
> diff --git a/common/btrfs b/common/btrfs
> index aa344706cd5f81..c1c3c4683a6fe6 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -148,6 +148,16 @@ _require_btrfs_free_space_tree()
>   	fi
>   }
>   
> +_require_btrfs_no_block_group_tree()
> +{
> +	_scratch_mkfs > /dev/null 2>&1
> +	if $BTRFS_UTIL_PROG inspect-internal dump-super $SCRATCH_DEV | \
> +		grep -q "BLOCK_GROUP_TREE"
> +	then
> +		_notrun "This test requires no block-group-tree"
> +	fi
> +}
> +
>   _check_btrfs_filesystem()
>   {
>   	device=$1
> diff --git a/tests/btrfs/131 b/tests/btrfs/131
> index 529ee3e80f87eb..d34fc8d53801b3 100755
> --- a/tests/btrfs/131
> +++ b/tests/btrfs/131
> @@ -20,6 +20,8 @@ _require_btrfs_command inspect-internal dump-super
>   _require_btrfs_fs_feature free_space_tree
>   # Zoned btrfs does not support space_cache(v1)
>   _require_non_zoned_device "${SCRATCH_DEV}"
> +# Block group tree does not support space_cache(v1)
> +_require_btrfs_no_block_group_tree
>   
>   _scratch_mkfs >/dev/null 2>&1
>   [ "$(_get_page_size)" -gt "$(_scratch_btrfs_sectorsize)" ] && \
> diff --git a/tests/btrfs/172 b/tests/btrfs/172
> index f2997c047effb0..964251b449de20 100755
> --- a/tests/btrfs/172
> +++ b/tests/btrfs/172
> @@ -32,6 +32,9 @@ _require_scratch
>   _require_log_writes
>   _require_xfs_io_command "sync_range"
>   
> +# block-group-tree requires no-holes
> +_require_btrfs_no_block_group_tree
> +
>   _log_writes_init $SCRATCH_DEV
>   _log_writes_mkfs "-O ^no-holes" >> $seqres.full 2>&1
>   
> diff --git a/tests/btrfs/206 b/tests/btrfs/206
> index 6ac3d27bbde375..02a5082046f4a5 100755
> --- a/tests/btrfs/206
> +++ b/tests/btrfs/206
> @@ -33,6 +33,9 @@ _require_log_writes
>   _require_xfs_io_command "falloc" "-k"
>   _require_xfs_io_command "fpunch"
>   
> +# block-group-tree requires no-holes
> +_require_btrfs_no_block_group_tree
> +
>   _log_writes_init $SCRATCH_DEV
>   _log_writes_mkfs "-O ^no-holes" >> $seqres.full 2>&1
>   

Re: [PATCH 3/5] btrfs/330: add test to validate ro/rw subvol mounting

[Anand Jain]

On 3/19/24 23:42, David Sterba wrote:
> From: Josef Bacik <josef@toxicpanda.com>
> 
> Btrfs has had the ability for almost a decade to allow ro and rw
> mounting of subvols.  This behavior specifically
> 
> mount -o subvol=foo,ro /some/dir
> mount -o subvol=bar,rw /some/other/dir
> 
> This seems simple, but because of the limitations of how we did mounting
> in ye olde days we would mark the super block as RO and the mount if we
> mounted RO first.  In the case above /some/dir would instantiate the
> super block as read only and the mount point.  So the second mount
> command under the covers would convert the super block to RW, and then
> allow the mount to continue.
> 
> The results were still consistent, /some/dir was still read only because
> the mount was marked read only, but /some/other/dir could be written to.
> 
> This is a test to make sure we maintain this behavior, as I almost
> regressed this behavior while converting us to the new mount API.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Nits below.

> ---
>   tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/btrfs/330.out |  6 +++++
>   2 files changed, 60 insertions(+)
>   create mode 100755 tests/btrfs/330
>   create mode 100644 tests/btrfs/330.out
> 
> diff --git a/tests/btrfs/330 b/tests/btrfs/330
> new file mode 100755
> index 00000000000000..3ce9840e76d028
> --- /dev/null
> +++ b/tests/btrfs/330
> @@ -0,0 +1,54 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2023 Meta Platforms, Inc.  All Rights Reserved.
> +#
> +# FS QA Test No. btrfs/330
> +#
> +# Test mounting one subvolume as ro and another as rw
> +#
> +. ./common/preamble
> +_begin_fstest auto quick subvol
> +
> +_cleanup()
> +{
> +	rm -rf $TEST_DIR/$seq
> +}
> +
> +# Import common functions.

> +. ./common/filter

This can be deleted, as the filter.btrfs also calls the filter.

> +. ./common/filter.btrfs


> +
> +# real QA test starts here
> +_supported_fs btrfs
> +_require_scratch
> +
> +$MOUNT_PROG -V | grep -q 'fd-based-mount'
> +[ "$?" -eq 0 ] && _notrun "mount uses the new mount api"
> +
> +_scratch_mkfs > /dev/null 2>&1

_scratch_mkfs >> $seqres.full

Errors, if any, go to stdout.

Thanks, Anand

> +_scratch_mount
> +
> +# Create our subvolumes to mount
> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/foo | _filter_scratch
> +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/bar | _filter_scratch
> +
> +_scratch_unmount
> +
> +mkdir -p $TEST_DIR/$seq/foo
> +mkdir -p $TEST_DIR/$seq/bar
> +
> +_mount -t btrfs -o subvol=foo,ro $SCRATCH_DEV $TEST_DIR/$seq/foo
> +_mount -t btrfs -o subvol=bar,rw $SCRATCH_DEV $TEST_DIR/$seq/bar
> +
> +echo "making sure foo is read only"
> +touch $TEST_DIR/$seq/foo/baz > /dev/null 2&>1
> +ls $TEST_DIR/$seq/foo
> +
> +echo "making sure bar allows writes"
> +touch $TEST_DIR/$seq/bar/qux
> +ls $TEST_DIR/$seq/bar
> +
> +$UMOUNT_PROG $TEST_DIR/$seq/foo
> +$UMOUNT_PROG $TEST_DIR/$seq/bar
> +
> +status=0 ; exit
> diff --git a/tests/btrfs/330.out b/tests/btrfs/330.out
> new file mode 100644
> index 00000000000000..4795a2ccc8cb62
> --- /dev/null
> +++ b/tests/btrfs/330.out
> @@ -0,0 +1,6 @@
> +QA output created by 330
> +Create subvolume 'SCRATCH_MNT/foo'
> +Create subvolume 'SCRATCH_MNT/bar'
> +making sure foo is read only
> +making sure bar allows writes
> +qux

Re: [PATCH 4/5] common/rc: use proper temporary file path in _repair_test_fs()

[Anand Jain]

On 3/19/24 23:42, David Sterba wrote:
> The path /tmp.repair would be on the system root that could not be
> writable, the temporary files are available at $tmp .
> 
> Signed-off-by: David Sterba <dsterba@suse.com>

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks,
Anand

> ---
>   common/rc | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 37d26bf21b7c5f..ff2949bd05353d 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1301,9 +1301,9 @@ _repair_test_fs()
>   		;;
>   	btrfs)
>   	echo 'yes|$BTRFS_UTIL_PROG check --repair --force "$TEST_DEV"' > \
> -								/tmp.repair 2>&1
> +								$tmp.repair 2>&1
>   	yes | $BTRFS_UTIL_PROG check --repair --force "$TEST_DEV" >> \
> -								/tmp.repair 2>&1
> +								$tmp.repair 2>&1
>   		res=$?
>   		;;
>   	*)

Re: [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block

[David Sterba]

On Wed, Mar 20, 2024 at 03:28:52PM +0530, Anand Jain wrote:
> On 3/19/24 23:42, David Sterba wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> > 
> > A long time ago we changed the short options to long options in
> > btrfs-corrupt-block, so adjust the helper to use the correct options so
> > the verity tests pass properly.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >   common/verity | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/common/verity b/common/verity
> > index 03d175ce1b7a18..0e5f0d75e746a8 100644
> > --- a/common/verity
> > +++ b/common/verity
> > @@ -402,7 +402,7 @@ _fsv_scratch_corrupt_merkle_tree()
> >   			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
> >   			# $offset (-o $offset) with the ascii representation of the byte we read
> >   			# (-v $ascii)
> 
> > -			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
> > +			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value $ascii --offset $offset -b 1 $SCRATCH_DEV
> 
> 
> Can we still make it work with the older btrfs-corrupt-block?
> 
> How about..
> 
> corrupt_block_value_opt()
> {
> 	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q -e "--value"
> 	if [ $? == 0 ]; then
> 		echo "--value"
> 	else
> 		echo "-v"
> 	fi
> }
> 
> And to use,
> 
> $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 $(corrupt_block_value_opt) 
> $ascii --offset $offset -b 1 $SCRATCH_DEV
> 
> 
> I will make this change before submitting the PR if no objection.

Thanks, that would be great. The option changed in btrfs-progs 5.18
which is still relatively recent so both options should be supported.

Re: [PATCH 0/5] Btrfs fstests fixups and updates

[David Sterba]

On Wed, Mar 20, 2024 at 03:19:12PM +0530, Anand Jain wrote:
> On 3/19/24 23:41, David Sterba wrote:
> > Hi,
> > 
> > a few more fixup from our repository.  They have been in our CI branch
> > and consider them tested and working for our needs. All of them are
> > btrfs-specific and will not affect other filesystems.
> > 
> > Please apply, thanks.
> > 
> > David Sterba (2):
> >    common/rc: use proper temporary file path in _repair_test_fs()
> >    generic/733: disable for btrfs
> > 
> > Josef Bacik (3):
> >    common/verity: use the correct options for btrfs-corrupt-block
> >    btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature
> >      in btrfs
> >    btrfs/330: add test to validate ro/rw subvol mounting
> > 
> 
> Which repo base is this from? Fails to apply without conflicts.

I used for-next (ec5b77f70e602b2312) as a base as it was most up to
date, unlike patches-in-queue.

Re: [PATCH 5/5] generic/733: disable for btrfs

[David Sterba]

On Tue, Mar 19, 2024 at 02:01:00PM -0700, Christoph Hellwig wrote:
> On Tue, Mar 19, 2024 at 07:12:10PM +0100, David Sterba wrote:
> > This tests if a clone source can be read but in btrfs there's an
> > exclusive lock and the test always fails. The functionality might be
> > implemented in btrfs in the future but for now disable the test.
> 
> Well, this sounds like btrf sis broken and it should fail?

It does not sound like that to me, rather that the test was tailored
for xfs and assumes too much about the behaviour that does not apply to
btrfs. If you look at src/t_reflink_read_race.c it's full of magic
constants used for timing, this itself points to an unreliable test
depending on hardware and other system load.

> 
> >  # real QA test starts here
> > -_supported_fs generic
> > +_supported_fs ^btrfs
> 
> 
> and just throwing random not supported in generic testes is just a mine
> field.  You;d better explain very well why something is not supported,
> and do that by adding a _require_.  If you can't explain the
> _require this probably actually is a bug and not just an exclude..

It is a bug in the test, it should have been xfs specific and never
promoted to generic/ and not affect btrfs unless explained how
in the first place.

Re: [PATCH 3/5] btrfs/330: add test to validate ro/rw subvol mounting

[Filipe Manana]

On Wed, Mar 20, 2024 at 11:34 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> On 3/19/24 23:42, David Sterba wrote:
> > From: Josef Bacik <josef@toxicpanda.com>
> >
> > Btrfs has had the ability for almost a decade to allow ro and rw
> > mounting of subvols.  This behavior specifically
> >
> > mount -o subvol=foo,ro /some/dir
> > mount -o subvol=bar,rw /some/other/dir
> >
> > This seems simple, but because of the limitations of how we did mounting
> > in ye olde days we would mark the super block as RO and the mount if we
> > mounted RO first.  In the case above /some/dir would instantiate the
> > super block as read only and the mount point.  So the second mount
> > command under the covers would convert the super block to RW, and then
> > allow the mount to continue.
> >
> > The results were still consistent, /some/dir was still read only because
> > the mount was marked read only, but /some/other/dir could be written to.
> >
> > This is a test to make sure we maintain this behavior, as I almost
> > regressed this behavior while converting us to the new mount API.
> >
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>
> looks good.
>
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>
> Nits below.
>
> > ---
> >   tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
> >   tests/btrfs/330.out |  6 +++++
> >   2 files changed, 60 insertions(+)
> >   create mode 100755 tests/btrfs/330
> >   create mode 100644 tests/btrfs/330.out
> >
> > diff --git a/tests/btrfs/330 b/tests/btrfs/330
> > new file mode 100755
> > index 00000000000000..3ce9840e76d028
> > --- /dev/null
> > +++ b/tests/btrfs/330
> > @@ -0,0 +1,54 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2023 Meta Platforms, Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test No. btrfs/330
> > +#
> > +# Test mounting one subvolume as ro and another as rw
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick subvol
> > +
> > +_cleanup()
> > +{
> > +     rm -rf $TEST_DIR/$seq
> > +}
> > +
> > +# Import common functions.
>
> > +. ./common/filter
>
> This can be deleted, as the filter.btrfs also calls the filter.
>
> > +. ./common/filter.btrfs
>
>
> > +
> > +# real QA test starts here
> > +_supported_fs btrfs
> > +_require_scratch
> > +
> > +$MOUNT_PROG -V | grep -q 'fd-based-mount'
> > +[ "$?" -eq 0 ] && _notrun "mount uses the new mount api"
> > +
> > +_scratch_mkfs > /dev/null 2>&1
>
> _scratch_mkfs >> $seqres.full
>
> Errors, if any, go to stdout.

We typically redirect stderr to stdout because in the past mkfs.btrfs
used to output to stderr a message when it performed trim.
See this old commit:

https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=afadb6e5958c5acf2425d6a8a9372b63afcb4f2a

And nowadays we're encouraged to do:

_scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"

So in case mkfs fails the test doesn't continue and silently passes by
using the filesystem SCRATCH_MNT belongs to.
This has been discussed and pointed out several times, but I don't
have a link to such discussion.

>
> Thanks, Anand
>
> > +_scratch_mount
> > +
> > +# Create our subvolumes to mount
> > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/foo | _filter_scratch
> > +$BTRFS_UTIL_PROG subvolume create $SCRATCH_MNT/bar | _filter_scratch
> > +
> > +_scratch_unmount
> > +
> > +mkdir -p $TEST_DIR/$seq/foo
> > +mkdir -p $TEST_DIR/$seq/bar
> > +
> > +_mount -t btrfs -o subvol=foo,ro $SCRATCH_DEV $TEST_DIR/$seq/foo
> > +_mount -t btrfs -o subvol=bar,rw $SCRATCH_DEV $TEST_DIR/$seq/bar
> > +
> > +echo "making sure foo is read only"
> > +touch $TEST_DIR/$seq/foo/baz > /dev/null 2&>1
> > +ls $TEST_DIR/$seq/foo
> > +
> > +echo "making sure bar allows writes"
> > +touch $TEST_DIR/$seq/bar/qux
> > +ls $TEST_DIR/$seq/bar
> > +
> > +$UMOUNT_PROG $TEST_DIR/$seq/foo
> > +$UMOUNT_PROG $TEST_DIR/$seq/bar
> > +
> > +status=0 ; exit
> > diff --git a/tests/btrfs/330.out b/tests/btrfs/330.out
> > new file mode 100644
> > index 00000000000000..4795a2ccc8cb62
> > --- /dev/null
> > +++ b/tests/btrfs/330.out
> > @@ -0,0 +1,6 @@
> > +QA output created by 330
> > +Create subvolume 'SCRATCH_MNT/foo'
> > +Create subvolume 'SCRATCH_MNT/bar'
> > +making sure foo is read only
> > +making sure bar allows writes
> > +qux
>
>

Re: [PATCH 3/5] btrfs/330: add test to validate ro/rw subvol mounting

[Anand Jain]

On 3/20/24 22:31, Filipe Manana wrote:
> On Wed, Mar 20, 2024 at 11:34 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> On 3/19/24 23:42, David Sterba wrote:
>>> From: Josef Bacik <josef@toxicpanda.com>
>>>
>>> Btrfs has had the ability for almost a decade to allow ro and rw
>>> mounting of subvols.  This behavior specifically
>>>
>>> mount -o subvol=foo,ro /some/dir
>>> mount -o subvol=bar,rw /some/other/dir
>>>
>>> This seems simple, but because of the limitations of how we did mounting
>>> in ye olde days we would mark the super block as RO and the mount if we
>>> mounted RO first.  In the case above /some/dir would instantiate the
>>> super block as read only and the mount point.  So the second mount
>>> command under the covers would convert the super block to RW, and then
>>> allow the mount to continue.
>>>
>>> The results were still consistent, /some/dir was still read only because
>>> the mount was marked read only, but /some/other/dir could be written to.
>>>
>>> This is a test to make sure we maintain this behavior, as I almost
>>> regressed this behavior while converting us to the new mount API.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>
>> looks good.
>>
>> Reviewed-by: Anand Jain <anand.jain@oracle.com>
>>
>> Nits below.
>>
>>> ---
>>>    tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
>>>    tests/btrfs/330.out |  6 +++++
>>>    2 files changed, 60 insertions(+)
>>>    create mode 100755 tests/btrfs/330
>>>    create mode 100644 tests/btrfs/330.out
>>>
>>> diff --git a/tests/btrfs/330 b/tests/btrfs/330
>>> new file mode 100755
>>> index 00000000000000..3ce9840e76d028
>>> --- /dev/null
>>> +++ b/tests/btrfs/330
>>> @@ -0,0 +1,54 @@
>>> +#! /bin/bash
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Copyright (c) 2023 Meta Platforms, Inc.  All Rights Reserved.
>>> +#
>>> +# FS QA Test No. btrfs/330
>>> +#
>>> +# Test mounting one subvolume as ro and another as rw
>>> +#
>>> +. ./common/preamble
>>> +_begin_fstest auto quick subvol
>>> +
>>> +_cleanup()
>>> +{
>>> +     rm -rf $TEST_DIR/$seq
>>> +}
>>> +
>>> +# Import common functions.
>>
>>> +. ./common/filter
>>
>> This can be deleted, as the filter.btrfs also calls the filter.
>>
>>> +. ./common/filter.btrfs
>>
>>
>>> +
>>> +# real QA test starts here
>>> +_supported_fs btrfs
>>> +_require_scratch
>>> +
>>> +$MOUNT_PROG -V | grep -q 'fd-based-mount'
>>> +[ "$?" -eq 0 ] && _notrun "mount uses the new mount api"
>>> +
>>> +_scratch_mkfs > /dev/null 2>&1
>>
>> _scratch_mkfs >> $seqres.full
>>
>> Errors, if any, go to stdout.
> 
> We typically redirect stderr to stdout because in the past mkfs.btrfs
> used to output to stderr a message when it performed trim.
> See this old commit:
> 
> https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/commit/?id=afadb6e5958c5acf2425d6a8a9372b63afcb4f2a
> 
> And nowadays we're encouraged to do:
> 
> _scratch_mkfs >> $seqres.full 2>&1 || _fail "mkfs failed"
> 
> So in case mkfs fails the test doesn't continue and silently passes by
> using the filesystem SCRATCH_MNT belongs to.

Darn it, I keep forgetting about the trimmed message history that went 
to stderr. My bad. And I did search mkfs for what went to stderr, but it 
wasn't an error. Btrfs/303 fixed.

Thanks, Anand

[PATCH] common/btrfs: set BTRFS_CORRUPT_BLOCK_OPT_<VALUE|OFFSET>

[Anand Jain]

As btrfs-corrupt-block now uses --value instead of -v, and --offset
instead of -o, provide backward compatibility for the testcases, by
storing the option to be used in BTRFS_CORRUPT_BLOCK_OPT_VALUE and
BTRFS_CORRUPT_BLOCK_OPT_OFFSET. Also, removes the stdout and stderr
redirection to /dev/null.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
This replaces the patch:
   [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block

 common/btrfs    | 16 ++++++++++++++++
 common/verity   |  9 ++++++---
 tests/btrfs/290 | 30 ++++++++++++++++++++++--------
 3 files changed, 44 insertions(+), 11 deletions(-)

diff --git a/common/btrfs b/common/btrfs
index ae13fb55cbc6..11d74bea9111 100644
--- a/common/btrfs
+++ b/common/btrfs
@@ -660,6 +660,22 @@ _btrfs_buffered_read_on_mirror()
 _require_btrfs_corrupt_block()
 {
 	_require_command "$BTRFS_CORRUPT_BLOCK_PROG" btrfs-corrupt-block
+
+	# In the newer version, the option -v is replaced by --value,
+	# and -o is replaced by --offset, so normalize them.
+	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "value VALUE"
+	if [ $? == 0 ]; then
+		export BTRFS_CORRUPT_BLOCK_OPT_VALUE="--value"
+	else
+		export BTRFS_CORRUPT_BLOCK_OPT_VALUE="-v"
+	fi
+
+	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "offset OFFSET"
+	if [ $? == 0 ]; then
+		export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="--offset"
+	else
+		export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="-o"
+	fi
 }
 
 _require_btrfs_send_version()
diff --git a/common/verity b/common/verity
index 03d175ce1b7a..33a1c12f558e 100644
--- a/common/verity
+++ b/common/verity
@@ -400,9 +400,12 @@ _fsv_scratch_corrupt_merkle_tree()
 			local ascii=$(printf "%d" "'$byte'")
 			# This command will find a Merkle tree item for the inode (-I $ino,37,0)
 			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
-			# $offset (-o $offset) with the ascii representation of the byte we read
-			# (-v $ascii)
-			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
+			# $offset (-o|--offset $offset) with the ascii
+			# representation of the byte we read (-v|--value $ascii)
+			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
+				$BTRFS_CORRUPT_BLOCK_OPT_VALUE $ascii \
+				$BTRFS_CORRUPT_BLOCK_OPT_OFFSET $offset \
+							-b 1 $SCRATCH_DEV
 			(( offset += 1 ))
 		done
 		_scratch_mount
diff --git a/tests/btrfs/290 b/tests/btrfs/290
index 61e741faeb45..d6f777776838 100755
--- a/tests/btrfs/290
+++ b/tests/btrfs/290
@@ -58,7 +58,7 @@ corrupt_inline() {
 	_scratch_unmount
 	# inline data starts at disk_bytenr
 	# overwrite the first u64 with random bogus junk
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -72,7 +72,8 @@ corrupt_prealloc_to_reg() {
 	_scratch_unmount
 	# ensure non-zero at the pre-allocated region on disk
 	# set extent type from prealloc (2) to reg (1)
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 1 $SCRATCH_DEV >/dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
+				$BTRFS_CORRUPT_BLOCK_OPT_VALUE 1 $SCRATCH_DEV
 	_scratch_mount
 	# now that it's a regular file, reading actually looks at the previously
 	# preallocated region, so ensure that has non-zero contents.
@@ -88,7 +89,8 @@ corrupt_reg_to_prealloc() {
 	_fsv_enable $f
 	_scratch_unmount
 	# set type from reg (1) to prealloc (2)
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 2 $SCRATCH_DEV >/dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
+				$BTRFS_CORRUPT_BLOCK_OPT_VALUE 2 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -104,7 +106,8 @@ corrupt_punch_hole() {
 	_fsv_enable $f
 	_scratch_unmount
 	# change disk_bytenr to 0, representing a hole
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 0 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
+				$BTRFS_CORRUPT_BLOCK_OPT_VALUE 0 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -118,7 +121,8 @@ corrupt_plug_hole() {
 	_fsv_enable $f
 	_scratch_unmount
 	# change disk_bytenr to some value, plugging the hole
-	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 13639680 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
+			$BTRFS_CORRUPT_BLOCK_OPT_VALUE 13639680 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -132,7 +136,11 @@ corrupt_verity_descriptor() {
 	_scratch_unmount
 	# key for the descriptor item is <inode, BTRFS_VERITY_DESC_ITEM_KEY, 1>,
 	# 88 is X. So we write 5 Xs to the start of the descriptor
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 0 -b 5 $SCRATCH_DEV > /dev/null 2>&1
+	btrfs in dump-tree -t 5 $SCRATCH_DEV > $tmp.desc_dump_tree
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
+					$BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
+					$BTRFS_CORRUPT_BLOCK_OPT_OFFSET 0 \
+					-b 5 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -144,7 +152,10 @@ corrupt_root_hash() {
 	local ino=$(get_ino $f)
 	_fsv_enable $f
 	_scratch_unmount
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 16 -b 1 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
+					$BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
+					$BTRFS_CORRUPT_BLOCK_OPT_OFFSET 16 \
+					-b 1 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
@@ -159,7 +170,10 @@ corrupt_merkle_tree() {
 	# key for the descriptor item is <inode, BTRFS_VERITY_MERKLE_ITEM_KEY, 0>,
 	# 88 is X. So we write 5 Xs to somewhere in the middle of the first
 	# merkle item
-	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v 88 -o 100 -b 5 $SCRATCH_DEV > /dev/null 2>&1
+	$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
+					$BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
+					$BTRFS_CORRUPT_BLOCK_OPT_OFFSET 100 \
+					-b 5 $SCRATCH_DEV
 	_scratch_mount
 	validate $f
 }
-- 
2.39.3

Re: [PATCH] common/btrfs: set BTRFS_CORRUPT_BLOCK_OPT_<VALUE|OFFSET>

[Filipe Manana]

On Thu, Mar 21, 2024 at 4:10 AM Anand Jain <anand.jain@oracle.com> wrote:
>
> As btrfs-corrupt-block now uses --value instead of -v, and --offset
> instead of -o, provide backward compatibility for the testcases, by
> storing the option to be used in BTRFS_CORRUPT_BLOCK_OPT_VALUE and
> BTRFS_CORRUPT_BLOCK_OPT_OFFSET. Also, removes the stdout and stderr
> redirection to /dev/null.

This is complex and ugly, but most importantly this is not needed at all.

Just let all users of btrfs-corrupt-block use --value and --offset,
because there was never
a released version of btrfs-progs with the short options available.

The short options were introduced with:

commit b2ada0594116f3f4458581317e226c5976443ad0
Author: Boris Burkov <boris@bur.io>
Date:   Tue Jul 26 13:43:23 2022 -0700

    btrfs-progs: corrupt-block: corrupt generic item data

And then replacing them with long options happened in this commit:

commit 22ffee3c6cf2e6f285e6fd6cb22b88c02510e10e
Author: David Sterba <dsterba@suse.com>
Date:   Wed Jul 27 20:47:57 2022 +0200

    btrfs-progs: corrupt-block: use only long options for value and offset

Both commits landed in btrfs-progs 5.19, meaning there are no released
versions with the short options.

The reason btrfs/290 is using the short options is because the
btrfs-progs patch had just been submitted shortly before the test case
was added.

However what we need is to have a _require_* helper that will make the
test btrfs/290 not run if we're using a btrfs-progs version without
those new options.

Thanks.


>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> This replaces the patch:
>    [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block
>
>  common/btrfs    | 16 ++++++++++++++++
>  common/verity   |  9 ++++++---
>  tests/btrfs/290 | 30 ++++++++++++++++++++++--------
>  3 files changed, 44 insertions(+), 11 deletions(-)
>
> diff --git a/common/btrfs b/common/btrfs
> index ae13fb55cbc6..11d74bea9111 100644
> --- a/common/btrfs
> +++ b/common/btrfs
> @@ -660,6 +660,22 @@ _btrfs_buffered_read_on_mirror()
>  _require_btrfs_corrupt_block()
>  {
>         _require_command "$BTRFS_CORRUPT_BLOCK_PROG" btrfs-corrupt-block
> +
> +       # In the newer version, the option -v is replaced by --value,
> +       # and -o is replaced by --offset, so normalize them.
> +       $BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "value VALUE"
> +       if [ $? == 0 ]; then
> +               export BTRFS_CORRUPT_BLOCK_OPT_VALUE="--value"
> +       else
> +               export BTRFS_CORRUPT_BLOCK_OPT_VALUE="-v"
> +       fi
> +
> +       $BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "offset OFFSET"
> +       if [ $? == 0 ]; then
> +               export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="--offset"
> +       else
> +               export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="-o"
> +       fi
>  }
>
>  _require_btrfs_send_version()
> diff --git a/common/verity b/common/verity
> index 03d175ce1b7a..33a1c12f558e 100644
> --- a/common/verity
> +++ b/common/verity
> @@ -400,9 +400,12 @@ _fsv_scratch_corrupt_merkle_tree()
>                         local ascii=$(printf "%d" "'$byte'")
>                         # This command will find a Merkle tree item for the inode (-I $ino,37,0)
>                         # in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
> -                       # $offset (-o $offset) with the ascii representation of the byte we read
> -                       # (-v $ascii)
> -                       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
> +                       # $offset (-o|--offset $offset) with the ascii
> +                       # representation of the byte we read (-v|--value $ascii)
> +                       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE $ascii \
> +                               $BTRFS_CORRUPT_BLOCK_OPT_OFFSET $offset \
> +                                                       -b 1 $SCRATCH_DEV
>                         (( offset += 1 ))
>                 done
>                 _scratch_mount
> diff --git a/tests/btrfs/290 b/tests/btrfs/290
> index 61e741faeb45..d6f777776838 100755
> --- a/tests/btrfs/290
> +++ b/tests/btrfs/290
> @@ -58,7 +58,7 @@ corrupt_inline() {
>         _scratch_unmount
>         # inline data starts at disk_bytenr
>         # overwrite the first u64 with random bogus junk
> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV > /dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -72,7 +72,8 @@ corrupt_prealloc_to_reg() {
>         _scratch_unmount
>         # ensure non-zero at the pre-allocated region on disk
>         # set extent type from prealloc (2) to reg (1)
> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 1 $SCRATCH_DEV >/dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 1 $SCRATCH_DEV
>         _scratch_mount
>         # now that it's a regular file, reading actually looks at the previously
>         # preallocated region, so ensure that has non-zero contents.
> @@ -88,7 +89,8 @@ corrupt_reg_to_prealloc() {
>         _fsv_enable $f
>         _scratch_unmount
>         # set type from reg (1) to prealloc (2)
> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 2 $SCRATCH_DEV >/dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 2 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -104,7 +106,8 @@ corrupt_punch_hole() {
>         _fsv_enable $f
>         _scratch_unmount
>         # change disk_bytenr to 0, representing a hole
> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 0 $SCRATCH_DEV > /dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 0 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -118,7 +121,8 @@ corrupt_plug_hole() {
>         _fsv_enable $f
>         _scratch_unmount
>         # change disk_bytenr to some value, plugging the hole
> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 13639680 $SCRATCH_DEV > /dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
> +                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 13639680 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -132,7 +136,11 @@ corrupt_verity_descriptor() {
>         _scratch_unmount
>         # key for the descriptor item is <inode, BTRFS_VERITY_DESC_ITEM_KEY, 1>,
>         # 88 is X. So we write 5 Xs to the start of the descriptor
> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 0 -b 5 $SCRATCH_DEV > /dev/null 2>&1
> +       btrfs in dump-tree -t 5 $SCRATCH_DEV > $tmp.desc_dump_tree
> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 0 \
> +                                       -b 5 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -144,7 +152,10 @@ corrupt_root_hash() {
>         local ino=$(get_ino $f)
>         _fsv_enable $f
>         _scratch_unmount
> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 16 -b 1 $SCRATCH_DEV > /dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 16 \
> +                                       -b 1 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> @@ -159,7 +170,10 @@ corrupt_merkle_tree() {
>         # key for the descriptor item is <inode, BTRFS_VERITY_MERKLE_ITEM_KEY, 0>,
>         # 88 is X. So we write 5 Xs to somewhere in the middle of the first
>         # merkle item
> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v 88 -o 100 -b 5 $SCRATCH_DEV > /dev/null 2>&1
> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 100 \
> +                                       -b 5 $SCRATCH_DEV
>         _scratch_mount
>         validate $f
>  }
> --
> 2.39.3
>
>

Re: [PATCH] common/btrfs: set BTRFS_CORRUPT_BLOCK_OPT_<VALUE|OFFSET>

[Anand Jain]

On 3/21/24 16:43, Filipe Manana wrote:
> On Thu, Mar 21, 2024 at 4:10 AM Anand Jain <anand.jain@oracle.com> wrote:
>>
>> As btrfs-corrupt-block now uses --value instead of -v, and --offset
>> instead of -o, provide backward compatibility for the testcases, by
>> storing the option to be used in BTRFS_CORRUPT_BLOCK_OPT_VALUE and
>> BTRFS_CORRUPT_BLOCK_OPT_OFFSET. Also, removes the stdout and stderr
>> redirection to /dev/null.
> 
> This is complex and ugly, but most importantly this is not needed at all.
> 
> Just let all users of btrfs-corrupt-block use --value and --offset,
> because there was never
> a released version of btrfs-progs with the short options available.
> 
> The short options were introduced with:
> 
> commit b2ada0594116f3f4458581317e226c5976443ad0
> Author: Boris Burkov <boris@bur.io>
> Date:   Tue Jul 26 13:43:23 2022 -0700
> 
>      btrfs-progs: corrupt-block: corrupt generic item data
> 
> And then replacing them with long options happened in this commit:
> 
> commit 22ffee3c6cf2e6f285e6fd6cb22b88c02510e10e
> Author: David Sterba <dsterba@suse.com>
> Date:   Wed Jul 27 20:47:57 2022 +0200
> 
>      btrfs-progs: corrupt-block: use only long options for value and offset
> 

> Both commits landed in btrfs-progs 5.19, meaning there are no released
> versions with the short options.
> 
> The reason btrfs/290 is using the short options is because the
> btrfs-progs patch had just been submitted shortly before the test case
> was added.

fstests commits introducing the short options.

6defaf786e80 btrfs: test btrfs specific fsverity corruption
ea5b5f41fb61 common/verity: support btrfs in generic fsverity tests

btrfs-progs:

b2ada0594116 btrfs-progs: corrupt-block: corrupt generic item data
22ffee3c6cf2 btrfs-progs: corrupt-block: use only long options for value 
and offset

Good point. They are together in 5.19; they should have merged instead.
If any backports (which I doubt), should occur both patches.

Although I looked for the commits, I didn't notice that
there isn't a release with the short option. Thanks!

I found it ugly too, but I had no better solution.

Now, it's a relief that we don't need it anymore.

> However what we need is to have a _require_* helper that will make the
> test btrfs/290 not run if we're using a btrfs-progs version without
> those new options.

Yep. I'll send a patch.

Thanks.

> 
> Thanks.
> 
> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> This replaces the patch:
>>     [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block
>>
>>   common/btrfs    | 16 ++++++++++++++++
>>   common/verity   |  9 ++++++---
>>   tests/btrfs/290 | 30 ++++++++++++++++++++++--------
>>   3 files changed, 44 insertions(+), 11 deletions(-)
>>
>> diff --git a/common/btrfs b/common/btrfs
>> index ae13fb55cbc6..11d74bea9111 100644
>> --- a/common/btrfs
>> +++ b/common/btrfs
>> @@ -660,6 +660,22 @@ _btrfs_buffered_read_on_mirror()
>>   _require_btrfs_corrupt_block()
>>   {
>>          _require_command "$BTRFS_CORRUPT_BLOCK_PROG" btrfs-corrupt-block
>> +
>> +       # In the newer version, the option -v is replaced by --value,
>> +       # and -o is replaced by --offset, so normalize them.
>> +       $BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "value VALUE"
>> +       if [ $? == 0 ]; then
>> +               export BTRFS_CORRUPT_BLOCK_OPT_VALUE="--value"
>> +       else
>> +               export BTRFS_CORRUPT_BLOCK_OPT_VALUE="-v"
>> +       fi
>> +
>> +       $BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q "offset OFFSET"
>> +       if [ $? == 0 ]; then
>> +               export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="--offset"
>> +       else
>> +               export BTRFS_CORRUPT_BLOCK_OPT_OFFSET="-o"
>> +       fi
>>   }
>>
>>   _require_btrfs_send_version()
>> diff --git a/common/verity b/common/verity
>> index 03d175ce1b7a..33a1c12f558e 100644
>> --- a/common/verity
>> +++ b/common/verity
>> @@ -400,9 +400,12 @@ _fsv_scratch_corrupt_merkle_tree()
>>                          local ascii=$(printf "%d" "'$byte'")
>>                          # This command will find a Merkle tree item for the inode (-I $ino,37,0)
>>                          # in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
>> -                       # $offset (-o $offset) with the ascii representation of the byte we read
>> -                       # (-v $ascii)
>> -                       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
>> +                       # $offset (-o|--offset $offset) with the ascii
>> +                       # representation of the byte we read (-v|--value $ascii)
>> +                       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
>> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE $ascii \
>> +                               $BTRFS_CORRUPT_BLOCK_OPT_OFFSET $offset \
>> +                                                       -b 1 $SCRATCH_DEV
>>                          (( offset += 1 ))
>>                  done
>>                  _scratch_mount
>> diff --git a/tests/btrfs/290 b/tests/btrfs/290
>> index 61e741faeb45..d6f777776838 100755
>> --- a/tests/btrfs/290
>> +++ b/tests/btrfs/290
>> @@ -58,7 +58,7 @@ corrupt_inline() {
>>          _scratch_unmount
>>          # inline data starts at disk_bytenr
>>          # overwrite the first u64 with random bogus junk
>> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV > /dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f disk_bytenr $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -72,7 +72,8 @@ corrupt_prealloc_to_reg() {
>>          _scratch_unmount
>>          # ensure non-zero at the pre-allocated region on disk
>>          # set extent type from prealloc (2) to reg (1)
>> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 1 $SCRATCH_DEV >/dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
>> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 1 $SCRATCH_DEV
>>          _scratch_mount
>>          # now that it's a regular file, reading actually looks at the previously
>>          # preallocated region, so ensure that has non-zero contents.
>> @@ -88,7 +89,8 @@ corrupt_reg_to_prealloc() {
>>          _fsv_enable $f
>>          _scratch_unmount
>>          # set type from reg (1) to prealloc (2)
>> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type -v 2 $SCRATCH_DEV >/dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 0 -f type \
>> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 2 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -104,7 +106,8 @@ corrupt_punch_hole() {
>>          _fsv_enable $f
>>          _scratch_unmount
>>          # change disk_bytenr to 0, representing a hole
>> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 0 $SCRATCH_DEV > /dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
>> +                               $BTRFS_CORRUPT_BLOCK_OPT_VALUE 0 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -118,7 +121,8 @@ corrupt_plug_hole() {
>>          _fsv_enable $f
>>          _scratch_unmount
>>          # change disk_bytenr to some value, plugging the hole
>> -       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr -v 13639680 $SCRATCH_DEV > /dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -i $ino -x 4096 -f disk_bytenr \
>> +                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 13639680 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -132,7 +136,11 @@ corrupt_verity_descriptor() {
>>          _scratch_unmount
>>          # key for the descriptor item is <inode, BTRFS_VERITY_DESC_ITEM_KEY, 1>,
>>          # 88 is X. So we write 5 Xs to the start of the descriptor
>> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 0 -b 5 $SCRATCH_DEV > /dev/null 2>&1
>> +       btrfs in dump-tree -t 5 $SCRATCH_DEV > $tmp.desc_dump_tree
>> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 0 \
>> +                                       -b 5 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -144,7 +152,10 @@ corrupt_root_hash() {
>>          local ino=$(get_ino $f)
>>          _fsv_enable $f
>>          _scratch_unmount
>> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 -v 88 -o 16 -b 1 $SCRATCH_DEV > /dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,36,1 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 16 \
>> +                                       -b 1 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> @@ -159,7 +170,10 @@ corrupt_merkle_tree() {
>>          # key for the descriptor item is <inode, BTRFS_VERITY_MERKLE_ITEM_KEY, 0>,
>>          # 88 is X. So we write 5 Xs to somewhere in the middle of the first
>>          # merkle item
>> -       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v 88 -o 100 -b 5 $SCRATCH_DEV > /dev/null 2>&1
>> +       $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_VALUE 88 \
>> +                                       $BTRFS_CORRUPT_BLOCK_OPT_OFFSET 100 \
>> +                                       -b 5 $SCRATCH_DEV
>>          _scratch_mount
>>          validate $f
>>   }
>> --
>> 2.39.3
>>
>>

Re: [PATCH 5/5] generic/733: disable for btrfs

[Christoph Hellwig]

On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> It is a bug in the test, it should have been xfs specific and never
> promoted to generic/ and not affect btrfs unless explained how
> in the first place.

Well, the concept that reflinks are reasonably fast and are not live
locked by ongoing I/O seems pretty natural.  But I guess we can't
just asusme quality of implementation everywhere, and do an opt-in
like _require_non_sucky_reflink.  Either way we need to document
the assumptions and not add a magic exclude for a single fs.

Re: [PATCH 5/5] generic/733: disable for btrfs

[Kent Overstreet]

On Thu, Mar 21, 2024 at 02:36:47PM -0700, Christoph Hellwig wrote:
> On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> > It is a bug in the test, it should have been xfs specific and never
> > promoted to generic/ and not affect btrfs unless explained how
> > in the first place.
> 
> Well, the concept that reflinks are reasonably fast and are not live
> locked by ongoing I/O seems pretty natural.  But I guess we can't
> just asusme quality of implementation everywhere, and do an opt-in
> like _require_non_sucky_reflink.  Either way we need to document
> the assumptions and not add a magic exclude for a single fs.

generic/733 runs just fine on bcachefs, sounds like btrfs folks just
need to fix their filesystem

Re: [PATCH 5/5] generic/733: disable for btrfs

[Josef Bacik]

On Thu, Mar 21, 2024 at 05:52:33PM -0400, Kent Overstreet wrote:
> On Thu, Mar 21, 2024 at 02:36:47PM -0700, Christoph Hellwig wrote:
> > On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> > > It is a bug in the test, it should have been xfs specific and never
> > > promoted to generic/ and not affect btrfs unless explained how
> > > in the first place.
> > 
> > Well, the concept that reflinks are reasonably fast and are not live
> > locked by ongoing I/O seems pretty natural.  But I guess we can't
> > just asusme quality of implementation everywhere, and do an opt-in
> > like _require_non_sucky_reflink.  Either way we need to document
> > the assumptions and not add a magic exclude for a single fs.
> 
> generic/733 runs just fine on bcachefs, sounds like btrfs folks just
> need to fix their filesystem

Neither of these comments are particularly helpful or relevant to the
conversation.

Btrfs range locks the extent during the clone operation, and also range locks
the area that it reads.  This doesn't make it "sucky" or "worse", simply
different.  I'm not entirely sure why protecting a range of extents that's
currently being modified is considered "bad" or "broken".

In any case, I can accept that we need to have a different option for skipping
this test, but this is sort of an argument for the shared/ directory or some
other mechanism, or at the very least validating the test passes on all the
major file systems before including it as a generic test.

Adding an opt in _require feels like the best option, or we can simply keep
excluding it in our own fstests branch and ignore the upstream branch.  I'm good
with either solution.  Thanks,

Josef

Re: [PATCH 5/5] generic/733: disable for btrfs

[Darrick J. Wong]

On Fri, Mar 22, 2024 at 11:08:00AM -0400, Josef Bacik wrote:
> On Thu, Mar 21, 2024 at 05:52:33PM -0400, Kent Overstreet wrote:
> > On Thu, Mar 21, 2024 at 02:36:47PM -0700, Christoph Hellwig wrote:
> > > On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> > > > It is a bug in the test, it should have been xfs specific and never
> > > > promoted to generic/ and not affect btrfs unless explained how
> > > > in the first place.
> > > 
> > > Well, the concept that reflinks are reasonably fast and are not live
> > > locked by ongoing I/O seems pretty natural.  But I guess we can't
> > > just asusme quality of implementation everywhere, and do an opt-in
> > > like _require_non_sucky_reflink.  Either way we need to document
> > > the assumptions and not add a magic exclude for a single fs.
> > 
> > generic/733 runs just fine on bcachefs, sounds like btrfs folks just
> > need to fix their filesystem
> 
> Neither of these comments are particularly helpful or relevant to the
> conversation.
> 
> Btrfs range locks the extent during the clone operation, and also range locks
> the area that it reads.  This doesn't make it "sucky" or "worse", simply
> different.  I'm not entirely sure why protecting a range of extents that's
> currently being modified is considered "bad" or "broken".
> 
> In any case, I can accept that we need to have a different option for skipping
> this test, but this is sort of an argument for the shared/ directory or some
> other mechanism, or at the very least validating the test passes on all the
> major file systems before including it as a generic test.
> 
> Adding an opt in _require feels like the best option, or we can simply keep
> excluding it in our own fstests branch and ignore the upstream branch.  I'm good
> with either solution.  Thanks,

Frankly I'd have been ok with a per-test exclusion with some
documentation:

test $FSTYP = btrfs && \
	_notrun 'FIXME: btrfs does not support concurrent read+ficlone'

There's nothing in the FICLONE spec about implementations needing to
support concurrent reads and clones on the source file.

--D

> Josef
> 

Re: [PATCH 5/5] generic/733: disable for btrfs

[Kent Overstreet]

On Fri, Mar 22, 2024 at 11:08:00AM -0400, Josef Bacik wrote:
> On Thu, Mar 21, 2024 at 05:52:33PM -0400, Kent Overstreet wrote:
> > On Thu, Mar 21, 2024 at 02:36:47PM -0700, Christoph Hellwig wrote:
> > > On Wed, Mar 20, 2024 at 04:58:24PM +0100, David Sterba wrote:
> > > > It is a bug in the test, it should have been xfs specific and never
> > > > promoted to generic/ and not affect btrfs unless explained how
> > > > in the first place.
> > > 
> > > Well, the concept that reflinks are reasonably fast and are not live
> > > locked by ongoing I/O seems pretty natural.  But I guess we can't
> > > just asusme quality of implementation everywhere, and do an opt-in
> > > like _require_non_sucky_reflink.  Either way we need to document
> > > the assumptions and not add a magic exclude for a single fs.
> > 
> > generic/733 runs just fine on bcachefs, sounds like btrfs folks just
> > need to fix their filesystem
> 
> Neither of these comments are particularly helpful or relevant to the
> conversation.
> 
> Btrfs range locks the extent during the clone operation, and also range locks
> the area that it reads.  This doesn't make it "sucky" or "worse", simply
> different.  I'm not entirely sure why protecting a range of extents that's
> currently being modified is considered "bad" or "broken".
> 
> In any case, I can accept that we need to have a different option for skipping
> this test, but this is sort of an argument for the shared/ directory or some
> other mechanism, or at the very least validating the test passes on all the
> major file systems before including it as a generic test.
> 
> Adding an opt in _require feels like the best option, or we can simply keep
> excluding it in our own fstests branch and ignore the upstream branch.  I'm good
> with either solution.  Thanks,

Well, if you really don't intend to change btrfs that's fine; if it was
me I consider performance bugs to be bugs - not cases where we're off by
some small ratio, but if it's significant I'd want to fix it.

But if it's just that you're doing range locks (not locking the whole
inode, as was my previous impression) - that makes sense, there's a read
vs. write atomicity tradeoff there.

Re: [PATCH 1/5] common/verity: use the correct options for btrfs-corrupt-block

[Anand Jain]

On 3/20/24 20:53, David Sterba wrote:
> On Wed, Mar 20, 2024 at 03:28:52PM +0530, Anand Jain wrote:
>> On 3/19/24 23:42, David Sterba wrote:
>>> From: Josef Bacik <josef@toxicpanda.com>
>>>
>>> A long time ago we changed the short options to long options in
>>> btrfs-corrupt-block, so adjust the helper to use the correct options so
>>> the verity tests pass properly.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>    common/verity | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/common/verity b/common/verity
>>> index 03d175ce1b7a18..0e5f0d75e746a8 100644
>>> --- a/common/verity
>>> +++ b/common/verity
>>> @@ -402,7 +402,7 @@ _fsv_scratch_corrupt_merkle_tree()
>>>    			# in the default filesystem tree (-r 5) and corrupt one byte (-b 1) at
>>>    			# $offset (-o $offset) with the ascii representation of the byte we read
>>>    			# (-v $ascii)
>>
>>> -			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 -v $ascii -o $offset -b 1 $SCRATCH_DEV
>>> +			$BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 --value $ascii --offset $offset -b 1 $SCRATCH_DEV
>>
>>
>> Can we still make it work with the older btrfs-corrupt-block?
>>
>> How about..
>>
>> corrupt_block_value_opt()
>> {
>> 	$BTRFS_CORRUPT_BLOCK_PROG -h 2>&1 | grep -q -e "--value"
>> 	if [ $? == 0 ]; then
>> 		echo "--value"
>> 	else
>> 		echo "-v"
>> 	fi
>> }
>>
>> And to use,
>>
>> $BTRFS_CORRUPT_BLOCK_PROG -r 5 -I $ino,37,0 $(corrupt_block_value_opt)
>> $ascii --offset $offset -b 1 $SCRATCH_DEV
>>
>>
>> I will make this change before submitting the PR if no objection.
> 
> Thanks, that would be great. The option changed in btrfs-progs 5.18
> which is still relatively recent so both options should be supported.


This patch has been replaced, [1], is for review comments.

[1] 
https://lore.kernel.org/fstests/cover.1711097698.git.anand.jain@oracle.com/

Thanks, Anand

Re: [PATCH 0/5] Btrfs fstests fixups and updates

[Anand Jain]

On 3/19/24 23:41, David Sterba wrote:
> Hi,
> 
> a few more fixup from our repository.  They have been in our CI branch
> and consider them tested and working for our needs. All of them are
> btrfs-specific and will not affect other filesystems.
> 
> Please apply, thanks.
> 


> David Sterba (2):
>    common/rc: use proper temporary file path in _repair_test_fs()


Except for these two patches, the rest have been applied for the PR.
{
>    generic/733: disable for btrfs > Josef Bacik (3):
>    common/verity: use the correct options for btrfs-corrupt-block}

Thanks, Anand

>    btrfs/131,btrfs/172,btrfs/206: add check for block-group-tree feature
>      in btrfs
>    btrfs/330: add test to validate ro/rw subvol mounting
> 



>   common/btrfs        | 10 +++++++++
>   common/rc           |  4 ++--
>   common/verity       |  2 +-
>   tests/btrfs/131     |  2 ++
>   tests/btrfs/172     |  3 +++
>   tests/btrfs/206     |  3 +++
>   tests/btrfs/330     | 54 +++++++++++++++++++++++++++++++++++++++++++++
>   tests/btrfs/330.out |  6 +++++
>   tests/generic/733   |  2 +-
>   9 files changed, 82 insertions(+), 4 deletions(-)
>   create mode 100755 tests/btrfs/330
>   create mode 100644 tests/btrfs/330.out
>