* [PATCH 01/23] generic/476: fix fsstress process management
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
@ 2025-01-16 23:25 ` Darrick J. Wong
2025-01-21 3:03 ` Dave Chinner
2025-01-16 23:25 ` [PATCH 02/23] metadump: make non-local function variables more obvious Darrick J. Wong
` (21 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:25 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
generic/476 never used to run fsstress in the background, but
8973af00ec212f made it do that. This is incorrect, because now 476 runs
for three seconds (i.e. long enough to fall out the bottom of the test
and end up in _cleanup), ignoring any SOAK_DURATION/TIME_FACTOR
settings.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
tests/generic/476 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/generic/476 b/tests/generic/476
index 769b3745f75689..4537fcd77d2f07 100755
--- a/tests/generic/476
+++ b/tests/generic/476
@@ -22,7 +22,7 @@ nr_ops=$((25000 * nr_cpus * TIME_FACTOR))
fsstress_args=(-w -d $SCRATCH_MNT -n $nr_ops -p $nr_cpus)
test -n "$SOAK_DURATION" && fsstress_args+=(--duration="$SOAK_DURATION")
-_run_fsstress_bg "${fsstress_args[@]}"
+_run_fsstress "${fsstress_args[@]}"
# success, all done
status=0
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 01/23] generic/476: fix fsstress process management
2025-01-16 23:25 ` [PATCH 01/23] generic/476: fix fsstress process management Darrick J. Wong
@ 2025-01-21 3:03 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:25:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> generic/476 never used to run fsstress in the background, but
> 8973af00ec212f made it do that. This is incorrect, because now 476 runs
> for three seconds (i.e. long enough to fall out the bottom of the test
> and end up in _cleanup), ignoring any SOAK_DURATION/TIME_FACTOR
> settings.
>
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> tests/generic/476 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 02/23] metadump: make non-local function variables more obvious
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
2025-01-16 23:25 ` [PATCH 01/23] generic/476: fix fsstress process management Darrick J. Wong
@ 2025-01-16 23:25 ` Darrick J. Wong
2025-01-21 3:06 ` Dave Chinner
2025-01-16 23:25 ` [PATCH 03/23] metadump: fix cleanup for v1 metadump testing Darrick J. Wong
` (20 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:25 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
In _xfs_verify_metadump_v2(), we want to set up some loop devices,
record the names of those loop devices, and then leave the variables in
the global namespace so that _xfs_cleanup_verify_metadump can dispose of
them.
Elsewhere in fstests the convention for global variables is to put them
in all caps to make it obvious that they're global and not local
variables, so do that here too.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: ce79de11337e38 ("fstests: clean up loop device instantiation")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/metadump | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/common/metadump b/common/metadump
index 3aa7adf76da4bb..493f8b6379dc0b 100644
--- a/common/metadump
+++ b/common/metadump
@@ -25,8 +25,8 @@ _xfs_cleanup_verify_metadump()
test -n "$XFS_METADUMP_FILE" && rm -f "$XFS_METADUMP_FILE"
if [ -n "$XFS_METADUMP_IMG" ]; then
- [ -b "$md_data_loop_dev" ] && _destroy_loop_device $md_data_loop_dev
- [ -b "$md_log_loop_dev" ] && _destroy_loop_device $md_log_loop_dev
+ [ -b "$METADUMP_DATA_LOOP_DEV" ] && _destroy_loop_device $METADUMP_DATA_LOOP_DEV
+ [ -b "$METADUMP_LOG_LOOP_DEV" ] && _destroy_loop_device $METADUMP_LOG_LOOP_DEV
for img in "$XFS_METADUMP_IMG"*; do
test -e "$img" && rm -f "$img"
done
@@ -100,9 +100,7 @@ _xfs_verify_metadump_v2()
local metadump_file="$XFS_METADUMP_FILE"
local version="-v 2"
local data_img="$XFS_METADUMP_IMG.data"
- local data_loop
local log_img=""
- local log_loop
# Capture metadump, which creates metadump_file
_scratch_xfs_metadump $metadump_file $metadump_args $version
@@ -118,27 +116,27 @@ _xfs_verify_metadump_v2()
_scratch_xfs_mdrestore $metadump_file
# Create loopdev for data device so we can mount the fs
- md_data_loop_dev=$(_create_loop_device $data_img)
+ METADUMP_DATA_LOOP_DEV=$(_create_loop_device $data_img)
# Create loopdev for log device if we recovered anything
- test -s "$log_img" && md_log_loop_dev=$(_create_loop_device $log_img)
+ test -s "$log_img" && METADUMP_LOG_LOOP_DEV=$(_create_loop_device $log_img)
# Mount fs, run an extra test, fsck, and unmount
- SCRATCH_DEV=$md_data_loop_dev SCRATCH_LOGDEV=$md_log_loop_dev _scratch_mount
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV SCRATCH_LOGDEV=$METADUMP_LOG_LOOP_DEV _scratch_mount
if [ -n "$extra_test" ]; then
- SCRATCH_DEV=$md_data_loop_dev SCRATCH_LOGDEV=$md_log_loop_dev $extra_test
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV SCRATCH_LOGDEV=$METADUMP_LOG_LOOP_DEV $extra_test
fi
- SCRATCH_DEV=$md_data_loop_dev SCRATCH_LOGDEV=$md_log_loop_dev _check_xfs_scratch_fs
- _unmount $md_data_loop_dev
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV SCRATCH_LOGDEV=$METADUMP_LOG_LOOP_DEV _check_xfs_scratch_fs
+ _unmount $METADUMP_DATA_LOOP_DEV
# Tear down what we created
- if [ -b "$md_log_loop_dev" ]; then
- _destroy_loop_device $md_log_loop_dev
- unset md_log_loop_dev
+ if [ -b "$METADUMP_LOG_LOOP_DEV" ]; then
+ _destroy_loop_device $METADUMP_LOG_LOOP_DEV
+ unset METADUMP_LOG_LOOP_DEV
rm -f $log_img
fi
- _destroy_loop_device $md_data_loop_dev
- unset md_data_loop_dev
+ _destroy_loop_device $METADUMP_DATA_LOOP_DEV
+ unset METADUMP_DATA_LOOP_DEV
rm -f $data_img
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 02/23] metadump: make non-local function variables more obvious
2025-01-16 23:25 ` [PATCH 02/23] metadump: make non-local function variables more obvious Darrick J. Wong
@ 2025-01-21 3:06 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:06 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:25:42PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In _xfs_verify_metadump_v2(), we want to set up some loop devices,
> record the names of those loop devices, and then leave the variables in
> the global namespace so that _xfs_cleanup_verify_metadump can dispose of
> them.
>
> Elsewhere in fstests the convention for global variables is to put them
> in all caps to make it obvious that they're global and not local
> variables, so do that here too.
I did this because they are local to the file and nothing external
accesses them. So I converted them to be the same as all other
file-scope loop device names which are all lower case.
But it really doesn't matter at all, so if that's what you want
then:
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 03/23] metadump: fix cleanup for v1 metadump testing
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
2025-01-16 23:25 ` [PATCH 01/23] generic/476: fix fsstress process management Darrick J. Wong
2025-01-16 23:25 ` [PATCH 02/23] metadump: make non-local function variables more obvious Darrick J. Wong
@ 2025-01-16 23:25 ` Darrick J. Wong
2025-01-21 3:07 ` Dave Chinner
2025-01-16 23:26 ` [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem Darrick J. Wong
` (19 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:25 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
In commit ce79de11337e38, the metadump v2 tests were updated to leave
the names of loop devices in some global variables so that the cleanup
method can find them and remove the loop devices. Inexplicably, the
metadump v1 test function was not upgraded. Do so now.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: ce79de11337e38 ("fstests: clean up loop device instantiation")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/metadump | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/common/metadump b/common/metadump
index 493f8b6379dc0b..a4ec9a7f921acf 100644
--- a/common/metadump
+++ b/common/metadump
@@ -61,7 +61,6 @@ _xfs_verify_metadump_v1()
local metadump_file="$XFS_METADUMP_FILE"
local version=""
local data_img="$XFS_METADUMP_IMG.data"
- local data_loop
# Force v1 if we detect v2 support
if [[ $MAX_XFS_METADUMP_FORMAT > 1 ]]; then
@@ -75,18 +74,19 @@ _xfs_verify_metadump_v1()
SCRATCH_DEV=$data_img _scratch_xfs_mdrestore $metadump_file
# Create loopdev for data device so we can mount the fs
- data_loop=$(_create_loop_device $data_img)
+ METADUMP_DATA_LOOP_DEV=$(_create_loop_device $data_img)
# Mount fs, run an extra test, fsck, and unmount
- SCRATCH_DEV=$data_loop _scratch_mount
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV _scratch_mount
if [ -n "$extra_test" ]; then
- SCRATCH_DEV=$data_loop $extra_test
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV $extra_test
fi
- SCRATCH_DEV=$data_loop _check_xfs_scratch_fs
- SCRATCH_DEV=$data_loop _scratch_unmount
+ SCRATCH_DEV=$METADUMP_DATA_LOOP_DEV _check_xfs_scratch_fs
+ _unmount $METADUMP_DATA_LOOP_DEV
# Tear down what we created
- _destroy_loop_device $data_loop
+ _destroy_loop_device $METADUMP_DATA_LOOP_DEV
+ unset METADUMP_DATA_LOOP_DEV
rm -f $data_img
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 03/23] metadump: fix cleanup for v1 metadump testing
2025-01-16 23:25 ` [PATCH 03/23] metadump: fix cleanup for v1 metadump testing Darrick J. Wong
@ 2025-01-21 3:07 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:07 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:25:57PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> In commit ce79de11337e38, the metadump v2 tests were updated to leave
> the names of loop devices in some global variables so that the cleanup
> method can find them and remove the loop devices. Inexplicably, the
> metadump v1 test function was not upgraded. Do so now.
Probably because the xfsprogs version I was using defaulted to v2
formats and so I didn't notice I hadn't converted the v1 format
code.
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: ce79de11337e38 ("fstests: clean up loop device instantiation")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> common/metadump | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (2 preceding siblings ...)
2025-01-16 23:25 ` [PATCH 03/23] metadump: fix cleanup for v1 metadump testing Darrick J. Wong
@ 2025-01-16 23:26 ` Darrick J. Wong
2025-01-21 3:12 ` Dave Chinner
2025-01-16 23:26 ` [PATCH 05/23] generic/019: don't fail if fio crashes while shutting down Darrick J. Wong
` (18 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:26 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
The test filesystem is now a hard dependency of _run_fsstress because
the latter copies the fsstress binary to a different name on the test
filesystem:
generic/482 - output mismatch (see /var/tmp/fstests/generic/482.out.bad)
--- tests/generic/482.out 2024-02-28 16:20:24.262888854 -0800
+++ /var/tmp/fstests/generic/482.out.bad 2025-01-03 15:00:43.107625116 -0800
@@ -1,2 +1,3 @@
QA output created by 482
+cp: cannot create regular file '/mnt/482.fsstress': Read-only file system
Silence is golden
...
(Run 'diff -u /tmp/fstests/tests/generic/482.out /var/tmp/fstests/generic/482.out.bad' to see the entire diff)
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
tests/generic/482 | 1 -
1 file changed, 1 deletion(-)
diff --git a/tests/generic/482 b/tests/generic/482
index 8c114ee03058c6..0efc026a160040 100755
--- a/tests/generic/482
+++ b/tests/generic/482
@@ -68,7 +68,6 @@ lowspace=$((1024*1024 / 512)) # 1m low space threshold
# Use a thin device to provide deterministic discard behavior. Discards are used
# by the log replay tool for fast zeroing to prevent out-of-order replay issues.
-_test_unmount
_dmthin_init $devsize $devsize $csize $lowspace
_log_writes_init $DMTHIN_VOL_DEV
_log_writes_mkfs >> $seqres.full 2>&1
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem
2025-01-16 23:26 ` [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem Darrick J. Wong
@ 2025-01-21 3:12 ` Dave Chinner
2025-01-22 3:27 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:12 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:26:13PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The test filesystem is now a hard dependency of _run_fsstress because
> the latter copies the fsstress binary to a different name on the test
> filesystem:
>
> generic/482 - output mismatch (see /var/tmp/fstests/generic/482.out.bad)
> --- tests/generic/482.out 2024-02-28 16:20:24.262888854 -0800
> +++ /var/tmp/fstests/generic/482.out.bad 2025-01-03 15:00:43.107625116 -0800
> @@ -1,2 +1,3 @@
> QA output created by 482
> +cp: cannot create regular file '/mnt/482.fsstress': Read-only file system
> Silence is golden
> ...
> (Run 'diff -u /tmp/fstests/tests/generic/482.out /var/tmp/fstests/generic/482.out.bad' to see the entire diff)
Ah, because I hadn't added dm-logwrite support to check-parallel
this test wasn't being run....
However, this patch doesn't need to exist - this dependency is
removed later in the series by using the changes to use a unique
session ID for each test and so the fsstress binary doesn't need to
be rename. The change in this patch is then reverted....
I'd just drop this patch (and the later revert).
-Dave.
>
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> tests/generic/482 | 1 -
> 1 file changed, 1 deletion(-)
>
>
> diff --git a/tests/generic/482 b/tests/generic/482
> index 8c114ee03058c6..0efc026a160040 100755
> --- a/tests/generic/482
> +++ b/tests/generic/482
> @@ -68,7 +68,6 @@ lowspace=$((1024*1024 / 512)) # 1m low space threshold
>
> # Use a thin device to provide deterministic discard behavior. Discards are used
> # by the log replay tool for fast zeroing to prevent out-of-order replay issues.
> -_test_unmount
> _dmthin_init $devsize $devsize $csize $lowspace
> _log_writes_init $DMTHIN_VOL_DEV
> _log_writes_mkfs >> $seqres.full 2>&1
>
>
>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem
2025-01-21 3:12 ` Dave Chinner
@ 2025-01-22 3:27 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:27 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 02:12:05PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:26:13PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > The test filesystem is now a hard dependency of _run_fsstress because
> > the latter copies the fsstress binary to a different name on the test
> > filesystem:
> >
> > generic/482 - output mismatch (see /var/tmp/fstests/generic/482.out.bad)
> > --- tests/generic/482.out 2024-02-28 16:20:24.262888854 -0800
> > +++ /var/tmp/fstests/generic/482.out.bad 2025-01-03 15:00:43.107625116 -0800
> > @@ -1,2 +1,3 @@
> > QA output created by 482
> > +cp: cannot create regular file '/mnt/482.fsstress': Read-only file system
> > Silence is golden
> > ...
> > (Run 'diff -u /tmp/fstests/tests/generic/482.out /var/tmp/fstests/generic/482.out.bad' to see the entire diff)
>
> Ah, because I hadn't added dm-logwrite support to check-parallel
> this test wasn't being run....
>
> However, this patch doesn't need to exist - this dependency is
> removed later in the series by using the changes to use a unique
> session ID for each test and so the fsstress binary doesn't need to
> be rename. The change in this patch is then reverted....
>
> I'd just drop this patch (and the later revert).
Done, thanks for reviewing.
--D
> -Dave.
>
> >
> > Cc: <fstests@vger.kernel.org> # v2024.12.08
> > Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> > tests/generic/482 | 1 -
> > 1 file changed, 1 deletion(-)
> >
> >
> > diff --git a/tests/generic/482 b/tests/generic/482
> > index 8c114ee03058c6..0efc026a160040 100755
> > --- a/tests/generic/482
> > +++ b/tests/generic/482
> > @@ -68,7 +68,6 @@ lowspace=$((1024*1024 / 512)) # 1m low space threshold
> >
> > # Use a thin device to provide deterministic discard behavior. Discards are used
> > # by the log replay tool for fast zeroing to prevent out-of-order replay issues.
> > -_test_unmount
> > _dmthin_init $devsize $devsize $csize $lowspace
> > _log_writes_init $DMTHIN_VOL_DEV
> > _log_writes_mkfs >> $seqres.full 2>&1
> >
> >
> >
>
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 05/23] generic/019: don't fail if fio crashes while shutting down
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (3 preceding siblings ...)
2025-01-16 23:26 ` [PATCH 04/23] generic/482: _run_fsstress needs the test filesystem Darrick J. Wong
@ 2025-01-16 23:26 ` Darrick J. Wong
2025-01-21 3:12 ` Dave Chinner
2025-01-16 23:26 ` [PATCH 06/23] fuzzy: do not set _FSSTRESS_PID when exercising fsx Darrick J. Wong
` (17 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:26 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
My system (Debian 12) has fio 3.33. Once in a while, fio crashes while
shutting down after it receives a SIGBUS on account of the filesystem
going down. This causes the test to fail with:
generic/019 - output mismatch (see /var/tmp/fstests/generic/019.out.bad)
--- tests/generic/019.out 2024-02-28 16:20:24.130889521 -0800
+++ /var/tmp/fstests/generic/019.out.bad 2025-01-03 15:00:35.903564431 -0800
@@ -5,5 +5,6 @@
Start fio..
Force SCRATCH_DEV device failure
+/tmp/fstests/tests/generic/019: line 112: 90841 Segmentation fault $FIO_PROG $fio_config >> $seqres.full 2>&1
Make SCRATCH_DEV device operable again
Disallow global fail_make_request feature
...
(Run 'diff -u /tmp/fstests/tests/generic/019.out /var/tmp/fstests/generic/019.out.bad' to see the entire diff)
because the wait command will dutifully report fatal signals that kill
the fio process. Unfortunately, a core dump shows that we blew up in
some library's exit handler somewhere:
(gdb) where
#0 unlink_chunk (p=p@entry=0x55b31cb9a430, av=0x7f8b4475ec60 <main_arena>) at ./malloc/malloc.c:1628
#1 0x00007f8b446222ff in _int_free (av=0x7f8b4475ec60 <main_arena>, p=0x55b31cb9a430, have_lock=<optimized out>, have_lock@entry=0) at ./malloc/malloc.c:4603
#2 0x00007f8b44624f1f in __GI___libc_free (mem=<optimized out>) at ./malloc/malloc.c:3385
#3 0x00007f8b3a71cf0e in ?? () from /lib/x86_64-linux-gnu/libtasn1.so.6
#4 0x00007f8b4426447c in ?? () from /lib/x86_64-linux-gnu/libgnutls.so.30
#5 0x00007f8b4542212a in _dl_call_fini (closure_map=closure_map@entry=0x7f8b44465620) at ./elf/dl-call_fini.c:43
#6 0x00007f8b4542581e in _dl_fini () at ./elf/dl-fini.c:114
#7 0x00007f8b445ca55d in __run_exit_handlers (status=0, listp=0x7f8b4475e820 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true)
at ./stdlib/exit.c:116
#8 0x00007f8b445ca69a in __GI_exit (status=<optimized out>) at ./stdlib/exit.c:146
#9 0x00007f8b445b3251 in __libc_start_call_main (main=main@entry=0x55b319278e10 <main>, argc=argc@entry=2, argv=argv@entry=0x7ffec6f8b468) at ../sysdeps/nptl/libc_start_call_main.h:74
#10 0x00007f8b445b3305 in __libc_start_main_impl (main=0x55b319278e10 <main>, argc=2, argv=0x7ffec6f8b468, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
stack_end=0x7ffec6f8b458) at ../csu/libc-start.c:360
#11 0x000055b319278ed1 in _start ()
This isn't a filesystem failure, so mask this by shovelling the output
to seqres.full.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
tests/generic/019 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/generic/019 b/tests/generic/019
index bed916b53f98e5..676ff27dab8062 100755
--- a/tests/generic/019
+++ b/tests/generic/019
@@ -109,7 +109,7 @@ _workout()
_fail "failed: still able to perform integrity fsync on $SCRATCH_MNT"
_kill_fsstress
- wait $fio_pid
+ wait $fio_pid &>> $seqres.full # old fio can crash on EIO, ignore segfault reporting
unset fio_pid
# We expect that broken FS still can be umounted
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 05/23] generic/019: don't fail if fio crashes while shutting down
2025-01-16 23:26 ` [PATCH 05/23] generic/019: don't fail if fio crashes while shutting down Darrick J. Wong
@ 2025-01-21 3:12 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:12 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:26:28PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> My system (Debian 12) has fio 3.33. Once in a while, fio crashes while
> shutting down after it receives a SIGBUS on account of the filesystem
> going down. This causes the test to fail with:
>
> generic/019 - output mismatch (see /var/tmp/fstests/generic/019.out.bad)
> --- tests/generic/019.out 2024-02-28 16:20:24.130889521 -0800
> +++ /var/tmp/fstests/generic/019.out.bad 2025-01-03 15:00:35.903564431 -0800
> @@ -5,5 +5,6 @@
>
> Start fio..
> Force SCRATCH_DEV device failure
> +/tmp/fstests/tests/generic/019: line 112: 90841 Segmentation fault $FIO_PROG $fio_config >> $seqres.full 2>&1
> Make SCRATCH_DEV device operable again
> Disallow global fail_make_request feature
> ...
> (Run 'diff -u /tmp/fstests/tests/generic/019.out /var/tmp/fstests/generic/019.out.bad' to see the entire diff)
>
> because the wait command will dutifully report fatal signals that kill
> the fio process. Unfortunately, a core dump shows that we blew up in
> some library's exit handler somewhere:
>
> (gdb) where
> #0 unlink_chunk (p=p@entry=0x55b31cb9a430, av=0x7f8b4475ec60 <main_arena>) at ./malloc/malloc.c:1628
> #1 0x00007f8b446222ff in _int_free (av=0x7f8b4475ec60 <main_arena>, p=0x55b31cb9a430, have_lock=<optimized out>, have_lock@entry=0) at ./malloc/malloc.c:4603
> #2 0x00007f8b44624f1f in __GI___libc_free (mem=<optimized out>) at ./malloc/malloc.c:3385
> #3 0x00007f8b3a71cf0e in ?? () from /lib/x86_64-linux-gnu/libtasn1.so.6
> #4 0x00007f8b4426447c in ?? () from /lib/x86_64-linux-gnu/libgnutls.so.30
> #5 0x00007f8b4542212a in _dl_call_fini (closure_map=closure_map@entry=0x7f8b44465620) at ./elf/dl-call_fini.c:43
> #6 0x00007f8b4542581e in _dl_fini () at ./elf/dl-fini.c:114
> #7 0x00007f8b445ca55d in __run_exit_handlers (status=0, listp=0x7f8b4475e820 <__exit_funcs>, run_list_atexit=run_list_atexit@entry=true, run_dtors=run_dtors@entry=true)
> at ./stdlib/exit.c:116
> #8 0x00007f8b445ca69a in __GI_exit (status=<optimized out>) at ./stdlib/exit.c:146
> #9 0x00007f8b445b3251 in __libc_start_call_main (main=main@entry=0x55b319278e10 <main>, argc=argc@entry=2, argv=argv@entry=0x7ffec6f8b468) at ../sysdeps/nptl/libc_start_call_main.h:74
> #10 0x00007f8b445b3305 in __libc_start_main_impl (main=0x55b319278e10 <main>, argc=2, argv=0x7ffec6f8b468, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>,
> stack_end=0x7ffec6f8b458) at ../csu/libc-start.c:360
> #11 0x000055b319278ed1 in _start ()
>
> This isn't a filesystem failure, so mask this by shovelling the output
> to seqres.full.
looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 06/23] fuzzy: do not set _FSSTRESS_PID when exercising fsx
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (4 preceding siblings ...)
2025-01-16 23:26 ` [PATCH 05/23] generic/019: don't fail if fio crashes while shutting down Darrick J. Wong
@ 2025-01-16 23:26 ` Darrick J. Wong
2025-01-21 3:13 ` Dave Chinner
2025-01-16 23:27 ` [PATCH 07/23] common/rc: create a wrapper for the su command Darrick J. Wong
` (16 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:26 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
If we're not running fsstress as the scrub exerciser, don't set
_FSSTRESS_PID because the _kill_fsstress call in the cleanup function
will think that it has to wait for a nonexistant fsstress process.
This fixes the problem of xfs/565 runtime increasing from 30s to 800s
because it tries to kill a nonexistent "565.fsstress" process and then
waits for the fsx loop control process, which hasn't been sent any
signals.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/fuzzy | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/common/fuzzy b/common/fuzzy
index 534e91dedbbb43..0a2d91542b561e 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -1392,7 +1392,11 @@ _scratch_xfs_stress_scrub() {
"__stress_scrub_${exerciser}_loop" "$end" "$runningfile" \
"$remount_period" "$stress_tgt" &
- _FSSTRESS_PID=$!
+ # The loop is a background process, so _FSSTRESS_PID is set in that
+ # child. Unfortunately, this process doesn't know about it. Therefore
+ # we need to set _FSSTRESS_PID ourselves so that cleanup tries to kill
+ # fsstress.
+ test "${exerciser}" = "fsstress" && _FSSTRESS_PID=$!
if [ -n "$freeze" ]; then
__stress_scrub_freeze_loop "$end" "$runningfile" &
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 06/23] fuzzy: do not set _FSSTRESS_PID when exercising fsx
2025-01-16 23:26 ` [PATCH 06/23] fuzzy: do not set _FSSTRESS_PID when exercising fsx Darrick J. Wong
@ 2025-01-21 3:13 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:13 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:26:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> If we're not running fsstress as the scrub exerciser, don't set
> _FSSTRESS_PID because the _kill_fsstress call in the cleanup function
> will think that it has to wait for a nonexistant fsstress process.
> This fixes the problem of xfs/565 runtime increasing from 30s to 800s
> because it tries to kill a nonexistent "565.fsstress" process and then
> waits for the fsx loop control process, which hasn't been sent any
> signals.
>
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> common/fuzzy | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>
> diff --git a/common/fuzzy b/common/fuzzy
> index 534e91dedbbb43..0a2d91542b561e 100644
> --- a/common/fuzzy
> +++ b/common/fuzzy
> @@ -1392,7 +1392,11 @@ _scratch_xfs_stress_scrub() {
>
> "__stress_scrub_${exerciser}_loop" "$end" "$runningfile" \
> "$remount_period" "$stress_tgt" &
> - _FSSTRESS_PID=$!
> + # The loop is a background process, so _FSSTRESS_PID is set in that
> + # child. Unfortunately, this process doesn't know about it. Therefore
> + # we need to set _FSSTRESS_PID ourselves so that cleanup tries to kill
> + # fsstress.
> + test "${exerciser}" = "fsstress" && _FSSTRESS_PID=$!
Yup, looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 07/23] common/rc: create a wrapper for the su command
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (5 preceding siblings ...)
2025-01-16 23:26 ` [PATCH 06/23] fuzzy: do not set _FSSTRESS_PID when exercising fsx Darrick J. Wong
@ 2025-01-16 23:27 ` Darrick J. Wong
2025-01-16 23:27 ` [PATCH 08/23] common: fix pkill by running test program in a separate session Darrick J. Wong
` (15 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:27 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Create a _su wrapper around the su command so that the next patch can
fix all the pkill isolation code so that this test can only kill
processes started for this test.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/rc | 9 +++++++--
tests/generic/093 | 2 +-
tests/generic/125 | 2 +-
tests/generic/128 | 2 +-
tests/generic/193 | 36 ++++++++++++++++++------------------
tests/generic/230 | 14 +++++++-------
tests/generic/231 | 2 +-
tests/generic/233 | 2 +-
tests/generic/270 | 4 ++--
tests/generic/314 | 2 +-
tests/generic/327 | 2 +-
tests/generic/328 | 4 ++--
tests/generic/355 | 2 +-
tests/generic/453 | 6 +++---
tests/generic/514 | 2 +-
tests/generic/573 | 2 +-
tests/generic/600 | 2 +-
tests/generic/601 | 2 +-
tests/generic/603 | 10 +++++-----
tests/generic/673 | 2 +-
tests/generic/674 | 2 +-
tests/generic/675 | 2 +-
tests/generic/680 | 2 +-
tests/generic/681 | 2 +-
tests/generic/682 | 2 +-
tests/generic/683 | 2 +-
tests/generic/684 | 2 +-
tests/generic/685 | 2 +-
tests/generic/686 | 2 +-
tests/generic/687 | 2 +-
tests/generic/688 | 2 +-
tests/generic/691 | 8 ++++----
tests/generic/721 | 10 +++++-----
tests/generic/726 | 2 +-
tests/generic/727 | 2 +-
tests/xfs/720 | 2 +-
tests/xfs/795 | 2 +-
tests/xfs/803 | 2 +-
38 files changed, 82 insertions(+), 77 deletions(-)
diff --git a/common/rc b/common/rc
index c5421c9454fa62..459be11c11c405 100644
--- a/common/rc
+++ b/common/rc
@@ -2740,6 +2740,11 @@ _require_user_exists()
[ "$?" == "0" ] || _notrun "$user user not defined."
}
+_su()
+{
+ su "$@"
+}
+
# check if a user exists and is able to execute commands.
# Uses 'fsgqa' user as default.
#
@@ -2750,7 +2755,7 @@ _require_user()
qa_user=$1
fi
_require_user_exists $qa_user
- echo /bin/true | su $qa_user
+ echo /bin/true | _su $qa_user
[ "$?" == "0" ] || _notrun "$qa_user cannot execute commands."
}
@@ -2812,7 +2817,7 @@ s,^\s*$,,;
_user_do()
{
- echo $1 | su -s /bin/bash $qa_user 2>&1 | _filter_user_do
+ echo $1 | _su -s /bin/bash $qa_user 2>&1 | _filter_user_do
}
_require_xfs_io_command()
diff --git a/tests/generic/093 b/tests/generic/093
index c4e866da1c18eb..047cc821e0e608 100755
--- a/tests/generic/093
+++ b/tests/generic/093
@@ -62,7 +62,7 @@ chmod ugo+w $TEST_DIR
# don't use $here/src/writemod, as we're running it as a regular user, and
# $here may contain path component that a regular user doesn't have search
# permission
-su $qa_user -c "src/writemod $file" | filefilter
+_su $qa_user -c "src/writemod $file" | filefilter
cat $file
# success, all done
diff --git a/tests/generic/125 b/tests/generic/125
index e2bc5fa125da6e..011a51e8fa21c2 100755
--- a/tests/generic/125
+++ b/tests/generic/125
@@ -32,7 +32,7 @@ chmod a+rw $TESTFILE
# don't use $here/src/ftrunc, as we're running it as a regular user, and $here
# may contain path component that a regular user doesn't have search permission
-su $qa_user -c "./src/ftrunc -f $TESTFILE"
+_su $qa_user -c "./src/ftrunc -f $TESTFILE"
if [ "$?" != "0" ]; then
echo src/ftrunc returned non 0 status!
diff --git a/tests/generic/128 b/tests/generic/128
index f931ca0639687b..5a497cdea7382b 100755
--- a/tests/generic/128
+++ b/tests/generic/128
@@ -26,7 +26,7 @@ cp "$(type -P ls)" $SCRATCH_MNT
chmod 700 $SCRATCH_MNT/nosuid
chmod 4755 $SCRATCH_MNT/ls
-su -s/bin/bash - $qa_user -c "$SCRATCH_MNT/ls $SCRATCH_MNT/nosuid >/dev/null 2>&1"
+_su -s/bin/bash - $qa_user -c "$SCRATCH_MNT/ls $SCRATCH_MNT/nosuid >/dev/null 2>&1"
if [ $? -eq 0 ] ; then
echo "Error: we shouldn't be able to ls the directory"
fi
diff --git a/tests/generic/193 b/tests/generic/193
index d251d3a5c4f8ee..ba557428f77b9c 100755
--- a/tests/generic/193
+++ b/tests/generic/193
@@ -69,17 +69,17 @@ echo
_create_files
echo "user: chown root owned file to qa_user (should fail)"
-su ${qa_user} -c "chown ${qa_user} $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "chown ${qa_user} $test_root" 2>&1 | _filter_files
echo "user: chown root owned file to root (should fail)"
-su ${qa_user} -c "chown root $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "chown root $test_root" 2>&1 | _filter_files
echo "user: chown qa_user owned file to qa_user (should succeed)"
-su ${qa_user} -c "chown ${qa_user} $test_user"
+_su ${qa_user} -c "chown ${qa_user} $test_user"
# this would work without _POSIX_CHOWN_RESTRICTED
echo "user: chown qa_user owned file to root (should fail)"
-su ${qa_user} -c "chown root $test_user" 2>&1 | _filter_files
+_su ${qa_user} -c "chown root $test_user" 2>&1 | _filter_files
_cleanup_files
@@ -93,19 +93,19 @@ echo
_create_files
echo "user: chgrp root owned file to root (should fail)"
-su ${qa_user} -c "chgrp root $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "chgrp root $test_root" 2>&1 | _filter_files
echo "user: chgrp qa_user owned file to root (should fail)"
-su ${qa_user} -c "chgrp root $test_user" 2>&1 | _filter_files
+_su ${qa_user} -c "chgrp root $test_user" 2>&1 | _filter_files
echo "user: chgrp root owned file to qa_user (should fail)"
-su ${qa_user} -c "chgrp ${qa_user} $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "chgrp ${qa_user} $test_root" 2>&1 | _filter_files
echo "user: chgrp qa_user owned file to qa_user (should succeed)"
-su ${qa_user} -c "chgrp ${qa_user} $test_user"
+_su ${qa_user} -c "chgrp ${qa_user} $test_user"
#echo "user: chgrp qa_user owned file to secondary group (should succeed)"
-#su ${qa_user} -c "chgrp ${group2} $test_user"
+#_su ${qa_user} -c "chgrp ${group2} $test_user"
_cleanup_files
@@ -119,10 +119,10 @@ echo
_create_files
echo "user: chmod a+r on qa_user owned file (should succeed)"
-su ${qa_user} -c "chmod a+r $test_user"
+_su ${qa_user} -c "chmod a+r $test_user"
echo "user: chmod a+r on root owned file (should fail)"
-su ${qa_user} -c "chmod a+r $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "chmod a+r $test_root" 2>&1 | _filter_files
#
# Setup a file owned by the qa_user, but with a group ID that
@@ -143,7 +143,7 @@ chown ${qa_user}:root $test_user
chmod g+s $test_user
# and let the qa_user change permission bits
-su ${qa_user} -c "chmod a+w $test_user"
+_su ${qa_user} -c "chmod a+w $test_user"
stat -c '%A' $test_user
#
@@ -220,7 +220,7 @@ echo "with no exec perm"
echo frobnozzle >> $test_user
chmod ug+s $test_user
echo -n "before: "; stat -c '%A' $test_user
-su ${qa_user} -c "echo > $test_user"
+_su ${qa_user} -c "echo > $test_user"
echo -n "after: "; stat -c '%A' $test_user
echo "with user exec perm"
@@ -228,7 +228,7 @@ echo frobnozzle >> $test_user
chmod ug+s $test_user
chmod u+x $test_user
echo -n "before: "; stat -c '%A' $test_user
-su ${qa_user} -c "echo > $test_user"
+_su ${qa_user} -c "echo > $test_user"
echo -n "after: "; stat -c '%A' $test_user
echo "with group exec perm"
@@ -237,7 +237,7 @@ chmod ug+s $test_user
chmod g+x $test_user
chmod u-x $test_user
echo -n "before: "; stat -c '%A' $test_user
-su ${qa_user} -c "echo > $test_user"
+_su ${qa_user} -c "echo > $test_user"
echo -n "after: "; stat -c '%A' $test_user
echo "with user+group exec perm"
@@ -245,7 +245,7 @@ echo frobnozzle >> $test_user
chmod ug+s $test_user
chmod ug+x $test_user
echo -n "before: "; stat -c '%A' $test_user
-su ${qa_user} -c "echo > $test_user"
+_su ${qa_user} -c "echo > $test_user"
echo -n "after: "; stat -c '%A' $test_user
#
@@ -258,10 +258,10 @@ echo
_create_files
echo "user: touch qa_user file (should succeed)"
-su ${qa_user} -c "touch $test_user"
+_su ${qa_user} -c "touch $test_user"
echo "user: touch root file (should fail)"
-su ${qa_user} -c "touch $test_root" 2>&1 | _filter_files
+_su ${qa_user} -c "touch $test_root" 2>&1 | _filter_files
_cleanup_files
diff --git a/tests/generic/230 b/tests/generic/230
index ba95fbe725ad28..18e20f4b2e9439 100755
--- a/tests/generic/230
+++ b/tests/generic/230
@@ -33,13 +33,13 @@ test_enforcement()
echo "--- initiating IO..." >>$seqres.full
# Firstly fit below block soft limit
echo "Write 225 blocks..."
- su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((225 * $BLOCK_SIZE))' -c fsync \
+ _su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((225 * $BLOCK_SIZE))' -c fsync \
$SCRATCH_MNT/file1" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Secondly overcome block soft limit
echo "Rewrite 250 blocks plus 1 byte..."
- su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE + 1))' -c fsync \
+ _su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE + 1))' -c fsync \
$SCRATCH_MNT/file1" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
@@ -49,14 +49,14 @@ test_enforcement()
# scratch device that may leed to grace time exceed.
setquota -$type $qa_user -T $grace $grace $SCRATCH_MNT 2>/dev/null
echo "Write 250 blocks..."
- su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE))' -c fsync \
+ _su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE))' -c fsync \
$SCRATCH_MNT/file2" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Now sleep for grace time and check that softlimit got enforced
sleep $((grace+1))
echo "Write 1 block..."
- su $qa_user -c "$XFS_IO_PROG -c 'truncate 0' -c 'pwrite 0 $BLOCK_SIZE' \
+ _su $qa_user -c "$XFS_IO_PROG -c 'truncate 0' -c 'pwrite 0 $BLOCK_SIZE' \
$SCRATCH_MNT/file2" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
@@ -65,7 +65,7 @@ test_enforcement()
# space reservations on XFS
setquota -$type $qa_user 0 0 3 5 $SCRATCH_MNT
echo "Touch 3+4"
- su $qa_user -c "touch $SCRATCH_MNT/file3 $SCRATCH_MNT/file4" \
+ _su $qa_user -c "touch $SCRATCH_MNT/file3 $SCRATCH_MNT/file4" \
2>&1 >>$seqres.full | _filter_scratch | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Try to exceed inode hardlimit
@@ -74,14 +74,14 @@ test_enforcement()
# scratch device that may leed to grace time exceed.
setquota -$type $qa_user -T $grace $grace $SCRATCH_MNT 2>/dev/null
echo "Touch 5+6"
- su $qa_user -c "touch $SCRATCH_MNT/file5 $SCRATCH_MNT/file6" \
+ _su $qa_user -c "touch $SCRATCH_MNT/file5 $SCRATCH_MNT/file6" \
2>&1 >>$seqres.full | _filter_scratch | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Wait and check grace time enforcement
rm -f $SCRATCH_MNT/file5 >>$seqres.full 2>&1
sleep $((grace+1))
echo "Touch 5"
- su $qa_user -c "touch $SCRATCH_MNT/file5" 2>&1 >>$seqres.full |
+ _su $qa_user -c "touch $SCRATCH_MNT/file5" 2>&1 >>$seqres.full |
_filter_scratch | tee -a $seqres.full
repquota -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
echo "--- completed IO ($type)" >>$seqres.full
diff --git a/tests/generic/231 b/tests/generic/231
index ef3ea45d45d736..8dda926d875e88 100755
--- a/tests/generic/231
+++ b/tests/generic/231
@@ -24,7 +24,7 @@ _fsx()
for (( i = 1; i <= $tasks; i++ )); do
SEED=$RANDOM
echo "ltp/fsx $FSX_ARGS -S $SEED $SCRATCH_MNT/fsx_file$i" >>$seqres.full
- su $qa_user -c "ltp/fsx $FSX_ARGS -S $SEED \
+ _su $qa_user -c "ltp/fsx $FSX_ARGS -S $SEED \
$FSX_AVOID $SCRATCH_MNT/fsx_file$i" >$tmp.output$i 2>&1 &
done
diff --git a/tests/generic/233 b/tests/generic/233
index b4c804ff217fbb..df67b39092b7cb 100755
--- a/tests/generic/233
+++ b/tests/generic/233
@@ -43,7 +43,7 @@ _fsstress()
# temporarily.
ulimit -l unlimited
echo "fsstress $args" >> $seqres.full
- if ! su $qa_user -c "$FSSTRESS_PROG $args" | tee -a $seqres.full | _filter_num
+ if ! _su $qa_user -c "$FSSTRESS_PROG $args" | tee -a $seqres.full | _filter_num
then
echo " fsstress $args returned $?"
cat $tmp.out | tee -a $seqres.full
diff --git a/tests/generic/270 b/tests/generic/270
index 342ac8b5d3d963..d74971bb535239 100755
--- a/tests/generic/270
+++ b/tests/generic/270
@@ -37,14 +37,14 @@ _workout()
# io_uring_queue_init fail on ENOMEM, set max locked memory to unlimited
# temporarily.
ulimit -l unlimited
- su $qa_user -c "$_FSSTRESS_PROG $args" > /dev/null 2>&1 &
+ _su $qa_user -c "$_FSSTRESS_PROG $args" > /dev/null 2>&1 &
_FSSTRESS_PID=$!
echo "Run dd writers in parallel"
for ((i=0; i < num_iterations; i++))
do
# File will be opened with O_TRUNC each time
- su $qa_user -c "dd if=/dev/zero \
+ _su $qa_user -c "dd if=/dev/zero \
of=$SCRATCH_MNT/SPACE_CONSUMER bs=1M " \
>> $seqres.full 2>&1
sleep $enospc_time
diff --git a/tests/generic/314 b/tests/generic/314
index 5fbc6424de3ab4..65f7f9d9619f2e 100755
--- a/tests/generic/314
+++ b/tests/generic/314
@@ -27,7 +27,7 @@ chown $qa_user:12345 $TEST_DIR/$seq-dir
chmod 2775 $TEST_DIR/$seq-dir
# Make subdir
-su $qa_user -c "umask 022; mkdir $TEST_DIR/$seq-dir/subdir"
+_su $qa_user -c "umask 022; mkdir $TEST_DIR/$seq-dir/subdir"
# Subdir should have inherited sgid
_ls_l $TEST_DIR/$seq-dir/ | grep -v total | _filter_test_dir | awk '{print $1,$NF}'
diff --git a/tests/generic/327 b/tests/generic/327
index 2323e1e6a12ec8..18cfcd1f655bd7 100755
--- a/tests/generic/327
+++ b/tests/generic/327
@@ -47,7 +47,7 @@ _report_quota_blocks $SCRATCH_MNT
echo "Try to reflink again"
touch $testdir/file3
chown $qa_user $testdir/file3
-su $qa_user -c "cp --reflink=always -f $testdir/file1 $testdir/file3" 2>&1 | _filter_scratch
+_su $qa_user -c "cp --reflink=always -f $testdir/file1 $testdir/file3" 2>&1 | _filter_scratch
_report_quota_blocks $SCRATCH_MNT
# success, all done
diff --git a/tests/generic/328 b/tests/generic/328
index 7071ded259ddb3..fa33bdb78dba12 100755
--- a/tests/generic/328
+++ b/tests/generic/328
@@ -47,12 +47,12 @@ setquota -u $qa_user 0 1024 0 0 $SCRATCH_MNT
_report_quota_blocks $SCRATCH_MNT
echo "Try to dio write the whole file"
-su $qa_user -c '$XFS_IO_PROG -d -c "pwrite 0 '$((sz+65536))'" '$testdir'/file1' 2>&1 >> $seqres.full | \
+_su $qa_user -c '$XFS_IO_PROG -d -c "pwrite 0 '$((sz+65536))'" '$testdir'/file1' 2>&1 >> $seqres.full | \
_filter_xfs_io_error
_report_quota_blocks $SCRATCH_MNT
echo "Try to write the whole file"
-su $qa_user -c '$XFS_IO_PROG -c "pwrite 0 '$((sz+65536))'" '$testdir'/file1' 2>&1 >> $seqres.full | \
+_su $qa_user -c '$XFS_IO_PROG -c "pwrite 0 '$((sz+65536))'" '$testdir'/file1' 2>&1 >> $seqres.full | \
_filter_xfs_io_error
_report_quota_blocks $SCRATCH_MNT
diff --git a/tests/generic/355 b/tests/generic/355
index b0f8fc45d2b5f4..6b4f7ebae86405 100755
--- a/tests/generic/355
+++ b/tests/generic/355
@@ -22,7 +22,7 @@ rm -f $testfile
do_io()
{
- su $qa_user -c "$XFS_IO_PROG -d -c 'pwrite 0 4k' $testfile" \
+ _su $qa_user -c "$XFS_IO_PROG -d -c 'pwrite 0 4k' $testfile" \
>>$seqres.full
}
diff --git a/tests/generic/453 b/tests/generic/453
index b7e686f37100da..04945ad1085b2d 100755
--- a/tests/generic/453
+++ b/tests/generic/453
@@ -196,7 +196,7 @@ setf "job offer\xef\xb9\x92pdf" "small full stop"
setf "job offer\xef\xbc\x8epdf" "fullwidth full stop"
setf "job offer\xdc\x81pdf" "syriac supralinear full stop"
setf "job offer\xdc\x82pdf" "syriac sublinear full stop"
-setf "job offer\xea\x93\xb8pdf" "lisu letter tone mya ti"
+setf "job offer\xea\x93\xb8pdf" "li_su letter tone mya ti"
setf "job offer.pdf" "actual period"
# encoding hidden tag characters in filenames to create confusing names
@@ -270,7 +270,7 @@ testf "job offer\xef\xb9\x92pdf" "small full stop"
testf "job offer\xef\xbc\x8epdf" "fullwidth full stop"
testf "job offer\xdc\x81pdf" "syriac supralinear full stop"
testf "job offer\xdc\x82pdf" "syriac sublinear full stop"
-testf "job offer\xea\x93\xb8pdf" "lisu letter tone mya ti"
+testf "job offer\xea\x93\xb8pdf" "li_su letter tone mya ti"
testf "job offer.pdf" "actual period"
testf "llamapirate\xf3\xa0\x80\x81\xf3\xa0\x81\x94\xf3\xa0\x81\xa8\xf3\xa0\x81\xa5\xf3\xa0\x80\xa0\xf3\xa0\x81\xb3\xf3\xa0\x81\xa1\xf3\xa0\x81\xac\xf3\xa0\x81\xa5\xf3\xa0\x81\xb3\xf3\xa0\x80\xa0\xf3\xa0\x81\xa6\xf3\xa0\x81\xaf\xf3\xa0\x81\xb2\xf3\xa0\x80\xa0\xf3\xa0\x81\x93\xf3\xa0\x81\xa5\xf3\xa0\x81\xa1\xf3\xa0\x81\xb4\xf3\xa0\x81\xb4\xf3\xa0\x81\xac\xf3\xa0\x81\xa5\xf3\xa0\x80\xa0\xf3\xa0\x81\xb7\xf3\xa0\x81\xa5\xf3\xa0\x81\xb2\xf3\xa0\x81\xa5\xf3\xa0\x80\xa0\xf3\xa0\x81\x95\xf3\xa0\x81\x93\xf3\xa0\x81\x84\xf3\xa0\x80\xa0\xf3\xa0\x80\xb1\xf3\xa0\x80\xb2\xf3\xa0\x80\xb0\xf3\xa0\x80\xb0\xf3\xa0\x80\xb0\xf3\xa0\x80\xb0\xf3\xa0\x81\xbf"
@@ -307,7 +307,7 @@ if _check_xfs_scrub_does_unicode "$SCRATCH_MNT" "$SCRATCH_DEV"; then
grep -q "job offer.xef.xbc.x8epdf" $tmp.scrub || echo "No complaints about fullwidth full stop?"
grep -q "job offer.xdc.x81pdf" $tmp.scrub || echo "No complaints about syriac supralinear full stop?"
grep -q "job offer.xdc.x82pdf" $tmp.scrub || echo "No complaints about syriac sublinear full stop?"
- grep -q "job offer.xea.x93.xb8pdf" $tmp.scrub || echo "No complaints about lisu letter tone mya ti?"
+ grep -q "job offer.xea.x93.xb8pdf" $tmp.scrub || echo "No complaints about li_su letter tone mya ti?"
grep -q "job offer.*could be confused with" $tmp.scrub || echo "No complaints about confusing job offers?"
grep -q "job offer.xe2.x80.xa4.xe2.x80.x8dpdf" $tmp.scrub || echo "No complaints about one dot leader with invisible space?"
grep -q "llamapirate" $tmp.scrub || echo "No complaints about hidden llm instructions in filenames?"
diff --git a/tests/generic/514 b/tests/generic/514
index 7f3d9c16cb70a4..a2086a255c77c6 100755
--- a/tests/generic/514
+++ b/tests/generic/514
@@ -21,7 +21,7 @@ _scratch_mount
chmod a+rwx $SCRATCH_MNT
$XFS_IO_PROG -f -c "pwrite -S 0x18 0 1m" $SCRATCH_MNT/foo >>$seqres.full
-su -s/bin/bash - $qa_user -c "ulimit -f 64 ; $XFS_IO_PROG -f -c \"reflink $SCRATCH_MNT/foo\" $SCRATCH_MNT/bar" >> $seqres.full 2>&1
+_su -s/bin/bash - $qa_user -c "ulimit -f 64 ; $XFS_IO_PROG -f -c \"reflink $SCRATCH_MNT/foo\" $SCRATCH_MNT/bar" >> $seqres.full 2>&1
sz="$(_get_filesize $SCRATCH_MNT/bar)"
if [ "$sz" -ne 0 ] && [ "$sz" -ne 65536 ]; then
diff --git a/tests/generic/573 b/tests/generic/573
index b310fccbddda56..d3f3296cb6bafa 100755
--- a/tests/generic/573
+++ b/tests/generic/573
@@ -56,7 +56,7 @@ $CHATTR_PROG -i $fsv_file
_fsv_scratch_begin_subtest "FS_IOC_MEASURE_VERITY doesn't require root"
_fsv_create_enable_file $fsv_file >> $seqres.full
chmod 444 $fsv_file
-su $qa_user -c "$FSVERITY_PROG measure $fsv_file" >> $seqres.full
+_su $qa_user -c "$FSVERITY_PROG measure $fsv_file" >> $seqres.full
# success, all done
status=0
diff --git a/tests/generic/600 b/tests/generic/600
index 43f75376a10efc..31c832251ebb6f 100755
--- a/tests/generic/600
+++ b/tests/generic/600
@@ -33,7 +33,7 @@ setquota -t -u 0 1 $SCRATCH_MNT
# Soft inode limit 1, hard limit 5
setquota -u $qa_user 0 0 1 5 $SCRATCH_MNT
# Run qa user over soft limit and go over grace period
-su $qa_user -c "touch $SCRATCH_MNT/file1 $SCRATCH_MNT/file2"
+_su $qa_user -c "touch $SCRATCH_MNT/file1 $SCRATCH_MNT/file2"
sleep 3
# Extend grace to now + 100s
now=`date +%s`
diff --git a/tests/generic/601 b/tests/generic/601
index 78b6a4aaa13748..320ac0c758b766 100755
--- a/tests/generic/601
+++ b/tests/generic/601
@@ -42,7 +42,7 @@ $XFS_QUOTA_PROG -x -c "timer -u -i -d 1" $SCRATCH_MNT
# Soft inode limit 1, hard limit 5
$XFS_QUOTA_PROG -x -c "limit -u isoft=1 ihard=5 $qa_user" $SCRATCH_MNT
# Run qa user over soft limit and go over grace period
-su $qa_user -c "touch $SCRATCH_MNT/file1 $SCRATCH_MNT/file2"
+_su $qa_user -c "touch $SCRATCH_MNT/file1 $SCRATCH_MNT/file2"
sleep 3
# Extend grace to now + 100s
now=`date +%s`
diff --git a/tests/generic/603 b/tests/generic/603
index 2a75cf9e022750..b199f801a8f03f 100755
--- a/tests/generic/603
+++ b/tests/generic/603
@@ -66,13 +66,13 @@ test_grace()
echo "--- Test block quota ---"
# Firstly fit below block soft limit
echo "Write 225 blocks..."
- su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((225 * $BLOCK_SIZE))' \
+ _su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((225 * $BLOCK_SIZE))' \
-c fsync $dir/file1" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -v -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Secondly overcome block soft limit
echo "Rewrite 250 blocks plus 1 byte, over the block softlimit..."
- su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE + 1))' \
+ _su $qa_user -c "$XFS_IO_PROG -c 'pwrite 0 $((250 * $BLOCK_SIZE + 1))' \
-c fsync $dir/file1" 2>&1 >>$seqres.full | \
_filter_xfs_io_error | tee -a $seqres.full
repquota -v -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
@@ -81,7 +81,7 @@ test_grace()
# Now sleep enough grace time and check that softlimit got enforced
sleep $((bgrace + 1))
echo "Try to write 1 one more block after grace..."
- su $qa_user -c "$XFS_IO_PROG -c 'truncate 0' -c 'pwrite 0 $BLOCK_SIZE' \
+ _su $qa_user -c "$XFS_IO_PROG -c 'truncate 0' -c 'pwrite 0 $BLOCK_SIZE' \
$dir/file2" 2>&1 >>$seqres.full | _filter_xfs_io_error | \
filter_enospc_edquot $type | tee -a $seqres.full
repquota -v -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
@@ -91,7 +91,7 @@ test_grace()
# space reservations on XFS
setquota -$type $qa_user 0 0 3 100 $SCRATCH_MNT
echo "Create 2 more files, over the inode softlimit..."
- su $qa_user -c "touch $dir/file3 $dir/file4" 2>&1 >>$seqres.full | \
+ _su $qa_user -c "touch $dir/file3 $dir/file4" 2>&1 >>$seqres.full | \
_filter_scratch | tee -a $seqres.full
repquota -v -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Reset grace time here, make below grace time test more accurate
@@ -99,7 +99,7 @@ test_grace()
# Wait and check grace time enforcement
sleep $((igrace+1))
echo "Try to create one more inode after grace..."
- su $qa_user -c "touch $dir/file5" 2>&1 >>$seqres.full | \
+ _su $qa_user -c "touch $dir/file5" 2>&1 >>$seqres.full | \
filter_enospc_edquot $type | _filter_scratch | \
tee -a $seqres.full
repquota -v -$type $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
diff --git a/tests/generic/673 b/tests/generic/673
index 8f6def9c78881a..6c54ade81f0cec 100755
--- a/tests/generic/673
+++ b/tests/generic/673
@@ -39,7 +39,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'reflink $SCRATCH_MNT/b 0 0 1m' $SCRATCH_MNT/a"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/674 b/tests/generic/674
index 1b711f27f39ed1..41fbdeb7d9eb17 100755
--- a/tests/generic/674
+++ b/tests/generic/674
@@ -42,7 +42,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'dedupe $SCRATCH_MNT/b 0 0 1m' $SCRATCH_MNT/a"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/675 b/tests/generic/675
index e66de84b546a25..87dfbdfe278dd2 100755
--- a/tests/generic/675
+++ b/tests/generic/675
@@ -44,7 +44,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'reflink $SCRATCH_MNT/b 0 0 1m' $SCRATCH_MNT/a"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/680 b/tests/generic/680
index 07048db5cc39a9..1a418fa3b61b0b 100755
--- a/tests/generic/680
+++ b/tests/generic/680
@@ -38,7 +38,7 @@ chmod 0644 $localfile
cp $here/src/splice2pipe $tmp.splice2pipe
# Test unprivileged user's privilege escalation
echo "Test unprivileged user:"
-su ${qa_user} -c "$tmp.splice2pipe $localfile 1 AAAAAAAABBBBBBBB"
+_su ${qa_user} -c "$tmp.splice2pipe $localfile 1 AAAAAAAABBBBBBBB"
_hexdump $localfile
# success, all done
diff --git a/tests/generic/681 b/tests/generic/681
index aef54205d26f3a..dc4252013fc058 100755
--- a/tests/generic/681
+++ b/tests/generic/681
@@ -55,7 +55,7 @@ ls -sld $scratchdir >> $seqres.full
echo "fail quota" >> $seqres.full
for ((i = 0; i < dirents; i++)); do
name=$(printf "y%0254d" $i)
- su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
+ _su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
test "${PIPESTATUS[0]}" -ne 0 && break
done
diff --git a/tests/generic/682 b/tests/generic/682
index 3572af173cbe63..6914a549dc0975 100755
--- a/tests/generic/682
+++ b/tests/generic/682
@@ -65,7 +65,7 @@ echo "fail quota" >> $seqres.full
for ((i = 0; i < dirents; i++)); do
name=$(printf "y%0254d" $i)
ln $scratchfile $stagedir/$name
- su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \
+ _su - "$qa_user" -c "mv $stagedir/$name $scratchdir/$name" 2>&1 | \
_filter_scratch | _filter_mv_output
test "${PIPESTATUS[0]}" -ne 0 && break
done
diff --git a/tests/generic/683 b/tests/generic/683
index cc9a9786bde4bf..883905da5f9082 100755
--- a/tests/generic/683
+++ b/tests/generic/683
@@ -49,7 +49,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/684 b/tests/generic/684
index 2ca036fe518050..9cdfe4ab4f4463 100755
--- a/tests/generic/684
+++ b/tests/generic/684
@@ -49,7 +49,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/685 b/tests/generic/685
index de07a798a68594..5567255032e1c7 100755
--- a/tests/generic/685
+++ b/tests/generic/685
@@ -49,7 +49,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/686 b/tests/generic/686
index fc6761fe61a91e..a3fa8e060aeab6 100755
--- a/tests/generic/686
+++ b/tests/generic/686
@@ -49,7 +49,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/687 b/tests/generic/687
index 82dce88b85ef6d..0c4b09d29fe5e6 100755
--- a/tests/generic/687
+++ b/tests/generic/687
@@ -49,7 +49,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c '$command $start $end' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/688 b/tests/generic/688
index e491d5cf30af23..77db29461415b3 100755
--- a/tests/generic/688
+++ b/tests/generic/688
@@ -51,7 +51,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'falloc 0 64k' $junk_file"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/691 b/tests/generic/691
index f33d6edf18a1d1..30ae4a1e384a05 100755
--- a/tests/generic/691
+++ b/tests/generic/691
@@ -75,11 +75,11 @@ exercise()
setquota -${type} -t 86400 86400 $SCRATCH_MNT
repquota -v -${type} $SCRATCH_MNT | grep -v "^root" >>$seqres.full 2>&1
# Exceed the soft quota limit a bit at first
- su $qa_user -c "$XFS_IO_PROG -f -t -c 'pwrite 0 2m' -c fsync ${file}.0" >>$seqres.full
+ _su $qa_user -c "$XFS_IO_PROG -f -t -c 'pwrite 0 2m' -c fsync ${file}.0" >>$seqres.full
# Write more data more times under soft quota limit exhausted condition,
# but not reach hard limit. To make sure each write won't trigger EDQUOT.
for ((i=1; i<=100; i++));do
- su "$qa_user" -c "$XFS_IO_PROG -f -c 'pwrite 0 1m' -c fsync ${file}.$i" >>$seqres.full
+ _su "$qa_user" -c "$XFS_IO_PROG -f -c 'pwrite 0 1m' -c fsync ${file}.$i" >>$seqres.full
if [ $? -ne 0 ];then
echo "Unexpected error (type=$type)!"
break
@@ -89,9 +89,9 @@ exercise()
# As we've tested soft limit, now exceed the hard limit and give it a
# test in passing.
- su $qa_user -c "$XFS_IO_PROG -f -t -c 'pwrite 0 100m' -c fsync ${file}.hard.0" 2>&1 >/dev/null | filter_quota $type
+ _su $qa_user -c "$XFS_IO_PROG -f -t -c 'pwrite 0 100m' -c fsync ${file}.hard.0" 2>&1 >/dev/null | filter_quota $type
for ((i=1; i<=10; i++));do
- su "$qa_user" -c "$XFS_IO_PROG -f -c 'pwrite 0 1m' -c fsync ${file}.hard.$i" 2>&1 | filter_quota $type
+ _su "$qa_user" -c "$XFS_IO_PROG -f -c 'pwrite 0 1m' -c fsync ${file}.hard.$i" 2>&1 | filter_quota $type
done
}
diff --git a/tests/generic/721 b/tests/generic/721
index a9565f18917831..75d5063c2d1701 100755
--- a/tests/generic/721
+++ b/tests/generic/721
@@ -52,7 +52,7 @@ cmd="$XFS_IO_PROG \
-c 'pwrite -S 0x60 44k 55k -b 1m' \
-c 'commitupdate -q' \
\"$dir/a\""
-su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
+_su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
filesnap "after commit" $dir/a
echo
@@ -67,7 +67,7 @@ cmd="$XFS_IO_PROG \
-c 'pwrite -S 0x60 0 55k' \
-c 'commitupdate -q' \
\"$dir/a\""
-su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
+_su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
filesnap "after shorten commit" $dir/a
echo
@@ -81,7 +81,7 @@ cmd="$XFS_IO_PROG \
-c \"pwrite -S 0x60 0 $(( (blksz * nrblks) + 37373 ))\" \
-c 'commitupdate -q' \
\"$dir/a\""
-su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
+_su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
filesnap "after lengthen commit" $dir/a
echo
@@ -96,7 +96,7 @@ cmd="$XFS_IO_PROG \
-c 'pwrite -S 0x60 44k 55k -b 1m' \
-c 'cancelupdate' \
\"$dir/a\""
-su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
+_su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
filesnap "after cancel" $dir/a
echo
@@ -115,7 +115,7 @@ cmd="$XFS_IO_PROG \
-c 'pwrite -S 0x61 22k 11k -b 1m' \
-c 'commitupdate -q' \
\"$dir/a\""
-su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
+_su -s /bin/bash -c "$cmd" $qa_user 2>&1 | _filter_xfs_io | _filter_test_dir
filesnap "after fail commit" $dir/a
echo
diff --git a/tests/generic/726 b/tests/generic/726
index 131ac5b503e1a4..d2a2a2cebe522e 100755
--- a/tests/generic/726
+++ b/tests/generic/726
@@ -46,7 +46,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'startupdate' -c 'pwrite -S 0x57 0 1m' -c 'commitupdate' $SCRATCH_MNT/a"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/generic/727 b/tests/generic/727
index ee7ed9760a165a..9551e47cb13b06 100755
--- a/tests/generic/727
+++ b/tests/generic/727
@@ -54,7 +54,7 @@ commit_and_check() {
local cmd="$XFS_IO_PROG -c 'startupdate' -c 'pwrite -S 0x57 0 1m' -c 'commitupdate' $SCRATCH_MNT/a"
if [ -n "$user" ]; then
- su - "$user" -c "$cmd" >> $seqres.full
+ _su - "$user" -c "$cmd" >> $seqres.full
else
$SHELL -c "$cmd" >> $seqres.full
fi
diff --git a/tests/xfs/720 b/tests/xfs/720
index f928cc43d3bc54..68a6c7f6e2d584 100755
--- a/tests/xfs/720
+++ b/tests/xfs/720
@@ -61,7 +61,7 @@ $XFS_QUOTA_PROG -x -c 'report -u' $SCRATCH_MNT >> $seqres.full
# Fail at appending the file as qa_user to ensure quota enforcement works
echo "fail quota" >> $seqres.full
-su - "$qa_user" -c "$XFS_IO_PROG -c 'pwrite 10g 1' $scratchfile" >> $seqres.full
+_su - "$qa_user" -c "$XFS_IO_PROG -c 'pwrite 10g 1' $scratchfile" >> $seqres.full
$XFS_QUOTA_PROG -x -c 'report -u' $SCRATCH_MNT >> $seqres.full
# success, all done
diff --git a/tests/xfs/795 b/tests/xfs/795
index 5a67f02ec92eca..217f96092a4c42 100755
--- a/tests/xfs/795
+++ b/tests/xfs/795
@@ -63,7 +63,7 @@ $XFS_QUOTA_PROG -x -c 'report -u' $SCRATCH_MNT >> $seqres.full
echo "fail quota" >> $seqres.full
for ((i = 0; i < dirents; i++)); do
name=$(printf "y%0254d" $i)
- su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
+ _su - "$qa_user" -c "ln $scratchfile $scratchdir/$name" 2>&1 | \
_filter_scratch | sed -e 's/y[0-9]*/yXXX/g'
test "${PIPESTATUS[0]}" -ne 0 && break
done
diff --git a/tests/xfs/803 b/tests/xfs/803
index e3277181b41af0..4e5a58c4a3328e 100755
--- a/tests/xfs/803
+++ b/tests/xfs/803
@@ -88,7 +88,7 @@ echo set too long value
$XFS_IO_PROG -c "setfsprops $propname=$longpropval" $TEST_DIR
echo not enough permissions
-su - "$qa_user" -c "$XFS_IO_PROG -c \"setfsprops $propname=$propval\" $TEST_DIR" 2>&1 | _filter_test_dir
+_su - "$qa_user" -c "$XFS_IO_PROG -c \"setfsprops $propname=$propval\" $TEST_DIR" 2>&1 | _filter_test_dir
echo "*** DB TEST ***"
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (6 preceding siblings ...)
2025-01-16 23:27 ` [PATCH 07/23] common/rc: create a wrapper for the su command Darrick J. Wong
@ 2025-01-16 23:27 ` Darrick J. Wong
2025-01-21 3:28 ` Dave Chinner
2025-01-16 23:27 ` [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering Darrick J. Wong
` (14 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:27 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Run each test program with a separate session id so that we can tell
pkill to kill all processes of a given name, but only within our own
session id. This /should/ suffice to run multiple fstests on the same
machine without one instance shooting down processes of another
instance.
This fixes a general problem with using "pkill --parent" -- if the
process being targeted is not a direct descendant of the bash script
calling pkill, then pkill will not do anything. The scrub stress tests
make use of multiple background subshells, which is how a ^C in the
parent process fails to result in fsx/fsstress being killed.
This is necessary to fix SOAK_DURATION runtime constraints for all the
scrub stress tests. However, there is a cost -- the test program no
longer runs with the same controlling tty as ./check, which means that
^Z doesn't work and SIGINT/SIGQUIT are set to SIG_IGN. IOWs, if a test
wants to kill its subprocesses, it must use another signal such as
SIGPIPE. Fortunately, bash doesn't whine about children dying due to
fatal signals if the children run in a different session id.
I also explored alternate designs, and this was the least unsatisfying:
a) Setting the process group didn't work because background subshells
are assigned a new group id.
b) Constraining the pkill/pgrep search to a cgroup could work, but we'd
have to set up a cgroup in which to run the fstest.
c) Putting test subprocesses in a systemd sub-scope and telling systemd
to kill the sub-scope could work because ./check can already use it to
ensure that all child processes of a test are killed. However, this is
an *optional* feature, which means that we'd have to require systemd.
d) Constraining the pkill/pgrep search to a particular mount namespace
could work, but we already have tests that set up their own mount
namespaces, which means the constrained pgrep will not find all child
processes of a test.
e) Constraining to any other type of namespace (uts, pid, etc) might not
work because those namespaces might not be enabled.
f) Revert check-parallel and go back to one fstests instance per system.
Zorro already chose not to revert.
So. Change _run_seq to create a the ./$seq process with a new session
id, update _su calls to use the same session as the parent test, update
all the pkill sites to use a wrapper so that we only target processes
created by *this* instance of fstests, and update SIGINT to SIGPIPE.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
check | 33 ++++++++++++++++++++++++++++-----
common/fuzzy | 17 ++++++++---------
common/rc | 12 ++++++++++--
tests/generic/310 | 6 +++---
tests/generic/561 | 2 +-
5 files changed, 50 insertions(+), 20 deletions(-)
diff --git a/check b/check
index 607d2456e6a1fe..bafe48bf12db67 100755
--- a/check
+++ b/check
@@ -698,18 +698,41 @@ _adjust_oom_score -500
# systemd doesn't automatically remove transient scopes that fail to terminate
# when systemd tells them to terminate (e.g. programs stuck in D state when
# systemd sends SIGKILL), so we use reset-failed to tear down the scope.
+#
+# Use setsid to run the test program with a separate session id so that we
+# can pkill only the processes started by this test.
_run_seq() {
- local cmd=(bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec ./$seq")
+ local cmd=(bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec setsid bash ./$seq")
if [ -n "${HAVE_SYSTEMD_SCOPES}" ]; then
local unit="$(systemd-escape "fs$seq").scope"
systemctl reset-failed "${unit}" &> /dev/null
- systemd-run --quiet --unit "${unit}" --scope "${cmd[@]}"
+ systemd-run --quiet --unit "${unit}" --scope "${cmd[@]}" &
+ CHILDPID=$!
+ wait
res=$?
+ unset CHILDPID
systemctl stop "${unit}" &> /dev/null
return "${res}"
else
- "${cmd[@]}"
+ # bash won't run the SIGINT trap handler while there are
+ # foreground children in a separate session, so we must run
+ # the test in the background and wait for it.
+ "${cmd[@]}" &
+ CHILDPID=$!
+ wait
+ unset CHILDPID
+ fi
+}
+
+_kill_seq() {
+ if [ -n "$CHILDPID" ]; then
+ # SIGPIPE will kill all the children (including fsstress)
+ # without bash logging fatal signal termination messages to the
+ # console
+ pkill -PIPE --session "$CHILDPID"
+ wait
+ unset CHILDPID
fi
}
@@ -718,9 +741,9 @@ _prepare_test_list
fstests_start_time="$(date +"%F %T")"
if $OPTIONS_HAVE_SECTIONS; then
- trap "_summary; exit \$status" 0 1 2 3 15
+ trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
else
- trap "_wrapup; exit \$status" 0 1 2 3 15
+ trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
fi
function run_section()
diff --git a/common/fuzzy b/common/fuzzy
index 0a2d91542b561e..772ce7ddcff6d8 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -891,7 +891,7 @@ __stress_xfs_scrub_loop() {
local runningfile="$2"
local scrub_startat="$3"
shift; shift; shift
- local sigint_ret="$(( $(kill -l SIGINT) + 128 ))"
+ local signal_ret="$(( $(kill -l SIGPIPE) + 128 ))"
local scrublog="$tmp.scrub"
while __stress_scrub_running "$scrub_startat" "$runningfile"; do
@@ -901,8 +901,8 @@ __stress_xfs_scrub_loop() {
while __stress_scrub_running "$end" "$runningfile"; do
_scratch_scrub "$@" &> $scrublog
res=$?
- if [ "$res" -eq "$sigint_ret" ]; then
- # Ignore SIGINT because the cleanup function sends
+ if [ "$res" -eq "$signal_ret" ]; then
+ # Ignore SIGPIPE because the cleanup function sends
# that to terminate xfs_scrub
res=0
fi
@@ -1173,13 +1173,11 @@ _scratch_xfs_stress_scrub_cleanup() {
rm -f "$runningfile"
echo "Cleaning up scrub stress run at $(date)" >> $seqres.full
- # Send SIGINT so that bash won't print a 'Terminated' message that
- # distorts the golden output.
echo "Killing stressor processes at $(date)" >> $seqres.full
- _kill_fsstress
- pkill -INT --parent $$ xfs_io >> $seqres.full 2>&1
- pkill -INT --parent $$ fsx >> $seqres.full 2>&1
- pkill -INT --parent $$ xfs_scrub >> $seqres.full 2>&1
+ _pkill --echo -PIPE fsstress >> $seqres.full 2>&1
+ _pkill --echo -PIPE xfs_io >> $seqres.full 2>&1
+ _pkill --echo -PIPE fsx >> $seqres.full 2>&1
+ _pkill --echo -PIPE xfs_scrub >> $seqres.full 2>&1
# Tests are not allowed to exit with the scratch fs frozen. If we
# started a fs freeze/thaw background loop, wait for that loop to exit
@@ -1209,6 +1207,7 @@ _scratch_xfs_stress_scrub_cleanup() {
# Wait for the remaining children to exit.
echo "Waiting for children to exit at $(date)" >> $seqres.full
wait
+ echo "Children exited as of $(date)" >> $seqres.full
# Ensure the scratch fs is also writable before we exit.
if [ -n "$__SCRUB_STRESS_REMOUNT_LOOP" ]; then
diff --git a/common/rc b/common/rc
index 459be11c11c405..d143ba36265c6c 100644
--- a/common/rc
+++ b/common/rc
@@ -30,6 +30,12 @@ _test_sync()
_sync_fs $TEST_DIR
}
+# Kill only the test processes started by this test
+_pkill()
+{
+ pkill --session 0 "$@"
+}
+
# Common execution handling for fsstress invocation.
#
# We need per-test fsstress binaries because of the way fsstress forks and
@@ -69,7 +75,7 @@ _kill_fsstress()
if [ -n "$_FSSTRESS_PID" ]; then
# use SIGPIPE to avoid "Killed" messages from bash
echo "killing $_FSSTRESS_BIN" >> $seqres.full
- pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
+ _pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
_wait_for_fsstress
return $?
fi
@@ -2740,9 +2746,11 @@ _require_user_exists()
[ "$?" == "0" ] || _notrun "$user user not defined."
}
+# Run all non-root processes in the same session as the root. Believe it or
+# not, passing $SHELL in this manner works both for "su" and "su -c cmd".
_su()
{
- su "$@"
+ su --session-command $SHELL "$@"
}
# check if a user exists and is able to execute commands.
diff --git a/tests/generic/310 b/tests/generic/310
index 52babfdc803a21..570cc5f3859548 100755
--- a/tests/generic/310
+++ b/tests/generic/310
@@ -29,7 +29,7 @@ _begin_fstest auto
# Override the default cleanup function.
_cleanup()
{
- pkill -9 $seq.t_readdir > /dev/null 2>&1
+ _pkill -9 $seq.t_readdir > /dev/null 2>&1
wait
rm -rf $TEST_DIR/tmp
rm -f $tmp.*
@@ -83,7 +83,7 @@ _test_read()
{
$TEST_DIR/$seq.t_readdir_1 $SEQ_DIR > /dev/null 2>&1 &
sleep $RUN_TIME
- pkill -PIPE $seq.t_readdir_1
+ _pkill -PIPE $seq.t_readdir_1
wait
check_kernel_bug
@@ -97,7 +97,7 @@ _test_lseek()
$TEST_DIR/$seq.t_readdir_2 $SEQ_DIR > /dev/null 2>&1 &
readdir_pid=$!
sleep $RUN_TIME
- pkill -PIPE $seq.t_readdir_2
+ _pkill -PIPE $seq.t_readdir_2
wait
check_kernel_bug
diff --git a/tests/generic/561 b/tests/generic/561
index afe727ac56cbd9..b260aaf16c9256 100755
--- a/tests/generic/561
+++ b/tests/generic/561
@@ -40,7 +40,7 @@ function end_test()
# stop duperemove running
if [ -e $dupe_run ]; then
rm -f $dupe_run
- pkill $dedup_bin >/dev/null 2>&1
+ _pkill $dedup_bin >/dev/null 2>&1
wait $dedup_pids
rm -f $dedup_prog
fi
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-16 23:27 ` [PATCH 08/23] common: fix pkill by running test program in a separate session Darrick J. Wong
@ 2025-01-21 3:28 ` Dave Chinner
2025-01-22 4:24 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:28 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Run each test program with a separate session id so that we can tell
> pkill to kill all processes of a given name, but only within our own
> session id. This /should/ suffice to run multiple fstests on the same
> machine without one instance shooting down processes of another
> instance.
>
> This fixes a general problem with using "pkill --parent" -- if the
> process being targeted is not a direct descendant of the bash script
> calling pkill, then pkill will not do anything. The scrub stress tests
> make use of multiple background subshells, which is how a ^C in the
> parent process fails to result in fsx/fsstress being killed.
Yeah, 'pkill --parent' was the best I had managed to come up that
mostly worked, not because it perfect. That was something I wanted
feedback on before merge because it still had problems...
> This is necessary to fix SOAK_DURATION runtime constraints for all the
> scrub stress tests. However, there is a cost -- the test program no
> longer runs with the same controlling tty as ./check, which means that
> ^Z doesn't work and SIGINT/SIGQUIT are set to SIG_IGN. IOWs, if a test
> wants to kill its subprocesses, it must use another signal such as
> SIGPIPE. Fortunately, bash doesn't whine about children dying due to
> fatal signals if the children run in a different session id.
>
> I also explored alternate designs, and this was the least unsatisfying:
>
> a) Setting the process group didn't work because background subshells
> are assigned a new group id.
Yup, tried that.
> b) Constraining the pkill/pgrep search to a cgroup could work, but we'd
> have to set up a cgroup in which to run the fstest.
thought about that, too, and considered if systemd scopes could do
that, but ...
>
> c) Putting test subprocesses in a systemd sub-scope and telling systemd
> to kill the sub-scope could work because ./check can already use it to
> ensure that all child processes of a test are killed. However, this is
> an *optional* feature, which means that we'd have to require systemd.
... requiring systemd was somewhat of a show-stopper for testing
older distros.
> d) Constraining the pkill/pgrep search to a particular mount namespace
> could work, but we already have tests that set up their own mount
> namespaces, which means the constrained pgrep will not find all child
> processes of a test.
*nod*.
> e) Constraining to any other type of namespace (uts, pid, etc) might not
> work because those namespaces might not be enabled.
*nod*
I also tried modifying fsstress to catch and propagate signals and a
couple of other ways of managing processes in the stress code, but
all ended up having significantly worse downsides than using 'pkill
--parent'.
I was aware of session IDs, but I've never used them before and
hadn't gone down the rabbit hole of working out how to use them when
I posted the initial RFC patchset.
> f) Revert check-parallel and go back to one fstests instance per system.
> Zorro already chose not to revert.
>
> So. Change _run_seq to create a the ./$seq process with a new session
> id, update _su calls to use the same session as the parent test, update
> all the pkill sites to use a wrapper so that we only target processes
> created by *this* instance of fstests, and update SIGINT to SIGPIPE.
Yeah, that's definitely cleaner.
.....
> @@ -1173,13 +1173,11 @@ _scratch_xfs_stress_scrub_cleanup() {
> rm -f "$runningfile"
> echo "Cleaning up scrub stress run at $(date)" >> $seqres.full
>
> - # Send SIGINT so that bash won't print a 'Terminated' message that
> - # distorts the golden output.
> echo "Killing stressor processes at $(date)" >> $seqres.full
> - _kill_fsstress
> - pkill -INT --parent $$ xfs_io >> $seqres.full 2>&1
> - pkill -INT --parent $$ fsx >> $seqres.full 2>&1
> - pkill -INT --parent $$ xfs_scrub >> $seqres.full 2>&1
> + _pkill --echo -PIPE fsstress >> $seqres.full 2>&1
> + _pkill --echo -PIPE xfs_io >> $seqres.full 2>&1
> + _pkill --echo -PIPE fsx >> $seqres.full 2>&1
> + _pkill --echo -PIPE xfs_scrub >> $seqres.full 2>&1
Removing _kill_fsstress is wrong - the fsstress process has already
been renamed, so open coding 'pkill fsstress' may not match. The
_kill_fsstress() code gets changed to do the right thing here:
> @@ -69,7 +75,7 @@ _kill_fsstress()
> if [ -n "$_FSSTRESS_PID" ]; then
> # use SIGPIPE to avoid "Killed" messages from bash
> echo "killing $_FSSTRESS_BIN" >> $seqres.full
> - pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
> + _pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
> _wait_for_fsstress
> return $?
> fi
Then in the next patch when the _FSSTRESS_BIN workaround goes away,
_kill_fsstress() is exactly what you open coded in
_scratch_xfs_stress_scrub_cleanup()....
i.e. common/fuzzy really shouldn't open code the fsstress process
management - it should use the wrapper like everything else does.
Everything else in the patch looks good.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-21 3:28 ` Dave Chinner
@ 2025-01-22 4:24 ` Darrick J. Wong
2025-01-22 6:08 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 4:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Run each test program with a separate session id so that we can tell
> > pkill to kill all processes of a given name, but only within our own
> > session id. This /should/ suffice to run multiple fstests on the same
> > machine without one instance shooting down processes of another
> > instance.
> >
> > This fixes a general problem with using "pkill --parent" -- if the
> > process being targeted is not a direct descendant of the bash script
> > calling pkill, then pkill will not do anything. The scrub stress tests
> > make use of multiple background subshells, which is how a ^C in the
> > parent process fails to result in fsx/fsstress being killed.
>
> Yeah, 'pkill --parent' was the best I had managed to come up that
> mostly worked, not because it perfect. That was something I wanted
> feedback on before merge because it still had problems...
>
> > This is necessary to fix SOAK_DURATION runtime constraints for all the
> > scrub stress tests. However, there is a cost -- the test program no
> > longer runs with the same controlling tty as ./check, which means that
> > ^Z doesn't work and SIGINT/SIGQUIT are set to SIG_IGN. IOWs, if a test
> > wants to kill its subprocesses, it must use another signal such as
> > SIGPIPE. Fortunately, bash doesn't whine about children dying due to
> > fatal signals if the children run in a different session id.
> >
> > I also explored alternate designs, and this was the least unsatisfying:
> >
> > a) Setting the process group didn't work because background subshells
> > are assigned a new group id.
>
> Yup, tried that.
>
> > b) Constraining the pkill/pgrep search to a cgroup could work, but we'd
> > have to set up a cgroup in which to run the fstest.
>
> thought about that, too, and considered if systemd scopes could do
> that, but ...
>
> >
> > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > to kill the sub-scope could work because ./check can already use it to
> > ensure that all child processes of a test are killed. However, this is
> > an *optional* feature, which means that we'd have to require systemd.
>
> ... requiring systemd was somewhat of a show-stopper for testing
> older distros.
Isn't RHEL7 the oldest one at this point? And it does systemd. At this
point the only reason I didn't go full systemd is out of consideration
for Devuan, since they probably need QA.
> > d) Constraining the pkill/pgrep search to a particular mount namespace
> > could work, but we already have tests that set up their own mount
> > namespaces, which means the constrained pgrep will not find all child
> > processes of a test.
>
> *nod*.
>
> > e) Constraining to any other type of namespace (uts, pid, etc) might not
> > work because those namespaces might not be enabled.
>
> *nod*
>
> I also tried modifying fsstress to catch and propagate signals and a
> couple of other ways of managing processes in the stress code, but
> all ended up having significantly worse downsides than using 'pkill
> --parent'.
Yeah, and then you'd still have to figure out fsx and any other random
little utility that a test might run in a background.
> I was aware of session IDs, but I've never used them before and
> hadn't gone down the rabbit hole of working out how to use them when
> I posted the initial RFC patchset.
<nod> Session IDs kinda suck, but they suck the least for nearly minimal
extra effort.
> > f) Revert check-parallel and go back to one fstests instance per system.
> > Zorro already chose not to revert.
> >
> > So. Change _run_seq to create a the ./$seq process with a new session
> > id, update _su calls to use the same session as the parent test, update
> > all the pkill sites to use a wrapper so that we only target processes
> > created by *this* instance of fstests, and update SIGINT to SIGPIPE.
>
> Yeah, that's definitely cleaner.
>
> .....
>
> > @@ -1173,13 +1173,11 @@ _scratch_xfs_stress_scrub_cleanup() {
> > rm -f "$runningfile"
> > echo "Cleaning up scrub stress run at $(date)" >> $seqres.full
> >
> > - # Send SIGINT so that bash won't print a 'Terminated' message that
> > - # distorts the golden output.
> > echo "Killing stressor processes at $(date)" >> $seqres.full
> > - _kill_fsstress
> > - pkill -INT --parent $$ xfs_io >> $seqres.full 2>&1
> > - pkill -INT --parent $$ fsx >> $seqres.full 2>&1
> > - pkill -INT --parent $$ xfs_scrub >> $seqres.full 2>&1
> > + _pkill --echo -PIPE fsstress >> $seqres.full 2>&1
> > + _pkill --echo -PIPE xfs_io >> $seqres.full 2>&1
> > + _pkill --echo -PIPE fsx >> $seqres.full 2>&1
> > + _pkill --echo -PIPE xfs_scrub >> $seqres.full 2>&1
>
> Removing _kill_fsstress is wrong - the fsstress process has already
> been renamed, so open coding 'pkill fsstress' may not match. The
> _kill_fsstress() code gets changed to do the right thing here:
>
> > @@ -69,7 +75,7 @@ _kill_fsstress()
> > if [ -n "$_FSSTRESS_PID" ]; then
> > # use SIGPIPE to avoid "Killed" messages from bash
> > echo "killing $_FSSTRESS_BIN" >> $seqres.full
> > - pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
> > + _pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
> > _wait_for_fsstress
> > return $?
> > fi
>
> Then in the next patch when the _FSSTRESS_BIN workaround goes away,
> _kill_fsstress() is exactly what you open coded in
> _scratch_xfs_stress_scrub_cleanup()....
>
> i.e. common/fuzzy really shouldn't open code the fsstress process
> management - it should use the wrapper like everything else does.
Ok will change. I suppose I did go fix up the setting (or not) of
_FSSTRESS_PID.
> Everything else in the patch looks good.
Cool!
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-22 4:24 ` Darrick J. Wong
@ 2025-01-22 6:08 ` Dave Chinner
2025-01-22 7:05 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 6:08 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > to kill the sub-scope could work because ./check can already use it to
> > > ensure that all child processes of a test are killed. However, this is
> > > an *optional* feature, which means that we'd have to require systemd.
> >
> > ... requiring systemd was somewhat of a show-stopper for testing
> > older distros.
>
> Isn't RHEL7 the oldest one at this point? And it does systemd. At this
> point the only reason I didn't go full systemd is out of consideration
> for Devuan, since they probably need QA.
I have no idea what is out there in distro land vs what fstests
"supports". All I know is that there are distros out there that
don't use systemd.
It feels like poor form to prevent generic filesystem QA
infrastructure from running on those distros by making an avoidable
choice to tie the infrastructure exclusively to systemd-based
functionality....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-22 6:08 ` Dave Chinner
@ 2025-01-22 7:05 ` Darrick J. Wong
2025-01-22 9:42 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 7:05 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 05:08:17PM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> > On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > > to kill the sub-scope could work because ./check can already use it to
> > > > ensure that all child processes of a test are killed. However, this is
> > > > an *optional* feature, which means that we'd have to require systemd.
> > >
> > > ... requiring systemd was somewhat of a show-stopper for testing
> > > older distros.
> >
> > Isn't RHEL7 the oldest one at this point? And it does systemd. At this
> > point the only reason I didn't go full systemd is out of consideration
> > for Devuan, since they probably need QA.
>
> I have no idea what is out there in distro land vs what fstests
> "supports". All I know is that there are distros out there that
> don't use systemd.
>
> It feels like poor form to prevent generic filesystem QA
> infrastructure from running on those distros by making an avoidable
> choice to tie the infrastructure exclusively to systemd-based
> functionality....
Agreed, though at some point after these bugfixes are merged I'll see if
I can build on the existing "if you have systemd then ___ else here's
your shabby opencoded version" logic in fstests to isolate the ./checks
from each other a little better. It'd be kinda nice if we could
actually just put them in something resembling a modernish container,
albeit with the same underlying fs.
<shrug> Anyone else interested in that?
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-22 7:05 ` Darrick J. Wong
@ 2025-01-22 9:42 ` Dave Chinner
2025-01-22 21:46 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 9:42 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 11:05:20PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 22, 2025 at 05:08:17PM +1100, Dave Chinner wrote:
> > On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> > > On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > > > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > > > to kill the sub-scope could work because ./check can already use it to
> > > > > ensure that all child processes of a test are killed. However, this is
> > > > > an *optional* feature, which means that we'd have to require systemd.
> > > >
> > > > ... requiring systemd was somewhat of a show-stopper for testing
> > > > older distros.
> > >
> > > Isn't RHEL7 the oldest one at this point? And it does systemd. At this
> > > point the only reason I didn't go full systemd is out of consideration
> > > for Devuan, since they probably need QA.
> >
> > I have no idea what is out there in distro land vs what fstests
> > "supports". All I know is that there are distros out there that
> > don't use systemd.
> >
> > It feels like poor form to prevent generic filesystem QA
> > infrastructure from running on those distros by making an avoidable
> > choice to tie the infrastructure exclusively to systemd-based
> > functionality....
>
> Agreed, though at some point after these bugfixes are merged I'll see if
> I can build on the existing "if you have systemd then ___ else here's
> your shabby opencoded version" logic in fstests to isolate the ./checks
> from each other a little better. It'd be kinda nice if we could
> actually just put them in something resembling a modernish container,
> albeit with the same underlying fs.
Agreed, but I don't think we need to depend on systemd for that,
either.
> <shrug> Anyone else interested in that?
check-parallel has already started down that road with the
mount namespace isolation it uses for the runner tasks via
src/nsexec.c.
My plan has been to factor more of the check test running code
(similar to what I did with the test list parsing) so that the
check-parallel can iterate sections itself and runners can execute
individual tests directly, rather than bouncing them through check
to execute a set of tests serially. Then check-parallel could do
whatever it needed to isolate individual tests from each other and
nothing in check would need to change.
Now I'm wondering if I can just run each runner's check instance
in it's own private PID namespace as easily as I'm running them in
their own private mount namespace...
Hmmm - looks like src/nsexec.c can create new PID namespaces via
the "-p" option. I haven't used that before - I wonder if that's a
better solution that using per-test session IDs to solve the pkill
--parent problem? Something to look into in the morning....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-22 9:42 ` Dave Chinner
@ 2025-01-22 21:46 ` Darrick J. Wong
2025-01-23 1:16 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 21:46 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 08:42:49PM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 11:05:20PM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 22, 2025 at 05:08:17PM +1100, Dave Chinner wrote:
> > > On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> > > > On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > > > > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > > > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > > > > to kill the sub-scope could work because ./check can already use it to
> > > > > > ensure that all child processes of a test are killed. However, this is
> > > > > > an *optional* feature, which means that we'd have to require systemd.
> > > > >
> > > > > ... requiring systemd was somewhat of a show-stopper for testing
> > > > > older distros.
> > > >
> > > > Isn't RHEL7 the oldest one at this point? And it does systemd. At this
> > > > point the only reason I didn't go full systemd is out of consideration
> > > > for Devuan, since they probably need QA.
> > >
> > > I have no idea what is out there in distro land vs what fstests
> > > "supports". All I know is that there are distros out there that
> > > don't use systemd.
> > >
> > > It feels like poor form to prevent generic filesystem QA
> > > infrastructure from running on those distros by making an avoidable
> > > choice to tie the infrastructure exclusively to systemd-based
> > > functionality....
> >
> > Agreed, though at some point after these bugfixes are merged I'll see if
> > I can build on the existing "if you have systemd then ___ else here's
> > your shabby opencoded version" logic in fstests to isolate the ./checks
> > from each other a little better. It'd be kinda nice if we could
> > actually just put them in something resembling a modernish container,
> > albeit with the same underlying fs.
>
> Agreed, but I don't think we need to depend on systemd for that,
> either.
>
> > <shrug> Anyone else interested in that?
>
> check-parallel has already started down that road with the
> mount namespace isolation it uses for the runner tasks via
> src/nsexec.c.
>
> My plan has been to factor more of the check test running code
> (similar to what I did with the test list parsing) so that the
> check-parallel can iterate sections itself and runners can execute
> individual tests directly, rather than bouncing them through check
> to execute a set of tests serially. Then check-parallel could do
> whatever it needed to isolate individual tests from each other and
> nothing in check would need to change.
>
> Now I'm wondering if I can just run each runner's check instance
> in it's own private PID namespace as easily as I'm running them in
> their own private mount namespace...
>
> Hmmm - looks like src/nsexec.c can create new PID namespaces via
> the "-p" option. I haven't used that before - I wonder if that's a
> better solution that using per-test session IDs to solve the pkill
> --parent problem? Something to look into in the morning....
I tried that -- it appears to work, but then:
# ./src/nsexec -p bash
Current time: Wed Jan 22 13:43:33 PST 2025; Terminal: /dev/pts/0
# ps
fatal library error, lookup self
#
Regular processes and pkill/pgrep seem to work, but that didn't seem
especially encouraging so I stopped. :/
--D
> -Dave.
>
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-22 21:46 ` Darrick J. Wong
@ 2025-01-23 1:16 ` Dave Chinner
2025-01-28 4:34 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-23 1:16 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 01:46:09PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 22, 2025 at 08:42:49PM +1100, Dave Chinner wrote:
> > On Tue, Jan 21, 2025 at 11:05:20PM -0800, Darrick J. Wong wrote:
> > > On Wed, Jan 22, 2025 at 05:08:17PM +1100, Dave Chinner wrote:
> > > > On Tue, Jan 21, 2025 at 08:24:00PM -0800, Darrick J. Wong wrote:
> > > > > On Tue, Jan 21, 2025 at 02:28:26PM +1100, Dave Chinner wrote:
> > > > > > On Thu, Jan 16, 2025 at 03:27:15PM -0800, Darrick J. Wong wrote:
> > > > > > > c) Putting test subprocesses in a systemd sub-scope and telling systemd
> > > > > > > to kill the sub-scope could work because ./check can already use it to
> > > > > > > ensure that all child processes of a test are killed. However, this is
> > > > > > > an *optional* feature, which means that we'd have to require systemd.
> > > > > >
> > > > > > ... requiring systemd was somewhat of a show-stopper for testing
> > > > > > older distros.
> > > > >
> > > > > Isn't RHEL7 the oldest one at this point? And it does systemd. At this
> > > > > point the only reason I didn't go full systemd is out of consideration
> > > > > for Devuan, since they probably need QA.
> > > >
> > > > I have no idea what is out there in distro land vs what fstests
> > > > "supports". All I know is that there are distros out there that
> > > > don't use systemd.
> > > >
> > > > It feels like poor form to prevent generic filesystem QA
> > > > infrastructure from running on those distros by making an avoidable
> > > > choice to tie the infrastructure exclusively to systemd-based
> > > > functionality....
> > >
> > > Agreed, though at some point after these bugfixes are merged I'll see if
> > > I can build on the existing "if you have systemd then ___ else here's
> > > your shabby opencoded version" logic in fstests to isolate the ./checks
> > > from each other a little better. It'd be kinda nice if we could
> > > actually just put them in something resembling a modernish container,
> > > albeit with the same underlying fs.
> >
> > Agreed, but I don't think we need to depend on systemd for that,
> > either.
> >
> > > <shrug> Anyone else interested in that?
> >
> > check-parallel has already started down that road with the
> > mount namespace isolation it uses for the runner tasks via
> > src/nsexec.c.
> >
> > My plan has been to factor more of the check test running code
> > (similar to what I did with the test list parsing) so that the
> > check-parallel can iterate sections itself and runners can execute
> > individual tests directly, rather than bouncing them through check
> > to execute a set of tests serially. Then check-parallel could do
> > whatever it needed to isolate individual tests from each other and
> > nothing in check would need to change.
> >
> > Now I'm wondering if I can just run each runner's check instance
> > in it's own private PID namespace as easily as I'm running them in
> > their own private mount namespace...
> >
> > Hmmm - looks like src/nsexec.c can create new PID namespaces via
> > the "-p" option. I haven't used that before - I wonder if that's a
> > better solution that using per-test session IDs to solve the pkill
> > --parent problem? Something to look into in the morning....
>
> I tried that -- it appears to work, but then:
>
> # ./src/nsexec -p bash
> Current time: Wed Jan 22 13:43:33 PST 2025; Terminal: /dev/pts/0
> # ps
> fatal library error, lookup self
> #
That looks like a bug in whatever distro you are using - it works
as it should here on a recent debian unstable userspace.
Note, however, that ps will show all processes in both the parent
and the child namespace the shell is running on because the contents
of /proc are the same for both.
However, because we are also using private mount namespaces for the
check process, pid_namespaces(7) tells us:
/proc and PID namespaces
A /proc filesystem shows (in the /proc/pid directories) only
processes visible in the PID namespace of the process that
performed the mount, even if the /proc filesystem is viewed
from processes in other namespaces.
After creating a new PID namespace, it is useful for the
child to change its root directory and mount a new procfs
instance at /proc so that tools such as ps(1) work
>>> correctly. If a new mount namespace is simultaneously
>>> created by including CLONE_NEWNS in the flags argument of
>>> clone(2) or unshare(2), then it isn't necessary to change the
>>> root directory: a new procfs instance can be mounted directly
>>> over /proc.
From a shell, the command to mount /proc is:
$ mount -t proc proc /proc
Calling readlink(2) on the path /proc/self yields the process
ID of the caller in the PID namespace of the procfs mount
(i.e., the PID name‐ space of the process that mounted the
procfs). This can be useful for introspection purposes, when
a process wants to discover its PID in other namespaces.
This appears to give us an environment that only shows the processes
within the current PID namespace:
$ sudo src/nsexec -p -m bash
# mount -t proc proc /proc
# ps waux
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
root 1 0.0 0.0 7384 3744 pts/1 S 11:55 0:00 bash
root 72 0.0 0.0 8300 3736 pts/1 R+ 12:04 0:00 ps waux
# pstree -N pid
[4026538173]
bash───pstree
#
Yup, there we go - we have full PID isolation for this shell.
OK, I suspect this is a better way to proceed for check-parallel than
to need session IDs for individual tests and wrappers for
pgrep/pkill/pidwait. Let me see what breaks when I use this.....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-23 1:16 ` Dave Chinner
@ 2025-01-28 4:34 ` Dave Chinner
2025-01-28 7:23 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-28 4:34 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> On Wed, Jan 22, 2025 at 01:46:09PM -0800, Darrick J. Wong wrote:
> > > > Agreed, though at some point after these bugfixes are merged I'll see if
> > > > I can build on the existing "if you have systemd then ___ else here's
> > > > your shabby opencoded version" logic in fstests to isolate the ./checks
> > > > from each other a little better. It'd be kinda nice if we could
> > > > actually just put them in something resembling a modernish container,
> > > > albeit with the same underlying fs.
> > >
> > > Agreed, but I don't think we need to depend on systemd for that,
> > > either.
> > >
> > > > <shrug> Anyone else interested in that?
> > >
> > > check-parallel has already started down that road with the
> > > mount namespace isolation it uses for the runner tasks via
> > > src/nsexec.c.
.....
> > > Hmmm - looks like src/nsexec.c can create new PID namespaces via
> > > the "-p" option. I haven't used that before - I wonder if that's a
> > > better solution that using per-test session IDs to solve the pkill
> > > --parent problem? Something to look into in the morning....
.....
> Note, however, that ps will show all processes in both the parent
> and the child namespace the shell is running on because the contents
> of /proc are the same for both.
>
> However, because we are also using private mount namespaces for the
> check process, pid_namespaces(7) tells us:
>
> /proc and PID namespaces
>
> A /proc filesystem shows (in the /proc/pid directories) only
> processes visible in the PID namespace of the process that
> performed the mount, even if the /proc filesystem is viewed
> from processes in other namespaces.
>
> After creating a new PID namespace, it is useful for the
> child to change its root directory and mount a new procfs
> instance at /proc so that tools such as ps(1) work
> >>> correctly. If a new mount namespace is simultaneously
> >>> created by including CLONE_NEWNS in the flags argument of
> >>> clone(2) or unshare(2), then it isn't necessary to change the
> >>> root directory: a new procfs instance can be mounted directly
> >>> over /proc.
>
> From a shell, the command to mount /proc is:
>
> $ mount -t proc proc /proc
>
> Calling readlink(2) on the path /proc/self yields the process
> ID of the caller in the PID namespace of the procfs mount
> (i.e., the PID name‐ space of the process that mounted the
> procfs). This can be useful for introspection purposes, when
> a process wants to discover its PID in other namespaces.
>
> This appears to give us an environment that only shows the processes
> within the current PID namespace:
>
> $ sudo src/nsexec -p -m bash
> # mount -t proc proc /proc
> # ps waux
> USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
> root 1 0.0 0.0 7384 3744 pts/1 S 11:55 0:00 bash
> root 72 0.0 0.0 8300 3736 pts/1 R+ 12:04 0:00 ps waux
> # pstree -N pid
> [4026538173]
> bash───pstree
> #
>
> Yup, there we go - we have full PID isolation for this shell.
Ok, it took me some time to get this to work reliably - the way was
full of landmines with little documentation to guide around them.
1. If multiple commands are needed to be run from nsexec, a helper
script is needed (I called it check-helper).
2. You have to `mount --make-private /proc` before doing anything to
make the mounts of /proc inside the pid namespace work correctly.
If you don't do this, every other mount namespace will also see
only the newest mount, and that means every runner but the last
one to start with the wrong mount and PID namespace view in /proc.
3. if you get the system into the state described by 1), unmounting
/proc in each runner then races to remove the top /proc mount.
Many threads get -EBUSY failures from unmount, leaving many stale
mounts of /proc behind.
4. the top level shell where check-parallel was invoked is left with
the view where /proc has been unmounted from a PID/mount
namespace that has gone away. Hence /proc now has no processes or
mounts being reported and nothing works until you mount a new
instance /proc in that namespace.
5. After mounting proc again there are lots of mounts of stale /proc
mounts still reported. They cannot be unmounted as the mount
namespaces that created them have gone away and unmounting /proc
in the current shell simply removes the last mounted one and we
goto 4).
4. /tmp is still shared across all runner instances so all the
concurrent runners dump all their tmp files in /tmp. However, the
runners no longer have unique PIDs (i.e. check runs as PID 3 in
all runner instaces). This means using /tmp/tmp.$$ as the
check/test temp file definition results is instant tmp file name
collisions and random things in check and tests fail. check and
common/preamble have to be converted to use `mktemp` to provide
unique tempfile name prefixes again.
5. Don't forget to revert the parent /proc mount back to shared
after check has finished running (or was aborted).
I think with this (current prototype patch below), we can use PID
namespaces rather than process session IDs for check-parallel safe
process management.
Thoughts?
-Dave.
--
Dave Chinner
david@fromorbit.com
check-parallel: use PID namespaces for runner process isolation
From: Dave Chinner <dchinner@redhat.com>
This provides isolation between individual runners so that they
cannot see the processes that other test runners have created.
This means tools like pkill will only find processes run by the test
that is calling it, hence there is no danger taht it might kill
processes owned by a different test in a different runner context.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
check | 6 +++++-
check-helper | 23 +++++++++++++++++++++++
check-parallel | 25 +++++++++++++++++++------
common/preamble | 5 ++++-
4 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/check b/check
index 4dc266dcf..314436667 100755
--- a/check
+++ b/check
@@ -4,7 +4,11 @@
#
# Control script for QA
#
-tmp=/tmp/$$
+
+# When run from a pid namespace, /tmp/tmp.$$ is not a unique identifier.
+# Use mktemp instead.
+tmp=`mktemp`
+
status=0
needwrap=true
needsum=true
diff --git a/check-helper b/check-helper
new file mode 100755
index 000000000..47a92de8b
--- /dev/null
+++ b/check-helper
@@ -0,0 +1,23 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Red Hat, Inc. All Rights Reserved.
+#
+
+# When check is run from check-parallel, it is run in private mount and PID
+# namespaces. We need to set up /proc to only show processes in this PID
+# namespace, so we mount a new instance of /proc over the top of the existing
+# version. Because we are in a private mount namespace, the does not propagate
+# outside this context and hence does not conflict with the other test runners
+# that are performing the same setup actions.
+
+args="$@"
+
+#echo $args
+mount -t proc proc /proc
+ret=$?
+if [ $ret -eq 0 ]; then
+ ./check $args
+ umount -l /proc
+else
+ echo failed to mount /proc, ret $ret!
+fi
diff --git a/check-parallel b/check-parallel
index 2fbf0fdbe..6082baf5e 100755
--- a/check-parallel
+++ b/check-parallel
@@ -259,14 +259,14 @@ runner_go()
rm -f $RESULT_BASE/check.*
# Only supports default mkfs parameters right now
- wipefs -a $TEST_DEV > /dev/null 2>&1
- yes | mkfs -t $FSTYP $TEST_MKFS_OPTS $TEST_DEV > /dev/null 2>&1
+ wipefs -a $TEST_DEV > $me/log 2>&1
+ yes | mkfs -t $FSTYP $TEST_MKFS_OPTS $TEST_DEV >> $me/log 2>&1
# export DUMP_CORRUPT_FS=1
- # Run the tests in it's own mount namespace, as per the comment below
- # that precedes making the basedir a private mount.
- ./src/nsexec -m ./check $run_section -x unreliable_in_parallel --exact-order ${runner_list[$id]} > $me/log 2>&1
+ # Run the tests in it's own mount and PID namespace, as per the comment
+ # below that precedes making the basedir a private mount.
+ ./src/nsexec -m -p ./check-helper $run_section -x unreliable_in_parallel --exact-order ${runner_list[$id]} >> $me/log 2>&1
wait
sleep 1
@@ -291,6 +291,8 @@ cleanup()
umount -R $basedir/*/test 2> /dev/null
umount -R $basedir/*/scratch 2> /dev/null
losetup --detach-all
+ mount --make-shared /proc
+ mount --make-shared $basedir
}
trap "cleanup; exit" HUP INT QUIT TERM
@@ -311,10 +313,17 @@ fi
# controls the mount to succeed without actually unmounting the filesytsem
# because a mount namespace still holds a reference to it. This causes other
# operations on the block device to fail as it is still busy (e.g. fsck, mkfs,
-# etc). Hence we make the basedir private here and then run each check instance
+# etc).
+#
+# Hence we make the basedir private here and then run each check instance
# in it's own mount namespace so that they cannot see mounts that other tests
# are performing.
+#
+# We also need to make /proc private so that the runners can be run cleanly in
+# a PID namespace. This requires an new mount of /proc inside the PID namespace,
+# and this requires a private mount namespace to work correctly.
mount --make-private $basedir
+mount --make-private /proc
now=`date +%Y-%m-%d-%H:%M:%S`
for ((i = 0; i < $runners; i++)); do
@@ -328,6 +337,10 @@ for ((i = 0; i < $runners; i++)); do
done;
wait
+# Restore the parents back to shared mount namespace behaviour.
+mount --make-shared /proc
+mount --make-shared $basedir
+
if [ -n "$show_test_list" ]; then
exit 0
fi
diff --git a/common/preamble b/common/preamble
index 78e45d522..8f47b172a 100644
--- a/common/preamble
+++ b/common/preamble
@@ -43,8 +43,11 @@ _begin_fstest()
seqres=$RESULT_DIR/$seq
echo "QA output created by $seq"
+ # When run from a pid namespace, /tmp/tmp.$$ is not a unique identifier.
+ # Use mktemp instead.
+ tmp=`mktemp`
+
here=`pwd`
- tmp=/tmp/$$
status=1 # failure is the default!
_register_cleanup _cleanup
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-28 4:34 ` Dave Chinner
@ 2025-01-28 7:23 ` Darrick J. Wong
2025-01-28 20:39 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-28 7:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 28, 2025 at 03:34:50PM +1100, Dave Chinner wrote:
> On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> > On Wed, Jan 22, 2025 at 01:46:09PM -0800, Darrick J. Wong wrote:
> > > > > Agreed, though at some point after these bugfixes are merged I'll see if
> > > > > I can build on the existing "if you have systemd then ___ else here's
> > > > > your shabby opencoded version" logic in fstests to isolate the ./checks
> > > > > from each other a little better. It'd be kinda nice if we could
> > > > > actually just put them in something resembling a modernish container,
> > > > > albeit with the same underlying fs.
> > > >
> > > > Agreed, but I don't think we need to depend on systemd for that,
> > > > either.
> > > >
> > > > > <shrug> Anyone else interested in that?
> > > >
> > > > check-parallel has already started down that road with the
> > > > mount namespace isolation it uses for the runner tasks via
> > > > src/nsexec.c.
> .....
> > > > Hmmm - looks like src/nsexec.c can create new PID namespaces via
> > > > the "-p" option. I haven't used that before - I wonder if that's a
> > > > better solution that using per-test session IDs to solve the pkill
> > > > --parent problem? Something to look into in the morning....
>
> .....
>
> > Note, however, that ps will show all processes in both the parent
> > and the child namespace the shell is running on because the contents
> > of /proc are the same for both.
> >
> > However, because we are also using private mount namespaces for the
> > check process, pid_namespaces(7) tells us:
> >
> > /proc and PID namespaces
> >
> > A /proc filesystem shows (in the /proc/pid directories) only
> > processes visible in the PID namespace of the process that
> > performed the mount, even if the /proc filesystem is viewed
> > from processes in other namespaces.
> >
> > After creating a new PID namespace, it is useful for the
> > child to change its root directory and mount a new procfs
> > instance at /proc so that tools such as ps(1) work
> > >>> correctly. If a new mount namespace is simultaneously
> > >>> created by including CLONE_NEWNS in the flags argument of
> > >>> clone(2) or unshare(2), then it isn't necessary to change the
> > >>> root directory: a new procfs instance can be mounted directly
> > >>> over /proc.
> >
> > From a shell, the command to mount /proc is:
> >
> > $ mount -t proc proc /proc
> >
> > Calling readlink(2) on the path /proc/self yields the process
> > ID of the caller in the PID namespace of the procfs mount
> > (i.e., the PID name‐ space of the process that mounted the
> > procfs). This can be useful for introspection purposes, when
> > a process wants to discover its PID in other namespaces.
> >
> > This appears to give us an environment that only shows the processes
> > within the current PID namespace:
> >
> > $ sudo src/nsexec -p -m bash
> > # mount -t proc proc /proc
> > # ps waux
> > USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
> > root 1 0.0 0.0 7384 3744 pts/1 S 11:55 0:00 bash
> > root 72 0.0 0.0 8300 3736 pts/1 R+ 12:04 0:00 ps waux
> > # pstree -N pid
> > [4026538173]
> > bash───pstree
> > #
> >
> > Yup, there we go - we have full PID isolation for this shell.
>
> Ok, it took me some time to get this to work reliably - the way was
> full of landmines with little documentation to guide around them.
>
> 1. If multiple commands are needed to be run from nsexec, a helper
> script is needed (I called it check-helper).
>
> 2. You have to `mount --make-private /proc` before doing anything to
> make the mounts of /proc inside the pid namespace work correctly.
> If you don't do this, every other mount namespace will also see
> only the newest mount, and that means every runner but the last
> one to start with the wrong mount and PID namespace view in /proc.
>
> 3. if you get the system into the state described by 1), unmounting
> /proc in each runner then races to remove the top /proc mount.
> Many threads get -EBUSY failures from unmount, leaving many stale
> mounts of /proc behind.
>
> 4. the top level shell where check-parallel was invoked is left with
> the view where /proc has been unmounted from a PID/mount
> namespace that has gone away. Hence /proc now has no processes or
> mounts being reported and nothing works until you mount a new
> instance /proc in that namespace.
>
> 5. After mounting proc again there are lots of mounts of stale /proc
> mounts still reported. They cannot be unmounted as the mount
> namespaces that created them have gone away and unmounting /proc
> in the current shell simply removes the last mounted one and we
> goto 4).
>
> 4. /tmp is still shared across all runner instances so all the
>
> concurrent runners dump all their tmp files in /tmp. However, the
> runners no longer have unique PIDs (i.e. check runs as PID 3 in
> all runner instaces). This means using /tmp/tmp.$$ as the
> check/test temp file definition results is instant tmp file name
> collisions and random things in check and tests fail. check and
> common/preamble have to be converted to use `mktemp` to provide
> unique tempfile name prefixes again.
>
> 5. Don't forget to revert the parent /proc mount back to shared
> after check has finished running (or was aborted).
>
> I think with this (current prototype patch below), we can use PID
> namespaces rather than process session IDs for check-parallel safe
> process management.
>
> Thoughts?
Was about to go to bed, but can we also start a new mount namespace,
create a private (or at least non-global) /tmp to put files into, and
then each test instance is isolated from clobbering the /tmpfiles of
other ./check instances *and* the rest of the system?
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
>
> check-parallel: use PID namespaces for runner process isolation
>
> From: Dave Chinner <dchinner@redhat.com>
>
> This provides isolation between individual runners so that they
> cannot see the processes that other test runners have created.
> This means tools like pkill will only find processes run by the test
> that is calling it, hence there is no danger taht it might kill
> processes owned by a different test in a different runner context.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> check | 6 +++++-
> check-helper | 23 +++++++++++++++++++++++
> check-parallel | 25 +++++++++++++++++++------
> common/preamble | 5 ++++-
> 4 files changed, 51 insertions(+), 8 deletions(-)
>
> diff --git a/check b/check
> index 4dc266dcf..314436667 100755
> --- a/check
> +++ b/check
> @@ -4,7 +4,11 @@
> #
> # Control script for QA
> #
> -tmp=/tmp/$$
> +
> +# When run from a pid namespace, /tmp/tmp.$$ is not a unique identifier.
> +# Use mktemp instead.
> +tmp=`mktemp`
> +
> status=0
> needwrap=true
> needsum=true
> diff --git a/check-helper b/check-helper
> new file mode 100755
> index 000000000..47a92de8b
> --- /dev/null
> +++ b/check-helper
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2025 Red Hat, Inc. All Rights Reserved.
> +#
> +
> +# When check is run from check-parallel, it is run in private mount and PID
> +# namespaces. We need to set up /proc to only show processes in this PID
> +# namespace, so we mount a new instance of /proc over the top of the existing
> +# version. Because we are in a private mount namespace, the does not propagate
> +# outside this context and hence does not conflict with the other test runners
> +# that are performing the same setup actions.
> +
> +args="$@"
> +
> +#echo $args
> +mount -t proc proc /proc
> +ret=$?
> +if [ $ret -eq 0 ]; then
> + ./check $args
> + umount -l /proc
> +else
> + echo failed to mount /proc, ret $ret!
> +fi
> diff --git a/check-parallel b/check-parallel
> index 2fbf0fdbe..6082baf5e 100755
> --- a/check-parallel
> +++ b/check-parallel
> @@ -259,14 +259,14 @@ runner_go()
> rm -f $RESULT_BASE/check.*
>
> # Only supports default mkfs parameters right now
> - wipefs -a $TEST_DEV > /dev/null 2>&1
> - yes | mkfs -t $FSTYP $TEST_MKFS_OPTS $TEST_DEV > /dev/null 2>&1
> + wipefs -a $TEST_DEV > $me/log 2>&1
> + yes | mkfs -t $FSTYP $TEST_MKFS_OPTS $TEST_DEV >> $me/log 2>&1
>
> # export DUMP_CORRUPT_FS=1
>
> - # Run the tests in it's own mount namespace, as per the comment below
> - # that precedes making the basedir a private mount.
> - ./src/nsexec -m ./check $run_section -x unreliable_in_parallel --exact-order ${runner_list[$id]} > $me/log 2>&1
> + # Run the tests in it's own mount and PID namespace, as per the comment
> + # below that precedes making the basedir a private mount.
> + ./src/nsexec -m -p ./check-helper $run_section -x unreliable_in_parallel --exact-order ${runner_list[$id]} >> $me/log 2>&1
>
> wait
> sleep 1
> @@ -291,6 +291,8 @@ cleanup()
> umount -R $basedir/*/test 2> /dev/null
> umount -R $basedir/*/scratch 2> /dev/null
> losetup --detach-all
> + mount --make-shared /proc
> + mount --make-shared $basedir
> }
>
> trap "cleanup; exit" HUP INT QUIT TERM
> @@ -311,10 +313,17 @@ fi
> # controls the mount to succeed without actually unmounting the filesytsem
> # because a mount namespace still holds a reference to it. This causes other
> # operations on the block device to fail as it is still busy (e.g. fsck, mkfs,
> -# etc). Hence we make the basedir private here and then run each check instance
> +# etc).
> +#
> +# Hence we make the basedir private here and then run each check instance
> # in it's own mount namespace so that they cannot see mounts that other tests
> # are performing.
> +#
> +# We also need to make /proc private so that the runners can be run cleanly in
> +# a PID namespace. This requires an new mount of /proc inside the PID namespace,
> +# and this requires a private mount namespace to work correctly.
> mount --make-private $basedir
> +mount --make-private /proc
>
> now=`date +%Y-%m-%d-%H:%M:%S`
> for ((i = 0; i < $runners; i++)); do
> @@ -328,6 +337,10 @@ for ((i = 0; i < $runners; i++)); do
> done;
> wait
>
> +# Restore the parents back to shared mount namespace behaviour.
> +mount --make-shared /proc
> +mount --make-shared $basedir
> +
> if [ -n "$show_test_list" ]; then
> exit 0
> fi
> diff --git a/common/preamble b/common/preamble
> index 78e45d522..8f47b172a 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -43,8 +43,11 @@ _begin_fstest()
> seqres=$RESULT_DIR/$seq
> echo "QA output created by $seq"
>
> + # When run from a pid namespace, /tmp/tmp.$$ is not a unique identifier.
> + # Use mktemp instead.
> + tmp=`mktemp`
> +
> here=`pwd`
> - tmp=/tmp/$$
> status=1 # failure is the default!
>
> _register_cleanup _cleanup
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-28 7:23 ` Darrick J. Wong
@ 2025-01-28 20:39 ` Dave Chinner
2025-01-29 3:13 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-28 20:39 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Mon, Jan 27, 2025 at 11:23:52PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 28, 2025 at 03:34:50PM +1100, Dave Chinner wrote:
> > On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> > 4. /tmp is still shared across all runner instances so all the
> >
> > concurrent runners dump all their tmp files in /tmp. However, the
> > runners no longer have unique PIDs (i.e. check runs as PID 3 in
> > all runner instaces). This means using /tmp/tmp.$$ as the
> > check/test temp file definition results is instant tmp file name
> > collisions and random things in check and tests fail. check and
> > common/preamble have to be converted to use `mktemp` to provide
> > unique tempfile name prefixes again.
> >
> > 5. Don't forget to revert the parent /proc mount back to shared
> > after check has finished running (or was aborted).
> >
> > I think with this (current prototype patch below), we can use PID
> > namespaces rather than process session IDs for check-parallel safe
> > process management.
> >
> > Thoughts?
>
> Was about to go to bed, but can we also start a new mount namespace,
> create a private (or at least non-global) /tmp to put files into, and
> then each test instance is isolated from clobbering the /tmpfiles of
> other ./check instances *and* the rest of the system?
We probably can. I didn't want to go down that rat hole straight
away, because then I'd have to make a decision about what to mount
there. One thing at a time....
I suspect that I can just use a tmpfs filesystem for it - there's
heaps of memory available on my test machines and we don't use /tmp
to hold large files, so that should work fine for me. However, I'm
a little concerned about what will happen when testing under memory
pressure situations if /tmp needs memory to operate correctly.
I'll have a look at what is needed for private tmpfs /tmp instances
to work - it should work just fine.
However, if check-parallel has taught me anything, it is that trying
to use "should work" features on a modern system tends to mean "this
is a poorly documented rat-hole that with many dead-ends that will
be explored before a working solution is found"...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-28 20:39 ` Dave Chinner
@ 2025-01-29 3:13 ` Darrick J. Wong
2025-01-29 6:06 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-29 3:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 29, 2025 at 07:39:22AM +1100, Dave Chinner wrote:
> On Mon, Jan 27, 2025 at 11:23:52PM -0800, Darrick J. Wong wrote:
> > On Tue, Jan 28, 2025 at 03:34:50PM +1100, Dave Chinner wrote:
> > > On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> > > 4. /tmp is still shared across all runner instances so all the
> > >
> > > concurrent runners dump all their tmp files in /tmp. However, the
> > > runners no longer have unique PIDs (i.e. check runs as PID 3 in
> > > all runner instaces). This means using /tmp/tmp.$$ as the
> > > check/test temp file definition results is instant tmp file name
> > > collisions and random things in check and tests fail. check and
> > > common/preamble have to be converted to use `mktemp` to provide
> > > unique tempfile name prefixes again.
> > >
> > > 5. Don't forget to revert the parent /proc mount back to shared
> > > after check has finished running (or was aborted).
> > >
> > > I think with this (current prototype patch below), we can use PID
> > > namespaces rather than process session IDs for check-parallel safe
> > > process management.
> > >
> > > Thoughts?
> >
> > Was about to go to bed, but can we also start a new mount namespace,
> > create a private (or at least non-global) /tmp to put files into, and
> > then each test instance is isolated from clobbering the /tmpfiles of
> > other ./check instances *and* the rest of the system?
>
> We probably can. I didn't want to go down that rat hole straight
> away, because then I'd have to make a decision about what to mount
> there. One thing at a time....
>
> I suspect that I can just use a tmpfs filesystem for it - there's
> heaps of memory available on my test machines and we don't use /tmp
> to hold large files, so that should work fine for me. However, I'm
> a little concerned about what will happen when testing under memory
> pressure situations if /tmp needs memory to operate correctly.
>
> I'll have a look at what is needed for private tmpfs /tmp instances
> to work - it should work just fine.
>
> However, if check-parallel has taught me anything, it is that trying
> to use "should work" features on a modern system tends to mean "this
> is a poorly documented rat-hole that with many dead-ends that will
> be explored before a working solution is found"...
<nod> I'm running an experiment overnight with the following patch to
get rid of the session id grossness. AFAICT it can also be used by
check-parallel to start its subprocesses in separate pid namespaces,
though I didn't investigate thoroughly.
I'm also not sure it's required for check-helper to unmount the /proc
that it creates; merely exiting seems to clean everything up? <shrug>
I also tried using systemd-nspawn to run fstests inside a "container"
but very quickly discovered that you can't pass block devices to the
container which makes fstests pretty useless for testing real scsi
devices. :/
--D
check: run tests in a private pid/mount namespace
Experiment with running tests in a private pid/mount namespace for
better isolation of the scheduler (check) vs the test (./$seq). This
also makes it cleaner to adjust the oom score and is a convenient place
to set up the environment before invoking the test.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
check | 30 +++++-------------------------
check-helper | 31 +++++++++++++++++++++++++++++++
common/rc | 8 +++-----
3 files changed, 39 insertions(+), 30 deletions(-)
create mode 100755 check-helper
diff --git a/check b/check
index 00ee5af2a31e34..9c70f6f1e10110 100755
--- a/check
+++ b/check
@@ -698,41 +698,21 @@ _adjust_oom_score -500
# systemd doesn't automatically remove transient scopes that fail to terminate
# when systemd tells them to terminate (e.g. programs stuck in D state when
# systemd sends SIGKILL), so we use reset-failed to tear down the scope.
-#
-# Use setsid to run the test program with a separate session id so that we
-# can pkill only the processes started by this test.
_run_seq() {
- local cmd=(bash -c "test -w ${OOM_SCORE_ADJ} && echo 250 > ${OOM_SCORE_ADJ}; exec setsid bash ./$seq")
+ local cmd=("./check-helper" "./$seq")
if [ -n "${HAVE_SYSTEMD_SCOPES}" ]; then
local unit="$(systemd-escape "fs$seq").scope"
systemctl reset-failed "${unit}" &> /dev/null
- systemd-run --quiet --unit "${unit}" --scope "${cmd[@]}" &
- CHILDPID=$!
- wait
+ systemd-run --quiet --unit "${unit}" --scope "${cmd[@]}"
res=$?
- unset CHILDPID
systemctl stop "${unit}" &> /dev/null
return "${res}"
else
# bash won't run the SIGINT trap handler while there are
# foreground children in a separate session, so we must run
# the test in the background and wait for it.
- "${cmd[@]}" &
- CHILDPID=$!
- wait
- unset CHILDPID
- fi
-}
-
-_kill_seq() {
- if [ -n "$CHILDPID" ]; then
- # SIGPIPE will kill all the children (including fsstress)
- # without bash logging fatal signal termination messages to the
- # console
- pkill -PIPE --session "$CHILDPID"
- wait
- unset CHILDPID
+ "${cmd[@]}"
fi
}
@@ -741,9 +721,9 @@ _prepare_test_list
fstests_start_time="$(date +"%F %T")"
if $OPTIONS_HAVE_SECTIONS; then
- trap "_kill_seq; _summary; exit \$status" 0 1 2 3 15
+ trap "_summary; exit \$status" 0 1 2 3 15
else
- trap "_kill_seq; _wrapup; exit \$status" 0 1 2 3 15
+ trap "_wrapup; exit \$status" 0 1 2 3 15
fi
function run_section()
diff --git a/check-helper b/check-helper
new file mode 100755
index 00000000000000..2cc8dbe5cfc791
--- /dev/null
+++ b/check-helper
@@ -0,0 +1,31 @@
+#!/bin/bash
+
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Oracle. All Rights Reserved.
+#
+# Try starting things in a private pid/mount namespace with a private /tmp
+# and /proc so that child process trees cannot interfere with each other.
+
+if [ -n "${IN_NSEXEC}" ]; then
+ for path in /proc /tmp; do
+ mount --make-private "$path"
+ done
+ unset IN_NSEXEC
+ mount -t proc proc /proc
+ mount -t tmpfs tmpfs /tmp
+
+ oom_knob="/proc/self/oom_score_adj"
+ test -w "${oom_knob}" && echo 250 > "${oom_knob}"
+
+ # Run the test, but don't let it be pid 1 because that will confuse
+ # the filter functions in common/dump.
+ "$@"
+ exit
+fi
+
+if [ -z "$1" ] || [ "$1" = "--help" ]; then
+ echo "Usage: $0 command [args...]"
+ exit 1
+fi
+
+IN_NSEXEC=1 exec "$(dirname "$0")/src/nsexec" -m -p $0 "$@"
diff --git a/common/rc b/common/rc
index 1c28a2d190f5a0..cc080ecaa9f801 100644
--- a/common/rc
+++ b/common/rc
@@ -33,13 +33,13 @@ _test_sync()
# Kill only the test processes started by this test
_pkill()
{
- pkill --session 0 "$@"
+ pkill "$@"
}
# Find only the test processes started by this test
_pgrep()
{
- pgrep --session 0 "$@"
+ pgrep "$@"
}
# Common execution handling for fsstress invocation.
@@ -2817,11 +2817,9 @@ _require_user_exists()
[ "$?" == "0" ] || _notrun "$user user not defined."
}
-# Run all non-root processes in the same session as the root. Believe it or
-# not, passing $SHELL in this manner works both for "su" and "su -c cmd".
_su()
{
- su --session-command $SHELL "$@"
+ su "$@"
}
# check if a user exists and is able to execute commands.
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-29 3:13 ` Darrick J. Wong
@ 2025-01-29 6:06 ` Dave Chinner
2025-01-29 7:33 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-29 6:06 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 28, 2025 at 07:13:13PM -0800, Darrick J. Wong wrote:
> On Wed, Jan 29, 2025 at 07:39:22AM +1100, Dave Chinner wrote:
> > On Mon, Jan 27, 2025 at 11:23:52PM -0800, Darrick J. Wong wrote:
> > > On Tue, Jan 28, 2025 at 03:34:50PM +1100, Dave Chinner wrote:
> > > > On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> > > > 4. /tmp is still shared across all runner instances so all the
> > > >
> > > > concurrent runners dump all their tmp files in /tmp. However, the
> > > > runners no longer have unique PIDs (i.e. check runs as PID 3 in
> > > > all runner instaces). This means using /tmp/tmp.$$ as the
> > > > check/test temp file definition results is instant tmp file name
> > > > collisions and random things in check and tests fail. check and
> > > > common/preamble have to be converted to use `mktemp` to provide
> > > > unique tempfile name prefixes again.
> > > >
> > > > 5. Don't forget to revert the parent /proc mount back to shared
> > > > after check has finished running (or was aborted).
> > > >
> > > > I think with this (current prototype patch below), we can use PID
> > > > namespaces rather than process session IDs for check-parallel safe
> > > > process management.
> > > >
> > > > Thoughts?
> > >
> > > Was about to go to bed, but can we also start a new mount namespace,
> > > create a private (or at least non-global) /tmp to put files into, and
> > > then each test instance is isolated from clobbering the /tmpfiles of
> > > other ./check instances *and* the rest of the system?
> >
> > We probably can. I didn't want to go down that rat hole straight
> > away, because then I'd have to make a decision about what to mount
> > there. One thing at a time....
> >
> > I suspect that I can just use a tmpfs filesystem for it - there's
> > heaps of memory available on my test machines and we don't use /tmp
> > to hold large files, so that should work fine for me. However, I'm
> > a little concerned about what will happen when testing under memory
> > pressure situations if /tmp needs memory to operate correctly.
> >
> > I'll have a look at what is needed for private tmpfs /tmp instances
> > to work - it should work just fine.
> >
> > However, if check-parallel has taught me anything, it is that trying
> > to use "should work" features on a modern system tends to mean "this
> > is a poorly documented rat-hole that with many dead-ends that will
> > be explored before a working solution is found"...
>
> <nod> I'm running an experiment overnight with the following patch to
> get rid of the session id grossness. AFAICT it can also be used by
> check-parallel to start its subprocesses in separate pid namespaces,
> though I didn't investigate thoroughly.
I don't think check-parallel needs to start each check instance in
it's own PID namespace - it's the tests themselves that need the
isolation from each other.
However, the check instances require a private mount namespace, as
they mount and unmount test/scratch devices themselves and we do not
want other check instances seeing those mounts.
Hence I think the current check-parallel code doing mount namespace
isolation as it already does should work with this patch enabling
per-test process isolation inside check itself.
> I'm also not sure it's required for check-helper to unmount the /proc
> that it creates; merely exiting seems to clean everything up? <shrug>
Yeah, I think tearing down the mount namespace (i.e. exiting the
process that nsexec created) drops the last active reference to the
mounts inside the private namespace and so it gets torn down that
way.
So from my perspective, I think your check-helper namespace patch is
a good improvement and I'll build/fix anything I come across on top
of it. Once your series of fixes goes in, I'll rebase all the stuff
I've got on top it and go from there...
> I also tried using systemd-nspawn to run fstests inside a "container"
> but very quickly discovered that you can't pass block devices to the
> container which makes fstests pretty useless for testing real scsi
> devices. :/
Yet another dead-end in the poorly sign-posted rat-hole, eh?
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 08/23] common: fix pkill by running test program in a separate session
2025-01-29 6:06 ` Dave Chinner
@ 2025-01-29 7:33 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-29 7:33 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 29, 2025 at 05:06:52PM +1100, Dave Chinner wrote:
> On Tue, Jan 28, 2025 at 07:13:13PM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 29, 2025 at 07:39:22AM +1100, Dave Chinner wrote:
> > > On Mon, Jan 27, 2025 at 11:23:52PM -0800, Darrick J. Wong wrote:
> > > > On Tue, Jan 28, 2025 at 03:34:50PM +1100, Dave Chinner wrote:
> > > > > On Thu, Jan 23, 2025 at 12:16:50PM +1100, Dave Chinner wrote:
> > > > > 4. /tmp is still shared across all runner instances so all the
> > > > >
> > > > > concurrent runners dump all their tmp files in /tmp. However, the
> > > > > runners no longer have unique PIDs (i.e. check runs as PID 3 in
> > > > > all runner instaces). This means using /tmp/tmp.$$ as the
> > > > > check/test temp file definition results is instant tmp file name
> > > > > collisions and random things in check and tests fail. check and
> > > > > common/preamble have to be converted to use `mktemp` to provide
> > > > > unique tempfile name prefixes again.
> > > > >
> > > > > 5. Don't forget to revert the parent /proc mount back to shared
> > > > > after check has finished running (or was aborted).
> > > > >
> > > > > I think with this (current prototype patch below), we can use PID
> > > > > namespaces rather than process session IDs for check-parallel safe
> > > > > process management.
> > > > >
> > > > > Thoughts?
> > > >
> > > > Was about to go to bed, but can we also start a new mount namespace,
> > > > create a private (or at least non-global) /tmp to put files into, and
> > > > then each test instance is isolated from clobbering the /tmpfiles of
> > > > other ./check instances *and* the rest of the system?
> > >
> > > We probably can. I didn't want to go down that rat hole straight
> > > away, because then I'd have to make a decision about what to mount
> > > there. One thing at a time....
> > >
> > > I suspect that I can just use a tmpfs filesystem for it - there's
> > > heaps of memory available on my test machines and we don't use /tmp
> > > to hold large files, so that should work fine for me. However, I'm
> > > a little concerned about what will happen when testing under memory
> > > pressure situations if /tmp needs memory to operate correctly.
> > >
> > > I'll have a look at what is needed for private tmpfs /tmp instances
> > > to work - it should work just fine.
> > >
> > > However, if check-parallel has taught me anything, it is that trying
> > > to use "should work" features on a modern system tends to mean "this
> > > is a poorly documented rat-hole that with many dead-ends that will
> > > be explored before a working solution is found"...
> >
> > <nod> I'm running an experiment overnight with the following patch to
> > get rid of the session id grossness. AFAICT it can also be used by
> > check-parallel to start its subprocesses in separate pid namespaces,
> > though I didn't investigate thoroughly.
>
> I don't think check-parallel needs to start each check instance in
> it's own PID namespace - it's the tests themselves that need the
> isolation from each other.
>
> However, the check instances require a private mount namespace, as
> they mount and unmount test/scratch devices themselves and we do not
> want other check instances seeing those mounts.
>
> Hence I think the current check-parallel code doing mount namespace
> isolation as it already does should work with this patch enabling
> per-test process isolation inside check itself.
>
> > I'm also not sure it's required for check-helper to unmount the /proc
> > that it creates; merely exiting seems to clean everything up? <shrug>
>
> Yeah, I think tearing down the mount namespace (i.e. exiting the
> process that nsexec created) drops the last active reference to the
> mounts inside the private namespace and so it gets torn down that
> way.
>
> So from my perspective, I think your check-helper namespace patch is
> a good improvement and I'll build/fix anything I come across on top
> of it. Once your series of fixes goes in, I'll rebase all the stuff
> I've got on top it and go from there...
<nod> I might reformulate the pkill code to use nsexec (and not systemd)
if it's available; systemd scopes if those are available (I figured out
how to get systemd to tell me the cgroup name); or worst case fall back
to process sessions if neither are available.
I don't know how ancient of a userspace we realistically have to support
since (afaict) namespaces and systemd both showed up around the 2.6.24
era? But I also don't know if Devuan at least does pid/mount
namespaces.
--D
> > I also tried using systemd-nspawn to run fstests inside a "container"
> > but very quickly discovered that you can't pass block devices to the
> > container which makes fstests pretty useless for testing real scsi
> > devices. :/
>
> Yet another dead-end in the poorly sign-posted rat-hole, eh?
Yup.
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (7 preceding siblings ...)
2025-01-16 23:27 ` [PATCH 08/23] common: fix pkill by running test program in a separate session Darrick J. Wong
@ 2025-01-16 23:27 ` Darrick J. Wong
2025-01-21 3:52 ` Dave Chinner
2025-01-16 23:27 ` [PATCH 10/23] mkfs: don't hardcode log size Darrick J. Wong
` (13 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:27 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
There's a number of places where a test program calls a variant of
_unmount but then pipes the output through a _filter script or
something. The new _unmount helper redirects stdout and stderr to
seqres.full, which means that those error messages (some of which are
encoded in the golden outputs) are suppressed. This leads to test
regressions in generic/050 and other places, so let's resume logging.
This also undoes all the changes that removed /dev/null redirection of
unmount calls.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 4c6bc4565105e6 ("fstests: clean up mount and unmount operations")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
check | 10 ++++++++--
common/quota | 2 +-
common/rc | 21 +++++++++++++++++++--
tests/generic/050 | 2 +-
tests/generic/085 | 2 +-
tests/generic/361 | 4 ++--
tests/generic/590 | 2 +-
tests/generic/746 | 2 +-
tests/xfs/149 | 2 +-
tests/xfs/530 | 2 +-
10 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/check b/check
index bafe48bf12db67..126a77441d700d 100755
--- a/check
+++ b/check
@@ -1026,8 +1026,8 @@ function run_section()
if [ $sts -ne 0 ]; then
_dump_err_cont "[failed, exit status $sts]"
- _test_unmount 2>> $seqres.full
- _scratch_unmount 2>> $seqres.full
+ _test_unmount 2> /dev/null
+ _scratch_unmount 2> /dev/null
rm -f ${RESULT_DIR}/require_test*
rm -f ${RESULT_DIR}/require_scratch*
# Even though we failed, there may be something interesting in
@@ -1113,6 +1113,12 @@ function run_section()
_stash_test_status "$seqnum" "$tc_status"
done
+ # Reset these three variables so that unmount output doesn't get
+ # written to $seqres.full of the last test to run.
+ seq="check.$$"
+ check="$RESULT_BASE/check"
+ seqres="$check"
+
sect_stop=`_wallclock`
interrupt=false
_wrapup
diff --git a/common/quota b/common/quota
index 8688116c6547a9..4dad9b79a27a7f 100644
--- a/common/quota
+++ b/common/quota
@@ -274,7 +274,7 @@ _choose_prid()
_qmount()
{
- _scratch_unmount
+ _scratch_unmount >/dev/null 2>&1
_try_scratch_mount || _fail "qmount failed"
# xfs doesn't need these setups and quotacheck even fails on xfs
# redirect the output to $seqres.full for debug purpose and ignore results
diff --git a/common/rc b/common/rc
index d143ba36265c6c..9e34c301b0deb0 100644
--- a/common/rc
+++ b/common/rc
@@ -480,11 +480,28 @@ _scratch_mount_idmapped()
}
# Unmount the filesystem based on the directory or device passed.
+# Log everything that happens to seqres.full, and use BASHPID because
+# background subshells have the same $$ as the parent but not the same
+# $BASHPID.
_unmount()
{
- local args="$*"
+ local outlog="$tmp.$BASHPID.umount"
+ local errlog="$tmp.$BASHPID.umount.err"
- $UMOUNT_PROG $args >> $seqres.full 2>&1
+ rm -f "$outlog" "$errlog"
+ $UMOUNT_PROG "$@" 2> "$errlog" > "$outlog"
+ local res="${PIPESTATUS[0]}"
+
+ if [ -s "$outlog" ]; then
+ cat "$outlog" >> $seqres.full
+ cat "$outlog"
+ fi
+ if [ -s "$errlog" ]; then
+ cat "$errlog" >> $seqres.full
+ >&2 cat "$errlog"
+ fi
+ rm -f "$outlog" "$errlog"
+ return $res
}
_scratch_unmount()
diff --git a/tests/generic/050 b/tests/generic/050
index 8e9456db279003..affb072df5969f 100755
--- a/tests/generic/050
+++ b/tests/generic/050
@@ -89,7 +89,7 @@ _try_scratch_mount 2>&1 | _filter_ro_mount | _filter_scratch
# expects an error, so open code the unmount
echo "unmounting read-only filesystem"
-$UMOUNT_PROG $SCRATCH_DEV 2>&1 | _filter_scratch | _filter_ending_dot
+_scratch_unmount 2>&1 | _filter_scratch | _filter_ending_dot
#
# This is the way out if the underlying device really is read-only.
diff --git a/tests/generic/085 b/tests/generic/085
index 7671a36ab9524f..d3fa10be9ccace 100755
--- a/tests/generic/085
+++ b/tests/generic/085
@@ -29,7 +29,7 @@ cleanup_dmdev()
fi
# in case it's still suspended and/or mounted
$DMSETUP_PROG resume $lvdev >> $seqres.full 2>&1
- _unmount -q $SCRATCH_MNT
+ _unmount -q $SCRATCH_MNT >/dev/null 2>&1
_dmsetup_remove $node
}
diff --git a/tests/generic/361 b/tests/generic/361
index e2b7984361e87c..b584af47540020 100755
--- a/tests/generic/361
+++ b/tests/generic/361
@@ -16,7 +16,7 @@ _begin_fstest auto quick
# Override the default cleanup function.
_cleanup()
{
- _unmount $fs_mnt
+ _unmount $fs_mnt &>> /dev/null
[ -n "$loop_dev" ] && _destroy_loop_device $loop_dev
cd /
rm -f $tmp.*
@@ -54,7 +54,7 @@ $XFS_IO_PROG -fc "pwrite 0 520m" $fs_mnt/testfile >>$seqres.full 2>&1
# remount should not hang
$MOUNT_PROG -o remount,ro $fs_mnt >>$seqres.full 2>&1
-_unmount $fs_mnt
+_unmount $fs_mnt &>/dev/null
_destroy_loop_device $loop_dev
unset loop_dev
diff --git a/tests/generic/590 b/tests/generic/590
index 1adeef4c2ad52c..ba1337a856f15d 100755
--- a/tests/generic/590
+++ b/tests/generic/590
@@ -15,7 +15,7 @@ _begin_fstest auto prealloc preallocrw
# Override the default cleanup function.
_cleanup()
{
- _scratch_unmount
+ _scratch_unmount &>/dev/null
[ -n "$loop_dev" ] && _destroy_loop_device $loop_dev
cd /
rm -f $tmp.*
diff --git a/tests/generic/746 b/tests/generic/746
index ba8ed25e845776..6f02b1cc354782 100755
--- a/tests/generic/746
+++ b/tests/generic/746
@@ -223,7 +223,7 @@ while read line; do
done < $fiemap_after
echo "done."
-_unmount $loop_mnt
+_unmount $loop_mnt &>/dev/null
_destroy_loop_device $loop_dev
unset loop_dev
diff --git a/tests/xfs/149 b/tests/xfs/149
index 9a96f82ede1761..28dfc7f04c1773 100755
--- a/tests/xfs/149
+++ b/tests/xfs/149
@@ -22,7 +22,7 @@ loop_symlink=$TEST_DIR/loop_symlink.$$
# Override the default cleanup function.
_cleanup()
{
- _unmount $mntdir
+ _unmount $mntdir &>/dev/null
[ -n "$loop_dev" ] && _destroy_loop_device $loop_dev
rmdir $mntdir
rm -f $loop_symlink
diff --git a/tests/xfs/530 b/tests/xfs/530
index d0d0e2665070f8..95ab32f1e1f828 100755
--- a/tests/xfs/530
+++ b/tests/xfs/530
@@ -116,7 +116,7 @@ done
echo "Check filesystem"
_check_scratch_fs
-_scratch_unmount
+_scratch_unmount &> /dev/null
_destroy_loop_device $rt_loop_dev
unset rt_loop_dev
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering
2025-01-16 23:27 ` [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering Darrick J. Wong
@ 2025-01-21 3:52 ` Dave Chinner
2025-01-22 3:29 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:52 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:27:31PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> There's a number of places where a test program calls a variant of
> _unmount but then pipes the output through a _filter script or
> something. The new _unmount helper redirects stdout and stderr to
> seqres.full, which means that those error messages (some of which are
> encoded in the golden outputs) are suppressed. This leads to test
> regressions in generic/050 and other places, so let's resume logging.
Huh. g/050 hasn't been failing in my test runs. Bizarre.
Anyway, change looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering
2025-01-21 3:52 ` Dave Chinner
@ 2025-01-22 3:29 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 02:52:35PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:27:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > There's a number of places where a test program calls a variant of
> > _unmount but then pipes the output through a _filter script or
> > something. The new _unmount helper redirects stdout and stderr to
> > seqres.full, which means that those error messages (some of which are
> > encoded in the golden outputs) are suppressed. This leads to test
> > regressions in generic/050 and other places, so let's resume logging.
>
> Huh. g/050 hasn't been failing in my test runs. Bizarre.
Yeah, it's bombing out here on the ro block device while trying to
enable quotas.
> Anyway, change looks fine.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
Thanks!
--D
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 10/23] mkfs: don't hardcode log size
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (8 preceding siblings ...)
2025-01-16 23:27 ` [PATCH 09/23] unmount: resume logging of stdout and stderr for filtering Darrick J. Wong
@ 2025-01-16 23:27 ` Darrick J. Wong
2025-01-21 3:58 ` Dave Chinner
2025-01-16 23:28 ` [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown Darrick J. Wong
` (12 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:27 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Commit 000813899afb46 hardcoded a log size of 256MB into xfs/501,
xfs/502, and generic/530. This seems to be an attempt to reduce test
run times by increasing the log size so that more background threads can
run in parallel. Unfortunately, this breaks a couple of my test
configurations:
- External logs smaller than 256MB
- Internal logs where the AG size is less than 256MB
For example, here's seqres.full from a failed xfs/501 invocation:
** mkfs failed with extra mkfs options added to " -m metadir=2,autofsck=1,uquota,gquota,pquota, -d rtinherit=1," by test 501 **
** attempting to mkfs using only test 501 options: -l size=256m **
size 256m specified for log subvolume is too large, maximum is 32768 blocks
<snip>
mount -ortdev=/dev/sdb4 -ologdev=/dev/sdb2 /dev/sda4 /opt failed
umount: /dev/sda4: not mounted.
Note that there's some formatting error here, so we jettison the entire
rt configuration to force the log size option, but then mount fails
because we didn't edit out the rtdev option there too.
Fortunately, mkfs.xfs already /has/ a few options to try to improve
parallelism in the filesystem by avoiding contention on the log grant
heads by scaling up the log size. These options are aware of log and AG
size constraints so they won't conflict with other geometry options.
Use them.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 000813899afb46 ("fstests: scale some tests for high CPU count sanity")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/rc | 27 +++++++++++++++++++++++++++
tests/generic/530 | 6 +-----
tests/generic/531 | 6 +-----
tests/xfs/501 | 2 +-
tests/xfs/502 | 2 +-
5 files changed, 31 insertions(+), 12 deletions(-)
diff --git a/common/rc b/common/rc
index 9e34c301b0deb0..885669beeb5e26 100644
--- a/common/rc
+++ b/common/rc
@@ -689,6 +689,33 @@ _test_cycle_mount()
_test_mount
}
+# Are there mkfs options to try to improve concurrency?
+_scratch_mkfs_concurrency_options()
+{
+ local nr_cpus="$(( $1 * LOAD_FACTOR ))"
+
+ case "$FSTYP" in
+ xfs)
+ # If any concurrency options are already specified, don't
+ # compute our own conflicting ones.
+ echo "$SCRATCH_OPTIONS $MKFS_OPTIONS" | \
+ grep -q 'concurrency=' &&
+ return
+
+ local sections=(d r)
+
+ # -l concurrency does not work with external logs
+ test _has_logdev || sections+=(l)
+
+ for section in "${sections[@]}"; do
+ $MKFS_XFS_PROG -$section concurrency=$nr_cpus 2>&1 | \
+ grep -q "unknown option -$section" ||
+ echo "-$section concurrency=$nr_cpus "
+ done
+ ;;
+ esac
+}
+
_scratch_mkfs_options()
{
_scratch_options mkfs
diff --git a/tests/generic/530 b/tests/generic/530
index f2513156a920e8..7413840476b588 100755
--- a/tests/generic/530
+++ b/tests/generic/530
@@ -25,11 +25,7 @@ _require_test_program "t_open_tmpfiles"
# For XFS, pushing 50000 unlinked inode inactivations through a small xfs log
# can result in bottlenecks on the log grant heads, so try to make the log
# larger to reduce runtime.
-if [ "$FSTYP" = "xfs" ] && ! _has_logdev; then
- _scratch_mkfs "-l size=256m" >> $seqres.full 2>&1
-else
- _scratch_mkfs >> $seqres.full 2>&1
-fi
+_scratch_mkfs $(_scratch_mkfs_concurrency_options 32) >> $seqres.full 2>&1
_scratch_mount
# Set ULIMIT_NOFILE to min(file-max / 2, 50000 files per LOAD_FACTOR)
diff --git a/tests/generic/531 b/tests/generic/531
index ed6c3f91153ecc..3ba2790c923464 100755
--- a/tests/generic/531
+++ b/tests/generic/531
@@ -23,11 +23,7 @@ _require_test_program "t_open_tmpfiles"
# On high CPU count machines, this runs a -lot- of create and unlink
# concurrency. Set the filesytsem up to handle this.
-if [ $FSTYP = "xfs" ]; then
- _scratch_mkfs "-d agcount=32" >> $seqres.full 2>&1
-else
- _scratch_mkfs >> $seqres.full 2>&1
-fi
+_scratch_mkfs $(_scratch_mkfs_concurrency_options 32) >> $seqres.full 2>&1
_scratch_mount
# Try to load up all the CPUs, two threads per CPU.
diff --git a/tests/xfs/501 b/tests/xfs/501
index 678c51b52948c5..4b29ef97d36c1a 100755
--- a/tests/xfs/501
+++ b/tests/xfs/501
@@ -33,7 +33,7 @@ _require_xfs_sysfs debug/log_recovery_delay
_require_scratch
_require_test_program "t_open_tmpfiles"
-_scratch_mkfs "-l size=256m" >> $seqres.full 2>&1
+_scratch_mkfs $(_scratch_mkfs_concurrency_options 32) >> $seqres.full 2>&1
_scratch_mount
# Set ULIMIT_NOFILE to min(file-max / 2, 30000 files per LOAD_FACTOR)
diff --git a/tests/xfs/502 b/tests/xfs/502
index 10b0017f6b2eb2..df3e7bcb17872d 100755
--- a/tests/xfs/502
+++ b/tests/xfs/502
@@ -23,7 +23,7 @@ _require_xfs_io_error_injection "iunlink_fallback"
_require_scratch
_require_test_program "t_open_tmpfiles"
-_scratch_mkfs "-l size=256m" | _filter_mkfs 2> $tmp.mkfs > /dev/null
+_scratch_mkfs $(_scratch_mkfs_concurrency_options 32) | _filter_mkfs 2> $tmp.mkfs > /dev/null
cat $tmp.mkfs >> $seqres.full
. $tmp.mkfs
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-16 23:27 ` [PATCH 10/23] mkfs: don't hardcode log size Darrick J. Wong
@ 2025-01-21 3:58 ` Dave Chinner
2025-01-21 12:44 ` Theodore Ts'o
2025-01-22 3:30 ` Darrick J. Wong
0 siblings, 2 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 3:58 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:27:46PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Commit 000813899afb46 hardcoded a log size of 256MB into xfs/501,
> xfs/502, and generic/530. This seems to be an attempt to reduce test
> run times by increasing the log size so that more background threads can
> run in parallel. Unfortunately, this breaks a couple of my test
> configurations:
>
> - External logs smaller than 256MB
> - Internal logs where the AG size is less than 256MB
....
> diff --git a/common/rc b/common/rc
> index 9e34c301b0deb0..885669beeb5e26 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -689,6 +689,33 @@ _test_cycle_mount()
> _test_mount
> }
>
> +# Are there mkfs options to try to improve concurrency?
> +_scratch_mkfs_concurrency_options()
> +{
> + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
caller does not need to pass a number of CPUs. This function can
simply do:
local nr_cpus=$(getconf _NPROCESSORS_CONF)
And that will set concurrency to be "optimal" for the number of CPUs
in the machine the test is going to run on. That way tests don't
need to hard code some number that is going to be too large for
small systems and to small for large systems...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-21 3:58 ` Dave Chinner
@ 2025-01-21 12:44 ` Theodore Ts'o
2025-01-21 22:05 ` Dave Chinner
2025-01-22 3:36 ` Darrick J. Wong
2025-01-22 3:30 ` Darrick J. Wong
1 sibling, 2 replies; 118+ messages in thread
From: Theodore Ts'o @ 2025-01-21 12:44 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 02:58:25PM +1100, Dave Chinner wrote:
> > +# Are there mkfs options to try to improve concurrency?
> > +_scratch_mkfs_concurrency_options()
> > +{
> > + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
>
> caller does not need to pass a number of CPUs. This function can
> simply do:
>
> local nr_cpus=$(getconf _NPROCESSORS_CONF)
>
> And that will set concurrency to be "optimal" for the number of CPUs
> in the machine the test is going to run on. That way tests don't
> need to hard code some number that is going to be too large for
> small systems and to small for large systems...
Hmm, but is this the right thing if you are using check-parallel? If
you are running multiple tests that are all running some kind of load
or stress-testing antagonist at the same time, then having 3x to 5x
the number of necessary antagonist threads is going to unnecessarily
slow down the test run, which goes against the original goal of what
we were hoping to achieve with check-parallel.
How many tests are you currently able to run in parallel today, and
what's the ultimate goal? We could have some kind of antagonist load
which is shared across multiple tests, but it's not clear to me that
it's worth the complexity. (And note that it's not just fs and cpu
load antagonistsw; there could also be memory stress antagonists, where
having multiple antagonists could lead to OOM kills...)
- Ted
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-21 12:44 ` Theodore Ts'o
@ 2025-01-21 22:05 ` Dave Chinner
2025-01-22 3:40 ` Darrick J. Wong
2025-01-22 3:36 ` Darrick J. Wong
1 sibling, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 22:05 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 07:44:30AM -0500, Theodore Ts'o wrote:
> On Tue, Jan 21, 2025 at 02:58:25PM +1100, Dave Chinner wrote:
> > > +# Are there mkfs options to try to improve concurrency?
> > > +_scratch_mkfs_concurrency_options()
> > > +{
> > > + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
> >
> > caller does not need to pass a number of CPUs. This function can
> > simply do:
> >
> > local nr_cpus=$(getconf _NPROCESSORS_CONF)
> >
> > And that will set concurrency to be "optimal" for the number of CPUs
> > in the machine the test is going to run on. That way tests don't
> > need to hard code some number that is going to be too large for
> > small systems and to small for large systems...
>
> Hmm, but is this the right thing if you are using check-parallel?
Yes. The whole point of check-parallel is to run the tests in such a
way as to max out the resources of the test machine for the entire
test run. Everything that can be run concurrently should be run
concurrently, and we should not be cutting down on the concurrency
just because we are running check-parallel.
> If
> you are running multiple tests that are all running some kind of load
> or stress-testing antagonist at the same time, then having 3x to 5x
> the number of necessary antagonist threads is going to unnecessarily
> slow down the test run, which goes against the original goal of what
> we were hoping to achieve with check-parallel.
There are tests that run a thousand concurrent fsstress processes -
check-parallel still runs all those thousand fsstress processes.
> How many tests are you currently able to run in parallel today,
All of them if I wanted. Default is to run one test per CPU at a
time, but also to allow tests that use concurrency to maximise it.
> and
> what's the ultimate goal?
My initial goal was to maximise the utilisation of the machine when
testing XFS. If I can't max out a 64p server with 1.5 million
IOPS/7GB/s IO and 64GB RAM with check-parallel, then check-parallel
is not running enough tests in parallel.
Right now with 64 runner threads (one per CPU), I'm seeing an
average utilisation across the whole auto group XFS test run of:
-50 CPUs
- 2.5GB/s IO @ 30k IOPS
- 40GB RAM
The utilisation on ext4 is much lower and runtimes are much longer
for (as yet) unknown reasons. Concurrent fsstress loads, in
particular, appear to run much slower on ext4 than XFS...
> We could have some kind of antagonist load
> which is shared across multiple tests, but it's not clear to me that
> it's worth the complexity.
Yes, that's the plan further down the track - stuff like background
CPU hotplug (instead of a test that specifically runs hotplug with
fsstress that takes about 5 minutes to run), cache dropping to add
memory reclaim during tests, etc
> (And note that it's not just fs and cpu
> load antagonistsw; there could also be memory stress antagonists, where
> having multiple antagonists could lead to OOM kills...)
Yes, I eventually plan to use the src/usemem.c memory locker to
create changing levels of background memory stress to the test
runs...
Right now "perturbations" are exercised as a side effect of random
tests performing these actions. I want to make them controllable by
check-parallel so we can exercise the system functionality across
the entire range of correctness tests we have, not just an isolated
test case.
IOWs, the whole point of check-parallel is to make use of large
machines to stress the whole OS at the same time as we are testing
for filesystem behavioural correctness.
I also want to do it in as short a time period as possible - outside
of dedicated QE environments, I don't beleive that long running
stress tests actually provide value for the machine time they
consume. i.e. returns rapidly diminish because stress tests
cover 99.99% of the code paths they are going to exercise in the
first few minutes of running.
Yes, letting them run for longer will -eventually- cover rarely
travelled code paths, but for developers, CI systems and
first/second level QE verification of bug fixes we don't need
extended stress tests.
Further, when we run fstests in the normal way, we never cover
things like memory reclaim racing against unmount, freeze, sync,
etc. And we never cover them when the system is under extremely
heavy load running multiple GB/s of IO whilst CPU hotplug is running
whilst the scheduler is running at nearly a million context
switches/s, etc.
That's exactly the sort of loads that check-parallel is generating
on a machine just running the correctness tests in parallel. It
combines correctness testing with a dynamic, stressful environment,
and it runs the tests -fast-. The coverage I get in a single 10
minute auto-group run of check-parallel is -much higher- than I get
in a single auto-group run of check that takes 4 hours on the same
hardware to complete....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-21 22:05 ` Dave Chinner
@ 2025-01-22 3:40 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:40 UTC (permalink / raw)
To: Dave Chinner; +Cc: Theodore Ts'o, zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 09:05:18AM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 07:44:30AM -0500, Theodore Ts'o wrote:
> > On Tue, Jan 21, 2025 at 02:58:25PM +1100, Dave Chinner wrote:
> > > > +# Are there mkfs options to try to improve concurrency?
> > > > +_scratch_mkfs_concurrency_options()
> > > > +{
> > > > + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
> > >
> > > caller does not need to pass a number of CPUs. This function can
> > > simply do:
> > >
> > > local nr_cpus=$(getconf _NPROCESSORS_CONF)
> > >
> > > And that will set concurrency to be "optimal" for the number of CPUs
> > > in the machine the test is going to run on. That way tests don't
> > > need to hard code some number that is going to be too large for
> > > small systems and to small for large systems...
> >
> > Hmm, but is this the right thing if you are using check-parallel?
>
> Yes. The whole point of check-parallel is to run the tests in such a
> way as to max out the resources of the test machine for the entire
> test run. Everything that can be run concurrently should be run
> concurrently, and we should not be cutting down on the concurrency
> just because we are running check-parallel.
>
> > If
> > you are running multiple tests that are all running some kind of load
> > or stress-testing antagonist at the same time, then having 3x to 5x
> > the number of necessary antagonist threads is going to unnecessarily
> > slow down the test run, which goes against the original goal of what
> > we were hoping to achieve with check-parallel.
>
> There are tests that run a thousand concurrent fsstress processes -
> check-parallel still runs all those thousand fsstress processes.
>
> > How many tests are you currently able to run in parallel today,
>
> All of them if I wanted. Default is to run one test per CPU at a
> time, but also to allow tests that use concurrency to maximise it.
>
> > and
> > what's the ultimate goal?
>
> My initial goal was to maximise the utilisation of the machine when
> testing XFS. If I can't max out a 64p server with 1.5 million
> IOPS/7GB/s IO and 64GB RAM with check-parallel, then check-parallel
> is not running enough tests in parallel.
>
> Right now with 64 runner threads (one per CPU), I'm seeing an
> average utilisation across the whole auto group XFS test run of:
>
> -50 CPUs
> - 2.5GB/s IO @ 30k IOPS
> - 40GB RAM
>
> The utilisation on ext4 is much lower and runtimes are much longer
> for (as yet) unknown reasons. Concurrent fsstress loads, in
> particular, appear to run much slower on ext4 than XFS...
>
> > We could have some kind of antagonist load
> > which is shared across multiple tests, but it's not clear to me that
> > it's worth the complexity.
>
> Yes, that's the plan further down the track - stuff like background
> CPU hotplug (instead of a test that specifically runs hotplug with
> fsstress that takes about 5 minutes to run), cache dropping to add
> memory reclaim during tests, etc
>
> > (And note that it's not just fs and cpu
> > load antagonistsw; there could also be memory stress antagonists, where
> > having multiple antagonists could lead to OOM kills...)
>
> Yes, I eventually plan to use the src/usemem.c memory locker to
> create changing levels of background memory stress to the test
> runs...
>
> Right now "perturbations" are exercised as a side effect of random
> tests performing these actions. I want to make them controllable by
> check-parallel so we can exercise the system functionality across
> the entire range of correctness tests we have, not just an isolated
> test case.
>
> IOWs, the whole point of check-parallel is to make use of large
> machines to stress the whole OS at the same time as we are testing
> for filesystem behavioural correctness.
>
> I also want to do it in as short a time period as possible - outside
> of dedicated QE environments, I don't beleive that long running
> stress tests actually provide value for the machine time they
> consume. i.e. returns rapidly diminish because stress tests
> cover 99.99% of the code paths they are going to exercise in the
> first few minutes of running.
>
> Yes, letting them run for longer will -eventually- cover rarely
> travelled code paths, but for developers, CI systems and
> first/second level QE verification of bug fixes we don't need
> extended stress tests.
Admittedly the long soak tests probably don't add much once the scratch
device has filled up and been cleaned out a few times. Maybe that
sacrificial usemem would be useful sooner than later.
ATM the online repair vs fsstress soak test is still pretty useful for
probing just how bad things can get in terms of system stress and
stalling, but that's only because repairs are resource intensive. :)
--D
> Further, when we run fstests in the normal way, we never cover
> things like memory reclaim racing against unmount, freeze, sync,
> etc. And we never cover them when the system is under extremely
> heavy load running multiple GB/s of IO whilst CPU hotplug is running
> whilst the scheduler is running at nearly a million context
> switches/s, etc.
>
> That's exactly the sort of loads that check-parallel is generating
> on a machine just running the correctness tests in parallel. It
> combines correctness testing with a dynamic, stressful environment,
> and it runs the tests -fast-. The coverage I get in a single 10
> minute auto-group run of check-parallel is -much higher- than I get
> in a single auto-group run of check that takes 4 hours on the same
> hardware to complete....
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-21 12:44 ` Theodore Ts'o
2025-01-21 22:05 ` Dave Chinner
@ 2025-01-22 3:36 ` Darrick J. Wong
1 sibling, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:36 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Dave Chinner, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 07:44:30AM -0500, Theodore Ts'o wrote:
> On Tue, Jan 21, 2025 at 02:58:25PM +1100, Dave Chinner wrote:
> > > +# Are there mkfs options to try to improve concurrency?
> > > +_scratch_mkfs_concurrency_options()
> > > +{
> > > + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
> >
> > caller does not need to pass a number of CPUs. This function can
> > simply do:
> >
> > local nr_cpus=$(getconf _NPROCESSORS_CONF)
> >
> > And that will set concurrency to be "optimal" for the number of CPUs
> > in the machine the test is going to run on. That way tests don't
> > need to hard code some number that is going to be too large for
> > small systems and to small for large systems...
>
> Hmm, but is this the right thing if you are using check-parallel? If
> you are running multiple tests that are all running some kind of load
> or stress-testing antagonist at the same time, then having 3x to 5x
> the number of necessary antagonist threads is going to unnecessarily
> slow down the test run, which goes against the original goal of what
> we were hoping to achieve with check-parallel.
<shrug> Maybe a more appropriate thing to do is:
local nr_cpus=$(grep Cpus_allowed /proc/self/status | hweight)
So a check-parallel could (if they see such problems) constrain the
parallelism through cpu pinning. I think getconf _NPROCESSORS_CONF is
probably fine for now.
(The other day I /did/ see some program in either util-linux or
coreutils that told you the number of "available" cpus based on checking
the affinity mask and whatever cgroups constraints are applied. I can't
find it now, alas...)
> How many tests are you currently able to run in parallel today, and
> what's the ultimate goal? We could have some kind of antagonist load
> which is shared across multiple tests, but it's not clear to me that
> it's worth the complexity. (And note that it's not just fs and cpu
> load antagonistsw; there could also be memory stress antagonists, where
> having multiple antagonists could lead to OOM kills...)
On the other hand, perhaps having random antagonistic processes from
other ./check instances is exactly the kind of stress testing that we
want to shake out weirder bugs? It's clear from Dave's RFC that the
generic/650 cpu hotplug shenanigans had some effect. ;)
--D
> - Ted
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 10/23] mkfs: don't hardcode log size
2025-01-21 3:58 ` Dave Chinner
2025-01-21 12:44 ` Theodore Ts'o
@ 2025-01-22 3:30 ` Darrick J. Wong
1 sibling, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 02:58:25PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:27:46PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Commit 000813899afb46 hardcoded a log size of 256MB into xfs/501,
> > xfs/502, and generic/530. This seems to be an attempt to reduce test
> > run times by increasing the log size so that more background threads can
> > run in parallel. Unfortunately, this breaks a couple of my test
> > configurations:
> >
> > - External logs smaller than 256MB
> > - Internal logs where the AG size is less than 256MB
> ....
>
> > diff --git a/common/rc b/common/rc
> > index 9e34c301b0deb0..885669beeb5e26 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -689,6 +689,33 @@ _test_cycle_mount()
> > _test_mount
> > }
> >
> > +# Are there mkfs options to try to improve concurrency?
> > +_scratch_mkfs_concurrency_options()
> > +{
> > + local nr_cpus="$(( $1 * LOAD_FACTOR ))"
>
> caller does not need to pass a number of CPUs. This function can
> simply do:
>
> local nr_cpus=$(getconf _NPROCESSORS_CONF)
>
> And that will set concurrency to be "optimal" for the number of CPUs
> in the machine the test is going to run on. That way tests don't
> need to hard code some number that is going to be too large for
> small systems and to small for large systems...
Sounds good to me.
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (9 preceding siblings ...)
2025-01-16 23:27 ` [PATCH 10/23] mkfs: don't hardcode log size Darrick J. Wong
@ 2025-01-16 23:28 ` Darrick J. Wong
2025-01-21 4:37 ` Dave Chinner
2025-01-16 23:28 ` [PATCH 12/23] preamble: fix missing _kill_fsstress Darrick J. Wong
` (11 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:28 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
xfs/336 does this somewhat sketchy thing where it mdrestores into a
regular file, and then does this to validate the restored metadata:
SCRATCH_DEV=$TEST_DIR/image _scratch_mount
Unfortunately, commit 1a49022fab9b4d causes the following regression:
--- /tmp/fstests/tests/xfs/336.out 2024-11-12 16:17:36.733447713 -0800
+++ /var/tmp/fstests/xfs/336.out.bad 2025-01-04 19:10:39.861871114 -0800
@@ -5,4 +5,5 @@ Create big file
Explode the rtrmapbt
Create metadump file
Restore metadump
-Check restored fs
+Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>
+(see /var/tmp/fstests/xfs/336.full for details)
This is due to the fact that SCRATCH_DEV is temporarily reassigned to
the regular file. That path is passed straight through _scratch_mount
to _xfs_prepare_for_eio_shutdown, but that helper _fails because the
"dev" argument isn't actually a path to a block device.
Fix this by detecting non-bdevs and finding (we hope) the loop device
that was created to handle the mount. While we're at it, have the
helper return the exit code from mount, not _prepare_for_eio_shutdown.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 1a49022fab9b4d ("fstests: always use fail-at-unmount semantics for XFS")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/rc | 8 ++++++++
common/xfs | 6 ++++++
2 files changed, 14 insertions(+)
diff --git a/common/rc b/common/rc
index 885669beeb5e26..4419cfc3188374 100644
--- a/common/rc
+++ b/common/rc
@@ -441,6 +441,7 @@ _try_scratch_mount()
[ $mount_ret -ne 0 ] && return $mount_ret
_idmapped_mount $SCRATCH_DEV $SCRATCH_MNT
_prepare_for_eio_shutdown $SCRATCH_DEV
+ return $mount_ret
}
# mount scratch device with given options and _fail if mount fails
@@ -658,6 +659,7 @@ _test_mount()
[ $mount_ret -ne 0 ] && return $mount_ret
_idmapped_mount $TEST_DEV $TEST_DIR
_prepare_for_eio_shutdown $TEST_DEV
+ return $mount_ret
}
_test_unmount()
@@ -4469,6 +4471,12 @@ _destroy_loop_device()
losetup -d $dev || _fail "Cannot destroy loop device $dev"
}
+# Find the loop bdev for a given file, if there is one.
+_find_loop_device()
+{
+ losetup --list -n -O NAME -j "$1"
+}
+
_scale_fsstress_args()
{
local args=""
diff --git a/common/xfs b/common/xfs
index 0417a40adba3e2..c68bd6d7c773ac 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1110,6 +1110,12 @@ _xfs_prepare_for_eio_shutdown()
local dev="$1"
local ctlfile="error/fail_at_unmount"
+ # Is this a regular file? Check if there's a loop device somewhere.
+ # Hopefully that lines up with a mounted filesystem.
+ if [ ! -b "$dev" ]; then
+ dev=$(_find_loop_device "$1" | tail -n 1)
+ fi
+
# Once we enable IO errors, it's possible that a writer thread will
# trip over EIO, cancel the transaction, and shut down the system.
# This is expected behavior, so we need to remove the "Internal error"
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown
2025-01-16 23:28 ` [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown Darrick J. Wong
@ 2025-01-21 4:37 ` Dave Chinner
2025-01-22 4:05 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 4:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:28:02PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> xfs/336 does this somewhat sketchy thing where it mdrestores into a
> regular file, and then does this to validate the restored metadata:
>
> SCRATCH_DEV=$TEST_DIR/image _scratch_mount
That's a canonical example of what is called "stepping on a
landmine".
We validate that the SCRATCH_DEV is a block device at the start of
check and each section it reads and runs (via common/config), and
then make the assumption in all the infrastructure that SCRATCH_DEV
always points to a valid block device.
Now we have one new test that overwrites SCRATCH_DEV temporarily
with a file and so we have to add checks all through the
infrastructure to handle this one whacky test?
> Unfortunately, commit 1a49022fab9b4d causes the following regression:
>
> --- /tmp/fstests/tests/xfs/336.out 2024-11-12 16:17:36.733447713 -0800
> +++ /var/tmp/fstests/xfs/336.out.bad 2025-01-04 19:10:39.861871114 -0800
> @@ -5,4 +5,5 @@ Create big file
> Explode the rtrmapbt
> Create metadump file
> Restore metadump
> -Check restored fs
> +Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>
> +(see /var/tmp/fstests/xfs/336.full for details)
>
> This is due to the fact that SCRATCH_DEV is temporarily reassigned to
> the regular file. That path is passed straight through _scratch_mount
> to _xfs_prepare_for_eio_shutdown, but that helper _fails because the
> "dev" argument isn't actually a path to a block device.
_scratch_mount assumes that SCRATCH_DEV points to a valid block
device. xfs/336 is the problem here, not the code that assumes
SCRATCH_DEV points to a block device....
Why are these hacks needed? Why can't _xfs_verify_metadumps()
loopdev usage be extended to handle the new rt rmap code that this
test is supposed to be exercising?
> Fix this by detecting non-bdevs and finding (we hope) the loop device
> that was created to handle the mount.
What loop device? xfs/336 doesn't use loop devices at all.
Oh, this is assuming that mount will silently do a loopback mount
when passed a file rather than a block device. IOWs, it's relying on
some third party to do the loop device creation and hence allow it
to be mounted.
IOWs, this change is addressing a landmine by adding another
landmine.
I really think that xfs/336 needs to be fixed - one off test hacks
like this, while they may work, only make modifying and maintaining
the fstests infrastructure that much harder....
> While we're at it, have the
> helper return the exit code from mount, not _prepare_for_eio_shutdown.
That should be a separate patch.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown
2025-01-21 4:37 ` Dave Chinner
@ 2025-01-22 4:05 ` Darrick J. Wong
2025-01-22 5:21 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 4:05 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 03:37:17PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:28:02PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > xfs/336 does this somewhat sketchy thing where it mdrestores into a
> > regular file, and then does this to validate the restored metadata:
> >
> > SCRATCH_DEV=$TEST_DIR/image _scratch_mount
>
> That's a canonical example of what is called "stepping on a
> landmine".
60% of fstests is written in bash, all of it is a friggin land mine
because bash totally lets us do variable substitution at any time, and
any time you make a change you have to exhaustively test the whole mess
to make sure nothing broke...
(Yeah, I hate bash)
> We validate that the SCRATCH_DEV is a block device at the start of
> check and each section it reads and runs (via common/config), and
> then make the assumption in all the infrastructure that SCRATCH_DEV
> always points to a valid block device.
We do? Can you point me to the sentence in doc/ that says this
explicitly? There's nothing I can find in the any docs and
_try_scratch_mount does not check SCRATCH_DEV is a bdev for XFS.
That needs to be documented.
> Now we have one new test that overwrites SCRATCH_DEV temporarily
> with a file and so we have to add checks all through the
> infrastructure to handle this one whacky test?
>
> > Unfortunately, commit 1a49022fab9b4d causes the following regression:
> >
> > --- /tmp/fstests/tests/xfs/336.out 2024-11-12 16:17:36.733447713 -0800
> > +++ /var/tmp/fstests/xfs/336.out.bad 2025-01-04 19:10:39.861871114 -0800
> > @@ -5,4 +5,5 @@ Create big file
> > Explode the rtrmapbt
> > Create metadump file
> > Restore metadump
> > -Check restored fs
> > +Usage: _set_fs_sysfs_attr <mounted_device> <attr> <content>
> > +(see /var/tmp/fstests/xfs/336.full for details)
> >
> > This is due to the fact that SCRATCH_DEV is temporarily reassigned to
> > the regular file. That path is passed straight through _scratch_mount
> > to _xfs_prepare_for_eio_shutdown, but that helper _fails because the
> > "dev" argument isn't actually a path to a block device.
>
> _scratch_mount assumes that SCRATCH_DEV points to a valid block
> device. xfs/336 is the problem here, not the code that assumes
> SCRATCH_DEV points to a block device....
>
> Why are these hacks needed? Why can't _xfs_verify_metadumps()
> loopdev usage be extended to handle the new rt rmap code that this
> test is supposed to be exercising?
Because _xfs_verify_metadumps came long after xfs/336. 336 itself was
merged long ago when I was naïve and didn't think it would take quite so
long to merge the rtrmap code.
> > Fix this by detecting non-bdevs and finding (we hope) the loop device
> > that was created to handle the mount.
>
> What loop device? xfs/336 doesn't use loop devices at all.
>
> Oh, this is assuming that mount will silently do a loopback mount
> when passed a file rather than a block device. IOWs, it's relying on
> some third party to do the loop device creation and hence allow it
> to be mounted.
>
> IOWs, this change is addressing a landmine by adding another
> landmine.
Some would say that mount adding the ability to set up a loop dev was
itself *avoiding* a landmine from 90s era util-linux.
> I really think that xfs/336 needs to be fixed - one off test hacks
> like this, while they may work, only make modifying and maintaining
> the fstests infrastructure that much harder....
Yeah, it'll get cleaned up for the rtrmap fstests merge.
> > While we're at it, have the
> > helper return the exit code from mount, not _prepare_for_eio_shutdown.
>
> That should be a separate patch.
Ok.
--D
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown
2025-01-22 4:05 ` Darrick J. Wong
@ 2025-01-22 5:21 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 5:21 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 08:05:42PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 21, 2025 at 03:37:17PM +1100, Dave Chinner wrote:
> > On Thu, Jan 16, 2025 at 03:28:02PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > xfs/336 does this somewhat sketchy thing where it mdrestores into a
> > > regular file, and then does this to validate the restored metadata:
> > >
> > > SCRATCH_DEV=$TEST_DIR/image _scratch_mount
> >
> > That's a canonical example of what is called "stepping on a
> > landmine".
>
> 60% of fstests is written in bash, all of it is a friggin land mine
> because bash totally lets us do variable substitution at any time, and
> any time you make a change you have to exhaustively test the whole mess
> to make sure nothing broke...
Yes, I know, which is why the moment I saw xfs/336 I called it out -
it has never run on my machines, ever...
> (Yeah, I hate bash)
Not a great fan of it myself. But it's no worse than other scripting
languages that use JIT based syntax checking from the "if it wasn't
run it ain't tested" perspective.
> > We validate that the SCRATCH_DEV is a block device at the start of
> > check and each section it reads and runs (via common/config), and
> > then make the assumption in all the infrastructure that SCRATCH_DEV
> > always points to a valid block device.
>
> We do?
fstests configurations for block based filesystems have always been
based on block devices and mount points, not image files.
Yes, you can pass an image file to XFS utilities and they will do
the right thing, but not all filesystems or all the infrastructure
in fstests can handle an image file masquerading as a device.
I certainly didn't expect it.....
> Can you point me to the sentence in doc/ that says this
> explicitly?
fstests is woefully under-documented - especially when it comes to
configuration constraints and behaviours - so I doubt it is actually
specified anywhere. AFAIA it has never been raised in discussion
for a long time (not since we added network filesystem support a
long time ago, IIRC)
However, the code is pretty explicit - common/config is responsible
for setting up and validating the runtime config before any test can
run. All test and scratch devices are passed through this
validation:
_check_device()
{
local name=$1
local dev_needed=$2
local dev=$3
if [ -z "$dev" ]; then
if [ "$dev_needed" == "required" ]; then
_fatal "common/config: $name is required but not defined!"
fi
return 0
fi
if [ -b "$dev" ] || ( echo $dev | grep -qE ":|//" ); then
# block device or a network url
return 0
fi
case "$FSTYP" in
9p|fuse|tmpfs|virtiofs|afs)
.....
*)
_fatal "common/config: $name ($dev) is not a block device or a network filesystem"
esac
}
....
Basically, it says that all the test and scratch devices (including
the external ones) must be either a block device, a network URL, or
a string that the specific filesystem under test must recognise and
accept (e.g. a directory for overlay filesystems). Otherwise fstests
will fail to run with an explicit error message that says:
<device> is not a block device or network filesystem
Nowhere in this config validation process does fstests consider
image files as a valid device configuration for a block based
filesystem.
If we need to do stuff with a image files, we have the
infrastructure to create loop devices and then operate directly on
that dynamic loop device(s) (e.g. _mkfs_dev, _mount, _unmount,
_check_xfs_filesystem, etc) that are created.
> There's nothing I can find in the any docs and
> _try_scratch_mount does not check SCRATCH_DEV is a bdev for XFS.
That's because it's validated before we start running tests and the
assumption is that nobody is screwing with SCRATCH_DEV in a way
that makes it behave vastly differently.
Consider what it means to have to do runtime checking of the
device validity in common code before we do anything with the
device. We'd have to sprinkle _check_device calls -everywhere-.
We'd also have to check logdev and rtdev variables if USE_EXTERNAL
is set, too.
That's not a viable development strategy, nor is it a maintainable
solution to the issue at hand. It's far simpler to fix one test not
to use this trick than it is to declare "nobody can trust TEST_DEV
or SCRATCH_DEV to be a valid block device" and have to handle that
everywhere those variables are used...
> That needs to be documented.
Sure.
> > > Fix this by detecting non-bdevs and finding (we hope) the loop device
> > > that was created to handle the mount.
> >
> > What loop device? xfs/336 doesn't use loop devices at all.
> >
> > Oh, this is assuming that mount will silently do a loopback mount
> > when passed a file rather than a block device. IOWs, it's relying on
> > some third party to do the loop device creation and hence allow it
> > to be mounted.
> >
> > IOWs, this change is addressing a landmine by adding another
> > landmine.
>
> Some would say that mount adding the ability to set up a loop dev was
> itself *avoiding* a landmine from 90s era util-linux.
True.
But in the case of fstests we explicitly create loop
devices so that we don't have to play whacky games to find the
random loop device that mount magically creates when you pass it a
file.
Making all the image file and loop device usage consistent across
all of fstests was part of the infrastructure changes in my initial
check-parallel patchset. This was necessary because killing tests
with ctrl-c would randomly leave dangling mounts and loop devices
because many tests did not have _cleanup routines to tear down
mounts that auto-created loop devices or clean up loop
devices they created themselves properly.
Part of those changes was fixing up the mess in some XFS tests
where that mixed loop device and image file based operations
interchangably. I didn't notice x/336 because it wasn't
running on my test system and so didn't attempt to fix it at the
same time...
> > I really think that xfs/336 needs to be fixed - one off test hacks
> > like this, while they may work, only make modifying and maintaining
> > the fstests infrastructure that much harder....
>
> Yeah, it'll get cleaned up for the rtrmap fstests merge.
Thanks!
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 12/23] preamble: fix missing _kill_fsstress
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (10 preceding siblings ...)
2025-01-16 23:28 ` [PATCH 11/23] common/xfs: find loop devices for non-blockdevs passed to _prepare_for_eio_shutdown Darrick J. Wong
@ 2025-01-16 23:28 ` Darrick J. Wong
2025-01-21 4:37 ` Dave Chinner
2025-01-16 23:28 ` [PATCH 13/23] generic/650: revert SOAK DURATION changes Darrick J. Wong
` (10 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:28 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Commit 8973af00ec212f added a _kill_fsstress to the standard _cleanup
function. However, if something breaks during test program
initialization before it gets to sourcing common/rc, then you get
failures that look like this:
--- /tmp/fstests/tests/generic/556.out 2024-09-25 12:09:52.938797554 -0700
+++ /var/tmp/fstests/generic/556.out.bad 2025-01-04 22:34:01.268327003 -0800
@@ -1,16 +1,3 @@
QA output created by 556
-SCRATCH_MNT/basic Casefold
-SCRATCH_MNT/basic
-SCRATCH_MNT/casefold_flag_removal Casefold
-SCRATCH_MNT/casefold_flag_removal Casefold
-SCRATCH_MNT/flag_inheritance/d1/d2/d3 Casefold
-SCRATCH_MNT/symlink/ind1/TARGET
-mv: 'SCRATCH_MNT/rename/rename' and 'SCRATCH_MNT/rename/RENAME' are the same file
-# file: SCRATCH_MNT/xattrs/x
-user.foo="bar"
-
-# file: SCRATCH_MNT/xattrs/x/f1
-user.foo="bar"
-
-touch: 'SCRATCH_MNT/strict/corac'$'\314\247\303': Invalid argument
-touch: 'SCRATCH_MNT/strict/cora'$'\303\247\303': Invalid argument
+./tests/generic/556: 108: common/config: Syntax error: "&" unexpected
+./tests/generic/556: 10: _kill_fsstress: not found
It's that last line that's unnecessary. Fix this by checking for the
presence of a _kill_fsstress before invoking it.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/preamble | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/preamble b/common/preamble
index 78e45d522f482c..0c9ee2e0377dd5 100644
--- a/common/preamble
+++ b/common/preamble
@@ -7,7 +7,7 @@
# Standard cleanup function. Individual tests can override this.
_cleanup()
{
- _kill_fsstress
+ command -v _kill_fsstress &>/dev/null && _kill_fsstress
cd /
rm -r -f $tmp.*
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 12/23] preamble: fix missing _kill_fsstress
2025-01-16 23:28 ` [PATCH 12/23] preamble: fix missing _kill_fsstress Darrick J. Wong
@ 2025-01-21 4:37 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 4:37 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:28:18PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Commit 8973af00ec212f added a _kill_fsstress to the standard _cleanup
> function. However, if something breaks during test program
> initialization before it gets to sourcing common/rc, then you get
> failures that look like this:
>
> --- /tmp/fstests/tests/generic/556.out 2024-09-25 12:09:52.938797554 -0700
> +++ /var/tmp/fstests/generic/556.out.bad 2025-01-04 22:34:01.268327003 -0800
> @@ -1,16 +1,3 @@
> QA output created by 556
> -SCRATCH_MNT/basic Casefold
> -SCRATCH_MNT/basic
> -SCRATCH_MNT/casefold_flag_removal Casefold
> -SCRATCH_MNT/casefold_flag_removal Casefold
> -SCRATCH_MNT/flag_inheritance/d1/d2/d3 Casefold
> -SCRATCH_MNT/symlink/ind1/TARGET
> -mv: 'SCRATCH_MNT/rename/rename' and 'SCRATCH_MNT/rename/RENAME' are the same file
> -# file: SCRATCH_MNT/xattrs/x
> -user.foo="bar"
> -
> -# file: SCRATCH_MNT/xattrs/x/f1
> -user.foo="bar"
> -
> -touch: 'SCRATCH_MNT/strict/corac'$'\314\247\303': Invalid argument
> -touch: 'SCRATCH_MNT/strict/cora'$'\303\247\303': Invalid argument
> +./tests/generic/556: 108: common/config: Syntax error: "&" unexpected
> +./tests/generic/556: 10: _kill_fsstress: not found
>
> It's that last line that's unnecessary. Fix this by checking for the
> presence of a _kill_fsstress before invoking it.
>
> Cc: <fstests@vger.kernel.org> # v2024.12.08
> Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
> common/preamble | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>
> diff --git a/common/preamble b/common/preamble
> index 78e45d522f482c..0c9ee2e0377dd5 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -7,7 +7,7 @@
> # Standard cleanup function. Individual tests can override this.
> _cleanup()
> {
> - _kill_fsstress
> + command -v _kill_fsstress &>/dev/null && _kill_fsstress
> cd /
> rm -r -f $tmp.*
> }
Looks fine.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (11 preceding siblings ...)
2025-01-16 23:28 ` [PATCH 12/23] preamble: fix missing _kill_fsstress Darrick J. Wong
@ 2025-01-16 23:28 ` Darrick J. Wong
2025-01-21 4:57 ` Dave Chinner
2025-01-16 23:28 ` [PATCH 14/23] generic/032: fix pinned mount failure Darrick J. Wong
` (9 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:28 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Prior to commit 8973af00ec21, in the absence of an explicit
SOAK_DURATION, this test would run 2500 fsstress operations each of ten
times through the loop body. On the author's machines, this kept the
runtime to about 30s total. Oddly, this was changed to 30s per loop
body with no specific justification in the middle of an fsstress process
management change.
On the author's machine, this explodes the runtime from ~30s to 420s.
Put things back the way they were.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
tests/generic/650 | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/tests/generic/650 b/tests/generic/650
index 60f86fdf518961..d376488f2fedeb 100755
--- a/tests/generic/650
+++ b/tests/generic/650
@@ -68,11 +68,8 @@ test "$nr_cpus" -gt 1024 && nr_cpus="$nr_hotplug_cpus"
fsstress_args+=(-p $nr_cpus)
if [ -n "$SOAK_DURATION" ]; then
test "$SOAK_DURATION" -lt 10 && SOAK_DURATION=10
-else
- # run for 30s per iteration max
- SOAK_DURATION=300
+ fsstress_args+=(--duration="$((SOAK_DURATION / 10))")
fi
-fsstress_args+=(--duration="$((SOAK_DURATION / 10))")
nr_ops=$((2500 * TIME_FACTOR))
fsstress_args+=(-n $nr_ops)
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-16 23:28 ` [PATCH 13/23] generic/650: revert SOAK DURATION changes Darrick J. Wong
@ 2025-01-21 4:57 ` Dave Chinner
2025-01-21 13:00 ` Theodore Ts'o
2025-01-22 3:49 ` Darrick J. Wong
0 siblings, 2 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 4:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:28:33PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Prior to commit 8973af00ec21, in the absence of an explicit
> SOAK_DURATION, this test would run 2500 fsstress operations each of ten
> times through the loop body. On the author's machines, this kept the
> runtime to about 30s total. Oddly, this was changed to 30s per loop
> body with no specific justification in the middle of an fsstress process
> management change.
I'm pretty sure that was because when you run g/650 on a machine
with 64p, the number of ops performed on the filesystem is
nr_cpus * 2500 * nr_loops.
In that case, each loop was taking over 90s to run, so the overall
runtime was up in the 15-20 minute mark. I wanted to cap the runtime
of each loop to min(nr_ops, SOAK_DURATION) so that it ran in about 5
minutes in the worst case i.e. (nr_loops * SOAK_DURATION).
I probably misunderstood how -n nr_ops vs --duration=30 interact;
I expected it to run until either were exhausted, not for duration
to override nr_ops as implied by this:
> On the author's machine, this explodes the runtime from ~30s to 420s.
> Put things back the way they were.
Yeah, OK, that's exactly waht keep_running() does - duration
overrides nr_ops.
Ok, so keeping or reverting the change will simply make different
people unhappy because of the excessive runtime the test has at
either ends of the CPU count spectrum - what's the best way to go
about providing the desired min(nr_ops, max loop time) behaviour?
Do we simply cap the maximum process count to keep the number of ops
down to something reasonable (e.g. 16), or something else?
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-21 4:57 ` Dave Chinner
@ 2025-01-21 13:00 ` Theodore Ts'o
2025-01-21 22:15 ` Dave Chinner
2025-01-22 3:49 ` Darrick J. Wong
1 sibling, 1 reply; 118+ messages in thread
From: Theodore Ts'o @ 2025-01-21 13:00 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> I probably misunderstood how -n nr_ops vs --duration=30 interact;
> I expected it to run until either were exhausted, not for duration
> to override nr_ops as implied by this:
There are (at least) two ways that a soak duration is being used
today; one is where someone wants to run a very long soak for hours
and where if you go long by an hour or two it's no big deals. The
other is where you are specifying a soak duration as part of a smoke
test (using the smoketest group), where you might be hoping to keep
the overall run time to 15-20 minutes and so you set SOAK_DURATION to
3m.
(This was based on some research that Darrick did which showed that
running the original 5 tests in the smoketest group gave you most of
the code coverage of running all of the quick group, which had
ballooned from 15 minutes many years ago to an hour or more. I just
noticed that we've since added two more tests to the smoketest group;
it might be worth checking whether those two new tests addded to thhe
smoketest groups significantly improves code coverage or not. It
would be unfortunate if the runtime bloat that happened to the quick
group also happens to the smoketest group...)
The bottom line is in addition to trying to design semantics for users
who might be at either end of the CPU count spectrum, we should also
consider that SOAK_DURATION could be set for values ranging from
minutes to hours.
Thanks,
- Ted
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-21 13:00 ` Theodore Ts'o
@ 2025-01-21 22:15 ` Dave Chinner
2025-01-22 3:51 ` Darrick J. Wong
2025-01-22 4:08 ` Theodore Ts'o
0 siblings, 2 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 22:15 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 08:00:27AM -0500, Theodore Ts'o wrote:
> On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> > I probably misunderstood how -n nr_ops vs --duration=30 interact;
> > I expected it to run until either were exhausted, not for duration
> > to override nr_ops as implied by this:
>
> There are (at least) two ways that a soak duration is being used
> today; one is where someone wants to run a very long soak for hours
> and where if you go long by an hour or two it's no big deals. The
> other is where you are specifying a soak duration as part of a smoke
> test (using the smoketest group), where you might be hoping to keep
> the overall run time to 15-20 minutes and so you set SOAK_DURATION to
> 3m.
check-parallel on my 64p machine runs the full auto group test in
under 10 minutes.
i.e. if you have a typical modern server (64-128p, 256GB RAM and a
couple of NVMe SSDs), then check-parallel allows a full test run in
the same time that './check -g smoketest' will run....
> (This was based on some research that Darrick did which showed that
> running the original 5 tests in the smoketest group gave you most of
> the code coverage of running all of the quick group, which had
> ballooned from 15 minutes many years ago to an hour or more. I just
> noticed that we've since added two more tests to the smoketest group;
> it might be worth checking whether those two new tests addded to thhe
> smoketest groups significantly improves code coverage or not. It
> would be unfortunate if the runtime bloat that happened to the quick
> group also happens to the smoketest group...)
Yes, and I've previously made the point about how check-parallel
changes the way we should be looking at dev-test cycles. We no
longer have to care that auto group testing takes 4 hours to run and
have to work around that with things like smoketest groups. If you
can run the whole auto test group in 10-15 minutes, then we don't
need "quick", "smoketest", etc to reduce dev-test cycle time
anymore...
> The bottom line is in addition to trying to design semantics for users
> who might be at either end of the CPU count spectrum, we should also
> consider that SOAK_DURATION could be set for values ranging from
> minutes to hours.
I don't see much point in testing for hours with check-parallel. The
whole point of it is to enable iteration across the entire fs test
matrix as fast as possible.
If you want to do long running soak tests, then keep using check for
that. If you want to run the auto group test across 100 different
mkfs option combinations, then that is where check-parallel comes in
- it'll take a few hours to do this instead of a week.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-21 22:15 ` Dave Chinner
@ 2025-01-22 3:51 ` Darrick J. Wong
2025-01-22 4:08 ` Theodore Ts'o
1 sibling, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:51 UTC (permalink / raw)
To: Dave Chinner; +Cc: Theodore Ts'o, zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 09:15:48AM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 08:00:27AM -0500, Theodore Ts'o wrote:
> > On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> > > I probably misunderstood how -n nr_ops vs --duration=30 interact;
> > > I expected it to run until either were exhausted, not for duration
> > > to override nr_ops as implied by this:
> >
> > There are (at least) two ways that a soak duration is being used
> > today; one is where someone wants to run a very long soak for hours
> > and where if you go long by an hour or two it's no big deals. The
> > other is where you are specifying a soak duration as part of a smoke
> > test (using the smoketest group), where you might be hoping to keep
> > the overall run time to 15-20 minutes and so you set SOAK_DURATION to
> > 3m.
>
> check-parallel on my 64p machine runs the full auto group test in
> under 10 minutes.
>
> i.e. if you have a typical modern server (64-128p, 256GB RAM and a
> couple of NVMe SSDs), then check-parallel allows a full test run in
> the same time that './check -g smoketest' will run....
>
> > (This was based on some research that Darrick did which showed that
> > running the original 5 tests in the smoketest group gave you most of
> > the code coverage of running all of the quick group, which had
> > ballooned from 15 minutes many years ago to an hour or more. I just
> > noticed that we've since added two more tests to the smoketest group;
> > it might be worth checking whether those two new tests addded to thhe
> > smoketest groups significantly improves code coverage or not. It
> > would be unfortunate if the runtime bloat that happened to the quick
> > group also happens to the smoketest group...)
>
> Yes, and I've previously made the point about how check-parallel
> changes the way we should be looking at dev-test cycles. We no
> longer have to care that auto group testing takes 4 hours to run and
> have to work around that with things like smoketest groups. If you
> can run the whole auto test group in 10-15 minutes, then we don't
> need "quick", "smoketest", etc to reduce dev-test cycle time
> anymore...
>
> > The bottom line is in addition to trying to design semantics for users
> > who might be at either end of the CPU count spectrum, we should also
> > consider that SOAK_DURATION could be set for values ranging from
> > minutes to hours.
>
> I don't see much point in testing for hours with check-parallel. The
> whole point of it is to enable iteration across the entire fs test
> matrix as fast as possible.
I do -- running all the soak tests in parallel on a (probably old lower
spec) machine. Parallelism is all right for a lot of things.
--D
> If you want to do long running soak tests, then keep using check for
> that. If you want to run the auto group test across 100 different
> mkfs option combinations, then that is where check-parallel comes in
> - it'll take a few hours to do this instead of a week.
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-21 22:15 ` Dave Chinner
2025-01-22 3:51 ` Darrick J. Wong
@ 2025-01-22 4:08 ` Theodore Ts'o
2025-01-22 6:01 ` Dave Chinner
1 sibling, 1 reply; 118+ messages in thread
From: Theodore Ts'o @ 2025-01-22 4:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 09:15:48AM +1100, Dave Chinner wrote:
> check-parallel on my 64p machine runs the full auto group test in
> under 10 minutes.
>
> i.e. if you have a typical modern server (64-128p, 256GB RAM and a
> couple of NVMe SSDs), then check-parallel allows a full test run in
> the same time that './check -g smoketest' will run....
Interesting. I would have thought that even with NVMe SSD's, you'd be
I/O speed constrained, especially given that some of the tests
(especially the ENOSPC hitters) can take quite a lot of time to fill
the storage device, even if they are using fallocate.
How do you have your test and scratch devices configured?
> Yes, and I've previously made the point about how check-parallel
> changes the way we should be looking at dev-test cycles. We no
> longer have to care that auto group testing takes 4 hours to run and
> have to work around that with things like smoketest groups. If you
> can run the whole auto test group in 10-15 minutes, then we don't
> need "quick", "smoketest", etc to reduce dev-test cycle time
> anymore...
Well, yes, if the only consideration is test run time latency.
I can think of two off-setting considerations. The first is if you
care about cost. The cheapest you can get a 64 CPU, 24 GiB VM on
Google Cloud is $3.04 USD/hour (n1-stndard-64 in a Iowa data center),
so ten minutes of run time is about 51 cents USD (ignoring the storage
costs). Not bad. But running xfs/4k on the auto group on an
e2-standard-2 VM takes 3.2 hours; but the e2-standard-2 VM is much
cheaper, coming in at $0.087651 USD/ hour. So that translates to 28
cents for the VM, and that's not taking into account the fact you
almost certainly much more expensive, high-performance storge to
support the 64 CPU VM. So if you don't care about time to run
completion (for example, if I'm monitoring the 5.15, 6.1, 6.6, and
6.12 LTS LTS rc git trees, and kicking off a build whenever Greg or
Sasha updates them), using a serialized xfstests is going to be
cheaper because you can use less expensive cloud resources.
The second concern is that for certain class of failures (UBSAN,
KCSAN, Lockdep, RCU soft lockups, WARN_ON, BUG_ON, and other
panics/OOPS), if you are runnig 64 tests in parllel it might not be
obvious which test caused the failure. Today, even if the test VM
crashes or hangs, I can have test manager (which runs on a e2-small VM
costing $0.021913 USD/hour and can manage dozens of test VM's all at the
same time), can restart the test VM, and we know which test is at at
fault, and we mark that a particular test with the Junit XML status of
"error" (as distinct from "success" or "failure"). If there are 64
test runs in parallel, if I wanted to have automated recovery if the
test appliance hangs or crashes, life gets a lot more complicated.....
I suppose we could have the human (or test automation) try run each
individual test that had been running at the time of the crash but
that's a lot more complicated, and what if the tests pass when run
once at a time? I guess we should happen that check-parallel found a
bug that plain check didn't find, but the human being still has to
root cause the failure.
Cheers,
- Ted
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-22 4:08 ` Theodore Ts'o
@ 2025-01-22 6:01 ` Dave Chinner
2025-01-22 7:02 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 6:01 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Darrick J. Wong, zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 11:08:39PM -0500, Theodore Ts'o wrote:
> On Wed, Jan 22, 2025 at 09:15:48AM +1100, Dave Chinner wrote:
> > check-parallel on my 64p machine runs the full auto group test in
> > under 10 minutes.
> >
> > i.e. if you have a typical modern server (64-128p, 256GB RAM and a
> > couple of NVMe SSDs), then check-parallel allows a full test run in
> > the same time that './check -g smoketest' will run....
>
> Interesting. I would have thought that even with NVMe SSD's, you'd be
> I/O speed constrained, especially given that some of the tests
> (especially the ENOSPC hitters) can take quite a lot of time to fill
> the storage device, even if they are using fallocate.
You haven't looked at how check-parallel works, have you? :/
> How do you have your test and scratch devices configured?
Please go and read the check-parallel script. It does all the
per-runner process test and scratch device configuration itself
using loop devices.
> > Yes, and I've previously made the point about how check-parallel
> > changes the way we should be looking at dev-test cycles. We no
> > longer have to care that auto group testing takes 4 hours to run and
> > have to work around that with things like smoketest groups. If you
> > can run the whole auto test group in 10-15 minutes, then we don't
> > need "quick", "smoketest", etc to reduce dev-test cycle time
> > anymore...
>
> Well, yes, if the only consideration is test run time latency.
Sure.
> I can think of two off-setting considerations. The first is if you
> care about cost.
Which I really don't care about.
That's something for a QE organisation to worry about, and it's up
to them to make the best use of the tools they have within the
budget they have.
> The second concern is that for certain class of failures (UBSAN,
> KCSAN, Lockdep, RCU soft lockups, WARN_ON, BUG_ON, and other
> panics/OOPS), if you are runnig 64 tests in parllel it might not be
> obvious which test caused the failure.
Then multiple tests will fail with the same dmesg error, but it's
generally pretty clear which of the tests caused it. Yes, it's a bit
more work to isolate the specific test, but it's not hard or any
different to how a test failure is debugged now.
If you want to automate such failures, then my process is to grep
the log files for all the tests that failed with a dmesg error then
run them again using check instead of check-parallel. Then I get
exactly which test generated the dmesg output without having to put
time into trying to work out which test triggered the failure.
> Today, even if the test VM
> crashes or hangs, I can have test manager (which runs on a e2-small VM
> costing $0.021913 USD/hour and can manage dozens of test VM's all at the
> same time), can restart the test VM, and we know which test is at at
> fault, and we mark that a particular test with the Junit XML status of
> "error" (as distinct from "success" or "failure"). If there are 64
> test runs in parallel, if I wanted to have automated recovery if the
> test appliance hangs or crashes, life gets a lot more complicated.....
Not really. Both dmesg and the results files will have tracked all
the tests inflight when the system crashes, so it's just an extra
step to extract all those tests and run them again using check
and/or check-parallel to further isolate which test caused the
failure....
I'm sure this could be automated eventually, but that's way down my
priority list right now.
> I suppose we could have the human (or test automation) try run each
> individual test that had been running at the time of the crash but
> that's a lot more complicated, and what if the tests pass when run
> once at a time? I guess we should happen that check-parallel found a
> bug that plain check didn't find, but the human being still has to
> root cause the failure.
Yes. This is no different to a test that is flakey or compeltely
fails when run serially by check multiple times. You still need a
human to find the root cause of the failure.
Nobody is being forced to change their tooling or processes to use
check-parallel if they don't want or need to. It is an alternative
method for running the tests within the fstests suite - if using
check meets your needs, there is no reason to use check-parallel or
even care that it exists...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-22 6:01 ` Dave Chinner
@ 2025-01-22 7:02 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 7:02 UTC (permalink / raw)
To: Dave Chinner; +Cc: Theodore Ts'o, zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 05:01:47PM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 11:08:39PM -0500, Theodore Ts'o wrote:
> > On Wed, Jan 22, 2025 at 09:15:48AM +1100, Dave Chinner wrote:
> > > check-parallel on my 64p machine runs the full auto group test in
> > > under 10 minutes.
> > >
> > > i.e. if you have a typical modern server (64-128p, 256GB RAM and a
> > > couple of NVMe SSDs), then check-parallel allows a full test run in
> > > the same time that './check -g smoketest' will run....
> >
> > Interesting. I would have thought that even with NVMe SSD's, you'd be
> > I/O speed constrained, especially given that some of the tests
> > (especially the ENOSPC hitters) can take quite a lot of time to fill
> > the storage device, even if they are using fallocate.
>
> You haven't looked at how check-parallel works, have you? :/
>
> > How do you have your test and scratch devices configured?
>
> Please go and read the check-parallel script. It does all the
> per-runner process test and scratch device configuration itself
> using loop devices.
>
> > > Yes, and I've previously made the point about how check-parallel
> > > changes the way we should be looking at dev-test cycles. We no
> > > longer have to care that auto group testing takes 4 hours to run and
> > > have to work around that with things like smoketest groups. If you
> > > can run the whole auto test group in 10-15 minutes, then we don't
> > > need "quick", "smoketest", etc to reduce dev-test cycle time
> > > anymore...
> >
> > Well, yes, if the only consideration is test run time latency.
>
> Sure.
>
> > I can think of two off-setting considerations. The first is if you
> > care about cost.
>
> Which I really don't care about.
>
> That's something for a QE organisation to worry about, and it's up
> to them to make the best use of the tools they have within the
> budget they have.
>
> > The second concern is that for certain class of failures (UBSAN,
> > KCSAN, Lockdep, RCU soft lockups, WARN_ON, BUG_ON, and other
> > panics/OOPS), if you are runnig 64 tests in parllel it might not be
> > obvious which test caused the failure.
>
> Then multiple tests will fail with the same dmesg error, but it's
> generally pretty clear which of the tests caused it. Yes, it's a bit
> more work to isolate the specific test, but it's not hard or any
> different to how a test failure is debugged now.
>
> If you want to automate such failures, then my process is to grep
> the log files for all the tests that failed with a dmesg error then
> run them again using check instead of check-parallel. Then I get
> exactly which test generated the dmesg output without having to put
> time into trying to work out which test triggered the failure.
>
> > Today, even if the test VM
> > crashes or hangs, I can have test manager (which runs on a e2-small VM
> > costing $0.021913 USD/hour and can manage dozens of test VM's all at the
> > same time), can restart the test VM, and we know which test is at at
> > fault, and we mark that a particular test with the Junit XML status of
> > "error" (as distinct from "success" or "failure"). If there are 64
> > test runs in parallel, if I wanted to have automated recovery if the
> > test appliance hangs or crashes, life gets a lot more complicated.....
>
> Not really. Both dmesg and the results files will have tracked all
> the tests inflight when the system crashes, so it's just an extra
> step to extract all those tests and run them again using check
> and/or check-parallel to further isolate which test caused the
> failure....
That reminds me to go see if ./check actually fsyncs the state and
report files and whatnot between tests, so that we have a better chance
of figuring out where exactly fstests blew up the machine.
(Luckily xfs is stable enough I haven't had a machine explode in quite
some time, good job everyone! :))
--D
> I'm sure this could be automated eventually, but that's way down my
> priority list right now.
>
> > I suppose we could have the human (or test automation) try run each
> > individual test that had been running at the time of the crash but
> > that's a lot more complicated, and what if the tests pass when run
> > once at a time? I guess we should happen that check-parallel found a
> > bug that plain check didn't find, but the human being still has to
> > root cause the failure.
>
> Yes. This is no different to a test that is flakey or compeltely
> fails when run serially by check multiple times. You still need a
> human to find the root cause of the failure.
>
> Nobody is being forced to change their tooling or processes to use
> check-parallel if they don't want or need to. It is an alternative
> method for running the tests within the fstests suite - if using
> check meets your needs, there is no reason to use check-parallel or
> even care that it exists...
>
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-21 4:57 ` Dave Chinner
2025-01-21 13:00 ` Theodore Ts'o
@ 2025-01-22 3:49 ` Darrick J. Wong
2025-01-22 4:12 ` Dave Chinner
1 sibling, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:49 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:28:33PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Prior to commit 8973af00ec21, in the absence of an explicit
> > SOAK_DURATION, this test would run 2500 fsstress operations each of ten
> > times through the loop body. On the author's machines, this kept the
> > runtime to about 30s total. Oddly, this was changed to 30s per loop
> > body with no specific justification in the middle of an fsstress process
> > management change.
>
> I'm pretty sure that was because when you run g/650 on a machine
> with 64p, the number of ops performed on the filesystem is
> nr_cpus * 2500 * nr_loops.
Where does that happen?
Oh, heh. -n is the number of ops *per process*.
> In that case, each loop was taking over 90s to run, so the overall
> runtime was up in the 15-20 minute mark. I wanted to cap the runtime
> of each loop to min(nr_ops, SOAK_DURATION) so that it ran in about 5
> minutes in the worst case i.e. (nr_loops * SOAK_DURATION).
>
> I probably misunderstood how -n nr_ops vs --duration=30 interact;
> I expected it to run until either were exhausted, not for duration
> to override nr_ops as implied by this:
Yeah, SOAK_DURATION overrides pretty much everything.
> > On the author's machine, this explodes the runtime from ~30s to 420s.
> > Put things back the way they were.
>
> Yeah, OK, that's exactly waht keep_running() does - duration
> overrides nr_ops.
>
> Ok, so keeping or reverting the change will simply make different
> people unhappy because of the excessive runtime the test has at
> either ends of the CPU count spectrum - what's the best way to go
> about providing the desired min(nr_ops, max loop time) behaviour?
> Do we simply cap the maximum process count to keep the number of ops
> down to something reasonable (e.g. 16), or something else?
How about running fsstress with --duration=3 if SOAK_DURATION isn't set?
That should keep the runtime to 30 seconds or so even on larger
machines:
if [ -n "$SOAK_DURATION" ]; then
test "$SOAK_DURATION" -lt 10 && SOAK_DURATION=10
fsstress_args+=(--duration="$((SOAK_DURATION / 10))")
else
# run for 3s per iteration max for a default runtime of ~30s.
fsstress_args+=(--duration=3)
fi
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-22 3:49 ` Darrick J. Wong
@ 2025-01-22 4:12 ` Dave Chinner
2025-01-22 4:37 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 4:12 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 07:49:44PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> > On Thu, Jan 16, 2025 at 03:28:33PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > Prior to commit 8973af00ec21, in the absence of an explicit
> > > SOAK_DURATION, this test would run 2500 fsstress operations each of ten
> > > times through the loop body. On the author's machines, this kept the
> > > runtime to about 30s total. Oddly, this was changed to 30s per loop
> > > body with no specific justification in the middle of an fsstress process
> > > management change.
> >
> > I'm pretty sure that was because when you run g/650 on a machine
> > with 64p, the number of ops performed on the filesystem is
> > nr_cpus * 2500 * nr_loops.
>
> Where does that happen?
>
> Oh, heh. -n is the number of ops *per process*.
Yeah, I just noticed another case of this:
Ten slowest tests - runtime in seconds:
generic/750 559
generic/311 486
.....
generic/750 does:
nr_cpus=$((LOAD_FACTOR * 4))
nr_ops=$((25000 * nr_cpus * TIME_FACTOR))
fsstress_args=(-w -d $SCRATCH_MNT -n $nr_ops -p $nr_cpus)
So the actual load factor increase is exponential:
Load factor nr_cpus nr_ops total ops
1 4 100k 400k
2 8 200k 1.6M
3 12 300k 3.6M
4 16 400k 6.4M
and so on.
I suspect that there are other similar cpu scaling issues
lurking across the many fsstress tests...
> > > On the author's machine, this explodes the runtime from ~30s to 420s.
> > > Put things back the way they were.
> >
> > Yeah, OK, that's exactly waht keep_running() does - duration
> > overrides nr_ops.
> >
> > Ok, so keeping or reverting the change will simply make different
> > people unhappy because of the excessive runtime the test has at
> > either ends of the CPU count spectrum - what's the best way to go
> > about providing the desired min(nr_ops, max loop time) behaviour?
> > Do we simply cap the maximum process count to keep the number of ops
> > down to something reasonable (e.g. 16), or something else?
>
> How about running fsstress with --duration=3 if SOAK_DURATION isn't set?
> That should keep the runtime to 30 seconds or so even on larger
> machines:
>
> if [ -n "$SOAK_DURATION" ]; then
> test "$SOAK_DURATION" -lt 10 && SOAK_DURATION=10
> fsstress_args+=(--duration="$((SOAK_DURATION / 10))")
> else
> # run for 3s per iteration max for a default runtime of ~30s.
> fsstress_args+=(--duration=3)
> fi
Yeah, that works for me.
As a rainy day project, perhaps we should look to convert all the
fsstress invocations to be time bound rather than running a specific
number of ops. i.e. hard code nr_ops=<some huge number> in
_run_fstress_bg() and the tests only need to define parallelism and
runtime.
This would make the test runtimes more deterministic across machines
with vastly different capabilities and and largely make "test xyz is
slow on my test machine" reports largely go away.
Thoughts?
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 13/23] generic/650: revert SOAK DURATION changes
2025-01-22 4:12 ` Dave Chinner
@ 2025-01-22 4:37 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 4:37 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Wed, Jan 22, 2025 at 03:12:11PM +1100, Dave Chinner wrote:
> On Tue, Jan 21, 2025 at 07:49:44PM -0800, Darrick J. Wong wrote:
> > On Tue, Jan 21, 2025 at 03:57:23PM +1100, Dave Chinner wrote:
> > > On Thu, Jan 16, 2025 at 03:28:33PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > Prior to commit 8973af00ec21, in the absence of an explicit
> > > > SOAK_DURATION, this test would run 2500 fsstress operations each of ten
> > > > times through the loop body. On the author's machines, this kept the
> > > > runtime to about 30s total. Oddly, this was changed to 30s per loop
> > > > body with no specific justification in the middle of an fsstress process
> > > > management change.
> > >
> > > I'm pretty sure that was because when you run g/650 on a machine
> > > with 64p, the number of ops performed on the filesystem is
> > > nr_cpus * 2500 * nr_loops.
> >
> > Where does that happen?
> >
> > Oh, heh. -n is the number of ops *per process*.
>
> Yeah, I just noticed another case of this:
>
> Ten slowest tests - runtime in seconds:
> generic/750 559
> generic/311 486
> .....
>
> generic/750 does:
>
> nr_cpus=$((LOAD_FACTOR * 4))
> nr_ops=$((25000 * nr_cpus * TIME_FACTOR))
> fsstress_args=(-w -d $SCRATCH_MNT -n $nr_ops -p $nr_cpus)
>
> So the actual load factor increase is exponential:
>
> Load factor nr_cpus nr_ops total ops
> 1 4 100k 400k
> 2 8 200k 1.6M
> 3 12 300k 3.6M
> 4 16 400k 6.4M
>
> and so on.
>
> I suspect that there are other similar cpu scaling issues
> lurking across the many fsstress tests...
>
> > > > On the author's machine, this explodes the runtime from ~30s to 420s.
> > > > Put things back the way they were.
> > >
> > > Yeah, OK, that's exactly waht keep_running() does - duration
> > > overrides nr_ops.
> > >
> > > Ok, so keeping or reverting the change will simply make different
> > > people unhappy because of the excessive runtime the test has at
> > > either ends of the CPU count spectrum - what's the best way to go
> > > about providing the desired min(nr_ops, max loop time) behaviour?
> > > Do we simply cap the maximum process count to keep the number of ops
> > > down to something reasonable (e.g. 16), or something else?
> >
> > How about running fsstress with --duration=3 if SOAK_DURATION isn't set?
> > That should keep the runtime to 30 seconds or so even on larger
> > machines:
> >
> > if [ -n "$SOAK_DURATION" ]; then
> > test "$SOAK_DURATION" -lt 10 && SOAK_DURATION=10
> > fsstress_args+=(--duration="$((SOAK_DURATION / 10))")
> > else
> > # run for 3s per iteration max for a default runtime of ~30s.
> > fsstress_args+=(--duration=3)
> > fi
>
> Yeah, that works for me.
>
> As a rainy day project, perhaps we should look to convert all the
> fsstress invocations to be time bound rather than running a specific
> number of ops. i.e. hard code nr_ops=<some huge number> in
> _run_fstress_bg() and the tests only need to define parallelism and
> runtime.
I /think/ the only ones that do that are generic/1220 generic/476
generic/642 generic/750. I could drop the nr_cpus term from the nr_ops
calculation.
> This would make the test runtimes more deterministic across machines
> with vastly different capabilities and and largely make "test xyz is
> slow on my test machine" reports largely go away.
>
> Thoughts?
I'm fine with _run_fsstress injecting --duration=30 if no other duration
argument is passed in.
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 14/23] generic/032: fix pinned mount failure
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (12 preceding siblings ...)
2025-01-16 23:28 ` [PATCH 13/23] generic/650: revert SOAK DURATION changes Darrick J. Wong
@ 2025-01-16 23:28 ` Darrick J. Wong
2025-01-21 5:03 ` Dave Chinner
2025-01-16 23:29 ` [PATCH 15/23] fuzzy: stop __stress_scrub_fsx_loop if fsx fails Darrick J. Wong
` (8 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:28 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
generic/032 now periodically fails with:
--- /tmp/fstests/tests/generic/032.out 2025-01-05 11:42:14.427388698 -0800
+++ /var/tmp/fstests/generic/032.out.bad 2025-01-06 18:20:17.122818195 -0800
@@ -1,5 +1,7 @@
QA output created by 032
100 iterations
-000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd >................<
-*
-100000
+umount: /opt: target is busy.
+mount: /opt: /dev/sda4 already mounted on /opt.
+ dmesg(1) may have more information after failed mount system call.
+cycle mount failed
+(see /var/tmp/fstests/generic/032.full for details)
The root cause of this regression is the _syncloop subshell. This
background process runs _scratch_sync, which is actually an xfs_io
process that calls syncfs on the scratch mount.
Unfortunately, while the test kills the _syncloop subshell, it doesn't
actually kill the xfs_io process. If the xfs_io process is in D state
running the syncfs, it won't react to the signal, but it will pin the
mount. Then the _scratch_cycle_mount fails because the mount is pinned.
Prior to commit 8973af00ec212f the _syncloop ran sync(1) which avoided
pinning the scratch filesystem.
Fix this by pgrepping for the xfs_io process and killing and waiting for
it if necessary.
Cc: <fstests@vger.kernel.org> # v2024.12.08
Fixes: 8973af00ec212f ("fstests: cleanup fsstress process management")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/rc | 6 ++++++
tests/generic/032 | 9 +++++++++
2 files changed, 15 insertions(+)
diff --git a/common/rc b/common/rc
index 4419cfc3188374..d7f3c48eafe590 100644
--- a/common/rc
+++ b/common/rc
@@ -36,6 +36,12 @@ _pkill()
pkill --session 0 "$@"
}
+# Find only the test processes started by this test
+_pgrep()
+{
+ pgrep --session 0 "$@"
+}
+
# Common execution handling for fsstress invocation.
#
# We need per-test fsstress binaries because of the way fsstress forks and
diff --git a/tests/generic/032 b/tests/generic/032
index 30290c7225a2fa..48d594fe9315b8 100755
--- a/tests/generic/032
+++ b/tests/generic/032
@@ -81,6 +81,15 @@ echo $iters iterations
kill $syncpid
wait
+# The xfs_io instance started by _scratch_sync could be stuck in D state when
+# the subshell running _syncloop & is killed. That xfs_io process pins the
+# mount so we must kill it and wait for it to die before cycling the mount.
+dead_syncfs_pid=$(_pgrep xfs_io)
+if [ -n "$dead_syncfs_pid" ]; then
+ _pkill xfs_io
+ wait $dead_syncfs_pid
+fi
+
# clear page cache and dump the file
_scratch_cycle_mount
_hexdump $SCRATCH_MNT/file
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 14/23] generic/032: fix pinned mount failure
2025-01-16 23:28 ` [PATCH 14/23] generic/032: fix pinned mount failure Darrick J. Wong
@ 2025-01-21 5:03 ` Dave Chinner
2025-01-22 4:08 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 5:03 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:28:49PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> generic/032 now periodically fails with:
>
> --- /tmp/fstests/tests/generic/032.out 2025-01-05 11:42:14.427388698 -0800
> +++ /var/tmp/fstests/generic/032.out.bad 2025-01-06 18:20:17.122818195 -0800
> @@ -1,5 +1,7 @@
> QA output created by 032
> 100 iterations
> -000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd >................<
> -*
> -100000
> +umount: /opt: target is busy.
> +mount: /opt: /dev/sda4 already mounted on /opt.
> + dmesg(1) may have more information after failed mount system call.
> +cycle mount failed
> +(see /var/tmp/fstests/generic/032.full for details)
>
> The root cause of this regression is the _syncloop subshell. This
> background process runs _scratch_sync, which is actually an xfs_io
> process that calls syncfs on the scratch mount.
>
> Unfortunately, while the test kills the _syncloop subshell, it doesn't
> actually kill the xfs_io process. If the xfs_io process is in D state
> running the syncfs, it won't react to the signal, but it will pin the
> mount. Then the _scratch_cycle_mount fails because the mount is pinned.
>
> Prior to commit 8973af00ec212f the _syncloop ran sync(1) which avoided
> pinning the scratch filesystem.
How does running sync(1) prevent this? they run the same kernel
code, so I'm a little confused as to why this is a problem caused
by using the syncfs() syscall rather than the sync() syscall...
> Fix this by pgrepping for the xfs_io process and killing and waiting for
> it if necessary.
Change looks fine, though.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 14/23] generic/032: fix pinned mount failure
2025-01-21 5:03 ` Dave Chinner
@ 2025-01-22 4:08 ` Darrick J. Wong
2025-01-22 4:19 ` Dave Chinner
0 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 4:08 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 04:03:23PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:28:49PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > generic/032 now periodically fails with:
> >
> > --- /tmp/fstests/tests/generic/032.out 2025-01-05 11:42:14.427388698 -0800
> > +++ /var/tmp/fstests/generic/032.out.bad 2025-01-06 18:20:17.122818195 -0800
> > @@ -1,5 +1,7 @@
> > QA output created by 032
> > 100 iterations
> > -000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd >................<
> > -*
> > -100000
> > +umount: /opt: target is busy.
> > +mount: /opt: /dev/sda4 already mounted on /opt.
> > + dmesg(1) may have more information after failed mount system call.
> > +cycle mount failed
> > +(see /var/tmp/fstests/generic/032.full for details)
> >
> > The root cause of this regression is the _syncloop subshell. This
> > background process runs _scratch_sync, which is actually an xfs_io
> > process that calls syncfs on the scratch mount.
> >
> > Unfortunately, while the test kills the _syncloop subshell, it doesn't
> > actually kill the xfs_io process. If the xfs_io process is in D state
> > running the syncfs, it won't react to the signal, but it will pin the
> > mount. Then the _scratch_cycle_mount fails because the mount is pinned.
> >
> > Prior to commit 8973af00ec212f the _syncloop ran sync(1) which avoided
> > pinning the scratch filesystem.
>
> How does running sync(1) prevent this? they run the same kernel
> code, so I'm a little confused as to why this is a problem caused
> by using the syncfs() syscall rather than the sync() syscall...
Instead of:
_scratch_sync -> _sync_fs $SCRATCH_MNT -> $XFS_IO_PROG -rxc "syncfs" $SCRATCH_MNT
sync(1) just calls sync(2) with no open files other than
std{in,out,err}.
--D
> > Fix this by pgrepping for the xfs_io process and killing and waiting for
> > it if necessary.
>
> Change looks fine, though.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread* Re: [PATCH 14/23] generic/032: fix pinned mount failure
2025-01-22 4:08 ` Darrick J. Wong
@ 2025-01-22 4:19 ` Dave Chinner
0 siblings, 0 replies; 118+ messages in thread
From: Dave Chinner @ 2025-01-22 4:19 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 08:08:34PM -0800, Darrick J. Wong wrote:
> On Tue, Jan 21, 2025 at 04:03:23PM +1100, Dave Chinner wrote:
> > On Thu, Jan 16, 2025 at 03:28:49PM -0800, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > >
> > > generic/032 now periodically fails with:
> > >
> > > --- /tmp/fstests/tests/generic/032.out 2025-01-05 11:42:14.427388698 -0800
> > > +++ /var/tmp/fstests/generic/032.out.bad 2025-01-06 18:20:17.122818195 -0800
> > > @@ -1,5 +1,7 @@
> > > QA output created by 032
> > > 100 iterations
> > > -000000 cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd cd >................<
> > > -*
> > > -100000
> > > +umount: /opt: target is busy.
> > > +mount: /opt: /dev/sda4 already mounted on /opt.
> > > + dmesg(1) may have more information after failed mount system call.
> > > +cycle mount failed
> > > +(see /var/tmp/fstests/generic/032.full for details)
> > >
> > > The root cause of this regression is the _syncloop subshell. This
> > > background process runs _scratch_sync, which is actually an xfs_io
> > > process that calls syncfs on the scratch mount.
> > >
> > > Unfortunately, while the test kills the _syncloop subshell, it doesn't
> > > actually kill the xfs_io process. If the xfs_io process is in D state
> > > running the syncfs, it won't react to the signal, but it will pin the
> > > mount. Then the _scratch_cycle_mount fails because the mount is pinned.
> > >
> > > Prior to commit 8973af00ec212f the _syncloop ran sync(1) which avoided
> > > pinning the scratch filesystem.
> >
> > How does running sync(1) prevent this? they run the same kernel
> > code, so I'm a little confused as to why this is a problem caused
> > by using the syncfs() syscall rather than the sync() syscall...
>
> Instead of:
> _scratch_sync -> _sync_fs $SCRATCH_MNT -> $XFS_IO_PROG -rxc "syncfs" $SCRATCH_MNT
>
> sync(1) just calls sync(2) with no open files other than
> std{in,out,err}.
Sure, but while sync(2) is writing back a superblock it pins the
superblock by holding the s_umount lock. So even if the sync process
is killable, it still pins the dirty superblock for the same
amount of time as syncfs.
Oh, in the sync case unmount blocks on the s_umount lock rather than
returns -EBUSY because of the elevated open file count with syncfs.
Ok, gotcha, we've been using different definitions for the phrase
"mount is pinned". :/
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 15/23] fuzzy: stop __stress_scrub_fsx_loop if fsx fails
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (13 preceding siblings ...)
2025-01-16 23:28 ` [PATCH 14/23] generic/032: fix pinned mount failure Darrick J. Wong
@ 2025-01-16 23:29 ` Darrick J. Wong
2025-01-16 23:29 ` [PATCH 16/23] fuzzy: don't use readarray for xfsfind output Darrick J. Wong
` (7 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:29 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Stop the fsx scrub stress loop if fsx returns a nonzero error code.
Cc: <fstests@vger.kernel.org> # v2023.01.15
Fixes: a2056ca8917bc8 ("fuzzy: enhance scrub stress testing to use fsx")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/fuzzy | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/fuzzy b/common/fuzzy
index 772ce7ddcff6d8..46771b1d117106 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -942,6 +942,7 @@ __stress_scrub_fsx_loop() {
local remount_period="$3"
local stress_tgt="$4" # ignored
local focus=(-q -X) # quiet, validate file contents
+ local res
# As of November 2022, 2 million fsx ops should be enough to keep
# any filesystem busy for a couple of hours.
@@ -993,7 +994,9 @@ __stress_scrub_fsx_loop() {
# Need to recheck running conditions if we cleared anything
__stress_scrub_clean_scratch && continue
$here/ltp/fsx $args >> $seqres.full
- echo "fsx exits with $? at $(date)" >> $seqres.full
+ res=$?
+ echo "fsx exits with $res at $(date)" >> $seqres.full
+ test "$res" -ne 0 && break
done
rm -f "$runningfile"
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 16/23] fuzzy: don't use readarray for xfsfind output
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (14 preceding siblings ...)
2025-01-16 23:29 ` [PATCH 15/23] fuzzy: stop __stress_scrub_fsx_loop if fsx fails Darrick J. Wong
@ 2025-01-16 23:29 ` Darrick J. Wong
2025-01-16 23:29 ` [PATCH 17/23] fuzzy: always stop the scrub fsstress loop on error Darrick J. Wong
` (6 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:29 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Some of the scrub stress tests (e.g. xfs/796) walk the directory tree to
find filepaths to scrub, and load the entire list of paths into a bash
array. On a large filesystem or a long-running test this is hugely
wasteful of memory because we use each path exactly once.
Fix __stress_one_scrub_loop to avoid this by reading lines directly from
the output of the xfsfind utility. However, we play some games with fd
77 so that the processes in the loop body will use the same stdin as the
test and /not/ the piped stdout of xfsfind.
To avoid read(1) becoming confused by newlines in the file paths, adapt
xfsfind to print nulls between pathnames, and the bash code to recognize
them.
This was a debugging patch while I was trying to figure out why xfs/286
and other scrub soak tests started OOMing after the v2024.12.08 changes,
though in the end the OOMs were the result of memory leaks in fsstress.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/fuzzy | 22 +++++++++++++---------
src/xfsfind.c | 14 +++++++++++---
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/common/fuzzy b/common/fuzzy
index 46771b1d117106..f97c92340ba9ae 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -861,14 +861,14 @@ __stress_one_scrub_loop() {
;;
esac
- local target_cmd=(echo "$scrub_tgt")
+ local target_cmd=(echo -en "$scrub_tgt\0")
case "$scrub_tgt" in
- "%file%") target_cmd=($here/src/xfsfind -q "$SCRATCH_MNT");;
- "%attrfile%") target_cmd=($here/src/xfsfind -qa "$SCRATCH_MNT");;
- "%datafile%") target_cmd=($here/src/xfsfind -qb "$SCRATCH_MNT");;
- "%dir%") target_cmd=($here/src/xfsfind -qd "$SCRATCH_MNT");;
- "%regfile%") target_cmd=($here/src/xfsfind -qr "$SCRATCH_MNT");;
- "%cowfile%") target_cmd=($here/src/xfsfind -qs "$SCRATCH_MNT");;
+ "%file%") target_cmd=($here/src/xfsfind -0q "$SCRATCH_MNT");;
+ "%attrfile%") target_cmd=($here/src/xfsfind -0qa "$SCRATCH_MNT");;
+ "%datafile%") target_cmd=($here/src/xfsfind -0qb "$SCRATCH_MNT");;
+ "%dir%") target_cmd=($here/src/xfsfind -0qd "$SCRATCH_MNT");;
+ "%regfile%") target_cmd=($here/src/xfsfind -0qr "$SCRATCH_MNT");;
+ "%cowfile%") target_cmd=($here/src/xfsfind -0qs "$SCRATCH_MNT");;
esac
while __stress_scrub_running "$scrub_startat" "$runningfile"; do
@@ -876,12 +876,16 @@ __stress_one_scrub_loop() {
done
while __stress_scrub_running "$end" "$runningfile"; do
- readarray -t fnames < <("${target_cmd[@]}" 2>> $seqres.full)
- for fname in "${fnames[@]}"; do
+ # Attach the stdout of xfsfind to fd 77 so that we can read
+ # pathnames from that file descriptor without passing the pipe
+ # to the loop body as stdin.
+ exec 77< <("${target_cmd[@]}" 2>> $seqres.full)
+ while read -u 77 -d '' fname; do
$XFS_IO_PROG -x "${xfs_io_args[@]}" "$fname" 2>&1 | \
__stress_scrub_filter_output "${extra_filters[@]}"
__stress_scrub_running "$end" "$runningfile" || break
done
+ exec 77<&-
done
}
diff --git a/src/xfsfind.c b/src/xfsfind.c
index c81deaf64f57e9..2043d01ded3210 100644
--- a/src/xfsfind.c
+++ b/src/xfsfind.c
@@ -20,6 +20,7 @@ static int want_dir;
static int want_regfile;
static int want_sharedfile;
static int report_errors = 1;
+static int print0;
static int
check_datafile(
@@ -115,6 +116,7 @@ print_help(
printf("\n");
printf("Print all file paths matching any of the given predicates.\n");
printf("\n");
+ printf("-0 Print nulls between paths instead of newlines.\n");
printf("-a Match files with xattrs.\n");
printf("-b Match files with data blocks.\n");
printf("-d Match directories.\n");
@@ -208,8 +210,13 @@ visit(
out_fd:
close(fd);
out:
- if (printme)
- printf("%s\n", path);
+ if (printme) {
+ if (print0)
+ printf("%s%c", path, 0);
+ else
+ printf("%s\n", path);
+ fflush(stdout);
+ }
return retval;
}
@@ -236,8 +243,9 @@ main(
int c;
int ret;
- while ((c = getopt(argc, argv, "abdqrs")) >= 0) {
+ while ((c = getopt(argc, argv, "0abdqrs")) >= 0) {
switch (c) {
+ case '0': print0 = 1; break;
case 'a': want_attrfile = 1; break;
case 'b': want_datafile = 1; break;
case 'd': want_dir = 1; break;
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 17/23] fuzzy: always stop the scrub fsstress loop on error
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (15 preceding siblings ...)
2025-01-16 23:29 ` [PATCH 16/23] fuzzy: don't use readarray for xfsfind output Darrick J. Wong
@ 2025-01-16 23:29 ` Darrick J. Wong
2025-01-16 23:29 ` [PATCH 18/23] fuzzy: port fsx and fsstress loop to use --duration Darrick J. Wong
` (5 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:29 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Always abort the non-remount scrub fsstress loop in
__stress_scrub_fsstress_loop if fsstress returns a nonzero exit code.
Cc: <fstests@vger.kernel.org> # v2023.01.15
Fixes: 20df87599f66d0 ("fuzzy: make scrub stress loop control more robust")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/fuzzy | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/common/fuzzy b/common/fuzzy
index f97c92340ba9ae..e9150173a5d723 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -1012,6 +1012,7 @@ __stress_scrub_fsstress_loop() {
local remount_period="$3"
local stress_tgt="$4"
local focus=()
+ local res
case "$stress_tgt" in
"parent")
@@ -1143,7 +1144,9 @@ __stress_scrub_fsstress_loop() {
# Need to recheck running conditions if we cleared anything
__stress_scrub_clean_scratch && continue
_run_fsstress $args >> $seqres.full
- echo "fsstress exits with $? at $(date)" >> $seqres.full
+ res=$?
+ echo "$mode fsstress exits with $res at $(date)" >> $seqres.full
+ [ "$res" -ne 0 ] && break;
done
rm -f "$runningfile"
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 18/23] fuzzy: port fsx and fsstress loop to use --duration
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (16 preceding siblings ...)
2025-01-16 23:29 ` [PATCH 17/23] fuzzy: always stop the scrub fsstress loop on error Darrick J. Wong
@ 2025-01-16 23:29 ` Darrick J. Wong
2025-01-16 23:30 ` [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR Darrick J. Wong
` (4 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:29 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Quite a while ago, I added --duration= arguments to fsx and fsstress,
and apparently I forgot to update the scrub stress loops to use them.
Replace the usage of timeout(1) for the remount_period versions of the
loop to clean up that code; and convert the non-remount loop so that
they don't run over time.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/fuzzy | 40 ++++++++++++++++++++++++++++------------
1 file changed, 28 insertions(+), 12 deletions(-)
diff --git a/common/fuzzy b/common/fuzzy
index e9150173a5d723..331bf5ad7bbafa 100644
--- a/common/fuzzy
+++ b/common/fuzzy
@@ -939,6 +939,22 @@ __stress_scrub_clean_scratch() {
return 0
}
+# Compute a --duration= interval for fsx and fsstress
+___stress_scrub_duration()
+{
+ local end="$1"
+ local remount_period="$2"
+ local now="$(date +%s)"
+ local delta="$((end - now))"
+
+ test "$delta" -lt 0 && delta=0
+
+ test -n "$remount_period" && test "$remount_period" -lt "$delta" && \
+ delta="$remount_period"
+
+ echo "--duration=$delta"
+}
+
# Run fsx while we're testing online fsck.
__stress_scrub_fsx_loop() {
local end="$1"
@@ -946,6 +962,7 @@ __stress_scrub_fsx_loop() {
local remount_period="$3"
local stress_tgt="$4" # ignored
local focus=(-q -X) # quiet, validate file contents
+ local duration
local res
# As of November 2022, 2 million fsx ops should be enough to keep
@@ -965,17 +982,12 @@ __stress_scrub_fsx_loop() {
# anything.
test "$mode" = "rw" && __stress_scrub_clean_scratch && continue
- timeout -s TERM "$remount_period" $here/ltp/fsx \
- $args $rw_arg >> $seqres.full
+ duration=$(___stress_scrub_duration "$end" "$remount_period")
+ $here/ltp/fsx $duration $args $rw_arg >> $seqres.full
res=$?
echo "$mode fsx exits with $res at $(date)" >> $seqres.full
- if [ "$res" -ne 0 ] && [ "$res" -ne 124 ]; then
- # Stop if fsx returns error. Mask off
- # the magic code 124 because that is how the
- # timeout(1) program communicates that we ran
- # out of time.
- break;
- fi
+ test "$res" -ne 0 && break
+
if [ "$mode" = "rw" ]; then
mode="ro"
rw_arg="-t 0 -w 0 -FHzCIJBE0"
@@ -997,7 +1009,8 @@ __stress_scrub_fsx_loop() {
while __stress_scrub_running "$end" "$runningfile"; do
# Need to recheck running conditions if we cleared anything
__stress_scrub_clean_scratch && continue
- $here/ltp/fsx $args >> $seqres.full
+ duration=$(___stress_scrub_duration "$end" "$remount_period")
+ $here/ltp/fsx $duration $args >> $seqres.full
res=$?
echo "fsx exits with $res at $(date)" >> $seqres.full
test "$res" -ne 0 && break
@@ -1013,6 +1026,7 @@ __stress_scrub_fsstress_loop() {
local stress_tgt="$4"
local focus=()
local res
+ local duration
case "$stress_tgt" in
"parent")
@@ -1115,7 +1129,8 @@ __stress_scrub_fsstress_loop() {
# anything.
test "$mode" = "rw" && __stress_scrub_clean_scratch && continue
- _run_fsstress_bg $args $rw_arg >> $seqres.full
+ duration=$(___stress_scrub_duration "$end" "$remount_period")
+ _run_fsstress_bg $duration $args $rw_arg >> $seqres.full
sleep $remount_period
_kill_fsstress
res=$?
@@ -1143,7 +1158,8 @@ __stress_scrub_fsstress_loop() {
while __stress_scrub_running "$end" "$runningfile"; do
# Need to recheck running conditions if we cleared anything
__stress_scrub_clean_scratch && continue
- _run_fsstress $args >> $seqres.full
+ duration=$(___stress_scrub_duration "$end" "$remount_period")
+ _run_fsstress $duration $args >> $seqres.full
res=$?
echo "$mode fsstress exits with $res at $(date)" >> $seqres.full
[ "$res" -ne 0 ] && break;
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (17 preceding siblings ...)
2025-01-16 23:29 ` [PATCH 18/23] fuzzy: port fsx and fsstress loop to use --duration Darrick J. Wong
@ 2025-01-16 23:30 ` Darrick J. Wong
2025-01-21 5:05 ` Dave Chinner
2025-01-16 23:30 ` [PATCH 20/23] fix _require_scratch_duperemove ordering Darrick J. Wong
` (3 subsequent siblings)
22 siblings, 1 reply; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:30 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Now that we can pkill only processes that were started by this test, we
don't need to copy the fsstress binary to $TEST_DIR to avoid killing the
wrong program instances. This avoids a whole slew of ETXTBSY problems
with scrub stress tests that run multiple copies of fsstress in the
background.
Revert most of the changes to generic/270, because it wants to do
something fancy with the fsstress binary, so it needs to control the
process directly.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/rc | 14 +++++---------
tests/generic/270 | 10 ++++++----
tests/generic/482 | 1 +
3 files changed, 12 insertions(+), 13 deletions(-)
diff --git a/common/rc b/common/rc
index d7f3c48eafe590..25eb13ab2c5a48 100644
--- a/common/rc
+++ b/common/rc
@@ -56,11 +56,9 @@ _pgrep()
# task name to kill.
#
# If tasks want to start fsstress themselves (e.g. under a different uid) then
-# they can set up _FSSTRESS_BIN and record _FSSTRESS_PID themselves. Then if the
-# test is killed then it will get cleaned up automatically.
+# they can record _FSSTRESS_PID themselves. Then if the test is killed then it
+# will get cleaned up automatically.
-_FSSTRESS_BIN="$seq.fsstress"
-_FSSTRESS_PROG="$TEST_DIR/$seq.fsstress"
_FSSTRESS_PID=""
_wait_for_fsstress()
{
@@ -71,7 +69,6 @@ _wait_for_fsstress()
ret=$?
unset _FSSTRESS_PID
fi
- rm -f $_FSSTRESS_PROG
return $ret
}
@@ -80,8 +77,8 @@ _kill_fsstress()
{
if [ -n "$_FSSTRESS_PID" ]; then
# use SIGPIPE to avoid "Killed" messages from bash
- echo "killing $_FSSTRESS_BIN" >> $seqres.full
- _pkill -PIPE $_FSSTRESS_BIN >> $seqres.full 2>&1
+ echo "killing fsstress" >> $seqres.full
+ _pkill -PIPE fsstress >> $seqres.full 2>&1
_wait_for_fsstress
return $?
fi
@@ -89,8 +86,7 @@ _kill_fsstress()
_run_fsstress_bg()
{
- cp -f $FSSTRESS_PROG $_FSSTRESS_PROG
- $_FSSTRESS_PROG $FSSTRESS_AVOID "$@" >> $seqres.full 2>&1 &
+ $FSSTRESS_PROG $FSSTRESS_AVOID "$@" >> $seqres.full 2>&1 &
_FSSTRESS_PID=$!
}
diff --git a/tests/generic/270 b/tests/generic/270
index d74971bb535239..ce51592004fe77 100755
--- a/tests/generic/270
+++ b/tests/generic/270
@@ -28,8 +28,8 @@ _workout()
args=`_scale_fsstress_args -p128 -n999999999 -f setattr=1 $FSSTRESS_AVOID -d $out`
echo "fsstress $args" >> $seqres.full
# Grant chown capability
- cp $FSSTRESS_PROG $_FSSTRESS_PROG
- $SETCAP_PROG cap_chown=epi $_FSSTRESS_PROG
+ cp $FSSTRESS_PROG $tmp.fsstress.bin
+ $SETCAP_PROG cap_chown=epi $tmp.fsstress.bin
# io_uring accounts memory it needs under the rlimit memlocked option,
# which can be quite low on some setups (especially 64K pagesize). root
@@ -37,7 +37,7 @@ _workout()
# io_uring_queue_init fail on ENOMEM, set max locked memory to unlimited
# temporarily.
ulimit -l unlimited
- _su $qa_user -c "$_FSSTRESS_PROG $args" > /dev/null 2>&1 &
+ _su $qa_user -c "$tmp.fsstress.bin $args" > /dev/null 2>&1 &
_FSSTRESS_PID=$!
echo "Run dd writers in parallel"
@@ -50,7 +50,9 @@ _workout()
sleep $enospc_time
done
- _kill_fsstress
+ _pkill -PIPE -f fsstress
+ pidwait $_FSSTRESS_PID
+ return 0
}
_require_quota
diff --git a/tests/generic/482 b/tests/generic/482
index 0efc026a160040..8c114ee03058c6 100755
--- a/tests/generic/482
+++ b/tests/generic/482
@@ -68,6 +68,7 @@ lowspace=$((1024*1024 / 512)) # 1m low space threshold
# Use a thin device to provide deterministic discard behavior. Discards are used
# by the log replay tool for fast zeroing to prevent out-of-order replay issues.
+_test_unmount
_dmthin_init $devsize $devsize $csize $lowspace
_log_writes_init $DMTHIN_VOL_DEV
_log_writes_mkfs >> $seqres.full 2>&1
^ permalink raw reply related [flat|nested] 118+ messages in thread* Re: [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR
2025-01-16 23:30 ` [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR Darrick J. Wong
@ 2025-01-21 5:05 ` Dave Chinner
2025-01-22 3:52 ` Darrick J. Wong
0 siblings, 1 reply; 118+ messages in thread
From: Dave Chinner @ 2025-01-21 5:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: zlang, hch, fstests, linux-xfs
On Thu, Jan 16, 2025 at 03:30:07PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> Now that we can pkill only processes that were started by this test, we
> don't need to copy the fsstress binary to $TEST_DIR to avoid killing the
> wrong program instances. This avoids a whole slew of ETXTBSY problems
> with scrub stress tests that run multiple copies of fsstress in the
> background.
>
> Revert most of the changes to generic/270, because it wants to do
> something fancy with the fsstress binary, so it needs to control the
> process directly.
>
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
Change looks fine, though this should be a lot further up near the
top of the patchset so that the change to g/482 and the revert here
is not necessary at all.
With that reordering done:
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 118+ messages in thread
* Re: [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR
2025-01-21 5:05 ` Dave Chinner
@ 2025-01-22 3:52 ` Darrick J. Wong
0 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-22 3:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: zlang, hch, fstests, linux-xfs
On Tue, Jan 21, 2025 at 04:05:38PM +1100, Dave Chinner wrote:
> On Thu, Jan 16, 2025 at 03:30:07PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Now that we can pkill only processes that were started by this test, we
> > don't need to copy the fsstress binary to $TEST_DIR to avoid killing the
> > wrong program instances. This avoids a whole slew of ETXTBSY problems
> > with scrub stress tests that run multiple copies of fsstress in the
> > background.
> >
> > Revert most of the changes to generic/270, because it wants to do
> > something fancy with the fsstress binary, so it needs to control the
> > process directly.
> >
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
>
> Change looks fine, though this should be a lot further up near the
> top of the patchset so that the change to g/482 and the revert here
> is not necessary at all.
>
> With that reordering done:
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
Done, and thanks for responding to the big patch set.
--D
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply [flat|nested] 118+ messages in thread
* [PATCH 20/23] fix _require_scratch_duperemove ordering
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (18 preceding siblings ...)
2025-01-16 23:30 ` [PATCH 19/23] common/rc: don't copy fsstress to $TEST_DIR Darrick J. Wong
@ 2025-01-16 23:30 ` Darrick J. Wong
2025-01-16 23:30 ` [PATCH 21/23] fsstress: fix a memory leak Darrick J. Wong
` (2 subsequent siblings)
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:30 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Zorro complained that generic/559 stopped working, and I noticed that
the duperemove invocation in the _require_scratch_duperemove function
would fail with:
Error 2: No such file or directory while getting path to file /opt/file1. Skipping.
Error 2: No such file or directory while getting path to file /opt/file2. Skipping.
No dedupe candidates found.
Gathering file list...
The cause of this is the incorrect placement of _require_scratch_dedupe
after a _scratch_mount. _require_scratch_dedupe formats, mounts, tests,
and unmounts the scratch filesystem, which means that it should not come
between a _scratch_mount call and code that uses $SCRATCH_MNT.
Cc: <fstests@vger.kernel.org> # v2024.12.22
Fixes: 3b9f5fc7d7d853 ("common: call _require_scratch_dedupe from _require_scratch_duperemove")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
common/reflink | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/common/reflink b/common/reflink
index 9177c45e70bb37..757f06c1c69fa7 100644
--- a/common/reflink
+++ b/common/reflink
@@ -80,11 +80,11 @@ _require_scratch_duperemove()
{
_require_scratch
_require_command "$DUPEREMOVE_PROG" duperemove
-
- _scratch_mkfs > /dev/null
- _scratch_mount
_require_scratch_dedupe
+ _scratch_mkfs > /dev/null
+ _scratch_mount
+
dd if=/dev/zero of="$SCRATCH_MNT/file1" bs=128k count=1 >& /dev/null
dd if=/dev/zero of="$SCRATCH_MNT/file2" bs=128k count=1 >& /dev/null
if ! "$DUPEREMOVE_PROG" -d "$SCRATCH_MNT/file1" \
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 21/23] fsstress: fix a memory leak
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (19 preceding siblings ...)
2025-01-16 23:30 ` [PATCH 20/23] fix _require_scratch_duperemove ordering Darrick J. Wong
@ 2025-01-16 23:30 ` Darrick J. Wong
2025-01-16 23:30 ` [PATCH 22/23] fsx: fix leaked log file pointer Darrick J. Wong
2025-01-16 23:31 ` [PATCH 23/23] build: initialize stack variables to zero by default Darrick J. Wong
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:30 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Someone forgot to free the iovec that readv_f allocates.
Fixes: 80499d8f5f251e ("fsstress: new writev and readv operations test")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
ltp/fsstress.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 9e8eaa6d8656da..14c29921e8b0f8 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -4652,6 +4652,7 @@ readv_f(opnum_t opno, long r)
}
e = readv(fd, iov, iovcnt) < 0 ? errno : 0;
+ free(iov);
free(buf);
if (v)
printf("%d/%lld: readv %s%s [%lld,%d,%d] %d\n",
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 22/23] fsx: fix leaked log file pointer
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (20 preceding siblings ...)
2025-01-16 23:30 ` [PATCH 21/23] fsstress: fix a memory leak Darrick J. Wong
@ 2025-01-16 23:30 ` Darrick J. Wong
2025-01-16 23:31 ` [PATCH 23/23] build: initialize stack variables to zero by default Darrick J. Wong
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:30 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Fix a resource leaks in fsx, where we fail to close the fsx logfile,
because the C library could have some buffered contents that aren't
flushed when the program terminates. glibc seems to do this for us, but
I wouldn't be so sure about the others.
Fixes: 3f742550dfed84 ("fsx: add support for recording operations to a file")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
ltp/fsx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ltp/fsx.c b/ltp/fsx.c
index 9efd2f5c86d11c..2c19fff880330a 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -3345,6 +3345,7 @@ main(int argc, char **argv)
if (recordops)
logdump();
+ fclose(fsxlogf);
exit(0);
return 0;
}
^ permalink raw reply related [flat|nested] 118+ messages in thread* [PATCH 23/23] build: initialize stack variables to zero by default
2025-01-16 23:23 ` [PATCHSET 1/7] fstests: random fixes for v2025.01.12 Darrick J. Wong
` (21 preceding siblings ...)
2025-01-16 23:30 ` [PATCH 22/23] fsx: fix leaked log file pointer Darrick J. Wong
@ 2025-01-16 23:31 ` Darrick J. Wong
22 siblings, 0 replies; 118+ messages in thread
From: Darrick J. Wong @ 2025-01-16 23:31 UTC (permalink / raw)
To: zlang, djwong; +Cc: hch, fstests, linux-xfs
From: Darrick J. Wong <djwong@kernel.org>
Newer versions of gcc and clang can include the ability to zero stack
variables by default. Let's enable it so that we (a) reduce the risk of
writing stack contents to disk somewhere and (b) try to reduce
unpredictable program behavior based on random stack contents. The
kernel added this 6 years ago, so I think it's mature enough for
fstests.
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
configure.ac | 1 +
include/builddefs.in | 3 ++-
m4/package_libcdev.m4 | 14 ++++++++++++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/configure.ac b/configure.ac
index c81411e735a90d..f3c8c643f0eb9e 100644
--- a/configure.ac
+++ b/configure.ac
@@ -72,6 +72,7 @@ AC_HAVE_NFTW
AC_HAVE_RLIMIT_NOFILE
AC_NEED_INTERNAL_XFS_IOC_EXCHANGE_RANGE
AC_HAVE_FICLONE
+AC_HAVE_TRIVIAL_AUTO_VAR_INIT
AC_CHECK_FUNCS([renameat2])
AC_CHECK_FUNCS([reallocarray])
diff --git a/include/builddefs.in b/include/builddefs.in
index 7274cde8d0814c..5b5864278682a4 100644
--- a/include/builddefs.in
+++ b/include/builddefs.in
@@ -76,6 +76,7 @@ NEED_INTERNAL_XFS_IOC_EXCHANGE_RANGE = @need_internal_xfs_ioc_exchange_range@
HAVE_FICLONE = @have_ficlone@
GCCFLAGS = -funsigned-char -fno-strict-aliasing -Wall
+SANITIZER_CFLAGS += @autovar_init_cflags@
ifeq ($(PKG_PLATFORM),linux)
PCFLAGS = -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 $(GCCFLAGS)
@@ -90,7 +91,7 @@ GCFLAGS = $(OPTIMIZER) $(DEBUG) $(CPPFLAGS) \
-I$(TOPDIR)/include -DVERSION=\"$(PKG_VERSION)\"
# Global, Platform, Local CFLAGS
-CFLAGS += $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
+CFLAGS += $(GCFLAGS) $(PCFLAGS) $(LCFLAGS) $(SANITIZER_CFLAGS)
include $(TOPDIR)/include/buildmacros
diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
index a0d50f4d9b68e4..ed8fe6e32ae00a 100644
--- a/m4/package_libcdev.m4
+++ b/m4/package_libcdev.m4
@@ -72,3 +72,17 @@ AC_DEFUN([AC_HAVE_FICLONE],
AC_MSG_RESULT(yes)],[AC_MSG_RESULT(no)])
AC_SUBST(have_ficlone)
])
+
+# Check if we have -ftrivial-auto-var-init=zero
+AC_DEFUN([AC_HAVE_TRIVIAL_AUTO_VAR_INIT],
+ [ AC_MSG_CHECKING([if C compiler supports zeroing automatic vars])
+ OLD_CFLAGS="$CFLAGS"
+ TEST_CFLAGS="-ftrivial-auto-var-init=zero"
+ CFLAGS="$CFLAGS $TEST_CFLAGS"
+ AC_LINK_IFELSE([AC_LANG_PROGRAM([])],
+ [AC_MSG_RESULT([yes])]
+ [autovar_init_cflags=$TEST_CFLAGS],
+ [AC_MSG_RESULT([no])])
+ CFLAGS="${OLD_CFLAGS}"
+ AC_SUBST(autovar_init_cflags)
+ ])
^ permalink raw reply related [flat|nested] 118+ messages in thread