[PATCH v2 0/3] aborted fstests may leave frozen fs behind

[PATCH v2 0/3] aborted fstests may leave frozen fs behind

[Amir Goldstein]

Zorro,

As discussed on v1, this series fixes cleanup routines of
freeze tests without adding auto-cleanup in check.

I also took a closer look at some test cleanups and handled the
kill of background processes after unfreeze.

I tested that all the tests in the 'freeze' group that I modified
run to completion with no regressions.

I did not try to interrupt all the rest of the tests, because
interrupting a test at exact time that it is frozen is quite hard.

I did test that interrupting xfs/517 several times did not leave the fs
frozen.  Before the changes, it was rather easy to get it to leave a
frozen fs behind, because the freeze_loop() keeps the fs frozen around
half of the time that the test is running.

Thanks,
Amir.

Changes since v1:
- Remove auto-cleanup in check
- Add more kill+wait in cleanups where needed
- Change the cleanup routines of the freeze_loops

Amir Goldstein (3):
  fstests: add missing _require_freeze() to tests
  fstests: unfreeze fs on cleanup routines
  xfs/{422,517}: kill background jobs on test termination

 tests/generic/068 | 14 ++++++++++----
 tests/generic/085 |  3 +++
 tests/generic/280 | 13 ++++++++++++-
 tests/generic/390 |  8 +++++++-
 tests/generic/459 |  2 ++
 tests/generic/491 |  9 +++++++++
 tests/xfs/011     |  3 ++-
 tests/xfs/119     | 10 ++++++++++
 tests/xfs/297     | 11 +++++++++++
 tests/xfs/318     |  6 ++++--
 tests/xfs/325     |  6 ++++--
 tests/xfs/422     | 35 ++++++++++++++++++++++++++++++++---
 tests/xfs/438     |  4 +++-
 tests/xfs/517     | 27 ++++++++++++++++++++++++---
 14 files changed, 133 insertions(+), 18 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] fstests: add missing _require_freeze() to tests

[Amir Goldstein]

And add a few tests that use freeze to the freeze group

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/xfs/119 | 1 +
 tests/xfs/318 | 3 ++-
 tests/xfs/325 | 3 ++-
 tests/xfs/422 | 3 ++-
 tests/xfs/438 | 2 +-
 tests/xfs/517 | 1 +
 6 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/tests/xfs/119 b/tests/xfs/119
index a1180371..b6f96601 100755
--- a/tests/xfs/119
+++ b/tests/xfs/119
@@ -20,6 +20,7 @@ _begin_fstest log v2log auto freeze
 _supported_fs xfs
 
 _require_scratch
+_require_freeze
 
 # this may hang
 sync
diff --git a/tests/xfs/318 b/tests/xfs/318
index 38c7aa60..be93f9ab 100755
--- a/tests/xfs/318
+++ b/tests/xfs/318
@@ -7,7 +7,7 @@
 # Simulate free extent errors with a file write and a file remove.
 #
 . ./common/preamble
-_begin_fstest auto quick rw
+_begin_fstest auto quick rw freeze
 
 # Override the default cleanup function.
 _cleanup()
@@ -26,6 +26,7 @@ _supported_fs xfs
 _require_scratch
 _require_error_injection
 _require_xfs_io_error_injection "rmap_finish_one"
+_require_freeze
 
 blksz=65536
 blks=64
diff --git a/tests/xfs/325 b/tests/xfs/325
index 5b26b2b3..c6861fbc 100755
--- a/tests/xfs/325
+++ b/tests/xfs/325
@@ -8,7 +8,7 @@
 # Inject an error during extent freeing to test log recovery.
 #
 . ./common/preamble
-_begin_fstest auto quick clone
+_begin_fstest auto quick clone freeze
 
 # Override the default cleanup function.
 _cleanup()
@@ -29,6 +29,7 @@ _require_cp_reflink
 _require_scratch_reflink
 _require_error_injection
 _require_xfs_io_error_injection "free_extent"
+_require_freeze
 
 blksz=65536
 blks=30
diff --git a/tests/xfs/422 b/tests/xfs/422
index 175253aa..a83a66df 100755
--- a/tests/xfs/422
+++ b/tests/xfs/422
@@ -9,7 +9,7 @@
 # activity, so we can't have userspace wandering in and thawing it.
 #
 . ./common/preamble
-_begin_fstest dangerous_scrub dangerous_online_repair
+_begin_fstest dangerous_scrub dangerous_online_repair freeze
 
 _register_cleanup "_cleanup" BUS
 
@@ -24,6 +24,7 @@ _require_xfs_scratch_rmapbt
 _require_xfs_io_command "scrub"
 _require_xfs_io_error_injection "force_repair"
 _require_command "$KILLALL_PROG" killall
+_require_freeze
 
 echo "Format and populate"
 _scratch_mkfs > "$seqres.full" 2>&1
diff --git a/tests/xfs/438 b/tests/xfs/438
index c3008b1c..cfe75bd8 100755
--- a/tests/xfs/438
+++ b/tests/xfs/438
@@ -21,7 +21,7 @@
 # Fixed by upstream commit 373b058 ("xfs: Properly retry failed dquot
 # items in case of error during buffer writeback")
 . ./common/preamble
-_begin_fstest auto quick quota
+_begin_fstest auto quick quota freeze
 
 # Override the default cleanup function.
 _cleanup()
diff --git a/tests/xfs/517 b/tests/xfs/517
index 88c4f43b..f7f9a8a2 100755
--- a/tests/xfs/517
+++ b/tests/xfs/517
@@ -29,6 +29,7 @@ _supported_fs xfs
 _require_xfs_scratch_rmapbt
 _require_xfs_io_command "fsmap"
 _require_command "$KILLALL_PROG" killall
+_require_freeze
 
 echo "Format and populate"
 _scratch_mkfs > "$seqres.full" 2>&1
-- 
2.25.1

[PATCH v2 2/3] fstests: unfreeze fs on cleanup routines

[Amir Goldstein]

Many of tests that freeze fs do not make sure that fs is unfrozen on
test termination.

Some tests also need to kill and wait for writer processes after
unfreezing the fs.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/generic/068 | 14 ++++++++++----
 tests/generic/085 |  3 +++
 tests/generic/280 | 13 ++++++++++++-
 tests/generic/390 |  8 +++++++-
 tests/generic/459 |  2 ++
 tests/generic/491 |  9 +++++++++
 tests/xfs/011     |  3 ++-
 tests/xfs/119     |  9 +++++++++
 tests/xfs/297     | 11 +++++++++++
 tests/xfs/318     |  3 ++-
 tests/xfs/325     |  3 ++-
 tests/xfs/438     |  2 ++
 12 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/tests/generic/068 b/tests/generic/068
index 56262cd7..eeddf6d1 100755
--- a/tests/generic/068
+++ b/tests/generic/068
@@ -17,9 +17,12 @@ ITERATIONS=10
 # Override the default cleanup function.
 _cleanup()
 {
-    cd /
-
-    trap 0 1 2 3 15
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
+	wait $pid
+	cd /
+	rm -f $tmp.*
 }
 
 # Import common functions.
@@ -60,6 +63,7 @@ touch $tmp.running
     rm -r $STRESS_DIR/*
     rmdir $STRESS_DIR
 } &
+pid=$!
 
 # start fstest -m loop in a background block; this gets us mmap coverage
 {
@@ -75,6 +79,7 @@ touch $tmp.running
     rm -rf $FSTEST_DIR/*
     rmdir $FSTEST_DIR
 } &
+pid="$pid $!"
 
 i=0
 let ITERATIONS=$ITERATIONS-1
@@ -103,6 +108,7 @@ done
 rm $tmp.running
 
 # wait for fsstresses to finish
-wait
+wait $pid
+unset pid
 
 exit 1
diff --git a/tests/generic/085 b/tests/generic/085
index 20cf875a..786d8e6f 100755
--- a/tests/generic/085
+++ b/tests/generic/085
@@ -25,6 +25,8 @@ cleanup_dmdev()
 {
 	# in case it's still suspended and/or mounted
 	$DMSETUP_PROG resume $lvdev >/dev/null 2>&1
+	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
+	wait $pid
 	$UMOUNT_PROG $lvdev >/dev/null 2>&1
 	_dmsetup_remove $node
 }
@@ -75,6 +77,7 @@ done &
 pid="$pid $!"
 
 wait $pid
+unset pid
 
 status=0
 exit
diff --git a/tests/generic/280 b/tests/generic/280
index 07144555..8e1ae4d2 100755
--- a/tests/generic/280
+++ b/tests/generic/280
@@ -10,6 +10,17 @@
 . ./common/preamble
 _begin_fstest auto quota freeze
 
+# Override the default cleanup function.
+_cleanup()
+{
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
+	wait $pid
+	cd /
+	rm -f $tmp.*
+}
+
 # Import common functions.
 . ./common/filter
 . ./common/quota
@@ -34,7 +45,7 @@ pid=$!
 sleep 1
 xfs_freeze -u $SCRATCH_MNT
 wait $pid
-_scratch_unmount
+unset pid
 
 # Failure comes in the form of a deadlock.
 
diff --git a/tests/generic/390 b/tests/generic/390
index 20c66e22..e8f6a5dc 100755
--- a/tests/generic/390
+++ b/tests/generic/390
@@ -14,8 +14,12 @@ _begin_fstest auto freeze stress
 _cleanup()
 {
 	cd /
-	# Make sure $SCRATCH_MNT is unfreezed
+	# Kill freeze loops and make sure $SCRATCH_MNT is unfreezed
+	[ -n "$freeze_pids" ] && kill -9 $freeze_pids 2>/dev/null
+	wait $freeze_pids
 	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	[ -n "$fsstress_pid" ] && kill -9 $fsstress_pid 2>/dev/null
+	wait $fsstress_pid
 	rm -f $tmp.*
 }
 
@@ -62,7 +66,9 @@ done
 
 wait $fsstress_pid
 result=$?
+unset fsstress_pid
 wait $freeze_pids
+unset freeze_pids
 
 # Exit with fsstress return value
 status=$result
diff --git a/tests/generic/459 b/tests/generic/459
index 57d58e55..7be39089 100755
--- a/tests/generic/459
+++ b/tests/generic/459
@@ -24,6 +24,8 @@ _begin_fstest auto freeze thin
 # Override the default cleanup function.
 _cleanup()
 {
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
 	cd /
 	rm -f $tmp.*
 	$UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
diff --git a/tests/generic/491 b/tests/generic/491
index e6e57dcd..797b08d5 100755
--- a/tests/generic/491
+++ b/tests/generic/491
@@ -12,6 +12,15 @@
 . ./common/preamble
 _begin_fstest auto quick freeze mount
 
+# Override the default cleanup function.
+_cleanup()
+{
+	cd /
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	rm -f $tmp.*
+}
+
 # Import common functions.
 . ./common/filter
 
diff --git a/tests/xfs/011 b/tests/xfs/011
index d6e9099e..ed44d074 100755
--- a/tests/xfs/011
+++ b/tests/xfs/011
@@ -16,10 +16,11 @@ _begin_fstest auto freeze log metadata quick
 # Override the default cleanup function.
 _cleanup()
 {
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
 	$KILLALL_PROG -9 fsstress 2>/dev/null
 	wait
 	cd /
-	_scratch_unmount 2>/dev/null
 	rm -f $tmp.*
 }
 
diff --git a/tests/xfs/119 b/tests/xfs/119
index b6f96601..5ffbce25 100755
--- a/tests/xfs/119
+++ b/tests/xfs/119
@@ -11,6 +11,15 @@
 . ./common/preamble
 _begin_fstest log v2log auto freeze
 
+# Override the default cleanup function.
+_cleanup()
+{
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	cd /
+	rm -f $tmp.*
+}
+
 # Import common functions.
 . ./common/filter
 
diff --git a/tests/xfs/297 b/tests/xfs/297
index ca482e06..07f84c25 100755
--- a/tests/xfs/297
+++ b/tests/xfs/297
@@ -11,6 +11,17 @@
 . ./common/preamble
 _begin_fstest auto freeze
 
+# Override the default cleanup function.
+_cleanup()
+{
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
+	$KILLALL_PROG -q -9 $FSSTRESS_PROG
+	wait
+	cd /
+	rm -f $tmp.*
+}
+
 # Import common functions.
 . ./common/filter
 
diff --git a/tests/xfs/318 b/tests/xfs/318
index be93f9ab..5798f9a3 100755
--- a/tests/xfs/318
+++ b/tests/xfs/318
@@ -12,8 +12,9 @@ _begin_fstest auto quick rw freeze
 # Override the default cleanup function.
 _cleanup()
 {
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
 	cd /
-	_scratch_unmount > /dev/null 2>&1
 	rm -rf $tmp.*
 }
 
diff --git a/tests/xfs/325 b/tests/xfs/325
index c6861fbc..43fb09a6 100755
--- a/tests/xfs/325
+++ b/tests/xfs/325
@@ -13,8 +13,9 @@ _begin_fstest auto quick clone freeze
 # Override the default cleanup function.
 _cleanup()
 {
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
 	cd /
-	_scratch_unmount > /dev/null 2>&1
 	rm -rf $tmp.*
 }
 
diff --git a/tests/xfs/438 b/tests/xfs/438
index cfe75bd8..0425c5b1 100755
--- a/tests/xfs/438
+++ b/tests/xfs/438
@@ -26,6 +26,8 @@ _begin_fstest auto quick quota freeze
 # Override the default cleanup function.
 _cleanup()
 {
+	# Make sure $SCRATCH_MNT is unfreezed
+	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
 	[ -z "${interval}" ] || \
 		sysctl -w fs.xfs.xfssyncd_centisecs=${interval} >/dev/null 2>&1
 	cd /
-- 
2.25.1

[PATCH v2 3/3] xfs/{422,517}: kill background jobs on test termination

[Amir Goldstein]

Those tests failed to cleanup background jobs properly after test
is interrupted and even sometimes when it completed succefully.

xfs/517 would sometime fails randomally with this false positive error:
     QA output created by 517
     Format and populate
     Concurrent fsmap and freeze
    +Terminated
     Test done

The tests have several background sub-shells that spawn short lived
programs in a loop.  By killing the spawned programs using killall,
killall could find no process to kill and the sub-shell loop could still
spawn another process that is not going to be killed and in the worst
case, the freeze_loop() could spawn the xfs_io "freeze" command after
test has thawn the fs before exit, which leaves the fs frozen after the
test.

The "Terminated" output is emitted by the sub-shell when killing the
programs that it has spawned when the loop did not finish before test
timeout.  By killing the sub-shell and not the spawned programs, we
avoid the false positive "Terminated" error.

Use a helper to perform this cleanup dance:
First kill and wait the freeze_loop so it won't try to freeze fs again
Then make sure fs is not frozen.
Then kill and wait for the rest of the sub-shells, because
if fs is frozen a killed writer process will never exit.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 tests/xfs/422 | 32 ++++++++++++++++++++++++++++++--
 tests/xfs/517 | 26 +++++++++++++++++++++++---
 2 files changed, 53 insertions(+), 5 deletions(-)

diff --git a/tests/xfs/422 b/tests/xfs/422
index a83a66df..fdbb8bf1 100755
--- a/tests/xfs/422
+++ b/tests/xfs/422
@@ -13,6 +13,32 @@ _begin_fstest dangerous_scrub dangerous_online_repair freeze
 
 _register_cleanup "_cleanup" BUS
 
+# First kill and wait the freeze loop so it won't try to freeze fs again
+# Then make sure fs is not frozen
+# Then kill and wait for the rest of the workers
+# Because if fs is frozen a killed writer will never exit
+kill_loops() {
+	local sig=$1
+
+	[ -n "$freeze_pid" ] && kill $sig $freeze_pid
+	wait $freeze_pid
+	unset freeze_pid
+	$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT
+	[ -n "$stress_pid" ] && kill $sig $stress_pid
+	[ -n "$repair_pid" ] && kill $sig $repair_pid
+	wait
+	unset stress_pid
+	unset repair_pid
+}
+
+# Override the default cleanup function.
+_cleanup()
+{
+	kill_loops -9 > /dev/null 2>&1
+	cd /
+	rm -rf $tmp.*
+}
+
 # Import common functions.
 . ./common/filter
 . ./common/fuzzy
@@ -78,8 +104,11 @@ end=$((start + (30 * TIME_FACTOR) ))
 
 echo "Loop started at $(date --date="@${start}"), ending at $(date --date="@${end}")" >> $seqres.full
 stress_loop $end &
+stress_pid=$!
 freeze_loop $end &
+freeze_pid=$!
 repair_loop $end &
+repair_pid=$!
 
 # Wait until 2 seconds after the loops should have finished...
 while [ "$(date +%s)" -lt $((end + 2)) ]; do
@@ -87,8 +116,7 @@ while [ "$(date +%s)" -lt $((end + 2)) ]; do
 done
 
 # ...and clean up after the loops in case they didn't do it themselves.
-$KILLALL_PROG -TERM xfs_io fsstress >> $seqres.full 2>&1
-$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT >> $seqres.full 2>&1
+kill_loops >> $seqres.full 2>&1
 
 echo "Loop finished at $(date)" >> $seqres.full
 echo "Test done"
diff --git a/tests/xfs/517 b/tests/xfs/517
index f7f9a8a2..6877af13 100755
--- a/tests/xfs/517
+++ b/tests/xfs/517
@@ -11,11 +11,29 @@ _begin_fstest auto quick fsmap freeze
 
 _register_cleanup "_cleanup" BUS
 
+# First kill and wait the freeze loop so it won't try to freeze fs again
+# Then make sure fs is not frozen
+# Then kill and wait for the rest of the workers
+# Because if fs is frozen a killed writer will never exit
+kill_loops() {
+	local sig=$1
+
+	[ -n "$freeze_pid" ] && kill $sig $freeze_pid
+	wait $freeze_pid
+	unset freeze_pid
+	$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT
+	[ -n "$stress_pid" ] && kill $sig $stress_pid
+	[ -n "$fsmap_pid" ] && kill $sig $fsmap_pid
+	wait
+	unset stress_pid
+	unset fsmap_pid
+}
+
 # Override the default cleanup function.
 _cleanup()
 {
+	kill_loops -9 > /dev/null 2>&1
 	cd /
-	$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT > /dev/null 2>&1
 	rm -rf $tmp.*
 }
 
@@ -83,8 +101,11 @@ end=$((start + (30 * TIME_FACTOR) ))
 
 echo "Loop started at $(date --date="@${start}"), ending at $(date --date="@${end}")" >> $seqres.full
 stress_loop $end &
+stress_pid=$!
 freeze_loop $end &
+freeze_pid=$!
 fsmap_loop $end &
+fsmap_pid=$!
 
 # Wait until 2 seconds after the loops should have finished...
 while [ "$(date +%s)" -lt $((end + 2)) ]; do
@@ -92,8 +113,7 @@ while [ "$(date +%s)" -lt $((end + 2)) ]; do
 done
 
 # ...and clean up after the loops in case they didn't do it themselves.
-$KILLALL_PROG -TERM xfs_io fsstress >> $seqres.full 2>&1
-$XFS_IO_PROG -x -c 'thaw' $SCRATCH_MNT >> $seqres.full 2>&1
+kill_loops >> $seqres.full 2>&1
 
 echo "Loop finished at $(date)" >> $seqres.full
 echo "Test done"
-- 
2.25.1

Re: [PATCH v2 1/3] fstests: add missing _require_freeze() to tests

[Zorro Lang]

On Tue, Jun 21, 2022 at 08:37:27PM +0300, Amir Goldstein wrote:
> And add a few tests that use freeze to the freeze group
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Although there were some different points about if "318, 325, 422, 438"
should be in freeze group, I don't mind having them in freeze group personally.
So ...

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/xfs/119 | 1 +
>  tests/xfs/318 | 3 ++-
>  tests/xfs/325 | 3 ++-
>  tests/xfs/422 | 3 ++-
>  tests/xfs/438 | 2 +-
>  tests/xfs/517 | 1 +
>  6 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/xfs/119 b/tests/xfs/119
> index a1180371..b6f96601 100755
> --- a/tests/xfs/119
> +++ b/tests/xfs/119
> @@ -20,6 +20,7 @@ _begin_fstest log v2log auto freeze
>  _supported_fs xfs
>  
>  _require_scratch
> +_require_freeze
>  
>  # this may hang
>  sync
> diff --git a/tests/xfs/318 b/tests/xfs/318
> index 38c7aa60..be93f9ab 100755
> --- a/tests/xfs/318
> +++ b/tests/xfs/318
> @@ -7,7 +7,7 @@
>  # Simulate free extent errors with a file write and a file remove.
>  #
>  . ./common/preamble
> -_begin_fstest auto quick rw
> +_begin_fstest auto quick rw freeze
>  
>  # Override the default cleanup function.
>  _cleanup()
> @@ -26,6 +26,7 @@ _supported_fs xfs
>  _require_scratch
>  _require_error_injection
>  _require_xfs_io_error_injection "rmap_finish_one"
> +_require_freeze
>  
>  blksz=65536
>  blks=64
> diff --git a/tests/xfs/325 b/tests/xfs/325
> index 5b26b2b3..c6861fbc 100755
> --- a/tests/xfs/325
> +++ b/tests/xfs/325
> @@ -8,7 +8,7 @@
>  # Inject an error during extent freeing to test log recovery.
>  #
>  . ./common/preamble
> -_begin_fstest auto quick clone
> +_begin_fstest auto quick clone freeze
>  
>  # Override the default cleanup function.
>  _cleanup()
> @@ -29,6 +29,7 @@ _require_cp_reflink
>  _require_scratch_reflink
>  _require_error_injection
>  _require_xfs_io_error_injection "free_extent"
> +_require_freeze
>  
>  blksz=65536
>  blks=30
> diff --git a/tests/xfs/422 b/tests/xfs/422
> index 175253aa..a83a66df 100755
> --- a/tests/xfs/422
> +++ b/tests/xfs/422
> @@ -9,7 +9,7 @@
>  # activity, so we can't have userspace wandering in and thawing it.
>  #
>  . ./common/preamble
> -_begin_fstest dangerous_scrub dangerous_online_repair
> +_begin_fstest dangerous_scrub dangerous_online_repair freeze
>  
>  _register_cleanup "_cleanup" BUS
>  
> @@ -24,6 +24,7 @@ _require_xfs_scratch_rmapbt
>  _require_xfs_io_command "scrub"
>  _require_xfs_io_error_injection "force_repair"
>  _require_command "$KILLALL_PROG" killall
> +_require_freeze
>  
>  echo "Format and populate"
>  _scratch_mkfs > "$seqres.full" 2>&1
> diff --git a/tests/xfs/438 b/tests/xfs/438
> index c3008b1c..cfe75bd8 100755
> --- a/tests/xfs/438
> +++ b/tests/xfs/438
> @@ -21,7 +21,7 @@
>  # Fixed by upstream commit 373b058 ("xfs: Properly retry failed dquot
>  # items in case of error during buffer writeback")
>  . ./common/preamble
> -_begin_fstest auto quick quota
> +_begin_fstest auto quick quota freeze
>  
>  # Override the default cleanup function.
>  _cleanup()
> diff --git a/tests/xfs/517 b/tests/xfs/517
> index 88c4f43b..f7f9a8a2 100755
> --- a/tests/xfs/517
> +++ b/tests/xfs/517
> @@ -29,6 +29,7 @@ _supported_fs xfs
>  _require_xfs_scratch_rmapbt
>  _require_xfs_io_command "fsmap"
>  _require_command "$KILLALL_PROG" killall
> +_require_freeze
>  
>  echo "Format and populate"
>  _scratch_mkfs > "$seqres.full" 2>&1
> -- 
> 2.25.1
> 

Re: [PATCH v2 2/3] fstests: unfreeze fs on cleanup routines

[Zorro Lang]

On Tue, Jun 21, 2022 at 08:37:28PM +0300, Amir Goldstein wrote:
> Many of tests that freeze fs do not make sure that fs is unfrozen on
> test termination.
> 
> Some tests also need to kill and wait for writer processes after
> unfreezing the fs.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

The changes look good, and I didn't find regression issue from it for now.
Please tell me your cencern if anyone has, or I'll merge it.

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  tests/generic/068 | 14 ++++++++++----
>  tests/generic/085 |  3 +++
>  tests/generic/280 | 13 ++++++++++++-
>  tests/generic/390 |  8 +++++++-
>  tests/generic/459 |  2 ++
>  tests/generic/491 |  9 +++++++++
>  tests/xfs/011     |  3 ++-
>  tests/xfs/119     |  9 +++++++++
>  tests/xfs/297     | 11 +++++++++++
>  tests/xfs/318     |  3 ++-
>  tests/xfs/325     |  3 ++-
>  tests/xfs/438     |  2 ++
>  12 files changed, 71 insertions(+), 9 deletions(-)
> 
> diff --git a/tests/generic/068 b/tests/generic/068
> index 56262cd7..eeddf6d1 100755
> --- a/tests/generic/068
> +++ b/tests/generic/068
> @@ -17,9 +17,12 @@ ITERATIONS=10
>  # Override the default cleanup function.
>  _cleanup()
>  {
> -    cd /
> -
> -    trap 0 1 2 3 15
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
> +	wait $pid
> +	cd /
> +	rm -f $tmp.*
>  }
>  
>  # Import common functions.
> @@ -60,6 +63,7 @@ touch $tmp.running
>      rm -r $STRESS_DIR/*
>      rmdir $STRESS_DIR
>  } &
> +pid=$!
>  
>  # start fstest -m loop in a background block; this gets us mmap coverage
>  {
> @@ -75,6 +79,7 @@ touch $tmp.running
>      rm -rf $FSTEST_DIR/*
>      rmdir $FSTEST_DIR
>  } &
> +pid="$pid $!"
>  
>  i=0
>  let ITERATIONS=$ITERATIONS-1
> @@ -103,6 +108,7 @@ done
>  rm $tmp.running
>  
>  # wait for fsstresses to finish
> -wait
> +wait $pid
> +unset pid
>  
>  exit 1
> diff --git a/tests/generic/085 b/tests/generic/085
> index 20cf875a..786d8e6f 100755
> --- a/tests/generic/085
> +++ b/tests/generic/085
> @@ -25,6 +25,8 @@ cleanup_dmdev()
>  {
>  	# in case it's still suspended and/or mounted
>  	$DMSETUP_PROG resume $lvdev >/dev/null 2>&1
> +	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
> +	wait $pid
>  	$UMOUNT_PROG $lvdev >/dev/null 2>&1
>  	_dmsetup_remove $node
>  }
> @@ -75,6 +77,7 @@ done &
>  pid="$pid $!"
>  
>  wait $pid
> +unset pid
>  
>  status=0
>  exit
> diff --git a/tests/generic/280 b/tests/generic/280
> index 07144555..8e1ae4d2 100755
> --- a/tests/generic/280
> +++ b/tests/generic/280
> @@ -10,6 +10,17 @@
>  . ./common/preamble
>  _begin_fstest auto quota freeze
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	[ -n "$pid" ] && kill -9 $pid 2>/dev/null
> +	wait $pid
> +	cd /
> +	rm -f $tmp.*
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  . ./common/quota
> @@ -34,7 +45,7 @@ pid=$!
>  sleep 1
>  xfs_freeze -u $SCRATCH_MNT
>  wait $pid
> -_scratch_unmount
> +unset pid
>  
>  # Failure comes in the form of a deadlock.
>  
> diff --git a/tests/generic/390 b/tests/generic/390
> index 20c66e22..e8f6a5dc 100755
> --- a/tests/generic/390
> +++ b/tests/generic/390
> @@ -14,8 +14,12 @@ _begin_fstest auto freeze stress
>  _cleanup()
>  {
>  	cd /
> -	# Make sure $SCRATCH_MNT is unfreezed
> +	# Kill freeze loops and make sure $SCRATCH_MNT is unfreezed
> +	[ -n "$freeze_pids" ] && kill -9 $freeze_pids 2>/dev/null
> +	wait $freeze_pids
>  	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	[ -n "$fsstress_pid" ] && kill -9 $fsstress_pid 2>/dev/null
> +	wait $fsstress_pid
>  	rm -f $tmp.*
>  }
>  
> @@ -62,7 +66,9 @@ done
>  
>  wait $fsstress_pid
>  result=$?
> +unset fsstress_pid
>  wait $freeze_pids
> +unset freeze_pids
>  
>  # Exit with fsstress return value
>  status=$result
> diff --git a/tests/generic/459 b/tests/generic/459
> index 57d58e55..7be39089 100755
> --- a/tests/generic/459
> +++ b/tests/generic/459
> @@ -24,6 +24,8 @@ _begin_fstest auto freeze thin
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	cd /
>  	rm -f $tmp.*
>  	$UMOUNT_PROG $SCRATCH_MNT >>$seqres.full 2>&1
> diff --git a/tests/generic/491 b/tests/generic/491
> index e6e57dcd..797b08d5 100755
> --- a/tests/generic/491
> +++ b/tests/generic/491
> @@ -12,6 +12,15 @@
>  . ./common/preamble
>  _begin_fstest auto quick freeze mount
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	cd /
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	rm -f $tmp.*
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  
> diff --git a/tests/xfs/011 b/tests/xfs/011
> index d6e9099e..ed44d074 100755
> --- a/tests/xfs/011
> +++ b/tests/xfs/011
> @@ -16,10 +16,11 @@ _begin_fstest auto freeze log metadata quick
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	$KILLALL_PROG -9 fsstress 2>/dev/null
>  	wait
>  	cd /
> -	_scratch_unmount 2>/dev/null
>  	rm -f $tmp.*
>  }
>  
> diff --git a/tests/xfs/119 b/tests/xfs/119
> index b6f96601..5ffbce25 100755
> --- a/tests/xfs/119
> +++ b/tests/xfs/119
> @@ -11,6 +11,15 @@
>  . ./common/preamble
>  _begin_fstest log v2log auto freeze
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	cd /
> +	rm -f $tmp.*
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  
> diff --git a/tests/xfs/297 b/tests/xfs/297
> index ca482e06..07f84c25 100755
> --- a/tests/xfs/297
> +++ b/tests/xfs/297
> @@ -11,6 +11,17 @@
>  . ./common/preamble
>  _begin_fstest auto freeze
>  
> +# Override the default cleanup function.
> +_cleanup()
> +{
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
> +	$KILLALL_PROG -q -9 $FSSTRESS_PROG
> +	wait
> +	cd /
> +	rm -f $tmp.*
> +}
> +
>  # Import common functions.
>  . ./common/filter
>  
> diff --git a/tests/xfs/318 b/tests/xfs/318
> index be93f9ab..5798f9a3 100755
> --- a/tests/xfs/318
> +++ b/tests/xfs/318
> @@ -12,8 +12,9 @@ _begin_fstest auto quick rw freeze
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	cd /
> -	_scratch_unmount > /dev/null 2>&1
>  	rm -rf $tmp.*
>  }
>  
> diff --git a/tests/xfs/325 b/tests/xfs/325
> index c6861fbc..43fb09a6 100755
> --- a/tests/xfs/325
> +++ b/tests/xfs/325
> @@ -13,8 +13,9 @@ _begin_fstest auto quick clone freeze
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	cd /
> -	_scratch_unmount > /dev/null 2>&1
>  	rm -rf $tmp.*
>  }
>  
> diff --git a/tests/xfs/438 b/tests/xfs/438
> index cfe75bd8..0425c5b1 100755
> --- a/tests/xfs/438
> +++ b/tests/xfs/438
> @@ -26,6 +26,8 @@ _begin_fstest auto quick quota freeze
>  # Override the default cleanup function.
>  _cleanup()
>  {
> +	# Make sure $SCRATCH_MNT is unfreezed
> +	xfs_freeze -u $SCRATCH_MNT 2>/dev/null
>  	[ -z "${interval}" ] || \
>  		sysctl -w fs.xfs.xfssyncd_centisecs=${interval} >/dev/null 2>&1
>  	cd /
> -- 
> 2.25.1
>