| Message ID | 173706974197.1927324.9208284704325894988.stgit@frogsfrogsfrogs (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [01/23] generic/476: fix fsstress process management | expand |
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.
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