On Tue, Mar 04, 2025 at 12:59:42PM -0800, Darrick J. Wong wrote:
> 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"
Oh, I didn't describe clearly, I mean this one bug:
https://lore.kernel.org/fstests/20250303064209.602577-1-naohiro.aota@xxxxxxx/
some systems might not have a tmpfs on /tmp ^^ I'll leave more review points
under that thread.
>
> 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
Do you hope to have TEST_DIR and SCRATCH_MNT under a tmpfs?
> 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.
I think we can talk about more details in another thread.
Thanks,
Zorro
>
> --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
> > > >
> > >
> >
>
