[PATCH 0/3] fstests: more fixes...

[PATCH 0/3] fstests: more fixes...

[Dave Chinner]

Hi folks,

Another couple of fixes for test failures I see in my local test
environments.  The g/081 failure (DM vs DAX issue) has been around
for a long while, I've only just got around to writing a _notrun fix
for it.

Darrick also noticed an issue with the new group list creation code,
so there's a fix for that, too.

Cheers,

Dave.

[PATCH 1/4] fstests: fix group list generation for whacky test names

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Darrick noticed that tests/xfs/191-input-validation didn't get
generated properly. Fix the regex to handle this.

$ grep -I -R "^_begin_fstest" tests/xfs | \
  sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
$
$ grep -I -R "^_begin_fstest" tests/xfs | \
  sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
191 auto quick mkfs realtime
$

Long term, we should rename that test to '191' and rip out all that
unused and unnecessary complexity for matching ascii test names
because we just don't use it. Numbers for tests are still working
just fine.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tools/mkgroupfile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/mkgroupfile b/tools/mkgroupfile
index 24435898..958d4e2f 100755
--- a/tools/mkgroupfile
+++ b/tools/mkgroupfile
@@ -60,7 +60,7 @@ ENDL
 
 	# Aggregate the groups each test belongs to for the group file
 	grep -I -R "^_begin_fstest" $test_dir/ | \
-		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
+		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
 
 	# Create the list of unique groups for existence checking
 	grep -I -R "^_begin_fstest" $test_dir/ | \
-- 
2.35.1

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Darrick J. Wong]

On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Darrick noticed that tests/xfs/191-input-validation didn't get
> generated properly. Fix the regex to handle this.
> 
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> $
> $ grep -I -R "^_begin_fstest" tests/xfs | \
>   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> 191 auto quick mkfs realtime
> $
> 
> Long term, we should rename that test to '191' and rip out all that
> unused and unnecessary complexity for matching ascii test names
> because we just don't use it. Numbers for tests are still working
> just fine.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  tools/mkgroupfile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> index 24435898..958d4e2f 100755
> --- a/tools/mkgroupfile
> +++ b/tools/mkgroupfile
> @@ -60,7 +60,7 @@ ENDL
>  
>  	# Aggregate the groups each test belongs to for the group file
>  	grep -I -R "^_begin_fstest" $test_dir/ | \
> -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups

Sorry I didn't get a chance to review this patch before it went in, but
this string parsing gets tripped up by things that the old code handled
just fine.  Back when we'd run _begin_fstest as a real bash subroutine
to print the group name arguments, a line like this:

_begin_fstest deprecated # log logprint quota

would put this test in *only* the group "deprecated".  Everything
starting with the '#' is a comment.  bash would also ignore extra spaces
between arguments, and if someone happened to use a tab, that would also
be fine because bash ignores all the unquoted whitespace between
arguments.  Yes, it's slow, but I chose that method because (a) make
-jXX, and (b) I hate string parsing with grep and sed gunk.

Instead, that above output (which I harvested from xfs/081) now becomes:

081 deprecated # log logprint quota

The first grepsed blobule should do more if it's going to
performance-optimize bash:

grep -I -R "^_begin_fstest" -Z $test_dir/ | \
	sed -e 's/#.*$//g' \
	    -e 's/[[:space:]]$//g' \
	    -e 's/[[:space:]]+/ /g' | \
	    -e 's/^.*\/\(.*\)\x0.*_begin_fstest[[:space:]]*/\1 /g' \
	sort -g

The -Z option separates the filename from the found content, which
enables sed to isolate the filename portion.  The first sed statement
removes all comments, the second removes all trailing whitespace so that
it won't end up in the output, and the third collapses whitespace runs
into a single space.  The fourth reformats the input to match group file
format.  The command ends with a sort -g so that the lines end up in
numeric order instead of readdir() order.

Even then, this still isn't sufficient, since a null in the test file
will confuse this.  I half wonder if this will even work universally,
since -Z is probably a GNUism, and I bet there are sed out there that
won't recognize '\x0' to detect the NULL in the output.  But hey, -I and
-R aren't in the posix definition either...

>  	# Create the list of unique groups for existence checking
>  	grep -I -R "^_begin_fstest" $test_dir/ | \

This second blobule isn't so bad; it becomes:

	grep -I -R "^_begin_fstest" -h $test_dir/ | \
		sed -e 's/#.*$//g' \
		    -e 's/[[:space:]]$//g' \
		    -e 's/[[:space:]]+/ /g' \
		    -e 's/^.*_begin_fstest[[:space:]]*//g' \
		    -e 's/ /\n/g' | \
		sort -u > $new_groups.check

Where -h turns off the filename printing since we don't need that for
the unique group list.  But still, UGH STRING PARSING.

--D

> -- 
> 2.35.1
> 

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Dave Chinner]

On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Darrick noticed that tests/xfs/191-input-validation didn't get
> > generated properly. Fix the regex to handle this.
> > 
> > $ grep -I -R "^_begin_fstest" tests/xfs | \
> >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > $
> > $ grep -I -R "^_begin_fstest" tests/xfs | \
> >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > 191 auto quick mkfs realtime
> > $
> > 
> > Long term, we should rename that test to '191' and rip out all that
> > unused and unnecessary complexity for matching ascii test names
> > because we just don't use it. Numbers for tests are still working
> > just fine.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  tools/mkgroupfile | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > index 24435898..958d4e2f 100755
> > --- a/tools/mkgroupfile
> > +++ b/tools/mkgroupfile
> > @@ -60,7 +60,7 @@ ENDL
> >  
> >  	# Aggregate the groups each test belongs to for the group file
> >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> 
> Sorry I didn't get a chance to review this patch before it went in, but
> this string parsing gets tripped up by things that the old code handled
> just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> to print the group name arguments, a line like this:
> 
> _begin_fstest deprecated # log logprint quota

And I just don't care about that.

Handling these whacky corner cases is the *wrong solution* - we're
just creating more technical debt for ourselves going down that
path.

$ git grep _begin_fstest tests/ |grep "#"
tests/xfs/018:_begin_fstest deprecated # log logprint v2log
tests/xfs/081:_begin_fstest deprecated # log logprint quota
tests/xfs/082:_begin_fstest deprecated # log logprint v2log
$

There are the only 3 tests marked deprecated. *Nobody runs them* and
if they did, they all fail on a current kernel:

....
Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
....
Failures: xfs/018 xfs/081 xfs/082
Failed 3 of 3 tests

We don't need to support this whacky corner case - just remove
the three deprecated tests completely. If anyone needs them, they
are still in the git history. But the right thing to do is to remove
the corner case altogether, not add complexity to handle it
needlessly.

> Instead, that above output (which I harvested from xfs/081) now becomes:
> 
> 081 deprecated # log logprint quota
> 
> The first grepsed blobule should do more if it's going to
> performance-optimize bash:

We don't need to "performance-optimize bash" - we've already fixed
the problem by addressing the low hanging fruit (35s down to less
than 0.4s, and less than 0.1s with make -j8).

Beyond where we iare now is really "don't care" territory because
nobody is going to notice it going 0.1s faster now. OTOH, we are sure
as anything going to notice the complexity of the regexes in a
year's time when we have to work out what it does from first
principles again....

> grep -I -R "^_begin_fstest" -Z $test_dir/ | \
> 	sed -e 's/#.*$//g' \

This is the only bit that is needed to handle the commented out
group case, everythign else is just complexity that comes from
over-optimisation. And even handling comments is largely unnecessary
because removing deprecated tests is the right way to fix this...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Darrick J. Wong]

On Tue, May 17, 2022 at 07:35:17AM +1000, Dave Chinner wrote:
> On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> > On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > generated properly. Fix the regex to handle this.
> > > 
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > $
> > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > 191 auto quick mkfs realtime
> > > $
> > > 
> > > Long term, we should rename that test to '191' and rip out all that
> > > unused and unnecessary complexity for matching ascii test names
> > > because we just don't use it. Numbers for tests are still working
> > > just fine.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  tools/mkgroupfile | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > index 24435898..958d4e2f 100755
> > > --- a/tools/mkgroupfile
> > > +++ b/tools/mkgroupfile
> > > @@ -60,7 +60,7 @@ ENDL
> > >  
> > >  	# Aggregate the groups each test belongs to for the group file
> > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> > 
> > Sorry I didn't get a chance to review this patch before it went in, but
> > this string parsing gets tripped up by things that the old code handled
> > just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> > to print the group name arguments, a line like this:
> > 
> > _begin_fstest deprecated # log logprint quota
> 
> And I just don't care about that.

I *do*, because my entire testing dashboard turned red overnight because
these changes broke the group list file generation and pulled in tests
that normally get excluded from my nightly test runs.

Did _anyone_ actually compare the group.list output before and after
applying the patch?

$ diff -U 1 -r build-old/tests/ build-x86_64/tests/
diff -U 1 -r build-old/tests/xfs/group.list
build-x86_64/tests/xfs/group.list
--- build-old/tests/xfs/group.list      2022-05-16 15:05:55.655374923 -0700
+++ build-x86_64/tests/xfs/group.list   2022-05-16 15:07:12.562086066 -0700
@@ -3,2 +3,3 @@
 
+/home/djwong/cdev/work/fstests/build-x86_64/tests/xfs/191-input-validation:_begin_fstest
auto quick mkfs realtime broken
 001 db auto quick
@@ -20,3 +21,3 @@
 017 mount auto quick stress
-018 deprecated
+018 deprecated # log logprint v2log
 019 mkfs auto quick
@@ -83,4 +84,4 @@
 080 rw ioctl
-081 deprecated
-082 deprecated
+081 deprecated # log logprint quota
+082 deprecated # log logprint v2log
 083 dangerous_fuzzers punch
@@ -191,3 +192,2 @@
 190 rw auto quick
-191-input-validation auto quick mkfs realtime broken
 192 auto quick clone


> Handling these whacky corner cases is the *wrong solution* - we're
> just creating more technical debt for ourselves going down that
> path.

Frankly, I think this whole change *introduces* technical debt.

Test files used to be bash scripts that have /all/ the standard
behaviors of bash scripts -- commands can have comments at the end of
the line, continuations are supported, etc.  Test authors merely have to
ensure that the script parses correctly, and that the groups for each
test are specified as options to _begin_fstest.

Now you've effectively made it so that there's this one weird line that
looks like any other piece of shell script, but it isn't.  You can't
have comments, you can't use line continuations, and if you
accidentally drop something like a colon in the line, there's a risk
that a regular expression will do the wrong thing and explode.

In other words, test scripts aren't purely bash scripts anymore even
though they sure look like pure bash.  Every author must now be aware
that there's a behavioral quirk pertaining to exactly one required line
in every test script.

> $ git grep _begin_fstest tests/ |grep "#"
> tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> tests/xfs/081:_begin_fstest deprecated # log logprint quota
> tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> $
> 
> There are the only 3 tests marked deprecated. *Nobody runs them* and
> if they did, they all fail on a current kernel:
> 
> ....
> Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
> ....
> Failures: xfs/018 xfs/081 xfs/082
> Failed 3 of 3 tests
> 
> We don't need to support this whacky corner case - just remove
> the three deprecated tests completely. If anyone needs them, they
> are still in the git history. But the right thing to do is to remove
> the corner case altogether, not add complexity to handle it
> needlessly.

Then why didn't you propose remove them?  I agree that these three are
hopelessly broken and I've never gotten x/191 to pass, and fully support
removing them.

> > Instead, that above output (which I harvested from xfs/081) now
> > becomes:
> > 
> > 081 deprecated # log logprint quota
> > 
> > The first grepsed blobule should do more if it's going to
> > performance-optimize bash:
> 
> We don't need to "performance-optimize bash" - we've already fixed
> the problem by addressing the low hanging fruit (35s down to less
> than 0.4s, and less than 0.1s with make -j8).

That's nice that it's faster to build, but IMHO it's *broken*.  The
original patch was a performance improvement that submarined in a change
in how we have to write tests.

The comments for _begin_fstest don't make any mention at all of the new
requirements for the _begin_fstest callsite.

> Beyond where we iare now is really "don't care" territory because
> nobody is going to notice it going 0.1s faster now. OTOH, we are sure
> as anything going to notice the complexity of the regexes in a
> year's time when we have to work out what it does from first
> principles again....

I already had to do that, and it's barely been 10 days since this was
committed.

> > grep -I -R "^_begin_fstest" -Z $test_dir/ | \
> > 	sed -e 's/#.*$//g' \
> 
> This is the only bit that is needed to handle the commented out
> group case, everythign else is just complexity that comes from
> over-optimisation. And even handling comments is largely unnecessary
> because removing deprecated tests is the right way to fix this...

...until the next person trips over this.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Dave Chinner]

On Mon, May 16, 2022 at 03:26:04PM -0700, Darrick J. Wong wrote:
> On Tue, May 17, 2022 at 07:35:17AM +1000, Dave Chinner wrote:
> > On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> > > On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > > generated properly. Fix the regex to handle this.
> > > > 
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > > $
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > > 191 auto quick mkfs realtime
> > > > $
> > > > 
> > > > Long term, we should rename that test to '191' and rip out all that
> > > > unused and unnecessary complexity for matching ascii test names
> > > > because we just don't use it. Numbers for tests are still working
> > > > just fine.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  tools/mkgroupfile | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > index 24435898..958d4e2f 100755
> > > > --- a/tools/mkgroupfile
> > > > +++ b/tools/mkgroupfile
> > > > @@ -60,7 +60,7 @@ ENDL
> > > >  
> > > >  	# Aggregate the groups each test belongs to for the group file
> > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> > > 
> > > Sorry I didn't get a chance to review this patch before it went in, but
> > > this string parsing gets tripped up by things that the old code handled
> > > just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> > > to print the group name arguments, a line like this:
> > > 
> > > _begin_fstest deprecated # log logprint quota
> > 
> > And I just don't care about that.
> 
> I *do*, because my entire testing dashboard turned red overnight because
> these changes broke the group list file generation and pulled in tests
> that normally get excluded from my nightly test runs.

How did that happen? I missed xfs/191-..., but that won't cause it
to run.  The others should only been seen by check as belonging to
the deprecated group, so I'm not sure how they got run, either.

check is supposed to be stripping the commented out regions of the
group file (e.g. the header) with this code:

get_sub_group_list()

        local d=$1
        local grp=$2

        test -s "$SRC_DIR/$d/group.list" || return 1

        local grpl=$(sed -n < $SRC_DIR/$d/group.list \
>>>>>>>         -e 's/#.*//' \
                -e 's/$/ /' \
                -e "s;^\($VALID_TEST_NAME\).* $grp .*;$SRC_DIR/$d/\1;p")
        echo $grpl
}

If the comments not being stripped correctly, then the three
deprecated tests:

> > $ git grep _begin_fstest tests/ |grep "#"
> > tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> > tests/xfs/081:_begin_fstest deprecated # log logprint quota
> > tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> > $

should all be listed as members of the log group. So let's list the
tests that the log group will run:

# MOUNT_OPTIONS= ./check -b -s xfs -n -g log
SECTION       -- xfs
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 test2 5.18.0-rc7-dgc+ #1245 SMP PREEMPT_DYNAMIC Mon May 16 11:51:16 AEST 2022
MKFS_OPTIONS  -- -f -m rmapbt=1 /dev/vdb
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch

generic/034
generic/039
generic/040
generic/041
generic/043
generic/044
generic/045
generic/046
generic/051
generic/052
generic/054
generic/055
generic/056
generic/057
generic/059
generic/065
generic/066
generic/073
generic/090
generic/101
generic/104
generic/106
generic/107
generic/177
generic/311
generic/321
generic/322
generic/325
generic/335
generic/336
generic/341
generic/342
generic/343
generic/376
generic/388
generic/417
generic/455
generic/457
generic/475
generic/479
generic/480
generic/481
generic/483
generic/489
generic/498
generic/501
generic/502
generic/509
generic/510
generic/512
generic/520
generic/526
generic/527
generic/534
generic/535
generic/546
generic/547
generic/552
generic/557
generic/588
generic/640
generic/648
generic/677
generic/690
shared/002
xfs/011
xfs/029
xfs/051
xfs/057
xfs/079
xfs/095
xfs/119
xfs/121
xfs/141
xfs/181
xfs/216
xfs/217
xfs/439

$

They aren't there.

And to check that they are actually getting parsed correctly:

# MOUNT_OPTIONS= ./check -b -s xfs -n -g deprecated
SECTION       -- xfs
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 test2 5.18.0-rc7-dgc+ #1245 SMP PREEMPT_DYNAMIC Mon May 16 11:51:16 AEST 2022
MKFS_OPTIONS  -- -f -m rmapbt=1 /dev/vdb
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch

xfs/018
xfs/081
xfs/082

$

Yup, there they are in the deprecated group.

AFAICT, nothing except for the handling of xfs/191-... is broken.
Comments in group lists are being stripped just fine, and so I have
no idea what is going wrong in your test environment to cause these
tests to run and/or be marked as failing.

> Did _anyone_ actually compare the group.list output before and after
> applying the patch?

Yup. I did a visual inspection, ran group listings like above, ran
it for a couple of weeks on all my machines before sending it out
for review.  No extra tests ran, no tests that should have run
didn't run, no unexpected errors occur.

Yes, I missed that 191-...  was missing in my inspection, but we 
all make mistakes from time to time. I noticed when pointing out
the comment stripping in get_sub_group_list() that we have
$VALID_TEST_NAME for regex matching test names. I should change the
regex to use that - if I had remembered this existed, I wouldn't
have written my own and got it wrong....

> > Handling these whacky corner cases is the *wrong solution* - we're
> > just creating more technical debt for ourselves going down that
> > path.
> 
> Frankly, I think this whole change *introduces* technical debt.
> 
> Test files used to be bash scripts that have /all/ the standard
> behaviors of bash scripts -- commands can have comments at the end of
> the line, continuations are supported, etc.  Test authors merely have to
> ensure that the script parses correctly, and that the groups for each
> test are specified as options to _begin_fstest.

Only 3 tests out of 1700 make any use of these features, and check
handles those 3 tests just fine. The list generation change has made
no practical difference to anything inside fstests, and the group
files do not form a stable ABI that will never change.

I use comments in group lists occasionally, I made sure that works.
We don't have anything that uses any of that other functionality in
then ~1700 tests we already have, so there's no current need for
the special case requirements being described.

"Make the requirements less dumb."
	- Elon Musk's 1st rule for engineering success.

> Now you've effectively made it so that there's this one weird line that
> looks like any other piece of shell script, but it isn't.  You can't
> have comments, you can't use line continuations, and if you
> accidentally drop something like a colon in the line, there's a risk
> that a regular expression will do the wrong thing and explode.

<shrug>

All we need is a list of the {test number, {groups}} sets. We do not
need to execute 1700 tests and several grep/sed invocations per test
to build these lists - all we need to do is parse the single line in
the existing test files that defines the groups.

It is trivial to document that single line requirement for future
reference, and given that every test already conforms to that
requirement it has zero impact on anyone or any existing test.

We don't need to over-engineer this again.

> In other words, test scripts aren't purely bash scripts anymore even
> though they sure look like pure bash.  Every author must now be aware
> that there's a behavioral quirk pertaining to exactly one required line
> in every test script.

<shrug>

We have had restrictions on the format of the test file that aren't
"bash" for a long, long time. lsqa.pl expects the test comment
headers to be in a specific format so it can parse tests as
text files to extract the test titles and descriptions.

Nothing is perfect. The old code wasn't perfect. The new code isn't
perfect, either, but it's simpler and faster and I'm fixing
problems as they are reported.

I didn't intend to break anything. I make mistakes and miss things
and I expect that everyone else does, too. I try to fix mistakes as
fast as I can and I expect everyone else to do the same.

> > $ git grep _begin_fstest tests/ |grep "#"
> > tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> > tests/xfs/081:_begin_fstest deprecated # log logprint quota
> > tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> > $
> > 
> > There are the only 3 tests marked deprecated. *Nobody runs them* and
> > if they did, they all fail on a current kernel:
> > 
> > ....
> > Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
> > ....
> > Failures: xfs/018 xfs/081 xfs/082
> > Failed 3 of 3 tests
> > 
> > We don't need to support this whacky corner case - just remove
> > the three deprecated tests completely. If anyone needs them, they
> > are still in the git history. But the right thing to do is to remove
> > the corner case altogether, not add complexity to handle it
> > needlessly.
> 
> Then why didn't you propose remove them?

I just did!

I can't propose solutions before I know about them being a problem;
of the many things I can do, "violating causality" is not on that
list.

> > > Instead, that above output (which I harvested from xfs/081) now
> > > becomes:
> > > 
> > > 081 deprecated # log logprint quota
> > > 
> > > The first grepsed blobule should do more if it's going to
> > > performance-optimize bash:
> > 
> > We don't need to "performance-optimize bash" - we've already fixed
> > the problem by addressing the low hanging fruit (35s down to less
> > than 0.4s, and less than 0.1s with make -j8).
> 
> That's nice that it's faster to build, but IMHO it's *broken*.  The
> original patch was a performance improvement that submarined in a change
> in how we have to write tests.

<deep breathe, don't shout, be calm>

Please choose your words more carefully in future, Darrick.
Submarining implies that people are acting in bad faith.

I doubt this is what you meant, but that was kind of the last straw
after implying the change wasn't tested properly, wasn't reviewed
properly, was just straight out broken and that I should have broken
the laws of physics to propose fixes before they were reported as
problems.

It has taken me half a day to calm down enough to write a reasonable
reply to address these issues.

I Am Not A Happy Camper.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Eric Sandeen]

On 5/16/22 11:36 PM, Dave Chinner wrote:
> On Mon, May 16, 2022 at 03:26:04PM -0700, Darrick J. Wong wrote:
...
>> Did _anyone_ actually compare the group.list output before and after
>> applying the patch?

...

> I Am Not A Happy Camper.

...

And this whole thing has made me Very Sad.

Everybody's trying to do the right thing here. Darrick's original patch
tried to ease maintainer burden with group file conflicts. Dave's patch
tried to speed it up. Zorro reviewed it, cycled it through for-next and
landed it in main in a timely manner. A problem or two remained, and now
everybody's... angry. Of course.

Can we try to just show a little more grace here? Assume good intent?
Interpret things as charitably as possible? Voice concerns calmly, and
hear them nondefensively? Accept that mistakes will happen, fix them, and
move forward? It would go a long ways to making this all a lot more fun.

I'm coming at this without most of the technical context or background
for these changes. All of the issues raised seem reasonable to me:
slowing down "make" has some drawbacks. Special requirements on lines
of bash has drawbacks too. None of this is insurmountable. Document
the specialness in the template. Make the parsing more robust, or make
it fail in a more obvious way. Spend time on yet another approach to
generate group files efficiently, if that's worth it ... I dunno.

But we've got to be able to get through relatively minor issues like
this without tempers flaring, it's just not worth it.

It's not my intent to take sides or point fingers in this particular
exchange, it's just that this dynamic has played out too many times
recently, and I really wish we could collectively do better. It's not
good for us as individuals or as a community.

-Eric

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Darrick J. Wong]

On Tue, May 17, 2022 at 02:36:03PM +1000, Dave Chinner wrote:
> On Mon, May 16, 2022 at 03:26:04PM -0700, Darrick J. Wong wrote:
> > On Tue, May 17, 2022 at 07:35:17AM +1000, Dave Chinner wrote:
> > > On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> > > > On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > 
> > > > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > > > generated properly. Fix the regex to handle this.
> > > > > 
> > > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > > > $
> > > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > > > 191 auto quick mkfs realtime
> > > > > $
> > > > > 
> > > > > Long term, we should rename that test to '191' and rip out all that
> > > > > unused and unnecessary complexity for matching ascii test names
> > > > > because we just don't use it. Numbers for tests are still working
> > > > > just fine.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  tools/mkgroupfile | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > > index 24435898..958d4e2f 100755
> > > > > --- a/tools/mkgroupfile
> > > > > +++ b/tools/mkgroupfile
> > > > > @@ -60,7 +60,7 @@ ENDL
> > > > >  
> > > > >  	# Aggregate the groups each test belongs to for the group file
> > > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > > > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> > > > 
> > > > Sorry I didn't get a chance to review this patch before it went in, but
> > > > this string parsing gets tripped up by things that the old code handled
> > > > just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> > > > to print the group name arguments, a line like this:
> > > > 
> > > > _begin_fstest deprecated # log logprint quota
> > > 
> > > And I just don't care about that.
> > 
> > I *do*, because my entire testing dashboard turned red overnight because
> > these changes broke the group list file generation and pulled in tests
> > that normally get excluded from my nightly test runs.
> 
> How did that happen? I missed xfs/191-..., but that won't cause it
> to run.  The others should only been seen by check as belonging to
> the deprecated group, so I'm not sure how they got run, either.

xfs/191* got added to the test list on the "-g all -x broken" VM because
tests/xfs/group.list contained:

/home/djwong/cdev/work/fstests/build-x86_64/tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime broken

Which meant that we didn't filter out 191 because that wasn't the
recognized group list format.

I still have no idea how xfs/081 ended up banging around in the
recoveryloop VMs.  The dense syntax of the bash parsing makes my brain
hurt; if I figure it out I'll post again.

> check is supposed to be stripping the commented out regions of the
> group file (e.g. the header) with this code:
> 
> get_sub_group_list()
> 
>         local d=$1
>         local grp=$2
> 
>         test -s "$SRC_DIR/$d/group.list" || return 1
> 
>         local grpl=$(sed -n < $SRC_DIR/$d/group.list \
> >>>>>>>         -e 's/#.*//' \
>                 -e 's/$/ /' \
>                 -e "s;^\($VALID_TEST_NAME\).* $grp .*;$SRC_DIR/$d/\1;p")
>         echo $grpl
> }
> 
> If the comments not being stripped correctly, then the three
> deprecated tests:
> 
> > > $ git grep _begin_fstest tests/ |grep "#"
> > > tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> > > tests/xfs/081:_begin_fstest deprecated # log logprint quota
> > > tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> > > $
> 
> should all be listed as members of the log group. So let's list the
> tests that the log group will run:
> 
> # MOUNT_OPTIONS= ./check -b -s xfs -n -g log
> SECTION       -- xfs
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 test2 5.18.0-rc7-dgc+ #1245 SMP PREEMPT_DYNAMIC Mon May 16 11:51:16 AEST 2022
> MKFS_OPTIONS  -- -f -m rmapbt=1 /dev/vdb
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch
> 
> generic/034
> generic/039
> generic/040
> generic/041
> generic/043
> generic/044
> generic/045
> generic/046
> generic/051
> generic/052
> generic/054
> generic/055
> generic/056
> generic/057
> generic/059
> generic/065
> generic/066
> generic/073
> generic/090
> generic/101
> generic/104
> generic/106
> generic/107
> generic/177
> generic/311
> generic/321
> generic/322
> generic/325
> generic/335
> generic/336
> generic/341
> generic/342
> generic/343
> generic/376
> generic/388
> generic/417
> generic/455
> generic/457
> generic/475
> generic/479
> generic/480
> generic/481
> generic/483
> generic/489
> generic/498
> generic/501
> generic/502
> generic/509
> generic/510
> generic/512
> generic/520
> generic/526
> generic/527
> generic/534
> generic/535
> generic/546
> generic/547
> generic/552
> generic/557
> generic/588
> generic/640
> generic/648
> generic/677
> generic/690
> shared/002
> xfs/011
> xfs/029
> xfs/051
> xfs/057
> xfs/079
> xfs/095
> xfs/119
> xfs/121
> xfs/141
> xfs/181
> xfs/216
> xfs/217
> xfs/439
> 
> $
> 
> They aren't there.
> 
> And to check that they are actually getting parsed correctly:
> 
> # MOUNT_OPTIONS= ./check -b -s xfs -n -g deprecated
> SECTION       -- xfs
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 test2 5.18.0-rc7-dgc+ #1245 SMP PREEMPT_DYNAMIC Mon May 16 11:51:16 AEST 2022
> MKFS_OPTIONS  -- -f -m rmapbt=1 /dev/vdb
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch
> 
> xfs/018
> xfs/081
> xfs/082
> 
> $
> 
> Yup, there they are in the deprecated group.
> 
> AFAICT, nothing except for the handling of xfs/191-... is broken.
> Comments in group lists are being stripped just fine, and so I have
> no idea what is going wrong in your test environment to cause these
> tests to run and/or be marked as failing.
> 
> > Did _anyone_ actually compare the group.list output before and after
> > applying the patch?
> 
> Yup. I did a visual inspection, ran group listings like above, ran
> it for a couple of weeks on all my machines before sending it out
> for review.  No extra tests ran, no tests that should have run
> didn't run, no unexpected errors occur.
> 
> Yes, I missed that 191-...  was missing in my inspection, but we 
> all make mistakes from time to time. I noticed when pointing out
> the comment stripping in get_sub_group_list() that we have
> $VALID_TEST_NAME for regex matching test names. I should change the
> regex to use that - if I had remembered this existed, I wouldn't
> have written my own and got it wrong....

> > > Handling these whacky corner cases is the *wrong solution* - we're
> > > just creating more technical debt for ourselves going down that
> > > path.
> > 
> > Frankly, I think this whole change *introduces* technical debt.
> > 
> > Test files used to be bash scripts that have /all/ the standard
> > behaviors of bash scripts -- commands can have comments at the end of
> > the line, continuations are supported, etc.  Test authors merely have to
> > ensure that the script parses correctly, and that the groups for each
> > test are specified as options to _begin_fstest.
> 
> Only 3 tests out of 1700 make any use of these features, and check
> handles those 3 tests just fine. The list generation change has made
> no practical difference to anything inside fstests, and the group
> files do not form a stable ABI that will never change.
> 
> I use comments in group lists occasionally, I made sure that works.
> We don't have anything that uses any of that other functionality in
> then ~1700 tests we already have, so there's no current need for
> the special case requirements being described.

Sometimes I use comments to turn off group memberships of tests
temporarily.  Anyway, I saw you added that to the documentation, so
thank you for that.

> "Make the requirements less dumb."
> 	- Elon Musk's 1st rule for engineering success.
> 
> > Now you've effectively made it so that there's this one weird line that
> > looks like any other piece of shell script, but it isn't.  You can't
> > have comments, you can't use line continuations, and if you
> > accidentally drop something like a colon in the line, there's a risk
> > that a regular expression will do the wrong thing and explode.
> 
> <shrug>
> 
> All we need is a list of the {test number, {groups}} sets. We do not
> need to execute 1700 tests and several grep/sed invocations per test
> to build these lists - all we need to do is parse the single line in
> the existing test files that defines the groups.
> 
> It is trivial to document that single line requirement for future
> reference, and given that every test already conforms to that
> requirement it has zero impact on anyone or any existing test.
> 
> We don't need to over-engineer this again.
> 
> > In other words, test scripts aren't purely bash scripts anymore even
> > though they sure look like pure bash.  Every author must now be aware
> > that there's a behavioral quirk pertaining to exactly one required line
> > in every test script.
> 
> <shrug>
> 
> We have had restrictions on the format of the test file that aren't
> "bash" for a long, long time. lsqa.pl expects the test comment
> headers to be in a specific format so it can parse tests as
> text files to extract the test titles and descriptions.

Say what?  I had /no idea/ there were more formatting requirements.
Where are those documented?

This is where I get cranky -- we've now had several clashes on the list
involving bits and pieces of underdocumented XFS (like quota warnings
and ALLOCSP) where nobody ever wrote down things like what does this
code do, what inputs does it expect to result in a particular output,
etc.  Then nobody knows if it's working correctly or how to interpret
any of the weird side behaviors that could be benign or they could be
broken.

I've gotten very burnt out on trying to navigate towards a resolution to
these things, and all I saw was "Oh good, the rules changed from my
understanding to something else, the something else isn't written down,
and here we go again!" Combine that with "And I just don't care about
that" and my frustration goes high.

> Nothing is perfect. The old code wasn't perfect. The new code isn't
> perfect, either, but it's simpler and faster and I'm fixing
> problems as they are reported.
> 
> I didn't intend to break anything. I make mistakes and miss things
> and I expect that everyone else does, too. I try to fix mistakes as
> fast as I can and I expect everyone else to do the same.

Yeah.  Same here.  I make mistakes and send out the occasional rotten
patch, and I usually try to fix them, so long as that doesn't involve a
ton of scope creep.

The part that surprises me about all this is that you frequently
encourage me to add and improve the documentation when I change things,
especially when they involve changes to yours or my mental models of
how a certain thing works.  I appreciate that, and I usually make
changes to try to make it easier for everyone else to understand things.
In turn, I try to encourage that when I'm reviewing things too.

In that context, I don't get why the original patch wasn't accompanied
by a change to the documentation to spell out conversion of
_begin_fstest from a mere shell function into a Proper Keyword.
You've done that now, so thank you.

> > > $ git grep _begin_fstest tests/ |grep "#"
> > > tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> > > tests/xfs/081:_begin_fstest deprecated # log logprint quota
> > > tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> > > $
> > > 
> > > There are the only 3 tests marked deprecated. *Nobody runs them* and
> > > if they did, they all fail on a current kernel:
> > > 
> > > ....
> > > Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
> > > ....
> > > Failures: xfs/018 xfs/081 xfs/082
> > > Failed 3 of 3 tests
> > > 
> > > We don't need to support this whacky corner case - just remove
> > > the three deprecated tests completely. If anyone needs them, they
> > > are still in the git history. But the right thing to do is to remove
> > > the corner case altogether, not add complexity to handle it
> > > needlessly.
> > 
> > Then why didn't you propose remove them?
> 
> I just did!
> 
> I can't propose solutions before I know about them being a problem;
> of the many things I can do, "violating causality" is not on that
> list.
> 
> > > > Instead, that above output (which I harvested from xfs/081) now
> > > > becomes:
> > > > 
> > > > 081 deprecated # log logprint quota
> > > > 
> > > > The first grepsed blobule should do more if it's going to
> > > > performance-optimize bash:
> > > 
> > > We don't need to "performance-optimize bash" - we've already fixed
> > > the problem by addressing the low hanging fruit (35s down to less
> > > than 0.4s, and less than 0.1s with make -j8).
> > 
> > That's nice that it's faster to build, but IMHO it's *broken*.  The
> > original patch was a performance improvement that submarined in a change
> > in how we have to write tests.
> 
> <deep breathe, don't shout, be calm>
> 
> Please choose your words more carefully in future, Darrick.
> Submarining implies that people are acting in bad faith.

...

Hearing that a patch author doesn't care about something I complained
about makes it *very* difficult for me NOT to feel like something is
going on in bad faith.

> I doubt this is what you meant, but that was kind of the last straw
> after implying the change wasn't tested properly, wasn't reviewed
> properly, was just straight out broken and that I should have broken
> the laws of physics to propose fixes before they were reported as
> problems.

That's not fair.  I don't expect you to bend physics.  I /had/ expected
that anyone changing the build system might compare the old and new
outputs of the build system and try to spot anything that didn't fit
their assumptions.

Eric encouraged me to try to have a little more grace, so I will end
there.  Perhaps in another three months I'll have calmed down more.

--D

Re: [PATCH 1/4] fstests: fix group list generation for whacky test names

[Zorro Lang]

On Mon, May 16, 2022 at 03:26:04PM -0700, Darrick J. Wong wrote:
> On Tue, May 17, 2022 at 07:35:17AM +1000, Dave Chinner wrote:
> > On Mon, May 16, 2022 at 09:20:26AM -0700, Darrick J. Wong wrote:
> > > On Mon, May 16, 2022 at 06:59:19PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Darrick noticed that tests/xfs/191-input-validation didn't get
> > > > generated properly. Fix the regex to handle this.
> > > > 
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' |grep 191
> > > > tests/xfs/191-input-validation:_begin_fstest auto quick mkfs realtime
> > > > $
> > > > $ grep -I -R "^_begin_fstest" tests/xfs | \
> > > >   sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/ ' |grep 191
> > > > 191 auto quick mkfs realtime
> > > > $
> > > > 
> > > > Long term, we should rename that test to '191' and rip out all that
> > > > unused and unnecessary complexity for matching ascii test names
> > > > because we just don't use it. Numbers for tests are still working
> > > > just fine.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  tools/mkgroupfile | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > index 24435898..958d4e2f 100755
> > > > --- a/tools/mkgroupfile
> > > > +++ b/tools/mkgroupfile
> > > > @@ -60,7 +60,7 @@ ENDL
> > > >  
> > > >  	# Aggregate the groups each test belongs to for the group file
> > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > -		sed -e 's/^.*\/\([0-9]*\):_begin_fstest/\1/' >> $new_groups
> > > > +		sed -e 's/^.*\/\([0-9]*\).*:_begin_fstest/\1/' >> $new_groups
> > > 
> > > Sorry I didn't get a chance to review this patch before it went in, but

Sorry, I'll try to leave more time to you next time, if a patch touches the code
related with you.

> > > this string parsing gets tripped up by things that the old code handled
> > > just fine.  Back when we'd run _begin_fstest as a real bash subroutine
> > > to print the group name arguments, a line like this:
> > > 
> > > _begin_fstest deprecated # log logprint quota
> > 
> > And I just don't care about that.
> 
> I *do*, because my entire testing dashboard turned red overnight because
> these changes broke the group list file generation and pulled in tests
> that normally get excluded from my nightly test runs.

Sorry Darrick, my general regression test[1] didn't cover your usage, cause
I didn't hit this "broken" error... Actually after talking with you last week,
I'm planning to change the ways I use fstests, especially for those groups
not belong to "auto". That might help to find issues like this in time.

Furthermore, I don't know why no one found this regression either in the past
one week after for-next branch been updated. I'm thinking about extend the
interval between for-next with master update, change from 1 week to 2 weeks.
As 1 week "soak period" looks like a little short.

Feel free to ping me, if you have more suggestions.

Thanks,
Zorro

[1]
./check -g auto
./check -g all -x auto

> 
> Did _anyone_ actually compare the group.list output before and after
> applying the patch?
> 
> $ diff -U 1 -r build-old/tests/ build-x86_64/tests/
> diff -U 1 -r build-old/tests/xfs/group.list
> build-x86_64/tests/xfs/group.list
> --- build-old/tests/xfs/group.list      2022-05-16 15:05:55.655374923 -0700
> +++ build-x86_64/tests/xfs/group.list   2022-05-16 15:07:12.562086066 -0700
> @@ -3,2 +3,3 @@
>  
> +/home/djwong/cdev/work/fstests/build-x86_64/tests/xfs/191-input-validation:_begin_fstest
> auto quick mkfs realtime broken
>  001 db auto quick
> @@ -20,3 +21,3 @@
>  017 mount auto quick stress
> -018 deprecated
> +018 deprecated # log logprint v2log
>  019 mkfs auto quick
> @@ -83,4 +84,4 @@
>  080 rw ioctl
> -081 deprecated
> -082 deprecated
> +081 deprecated # log logprint quota
> +082 deprecated # log logprint v2log
>  083 dangerous_fuzzers punch
> @@ -191,3 +192,2 @@
>  190 rw auto quick
> -191-input-validation auto quick mkfs realtime broken
>  192 auto quick clone
> 
> 
> > Handling these whacky corner cases is the *wrong solution* - we're
> > just creating more technical debt for ourselves going down that
> > path.
> 
> Frankly, I think this whole change *introduces* technical debt.
> 
> Test files used to be bash scripts that have /all/ the standard
> behaviors of bash scripts -- commands can have comments at the end of
> the line, continuations are supported, etc.  Test authors merely have to
> ensure that the script parses correctly, and that the groups for each
> test are specified as options to _begin_fstest.
> 
> Now you've effectively made it so that there's this one weird line that
> looks like any other piece of shell script, but it isn't.  You can't
> have comments, you can't use line continuations, and if you
> accidentally drop something like a colon in the line, there's a risk
> that a regular expression will do the wrong thing and explode.
> 
> In other words, test scripts aren't purely bash scripts anymore even
> though they sure look like pure bash.  Every author must now be aware
> that there's a behavioral quirk pertaining to exactly one required line
> in every test script.
> 
> > $ git grep _begin_fstest tests/ |grep "#"
> > tests/xfs/018:_begin_fstest deprecated # log logprint v2log
> > tests/xfs/081:_begin_fstest deprecated # log logprint quota
> > tests/xfs/082:_begin_fstest deprecated # log logprint v2log
> > $
> > 
> > There are the only 3 tests marked deprecated. *Nobody runs them* and
> > if they did, they all fail on a current kernel:
> > 
> > ....
> > Running: MOUNT_OPTIONS= ./check -R xunit -b -s xfs xfs/018 xfs/081 xfs/082
> > ....
> > Failures: xfs/018 xfs/081 xfs/082
> > Failed 3 of 3 tests
> > 
> > We don't need to support this whacky corner case - just remove
> > the three deprecated tests completely. If anyone needs them, they
> > are still in the git history. But the right thing to do is to remove
> > the corner case altogether, not add complexity to handle it
> > needlessly.
> 
> Then why didn't you propose remove them?  I agree that these three are
> hopelessly broken and I've never gotten x/191 to pass, and fully support
> removing them.
> 
> > > Instead, that above output (which I harvested from xfs/081) now
> > > becomes:
> > > 
> > > 081 deprecated # log logprint quota
> > > 
> > > The first grepsed blobule should do more if it's going to
> > > performance-optimize bash:
> > 
> > We don't need to "performance-optimize bash" - we've already fixed
> > the problem by addressing the low hanging fruit (35s down to less
> > than 0.4s, and less than 0.1s with make -j8).
> 
> That's nice that it's faster to build, but IMHO it's *broken*.  The
> original patch was a performance improvement that submarined in a change
> in how we have to write tests.
> 
> The comments for _begin_fstest don't make any mention at all of the new
> requirements for the _begin_fstest callsite.
> 
> > Beyond where we iare now is really "don't care" territory because
> > nobody is going to notice it going 0.1s faster now. OTOH, we are sure
> > as anything going to notice the complexity of the regexes in a
> > year's time when we have to work out what it does from first
> > principles again....
> 
> I already had to do that, and it's barely been 10 days since this was
> committed.
> 
> > > grep -I -R "^_begin_fstest" -Z $test_dir/ | \
> > > 	sed -e 's/#.*$//g' \
> > 
> > This is the only bit that is needed to handle the commented out
> > group case, everythign else is just complexity that comes from
> > over-optimisation. And even handling comments is largely unnecessary
> > because removing deprecated tests is the right way to fix this...
> 
> ...until the next person trips over this.
> 
> --D
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> 

[PATCH 2/4] xfs/148: make test debuggable

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Don't clean up image files - leave them laying around on the test
device so that when the test fails there's a corpse left behind to
post-mortem....

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/148 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tests/xfs/148 b/tests/xfs/148
index 9427aff0..8d50a642 100755
--- a/tests/xfs/148
+++ b/tests/xfs/148
@@ -16,7 +16,7 @@ _cleanup()
 	cd /
 	$UMOUNT_PROG $mntpt > /dev/null 2>&1
 	test -n "$loopdev" && _destroy_loop_device $loopdev > /dev/null 2>&1
-	rm -r -f $imgfile $mntpt $tmp.*
+	rm -r -f $tmp.*
 }
 
 # Import common functions.
@@ -38,6 +38,8 @@ nullstr="too_many_beans"
 slashstr="are_bad_for_you"
 test_names=("something" "$nullstr" "$slashstr" "another")
 
+rm -f $imgfile $imgfile.old
+
 # Format image file w/o crcs so we can sed the image file
 $XFS_IO_PROG -f -c 'truncate 40m' $imgfile
 loopdev=$(_create_loop_device $imgfile)
@@ -91,7 +93,6 @@ sed -b \
 	-i $imgfile
 test "$(md5sum < $imgfile)" != "$(md5sum < $imgfile.old)" ||
 	_fail "sed failed to change the image file?"
-rm -f $imgfile.old
 loopdev=$(_create_loop_device $imgfile)
 _mount $loopdev $mntpt
 
-- 
2.35.1

[PATCH 3/4] xfs/148: fix failure from bad shortform size assumptions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

We replaced an attr named:

slashstr="are_bad_for_you"

with this substitution:

cp $imgfile $imgfile.old
sed -b \
        -e "s/$nullstr/too_many\x00beans/g" \
        -e "s/$slashstr/are_bad\/for_you/g" \
        -i $imgfile

We then try to retreive the attr named 'a_are_bad/for_you'. The
failure is:

    -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
    -heh
    +attr_get: No data available
    +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile

The error returned is ENODATA - the xattr does not exist. While the
name might exist in the attr leaf block:

....
nvlist[0].valuelen = 3
nvlist[0].namelen = 17
nvlist[0].name = "a_are_bad/for_you"
nvlist[0].value = "heh"
nvlist[1].valuelen = 3
....

xattrs are not looked up by name matches when in leaf or node form
like they are in short form.  They are looked up by *name hash*
matches, and if the hash is not found, then the name does not exist.
Only if the has match is found, then it goes and retrieves the xattr
pointed to by the hash and checks the name.

At this point, it should be obvious that the hash of
"a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
leaf lookup is always rejected at the hash match stage and never
gets to the name compare stage.

IOWs, this test can *never* pass when the xattr is in leaf/node
form, only when it is in short form.

The reason the attr fork is in leaf form is that we are using
"-m crc=0" and so the inodes are only 256 bytes in size and can only
hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
for shortform attr data. The test consumes ~80 bytes of shortform
space, so it should fit and the test pass.

However:

nvlist[4].valuelen = 37
nvlist[4].namelen = 7
nvlist[4].name = "selinux"
nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"

Yes, I run the fstests with selinux enabled on some of test
machines. The selinux attr pushes the attr fork way over the size
that can fit in the shortform literal area, and so it moves to leaf
form as the attrs are initially added and the test fails.

Fix this by forcing the test to use 512 byte inodes, so as to
provide around 350 bytes of usable attr fork literal area so it's
not affected by security attributes.

While there, clean up the silly conditional loop device cleanup
code.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/148 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/xfs/148 b/tests/xfs/148
index 8d50a642..5d0a0bf4 100755
--- a/tests/xfs/148
+++ b/tests/xfs/148
@@ -15,7 +15,7 @@ _cleanup()
 {
 	cd /
 	$UMOUNT_PROG $mntpt > /dev/null 2>&1
-	test -n "$loopdev" && _destroy_loop_device $loopdev > /dev/null 2>&1
+	_destroy_loop_device $loopdev > /dev/null 2>&1
 	rm -r -f $tmp.*
 }
 
@@ -41,9 +41,12 @@ test_names=("something" "$nullstr" "$slashstr" "another")
 rm -f $imgfile $imgfile.old
 
 # Format image file w/o crcs so we can sed the image file
+# We need to use 512 byte inodes to ensure the attr forks remain in short form
+# even when security xattrs are present so we are always doing name matches on
+# lookup and not name hash compares as leaf/node forms will do.
 $XFS_IO_PROG -f -c 'truncate 40m' $imgfile
 loopdev=$(_create_loop_device $imgfile)
-MKFS_OPTIONS="-m crc=0" _mkfs_dev $loopdev >> $seqres.full
+MKFS_OPTIONS="-m crc=0 -i size=512" _mkfs_dev $loopdev >> $seqres.full
 
 # Mount image file
 mkdir -p $mntpt
@@ -121,9 +124,6 @@ res=$?
 test $res -eq 1 || \
 	echo "repair failed to report corruption ($res)"
 
-_destroy_loop_device $loopdev
-loopdev=
-
 # success, all done
 status=0
 exit
-- 
2.35.1

Re: [PATCH 3/4] xfs/148: fix failure from bad shortform size assumptions

[Darrick J. Wong]

On Mon, May 16, 2022 at 06:59:21PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> We replaced an attr named:
> 
> slashstr="are_bad_for_you"
> 
> with this substitution:
> 
> cp $imgfile $imgfile.old
> sed -b \
>         -e "s/$nullstr/too_many\x00beans/g" \
>         -e "s/$slashstr/are_bad\/for_you/g" \
>         -i $imgfile
> 
> We then try to retreive the attr named 'a_are_bad/for_you'. The
> failure is:
> 
>     -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
>     -heh
>     +attr_get: No data available
>     +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile
> 
> The error returned is ENODATA - the xattr does not exist. While the
> name might exist in the attr leaf block:
> 
> ....
> nvlist[0].valuelen = 3
> nvlist[0].namelen = 17
> nvlist[0].name = "a_are_bad/for_you"
> nvlist[0].value = "heh"
> nvlist[1].valuelen = 3
> ....
> 
> xattrs are not looked up by name matches when in leaf or node form
> like they are in short form.  They are looked up by *name hash*
> matches, and if the hash is not found, then the name does not exist.
> Only if the has match is found, then it goes and retrieves the xattr
> pointed to by the hash and checks the name.
> 
> At this point, it should be obvious that the hash of
> "a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
> leaf lookup is always rejected at the hash match stage and never
> gets to the name compare stage.
> 
> IOWs, this test can *never* pass when the xattr is in leaf/node
> form, only when it is in short form.
> 
> The reason the attr fork is in leaf form is that we are using
> "-m crc=0" and so the inodes are only 256 bytes in size and can only
> hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
> for shortform attr data. The test consumes ~80 bytes of shortform
> space, so it should fit and the test pass.
> 
> However:
> 
> nvlist[4].valuelen = 37
> nvlist[4].namelen = 7
> nvlist[4].name = "selinux"
> nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"
> 
> Yes, I run the fstests with selinux enabled on some of test
> machines. The selinux attr pushes the attr fork way over the size
> that can fit in the shortform literal area, and so it moves to leaf
> form as the attrs are initially added and the test fails.
> 
> Fix this by forcing the test to use 512 byte inodes, so as to
> provide around 350 bytes of usable attr fork literal area so it's
> not affected by security attributes.

I've long wondered if I should patch in a "security" module that
automatically pastes in a fake "s3linux" attribute so that I can
experience the different fs behavior that y'all see.

> While there, clean up the silly conditional loop device cleanup
> code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/xfs/148 | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/xfs/148 b/tests/xfs/148
> index 8d50a642..5d0a0bf4 100755
> --- a/tests/xfs/148
> +++ b/tests/xfs/148
> @@ -15,7 +15,7 @@ _cleanup()
>  {
>  	cd /
>  	$UMOUNT_PROG $mntpt > /dev/null 2>&1
> -	test -n "$loopdev" && _destroy_loop_device $loopdev > /dev/null 2>&1
> +	_destroy_loop_device $loopdev > /dev/null 2>&1
>  	rm -r -f $tmp.*
>  }
>  
> @@ -41,9 +41,12 @@ test_names=("something" "$nullstr" "$slashstr" "another")
>  rm -f $imgfile $imgfile.old
>  
>  # Format image file w/o crcs so we can sed the image file
> +# We need to use 512 byte inodes to ensure the attr forks remain in short form
> +# even when security xattrs are present so we are always doing name matches on
> +# lookup and not name hash compares as leaf/node forms will do.
>  $XFS_IO_PROG -f -c 'truncate 40m' $imgfile
>  loopdev=$(_create_loop_device $imgfile)
> -MKFS_OPTIONS="-m crc=0" _mkfs_dev $loopdev >> $seqres.full
> +MKFS_OPTIONS="-m crc=0 -i size=512" _mkfs_dev $loopdev >> $seqres.full
>  
>  # Mount image file
>  mkdir -p $mntpt
> @@ -121,9 +124,6 @@ res=$?
>  test $res -eq 1 || \
>  	echo "repair failed to report corruption ($res)"
>  
> -_destroy_loop_device $loopdev
> -loopdev=
> -
>  # success, all done
>  status=0
>  exit
> -- 
> 2.35.1
> 

Re: [PATCH 3/4] xfs/148: fix failure from bad shortform size assumptions

[Dave Chinner]

On Mon, May 16, 2022 at 08:37:04AM -0700, Darrick J. Wong wrote:
> On Mon, May 16, 2022 at 06:59:21PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > We replaced an attr named:
> > 
> > slashstr="are_bad_for_you"
> > 
> > with this substitution:
> > 
> > cp $imgfile $imgfile.old
> > sed -b \
> >         -e "s/$nullstr/too_many\x00beans/g" \
> >         -e "s/$slashstr/are_bad\/for_you/g" \
> >         -i $imgfile
> > 
> > We then try to retreive the attr named 'a_are_bad/for_you'. The
> > failure is:
> > 
> >     -Attribute "a_are_bad/for_you" had a 3 byte value for TEST_DIR/mount-148/testfile:
> >     -heh
> >     +attr_get: No data available
> >     +Could not get "a_are_bad/for_you" for TEST_DIR/mount-148/testfile
> > 
> > The error returned is ENODATA - the xattr does not exist. While the
> > name might exist in the attr leaf block:
> > 
> > ....
> > nvlist[0].valuelen = 3
> > nvlist[0].namelen = 17
> > nvlist[0].name = "a_are_bad/for_you"
> > nvlist[0].value = "heh"
> > nvlist[1].valuelen = 3
> > ....
> > 
> > xattrs are not looked up by name matches when in leaf or node form
> > like they are in short form.  They are looked up by *name hash*
> > matches, and if the hash is not found, then the name does not exist.
> > Only if the has match is found, then it goes and retrieves the xattr
> > pointed to by the hash and checks the name.
> > 
> > At this point, it should be obvious that the hash of
> > "a_are_bad_for_you" is different to "a_a_are_bad/for_you". Hence the
> > leaf lookup is always rejected at the hash match stage and never
> > gets to the name compare stage.
> > 
> > IOWs, this test can *never* pass when the xattr is in leaf/node
> > form, only when it is in short form.
> > 
> > The reason the attr fork is in leaf form is that we are using
> > "-m crc=0" and so the inodes are only 256 bytes in size and can only
> > hold ~150 bytes in the literal area. That leaves ~100 bytes maximum
> > for shortform attr data. The test consumes ~80 bytes of shortform
> > space, so it should fit and the test pass.
> > 
> > However:
> > 
> > nvlist[4].valuelen = 37
> > nvlist[4].namelen = 7
> > nvlist[4].name = "selinux"
> > nvlist[4].value = "unconfined_u:object_r:unlabeled_t:s0\000"
> > 
> > Yes, I run the fstests with selinux enabled on some of test
> > machines. The selinux attr pushes the attr fork way over the size
> > that can fit in the shortform literal area, and so it moves to leaf
> > form as the attrs are initially added and the test fails.
> > 
> > Fix this by forcing the test to use 512 byte inodes, so as to
> > provide around 350 bytes of usable attr fork literal area so it's
> > not affected by security attributes.
> 
> I've long wondered if I should patch in a "security" module that
> automatically pastes in a fake "s3linux" attribute so that I can
> experience the different fs behavior that y'all see.

fstests basically already does that when selinux is turned on by
setting a context mount option automatically:

MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 /dev/vdb /mnt/scratch

The "-o context" options basically defines the security xattr that
is written to every file that is created on the scratch device. All
I've done on this VM is add this to the kernel CLI options:

selinux=1 security=selinux

and nothing else. Everyone should have at least on test VM set up
this way - we need to ensure that LSM paths are actually exercised
at some point during testing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 4/4] generic/081: don't run on DAX capable devices

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

LVM/DM has conniptions when you try to use snapshots on a device
that has DAX capability. It first sets up the underlying device as a
DAX capable mapping (type 3 or DM_TYPE_DAX_BIO_BASED) but because
snapshots require COW and shared mappings, it isn't supported on DAX
capable devices. Hence creating the snapshot device fails because it
requires a type 1 (DM_TYPE_BIO_BASED) device and DM can't change
types on a loaded mapping.

Hence we get this obscure error message in the log:

device-mapper: ioctl: can't change device type (old=3 vs new=1) after initial table load.

and these obscure, unhelpful error messages from the LVM command
outputs:

  device-mapper: reload ioctl on  (251:0) failed: Invalid argument
  Failed to suspend logical volume vg_081/base_081.
  Device vg_081-base_081-real (251:1) is used by another device.
  Failed to revert logical volume vg_081/base_081.
  Aborting. Manual intervention required.
Failed to create snapshot

How to turn off DAX capability is not documented in dmsetup or LVM
man pages, nor is dax mentioned anywhere in
Documentation/admin/device-mapper/ so I have no idea how to tell
LVM/DM "don't try to enable DAX support!".

As such, if the uderlying block device is dax capable, skip this
test.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 common/rc | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/common/rc b/common/rc
index 4201a059..f5ead044 100644
--- a/common/rc
+++ b/common/rc
@@ -2167,7 +2167,7 @@ _require_sane_bdev_flush()
 #	3. "dax=inode" or nothing means "use scratch dev capability" to
 #	    determine whether DAX is going to be used.
 #
-# Returns 0 if DAX will be used, 1 if DAX is not going to be used.
+# Returns 0 if the filesytem will use DAX, 1 if it won't.
 __scratch_uses_fsdax()
 {
 	local ops=$(_normalize_mount_options "$MOUNT_OPTIONS")
@@ -2175,9 +2175,19 @@ __scratch_uses_fsdax()
 	echo $ops | egrep -qw "dax(=always| |$)" && return 0
 	echo $ops | grep -qw "dax=never" && return 1
 
+	return 0
+}
+
+# Determine if the scratch device is DAX capable. Every if the fs is not
+# using DAX, we still can't use certain device mapper targets if the block
+# device is DAX capable. hence the check needs to be separat from the FS
+# capability.
+__scratch_dev_has_dax()
+{
 	local sysfs="/sys/block/$(_short_dev $SCRATCH_DEV)"
 	test -e "${sysfs}/dax" && return 0
 	test "$(cat "${sysfs}/queue/dax" 2>/dev/null)" = "1" && return 0
+
 	return 1
 }
 
@@ -2194,15 +2204,18 @@ _require_dm_target()
 	_require_sane_bdev_flush $SCRATCH_DEV
 	_require_command "$DMSETUP_PROG" dmsetup
 
-	if __scratch_uses_fsdax; then
-		case $target in
-		stripe|linear|log-writes)
-			;;
-		*)
-			_notrun "Cannot run tests with DAX on $target devices."
-			;;
-		esac
-	fi
+	case $target in
+	stripe|linear|log-writes)
+		;;
+	*)
+		if __scratch_uses_fsdax; then
+			_notrun "Cannot run tests with fsdax on $target devices."
+		fi
+		if __scratch_dev_has_dax; then
+			_notrun "Cannot use $target devices on DAX capable block devices."
+		fi
+		;;
+	esac
 
 	modprobe dm-$target >/dev/null 2>&1
 
-- 
2.35.1