[PATCH 0/4] Large extent counters tests

[PATCH 0/4] Large extent counters tests

[Chandan Babu R]

Hi,

This patchset adds two tests for verifying the behaviour of Large
extent counters feature. It also fixes xfs/270 test failure when
executed on a filesystem with Large extent counters enabled.

Chandan Babu R (4):
  xfs/270: Fix ro mount failure when nrext64 option is enabled
  common/xfs: Add helper to check if nrext64 option is supported
  xfs/547: Verify that the correct inode extent counters are updated
    with/without nrext64
  xfs/548: Verify correctness of upgrading an fs to support large extent
    counters

 common/xfs        |  13 ++++++
 tests/xfs/270     |  16 ++++++-
 tests/xfs/270.out |   1 -
 tests/xfs/547     |  91 ++++++++++++++++++++++++++++++++++++++
 tests/xfs/547.out |  13 ++++++
 tests/xfs/548     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/548.out |  12 +++++
 7 files changed, 252 insertions(+), 3 deletions(-)
 create mode 100755 tests/xfs/547
 create mode 100644 tests/xfs/547.out
 create mode 100755 tests/xfs/548
 create mode 100644 tests/xfs/548.out

-- 
2.35.1

[PATCH 1/4] xfs/270: Fix ro mount failure when nrext64 option is enabled

[Chandan Babu R]

With nrext64 option enabled at run time, the read-only mount performed by the
test fails because,
1. mkfs.xfs would have calculated log size based on reflink being enabled.
2. Clearing the reflink ro compat bit causes log size calculations to yield a
   different value.
3. In the case where nrext64 is enabled, this causes attr reservation to be
   the largest among all the transaction reservations.
4. This ends up causing XFS to require a larger ondisk log size than that
   which is available.

This commit fixes the problem by setting features_ro_compat to the value
obtained by the bitwise-OR of features_ro_compat field with 2^31.

Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
 tests/xfs/270     | 16 ++++++++++++++--
 tests/xfs/270.out |  1 -
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/tests/xfs/270 b/tests/xfs/270
index 0ab0c7d8..f3796691 100755
--- a/tests/xfs/270
+++ b/tests/xfs/270
@@ -27,8 +27,20 @@ _scratch_mkfs_xfs >>$seqres.full 2>&1
 # set the highest bit of features_ro_compat, use it as an unknown
 # feature bit. If one day this bit become known feature, please
 # change this case.
-_scratch_xfs_set_metadata_field "features_ro_compat" "$((2**31))" "sb 0" | \
-	grep 'features_ro_compat'
+ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
+ro_compat=${ro_compat##0x}
+ro_compat="16#"${ro_compat}
+ro_compat=$(($ro_compat|16#80000000))
+ro_compat=$(_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" \
+					    "sb 0" | grep 'features_ro_compat')
+
+ro_compat=${ro_compat##features_ro_compat = 0x}
+ro_compat="16#"${ro_compat}
+ro_compat=$(($ro_compat&16#80000000))
+if (( $ro_compat != 16#80000000 )); then
+	echo "Unable to set most significant bit of features_ro_compat"
+fi
+
 
 # rw mount with unknown ro-compat feature should fail
 echo "rw mount test"
diff --git a/tests/xfs/270.out b/tests/xfs/270.out
index 0a8b3851..edf4c254 100644
--- a/tests/xfs/270.out
+++ b/tests/xfs/270.out
@@ -1,5 +1,4 @@
 QA output created by 270
-features_ro_compat = 0x80000000
 rw mount test
 ro mount test
 rw remount test
-- 
2.35.1

[PATCH 2/4] common/xfs: Add helper to check if nrext64 option is supported

[Chandan Babu R]

This commit adds a new helper to allow tests to check if xfsprogs and xfs
kernel module support nrext64 option.

Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
 common/xfs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/common/xfs b/common/xfs
index 2123a4ab..dca7af57 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1328,3 +1328,16 @@ _xfs_filter_mkfs()
 		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
 	}'
 }
+
+_require_scratch_xfs_nrext64()
+{
+	_require_scratch
+
+	_scratch_mkfs -i nrext64=1 &>/dev/null || \
+		_notrun "mkfs.xfs doesn't support nrext64 feature"
+	_try_scratch_mount || \
+		_notrun "kernel doesn't support xfs nrext64 feature"
+	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "nrext64=1" || \
+		_notrun "nrext64 feature not advertised on mount?"
+	_scratch_unmount
+}
-- 
2.35.1

[PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Chandan Babu R]

This commit adds a new test to verify if the correct inode extent counter
fields are updated with/without nrext64 mkfs option.

Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
 tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/547.out | 13 +++++++
 2 files changed, 104 insertions(+)
 create mode 100755 tests/xfs/547
 create mode 100644 tests/xfs/547.out

diff --git a/tests/xfs/547 b/tests/xfs/547
new file mode 100755
index 00000000..d5137ca7
--- /dev/null
+++ b/tests/xfs/547
@@ -0,0 +1,91 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 547
+#
+# Verify that correct inode extent count fields are populated with and without
+# nrext64 feature.
+#
+. ./common/preamble
+_begin_fstest auto quick metadata
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+. ./common/inject
+. ./common/populate
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+_require_scratch_xfs_nrext64
+_require_attrs
+_require_xfs_debug
+_require_test_program "punch-alternating"
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+
+for nrext64 in 0 1; do
+	echo "* Verify extent counter fields with nrext64=${nrext64} option"
+
+	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
+		      >> $seqres.full
+	_scratch_mount >> $seqres.full
+
+	bsize=$(_get_file_block_size $SCRATCH_MNT)
+
+	testfile=$SCRATCH_MNT/testfile
+
+	nr_blks=20
+
+	echo "Add blocks to test file's data fork"
+	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
+		     >> $seqres.full
+	$here/src/punch-alternating $testfile
+
+	echo "Consume free space"
+	fillerdir=$SCRATCH_MNT/fillerdir
+	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
+	nr_free_blks=$((nr_free_blks * 90 / 100))
+
+	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
+		 >> $seqres.full 2>&1
+
+	echo "Create fragmented filesystem"
+	for dentry in $(ls -1 $fillerdir/); do
+		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
+	done
+
+	echo "Inject bmap_alloc_minlen_extent error tag"
+	_scratch_inject_error bmap_alloc_minlen_extent 1
+
+	echo "Add blocks to test file's attr fork"
+	attr_len=255
+	nr_attrs=$((nr_blks * bsize / attr_len))
+	for i in $(seq 1 $nr_attrs); do
+		attr="$(printf "trusted.%0247d" $i)"
+		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
+		[[ $? != 0 ]] && break
+	done
+
+	testino=$(stat -c '%i' $testfile)
+
+	_scratch_unmount >> $seqres.full
+
+	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
+	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
+
+	if (( $dcnt != 10 )); then
+		echo "Invalid data fork extent count: $dextcnt"
+		exit 1
+	fi
+
+	if (( $acnt < 10 )); then
+		echo "Invalid attr fork extent count: $aextcnt"
+		exit 1
+	fi
+done
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/547.out b/tests/xfs/547.out
new file mode 100644
index 00000000..49fcc3c2
--- /dev/null
+++ b/tests/xfs/547.out
@@ -0,0 +1,13 @@
+QA output created by 547
+* Verify extent counter fields with nrext64=0 option
+Add blocks to test file's data fork
+Consume free space
+Create fragmented filesystem
+Inject bmap_alloc_minlen_extent error tag
+Add blocks to test file's attr fork
+* Verify extent counter fields with nrext64=1 option
+Add blocks to test file's data fork
+Consume free space
+Create fragmented filesystem
+Inject bmap_alloc_minlen_extent error tag
+Add blocks to test file's attr fork
-- 
2.35.1

[PATCH 4/4] xfs/548: Verify correctness of upgrading an fs to support large extent counters

[Chandan Babu R]

This commit adds a test to verify upgrade of an existing V5 filesystem to
support large extent counters.

Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
---
 tests/xfs/548     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/548.out |  12 +++++
 2 files changed, 121 insertions(+)
 create mode 100755 tests/xfs/548
 create mode 100644 tests/xfs/548.out

diff --git a/tests/xfs/548 b/tests/xfs/548
new file mode 100755
index 00000000..6c577584
--- /dev/null
+++ b/tests/xfs/548
@@ -0,0 +1,109 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2022 Oracle.  All Rights Reserved.
+#
+# FS QA Test 548
+#
+# Test to verify upgrade of an existing V5 filesystem to support large extent
+# counters.
+#
+. ./common/preamble
+_begin_fstest auto quick metadata
+
+# Import common functions.
+. ./common/filter
+. ./common/attr
+. ./common/inject
+. ./common/populate
+
+# real QA test starts here
+_supported_fs xfs
+_require_scratch
+_require_scratch_xfs_nrext64
+_require_attrs
+_require_xfs_debug
+_require_test_program "punch-alternating"
+_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
+
+_scratch_mkfs -d size=$((512 * 1024 * 1024)) >> $seqres.full
+_scratch_mount >> $seqres.full
+
+bsize=$(_get_file_block_size $SCRATCH_MNT)
+
+testfile=$SCRATCH_MNT/testfile
+
+nr_blks=20
+
+echo "Add blocks to file's data fork"
+$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
+	     >> $seqres.full
+$here/src/punch-alternating $testfile
+
+echo "Consume free space"
+fillerdir=$SCRATCH_MNT/fillerdir
+nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
+nr_free_blks=$((nr_free_blks * 90 / 100))
+
+_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
+	 >> $seqres.full 2>&1
+
+echo "Create fragmented filesystem"
+for dentry in $(ls -1 $fillerdir/); do
+	$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
+done
+
+echo "Inject bmap_alloc_minlen_extent error tag"
+_scratch_inject_error bmap_alloc_minlen_extent 1
+
+echo "Add blocks to file's attr fork"
+nr_blks=10
+attr_len=255
+nr_attrs=$((nr_blks * bsize / attr_len))
+for i in $(seq 1 $nr_attrs); do
+	attr="$(printf "trusted.%0247d" $i)"
+	$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
+	[[ $? != 0 ]] && break
+done
+
+testino=$(stat -c '%i' $testfile)
+
+echo "Unmount filesystem"
+_scratch_unmount >> $seqres.full
+
+orig_dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
+orig_acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
+
+echo "Upgrade filesystem to support large extent counters"
+_scratch_xfs_admin -O nrext64=1 >> $seqres.full 2>&1
+if [[ $? != 0 ]]; then
+	_notrun "Filesystem geometry is not suitable for upgrading"
+fi
+
+
+echo "Mount filesystem"
+_scratch_mount >> $seqres.full
+
+echo "Modify inode core"
+touch $testfile
+
+echo "Unmount filesystem"
+_scratch_unmount >> $seqres.full
+
+dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
+acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
+
+echo "Verify inode extent counter values after fs upgrade"
+
+if [[ $orig_dcnt != $dcnt ]]; then
+	echo "Corrupt data extent counter"
+	exit 1
+fi
+
+if [[ $orig_acnt != $acnt ]]; then
+	echo "Corrupt attr extent counter"
+	exit 1
+fi
+
+# success, all done
+status=0
+exit
diff --git a/tests/xfs/548.out b/tests/xfs/548.out
new file mode 100644
index 00000000..19a7f907
--- /dev/null
+++ b/tests/xfs/548.out
@@ -0,0 +1,12 @@
+QA output created by 548
+Add blocks to file's data fork
+Consume free space
+Create fragmented filesystem
+Inject bmap_alloc_minlen_extent error tag
+Add blocks to file's attr fork
+Unmount filesystem
+Upgrade filesystem to support large extent counters
+Mount filesystem
+Modify inode core
+Unmount filesystem
+Verify inode extent counter values after fs upgrade
-- 
2.35.1

Re: [PATCH 2/4] common/xfs: Add helper to check if nrext64 option is supported

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 06:10:59PM +0530, Chandan Babu R wrote:
> This commit adds a new helper to allow tests to check if xfsprogs and xfs
> kernel module support nrext64 option.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>

Looks fine to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/xfs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 2123a4ab..dca7af57 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1328,3 +1328,16 @@ _xfs_filter_mkfs()
>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>  	}'
>  }
> +
> +_require_scratch_xfs_nrext64()
> +{
> +	_require_scratch
> +
> +	_scratch_mkfs -i nrext64=1 &>/dev/null || \
> +		_notrun "mkfs.xfs doesn't support nrext64 feature"
> +	_try_scratch_mount || \
> +		_notrun "kernel doesn't support xfs nrext64 feature"
> +	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "nrext64=1" || \
> +		_notrun "nrext64 feature not advertised on mount?"
> +	_scratch_unmount
> +}
> -- 
> 2.35.1
> 

Re: [PATCH 1/4] xfs/270: Fix ro mount failure when nrext64 option is enabled

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 06:10:58PM +0530, Chandan Babu R wrote:
> With nrext64 option enabled at run time, the read-only mount performed by the
> test fails because,
> 1. mkfs.xfs would have calculated log size based on reflink being enabled.
> 2. Clearing the reflink ro compat bit causes log size calculations to yield a
>    different value.
> 3. In the case where nrext64 is enabled, this causes attr reservation to be
>    the largest among all the transaction reservations.
> 4. This ends up causing XFS to require a larger ondisk log size than that
>    which is available.
> 
> This commit fixes the problem by setting features_ro_compat to the value
> obtained by the bitwise-OR of features_ro_compat field with 2^31.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  tests/xfs/270     | 16 ++++++++++++++--
>  tests/xfs/270.out |  1 -
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/xfs/270 b/tests/xfs/270
> index 0ab0c7d8..f3796691 100755
> --- a/tests/xfs/270
> +++ b/tests/xfs/270
> @@ -27,8 +27,20 @@ _scratch_mkfs_xfs >>$seqres.full 2>&1
>  # set the highest bit of features_ro_compat, use it as an unknown
>  # feature bit. If one day this bit become known feature, please
>  # change this case.
> -_scratch_xfs_set_metadata_field "features_ro_compat" "$((2**31))" "sb 0" | \
> -	grep 'features_ro_compat'
> +ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
> +ro_compat=${ro_compat##0x}
> +ro_compat="16#"${ro_compat}
> +ro_compat=$(($ro_compat|16#80000000))

I guess I just learned a  ^^^^^ new bashism today.

> +ro_compat=$(_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" \
> +					    "sb 0" | grep 'features_ro_compat')
> +
> +ro_compat=${ro_compat##features_ro_compat = 0x}
> +ro_compat="16#"${ro_compat}
> +ro_compat=$(($ro_compat&16#80000000))
> +if (( $ro_compat != 16#80000000 )); then
> +	echo "Unable to set most significant bit of features_ro_compat"
> +fi

Thanks for the fix,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  
>  # rw mount with unknown ro-compat feature should fail
>  echo "rw mount test"
> diff --git a/tests/xfs/270.out b/tests/xfs/270.out
> index 0a8b3851..edf4c254 100644
> --- a/tests/xfs/270.out
> +++ b/tests/xfs/270.out
> @@ -1,5 +1,4 @@
>  QA output created by 270
> -features_ro_compat = 0x80000000
>  rw mount test
>  ro mount test
>  rw remount test
> -- 
> 2.35.1
> 

Re: [PATCH 4/4] xfs/548: Verify correctness of upgrading an fs to support large extent counters

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 06:11:01PM +0530, Chandan Babu R wrote:
> This commit adds a test to verify upgrade of an existing V5 filesystem to
> support large extent counters.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  tests/xfs/548     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/548.out |  12 +++++
>  2 files changed, 121 insertions(+)
>  create mode 100755 tests/xfs/548
>  create mode 100644 tests/xfs/548.out
> 
> diff --git a/tests/xfs/548 b/tests/xfs/548
> new file mode 100755
> index 00000000..6c577584
> --- /dev/null
> +++ b/tests/xfs/548
> @@ -0,0 +1,109 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 548
> +#
> +# Test to verify upgrade of an existing V5 filesystem to support large extent
> +# counters.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick metadata
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +. ./common/inject
> +. ./common/populate
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_require_scratch
> +_require_scratch_xfs_nrext64
> +_require_attrs
> +_require_xfs_debug
> +_require_test_program "punch-alternating"
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +
> +_scratch_mkfs -d size=$((512 * 1024 * 1024)) >> $seqres.full
> +_scratch_mount >> $seqres.full
> +
> +bsize=$(_get_file_block_size $SCRATCH_MNT)
> +
> +testfile=$SCRATCH_MNT/testfile
> +
> +nr_blks=20
> +
> +echo "Add blocks to file's data fork"
> +$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
> +	     >> $seqres.full
> +$here/src/punch-alternating $testfile
> +
> +echo "Consume free space"
> +fillerdir=$SCRATCH_MNT/fillerdir
> +nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> +nr_free_blks=$((nr_free_blks * 90 / 100))
> +
> +_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
> +	 >> $seqres.full 2>&1
> +
> +echo "Create fragmented filesystem"
> +for dentry in $(ls -1 $fillerdir/); do
> +	$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
> +done
> +
> +echo "Inject bmap_alloc_minlen_extent error tag"
> +_scratch_inject_error bmap_alloc_minlen_extent 1
> +
> +echo "Add blocks to file's attr fork"
> +nr_blks=10
> +attr_len=255
> +nr_attrs=$((nr_blks * bsize / attr_len))
> +for i in $(seq 1 $nr_attrs); do
> +	attr="$(printf "trusted.%0247d" $i)"
> +	$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
> +	[[ $? != 0 ]] && break
> +done
> +
> +testino=$(stat -c '%i' $testfile)
> +
> +echo "Unmount filesystem"
> +_scratch_unmount >> $seqres.full
> +
> +orig_dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> +orig_acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
> +
> +echo "Upgrade filesystem to support large extent counters"
> +_scratch_xfs_admin -O nrext64=1 >> $seqres.full 2>&1
> +if [[ $? != 0 ]]; then
> +	_notrun "Filesystem geometry is not suitable for upgrading"
> +fi
> +
> +
> +echo "Mount filesystem"
> +_scratch_mount >> $seqres.full
> +
> +echo "Modify inode core"
> +touch $testfile
> +
> +echo "Unmount filesystem"
> +_scratch_unmount >> $seqres.full
> +
> +dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> +acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
> +
> +echo "Verify inode extent counter values after fs upgrade"

Is there a scenario where the inode counters would become corrupt after
enabling the superblock feature bit?  IIRC repair doesn't rewrite the
inodes during the upgrade... so is this test merely being cautious?  Or
is this covering a failure you found somewhere while writing the feature?

--D

> +
> +if [[ $orig_dcnt != $dcnt ]]; then
> +	echo "Corrupt data extent counter"
> +	exit 1
> +fi
> +
> +if [[ $orig_acnt != $acnt ]]; then
> +	echo "Corrupt attr extent counter"
> +	exit 1
> +fi
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/548.out b/tests/xfs/548.out
> new file mode 100644
> index 00000000..19a7f907
> --- /dev/null
> +++ b/tests/xfs/548.out
> @@ -0,0 +1,12 @@
> +QA output created by 548
> +Add blocks to file's data fork
> +Consume free space
> +Create fragmented filesystem
> +Inject bmap_alloc_minlen_extent error tag
> +Add blocks to file's attr fork
> +Unmount filesystem
> +Upgrade filesystem to support large extent counters
> +Mount filesystem
> +Modify inode core
> +Unmount filesystem
> +Verify inode extent counter values after fs upgrade
> -- 
> 2.35.1
> 

Re: [PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Darrick J. Wong]

On Mon, Jun 06, 2022 at 06:11:00PM +0530, Chandan Babu R wrote:
> This commit adds a new test to verify if the correct inode extent counter
> fields are updated with/without nrext64 mkfs option.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/547.out | 13 +++++++
>  2 files changed, 104 insertions(+)
>  create mode 100755 tests/xfs/547
>  create mode 100644 tests/xfs/547.out
> 
> diff --git a/tests/xfs/547 b/tests/xfs/547
> new file mode 100755
> index 00000000..d5137ca7
> --- /dev/null
> +++ b/tests/xfs/547
> @@ -0,0 +1,91 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> +#
> +# FS QA Test 547
> +#
> +# Verify that correct inode extent count fields are populated with and without
> +# nrext64 feature.
> +#
> +. ./common/preamble
> +_begin_fstest auto quick metadata
> +
> +# Import common functions.
> +. ./common/filter
> +. ./common/attr
> +. ./common/inject
> +. ./common/populate
> +
> +# real QA test starts here
> +_supported_fs xfs
> +_require_scratch
> +_require_scratch_xfs_nrext64
> +_require_attrs
> +_require_xfs_debug
> +_require_test_program "punch-alternating"
> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> +
> +for nrext64 in 0 1; do
> +	echo "* Verify extent counter fields with nrext64=${nrext64} option"
> +
> +	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
> +		      >> $seqres.full
> +	_scratch_mount >> $seqres.full
> +
> +	bsize=$(_get_file_block_size $SCRATCH_MNT)
> +
> +	testfile=$SCRATCH_MNT/testfile
> +
> +	nr_blks=20
> +
> +	echo "Add blocks to test file's data fork"
> +	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
> +		     >> $seqres.full
> +	$here/src/punch-alternating $testfile
> +
> +	echo "Consume free space"
> +	fillerdir=$SCRATCH_MNT/fillerdir
> +	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> +	nr_free_blks=$((nr_free_blks * 90 / 100))
> +
> +	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
> +		 >> $seqres.full 2>&1
> +
> +	echo "Create fragmented filesystem"
> +	for dentry in $(ls -1 $fillerdir/); do
> +		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
> +	done
> +
> +	echo "Inject bmap_alloc_minlen_extent error tag"
> +	_scratch_inject_error bmap_alloc_minlen_extent 1
> +
> +	echo "Add blocks to test file's attr fork"
> +	attr_len=255
> +	nr_attrs=$((nr_blks * bsize / attr_len))
> +	for i in $(seq 1 $nr_attrs); do
> +		attr="$(printf "trusted.%0247d" $i)"
> +		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
> +		[[ $? != 0 ]] && break
> +	done
> +
> +	testino=$(stat -c '%i' $testfile)
> +
> +	_scratch_unmount >> $seqres.full
> +
> +	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> +	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")

Note: For any test requiring functionality added after 5.10, you can use
the xfs_db path command to avoid this sort of inode number saving:

dcnt=$(_scratch_xfs_get_metadata_field core.nextents "path /testfile")

Up to you if you want to change the test to do that; otherwise,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
> +	if (( $dcnt != 10 )); then
> +		echo "Invalid data fork extent count: $dextcnt"
> +		exit 1
> +	fi
> +
> +	if (( $acnt < 10 )); then
> +		echo "Invalid attr fork extent count: $aextcnt"
> +		exit 1
> +	fi
> +done
> +
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/xfs/547.out b/tests/xfs/547.out
> new file mode 100644
> index 00000000..49fcc3c2
> --- /dev/null
> +++ b/tests/xfs/547.out
> @@ -0,0 +1,13 @@
> +QA output created by 547
> +* Verify extent counter fields with nrext64=0 option
> +Add blocks to test file's data fork
> +Consume free space
> +Create fragmented filesystem
> +Inject bmap_alloc_minlen_extent error tag
> +Add blocks to test file's attr fork
> +* Verify extent counter fields with nrext64=1 option
> +Add blocks to test file's data fork
> +Consume free space
> +Create fragmented filesystem
> +Inject bmap_alloc_minlen_extent error tag
> +Add blocks to test file's attr fork
> -- 
> 2.35.1
> 

Re: [PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Chandan Babu R]

On Mon, Jun 06, 2022 at 08:40:47 AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 06, 2022 at 06:11:00PM +0530, Chandan Babu R wrote:
>> This commit adds a new test to verify if the correct inode extent counter
>> fields are updated with/without nrext64 mkfs option.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/547.out | 13 +++++++
>>  2 files changed, 104 insertions(+)
>>  create mode 100755 tests/xfs/547
>>  create mode 100644 tests/xfs/547.out
>> 
>> diff --git a/tests/xfs/547 b/tests/xfs/547
>> new file mode 100755
>> index 00000000..d5137ca7
>> --- /dev/null
>> +++ b/tests/xfs/547
>> @@ -0,0 +1,91 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
>> +#
>> +# FS QA Test 547
>> +#
>> +# Verify that correct inode extent count fields are populated with and without
>> +# nrext64 feature.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick metadata
>> +
>> +# Import common functions.
>> +. ./common/filter
>> +. ./common/attr
>> +. ./common/inject
>> +. ./common/populate
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_require_scratch
>> +_require_scratch_xfs_nrext64
>> +_require_attrs
>> +_require_xfs_debug
>> +_require_test_program "punch-alternating"
>> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
>> +
>> +for nrext64 in 0 1; do
>> +	echo "* Verify extent counter fields with nrext64=${nrext64} option"
>> +
>> +	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
>> +		      >> $seqres.full
>> +	_scratch_mount >> $seqres.full
>> +
>> +	bsize=$(_get_file_block_size $SCRATCH_MNT)
>> +
>> +	testfile=$SCRATCH_MNT/testfile
>> +
>> +	nr_blks=20
>> +
>> +	echo "Add blocks to test file's data fork"
>> +	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
>> +		     >> $seqres.full
>> +	$here/src/punch-alternating $testfile
>> +
>> +	echo "Consume free space"
>> +	fillerdir=$SCRATCH_MNT/fillerdir
>> +	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
>> +	nr_free_blks=$((nr_free_blks * 90 / 100))
>> +
>> +	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
>> +		 >> $seqres.full 2>&1
>> +
>> +	echo "Create fragmented filesystem"
>> +	for dentry in $(ls -1 $fillerdir/); do
>> +		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
>> +	done
>> +
>> +	echo "Inject bmap_alloc_minlen_extent error tag"
>> +	_scratch_inject_error bmap_alloc_minlen_extent 1
>> +
>> +	echo "Add blocks to test file's attr fork"
>> +	attr_len=255
>> +	nr_attrs=$((nr_blks * bsize / attr_len))
>> +	for i in $(seq 1 $nr_attrs); do
>> +		attr="$(printf "trusted.%0247d" $i)"
>> +		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
>> +		[[ $? != 0 ]] && break
>> +	done
>> +
>> +	testino=$(stat -c '%i' $testfile)
>> +
>> +	_scratch_unmount >> $seqres.full
>> +
>> +	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
>> +	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
>
> Note: For any test requiring functionality added after 5.10, you can use
> the xfs_db path command to avoid this sort of inode number saving:
>
> dcnt=$(_scratch_xfs_get_metadata_field core.nextents "path /testfile")
>

Ok. I will post a v2 of the patchset to include the above suggestion.

> Up to you if you want to change the test to do that; otherwise,
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>

Thanks for the review.

-- 
chandan

Re: [PATCH 4/4] xfs/548: Verify correctness of upgrading an fs to support large extent counters

[Chandan Babu R]

On Mon, Jun 06, 2022 at 08:35:19 AM -0700, Darrick J. Wong wrote:
> On Mon, Jun 06, 2022 at 06:11:01PM +0530, Chandan Babu R wrote:
>> This commit adds a test to verify upgrade of an existing V5 filesystem to
>> support large extent counters.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  tests/xfs/548     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/548.out |  12 +++++
>>  2 files changed, 121 insertions(+)
>>  create mode 100755 tests/xfs/548
>>  create mode 100644 tests/xfs/548.out
>> 
>> diff --git a/tests/xfs/548 b/tests/xfs/548
>> new file mode 100755
>> index 00000000..6c577584
>> --- /dev/null
>> +++ b/tests/xfs/548
>> @@ -0,0 +1,109 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
>> +#
>> +# FS QA Test 548
>> +#
>> +# Test to verify upgrade of an existing V5 filesystem to support large extent
>> +# counters.
>> +#
>> +. ./common/preamble
>> +_begin_fstest auto quick metadata
>> +
>> +# Import common functions.
>> +. ./common/filter
>> +. ./common/attr
>> +. ./common/inject
>> +. ./common/populate
>> +
>> +# real QA test starts here
>> +_supported_fs xfs
>> +_require_scratch
>> +_require_scratch_xfs_nrext64
>> +_require_attrs
>> +_require_xfs_debug
>> +_require_test_program "punch-alternating"
>> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
>> +
>> +_scratch_mkfs -d size=$((512 * 1024 * 1024)) >> $seqres.full
>> +_scratch_mount >> $seqres.full
>> +
>> +bsize=$(_get_file_block_size $SCRATCH_MNT)
>> +
>> +testfile=$SCRATCH_MNT/testfile
>> +
>> +nr_blks=20
>> +
>> +echo "Add blocks to file's data fork"
>> +$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
>> +	     >> $seqres.full
>> +$here/src/punch-alternating $testfile
>> +
>> +echo "Consume free space"
>> +fillerdir=$SCRATCH_MNT/fillerdir
>> +nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
>> +nr_free_blks=$((nr_free_blks * 90 / 100))
>> +
>> +_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
>> +	 >> $seqres.full 2>&1
>> +
>> +echo "Create fragmented filesystem"
>> +for dentry in $(ls -1 $fillerdir/); do
>> +	$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
>> +done
>> +
>> +echo "Inject bmap_alloc_minlen_extent error tag"
>> +_scratch_inject_error bmap_alloc_minlen_extent 1
>> +
>> +echo "Add blocks to file's attr fork"
>> +nr_blks=10
>> +attr_len=255
>> +nr_attrs=$((nr_blks * bsize / attr_len))
>> +for i in $(seq 1 $nr_attrs); do
>> +	attr="$(printf "trusted.%0247d" $i)"
>> +	$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
>> +	[[ $? != 0 ]] && break
>> +done
>> +
>> +testino=$(stat -c '%i' $testfile)
>> +
>> +echo "Unmount filesystem"
>> +_scratch_unmount >> $seqres.full
>> +
>> +orig_dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
>> +orig_acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
>> +
>> +echo "Upgrade filesystem to support large extent counters"
>> +_scratch_xfs_admin -O nrext64=1 >> $seqres.full 2>&1
>> +if [[ $? != 0 ]]; then
>> +	_notrun "Filesystem geometry is not suitable for upgrading"
>> +fi
>> +
>> +
>> +echo "Mount filesystem"
>> +_scratch_mount >> $seqres.full
>> +
>> +echo "Modify inode core"
>> +touch $testfile
>> +
>> +echo "Unmount filesystem"
>> +_scratch_unmount >> $seqres.full
>> +
>> +dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
>> +acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
>> +
>> +echo "Verify inode extent counter values after fs upgrade"
>
> Is there a scenario where the inode counters would become corrupt after
> enabling the superblock feature bit?  IIRC repair doesn't rewrite the
> inodes during the upgrade... so is this test merely being cautious?  Or
> is this covering a failure you found somewhere while writing the feature?
>

I was just being cautious w.r.t "Large extent counters" functionality working
correctly. I used this test during my development to make sure that I was able
to capture failures before I ran the entire xfstests suite.

-- 
chandan

Re: [PATCH 4/4] xfs/548: Verify correctness of upgrading an fs to support large extent counters

[Darrick J. Wong]

On Tue, Jun 07, 2022 at 03:17:01PM +0530, Chandan Babu R wrote:
> On Mon, Jun 06, 2022 at 08:35:19 AM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 06, 2022 at 06:11:01PM +0530, Chandan Babu R wrote:
> >> This commit adds a test to verify upgrade of an existing V5 filesystem to
> >> support large extent counters.
> >> 
> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> >> ---
> >>  tests/xfs/548     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/xfs/548.out |  12 +++++
> >>  2 files changed, 121 insertions(+)
> >>  create mode 100755 tests/xfs/548
> >>  create mode 100644 tests/xfs/548.out
> >> 
> >> diff --git a/tests/xfs/548 b/tests/xfs/548
> >> new file mode 100755
> >> index 00000000..6c577584
> >> --- /dev/null
> >> +++ b/tests/xfs/548
> >> @@ -0,0 +1,109 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> >> +#
> >> +# FS QA Test 548
> >> +#
> >> +# Test to verify upgrade of an existing V5 filesystem to support large extent
> >> +# counters.
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto quick metadata
> >> +
> >> +# Import common functions.
> >> +. ./common/filter
> >> +. ./common/attr
> >> +. ./common/inject
> >> +. ./common/populate
> >> +
> >> +# real QA test starts here
> >> +_supported_fs xfs
> >> +_require_scratch
> >> +_require_scratch_xfs_nrext64
> >> +_require_attrs
> >> +_require_xfs_debug
> >> +_require_test_program "punch-alternating"
> >> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> >> +
> >> +_scratch_mkfs -d size=$((512 * 1024 * 1024)) >> $seqres.full
> >> +_scratch_mount >> $seqres.full
> >> +
> >> +bsize=$(_get_file_block_size $SCRATCH_MNT)
> >> +
> >> +testfile=$SCRATCH_MNT/testfile
> >> +
> >> +nr_blks=20
> >> +
> >> +echo "Add blocks to file's data fork"
> >> +$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
> >> +	     >> $seqres.full
> >> +$here/src/punch-alternating $testfile
> >> +
> >> +echo "Consume free space"
> >> +fillerdir=$SCRATCH_MNT/fillerdir
> >> +nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> >> +nr_free_blks=$((nr_free_blks * 90 / 100))
> >> +
> >> +_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
> >> +	 >> $seqres.full 2>&1
> >> +
> >> +echo "Create fragmented filesystem"
> >> +for dentry in $(ls -1 $fillerdir/); do
> >> +	$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
> >> +done
> >> +
> >> +echo "Inject bmap_alloc_minlen_extent error tag"
> >> +_scratch_inject_error bmap_alloc_minlen_extent 1
> >> +
> >> +echo "Add blocks to file's attr fork"
> >> +nr_blks=10
> >> +attr_len=255
> >> +nr_attrs=$((nr_blks * bsize / attr_len))
> >> +for i in $(seq 1 $nr_attrs); do
> >> +	attr="$(printf "trusted.%0247d" $i)"
> >> +	$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
> >> +	[[ $? != 0 ]] && break
> >> +done
> >> +
> >> +testino=$(stat -c '%i' $testfile)
> >> +
> >> +echo "Unmount filesystem"
> >> +_scratch_unmount >> $seqres.full
> >> +
> >> +orig_dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> >> +orig_acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
> >> +
> >> +echo "Upgrade filesystem to support large extent counters"
> >> +_scratch_xfs_admin -O nrext64=1 >> $seqres.full 2>&1
> >> +if [[ $? != 0 ]]; then
> >> +	_notrun "Filesystem geometry is not suitable for upgrading"
> >> +fi
> >> +
> >> +
> >> +echo "Mount filesystem"
> >> +_scratch_mount >> $seqres.full
> >> +
> >> +echo "Modify inode core"
> >> +touch $testfile
> >> +
> >> +echo "Unmount filesystem"
> >> +_scratch_unmount >> $seqres.full
> >> +
> >> +dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> >> +acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
> >> +
> >> +echo "Verify inode extent counter values after fs upgrade"
> >
> > Is there a scenario where the inode counters would become corrupt after
> > enabling the superblock feature bit?  IIRC repair doesn't rewrite the
> > inodes during the upgrade... so is this test merely being cautious?  Or
> > is this covering a failure you found somewhere while writing the feature?
> >
> 
> I was just being cautious w.r.t "Large extent counters" functionality working
> correctly. I used this test during my development to make sure that I was able
> to capture failures before I ran the entire xfstests suite.

Fair enough,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> 
> -- 
> chandan

Re: [PATCH 2/4] common/xfs: Add helper to check if nrext64 option is supported

[Dave Chinner]

On Mon, Jun 06, 2022 at 06:10:59PM +0530, Chandan Babu R wrote:
> This commit adds a new helper to allow tests to check if xfsprogs and xfs
> kernel module support nrext64 option.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  common/xfs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 2123a4ab..dca7af57 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1328,3 +1328,16 @@ _xfs_filter_mkfs()
>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>  	}'
>  }
> +
> +_require_scratch_xfs_nrext64()
> +{
> +	_require_scratch

Not needed - caller should be doing that first.

> +
> +	_scratch_mkfs -i nrext64=1 &>/dev/null || \
> +		_notrun "mkfs.xfs doesn't support nrext64 feature"

_scratch_mkfs_xfs_supported -i nrext64=1

see for example:

_require_xfs_mkfs_crc

> +	_try_scratch_mount || \
> +		_notrun "kernel doesn't support xfs nrext64 feature"
> +	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "nrext64=1" || \
> +		_notrun "nrext64 feature not advertised on mount?"

This seems unnecessary - if mkfs supports the feature bit, and the
kernel supports is, this should just work. We don't do checks like
this in any other feature bit test. e.g:

_require_xfs_finobt
_require_xfs_sparse_inodes

etc.

Also, you should put this in the same region of the file as all the
other feature checks, not right down the bottom by itself.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] xfs/270: Fix ro mount failure when nrext64 option is enabled

[Dave Chinner]

On Mon, Jun 06, 2022 at 06:10:58PM +0530, Chandan Babu R wrote:
> With nrext64 option enabled at run time, the read-only mount performed by the
> test fails because,
> 1. mkfs.xfs would have calculated log size based on reflink being enabled.
> 2. Clearing the reflink ro compat bit causes log size calculations to yield a
>    different value.
> 3. In the case where nrext64 is enabled, this causes attr reservation to be
>    the largest among all the transaction reservations.
> 4. This ends up causing XFS to require a larger ondisk log size than that
>    which is available.
> 
> This commit fixes the problem by setting features_ro_compat to the value
> obtained by the bitwise-OR of features_ro_compat field with 2^31.
> 
> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> ---
>  tests/xfs/270     | 16 ++++++++++++++--
>  tests/xfs/270.out |  1 -
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/xfs/270 b/tests/xfs/270
> index 0ab0c7d8..f3796691 100755
> --- a/tests/xfs/270
> +++ b/tests/xfs/270
> @@ -27,8 +27,20 @@ _scratch_mkfs_xfs >>$seqres.full 2>&1
>  # set the highest bit of features_ro_compat, use it as an unknown
>  # feature bit. If one day this bit become known feature, please
>  # change this case.
> -_scratch_xfs_set_metadata_field "features_ro_compat" "$((2**31))" "sb 0" | \
> -	grep 'features_ro_compat'
> +ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
> +ro_compat=${ro_compat##0x}
> +ro_compat="16#"${ro_compat}
> +ro_compat=$(($ro_compat|16#80000000))
> +ro_compat=$(_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" \
> +					    "sb 0" | grep 'features_ro_compat')
> +
> +ro_compat=${ro_compat##features_ro_compat = 0x}
> +ro_compat="16#"${ro_compat}
> +ro_compat=$(($ro_compat&16#80000000))
> +if (( $ro_compat != 16#80000000 )); then
> +	echo "Unable to set most significant bit of features_ro_compat"
> +fi

Urk. Bash - the new line noise generator. :(

This is basically just bit manipulation in hex format. db accepts
hex format integers (i.e. 0x1234), and according to the bash man
page, it understands the 0x1234 prefix as well. So AFAICT there's no
need for this weird "16#" prefix for the bit operations.

But regardless of that, just because you can do something in bash
doesn't mean you should:

wit://utcc.utoronto.ca/~cks/space/blog/programming/ShellScriptsBeClearFirst

IMO, this reads much better as something like:

# grab the current ro compat fields and add an invalid high bit.
ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
		awk '/features_ro_compat/ {
			printf("0x%x\n", or(strtonum($3), 0x80000000)
		}')

# write the new ro compat field to the superblock
_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0"

# read the newly set ro compat filed for verification
new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
		awk '/features_ro_compat/ {
			printf("0x%x\n", $3)
		}')

# verify the new ro_compat field is correct.
if [ $new_ro_compat != $ro_compat ]; then
	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
fi

Yes, it's more lines of code, but it's easy to read, easy to
understand, and easy to modify in future.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Zorro Lang]

On Tue, Jun 07, 2022 at 03:06:58PM +0530, Chandan Babu R wrote:
> On Mon, Jun 06, 2022 at 08:40:47 AM -0700, Darrick J. Wong wrote:
> > On Mon, Jun 06, 2022 at 06:11:00PM +0530, Chandan Babu R wrote:
> >> This commit adds a new test to verify if the correct inode extent counter
> >> fields are updated with/without nrext64 mkfs option.
> >> 
> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
> >> ---
> >>  tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/xfs/547.out | 13 +++++++
> >>  2 files changed, 104 insertions(+)
> >>  create mode 100755 tests/xfs/547
> >>  create mode 100644 tests/xfs/547.out
> >> 
> >> diff --git a/tests/xfs/547 b/tests/xfs/547
> >> new file mode 100755
> >> index 00000000..d5137ca7
> >> --- /dev/null
> >> +++ b/tests/xfs/547
> >> @@ -0,0 +1,91 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
> >> +#
> >> +# FS QA Test 547
> >> +#
> >> +# Verify that correct inode extent count fields are populated with and without
> >> +# nrext64 feature.
> >> +#
> >> +. ./common/preamble
> >> +_begin_fstest auto quick metadata
> >> +
> >> +# Import common functions.
> >> +. ./common/filter
> >> +. ./common/attr
> >> +. ./common/inject
> >> +. ./common/populate
> >> +
> >> +# real QA test starts here
> >> +_supported_fs xfs
> >> +_require_scratch
> >> +_require_scratch_xfs_nrext64
> >> +_require_attrs
> >> +_require_xfs_debug
> >> +_require_test_program "punch-alternating"
> >> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
> >> +
> >> +for nrext64 in 0 1; do
> >> +	echo "* Verify extent counter fields with nrext64=${nrext64} option"
> >> +
> >> +	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
> >> +		      >> $seqres.full
> >> +	_scratch_mount >> $seqres.full
> >> +
> >> +	bsize=$(_get_file_block_size $SCRATCH_MNT)
> >> +
> >> +	testfile=$SCRATCH_MNT/testfile
> >> +
> >> +	nr_blks=20
> >> +
> >> +	echo "Add blocks to test file's data fork"
> >> +	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
> >> +		     >> $seqres.full
> >> +	$here/src/punch-alternating $testfile
> >> +
> >> +	echo "Consume free space"
> >> +	fillerdir=$SCRATCH_MNT/fillerdir
> >> +	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
> >> +	nr_free_blks=$((nr_free_blks * 90 / 100))
> >> +
> >> +	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
> >> +		 >> $seqres.full 2>&1
> >> +
> >> +	echo "Create fragmented filesystem"
> >> +	for dentry in $(ls -1 $fillerdir/); do
> >> +		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
> >> +	done
> >> +
> >> +	echo "Inject bmap_alloc_minlen_extent error tag"
> >> +	_scratch_inject_error bmap_alloc_minlen_extent 1
> >> +
> >> +	echo "Add blocks to test file's attr fork"
> >> +	attr_len=255
> >> +	nr_attrs=$((nr_blks * bsize / attr_len))
> >> +	for i in $(seq 1 $nr_attrs); do
> >> +		attr="$(printf "trusted.%0247d" $i)"
> >> +		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
> >> +		[[ $? != 0 ]] && break
> >> +	done
> >> +
> >> +	testino=$(stat -c '%i' $testfile)
> >> +
> >> +	_scratch_unmount >> $seqres.full
> >> +
> >> +	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
> >> +	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
> >
> > Note: For any test requiring functionality added after 5.10, you can use
> > the xfs_db path command to avoid this sort of inode number saving:
> >
> > dcnt=$(_scratch_xfs_get_metadata_field core.nextents "path /testfile")
> >
> 
> Ok. I will post a v2 of the patchset to include the above suggestion.

_require_xfs_db_command path ?

Looks like the 'path' command is a new command will be in linux and xfsprogs
5.10.

It's not always recommended to use latest features/tools, that depends what does
this case test for. If this case is only for a bug/feature in 5.10, then the
'path' command is fine. If it's a common test case for old and new kernels, then
this new command isn't recommended, that will cause this test can't be run on
old system.

BTW, you'd better to not use a fixed case number in the patch subject, if the
patch is a new case. Due to the number might be changed when we merge it. And
a fixed case number in subject, might cause others feel this's a known case
update, not a new case.

Thanks,
Zorro

> 
> > Up to you if you want to change the test to do that; otherwise,
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> >
> 
> Thanks for the review.
> 
> -- 
> chandan
> 

Re: [PATCH 2/4] common/xfs: Add helper to check if nrext64 option is supported

[Chandan Babu R]

On Wed, Jun 08, 2022 at 09:01:50 AM +1000, Dave Chinner wrote:
> On Mon, Jun 06, 2022 at 06:10:59PM +0530, Chandan Babu R wrote:
>> This commit adds a new helper to allow tests to check if xfsprogs and xfs
>> kernel module support nrext64 option.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  common/xfs | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>> 
>> diff --git a/common/xfs b/common/xfs
>> index 2123a4ab..dca7af57 100644
>> --- a/common/xfs
>> +++ b/common/xfs
>> @@ -1328,3 +1328,16 @@ _xfs_filter_mkfs()
>>  		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
>>  	}'
>>  }
>> +
>> +_require_scratch_xfs_nrext64()
>> +{
>> +	_require_scratch
>
> Not needed - caller should be doing that first.
>
>> +
>> +	_scratch_mkfs -i nrext64=1 &>/dev/null || \
>> +		_notrun "mkfs.xfs doesn't support nrext64 feature"
>
> _scratch_mkfs_xfs_supported -i nrext64=1
>
> see for example:
>
> _require_xfs_mkfs_crc
>
>> +	_try_scratch_mount || \
>> +		_notrun "kernel doesn't support xfs nrext64 feature"
>> +	$XFS_INFO_PROG "$SCRATCH_MNT" | grep -q -w "nrext64=1" || \
>> +		_notrun "nrext64 feature not advertised on mount?"
>
> This seems unnecessary - if mkfs supports the feature bit, and the
> kernel supports is, this should just work. We don't do checks like
> this in any other feature bit test. e.g:
>
> _require_xfs_finobt
> _require_xfs_sparse_inodes
>
> etc.
>
> Also, you should put this in the same region of the file as all the
> other feature checks, not right down the bottom by itself.
>

I agree. I will make the relevant changes and post a V2.

-- 
chandan

Re: [PATCH 1/4] xfs/270: Fix ro mount failure when nrext64 option is enabled

[Chandan Babu R]

On Wed, Jun 08, 2022 at 09:51:33 AM +1000, Dave Chinner wrote:
> On Mon, Jun 06, 2022 at 06:10:58PM +0530, Chandan Babu R wrote:
>> With nrext64 option enabled at run time, the read-only mount performed by the
>> test fails because,
>> 1. mkfs.xfs would have calculated log size based on reflink being enabled.
>> 2. Clearing the reflink ro compat bit causes log size calculations to yield a
>>    different value.
>> 3. In the case where nrext64 is enabled, this causes attr reservation to be
>>    the largest among all the transaction reservations.
>> 4. This ends up causing XFS to require a larger ondisk log size than that
>>    which is available.
>> 
>> This commit fixes the problem by setting features_ro_compat to the value
>> obtained by the bitwise-OR of features_ro_compat field with 2^31.
>> 
>> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> ---
>>  tests/xfs/270     | 16 ++++++++++++++--
>>  tests/xfs/270.out |  1 -
>>  2 files changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/tests/xfs/270 b/tests/xfs/270
>> index 0ab0c7d8..f3796691 100755
>> --- a/tests/xfs/270
>> +++ b/tests/xfs/270
>> @@ -27,8 +27,20 @@ _scratch_mkfs_xfs >>$seqres.full 2>&1
>>  # set the highest bit of features_ro_compat, use it as an unknown
>>  # feature bit. If one day this bit become known feature, please
>>  # change this case.
>> -_scratch_xfs_set_metadata_field "features_ro_compat" "$((2**31))" "sb 0" | \
>> -	grep 'features_ro_compat'
>> +ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
>> +ro_compat=${ro_compat##0x}
>> +ro_compat="16#"${ro_compat}
>> +ro_compat=$(($ro_compat|16#80000000))
>> +ro_compat=$(_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" \
>> +					    "sb 0" | grep 'features_ro_compat')
>> +
>> +ro_compat=${ro_compat##features_ro_compat = 0x}
>> +ro_compat="16#"${ro_compat}
>> +ro_compat=$(($ro_compat&16#80000000))
>> +if (( $ro_compat != 16#80000000 )); then
>> +	echo "Unable to set most significant bit of features_ro_compat"
>> +fi
>
> Urk. Bash - the new line noise generator. :(
>
> This is basically just bit manipulation in hex format. db accepts
> hex format integers (i.e. 0x1234), and according to the bash man
> page, it understands the 0x1234 prefix as well. So AFAICT there's no
> need for this weird "16#" prefix for the bit operations.
>
> But regardless of that, just because you can do something in bash
> doesn't mean you should:
>
> wit://utcc.utoronto.ca/~cks/space/blog/programming/ShellScriptsBeClearFirst
>
> IMO, this reads much better as something like:
>
> # grab the current ro compat fields and add an invalid high bit.
> ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
> 		awk '/features_ro_compat/ {
> 			printf("0x%x\n", or(strtonum($3), 0x80000000)
> 		}')
>
> # write the new ro compat field to the superblock
> _scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0"
>
> # read the newly set ro compat filed for verification
> new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" | \
> 		awk '/features_ro_compat/ {
> 			printf("0x%x\n", $3)
> 		}')
>
> # verify the new ro_compat field is correct.
> if [ $new_ro_compat != $ro_compat ]; then
> 	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
> fi
>
> Yes, it's more lines of code, but it's easy to read, easy to
> understand, and easy to modify in future.
>

Thanks for the review. I will ensure to resort to weird bashisms unless there
are no alternate options available.

-- 
chandan

Re: [PATCH 3/4] xfs/547: Verify that the correct inode extent counters are updated with/without nrext64

[Chandan Babu R]

On Wed, Jun 08, 2022 at 11:59:33 AM +0800, Zorro Lang wrote:
> On Tue, Jun 07, 2022 at 03:06:58PM +0530, Chandan Babu R wrote:
>> On Mon, Jun 06, 2022 at 08:40:47 AM -0700, Darrick J. Wong wrote:
>> > On Mon, Jun 06, 2022 at 06:11:00PM +0530, Chandan Babu R wrote:
>> >> This commit adds a new test to verify if the correct inode extent counter
>> >> fields are updated with/without nrext64 mkfs option.
>> >> 
>> >> Signed-off-by: Chandan Babu R <chandan.babu@oracle.com>
>> >> ---
>> >>  tests/xfs/547     | 91 +++++++++++++++++++++++++++++++++++++++++++++++
>> >>  tests/xfs/547.out | 13 +++++++
>> >>  2 files changed, 104 insertions(+)
>> >>  create mode 100755 tests/xfs/547
>> >>  create mode 100644 tests/xfs/547.out
>> >> 
>> >> diff --git a/tests/xfs/547 b/tests/xfs/547
>> >> new file mode 100755
>> >> index 00000000..d5137ca7
>> >> --- /dev/null
>> >> +++ b/tests/xfs/547
>> >> @@ -0,0 +1,91 @@
>> >> +#! /bin/bash
>> >> +# SPDX-License-Identifier: GPL-2.0
>> >> +# Copyright (c) 2022 Oracle.  All Rights Reserved.
>> >> +#
>> >> +# FS QA Test 547
>> >> +#
>> >> +# Verify that correct inode extent count fields are populated with and without
>> >> +# nrext64 feature.
>> >> +#
>> >> +. ./common/preamble
>> >> +_begin_fstest auto quick metadata
>> >> +
>> >> +# Import common functions.
>> >> +. ./common/filter
>> >> +. ./common/attr
>> >> +. ./common/inject
>> >> +. ./common/populate
>> >> +
>> >> +# real QA test starts here
>> >> +_supported_fs xfs
>> >> +_require_scratch
>> >> +_require_scratch_xfs_nrext64
>> >> +_require_attrs
>> >> +_require_xfs_debug
>> >> +_require_test_program "punch-alternating"
>> >> +_require_xfs_io_error_injection "bmap_alloc_minlen_extent"
>> >> +
>> >> +for nrext64 in 0 1; do
>> >> +	echo "* Verify extent counter fields with nrext64=${nrext64} option"
>> >> +
>> >> +	_scratch_mkfs -i nrext64=${nrext64} -d size=$((512 * 1024 * 1024)) \
>> >> +		      >> $seqres.full
>> >> +	_scratch_mount >> $seqres.full
>> >> +
>> >> +	bsize=$(_get_file_block_size $SCRATCH_MNT)
>> >> +
>> >> +	testfile=$SCRATCH_MNT/testfile
>> >> +
>> >> +	nr_blks=20
>> >> +
>> >> +	echo "Add blocks to test file's data fork"
>> >> +	$XFS_IO_PROG -f -c "pwrite 0 $((nr_blks * bsize))" $testfile \
>> >> +		     >> $seqres.full
>> >> +	$here/src/punch-alternating $testfile
>> >> +
>> >> +	echo "Consume free space"
>> >> +	fillerdir=$SCRATCH_MNT/fillerdir
>> >> +	nr_free_blks=$(stat -f -c '%f' $SCRATCH_MNT)
>> >> +	nr_free_blks=$((nr_free_blks * 90 / 100))
>> >> +
>> >> +	_fill_fs $((bsize * nr_free_blks)) $fillerdir $bsize 0 \
>> >> +		 >> $seqres.full 2>&1
>> >> +
>> >> +	echo "Create fragmented filesystem"
>> >> +	for dentry in $(ls -1 $fillerdir/); do
>> >> +		$here/src/punch-alternating $fillerdir/$dentry >> $seqres.full
>> >> +	done
>> >> +
>> >> +	echo "Inject bmap_alloc_minlen_extent error tag"
>> >> +	_scratch_inject_error bmap_alloc_minlen_extent 1
>> >> +
>> >> +	echo "Add blocks to test file's attr fork"
>> >> +	attr_len=255
>> >> +	nr_attrs=$((nr_blks * bsize / attr_len))
>> >> +	for i in $(seq 1 $nr_attrs); do
>> >> +		attr="$(printf "trusted.%0247d" $i)"
>> >> +		$SETFATTR_PROG -n "$attr" $testfile >> $seqres.full 2>&1
>> >> +		[[ $? != 0 ]] && break
>> >> +	done
>> >> +
>> >> +	testino=$(stat -c '%i' $testfile)
>> >> +
>> >> +	_scratch_unmount >> $seqres.full
>> >> +
>> >> +	dcnt=$(_scratch_xfs_get_metadata_field core.nextents "inode $testino")
>> >> +	acnt=$(_scratch_xfs_get_metadata_field core.naextents "inode $testino")
>> >
>> > Note: For any test requiring functionality added after 5.10, you can use
>> > the xfs_db path command to avoid this sort of inode number saving:
>> >
>> > dcnt=$(_scratch_xfs_get_metadata_field core.nextents "path /testfile")
>> >
>> 
>> Ok. I will post a v2 of the patchset to include the above suggestion.
>
> _require_xfs_db_command path ?
>

I think I should include the above line in the script to make explicit to the
reader the requirements to run the test.

> Looks like the 'path' command is a new command will be in linux and xfsprogs
> 5.10.
>
> It's not always recommended to use latest features/tools, that depends what does
> this case test for. If this case is only for a bug/feature in 5.10, then the
> 'path' command is fine. If it's a common test case for old and new kernels, then
> this new command isn't recommended, that will cause this test can't be run on
> old system.

Large extent counters will be introduced in Linux v5.19. Hence I think it is
safe to assume that xfs_db's 'path' command will be available in xfsprogs
which supports Large extent counters.

>
> BTW, you'd better to not use a fixed case number in the patch subject, if the
> patch is a new case. Due to the number might be changed when we merge it. And
> a fixed case number in subject, might cause others feel this's a known case
> update, not a new case.

Sorry about that. I will add only the summary of the test in the subject line.

Thanks for the review comments!

-- 
chandan