On Wed, Mar 05, 2025 at 04:43:01AM +0800, Zorro Lang wrote:
> On Tue, Mar 04, 2025 at 12:04:58PM -0800, Darrick J. Wong wrote:
> > On Tue, Mar 04, 2025 at 04:50:46AM +0800, Zorro Lang wrote:
> > > Currently we have 3 ways to run a test case in _run_seq():
> > >
> > > if [ -n "${HAVE_PRIVATENS}" ]; then
> > > ./tools/run_privatens "./$seq"
> > > ...
> > > elif [ -n "${HAVE_SYSTEMD_SCOPES}" ]; then
> > > systemd-run --quiet --unit "${unit}" --scope \
> > > ./tools/run_setsid "./$seq" &
> > > ...
> > > else
> > > ./tools/run_setsid "./$seq" &
> > > ...
> > > fi
> > >
> > > The "privatens" way brings in some regressions. We need more time
> > > to develop and test this way, it's not time let it to be the
> > > first default choice, so isolate the HAVE_PRIVATENS initialization
> > > by a TRY_PRIVATENS parameter, and disable it by default.
> > >
> > > Set TRY_PRIVATENS=yes to give "privatens" a try, otherwise run in
> > > old ways. This patch can be removed after "privatens" way is stable.
> >
> > I think it's ok to turn off privatens for now, seeing as it's clearly
> > caused some regressions that I don't know how to fix. The ones I know
> > about are:
> >
> > 1. The btrfs/14[01] use of BASHPID (not fixed)
> > 2. The weird flock thing in generic/504 (fixed)
>
> And a bug report when /tmp isn't a mount point.
Oh yeah. Forgot that one. We could fix it this way:
diff --git a/check b/check
index ea92b0d62a..7196cfbfea 100755
--- a/check
+++ b/check
@@ -821,6 +821,20 @@ function run_section()
echo "SECTION -- $section"
fi
+ # If we're running in a private mount namespace, /tmp is a private
+ # directory. We /could/ just mkdir it, but we'd rather have people
+ # set those paths elsewhere.
+ if [ "$HAVE_PRIVATENS" = yes ] && [[ $TEST_DIR =~ ^\/tmp ]]; then
+ echo "$TEST_DIR: TEST_DIR must not be in /tmp"
+ status=1
+ exit
+ fi
+ if [ "$HAVE_PRIVATENS" = yes ] && [[ $SCRATCH_MNT =~ ^\/tmp ]]; then
+ echo "$SCRATCH_MNT: SCRATCH_MNT must not be in /tmp"
+ status=1
+ exit
+ fi
+
sect_start=`_wallclock`
if $RECREATE_TEST_DEV || [ "$OLD_FSTYP" != "$FSTYP" ]; then
echo "RECREATING -- $FSTYP on $TEST_DEV"
Or we could just have _begin_fstest mkdir them if they don't exist.
Unless it'd make more sense to have ./check define TEST_DIR and
SCRATCH_MNT itself? Imagine if it instead did this:
FSTESTS_RUN_DIR=/run/fstests-$$
mkdir -p $FSTESTS_RUN_DIR
mount -t tmpfs none $FSTESTS_RUN_DIR
test -n "$TEST_DEV" && mkdir -p $FSTESTS_RUN_DIR/test
test -n "$SCRATCH_MNT" && $FSTESTS_RUN_DIR/scratch
mount -o remount,ro $FSTESTS_RUN_DIR
test -n "$TEST_DEV" && TEST_DIR=$FSTESTS_RUN_DIR/test
test -n "$SCRATCH_MNT" && SCRATCH_MNT=$FSTESTS_RUN_DIR/scratch
Now the user doesn't have to create the directories themselves, and
since the underlying $FSTESTS_RUN_DIR is read only, mount failures will
show up immediately as write failures instead of filling up the wrong
filesystem.
--D
> > But then there's a bunch more complaints about "everything" being
> > broken. If things are still broken for you, please send detailed dumps
> > and let Zorro and I figure that out.
> >
> > Note that run_privatens is a lot cleaner than run_setsid -- as I've
> > stated elsewhere, using a new unix process session id to run a test is
> > very messy -- there's no longer a controlling tty, so SIGSTOP doesn't
> > work and SIGINT is masked by default.
> >
> > As I suggested elsewhere today, maybe the solution is to have the tests
> > that *require* the global pid namespace (anything calling the
> > btrfs*read*mirror* helpers) turn themselves off with a
> > "_require_non_privatens"; and then ./check can select setsid for any
> > test containing that phrase.
> >
> > Thoughts on this part?
>
> More likes a trick, anyway I think we can talk about the way to fix btrfs
> BASHPID related problems later. And I still don't know if it's the same
> root cause with Dave Sterba hit.
>
> >
> > --
> >
> > I'm ok with taking the quickest route to something that makes everyone
> > happy so that we can push to master again, and merge other peoples'
> > changes because I bet those have been backing up for a while.
> >
> > It's clearly time to talk about reorganizing who's responsible for
> > reviews and for pushing git tree changes, and to take pressure off
> > Zorro. Anand has been signalling interest in taking on some of the
> > btrfs workload, and I think that should happen in some form because
> > Zorro and I are clearly outmatched.
>
> Acutally I've tried to test most of major fs which fstests supports,
> before a release. Btrfs testing is supported by my test scripts now.
> Currently only default btrfs be tested. If Anand would like to test
> more for btrfs before each release, I think that helps btrfs more.
>
> >
> > > Signed-off-by: Zorro Lang <zlang@xxxxxxxxxx>
> > > ---
> > >
> > > Hi,
> > >
> > > This patch aims to be talked. Refer to above commit log. This patch
> > > is a workaround for 2 targets:
> > > 1) Avoid the regressions of lastest xfstests release.
> > > 2) Give us more time to improve the "privatens" method.
> > >
> > > And compare with revert that commit directly, this patch trys to
> > > give "privatens" method a chance to be enabled and tested (by
> > > export TRY_PRIVATENS=yes).
> >
> > Everyone else: do things work better if you manually patch out the
> > run_privatens selection code so that run_setsid is always used? The
> > setsid code itself is also new; before that fstests just used killall.
>
> If no one reports bugs to run_setsid, I'll think it's stable (temporarily),
> then we'll pay more attention to "privatens".
>
> >
> > > Thanks,
> > > Zorro
> > >
> > > check | 22 +++++++---------------
> > > 1 file changed, 7 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/check b/check
> > > index ea92b0d62..33eb3e085 100755
> > > --- a/check
> > > +++ b/check
> > > @@ -674,10 +674,13 @@ _stash_test_status() {
> > > esac
> > > }
> > >
> > > -# Can we run in a private pid/mount namespace?
> > > -HAVE_PRIVATENS=
> > > -./tools/run_privatens bash -c "exit 77"
> > > -test $? -eq 77 && HAVE_PRIVATENS=yes
> > > +# 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=
> > > @@ -692,15 +695,6 @@ function _adjust_oom_score() {
> > > }
> > > _adjust_oom_score -500
> > >
> > > -warn_deprecated_sessionid()
> > > -{
> > > - if [ -z "$WARNED_DEPRECATED_SESSIONID" ]; then
> > > - echo "WARNING: Running fstests without private pid/mount namespace"
> > > - echo "support is deprecated and will be removed in February 2026."
> > > - WARNED_DEPRECATED_SESSIONID=1
> > > - fi
> > > -}
> > > -
> > > # ...and make the tests themselves somewhat more attractive to it, so that if
> > > # 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
> > > @@ -900,8 +894,6 @@ function run_section()
> > > seqres="$check"
> > > _check_test_fs
> > >
> > > - test -n "$HAVE_PRIVATENS" || warn_deprecated_sessionid
> > > -
> >
> > Removal of the deprecation should be a separate patch, for which I will
> > pre-offer a
> > Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
>
> Ok, I'll splite this patch and send a v2. Thanks your reviewing!
>
> Thanks,
> Zorro
>
> >
> > --D
> >
> > > loop_status=() # track rerun-on-failure state
> > > local tc_status ix
> > > local -a _list=( $list )
> > > --
> > > 2.47.1
> > >
> >
>
