[PATCH 0/2] revert "setsid" and "privatens" test ways from check

[PATCH 0/2] revert "setsid" and "privatens" test ways from check

[Zorro Lang]

To isolate check-parallel from traditional check, let's revert "setsid" and
"privatens" test ways from check at first, due to "check" doesn't need them.
This also helps to avoid some regression issues or behavior changes.

The run_setsid and related things are removed totally, the privatens is removed
from check too, but the run_privatens script is retained for check-parallel.
Later, check-parallel will run in private name space way, won't affect the
check running.

More details refer to two patches. And please feel free to tell me if I miss
something.

Thanks,
Zorro

[PATCH 1/2] fstests: remove privatens test way from check

[Zorro Lang]

This patch totally revert:
  ce7f796ad check: remove the deprecation of sessionid
  336784e3d check: disable HAVE_PRIVATENS by default
  949bdf8ea check: deprecate using process sessions to isolate test instances
and partially revert:
  247ab01fa check: run tests in a private pid/mount namespace

So it does:
1. Remove "run_privatens" related things from xfstests/check, due to
   check doesn't need that test way.
2. Keep run_privatens script and related changes for check-parallel.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 check     | 37 +++++++++++--------------------------
 common/rc | 12 ++----------
 2 files changed, 13 insertions(+), 36 deletions(-)

diff --git a/check b/check
index 33eb3e085..ba853ae85 100755
--- a/check
+++ b/check
@@ -674,14 +674,6 @@ _stash_test_status() {
 	esac
 }
 
-# Don't try "privatens" by default, it's experimental for now.
-if [ "$TRY_PRIVATENS" = "yes" ];then
-	# Can we run in a private pid/mount namespace?
-	HAVE_PRIVATENS=
-	./tools/run_privatens bash -c "exit 77"
-	test $? -eq 77 && HAVE_PRIVATENS=yes
-fi
-
 # Can we run systemd scopes?
 HAVE_SYSTEMD_SCOPES=
 systemctl reset-failed "fstests-check" &>/dev/null
@@ -699,29 +691,22 @@ _adjust_oom_score -500
 # the system runs out of memory it'll be the test that gets killed and not the
 # test framework.  The test is run in a separate process without any of our
 # functions, so we open-code adjusting the OOM score.
+#
+# If systemd is available, run the entire test script in a scope so that we can
+# kill all subprocesses of the test if it fails to clean up after itself.  This
+# is essential for ensuring that the post-test unmount succeeds.  Note that
+# 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 res
 	unset CHILDPID
 	unset FSTESTS_ISOL	# set by tools/run_seq_*
 
-	if [ -n "${HAVE_PRIVATENS}" ]; then
-		# If pid and mount namespaces are available, run the whole test
-		# inside them so that the test cannot access any process or
-		# /tmp contents that it does not itself create.  The ./$seq
-		# process is considered the "init" process of the pid
-		# namespace, so all subprocesses will be sent SIGKILL when it
-		# terminates.
-		./tools/run_privatens "./$seq"
-		res=$?
-	elif [ -n "${HAVE_SYSTEMD_SCOPES}" ]; then
-		# If systemd is available, run the entire test script in a
-		# scope so that we can kill all subprocesses of the test if it
-		# fails to clean up after itself.  This is essential for
-		# ensuring that the post-test unmount succeeds.  Note that
-		# 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.
+	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 \
diff --git a/common/rc b/common/rc
index e51686389..55c384a63 100644
--- a/common/rc
+++ b/common/rc
@@ -33,11 +33,7 @@ _test_sync()
 # Kill only the processes started by this test.
 _pkill()
 {
-	if [ "$FSTESTS_ISOL" = "setsid" ]; then
-		pkill --session 0 "$@"
-	else
-		pkill "$@"
-	fi
+	pkill --session 0 "$@"
 }
 
 # Find only the test processes started by this test
@@ -2796,11 +2792,7 @@ _require_user_exists()
 # not, passing $SHELL in this manner works both for "su" and "su -c cmd".
 _su()
 {
-	if [ "$FSTESTS_ISOL" = "setsid" ]; then
-		su --session-command $SHELL "$@"
-	else
-		su "$@"
-	fi
+	su --session-command $SHELL "$@"
 }
 
 # check if a user exists and is able to execute commands.
-- 
2.47.1

Re: [PATCH 1/2] fstests: remove privatens test way from check

[Darrick J. Wong]

On Fri, Mar 21, 2025 at 03:28:11AM +0800, Zorro Lang wrote:
> This patch totally revert:
>   ce7f796ad check: remove the deprecation of sessionid
>   336784e3d check: disable HAVE_PRIVATENS by default
>   949bdf8ea check: deprecate using process sessions to isolate test instances
> and partially revert:
>   247ab01fa check: run tests in a private pid/mount namespace
> 
> So it does:
> 1. Remove "run_privatens" related things from xfstests/check, due to
>    check doesn't need that test way.
> 2. Keep run_privatens script and related changes for check-parallel.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>

Seems to run fine here, so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  check     | 37 +++++++++++--------------------------
>  common/rc | 12 ++----------
>  2 files changed, 13 insertions(+), 36 deletions(-)
> 
> diff --git a/check b/check
> index 33eb3e085..ba853ae85 100755
> --- a/check
> +++ b/check
> @@ -674,14 +674,6 @@ _stash_test_status() {
>  	esac
>  }
>  
> -# Don't try "privatens" by default, it's experimental for now.
> -if [ "$TRY_PRIVATENS" = "yes" ];then
> -	# Can we run in a private pid/mount namespace?
> -	HAVE_PRIVATENS=
> -	./tools/run_privatens bash -c "exit 77"
> -	test $? -eq 77 && HAVE_PRIVATENS=yes
> -fi
> -
>  # Can we run systemd scopes?
>  HAVE_SYSTEMD_SCOPES=
>  systemctl reset-failed "fstests-check" &>/dev/null
> @@ -699,29 +691,22 @@ _adjust_oom_score -500
>  # the system runs out of memory it'll be the test that gets killed and not the
>  # test framework.  The test is run in a separate process without any of our
>  # functions, so we open-code adjusting the OOM score.
> +#
> +# If systemd is available, run the entire test script in a scope so that we can
> +# kill all subprocesses of the test if it fails to clean up after itself.  This
> +# is essential for ensuring that the post-test unmount succeeds.  Note that
> +# 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 res
>  	unset CHILDPID
>  	unset FSTESTS_ISOL	# set by tools/run_seq_*
>  
> -	if [ -n "${HAVE_PRIVATENS}" ]; then
> -		# If pid and mount namespaces are available, run the whole test
> -		# inside them so that the test cannot access any process or
> -		# /tmp contents that it does not itself create.  The ./$seq
> -		# process is considered the "init" process of the pid
> -		# namespace, so all subprocesses will be sent SIGKILL when it
> -		# terminates.
> -		./tools/run_privatens "./$seq"
> -		res=$?
> -	elif [ -n "${HAVE_SYSTEMD_SCOPES}" ]; then
> -		# If systemd is available, run the entire test script in a
> -		# scope so that we can kill all subprocesses of the test if it
> -		# fails to clean up after itself.  This is essential for
> -		# ensuring that the post-test unmount succeeds.  Note that
> -		# 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.
> +	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 \
> diff --git a/common/rc b/common/rc
> index e51686389..55c384a63 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -33,11 +33,7 @@ _test_sync()
>  # Kill only the processes started by this test.
>  _pkill()
>  {
> -	if [ "$FSTESTS_ISOL" = "setsid" ]; then
> -		pkill --session 0 "$@"
> -	else
> -		pkill "$@"
> -	fi
> +	pkill --session 0 "$@"
>  }
>  
>  # Find only the test processes started by this test
> @@ -2796,11 +2792,7 @@ _require_user_exists()
>  # not, passing $SHELL in this manner works both for "su" and "su -c cmd".
>  _su()
>  {
> -	if [ "$FSTESTS_ISOL" = "setsid" ]; then
> -		su --session-command $SHELL "$@"
> -	else
> -		su "$@"
> -	fi
> +	su --session-command $SHELL "$@"
>  }
>  
>  # check if a user exists and is able to execute commands.
> -- 
> 2.47.1
> 
> 

[PATCH 2/2] fstests: remove run_setsid test way from check

[Zorro Lang]

This patch partially revert most of the 88d60f434 ("common: fix pkill
by running test program in a separate session"), it does:
1. Remove run_setsid script
2. Remove all run_setsid related things from check
3. Keep the change in _scratch_xfs_stress_scrub_cleanup() which is a
   bug fix for xfs_scrub test.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 check            | 39 ++++++---------------------------------
 common/rc        |  4 ++--
 tools/Makefile   |  1 -
 tools/run_setsid | 22 ----------------------
 4 files changed, 8 insertions(+), 58 deletions(-)
 delete mode 100755 tools/run_setsid

diff --git a/check b/check
index ba853ae85..32890470a 100755
--- a/check
+++ b/check
@@ -698,46 +698,19 @@ _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 res
-	unset CHILDPID
-	unset FSTESTS_ISOL	# set by tools/run_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 \
-			./tools/run_setsid "./$seq" &
-		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.
-		./tools/run_setsid "./$seq" &
-		CHILDPID=$!
-		wait
-		res=$?
-		unset CHILDPID
-	fi
-
-	return $res
-}
-
-_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
 }
 
@@ -746,9 +719,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/common/rc b/common/rc
index 55c384a63..fa0f02ac4 100644
--- a/common/rc
+++ b/common/rc
@@ -33,7 +33,7 @@ _test_sync()
 # Kill only the processes started by this test.
 _pkill()
 {
-	pkill --session 0 "$@"
+	pkill "$@"
 }
 
 # Find only the test processes started by this test
@@ -2792,7 +2792,7 @@ _require_user_exists()
 # 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.
diff --git a/tools/Makefile b/tools/Makefile
index 2d502884f..af7adc2a6 100644
--- a/tools/Makefile
+++ b/tools/Makefile
@@ -7,7 +7,6 @@ include $(TOPDIR)/include/builddefs
 
 TOOLS_DIR = tools
 helpers=\
-	run_setsid \
 	run_privatens
 
 include $(BUILDRULES)
diff --git a/tools/run_setsid b/tools/run_setsid
deleted file mode 100755
index 5938f80e6..000000000
--- a/tools/run_setsid
+++ /dev/null
@@ -1,22 +0,0 @@
-#!/bin/bash
-
-# SPDX-License-Identifier: GPL-2.0
-# Copyright (c) 2025 Oracle.  All Rights Reserved.
-#
-# Try starting things in a new process session so that test processes have
-# something with which to filter only their own subprocesses.
-
-if [ -n "${FSTESTS_ISOL}" ]; then
-	# Allow the test to become a target of the oom killer
-	oom_knob="/proc/self/oom_score_adj"
-	test -w "${oom_knob}" && echo 250 > "${oom_knob}"
-
-	exec "$@"
-fi
-
-if [ -z "$1" ] || [ "$1" = "--help" ]; then
-	echo "Usage: $0 command [args...]"
-	exit 1
-fi
-
-FSTESTS_ISOL=setsid exec setsid "$0" "$@"
-- 
2.47.1

Re: [PATCH 2/2] fstests: remove run_setsid test way from check

[Darrick J. Wong]

On Fri, Mar 21, 2025 at 03:28:12AM +0800, Zorro Lang wrote:
> This patch partially revert most of the 88d60f434 ("common: fix pkill
> by running test program in a separate session"), it does:
> 1. Remove run_setsid script
> 2. Remove all run_setsid related things from check
> 3. Keep the change in _scratch_xfs_stress_scrub_cleanup() which is a
>    bug fix for xfs_scrub test.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>

This looks like a reasonable revert to me and it doesn't seem to break
anything so
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> ---
>  check            | 39 ++++++---------------------------------
>  common/rc        |  4 ++--
>  tools/Makefile   |  1 -
>  tools/run_setsid | 22 ----------------------
>  4 files changed, 8 insertions(+), 58 deletions(-)
>  delete mode 100755 tools/run_setsid
> 
> diff --git a/check b/check
> index ba853ae85..32890470a 100755
> --- a/check
> +++ b/check
> @@ -698,46 +698,19 @@ _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 res
> -	unset CHILDPID
> -	unset FSTESTS_ISOL	# set by tools/run_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 \
> -			./tools/run_setsid "./$seq" &
> -		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.
> -		./tools/run_setsid "./$seq" &
> -		CHILDPID=$!
> -		wait
> -		res=$?
> -		unset CHILDPID
> -	fi
> -
> -	return $res
> -}
> -
> -_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
>  }
>  
> @@ -746,9 +719,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/common/rc b/common/rc
> index 55c384a63..fa0f02ac4 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -33,7 +33,7 @@ _test_sync()
>  # Kill only the processes started by this test.
>  _pkill()
>  {
> -	pkill --session 0 "$@"
> +	pkill "$@"
>  }
>  
>  # Find only the test processes started by this test
> @@ -2792,7 +2792,7 @@ _require_user_exists()
>  # 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.
> diff --git a/tools/Makefile b/tools/Makefile
> index 2d502884f..af7adc2a6 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -7,7 +7,6 @@ include $(TOPDIR)/include/builddefs
>  
>  TOOLS_DIR = tools
>  helpers=\
> -	run_setsid \
>  	run_privatens
>  
>  include $(BUILDRULES)
> diff --git a/tools/run_setsid b/tools/run_setsid
> deleted file mode 100755
> index 5938f80e6..000000000
> --- a/tools/run_setsid
> +++ /dev/null
> @@ -1,22 +0,0 @@
> -#!/bin/bash
> -
> -# SPDX-License-Identifier: GPL-2.0
> -# Copyright (c) 2025 Oracle.  All Rights Reserved.
> -#
> -# Try starting things in a new process session so that test processes have
> -# something with which to filter only their own subprocesses.
> -
> -if [ -n "${FSTESTS_ISOL}" ]; then
> -	# Allow the test to become a target of the oom killer
> -	oom_knob="/proc/self/oom_score_adj"
> -	test -w "${oom_knob}" && echo 250 > "${oom_knob}"
> -
> -	exec "$@"
> -fi
> -
> -if [ -z "$1" ] || [ "$1" = "--help" ]; then
> -	echo "Usage: $0 command [args...]"
> -	exit 1
> -fi
> -
> -FSTESTS_ISOL=setsid exec setsid "$0" "$@"
> -- 
> 2.47.1
> 
> 

Re: [PATCH 0/2] revert "setsid" and "privatens" test ways from check

[Christoph Hellwig]

I can't really review this code as I don't understand it, but keeping
the special logic for the parallel run out of the main check script that
everyone uses sounds like a good idea.

Btw, any chance we could rename check-parallel to parallel-check or
similar?  The current name really messes up tab completion and mussle
memory for interactive xfstests run.

Re: [PATCH 0/2] revert "setsid" and "privatens" test ways from check

[Dave Chinner]

On Fri, Mar 21, 2025 at 03:28:10AM +0800, Zorro Lang wrote:
> To isolate check-parallel from traditional check, let's revert "setsid" and
> "privatens" test ways from check at first, due to "check" doesn't need them.
> This also helps to avoid some regression issues or behavior changes.
> 
> The run_setsid and related things are removed totally, the privatens is removed
> from check too, but the run_privatens script is retained for check-parallel.
> Later, check-parallel will run in private name space way, won't affect the
> check running.
> 
> More details refer to two patches. And please feel free to tell me if I miss
> something.

Looks fine to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com