[PATCHSET v2 0/6] fstests: random fixes for v2022.09.25

[PATCHSET v2 0/6] fstests: random fixes for v2022.09.25

[Darrick J. Wong]

Hi all,

Here's the usual batch of odd fixes for fstests.

v2: accumulate a few more fixes for problems that I've found.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 common/populate   |    3 +--
 common/xfs        |    3 +++
 tests/generic/092 |    6 ++++++
 tests/xfs/114     |    2 ++
 tests/xfs/128     |   34 ++++++++++++++++++++++++++++++----
 tests/xfs/229     |    7 ++++++-
 6 files changed, 48 insertions(+), 7 deletions(-)

[PATCH 1/6] generic/092: skip test if file allocation unit isn't aligned

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This test exercises allocation behavior when truncating a preallocated
file down to 5M and then up to 7M.  If those two sizes aren't aligned
with the file allocation unit length, then the FIEMAP output will show
blocks beyond EOF.  That will cause trouble with the golden output, so
skip this test if that will be the case.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 tests/generic/092 |    6 ++++++
 1 file changed, 6 insertions(+)


diff --git a/tests/generic/092 b/tests/generic/092
index 505e0ec84f..d7c93ca792 100755
--- a/tests/generic/092
+++ b/tests/generic/092
@@ -28,6 +28,12 @@ _require_test
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "fiemap"
 
+# If the truncation sizes (5M/7M) aren't aligned with the file allocation unit
+# length, then the FIEMAP output will show blocks beyond EOF.  That will cause
+# trouble with the golden output, so skip this test if that will be the case.
+_require_congruent_file_oplen $TEST_DIR $((5 * 1048576))
+_require_congruent_file_oplen $TEST_DIR $((7 * 1048576))
+
 # First test to make sure that truncating at i_size trims the preallocated bit
 # past i_size
 $XFS_IO_PROG -f -c "falloc -k 0 10M" -c "pwrite 0 5M" -c "truncate 5M"\

[PATCH 2/6] xfs/114: fix missing reflink requires

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This test both requires cp --reflink and the scratch filesystem to
support reflink.  Add the missing _requires calls.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 tests/xfs/114 |    2 ++
 1 file changed, 2 insertions(+)


diff --git a/tests/xfs/114 b/tests/xfs/114
index 3aec814a5d..858dc3998e 100755
--- a/tests/xfs/114
+++ b/tests/xfs/114
@@ -18,6 +18,8 @@ _begin_fstest auto quick clone rmap collapse insert
 # real QA test starts here
 _supported_fs xfs
 _require_test_program "punch-alternating"
+_require_cp_reflink
+_require_scratch_reflink
 _require_xfs_scratch_rmapbt
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "fcollapse"

[PATCH 3/6] xfs/229: do not _xfs_force_bdev on TEST_DIR

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In commit ea15099b71, I observed that this test tries to test the
behavior of the extent size hint on the data device.  If the test runner
set up MKFS_OPTIONS such that the filesystem gets created with a
realtime section and rtinherit set on the root directory, then the
preconditions of this test (creating files on the data section) is not
satisfied and the results of this test are incorrect.  The solution was
to force all files created by this test to be assigned to the data
section.

Unfortunately, the correction that I made has side effects beyond this
test -- by clearing rtinherit on $TEST_DIR, all tests that run after
this one will create files on the data section, because the test
filesystem persists for the duration of the entire test run.  This leads
to the wrong things being tested.

Fix this new problem by clearing the rtinherit flag on $TDIR, which
contains the files created by this test and is removed during cleanup,
and leave a few comments celebrating our new discoveries.

Fixes: ea15099b71 ("xfs: force file creation to the data device for certain layout tests")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Zorro Lang <zlang@redhat.com>
---
 common/xfs    |    3 +++
 tests/xfs/229 |    7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)


diff --git a/common/xfs b/common/xfs
index 170dd621a1..e1c15d3d04 100644
--- a/common/xfs
+++ b/common/xfs
@@ -201,6 +201,9 @@ _xfs_get_file_block_size()
 # For each directory, each file subsequently created will target the given
 # device for file data allocations.  For each empty regular file, each
 # subsequent file data allocation will be on the given device.
+#
+# NOTE: If you call this on $TEST_DIR, you must reset the rtinherit flag state
+# before the end of the test to avoid polluting subsequent tests.
 _xfs_force_bdev()
 {
 	local device="$1"
diff --git a/tests/xfs/229 b/tests/xfs/229
index 2221b9c49c..a58fd16bba 100755
--- a/tests/xfs/229
+++ b/tests/xfs/229
@@ -31,11 +31,16 @@ _require_fs_space $TEST_DIR 3200000
 TDIR="${TEST_DIR}/t_holes"
 NFILES="10"
 EXTSIZE="256k"
-_xfs_force_bdev data $TEST_DIR
 
 # Create the test directory
 mkdir ${TDIR}
 
+# Per-directory extent size hints aren't particularly useful for files that
+# are created on the realtime section.  Force the test file to be created on
+# the data directory.  Do not change the rtinherit flag on $TEST_DIR because
+# that will affect other tests.
+_xfs_force_bdev data $TDIR
+
 # Set the test directory extsize
 $XFS_IO_PROG -c "extsize ${EXTSIZE}" ${TDIR}
 

[PATCH 4/6] xfs/128: try to force file allocation behavior

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Over the years, I've noticed that this test occasionally fails when I've
programmed the allocator to hand out the minimum amount of space with
each allocation or if extent size hints are enabled:

--- /tmp/fstests/tests/xfs/128.out      2022-09-01 15:09:11.506679341 -0700
+++ /var/tmp/fstests/xfs/128.out.bad    2022-10-04 17:32:50.992000000 -0700
@@ -20,7 +21,9 @@
 56ed2f712c91e035adeeb26ed105a982  SCRATCH_MNT/test-128/file3
 b81534f439aac5c34ce3ed60a03eba70  SCRATCH_MNT/test-128/file4
 Check files
 free blocks after creating some reflink copies is in range
 free blocks after CoW some reflink copies is in range
-free blocks after defragging all reflink copies is in range
-free blocks after all tests is in range
+free blocks after defragging all reflink copies has value of 8620027
+free blocks after defragging all reflink copies is NOT in range 8651819 .. 8652139
+free blocks after all tests has value of 8620027
+free blocks after all tests is NOT in range 8651867 .. 8652187

It turns out that under the right circumstances, the _pwrite_byte at the
start of this test will end up allocating two extents to file1.  This
almost never happens when delalloc is enabled or when the extent size is
large, and is more prone to happening if the extent size is > 1FSB but
small, the allocator hands out small allocations, or if writeback shoots
down pages in random order.

When file1 gets more than 1 extent, problems start to happen.  The free
space accounting checks at the end of the test assume that file1 and
file4 still share the same space at the end of the test.  This
definitely happens if file1 gets one extent (since fsr ignores
single-extent files), but if there's more than 1, fsr will try to
defragment it.  If fsr succeeds in copying the file contents to a temp
file with fewer extents than the source file, it will switch the
contents, but unsharing the contents in the process.  This cause the
free space to be lower than expected, and the test fails.

Resolve this situation by preallocating space beforehand to try to set
up file1 with a single space extent.  If the test fails and we got more
than one extent, note that in the output.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/128 |   34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)


diff --git a/tests/xfs/128 b/tests/xfs/128
index db5d9a60db..2d2975115e 100755
--- a/tests/xfs/128
+++ b/tests/xfs/128
@@ -34,7 +34,20 @@ margin=160
 blksz=65536
 real_blksz="$(_get_block_size $testdir)"
 blksz_factor=$((blksz / real_blksz))
+
+# The expected free space numbers in this test require file1 and file4 to share
+# the same blocks at the end of the test.  Therefore, we need the allocator to
+# give file1 a single extent at the start of the test so that fsr will not be
+# tempted to "defragment" a multi-extent file1 or file4.  Defragmenting really
+# means rewriting the file, and if that succeeds on either file, we'll have
+# unshared the space and there will be too little free space.  Therefore,
+# preallocate space to try to produce a single extent.
+$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $testdir/file1 >> $seqres.full
 _pwrite_byte 0x61 0 $((blks * blksz)) $testdir/file1 >> $seqres.full
+sync
+
+nextents=$($XFS_IO_PROG -c 'stat' $testdir/file1 | grep 'fsxattr.nextents' | awk '{print $3}')
+
 _cp_reflink $testdir/file1 $testdir/file2
 _cp_reflink $testdir/file2 $testdir/file3
 _cp_reflink $testdir/file3 $testdir/file4
@@ -106,10 +119,23 @@ test $c14 = $c24 || echo "File4 changed by defrag"
 
 #echo $free_blocks0 $free_blocks1 $free_blocks2 $free_blocks3
 
-_within_tolerance "free blocks after creating some reflink copies" $free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v
-_within_tolerance "free blocks after CoW some reflink copies" $free_blocks2 $((free_blocks1 - 2)) $margin -v
-_within_tolerance "free blocks after defragging all reflink copies" $free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v
-_within_tolerance "free blocks after all tests" $free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v
+freesp_bad=0
+
+_within_tolerance "free blocks after creating some reflink copies" \
+	$free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v || freesp_bad=1
+
+_within_tolerance "free blocks after CoW some reflink copies" \
+	$free_blocks2 $((free_blocks1 - 2)) $margin -v || freesp_bad=1
+
+_within_tolerance "free blocks after defragging all reflink copies" \
+	$free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v || freesp_bad=1
+
+_within_tolerance "free blocks after all tests" \
+	$free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v || freesp_bad=1
+
+if [ $freesp_bad -ne 0 ] && [ $nextents -gt 0 ]; then
+	echo "free space checks probably failed because file1 nextents was $nextents"
+fi
 
 # success, all done
 status=0

[PATCH 5/6] common/populate: don't metadump xfs filesystems twice

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Due to some braino on my part, _scratch_populate_cached will metadump
the filesystem twice -- once with compression disabled, and again with
it enabled, maybe.  Get rid of the first metadump.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    1 -
 1 file changed, 1 deletion(-)


diff --git a/common/populate b/common/populate
index 9739ac99e0..4eee7e8c66 100644
--- a/common/populate
+++ b/common/populate
@@ -890,7 +890,6 @@ _scratch_populate_cached() {
 	"xfs")
 		_scratch_xfs_populate $@
 		_scratch_xfs_populate_check
-		_scratch_xfs_metadump "${POPULATE_METADUMP}"
 
 		local logdev=
 		[ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \

[PATCH 6/6] common/populate: fix _xfs_metadump usage in _scratch_populate_cached

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

_xfs_metadump requires that the caller pass in "none" for the log device
if it doesn't have a log device, so fix this call site.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 common/populate |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/common/populate b/common/populate
index 4eee7e8c66..cfdaf766f0 100644
--- a/common/populate
+++ b/common/populate
@@ -891,7 +891,7 @@ _scratch_populate_cached() {
 		_scratch_xfs_populate $@
 		_scratch_xfs_populate_check
 
-		local logdev=
+		local logdev=none
 		[ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
 			logdev=$SCRATCH_LOGDEV
 

Re: [PATCH 4/6] xfs/128: try to force file allocation behavior

[Zorro Lang]

On Wed, Oct 05, 2022 at 03:30:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Over the years, I've noticed that this test occasionally fails when I've
> programmed the allocator to hand out the minimum amount of space with
> each allocation or if extent size hints are enabled:
> 
> --- /tmp/fstests/tests/xfs/128.out      2022-09-01 15:09:11.506679341 -0700
> +++ /var/tmp/fstests/xfs/128.out.bad    2022-10-04 17:32:50.992000000 -0700
> @@ -20,7 +21,9 @@
>  56ed2f712c91e035adeeb26ed105a982  SCRATCH_MNT/test-128/file3
>  b81534f439aac5c34ce3ed60a03eba70  SCRATCH_MNT/test-128/file4
>  Check files
>  free blocks after creating some reflink copies is in range
>  free blocks after CoW some reflink copies is in range
> -free blocks after defragging all reflink copies is in range
> -free blocks after all tests is in range
> +free blocks after defragging all reflink copies has value of 8620027
> +free blocks after defragging all reflink copies is NOT in range 8651819 .. 8652139
> +free blocks after all tests has value of 8620027
> +free blocks after all tests is NOT in range 8651867 .. 8652187
> 
> It turns out that under the right circumstances, the _pwrite_byte at the
> start of this test will end up allocating two extents to file1.  This
> almost never happens when delalloc is enabled or when the extent size is
> large, and is more prone to happening if the extent size is > 1FSB but
> small, the allocator hands out small allocations, or if writeback shoots
> down pages in random order.
> 
> When file1 gets more than 1 extent, problems start to happen.  The free
> space accounting checks at the end of the test assume that file1 and
> file4 still share the same space at the end of the test.  This
> definitely happens if file1 gets one extent (since fsr ignores
> single-extent files), but if there's more than 1, fsr will try to
> defragment it.  If fsr succeeds in copying the file contents to a temp
> file with fewer extents than the source file, it will switch the
> contents, but unsharing the contents in the process.  This cause the
> free space to be lower than expected, and the test fails.
> 
> Resolve this situation by preallocating space beforehand to try to set
> up file1 with a single space extent.  If the test fails and we got more
> than one extent, note that in the output.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/xfs/128 |   34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/tests/xfs/128 b/tests/xfs/128
> index db5d9a60db..2d2975115e 100755
> --- a/tests/xfs/128
> +++ b/tests/xfs/128
> @@ -34,7 +34,20 @@ margin=160
>  blksz=65536
>  real_blksz="$(_get_block_size $testdir)"
>  blksz_factor=$((blksz / real_blksz))
> +
> +# The expected free space numbers in this test require file1 and file4 to share
> +# the same blocks at the end of the test.  Therefore, we need the allocator to
> +# give file1 a single extent at the start of the test so that fsr will not be
> +# tempted to "defragment" a multi-extent file1 or file4.  Defragmenting really
> +# means rewriting the file, and if that succeeds on either file, we'll have
> +# unshared the space and there will be too little free space.  Therefore,
> +# preallocate space to try to produce a single extent.
> +$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $testdir/file1 >> $seqres.full
>  _pwrite_byte 0x61 0 $((blks * blksz)) $testdir/file1 >> $seqres.full
> +sync
> +
> +nextents=$($XFS_IO_PROG -c 'stat' $testdir/file1 | grep 'fsxattr.nextents' | awk '{print $3}')
> +
>  _cp_reflink $testdir/file1 $testdir/file2
>  _cp_reflink $testdir/file2 $testdir/file3
>  _cp_reflink $testdir/file3 $testdir/file4
> @@ -106,10 +119,23 @@ test $c14 = $c24 || echo "File4 changed by defrag"
>  
>  #echo $free_blocks0 $free_blocks1 $free_blocks2 $free_blocks3
>  
> -_within_tolerance "free blocks after creating some reflink copies" $free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v
> -_within_tolerance "free blocks after CoW some reflink copies" $free_blocks2 $((free_blocks1 - 2)) $margin -v
> -_within_tolerance "free blocks after defragging all reflink copies" $free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v
> -_within_tolerance "free blocks after all tests" $free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v
> +freesp_bad=0
> +
> +_within_tolerance "free blocks after creating some reflink copies" \
> +	$free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v || freesp_bad=1
> +
> +_within_tolerance "free blocks after CoW some reflink copies" \
> +	$free_blocks2 $((free_blocks1 - 2)) $margin -v || freesp_bad=1
> +
> +_within_tolerance "free blocks after defragging all reflink copies" \
> +	$free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v || freesp_bad=1
> +
> +_within_tolerance "free blocks after all tests" \
> +	$free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v || freesp_bad=1
> +
> +if [ $freesp_bad -ne 0 ] && [ $nextents -gt 0 ]; then
> +	echo "free space checks probably failed because file1 nextents was $nextents"
> +fi
>  
>  # success, all done
>  status=0
> 

Re: [PATCH 5/6] common/populate: don't metadump xfs filesystems twice

[Zorro Lang]

On Wed, Oct 05, 2022 at 03:31:01PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Due to some braino on my part, _scratch_populate_cached will metadump
> the filesystem twice -- once with compression disabled, and again with
> it enabled, maybe.  Get rid of the first metadump.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/populate |    1 -
>  1 file changed, 1 deletion(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 9739ac99e0..4eee7e8c66 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -890,7 +890,6 @@ _scratch_populate_cached() {
>  	"xfs")
>  		_scratch_xfs_populate $@
>  		_scratch_xfs_populate_check
> -		_scratch_xfs_metadump "${POPULATE_METADUMP}"
>  
>  		local logdev=
>  		[ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
> 

Re: [PATCH 6/6] common/populate: fix _xfs_metadump usage in _scratch_populate_cached

[Zorro Lang]

On Wed, Oct 05, 2022 at 03:31:06PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> _xfs_metadump requires that the caller pass in "none" for the log device
> if it doesn't have a log device, so fix this call site.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

Good to me,
Reviewed-by: Zorro Lang <zlang@redhat.com>

>  common/populate |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/common/populate b/common/populate
> index 4eee7e8c66..cfdaf766f0 100644
> --- a/common/populate
> +++ b/common/populate
> @@ -891,7 +891,7 @@ _scratch_populate_cached() {
>  		_scratch_xfs_populate $@
>  		_scratch_xfs_populate_check
>  
> -		local logdev=
> +		local logdev=none
>  		[ "$USE_EXTERNAL" = yes -a ! -z "$SCRATCH_LOGDEV" ] && \
>  			logdev=$SCRATCH_LOGDEV
>  
> 

Re: [PATCH 4/6] xfs/128: try to force file allocation behavior

[Darrick J. Wong]

On Sat, Oct 08, 2022 at 07:11:02PM +0800, Zorro Lang wrote:
> On Wed, Oct 05, 2022 at 03:30:55PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Over the years, I've noticed that this test occasionally fails when I've
> > programmed the allocator to hand out the minimum amount of space with
> > each allocation or if extent size hints are enabled:
> > 
> > --- /tmp/fstests/tests/xfs/128.out      2022-09-01 15:09:11.506679341 -0700
> > +++ /var/tmp/fstests/xfs/128.out.bad    2022-10-04 17:32:50.992000000 -0700
> > @@ -20,7 +21,9 @@
> >  56ed2f712c91e035adeeb26ed105a982  SCRATCH_MNT/test-128/file3
> >  b81534f439aac5c34ce3ed60a03eba70  SCRATCH_MNT/test-128/file4
> >  Check files
> >  free blocks after creating some reflink copies is in range
> >  free blocks after CoW some reflink copies is in range
> > -free blocks after defragging all reflink copies is in range
> > -free blocks after all tests is in range
> > +free blocks after defragging all reflink copies has value of 8620027
> > +free blocks after defragging all reflink copies is NOT in range 8651819 .. 8652139
> > +free blocks after all tests has value of 8620027
> > +free blocks after all tests is NOT in range 8651867 .. 8652187
> > 
> > It turns out that under the right circumstances, the _pwrite_byte at the
> > start of this test will end up allocating two extents to file1.  This
> > almost never happens when delalloc is enabled or when the extent size is
> > large, and is more prone to happening if the extent size is > 1FSB but
> > small, the allocator hands out small allocations, or if writeback shoots
> > down pages in random order.
> > 
> > When file1 gets more than 1 extent, problems start to happen.  The free
> > space accounting checks at the end of the test assume that file1 and
> > file4 still share the same space at the end of the test.  This
> > definitely happens if file1 gets one extent (since fsr ignores
> > single-extent files), but if there's more than 1, fsr will try to
> > defragment it.  If fsr succeeds in copying the file contents to a temp
> > file with fewer extents than the source file, it will switch the
> > contents, but unsharing the contents in the process.  This cause the
> > free space to be lower than expected, and the test fails.
> > 
> > Resolve this situation by preallocating space beforehand to try to set
> > up file1 with a single space extent.  If the test fails and we got more
> > than one extent, note that in the output.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> Good to me,
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 
> >  tests/xfs/128 |   34 ++++++++++++++++++++++++++++++----
> >  1 file changed, 30 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/tests/xfs/128 b/tests/xfs/128
> > index db5d9a60db..2d2975115e 100755
> > --- a/tests/xfs/128
> > +++ b/tests/xfs/128
> > @@ -34,7 +34,20 @@ margin=160
> >  blksz=65536
> >  real_blksz="$(_get_block_size $testdir)"
> >  blksz_factor=$((blksz / real_blksz))
> > +
> > +# The expected free space numbers in this test require file1 and file4 to share
> > +# the same blocks at the end of the test.  Therefore, we need the allocator to
> > +# give file1 a single extent at the start of the test so that fsr will not be
> > +# tempted to "defragment" a multi-extent file1 or file4.  Defragmenting really
> > +# means rewriting the file, and if that succeeds on either file, we'll have
> > +# unshared the space and there will be too little free space.  Therefore,
> > +# preallocate space to try to produce a single extent.
> > +$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $testdir/file1 >> $seqres.full
> >  _pwrite_byte 0x61 0 $((blks * blksz)) $testdir/file1 >> $seqres.full
> > +sync
> > +
> > +nextents=$($XFS_IO_PROG -c 'stat' $testdir/file1 | grep 'fsxattr.nextents' | awk '{print $3}')
> > +
> >  _cp_reflink $testdir/file1 $testdir/file2
> >  _cp_reflink $testdir/file2 $testdir/file3
> >  _cp_reflink $testdir/file3 $testdir/file4
> > @@ -106,10 +119,23 @@ test $c14 = $c24 || echo "File4 changed by defrag"
> >  
> >  #echo $free_blocks0 $free_blocks1 $free_blocks2 $free_blocks3
> >  
> > -_within_tolerance "free blocks after creating some reflink copies" $free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v
> > -_within_tolerance "free blocks after CoW some reflink copies" $free_blocks2 $((free_blocks1 - 2)) $margin -v
> > -_within_tolerance "free blocks after defragging all reflink copies" $free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v
> > -_within_tolerance "free blocks after all tests" $free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v
> > +freesp_bad=0
> > +
> > +_within_tolerance "free blocks after creating some reflink copies" \
> > +	$free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v || freesp_bad=1
> > +
> > +_within_tolerance "free blocks after CoW some reflink copies" \
> > +	$free_blocks2 $((free_blocks1 - 2)) $margin -v || freesp_bad=1
> > +
> > +_within_tolerance "free blocks after defragging all reflink copies" \
> > +	$free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v || freesp_bad=1
> > +
> > +_within_tolerance "free blocks after all tests" \
> > +	$free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v || freesp_bad=1
> > +
> > +if [ $freesp_bad -ne 0 ] && [ $nextents -gt 0 ]; then

If you decide to commit this, could you change ^^^ this to -gt 1, please?

--D

> > +	echo "free space checks probably failed because file1 nextents was $nextents"
> > +fi
> >  
> >  # success, all done
> >  status=0
> > 
> 

Re: [PATCH 4/6] xfs/128: try to force file allocation behavior

[Zorro Lang]

On Sat, Oct 08, 2022 at 10:54:55AM -0700, Darrick J. Wong wrote:
> On Sat, Oct 08, 2022 at 07:11:02PM +0800, Zorro Lang wrote:
> > On Wed, Oct 05, 2022 at 03:30:55PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Over the years, I've noticed that this test occasionally fails when I've
> > > programmed the allocator to hand out the minimum amount of space with
> > > each allocation or if extent size hints are enabled:
> > > 
> > > --- /tmp/fstests/tests/xfs/128.out      2022-09-01 15:09:11.506679341 -0700
> > > +++ /var/tmp/fstests/xfs/128.out.bad    2022-10-04 17:32:50.992000000 -0700
> > > @@ -20,7 +21,9 @@
> > >  56ed2f712c91e035adeeb26ed105a982  SCRATCH_MNT/test-128/file3
> > >  b81534f439aac5c34ce3ed60a03eba70  SCRATCH_MNT/test-128/file4
> > >  Check files
> > >  free blocks after creating some reflink copies is in range
> > >  free blocks after CoW some reflink copies is in range
> > > -free blocks after defragging all reflink copies is in range
> > > -free blocks after all tests is in range
> > > +free blocks after defragging all reflink copies has value of 8620027
> > > +free blocks after defragging all reflink copies is NOT in range 8651819 .. 8652139
> > > +free blocks after all tests has value of 8620027
> > > +free blocks after all tests is NOT in range 8651867 .. 8652187
> > > 
> > > It turns out that under the right circumstances, the _pwrite_byte at the
> > > start of this test will end up allocating two extents to file1.  This
> > > almost never happens when delalloc is enabled or when the extent size is
> > > large, and is more prone to happening if the extent size is > 1FSB but
> > > small, the allocator hands out small allocations, or if writeback shoots
> > > down pages in random order.
> > > 
> > > When file1 gets more than 1 extent, problems start to happen.  The free
> > > space accounting checks at the end of the test assume that file1 and
> > > file4 still share the same space at the end of the test.  This
> > > definitely happens if file1 gets one extent (since fsr ignores
> > > single-extent files), but if there's more than 1, fsr will try to
> > > defragment it.  If fsr succeeds in copying the file contents to a temp
> > > file with fewer extents than the source file, it will switch the
> > > contents, but unsharing the contents in the process.  This cause the
> > > free space to be lower than expected, and the test fails.
> > > 
> > > Resolve this situation by preallocating space beforehand to try to set
> > > up file1 with a single space extent.  If the test fails and we got more
> > > than one extent, note that in the output.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > 
> > Good to me,
> > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > 
> > >  tests/xfs/128 |   34 ++++++++++++++++++++++++++++++----
> > >  1 file changed, 30 insertions(+), 4 deletions(-)
> > > 
> > > 
> > > diff --git a/tests/xfs/128 b/tests/xfs/128
> > > index db5d9a60db..2d2975115e 100755
> > > --- a/tests/xfs/128
> > > +++ b/tests/xfs/128
> > > @@ -34,7 +34,20 @@ margin=160
> > >  blksz=65536
> > >  real_blksz="$(_get_block_size $testdir)"
> > >  blksz_factor=$((blksz / real_blksz))
> > > +
> > > +# The expected free space numbers in this test require file1 and file4 to share
> > > +# the same blocks at the end of the test.  Therefore, we need the allocator to
> > > +# give file1 a single extent at the start of the test so that fsr will not be
> > > +# tempted to "defragment" a multi-extent file1 or file4.  Defragmenting really
> > > +# means rewriting the file, and if that succeeds on either file, we'll have
> > > +# unshared the space and there will be too little free space.  Therefore,
> > > +# preallocate space to try to produce a single extent.
> > > +$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $testdir/file1 >> $seqres.full
> > >  _pwrite_byte 0x61 0 $((blks * blksz)) $testdir/file1 >> $seqres.full
> > > +sync
> > > +
> > > +nextents=$($XFS_IO_PROG -c 'stat' $testdir/file1 | grep 'fsxattr.nextents' | awk '{print $3}')
> > > +
> > >  _cp_reflink $testdir/file1 $testdir/file2
> > >  _cp_reflink $testdir/file2 $testdir/file3
> > >  _cp_reflink $testdir/file3 $testdir/file4
> > > @@ -106,10 +119,23 @@ test $c14 = $c24 || echo "File4 changed by defrag"
> > >  
> > >  #echo $free_blocks0 $free_blocks1 $free_blocks2 $free_blocks3
> > >  
> > > -_within_tolerance "free blocks after creating some reflink copies" $free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v
> > > -_within_tolerance "free blocks after CoW some reflink copies" $free_blocks2 $((free_blocks1 - 2)) $margin -v
> > > -_within_tolerance "free blocks after defragging all reflink copies" $free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v
> > > -_within_tolerance "free blocks after all tests" $free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v
> > > +freesp_bad=0
> > > +
> > > +_within_tolerance "free blocks after creating some reflink copies" \
> > > +	$free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v || freesp_bad=1
> > > +
> > > +_within_tolerance "free blocks after CoW some reflink copies" \
> > > +	$free_blocks2 $((free_blocks1 - 2)) $margin -v || freesp_bad=1
> > > +
> > > +_within_tolerance "free blocks after defragging all reflink copies" \
> > > +	$free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v || freesp_bad=1
> > > +
> > > +_within_tolerance "free blocks after all tests" \
> > > +	$free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v || freesp_bad=1
> > > +
> > > +if [ $freesp_bad -ne 0 ] && [ $nextents -gt 0 ]; then
> 
> If you decide to commit this, could you change ^^^ this to -gt 1, please?

Sure, done :)

> 
> --D
> 
> > > +	echo "free space checks probably failed because file1 nextents was $nextents"
> > > +fi
> > >  
> > >  # success, all done
> > >  status=0
> > > 
> > 
> 

Re: [PATCH 4/6] xfs/128: try to force file allocation behavior

[Darrick J. Wong]

On Sun, Oct 09, 2022 at 10:19:39AM +0800, Zorro Lang wrote:
> On Sat, Oct 08, 2022 at 10:54:55AM -0700, Darrick J. Wong wrote:
> > On Sat, Oct 08, 2022 at 07:11:02PM +0800, Zorro Lang wrote:
> > > On Wed, Oct 05, 2022 at 03:30:55PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Over the years, I've noticed that this test occasionally fails when I've
> > > > programmed the allocator to hand out the minimum amount of space with
> > > > each allocation or if extent size hints are enabled:
> > > > 
> > > > --- /tmp/fstests/tests/xfs/128.out      2022-09-01 15:09:11.506679341 -0700
> > > > +++ /var/tmp/fstests/xfs/128.out.bad    2022-10-04 17:32:50.992000000 -0700
> > > > @@ -20,7 +21,9 @@
> > > >  56ed2f712c91e035adeeb26ed105a982  SCRATCH_MNT/test-128/file3
> > > >  b81534f439aac5c34ce3ed60a03eba70  SCRATCH_MNT/test-128/file4
> > > >  Check files
> > > >  free blocks after creating some reflink copies is in range
> > > >  free blocks after CoW some reflink copies is in range
> > > > -free blocks after defragging all reflink copies is in range
> > > > -free blocks after all tests is in range
> > > > +free blocks after defragging all reflink copies has value of 8620027
> > > > +free blocks after defragging all reflink copies is NOT in range 8651819 .. 8652139
> > > > +free blocks after all tests has value of 8620027
> > > > +free blocks after all tests is NOT in range 8651867 .. 8652187
> > > > 
> > > > It turns out that under the right circumstances, the _pwrite_byte at the
> > > > start of this test will end up allocating two extents to file1.  This
> > > > almost never happens when delalloc is enabled or when the extent size is
> > > > large, and is more prone to happening if the extent size is > 1FSB but
> > > > small, the allocator hands out small allocations, or if writeback shoots
> > > > down pages in random order.
> > > > 
> > > > When file1 gets more than 1 extent, problems start to happen.  The free
> > > > space accounting checks at the end of the test assume that file1 and
> > > > file4 still share the same space at the end of the test.  This
> > > > definitely happens if file1 gets one extent (since fsr ignores
> > > > single-extent files), but if there's more than 1, fsr will try to
> > > > defragment it.  If fsr succeeds in copying the file contents to a temp
> > > > file with fewer extents than the source file, it will switch the
> > > > contents, but unsharing the contents in the process.  This cause the
> > > > free space to be lower than expected, and the test fails.
> > > > 
> > > > Resolve this situation by preallocating space beforehand to try to set
> > > > up file1 with a single space extent.  If the test fails and we got more
> > > > than one extent, note that in the output.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > 
> > > Good to me,
> > > Reviewed-by: Zorro Lang <zlang@redhat.com>
> > > 
> > > >  tests/xfs/128 |   34 ++++++++++++++++++++++++++++++----
> > > >  1 file changed, 30 insertions(+), 4 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/tests/xfs/128 b/tests/xfs/128
> > > > index db5d9a60db..2d2975115e 100755
> > > > --- a/tests/xfs/128
> > > > +++ b/tests/xfs/128
> > > > @@ -34,7 +34,20 @@ margin=160
> > > >  blksz=65536
> > > >  real_blksz="$(_get_block_size $testdir)"
> > > >  blksz_factor=$((blksz / real_blksz))
> > > > +
> > > > +# The expected free space numbers in this test require file1 and file4 to share
> > > > +# the same blocks at the end of the test.  Therefore, we need the allocator to
> > > > +# give file1 a single extent at the start of the test so that fsr will not be
> > > > +# tempted to "defragment" a multi-extent file1 or file4.  Defragmenting really
> > > > +# means rewriting the file, and if that succeeds on either file, we'll have
> > > > +# unshared the space and there will be too little free space.  Therefore,
> > > > +# preallocate space to try to produce a single extent.
> > > > +$XFS_IO_PROG -f -c "falloc 0 $((blks * blksz))" $testdir/file1 >> $seqres.full
> > > >  _pwrite_byte 0x61 0 $((blks * blksz)) $testdir/file1 >> $seqres.full
> > > > +sync
> > > > +
> > > > +nextents=$($XFS_IO_PROG -c 'stat' $testdir/file1 | grep 'fsxattr.nextents' | awk '{print $3}')
> > > > +
> > > >  _cp_reflink $testdir/file1 $testdir/file2
> > > >  _cp_reflink $testdir/file2 $testdir/file3
> > > >  _cp_reflink $testdir/file3 $testdir/file4
> > > > @@ -106,10 +119,23 @@ test $c14 = $c24 || echo "File4 changed by defrag"
> > > >  
> > > >  #echo $free_blocks0 $free_blocks1 $free_blocks2 $free_blocks3
> > > >  
> > > > -_within_tolerance "free blocks after creating some reflink copies" $free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v
> > > > -_within_tolerance "free blocks after CoW some reflink copies" $free_blocks2 $((free_blocks1 - 2)) $margin -v
> > > > -_within_tolerance "free blocks after defragging all reflink copies" $free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v
> > > > -_within_tolerance "free blocks after all tests" $free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v
> > > > +freesp_bad=0
> > > > +
> > > > +_within_tolerance "free blocks after creating some reflink copies" \
> > > > +	$free_blocks1 $((free_blocks0 - (blks * blksz_factor) )) $margin -v || freesp_bad=1
> > > > +
> > > > +_within_tolerance "free blocks after CoW some reflink copies" \
> > > > +	$free_blocks2 $((free_blocks1 - 2)) $margin -v || freesp_bad=1
> > > > +
> > > > +_within_tolerance "free blocks after defragging all reflink copies" \
> > > > +	$free_blocks3 $((free_blocks2 - (blks * 2 * blksz_factor))) $margin -v || freesp_bad=1
> > > > +
> > > > +_within_tolerance "free blocks after all tests" \
> > > > +	$free_blocks3 $((free_blocks0 - (blks * 3 * blksz_factor))) $margin -v || freesp_bad=1
> > > > +
> > > > +if [ $freesp_bad -ne 0 ] && [ $nextents -gt 0 ]; then
> > 
> > If you decide to commit this, could you change ^^^ this to -gt 1, please?
> 
> Sure, done :)

Cool, thank you!

--D

> > 
> > --D
> > 
> > > > +	echo "free space checks probably failed because file1 nextents was $nextents"
> > > > +fi
> > > >  
> > > >  # success, all done
> > > >  status=0
> > > > 
> > > 
> > 
>