| Message ID | 173870406322.546134.11678961837706398324.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/34] generic/476: fix fsstress process management | expand |
On Tue, Feb 04, 2025 at 01:25:57PM -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. I thought you were going to drop this because pidns stuff available. Also, because it is only check parallel that needs the pidns isolation, and I'm not doing that external to check. Hence we can just get rid of the 'pkill --parent' requirement because the concurrent tests are already being run in isolated PID namespaces... Regardless, if ppl still want to both pid session and pidns directly into check, the code is fine. Just one little nit: > diff --git a/tools/run_seq_setsid b/tools/run_seq_setsid > new file mode 100755 > index 00000000000000..5938f80e689255 > --- /dev/null > +++ b/tools/run_seq_setsid > @@ -0,0 +1,22 @@ > +#!/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" "$@" The wrapper should be called 'run_setsid' because what check is using it for has nothing to do with what the script actually does. With that change: Reviewed-by: Dave Chinner <dchinner@redhat.com>
On Wed, Feb 05, 2025 at 11:23:10AM +1100, Dave Chinner wrote: > On Tue, Feb 04, 2025 at 01:25:57PM -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. > > I thought you were going to drop this because pidns stuff available. > Also, because it is only check parallel that needs the pidns > isolation, and I'm not doing that external to check. Hence we can > just get rid of the 'pkill --parent' requirement because the > concurrent tests are already being run in isolated PID namespaces... > > Regardless, if ppl still want to both pid session and pidns directly > into check, the code is fine. I'll make that clearer in the commit message. > Just one little nit: > > > diff --git a/tools/run_seq_setsid b/tools/run_seq_setsid > > new file mode 100755 > > index 00000000000000..5938f80e689255 > > --- /dev/null > > +++ b/tools/run_seq_setsid > > @@ -0,0 +1,22 @@ > > +#!/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" "$@" > > The wrapper should be called 'run_setsid' because what check is > using it for has nothing to do with what the script actually does. Done. > With that change: > > Reviewed-by: Dave Chinner <dchinner@redhat.com> Thank you! --D > -- > Dave Chinner > david@fromorbit.com >
diff --git a/check b/check index 5cb4e7eb71ce07..d854515ed1aac5 100755 --- a/check +++ b/check @@ -698,18 +698,46 @@ _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 "${cmd[@]}" + systemd-run --quiet --unit "${unit}" --scope \ + ./tools/run_seq_setsid "./$seq" & + 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. + ./tools/run_seq_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 fi } @@ -718,9 +746,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/rc b/common/rc index 2b56d6de9c9cb1..bda80995f8dd55 100644 --- a/common/rc +++ b/common/rc @@ -33,7 +33,7 @@ _test_sync() # Kill only the processes started by this test. _pkill() { - pkill "$@" + pkill --session 0 "$@" } # Common execution handling for fsstress invocation. @@ -2732,9 +2732,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/tools/run_seq_setsid b/tools/run_seq_setsid new file mode 100755 index 00000000000000..5938f80e689255 --- /dev/null +++ b/tools/run_seq_setsid @@ -0,0 +1,22 @@ +#!/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" "$@"