[PATCH 00/13 V2] fstests: fixes and more fixes...

[PATCH 00/13 V2] fstests: fixes and more fixes...

[Dave Chinner]

Hi folks,

This is a rollup of the two patch series I sent yesterday titled:

[PATCH 0/3] fstests: test updates for XFS 5.19 kernel mods
[PATCH 0/3] fstests: more fixes...

I've fixed some patches, added documentation for those changes,
and added a bunch of patches to remove deprecated/broken tests that
should ahve been removed long ago.

I do not know if patches 9-11 will make it through vger this time -
they didn't as a single patch. They are titled:

[PATCH 09/12] xfs/018: remove deprecated test
[PATCH 10/12] xfs/081: remove deprecated test
[PATCH 11/12] xfs/082: remove deprecated test

and is thould be pretty obvious what they do from the title....

As for:

[PATCH 04/12] fstests: fix group list generation for whacky test

I've updated it to use $VALID_TEST_NAME so that it doesn't miss
names like xfs/191-input_validation as can be seen here:

$ sudo MOUNT_OPTIONS= ./check -b -s xfs -n -g mkfs
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/405
shared/032
xfs/019
xfs/029
xfs/031
xfs/096
xfs/178
xfs/191-input-validation
xfs/260
xfs/279
xfs/284
xfs/292
xfs/419
xfs/500
xfs/504
xfs/522
xfs/523
xfs/524
xfs/525
xfs/526
xfs/543

And I've document the use of _begin_fstests by the build
infrastructure and the constraints that introduces into it's
definition in the README file section that documents how to define
test groups for a given test. That's all in:

[PATCH 05/12] README: document _begin_fstests better

These fixes address all the known internal fstests problems with the
new group list generation code. I still don't know why Darrick had
deprecated tests run inappropriately or marked as failed - I can't
reproduce anything like what he described and he gave me no details
of how to reproduce what he saw.

Cheers,

Dave.

[PATCH 01/12] fstests: filter quota warnings

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Quota warning code has been removed from the upstream kernel and now
returns -EINVAL errors. Seeing as we can't set warnings anymore and
they have always been non-functional in the kernel, just remove the
calls to set warnings filter those errors out so those tests pass
again.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/050 | 9 ---------
 tests/xfs/153 | 9 ---------
 tests/xfs/299 | 9 ---------
 3 files changed, 27 deletions(-)

diff --git a/tests/xfs/050 b/tests/xfs/050
index 1847611b..2220e470 100755
--- a/tests/xfs/050
+++ b/tests/xfs/050
@@ -84,9 +84,6 @@ _exercise()
 	_qsetup $1
 
 	echo "Using type=$type id=$id" >>$seqres.full
-
-	$XFS_QUOTA_PROG -x -c "warn -$type 65535 -d" $SCRATCH_DEV
-
 	echo
 	echo "*** report no quota settings" | tee -a $seqres.full
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
@@ -113,7 +110,6 @@ _exercise()
 	_file_as_id $SCRATCH_MNT/softie3 $id $type 1024 0
 	_file_as_id $SCRATCH_MNT/softie4 $id $type 1024 0
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -122,8 +118,6 @@ _exercise()
 	echo "*** push past the soft block limit" | tee -a $seqres.full
 	_file_as_id $SCRATCH_MNT/softie $id $type $bsize 300
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" \
-		-c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -136,8 +130,6 @@ _exercise()
 		_file_as_id $SCRATCH_MNT/hard$i $id $type 1024 0
 	done
 	_qmount
-	$XFS_QUOTA_PROG -x  -c "warn -b -$type 0 $id" \
-		-c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -149,7 +141,6 @@ _exercise()
 	echo "ls -l $SCRATCH_MNT" >>$seqres.full
 	ls -l $SCRATCH_MNT >>$seqres.full
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_and_check_blks | LC_COLLATE=POSIX sort -ru
diff --git a/tests/xfs/153 b/tests/xfs/153
index 8e1430c0..dbe26b68 100755
--- a/tests/xfs/153
+++ b/tests/xfs/153
@@ -89,9 +89,6 @@ run_tests()
 	_qsetup $1
 
 	echo "Using type=$type id=$id" >>$seqres.full
-
-	$XFS_QUOTA_PROG -x -c "warn -$type 65535 -d" $SCRATCH_DEV
-
 	echo
 	echo "*** report no quota settings" | tee -a $seqres.full
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
@@ -122,7 +119,6 @@ run_tests()
 	_file_as_id $SCRATCH_MNT/softie4 0 $type 1024 0
 	_scratch_umount_idmapped
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -133,8 +129,6 @@ run_tests()
 	_file_as_id $SCRATCH_MNT/softie 0 $type $bsize 300
 	_scratch_umount_idmapped
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" \
-		-c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -149,8 +143,6 @@ run_tests()
 		_scratch_umount_idmapped
 	done
 	_qmount
-	$XFS_QUOTA_PROG -x  -c "warn -b -$type 0 $id" \
-		-c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -164,7 +156,6 @@ run_tests()
 	echo "ls -l $SCRATCH_MNT" >>$seqres.full
 	ls -l $SCRATCH_MNT >>$seqres.full
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_and_check_blks | LC_COLLATE=POSIX sort -ru
diff --git a/tests/xfs/299 b/tests/xfs/299
index a3077b0c..4b9df3c6 100755
--- a/tests/xfs/299
+++ b/tests/xfs/299
@@ -70,9 +70,6 @@ _exercise()
 	_qsetup $1
 
 	echo "Using type=$type id=$id" >>$seqres.full
-
-	$XFS_QUOTA_PROG -x -c "warn -$type 65535 -d" $SCRATCH_DEV
-
 	echo
 	echo "*** report no quota settings" | tee -a $seqres.full
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
@@ -99,7 +96,6 @@ _exercise()
 	_file_as_id $SCRATCH_MNT/softie3 $id $type 1024 0
 	_file_as_id $SCRATCH_MNT/softie4 $id $type 1024 0
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -108,8 +104,6 @@ _exercise()
 	echo "*** push past the soft block limit" | tee -a $seqres.full
 	_file_as_id $SCRATCH_MNT/softie $id $type $bsize 200
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -i -$type 0 $id" \
-		-c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -122,8 +116,6 @@ _exercise()
 		_file_as_id $SCRATCH_MNT/hard$i $id $type 1024 0
 	done
 	_qmount
-	$XFS_QUOTA_PROG -x  -c "warn -b -$type 0 $id" \
-		-c "warn -i -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_quota_report | LC_COLLATE=POSIX sort -ru
@@ -135,7 +127,6 @@ _exercise()
 	echo "ls -l $SCRATCH_MNT" >>$seqres.full
 	ls -l $SCRATCH_MNT >>$seqres.full
 	_qmount
-	$XFS_QUOTA_PROG -x -c "warn -b -$type 0 $id" $SCRATCH_DEV
 	$XFS_QUOTA_PROG -D $tmp.projects -P $tmp.projid -x \
 		-c "repquota -birnN -$type" $SCRATCH_DEV |
 		_filter_and_check_blks | LC_COLLATE=POSIX sort -ru
-- 
2.35.1

[PATCH 02/12] xfs/122: add attribute log formats to test output.

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Now that we have merged logged attributes, add the attribute log
format sizes to the on-disk structure size checking.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/122.out | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/xfs/122.out b/tests/xfs/122.out
index 608faa4b..a56cbee8 100644
--- a/tests/xfs/122.out
+++ b/tests/xfs/122.out
@@ -63,6 +63,8 @@ sizeof(struct xfs_attr3_leaf_hdr) = 80
 sizeof(struct xfs_attr3_leafblock) = 88
 sizeof(struct xfs_attr3_rmt_hdr) = 56
 sizeof(struct xfs_attr_shortform) = 8
+sizeof(struct xfs_attrd_log_format) = 16
+sizeof(struct xfs_attri_log_format) = 40
 sizeof(struct xfs_btree_block) = 72
 sizeof(struct xfs_btree_block_lhdr) = 64
 sizeof(struct xfs_btree_block_shdr) = 48
-- 
2.35.1

[PATCH 03/12] xfs/348: golden output is not correct

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

The output of xfs/348 encodes the observed behaviour at the time the
test was written, not the correct behaviour that should occur.
Recent improvements in verification checking in the upstream kernel
(1eb70f54c445 "xfs: validate inode fork size against fork format")
have resulted in the kernel having correct behaviour and now the
test fails. Fix the test output to reflect correct behaviour.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/348.out | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/xfs/348.out b/tests/xfs/348.out
index 0d407718..9130f42a 100644
--- a/tests/xfs/348.out
+++ b/tests/xfs/348.out
@@ -108,7 +108,7 @@ would have junked entry "DATA" in directory PARENT_INO
 would have junked entry "EMPTY" in directory PARENT_INO
 would have junked entry "FIFO" in directory PARENT_INO
 stat: 'SCRATCH_MNT/test/DIR' is a directory
-stat: 'SCRATCH_MNT/test/DATA' is a directory
+stat: cannot statx 'SCRATCH_MNT/test/DATA': Structure needs cleaning
 stat: cannot statx 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
 stat: cannot statx 'SCRATCH_MNT/test/SYMLINK': Structure needs cleaning
 stat: cannot statx 'SCRATCH_MNT/test/CHRDEV': Structure needs cleaning
@@ -240,7 +240,7 @@ would have junked entry "DIR" in directory PARENT_INO
 would have junked entry "EMPTY" in directory PARENT_INO
 would have junked entry "FIFO" in directory PARENT_INO
 stat: cannot statx 'SCRATCH_MNT/test/DIR': Structure needs cleaning
-stat: 'SCRATCH_MNT/test/DATA' is a symbolic link
+stat: cannot statx 'SCRATCH_MNT/test/DATA': Structure needs cleaning
 stat: cannot statx 'SCRATCH_MNT/test/EMPTY': Structure needs cleaning
 stat: 'SCRATCH_MNT/test/SYMLINK' is a symbolic link
 stat: cannot statx 'SCRATCH_MNT/test/CHRDEV': Structure needs cleaning
-- 
2.35.1

[PATCH 04/12] 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
$

Use the regexes for matching test names defined in common/test_names
rather than trying to open code it.

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

diff --git a/tools/mkgroupfile b/tools/mkgroupfile
index 24435898..414cb538 100755
--- a/tools/mkgroupfile
+++ b/tools/mkgroupfile
@@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
 	exit 1
 fi
 
+. ../../common/test_names
+
 cleanup()
 {
 	rm -f $new_groups.check
@@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
+		>> $new_groups
 
 	# Create the list of unique groups for existence checking
 	grep -I -R "^_begin_fstest" $test_dir/ | \
-- 
2.35.1

[PATCH 05/12] README: document _begin_fstests better

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Because how it actually gets used by the fstests infrastructure
has been undocumented and that has impact on how it should be set
up.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 README | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/README b/README
index 7da66cb6..eacf1acd 100644
--- a/README
+++ b/README
@@ -368,19 +368,42 @@ Test script environment:
 
      6. Test group membership: Each test can be associated with any number
 	of groups for convenient selection of subsets of tests.  Group names
-	can be any sequence of non-whitespace characters.  Test authors
-	associate a test with groups by passing the names of those groups as
-	arguments to the _begin_fstest function.  For example, the code:
+	can be any sequence of non-whitespace characters, though human-readable
+	names that match the set [A-Za-z0-9\-] are highly prefered.
 
-	_begin_fstest auto quick subvol snapshot
+	Test authors associate a test with groups by passing the names of those
+	groups as arguments to the _begin_fstest function. While _begin_fstests
+	is a shell function that must be called at the start of a test to
+	initialise the test environment correctly, the the build infrastructure
+	also scans the test files for _begin_fstests invocations. It does this
+	to compile the group lists that are used to determine which tests to run
+	when `check` is executed. In other words, test files files must call
+	_begin_fstest with their intended groups or they will not be run.
+
+	However, because the build infrastructure also uses _begin_fstests as
+	a defined keyword, addition restrictions are placed on how it must be
+	formatted:
+
+	(a) It must be a single line with no multi-line continuations.
+
+	(b) group names should be separated by spaces and not other whitespace
+
+	(c) A '#' placed anywhere in the list, even in the middle of a group
+	    name, will cause everything from the # to the end of the line to be
+	    ignored.
+
+	For example, the code:
+
+	_begin_fstest auto quick subvol snapshot # metadata
 
 	associates the current test with the "auto", "quick", "subvol", and
-	"snapshot" groups.  It is not necessary to specify the "all" group
-	in the list because that group is computed at run time.
+	"snapshot" groups. Because "metadata" is after the "#" comment
+	delimiter, it is ignored by the build infrastructure and so it will not
+	be associated with that group.
+
+	It is not necessary to specify the "all" group in the list because that
+	group is always computed at run time from the group lists.
 
-	The build process scans test files for _begin_fstest invocations and
-	compiles the group list from that information.  In other words, test
-	files must call _begin_fstest or they will not be run.
 
 Verified output:
 
-- 
2.35.1

[PATCH 06/12] 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 07/12] 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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

[PATCH 08/12] 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

[PATCH 12/12] xfs/191: remove broken test

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Test has been marked broken for a long time. Original intent of
validating all mkfs.xfs input is not realistic, nobody is going to
try to make that happen.

Remove it as it serves no useful purpose except to have a whacky
unique name that nobody expects or codes for.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 tests/xfs/191-input-validation     | 324 -----------------------------
 tests/xfs/191-input-validation.out |   2 -
 2 files changed, 326 deletions(-)
 delete mode 100755 tests/xfs/191-input-validation
 delete mode 100644 tests/xfs/191-input-validation.out

diff --git a/tests/xfs/191-input-validation b/tests/xfs/191-input-validation
deleted file mode 100755
index 2728846e..00000000
--- a/tests/xfs/191-input-validation
+++ /dev/null
@@ -1,324 +0,0 @@
-#! /bin/bash
-# SPDX-License-Identifier: GPL-2.0
-# Copyright (c) 2016 Red Hat, Inc.  All Rights Reserved.
-#
-# FS QA Test No. xfs/191
-#
-# mkfs.xfs input validation test. Designed to break mkfs.xfs if it doesn't
-# filter garbage input or invalid option combinations correctly.
-#
-. ./common/preamble
-_begin_fstest auto quick mkfs realtime
-
-# Override the default cleanup function.
-_cleanup()
-{
-	cd /
-	rm -f $tmp.*
-	[ -n "$loopdev" ] && _destroy_loop_device $loopdev
-}
-
-# Import common functions.
-. ./common/filter
-
-# real QA test starts here
-
-# Modify as appropriate.
-_supported_fs xfs
-_require_test
-_require_loop
-_require_xfs_mkfs_validation
-
-echo silence is golden
-
-# clear out any options to mkfs first. We want to test realtime and external log
-# devices if we can, but we also want to control them ourselves.
-logdev=$SCRATCH_LOGDEV
-rtdev=$SCRATCH_RTDEV
-
-MKFS_OPTIONS=
-SCRATCH_LOGDEV=
-SCRATCH_RTDEV=
-
-# limit the image size of the filesystem being created to something small
-fssize=$((4 * 1024 * 1024 * 1024))
-logsize=$((4 * 1024 * 1024 * 100))
-fsimg=$TEST_DIR/$seq.img
-
-#create a loop device to test
-loopimg=$TEST_DIR/$seq.loopimg
-$XFS_IO_PROG -f -c "truncate $fssize" $loopimg
-loopdev=$(_create_loop_device $loopimg)
-SCRATCH_DEV=$loopdev
-
-do_mkfs_pass()
-{
-	echo >> $seqres.full
-	echo "pass expected $*" >> $seqres.full
-	$MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
-	[ $? -ne 0 ] && echo "fail $*"
-}
-
-do_mkfs_fail()
-{
-	echo >> $seqres.full
-	echo "fail expected $*" >> $seqres.full
-	$MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
-	[ $? -eq 0 ] && echo "pass $*"
-}
-
-reset_fsimg()
-{
-	rm -f $fsimg
-	$XFS_IO_PROG -f -c "truncate $fssize" $fsimg
-}
-
-reset_fsimg
-
-do_mkfs_pass $SCRATCH_DEV
-
-# basic "should fail" options
-
-# specifying sector sizes in sectors or blocks or garbage
-do_mkfs_fail -s size=2s $SCRATCH_DEV
-do_mkfs_fail -d sectsize=2s $SCRATCH_DEV
-do_mkfs_fail -l sectsize=2s $SCRATCH_DEV
-do_mkfs_fail -s size=2b $SCRATCH_DEV
-do_mkfs_fail -d sectsize=2b $SCRATCH_DEV
-do_mkfs_fail -l sectsize=2b $SCRATCH_DEV
-
-do_mkfs_fail -s size=grot $SCRATCH_DEV
-do_mkfs_fail -s size=2yerk $SCRATCH_DEV
-do_mkfs_fail -d sectsize=blah $SCRATCH_DEV
-do_mkfs_fail -d sectsize=2foo $SCRATCH_DEV
-do_mkfs_fail -l sectsize=nggh $SCRATCH_DEV
-do_mkfs_fail -l sectsize=2nggh $SCRATCH_DEV
-
-# conflicting sector/block sizes
-do_mkfs_fail -s size=512 -d sectsize=1024 $SCRATCH_DEV
-do_mkfs_fail -s size=512 -l sectsize=1024 $SCRATCH_DEV
-do_mkfs_fail -d sectsize=2048 -l sectsize=1024 $SCRATCH_DEV
-
-do_mkfs_fail -b size=512 -s size=1024 $SCRATCH_DEV
-do_mkfs_fail -b size=512 -d sectsize=1024 $SCRATCH_DEV
-do_mkfs_fail -b size=512 -l sectsize=1024 $SCRATCH_DEV
-
-# specifying block sizes in sectors without specifying sector size
-# or in blocks or garbage
-do_mkfs_fail -b size=2s $SCRATCH_DEV
-do_mkfs_fail -b size=2b $SCRATCH_DEV
-do_mkfs_fail -b size=nfi $SCRATCH_DEV
-do_mkfs_fail -b size=4096nfi $SCRATCH_DEV
-do_mkfs_fail -n size=nfi $SCRATCH_DEV
-do_mkfs_fail -n size=4096nfi $SCRATCH_DEV
-
-do_mkfs_pass -n size=2b $SCRATCH_DEV
-do_mkfs_pass -n size=2b $SCRATCH_DEV
-
-# bad label length
-do_mkfs_fail -L thisiswaytoolong $SCRATCH_DEV
-
-# basic "should pass" data section tests
-do_mkfs_pass $SCRATCH_DEV
-do_mkfs_pass -d name=$SCRATCH_DEV
-do_mkfs_pass -d size=$fssize $SCRATCH_DEV
-do_mkfs_pass -d agcount=32 $SCRATCH_DEV
-do_mkfs_pass -d agsize=32m $SCRATCH_DEV
-do_mkfs_pass -d agsize=32M $SCRATCH_DEV
-do_mkfs_pass -d agsize=1g $SCRATCH_DEV
-do_mkfs_pass -d agsize=$((32 * 1024 * 1024)) $SCRATCH_DEV
-do_mkfs_pass -b size=4096 -d agsize=8192b $SCRATCH_DEV
-do_mkfs_pass -d agsize=8192b $SCRATCH_DEV
-do_mkfs_pass -d agsize=65536s $SCRATCH_DEV
-do_mkfs_pass -d sectsize=512,agsize=65536s $SCRATCH_DEV
-do_mkfs_pass -s size=512 -d agsize=65536s $SCRATCH_DEV
-do_mkfs_pass -d noalign $SCRATCH_DEV
-do_mkfs_pass -d sunit=0,swidth=0 $SCRATCH_DEV
-do_mkfs_pass -d sunit=8,swidth=8 $SCRATCH_DEV
-do_mkfs_pass -d sunit=8,swidth=64 $SCRATCH_DEV
-do_mkfs_pass -d su=0,sw=0 $SCRATCH_DEV
-do_mkfs_pass -d su=0,sw=64 $SCRATCH_DEV
-do_mkfs_pass -d su=4096,sw=1 $SCRATCH_DEV
-do_mkfs_pass -d su=4096s,sw=64 $SCRATCH_DEV
-do_mkfs_pass -d su=4096b,sw=64 $SCRATCH_DEV
-do_mkfs_pass -d su=4k,sw=1 $SCRATCH_DEV
-do_mkfs_pass -d su=4K,sw=8 $SCRATCH_DEV
-do_mkfs_pass -b size=4096 -d su=1b,sw=8 $SCRATCH_DEV
-do_mkfs_pass -d sectsize=512,su=8s,sw=8 $SCRATCH_DEV
-do_mkfs_pass -s size=512 -d su=8s,sw=8 $SCRATCH_DEV
-
-# invalid data section tests
-do_mkfs_fail -d size=${fssize}b $SCRATCH_DEV
-do_mkfs_fail -d size=${fssize}s $SCRATCH_DEV
-do_mkfs_fail -d size=${fssize}yerk $SCRATCH_DEV
-do_mkfs_fail -d agsize=32Mbsdfsdo $SCRATCH_DEV
-do_mkfs_fail -d agsize=1GB $SCRATCH_DEV
-do_mkfs_fail -d agcount=1k $SCRATCH_DEV
-do_mkfs_fail -d agcount=6b $SCRATCH_DEV
-do_mkfs_fail -d agcount=32,agsize=32m $SCRATCH_DEV
-do_mkfs_fail -d sunit=0,swidth=64 $SCRATCH_DEV
-do_mkfs_fail -d sunit=64,swidth=0 $SCRATCH_DEV
-do_mkfs_fail -d sunit=64,swidth=64,noalign $SCRATCH_DEV
-do_mkfs_fail -d sunit=64k,swidth=64 $SCRATCH_DEV
-do_mkfs_fail -d sunit=64,swidth=64m $SCRATCH_DEV
-do_mkfs_fail -d su=4096,sw=0 $SCRATCH_DEV
-do_mkfs_fail -d su=4097,sw=1 $SCRATCH_DEV
-do_mkfs_fail -d su=4096,sw=64,noalign $SCRATCH_DEV
-do_mkfs_fail -d su=4096,sw=64s $SCRATCH_DEV
-do_mkfs_fail -d su=4096garabge,sw=64 $SCRATCH_DEV
-do_mkfs_fail -d su=4096,sw=64,sunit=64,swidth=64 $SCRATCH_DEV
-do_mkfs_fail -d sectsize=10,agsize=65536s $SCRATCH_DEV
-do_mkfs_fail -d sectsize=512s,agsize=65536s $SCRATCH_DEV
-
-reset_fsimg
-
-# file section, should pass
-do_mkfs_pass $fsimg
-do_mkfs_pass -d file=0 $SCRATCH_DEV
-do_mkfs_pass -d size=$fssize,file=1,name=$fsimg
-do_mkfs_pass -d size=$fssize,file $fsimg
-do_mkfs_pass -d size=$fssize $fsimg
-do_mkfs_pass -d size=$fssize,name=$fsimg
-do_mkfs_pass -d size=$((fssize/2)) $fsimg
-# again this one, to check that we didn't truncated the file
-do_mkfs_pass -d size=$fssize $fsimg
-rm -f $fsimg
-do_mkfs_pass -d file,size=$fssize $fsimg
-
-reset_fsimg
-
-# file section, should fail
-do_mkfs_fail -d file=1 $SCRATCH_DEV
-do_mkfs_fail -d file $fsimg # no size given
-rm -f $fsimg
-do_mkfs_fail $fsimg
-do_mkfs_fail -d size=$fssize $fsimg
-
-reset_fsimg
-
-# log section, should pass
-do_mkfs_pass -l size=$logsize -d size=$fssize $SCRATCH_DEV
-do_mkfs_pass -l agnum=2 $SCRATCH_DEV
-do_mkfs_pass -l size=4096b $SCRATCH_DEV
-do_mkfs_pass -l sectsize=512 $SCRATCH_DEV
-do_mkfs_pass -l sunit=64 $SCRATCH_DEV
-do_mkfs_pass -l sunit=64 -d sunit=8,swidth=8 $SCRATCH_DEV
-do_mkfs_pass -l sunit=8 $SCRATCH_DEV
-do_mkfs_pass -l su=$((4096*10)) $SCRATCH_DEV
-do_mkfs_pass -l su=10b $SCRATCH_DEV
-do_mkfs_pass -b size=4096 -l su=10b $SCRATCH_DEV
-do_mkfs_pass -l sectsize=512,su=$((4096*10)) $SCRATCH_DEV
-do_mkfs_pass -l internal $SCRATCH_DEV
-$XFS_IO_PROG -f -c "truncate $logsize" $fsimg
-do_mkfs_pass -l logdev=$fsimg $SCRATCH_DEV
-do_mkfs_pass -l name=$fsimg $SCRATCH_DEV
-do_mkfs_pass -l lazy-count=0 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -l lazy-count=1 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -l version=1 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -l version=2 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -l version=2 $SCRATCH_DEV
-
-# log section, should fail
-do_mkfs_fail -l size=${fssize}b $SCRATCH_DEV
-do_mkfs_fail -l size=${fssize}s $SCRATCH_DEV
-do_mkfs_fail -l size=${fssize}yerk $SCRATCH_DEV
-do_mkfs_fail -l agnum=1k $SCRATCH_DEV
-do_mkfs_fail -l agnum=6b $SCRATCH_DEV
-do_mkfs_fail -l agnum=32 $SCRATCH_DEV
-do_mkfs_fail -l sunit=0  $SCRATCH_DEV
-do_mkfs_fail -l sunit=63 $SCRATCH_DEV
-do_mkfs_fail -l su=1 $SCRATCH_DEV
-do_mkfs_fail -l su=10s $SCRATCH_DEV
-do_mkfs_fail -l su=$((4096*10+1)) $SCRATCH_DEV
-do_mkfs_fail -l sectsize=10,agsize=65536s $SCRATCH_DEV
-do_mkfs_fail -l sectsize=512s,agsize=65536s $SCRATCH_DEV
-do_mkfs_fail -l internal=0 $SCRATCH_DEV
-reset_fsimg
-do_mkfs_fail -l internal=1,logdev=$fsimg $SCRATCH_DEV
-do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
-do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
-do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
-do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
-do_mkfs_fail -l version=0  $SCRATCH_DEV
-
-# naming section, should pass
-do_mkfs_pass -n size=65536 $SCRATCH_DEV
-do_mkfs_pass -n version=2 $SCRATCH_DEV
-do_mkfs_pass -n version=ci $SCRATCH_DEV
-do_mkfs_pass -n ftype=0 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -n ftype=1 $SCRATCH_DEV
-
-# naming section, should fail
-do_mkfs_fail -n version=1 $SCRATCH_DEV
-do_mkfs_fail -n version=cid $SCRATCH_DEV
-do_mkfs_fail -n ftype=4 $SCRATCH_DEV
-do_mkfs_fail -n ftype=0 $SCRATCH_DEV
-do_mkfs_fail -n log=15 $SCRATCH_DE
-
-reset_fsimg
-
-# metadata section, should pass
-do_mkfs_pass -m crc=1,finobt=1 $SCRATCH_DEV
-do_mkfs_pass -m crc=1,finobt=0 $SCRATCH_DEV
-do_mkfs_pass -m crc=0,finobt=0 $SCRATCH_DEV
-do_mkfs_pass -m crc=1 -n ftype=1 $SCRATCH_DEV
-do_mkfs_pass -m crc=0 -n ftype=1 $SCRATCH_DEV
-do_mkfs_pass -m crc=0 -n ftype=0 $SCRATCH_DEV
-
-# metadata section, should fail
-do_mkfs_fail -m crc=0,finobt=1 $SCRATCH_DEV
-do_mkfs_fail -m crc=1 -n ftype=0 $SCRATCH_DEV
-
-# realtime section, results depend on reflink
-_scratch_mkfs_xfs_supported -m reflink=0 >/dev/null 2>&1
-if [ $? -eq 0 ]; then
-	do_mkfs_pass -m reflink=0 -r rtdev=$fsimg $SCRATCH_DEV
-	do_mkfs_pass -m reflink=0 -r size=65536,rtdev=$fsimg $SCRATCH_DEV
-	do_mkfs_fail -m reflink=1 -r rtdev=$fsimg $SCRATCH_DEV
-	do_mkfs_fail -m reflink=1 -r size=65536,rtdev=$fsimg $SCRATCH_DEV
-else
-	do_mkfs_pass -r rtdev=$fsimg $SCRATCH_DEV
-	do_mkfs_pass -r size=65536,rtdev=$fsimg $SCRATCH_DEV
-fi
-
-# realtime section, should pass
-do_mkfs_pass -r extsize=4k $SCRATCH_DEV
-do_mkfs_pass -r extsize=1G $SCRATCH_DEV
-do_mkfs_pass -r noalign $SCRATCH_DEV
-
-# realtime section, should fail
-do_mkfs_fail -r rtdev=$SCRATCH_DEV
-do_mkfs_fail -r extsize=256 $SCRATCH_DEV
-do_mkfs_fail -r extsize=2G $SCRATCH_DEV
-do_mkfs_fail -r size=65536 $SCRATCH_DEV
-
-# inode section, should pass
-do_mkfs_pass -i size=256 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -i size=512 $SCRATCH_DEV
-do_mkfs_pass -i size=2048 $SCRATCH_DEV
-do_mkfs_pass -i perblock=2 $SCRATCH_DEV
-do_mkfs_pass -i maxpct=10 $SCRATCH_DEV
-do_mkfs_pass -i maxpct=100 $SCRATCH_DEV
-do_mkfs_pass -i maxpct=0 $SCRATCH_DEV
-do_mkfs_pass -i align=0 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -i align=1 -m crc=1 $SCRATCH_DEV
-do_mkfs_pass -i attr=1 -m crc=0 $SCRATCH_DEV
-do_mkfs_pass -i attr=2 $SCRATCH_DEV
-do_mkfs_pass -i projid32bit $SCRATCH_DEV
-do_mkfs_pass -i sparse=0 $SCRATCH_DEV
-do_mkfs_pass -i sparse -m crc $SCRATCH_DEV
-
-# inode section, should fail
-do_mkfs_fail -i size=256 -m crc $SCRATCH_DEV
-do_mkfs_fail -i size=128 $SCRATCH_DEV
-do_mkfs_fail -i size=513 $SCRATCH_DEV
-do_mkfs_fail -i size=4096 $SCRATCH_DEV
-do_mkfs_fail -i maxpct=110 $SCRATCH_DEV
-do_mkfs_fail -i align=2 $SCRATCH_DEV
-do_mkfs_fail -i sparse -m crc=0 $SCRATCH_DEV
-do_mkfs_fail -i align=0 -m crc=1 $SCRATCH_DEV
-do_mkfs_fail -i attr=1 -m crc=1 $SCRATCH_DEV
-do_mkfs_fail -i log=10 $SCRATCH_DEV
-
-status=0
-exit
diff --git a/tests/xfs/191-input-validation.out b/tests/xfs/191-input-validation.out
deleted file mode 100644
index 020bd625..00000000
--- a/tests/xfs/191-input-validation.out
+++ /dev/null
@@ -1,2 +0,0 @@
-QA output created by 191-input-validation
-silence is golden
-- 
2.35.1

Re: [PATCH 00/13 V2] fstests: fixes and more fixes...

[Dave Chinner]

On Tue, May 17, 2022 at 05:00:59PM +1000, Dave Chinner wrote:
> Hi folks,
> 
> This is a rollup of the two patch series I sent yesterday titled:
> 
> [PATCH 0/3] fstests: test updates for XFS 5.19 kernel mods
> [PATCH 0/3] fstests: more fixes...
> 
> I've fixed some patches, added documentation for those changes,
> and added a bunch of patches to remove deprecated/broken tests that
> should ahve been removed long ago.
> 
> I do not know if patches 9-11 will make it through vger this time -
> they didn't as a single patch. They are titled:
> 
> [PATCH 09/12] xfs/018: remove deprecated test
> [PATCH 10/12] xfs/081: remove deprecated test
> [PATCH 11/12] xfs/082: remove deprecated test
> 
> and is thould be pretty obvious what they do from the title....

Ok, it appears as though even separated vger still won't let a
single one of them through

Zorro, all these patches are 'git rm' commands

git rm tests/xfs/018*
git commit
git rm tests/xfs/081*
git commit
git rm tests/xfs/082*
git commit

I'm happy for you to just run them yourself, or do you want me to
send a pull request for them?

/me realises he hasn't updated his kernel.org fstests tree for 4
years....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 00/13 V2] fstests: fixes and more fixes...

[Zorro Lang]

On Tue, May 17, 2022 at 05:49:10PM +1000, Dave Chinner wrote:
> On Tue, May 17, 2022 at 05:00:59PM +1000, Dave Chinner wrote:
> > Hi folks,
> > 
> > This is a rollup of the two patch series I sent yesterday titled:
> > 
> > [PATCH 0/3] fstests: test updates for XFS 5.19 kernel mods
> > [PATCH 0/3] fstests: more fixes...
> > 
> > I've fixed some patches, added documentation for those changes,
> > and added a bunch of patches to remove deprecated/broken tests that
> > should ahve been removed long ago.
> > 
> > I do not know if patches 9-11 will make it through vger this time -
> > they didn't as a single patch. They are titled:

No, they didn't.

> > 
> > [PATCH 09/12] xfs/018: remove deprecated test
> > [PATCH 10/12] xfs/081: remove deprecated test
> > [PATCH 11/12] xfs/082: remove deprecated test
> > 
> > and is thould be pretty obvious what they do from the title....
> 
> Ok, it appears as though even separated vger still won't let a
> single one of them through
> 
> Zorro, all these patches are 'git rm' commands
> 
> git rm tests/xfs/018*
> git commit
> git rm tests/xfs/081*
> git commit
> git rm tests/xfs/082*
> git commit
> 
> I'm happy for you to just run them yourself, or do you want me to
> send a pull request for them?

Sure, I don't run this 3 cases either. As they're xfs specific cases, so if both
of you and Darrick agree to remove them entirely, I'm glad to help that :)

And how about remove them in single one commit/patch? Likes:
---
[PATCH] fstests: remove xfs deprecated test

Remove xfs/018, xfs/081 and xfs/082 entirely, as they're deprecated
and obsolete for long time.
---

Thanks,
Zorro

> 
> /me realises he hasn't updated his kernel.org fstests tree for 4
> years....
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 

Re: [PATCH 00/13 V2] fstests: fixes and more fixes...

[Dave Chinner]

On Tue, May 17, 2022 at 04:24:09PM +0800, Zorro Lang wrote:
> On Tue, May 17, 2022 at 05:49:10PM +1000, Dave Chinner wrote:
> > On Tue, May 17, 2022 at 05:00:59PM +1000, Dave Chinner wrote:
> > > Hi folks,
> > > 
> > > This is a rollup of the two patch series I sent yesterday titled:
> > > 
> > > [PATCH 0/3] fstests: test updates for XFS 5.19 kernel mods
> > > [PATCH 0/3] fstests: more fixes...
> > > 
> > > I've fixed some patches, added documentation for those changes,
> > > and added a bunch of patches to remove deprecated/broken tests that
> > > should ahve been removed long ago.
> > > 
> > > I do not know if patches 9-11 will make it through vger this time -
> > > they didn't as a single patch. They are titled:
> 
> No, they didn't.
> 
> > > 
> > > [PATCH 09/12] xfs/018: remove deprecated test
> > > [PATCH 10/12] xfs/081: remove deprecated test
> > > [PATCH 11/12] xfs/082: remove deprecated test
> > > 
> > > and is thould be pretty obvious what they do from the title....
> > 
> > Ok, it appears as though even separated vger still won't let a
> > single one of them through
> > 
> > Zorro, all these patches are 'git rm' commands
> > 
> > git rm tests/xfs/018*
> > git commit
> > git rm tests/xfs/081*
> > git commit
> > git rm tests/xfs/082*
> > git commit
> > 
> > I'm happy for you to just run them yourself, or do you want me to
> > send a pull request for them?
> 
> Sure, I don't run this 3 cases either. As they're xfs specific cases, so if both
> of you and Darrick agree to remove them entirely, I'm glad to help that :)
> 
> And how about remove them in single one commit/patch? Likes:
> ---
> [PATCH] fstests: remove xfs deprecated test
> 
> Remove xfs/018, xfs/081 and xfs/082 entirely, as they're deprecated
> and obsolete for long time.
> ---

Fine by me. That's exactly what the first patch I attempted to send
was.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 08/12] generic/081: don't run on DAX capable devices

[Dave Chinner]

On Tue, May 17, 2022 at 05:01:07PM +1000, Dave Chinner wrote:
> 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>

Self nack this one for now - this doesn't seem to be working
properly in the case of "no dax mount option" i.e. the default of
dax=inode. I think in that case __scratch_uses_fsdax() has to return
"no" so that the support check then falls through to
__scratch_dev_has_dax() to determine if DAX will be used or not.

I missed this because I have dax=never set by default on many of my
fstests configs because I use a mix of ramdisk and normal block
devices and I don't want them to use DAX at all when operating on a
ramdisk unless I'm running an explicit DAX-enabled test config.

I'll send an update to fix this soon.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 04/12] fstests: fix group list generation for whacky test names

[Darrick J. Wong]

On Tue, May 17, 2022 at 05:01:03PM +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
> $
> 
> Use the regexes for matching test names defined in common/test_names
> rather than trying to open code it.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Definitely an improvement, thank you.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tools/mkgroupfile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> index 24435898..414cb538 100755
> --- a/tools/mkgroupfile
> +++ b/tools/mkgroupfile
> @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
>  	exit 1
>  fi
>  
> +. ../../common/test_names
> +
>  cleanup()
>  {
>  	rm -f $new_groups.check
> @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_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 06/12] xfs/148: make test debuggable

[Darrick J. Wong]

On Tue, May 17, 2022 at 05:01:05PM +1000, Dave Chinner wrote:
> 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>

I guess I missed this one last time
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  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
> 

Re: [PATCH 12/12] xfs/191: remove broken test

[Darrick J. Wong]

On Tue, May 17, 2022 at 05:01:11PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Test has been marked broken for a long time. Original intent of
> validating all mkfs.xfs input is not realistic, nobody is going to
> try to make that happen.
> 
> Remove it as it serves no useful purpose except to have a whacky
> unique name that nobody expects or codes for.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Good riddance to this and the three other dead tests that vger ate...

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

--D

> ---
>  tests/xfs/191-input-validation     | 324 -----------------------------
>  tests/xfs/191-input-validation.out |   2 -
>  2 files changed, 326 deletions(-)
>  delete mode 100755 tests/xfs/191-input-validation
>  delete mode 100644 tests/xfs/191-input-validation.out
> 
> diff --git a/tests/xfs/191-input-validation b/tests/xfs/191-input-validation
> deleted file mode 100755
> index 2728846e..00000000
> --- a/tests/xfs/191-input-validation
> +++ /dev/null
> @@ -1,324 +0,0 @@
> -#! /bin/bash
> -# SPDX-License-Identifier: GPL-2.0
> -# Copyright (c) 2016 Red Hat, Inc.  All Rights Reserved.
> -#
> -# FS QA Test No. xfs/191
> -#
> -# mkfs.xfs input validation test. Designed to break mkfs.xfs if it doesn't
> -# filter garbage input or invalid option combinations correctly.
> -#
> -. ./common/preamble
> -_begin_fstest auto quick mkfs realtime
> -
> -# Override the default cleanup function.
> -_cleanup()
> -{
> -	cd /
> -	rm -f $tmp.*
> -	[ -n "$loopdev" ] && _destroy_loop_device $loopdev
> -}
> -
> -# Import common functions.
> -. ./common/filter
> -
> -# real QA test starts here
> -
> -# Modify as appropriate.
> -_supported_fs xfs
> -_require_test
> -_require_loop
> -_require_xfs_mkfs_validation
> -
> -echo silence is golden
> -
> -# clear out any options to mkfs first. We want to test realtime and external log
> -# devices if we can, but we also want to control them ourselves.
> -logdev=$SCRATCH_LOGDEV
> -rtdev=$SCRATCH_RTDEV
> -
> -MKFS_OPTIONS=
> -SCRATCH_LOGDEV=
> -SCRATCH_RTDEV=
> -
> -# limit the image size of the filesystem being created to something small
> -fssize=$((4 * 1024 * 1024 * 1024))
> -logsize=$((4 * 1024 * 1024 * 100))
> -fsimg=$TEST_DIR/$seq.img
> -
> -#create a loop device to test
> -loopimg=$TEST_DIR/$seq.loopimg
> -$XFS_IO_PROG -f -c "truncate $fssize" $loopimg
> -loopdev=$(_create_loop_device $loopimg)
> -SCRATCH_DEV=$loopdev
> -
> -do_mkfs_pass()
> -{
> -	echo >> $seqres.full
> -	echo "pass expected $*" >> $seqres.full
> -	$MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
> -	[ $? -ne 0 ] && echo "fail $*"
> -}
> -
> -do_mkfs_fail()
> -{
> -	echo >> $seqres.full
> -	echo "fail expected $*" >> $seqres.full
> -	$MKFS_XFS_PROG -f -N $* >> $seqres.full 2>&1
> -	[ $? -eq 0 ] && echo "pass $*"
> -}
> -
> -reset_fsimg()
> -{
> -	rm -f $fsimg
> -	$XFS_IO_PROG -f -c "truncate $fssize" $fsimg
> -}
> -
> -reset_fsimg
> -
> -do_mkfs_pass $SCRATCH_DEV
> -
> -# basic "should fail" options
> -
> -# specifying sector sizes in sectors or blocks or garbage
> -do_mkfs_fail -s size=2s $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=2s $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=2s $SCRATCH_DEV
> -do_mkfs_fail -s size=2b $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=2b $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=2b $SCRATCH_DEV
> -
> -do_mkfs_fail -s size=grot $SCRATCH_DEV
> -do_mkfs_fail -s size=2yerk $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=blah $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=2foo $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=nggh $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=2nggh $SCRATCH_DEV
> -
> -# conflicting sector/block sizes
> -do_mkfs_fail -s size=512 -d sectsize=1024 $SCRATCH_DEV
> -do_mkfs_fail -s size=512 -l sectsize=1024 $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=2048 -l sectsize=1024 $SCRATCH_DEV
> -
> -do_mkfs_fail -b size=512 -s size=1024 $SCRATCH_DEV
> -do_mkfs_fail -b size=512 -d sectsize=1024 $SCRATCH_DEV
> -do_mkfs_fail -b size=512 -l sectsize=1024 $SCRATCH_DEV
> -
> -# specifying block sizes in sectors without specifying sector size
> -# or in blocks or garbage
> -do_mkfs_fail -b size=2s $SCRATCH_DEV
> -do_mkfs_fail -b size=2b $SCRATCH_DEV
> -do_mkfs_fail -b size=nfi $SCRATCH_DEV
> -do_mkfs_fail -b size=4096nfi $SCRATCH_DEV
> -do_mkfs_fail -n size=nfi $SCRATCH_DEV
> -do_mkfs_fail -n size=4096nfi $SCRATCH_DEV
> -
> -do_mkfs_pass -n size=2b $SCRATCH_DEV
> -do_mkfs_pass -n size=2b $SCRATCH_DEV
> -
> -# bad label length
> -do_mkfs_fail -L thisiswaytoolong $SCRATCH_DEV
> -
> -# basic "should pass" data section tests
> -do_mkfs_pass $SCRATCH_DEV
> -do_mkfs_pass -d name=$SCRATCH_DEV
> -do_mkfs_pass -d size=$fssize $SCRATCH_DEV
> -do_mkfs_pass -d agcount=32 $SCRATCH_DEV
> -do_mkfs_pass -d agsize=32m $SCRATCH_DEV
> -do_mkfs_pass -d agsize=32M $SCRATCH_DEV
> -do_mkfs_pass -d agsize=1g $SCRATCH_DEV
> -do_mkfs_pass -d agsize=$((32 * 1024 * 1024)) $SCRATCH_DEV
> -do_mkfs_pass -b size=4096 -d agsize=8192b $SCRATCH_DEV
> -do_mkfs_pass -d agsize=8192b $SCRATCH_DEV
> -do_mkfs_pass -d agsize=65536s $SCRATCH_DEV
> -do_mkfs_pass -d sectsize=512,agsize=65536s $SCRATCH_DEV
> -do_mkfs_pass -s size=512 -d agsize=65536s $SCRATCH_DEV
> -do_mkfs_pass -d noalign $SCRATCH_DEV
> -do_mkfs_pass -d sunit=0,swidth=0 $SCRATCH_DEV
> -do_mkfs_pass -d sunit=8,swidth=8 $SCRATCH_DEV
> -do_mkfs_pass -d sunit=8,swidth=64 $SCRATCH_DEV
> -do_mkfs_pass -d su=0,sw=0 $SCRATCH_DEV
> -do_mkfs_pass -d su=0,sw=64 $SCRATCH_DEV
> -do_mkfs_pass -d su=4096,sw=1 $SCRATCH_DEV
> -do_mkfs_pass -d su=4096s,sw=64 $SCRATCH_DEV
> -do_mkfs_pass -d su=4096b,sw=64 $SCRATCH_DEV
> -do_mkfs_pass -d su=4k,sw=1 $SCRATCH_DEV
> -do_mkfs_pass -d su=4K,sw=8 $SCRATCH_DEV
> -do_mkfs_pass -b size=4096 -d su=1b,sw=8 $SCRATCH_DEV
> -do_mkfs_pass -d sectsize=512,su=8s,sw=8 $SCRATCH_DEV
> -do_mkfs_pass -s size=512 -d su=8s,sw=8 $SCRATCH_DEV
> -
> -# invalid data section tests
> -do_mkfs_fail -d size=${fssize}b $SCRATCH_DEV
> -do_mkfs_fail -d size=${fssize}s $SCRATCH_DEV
> -do_mkfs_fail -d size=${fssize}yerk $SCRATCH_DEV
> -do_mkfs_fail -d agsize=32Mbsdfsdo $SCRATCH_DEV
> -do_mkfs_fail -d agsize=1GB $SCRATCH_DEV
> -do_mkfs_fail -d agcount=1k $SCRATCH_DEV
> -do_mkfs_fail -d agcount=6b $SCRATCH_DEV
> -do_mkfs_fail -d agcount=32,agsize=32m $SCRATCH_DEV
> -do_mkfs_fail -d sunit=0,swidth=64 $SCRATCH_DEV
> -do_mkfs_fail -d sunit=64,swidth=0 $SCRATCH_DEV
> -do_mkfs_fail -d sunit=64,swidth=64,noalign $SCRATCH_DEV
> -do_mkfs_fail -d sunit=64k,swidth=64 $SCRATCH_DEV
> -do_mkfs_fail -d sunit=64,swidth=64m $SCRATCH_DEV
> -do_mkfs_fail -d su=4096,sw=0 $SCRATCH_DEV
> -do_mkfs_fail -d su=4097,sw=1 $SCRATCH_DEV
> -do_mkfs_fail -d su=4096,sw=64,noalign $SCRATCH_DEV
> -do_mkfs_fail -d su=4096,sw=64s $SCRATCH_DEV
> -do_mkfs_fail -d su=4096garabge,sw=64 $SCRATCH_DEV
> -do_mkfs_fail -d su=4096,sw=64,sunit=64,swidth=64 $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=10,agsize=65536s $SCRATCH_DEV
> -do_mkfs_fail -d sectsize=512s,agsize=65536s $SCRATCH_DEV
> -
> -reset_fsimg
> -
> -# file section, should pass
> -do_mkfs_pass $fsimg
> -do_mkfs_pass -d file=0 $SCRATCH_DEV
> -do_mkfs_pass -d size=$fssize,file=1,name=$fsimg
> -do_mkfs_pass -d size=$fssize,file $fsimg
> -do_mkfs_pass -d size=$fssize $fsimg
> -do_mkfs_pass -d size=$fssize,name=$fsimg
> -do_mkfs_pass -d size=$((fssize/2)) $fsimg
> -# again this one, to check that we didn't truncated the file
> -do_mkfs_pass -d size=$fssize $fsimg
> -rm -f $fsimg
> -do_mkfs_pass -d file,size=$fssize $fsimg
> -
> -reset_fsimg
> -
> -# file section, should fail
> -do_mkfs_fail -d file=1 $SCRATCH_DEV
> -do_mkfs_fail -d file $fsimg # no size given
> -rm -f $fsimg
> -do_mkfs_fail $fsimg
> -do_mkfs_fail -d size=$fssize $fsimg
> -
> -reset_fsimg
> -
> -# log section, should pass
> -do_mkfs_pass -l size=$logsize -d size=$fssize $SCRATCH_DEV
> -do_mkfs_pass -l agnum=2 $SCRATCH_DEV
> -do_mkfs_pass -l size=4096b $SCRATCH_DEV
> -do_mkfs_pass -l sectsize=512 $SCRATCH_DEV
> -do_mkfs_pass -l sunit=64 $SCRATCH_DEV
> -do_mkfs_pass -l sunit=64 -d sunit=8,swidth=8 $SCRATCH_DEV
> -do_mkfs_pass -l sunit=8 $SCRATCH_DEV
> -do_mkfs_pass -l su=$((4096*10)) $SCRATCH_DEV
> -do_mkfs_pass -l su=10b $SCRATCH_DEV
> -do_mkfs_pass -b size=4096 -l su=10b $SCRATCH_DEV
> -do_mkfs_pass -l sectsize=512,su=$((4096*10)) $SCRATCH_DEV
> -do_mkfs_pass -l internal $SCRATCH_DEV
> -$XFS_IO_PROG -f -c "truncate $logsize" $fsimg
> -do_mkfs_pass -l logdev=$fsimg $SCRATCH_DEV
> -do_mkfs_pass -l name=$fsimg $SCRATCH_DEV
> -do_mkfs_pass -l lazy-count=0 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -l lazy-count=1 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -l version=1 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -l version=2 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -l version=2 $SCRATCH_DEV
> -
> -# log section, should fail
> -do_mkfs_fail -l size=${fssize}b $SCRATCH_DEV
> -do_mkfs_fail -l size=${fssize}s $SCRATCH_DEV
> -do_mkfs_fail -l size=${fssize}yerk $SCRATCH_DEV
> -do_mkfs_fail -l agnum=1k $SCRATCH_DEV
> -do_mkfs_fail -l agnum=6b $SCRATCH_DEV
> -do_mkfs_fail -l agnum=32 $SCRATCH_DEV
> -do_mkfs_fail -l sunit=0  $SCRATCH_DEV
> -do_mkfs_fail -l sunit=63 $SCRATCH_DEV
> -do_mkfs_fail -l su=1 $SCRATCH_DEV
> -do_mkfs_fail -l su=10s $SCRATCH_DEV
> -do_mkfs_fail -l su=$((4096*10+1)) $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=10,agsize=65536s $SCRATCH_DEV
> -do_mkfs_fail -l sectsize=512s,agsize=65536s $SCRATCH_DEV
> -do_mkfs_fail -l internal=0 $SCRATCH_DEV
> -reset_fsimg
> -do_mkfs_fail -l internal=1,logdev=$fsimg $SCRATCH_DEV
> -do_mkfs_fail -l lazy-count=1garbage $SCRATCH_DEV
> -do_mkfs_fail -l lazy-count=2 $SCRATCH_DEV
> -do_mkfs_fail -l lazy-count=0 -m crc=1 $SCRATCH_DEV
> -do_mkfs_fail -l version=1 -m crc=1 $SCRATCH_DEV
> -do_mkfs_fail -l version=0  $SCRATCH_DEV
> -
> -# naming section, should pass
> -do_mkfs_pass -n size=65536 $SCRATCH_DEV
> -do_mkfs_pass -n version=2 $SCRATCH_DEV
> -do_mkfs_pass -n version=ci $SCRATCH_DEV
> -do_mkfs_pass -n ftype=0 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -n ftype=1 $SCRATCH_DEV
> -
> -# naming section, should fail
> -do_mkfs_fail -n version=1 $SCRATCH_DEV
> -do_mkfs_fail -n version=cid $SCRATCH_DEV
> -do_mkfs_fail -n ftype=4 $SCRATCH_DEV
> -do_mkfs_fail -n ftype=0 $SCRATCH_DEV
> -do_mkfs_fail -n log=15 $SCRATCH_DE
> -
> -reset_fsimg
> -
> -# metadata section, should pass
> -do_mkfs_pass -m crc=1,finobt=1 $SCRATCH_DEV
> -do_mkfs_pass -m crc=1,finobt=0 $SCRATCH_DEV
> -do_mkfs_pass -m crc=0,finobt=0 $SCRATCH_DEV
> -do_mkfs_pass -m crc=1 -n ftype=1 $SCRATCH_DEV
> -do_mkfs_pass -m crc=0 -n ftype=1 $SCRATCH_DEV
> -do_mkfs_pass -m crc=0 -n ftype=0 $SCRATCH_DEV
> -
> -# metadata section, should fail
> -do_mkfs_fail -m crc=0,finobt=1 $SCRATCH_DEV
> -do_mkfs_fail -m crc=1 -n ftype=0 $SCRATCH_DEV
> -
> -# realtime section, results depend on reflink
> -_scratch_mkfs_xfs_supported -m reflink=0 >/dev/null 2>&1
> -if [ $? -eq 0 ]; then
> -	do_mkfs_pass -m reflink=0 -r rtdev=$fsimg $SCRATCH_DEV
> -	do_mkfs_pass -m reflink=0 -r size=65536,rtdev=$fsimg $SCRATCH_DEV
> -	do_mkfs_fail -m reflink=1 -r rtdev=$fsimg $SCRATCH_DEV
> -	do_mkfs_fail -m reflink=1 -r size=65536,rtdev=$fsimg $SCRATCH_DEV
> -else
> -	do_mkfs_pass -r rtdev=$fsimg $SCRATCH_DEV
> -	do_mkfs_pass -r size=65536,rtdev=$fsimg $SCRATCH_DEV
> -fi
> -
> -# realtime section, should pass
> -do_mkfs_pass -r extsize=4k $SCRATCH_DEV
> -do_mkfs_pass -r extsize=1G $SCRATCH_DEV
> -do_mkfs_pass -r noalign $SCRATCH_DEV
> -
> -# realtime section, should fail
> -do_mkfs_fail -r rtdev=$SCRATCH_DEV
> -do_mkfs_fail -r extsize=256 $SCRATCH_DEV
> -do_mkfs_fail -r extsize=2G $SCRATCH_DEV
> -do_mkfs_fail -r size=65536 $SCRATCH_DEV
> -
> -# inode section, should pass
> -do_mkfs_pass -i size=256 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -i size=512 $SCRATCH_DEV
> -do_mkfs_pass -i size=2048 $SCRATCH_DEV
> -do_mkfs_pass -i perblock=2 $SCRATCH_DEV
> -do_mkfs_pass -i maxpct=10 $SCRATCH_DEV
> -do_mkfs_pass -i maxpct=100 $SCRATCH_DEV
> -do_mkfs_pass -i maxpct=0 $SCRATCH_DEV
> -do_mkfs_pass -i align=0 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -i align=1 -m crc=1 $SCRATCH_DEV
> -do_mkfs_pass -i attr=1 -m crc=0 $SCRATCH_DEV
> -do_mkfs_pass -i attr=2 $SCRATCH_DEV
> -do_mkfs_pass -i projid32bit $SCRATCH_DEV
> -do_mkfs_pass -i sparse=0 $SCRATCH_DEV
> -do_mkfs_pass -i sparse -m crc $SCRATCH_DEV
> -
> -# inode section, should fail
> -do_mkfs_fail -i size=256 -m crc $SCRATCH_DEV
> -do_mkfs_fail -i size=128 $SCRATCH_DEV
> -do_mkfs_fail -i size=513 $SCRATCH_DEV
> -do_mkfs_fail -i size=4096 $SCRATCH_DEV
> -do_mkfs_fail -i maxpct=110 $SCRATCH_DEV
> -do_mkfs_fail -i align=2 $SCRATCH_DEV
> -do_mkfs_fail -i sparse -m crc=0 $SCRATCH_DEV
> -do_mkfs_fail -i align=0 -m crc=1 $SCRATCH_DEV
> -do_mkfs_fail -i attr=1 -m crc=1 $SCRATCH_DEV
> -do_mkfs_fail -i log=10 $SCRATCH_DEV
> -
> -status=0
> -exit
> diff --git a/tests/xfs/191-input-validation.out b/tests/xfs/191-input-validation.out
> deleted file mode 100644
> index 020bd625..00000000
> --- a/tests/xfs/191-input-validation.out
> +++ /dev/null
> @@ -1,2 +0,0 @@
> -QA output created by 191-input-validation
> -silence is golden
> -- 
> 2.35.1
> 

Re: [PATCH 05/12] README: document _begin_fstests better

[Darrick J. Wong]

On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Because how it actually gets used by the fstests infrastructure
> has been undocumented and that has impact on how it should be set
> up.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  README | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/README b/README
> index 7da66cb6..eacf1acd 100644
> --- a/README
> +++ b/README
> @@ -368,19 +368,42 @@ Test script environment:
>  
>       6. Test group membership: Each test can be associated with any number
>  	of groups for convenient selection of subsets of tests.  Group names
> -	can be any sequence of non-whitespace characters.  Test authors
> -	associate a test with groups by passing the names of those groups as
> -	arguments to the _begin_fstest function.  For example, the code:
> +	can be any sequence of non-whitespace characters, though human-readable
> +	names that match the set [A-Za-z0-9\-] are highly prefered.
>  
> -	_begin_fstest auto quick subvol snapshot
> +	Test authors associate a test with groups by passing the names of those
> +	groups as arguments to the _begin_fstest function. While _begin_fstests
> +	is a shell function that must be called at the start of a test to
> +	initialise the test environment correctly, the the build infrastructure
> +	also scans the test files for _begin_fstests invocations. It does this
> +	to compile the group lists that are used to determine which tests to run
> +	when `check` is executed. In other words, test files files must call
> +	_begin_fstest with their intended groups or they will not be run.
> +
> +	However, because the build infrastructure also uses _begin_fstests as
> +	a defined keyword, addition restrictions are placed on how it must be
> +	formatted:
> +
> +	(a) It must be a single line with no multi-line continuations.
> +
> +	(b) group names should be separated by spaces and not other whitespace
> +
> +	(c) A '#' placed anywhere in the list, even in the middle of a group
> +	    name, will cause everything from the # to the end of the line to be
> +	    ignored.

I don't see where this is implemented in mkgroupfile?  Was that in the
part of the patchset that got eaten by vger?  Or is this patch a
proposal for how we want to define _begin_fstest usage and will be
followed by changes to mkgroupfile to make it do what we now say it
does?

Also, under the old behavior, a '#' not preceded by whitespace or
otherwise escaped on the command line is considered to be part of an
argument:

$ echo moo#cow
moo#cow

Not that we /had/ any groups like that.

Also, I think we ought to add:

	(d) Group names may not contain whitespace or punctuation.

	(e) Quotation marks are considered a part of the group name.

> +
> +	For example, the code:
> +
> +	_begin_fstest auto quick subvol snapshot # metadata
>  
>  	associates the current test with the "auto", "quick", "subvol", and
> -	"snapshot" groups.  It is not necessary to specify the "all" group
> -	in the list because that group is computed at run time.
> +	"snapshot" groups. Because "metadata" is after the "#" comment
> +	delimiter, it is ignored by the build infrastructure and so it will not
> +	be associated with that group.
> +
> +	It is not necessary to specify the "all" group in the list because that
> +	group is always computed at run time from the group lists.

Otherwise, I'm happy with this.

--D

>  
> -	The build process scans test files for _begin_fstest invocations and
> -	compiles the group list from that information.  In other words, test
> -	files must call _begin_fstest or they will not be run.
>  
>  Verified output:
>  
> -- 
> 2.35.1
> 

Re: [PATCH 05/12] README: document _begin_fstests better

[Dave Chinner]

On Thu, May 19, 2022 at 04:13:56PM -0700, Darrick J. Wong wrote:
> On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Because how it actually gets used by the fstests infrastructure
> > has been undocumented and that has impact on how it should be set
> > up.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  README | 41 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> > diff --git a/README b/README
> > index 7da66cb6..eacf1acd 100644
> > --- a/README
> > +++ b/README
> > @@ -368,19 +368,42 @@ Test script environment:
> >  
> >       6. Test group membership: Each test can be associated with any number
> >  	of groups for convenient selection of subsets of tests.  Group names
> > -	can be any sequence of non-whitespace characters.  Test authors
> > -	associate a test with groups by passing the names of those groups as
> > -	arguments to the _begin_fstest function.  For example, the code:
> > +	can be any sequence of non-whitespace characters, though human-readable
> > +	names that match the set [A-Za-z0-9\-] are highly prefered.
> >  
> > -	_begin_fstest auto quick subvol snapshot
> > +	Test authors associate a test with groups by passing the names of those
> > +	groups as arguments to the _begin_fstest function. While _begin_fstests
> > +	is a shell function that must be called at the start of a test to
> > +	initialise the test environment correctly, the the build infrastructure
> > +	also scans the test files for _begin_fstests invocations. It does this
> > +	to compile the group lists that are used to determine which tests to run
> > +	when `check` is executed. In other words, test files files must call
> > +	_begin_fstest with their intended groups or they will not be run.
> > +
> > +	However, because the build infrastructure also uses _begin_fstests as
> > +	a defined keyword, addition restrictions are placed on how it must be
> > +	formatted:
> > +
> > +	(a) It must be a single line with no multi-line continuations.
> > +
> > +	(b) group names should be separated by spaces and not other whitespace
> > +
> > +	(c) A '#' placed anywhere in the list, even in the middle of a group
> > +	    name, will cause everything from the # to the end of the line to be
> > +	    ignored.
> 
> I don't see where this is implemented in mkgroupfile?

It doesn't need to be. It just aggregates the entire group line,
comments and all. Comments *must* be stripped by the thing that reads
the group file - mkgroupfile adds comments to every group file it
builds.

> Was that in the
> part of the patchset that got eaten by vger?  Or is this patch a
> proposal for how we want to define _begin_fstest usage and will be
> followed by changes to mkgroupfile to make it do what we now say it
> does?

It documents the behaviour the mkgroupfile parser currently expects.

> Also, under the old behavior, a '#' not preceded by whitespace or
> otherwise escaped on the command line is considered to be part of an
> argument:
> 
> $ echo moo#cow
> moo#cow

Yeah, but we don't need to support that sort of weird thing. The
original "Group names can be any sequence of non-whitespace
characters" requirement is just a can of worms.

> 
> Not that we /had/ any groups like that.
> 
> Also, I think we ought to add:
> 
> 	(d) Group names may not contain whitespace or punctuation.
> 
> 	(e) Quotation marks are considered a part of the group name.

The specification after I modified it reads:

	.... Group names
	can be any sequence of non-whitespace characters, though
	human-readable names that match the set [A-Za-z0-9\-] are highly
	prefered.

I'm happy to change that to something like:

	Group names are to be humand readable names from the
	character set defined by [:isalnum:\-_].

No quotation marks, nothing outside the above as a single line
whitespace separated list.

I want to get rid of the group files altogether - all they are used
for is being read by check to build an in memory list of all the
tests and groups. We can do that quickly and easily now, we don't
need to do it at build time anymore. The group dictionary checks can be
done at build time, but that can easily be done with a make file
rule and doesn't need the group files to be built.

Also, I want to apply the same approach "grep, collate, cull"
process to evaluating _requires rules when check starts. We evaluate
the same requires rules with the same results hundreds of times
during an auto run - we only need to run each rule once and cull the
tests that require unsupported things from the test list before we
start running tests...

> Otherwise, I'm happy with this.

Thanks!

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] README: document _begin_fstests better

[Darrick J. Wong]

On Fri, May 20, 2022 at 11:58:30AM +1000, Dave Chinner wrote:
> On Thu, May 19, 2022 at 04:13:56PM -0700, Darrick J. Wong wrote:
> > On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Because how it actually gets used by the fstests infrastructure
> > > has been undocumented and that has impact on how it should be set
> > > up.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > >  README | 41 ++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 32 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/README b/README
> > > index 7da66cb6..eacf1acd 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -368,19 +368,42 @@ Test script environment:
> > >  
> > >       6. Test group membership: Each test can be associated with any number
> > >  	of groups for convenient selection of subsets of tests.  Group names
> > > -	can be any sequence of non-whitespace characters.  Test authors
> > > -	associate a test with groups by passing the names of those groups as
> > > -	arguments to the _begin_fstest function.  For example, the code:
> > > +	can be any sequence of non-whitespace characters, though human-readable
> > > +	names that match the set [A-Za-z0-9\-] are highly prefered.
> > >  
> > > -	_begin_fstest auto quick subvol snapshot
> > > +	Test authors associate a test with groups by passing the names of those
> > > +	groups as arguments to the _begin_fstest function. While _begin_fstests
> > > +	is a shell function that must be called at the start of a test to
> > > +	initialise the test environment correctly, the the build infrastructure
> > > +	also scans the test files for _begin_fstests invocations. It does this
> > > +	to compile the group lists that are used to determine which tests to run
> > > +	when `check` is executed. In other words, test files files must call
> > > +	_begin_fstest with their intended groups or they will not be run.
> > > +
> > > +	However, because the build infrastructure also uses _begin_fstests as
> > > +	a defined keyword, addition restrictions are placed on how it must be
> > > +	formatted:
> > > +
> > > +	(a) It must be a single line with no multi-line continuations.
> > > +
> > > +	(b) group names should be separated by spaces and not other whitespace
> > > +
> > > +	(c) A '#' placed anywhere in the list, even in the middle of a group
> > > +	    name, will cause everything from the # to the end of the line to be
> > > +	    ignored.
> > 
> > I don't see where this is implemented in mkgroupfile?
> 
> It doesn't need to be. It just aggregates the entire group line,
> comments and all. Comments *must* be stripped by the thing that reads
> the group file - mkgroupfile adds comments to every group file it
> builds.
> 
> > Was that in the
> > part of the patchset that got eaten by vger?  Or is this patch a
> > proposal for how we want to define _begin_fstest usage and will be
> > followed by changes to mkgroupfile to make it do what we now say it
> > does?
> 
> It documents the behaviour the mkgroupfile parser currently expects.

Ok.

> > Also, under the old behavior, a '#' not preceded by whitespace or
> > otherwise escaped on the command line is considered to be part of an
> > argument:
> > 
> > $ echo moo#cow
> > moo#cow
> 
> Yeah, but we don't need to support that sort of weird thing. The
> original "Group names can be any sequence of non-whitespace
> characters" requirement is just a can of worms.
> 
> > 
> > Not that we /had/ any groups like that.
> > 
> > Also, I think we ought to add:
> > 
> > 	(d) Group names may not contain whitespace or punctuation.
> > 
> > 	(e) Quotation marks are considered a part of the group name.
> 
> The specification after I modified it reads:
> 
> 	.... Group names
> 	can be any sequence of non-whitespace characters, though
> 	human-readable names that match the set [A-Za-z0-9\-] are highly
> 	prefered.
> 
> I'm happy to change that to something like:
> 
> 	Group names are to be humand readable names from the
> 	character set defined by [:isalnum:\-_].
> 
> No quotation marks, nothing outside the above as a single line
> whitespace separated list.

Yes, please.  Fewer possible characters are a plus.

> I want to get rid of the group files altogether - all they are used
> for is being read by check to build an in memory list of all the
> tests and groups. We can do that quickly and easily now, we don't
> need to do it at build time anymore. The group dictionary checks can be
> done at build time, but that can easily be done with a make file
> rule and doesn't need the group files to be built.

<nod>

> Also, I want to apply the same approach "grep, collate, cull"
> process to evaluating _requires rules when check starts. We evaluate
> the same requires rules with the same results hundreds of times
> during an auto run - we only need to run each rule once and cull the
> tests that require unsupported things from the test list before we
> start running tests...
> 
> > Otherwise, I'm happy with this.

<nod> I'm looking forward to the next version.

--D

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

Re: [PATCH 05/12] README: document _begin_fstests better

[Zorro Lang]

On Thu, May 19, 2022 at 07:02:44PM -0700, Darrick J. Wong wrote:
> On Fri, May 20, 2022 at 11:58:30AM +1000, Dave Chinner wrote:
> > On Thu, May 19, 2022 at 04:13:56PM -0700, Darrick J. Wong wrote:
> > > On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> > > > From: Dave Chinner <dchinner@redhat.com>
> > > > 
> > > > Because how it actually gets used by the fstests infrastructure
> > > > has been undocumented and that has impact on how it should be set
> > > > up.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > >  README | 41 ++++++++++++++++++++++++++++++++---------
> > > >  1 file changed, 32 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/README b/README
> > > > index 7da66cb6..eacf1acd 100644
> > > > --- a/README
> > > > +++ b/README
> > > > @@ -368,19 +368,42 @@ Test script environment:
> > > >  
> > > >       6. Test group membership: Each test can be associated with any number
> > > >  	of groups for convenient selection of subsets of tests.  Group names
> > > > -	can be any sequence of non-whitespace characters.  Test authors
> > > > -	associate a test with groups by passing the names of those groups as
> > > > -	arguments to the _begin_fstest function.  For example, the code:
> > > > +	can be any sequence of non-whitespace characters, though human-readable
> > > > +	names that match the set [A-Za-z0-9\-] are highly prefered.
> > > >  
> > > > -	_begin_fstest auto quick subvol snapshot
> > > > +	Test authors associate a test with groups by passing the names of those
> > > > +	groups as arguments to the _begin_fstest function. While _begin_fstests
> > > > +	is a shell function that must be called at the start of a test to
> > > > +	initialise the test environment correctly, the the build infrastructure
> > > > +	also scans the test files for _begin_fstests invocations. It does this
> > > > +	to compile the group lists that are used to determine which tests to run
> > > > +	when `check` is executed. In other words, test files files must call
> > > > +	_begin_fstest with their intended groups or they will not be run.
> > > > +
> > > > +	However, because the build infrastructure also uses _begin_fstests as
> > > > +	a defined keyword, addition restrictions are placed on how it must be
> > > > +	formatted:
> > > > +
> > > > +	(a) It must be a single line with no multi-line continuations.
> > > > +
> > > > +	(b) group names should be separated by spaces and not other whitespace
> > > > +
> > > > +	(c) A '#' placed anywhere in the list, even in the middle of a group
> > > > +	    name, will cause everything from the # to the end of the line to be
> > > > +	    ignored.
> > > 
> > > I don't see where this is implemented in mkgroupfile?
> > 
> > It doesn't need to be. It just aggregates the entire group line,
> > comments and all. Comments *must* be stripped by the thing that reads
> > the group file - mkgroupfile adds comments to every group file it
> > builds.
> > 
> > > Was that in the
> > > part of the patchset that got eaten by vger?  Or is this patch a
> > > proposal for how we want to define _begin_fstest usage and will be
> > > followed by changes to mkgroupfile to make it do what we now say it
> > > does?
> > 
> > It documents the behaviour the mkgroupfile parser currently expects.
> 
> Ok.
> 
> > > Also, under the old behavior, a '#' not preceded by whitespace or
> > > otherwise escaped on the command line is considered to be part of an
> > > argument:
> > > 
> > > $ echo moo#cow
> > > moo#cow
> > 
> > Yeah, but we don't need to support that sort of weird thing. The
> > original "Group names can be any sequence of non-whitespace
> > characters" requirement is just a can of worms.
> > 
> > > 
> > > Not that we /had/ any groups like that.
> > > 
> > > Also, I think we ought to add:
> > > 
> > > 	(d) Group names may not contain whitespace or punctuation.
> > > 
> > > 	(e) Quotation marks are considered a part of the group name.
> > 
> > The specification after I modified it reads:
> > 
> > 	.... Group names
> > 	can be any sequence of non-whitespace characters, though
> > 	human-readable names that match the set [A-Za-z0-9\-] are highly
> > 	prefered.
> > 
> > I'm happy to change that to something like:
> > 
> > 	Group names are to be humand readable names from the
> > 	character set defined by [:isalnum:\-_].

As you haven't sent the next version, I might be a little picky :)
I'm not a regex expert, just tried to run 'grep -E', I think [[:alnum:]_-] might
be better. The 'isalnum' is a function name, the '-' better to be at the end,
or it might be treated as a hyphen. For example:

...
$ echo a1-B_ | egrep -o [[:alnum:]\-_]
grep: Invalid range end
$ echo a1-B_ | egrep -o '[[:alnum:]\-_]'
a
1
B
_
$ echo a1-B_ | egrep -o [[:alnum:]_-]
a
1
-
B
_
...

Thanks,
Zorro

> > 
> > No quotation marks, nothing outside the above as a single line
> > whitespace separated list.
> 
> Yes, please.  Fewer possible characters are a plus.
> 
> > I want to get rid of the group files altogether - all they are used
> > for is being read by check to build an in memory list of all the
> > tests and groups. We can do that quickly and easily now, we don't
> > need to do it at build time anymore. The group dictionary checks can be
> > done at build time, but that can easily be done with a make file
> > rule and doesn't need the group files to be built.
> 
> <nod>
> 
> > Also, I want to apply the same approach "grep, collate, cull"
> > process to evaluating _requires rules when check starts. We evaluate
> > the same requires rules with the same results hundreds of times
> > during an auto run - we only need to run each rule once and cull the
> > tests that require unsupported things from the test list before we
> > start running tests...
> > 
> > > Otherwise, I'm happy with this.
> 
> <nod> I'm looking forward to the next version.
> 
> --D
> 
> > Thanks!
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> 

Re: [PATCH 05/12] README: document _begin_fstests better

[Dave Chinner]

On Fri, May 20, 2022 at 01:23:22PM +0800, Zorro Lang wrote:
> On Thu, May 19, 2022 at 07:02:44PM -0700, Darrick J. Wong wrote:
> > On Fri, May 20, 2022 at 11:58:30AM +1000, Dave Chinner wrote:
> > > On Thu, May 19, 2022 at 04:13:56PM -0700, Darrick J. Wong wrote:
> > > > On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > 
> > > > > Because how it actually gets used by the fstests infrastructure
> > > > > has been undocumented and that has impact on how it should be set
> > > > > up.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > >  README | 41 ++++++++++++++++++++++++++++++++---------
> > > > >  1 file changed, 32 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/README b/README
> > > > > index 7da66cb6..eacf1acd 100644
> > > > > --- a/README
> > > > > +++ b/README
> > > > > @@ -368,19 +368,42 @@ Test script environment:
> > > > >  
> > > > >       6. Test group membership: Each test can be associated with any number
> > > > >  	of groups for convenient selection of subsets of tests.  Group names
> > > > > -	can be any sequence of non-whitespace characters.  Test authors
> > > > > -	associate a test with groups by passing the names of those groups as
> > > > > -	arguments to the _begin_fstest function.  For example, the code:
> > > > > +	can be any sequence of non-whitespace characters, though human-readable
> > > > > +	names that match the set [A-Za-z0-9\-] are highly prefered.
> > > > >  
> > > > > -	_begin_fstest auto quick subvol snapshot
> > > > > +	Test authors associate a test with groups by passing the names of those
> > > > > +	groups as arguments to the _begin_fstest function. While _begin_fstests
> > > > > +	is a shell function that must be called at the start of a test to
> > > > > +	initialise the test environment correctly, the the build infrastructure
> > > > > +	also scans the test files for _begin_fstests invocations. It does this
> > > > > +	to compile the group lists that are used to determine which tests to run
> > > > > +	when `check` is executed. In other words, test files files must call
> > > > > +	_begin_fstest with their intended groups or they will not be run.
> > > > > +
> > > > > +	However, because the build infrastructure also uses _begin_fstests as
> > > > > +	a defined keyword, addition restrictions are placed on how it must be
> > > > > +	formatted:
> > > > > +
> > > > > +	(a) It must be a single line with no multi-line continuations.
> > > > > +
> > > > > +	(b) group names should be separated by spaces and not other whitespace
> > > > > +
> > > > > +	(c) A '#' placed anywhere in the list, even in the middle of a group
> > > > > +	    name, will cause everything from the # to the end of the line to be
> > > > > +	    ignored.
> > > > 
> > > > I don't see where this is implemented in mkgroupfile?
> > > 
> > > It doesn't need to be. It just aggregates the entire group line,
> > > comments and all. Comments *must* be stripped by the thing that reads
> > > the group file - mkgroupfile adds comments to every group file it
> > > builds.
> > > 
> > > > Was that in the
> > > > part of the patchset that got eaten by vger?  Or is this patch a
> > > > proposal for how we want to define _begin_fstest usage and will be
> > > > followed by changes to mkgroupfile to make it do what we now say it
> > > > does?
> > > 
> > > It documents the behaviour the mkgroupfile parser currently expects.
> > 
> > Ok.
> > 
> > > > Also, under the old behavior, a '#' not preceded by whitespace or
> > > > otherwise escaped on the command line is considered to be part of an
> > > > argument:
> > > > 
> > > > $ echo moo#cow
> > > > moo#cow
> > > 
> > > Yeah, but we don't need to support that sort of weird thing. The
> > > original "Group names can be any sequence of non-whitespace
> > > characters" requirement is just a can of worms.
> > > 
> > > > 
> > > > Not that we /had/ any groups like that.
> > > > 
> > > > Also, I think we ought to add:
> > > > 
> > > > 	(d) Group names may not contain whitespace or punctuation.
> > > > 
> > > > 	(e) Quotation marks are considered a part of the group name.
> > > 
> > > The specification after I modified it reads:
> > > 
> > > 	.... Group names
> > > 	can be any sequence of non-whitespace characters, though
> > > 	human-readable names that match the set [A-Za-z0-9\-] are highly
> > > 	prefered.
> > > 
> > > I'm happy to change that to something like:
> > > 
> > > 	Group names are to be humand readable names from the
> > > 	character set defined by [:isalnum:\-_].
> 
> As you haven't sent the next version, I might be a little picky :)
> I'm not a regex expert, just tried to run 'grep -E', I think [[:alnum:]_-] might
> be better. The 'isalnum' is a function name, the '-' better to be at the end,
> or it might be treated as a hyphen. For example:

Sure, I wasn't trying to write a fully working regex - that takes
thought and this was just a brain dump to indicate roughly what I
meant. It looks like you understood it well enough. :)

I do intend to change the code to tested and correct regex, then
copy it across to the documentation....

FWIW, if you want to improve your regex game, you can get plenty
of practice here:

https://regexcrossword.com/

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] README: document _begin_fstests better

[Zorro Lang]

On Fri, May 20, 2022 at 03:42:36PM +1000, Dave Chinner wrote:
> On Fri, May 20, 2022 at 01:23:22PM +0800, Zorro Lang wrote:
> > On Thu, May 19, 2022 at 07:02:44PM -0700, Darrick J. Wong wrote:
> > > On Fri, May 20, 2022 at 11:58:30AM +1000, Dave Chinner wrote:
> > > > On Thu, May 19, 2022 at 04:13:56PM -0700, Darrick J. Wong wrote:
> > > > > On Tue, May 17, 2022 at 05:01:04PM +1000, Dave Chinner wrote:
> > > > > > From: Dave Chinner <dchinner@redhat.com>
> > > > > > 
> > > > > > Because how it actually gets used by the fstests infrastructure
> > > > > > has been undocumented and that has impact on how it should be set
> > > > > > up.
> > > > > > 
> > > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > > ---
> > > > > >  README | 41 ++++++++++++++++++++++++++++++++---------
> > > > > >  1 file changed, 32 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/README b/README
> > > > > > index 7da66cb6..eacf1acd 100644
> > > > > > --- a/README
> > > > > > +++ b/README
> > > > > > @@ -368,19 +368,42 @@ Test script environment:
> > > > > >  
> > > > > >       6. Test group membership: Each test can be associated with any number
> > > > > >  	of groups for convenient selection of subsets of tests.  Group names
> > > > > > -	can be any sequence of non-whitespace characters.  Test authors
> > > > > > -	associate a test with groups by passing the names of those groups as
> > > > > > -	arguments to the _begin_fstest function.  For example, the code:
> > > > > > +	can be any sequence of non-whitespace characters, though human-readable
> > > > > > +	names that match the set [A-Za-z0-9\-] are highly prefered.
> > > > > >  
> > > > > > -	_begin_fstest auto quick subvol snapshot
> > > > > > +	Test authors associate a test with groups by passing the names of those
> > > > > > +	groups as arguments to the _begin_fstest function. While _begin_fstests
> > > > > > +	is a shell function that must be called at the start of a test to
> > > > > > +	initialise the test environment correctly, the the build infrastructure
> > > > > > +	also scans the test files for _begin_fstests invocations. It does this
> > > > > > +	to compile the group lists that are used to determine which tests to run
> > > > > > +	when `check` is executed. In other words, test files files must call
> > > > > > +	_begin_fstest with their intended groups or they will not be run.
> > > > > > +
> > > > > > +	However, because the build infrastructure also uses _begin_fstests as
> > > > > > +	a defined keyword, addition restrictions are placed on how it must be
> > > > > > +	formatted:
> > > > > > +
> > > > > > +	(a) It must be a single line with no multi-line continuations.
> > > > > > +
> > > > > > +	(b) group names should be separated by spaces and not other whitespace
> > > > > > +
> > > > > > +	(c) A '#' placed anywhere in the list, even in the middle of a group
> > > > > > +	    name, will cause everything from the # to the end of the line to be
> > > > > > +	    ignored.
> > > > > 
> > > > > I don't see where this is implemented in mkgroupfile?
> > > > 
> > > > It doesn't need to be. It just aggregates the entire group line,
> > > > comments and all. Comments *must* be stripped by the thing that reads
> > > > the group file - mkgroupfile adds comments to every group file it
> > > > builds.
> > > > 
> > > > > Was that in the
> > > > > part of the patchset that got eaten by vger?  Or is this patch a
> > > > > proposal for how we want to define _begin_fstest usage and will be
> > > > > followed by changes to mkgroupfile to make it do what we now say it
> > > > > does?
> > > > 
> > > > It documents the behaviour the mkgroupfile parser currently expects.
> > > 
> > > Ok.
> > > 
> > > > > Also, under the old behavior, a '#' not preceded by whitespace or
> > > > > otherwise escaped on the command line is considered to be part of an
> > > > > argument:
> > > > > 
> > > > > $ echo moo#cow
> > > > > moo#cow
> > > > 
> > > > Yeah, but we don't need to support that sort of weird thing. The
> > > > original "Group names can be any sequence of non-whitespace
> > > > characters" requirement is just a can of worms.
> > > > 
> > > > > 
> > > > > Not that we /had/ any groups like that.
> > > > > 
> > > > > Also, I think we ought to add:
> > > > > 
> > > > > 	(d) Group names may not contain whitespace or punctuation.
> > > > > 
> > > > > 	(e) Quotation marks are considered a part of the group name.
> > > > 
> > > > The specification after I modified it reads:
> > > > 
> > > > 	.... Group names
> > > > 	can be any sequence of non-whitespace characters, though
> > > > 	human-readable names that match the set [A-Za-z0-9\-] are highly
> > > > 	prefered.
> > > > 
> > > > I'm happy to change that to something like:
> > > > 
> > > > 	Group names are to be humand readable names from the
> > > > 	character set defined by [:isalnum:\-_].
> > 
> > As you haven't sent the next version, I might be a little picky :)
> > I'm not a regex expert, just tried to run 'grep -E', I think [[:alnum:]_-] might
> > be better. The 'isalnum' is a function name, the '-' better to be at the end,
> > or it might be treated as a hyphen. For example:
> 
> Sure, I wasn't trying to write a fully working regex - that takes
> thought and this was just a brain dump to indicate roughly what I
> meant. It looks like you understood it well enough. :)
> 
> I do intend to change the code to tested and correct regex, then
> copy it across to the documentation....

I'm pretty sure you're better at it than me, I just did that to save some of
your time :)

> 
> FWIW, if you want to improve your regex game, you can get plenty
> of practice here:
> 
> https://regexcrossword.com/

Thanks! That looks interesting. Regex things are still 'black magic' for me
sometimes. I have to google and give it enough test to make sure it works
each time.

Thanks,
Zorro

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

Re: [PATCH 07/12] xfs/148: fix failure from bad shortform size assumptions

[Zorro Lang]

On Tue, May 17, 2022 at 05:01:06PM +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.
> 
> While there, clean up the silly conditional loop device cleanup
> code.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  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

It's good to me, although I'm wondering why you didn't choose to use
$SELINUX_MOUNT_OPTIONS [1]. Anyway, extending inode size can help too.

Reviewed-by: Zorro Lang <zlang@redhat.com>

[1]
-_mount $loopdev $mntpt
+_mount $loopdev $mntpt $SELINUX_MOUNT_OPTIONS

>  
>  # 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 04/12] fstests: fix group list generation for whacky test names

[Zorro Lang]

On Tue, May 17, 2022 at 05:01:03PM +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
> $
> 
> Use the regexes for matching test names defined in common/test_names
> rather than trying to open code it.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

Hi Dave,

After testing, looks like this patch brings in a regression issue, it causes
case number aren't sorted in group.list, then the xfstests/new program can't
get a right new case number which is unused. For example:

# make
# ./new generic
Next test id is 019
Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
# ls tests/generic/019
tests/generic/019

Then comparing the tests/generic/group.list with old generic/group.list, found
lots of difference, due to the number in new group.list isn't sorted from
small to big (I haven't gotten chance to check if there're missing number).

Thanks,
Zorro

>  tools/mkgroupfile | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> index 24435898..414cb538 100755
> --- a/tools/mkgroupfile
> +++ b/tools/mkgroupfile
> @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
>  	exit 1
>  fi
>  
> +. ../../common/test_names
> +
>  cleanup()
>  {
>  	rm -f $new_groups.check
> @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_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 04/12] fstests: fix group list generation for whacky test names

[Zorro Lang]

On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> On Tue, May 17, 2022 at 05:01:03PM +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
> > $
> > 
> > Use the regexes for matching test names defined in common/test_names
> > rather than trying to open code it.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> 
> Hi Dave,
> 
> After testing, looks like this patch brings in a regression issue, it causes
> case number aren't sorted in group.list, then the xfstests/new program can't
> get a right new case number which is unused. For example:

Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
look into what's wrong with that.

Thanks,
Zorro

> 
> # make
> # ./new generic
> Next test id is 019
> Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> # ls tests/generic/019
> tests/generic/019
> 
> Then comparing the tests/generic/group.list with old generic/group.list, found
> lots of difference, due to the number in new group.list isn't sorted from
> small to big (I haven't gotten chance to check if there're missing number).
> 
> Thanks,
> Zorro
> 
> >  tools/mkgroupfile | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > index 24435898..414cb538 100755
> > --- a/tools/mkgroupfile
> > +++ b/tools/mkgroupfile
> > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> >  	exit 1
> >  fi
> >  
> > +. ../../common/test_names
> > +
> >  cleanup()
> >  {
> >  	rm -f $new_groups.check
> > @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_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 04/12] fstests: fix group list generation for whacky test names

[Zorro Lang]

On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > On Tue, May 17, 2022 at 05:01:03PM +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
> > > $
> > > 
> > > Use the regexes for matching test names defined in common/test_names
> > > rather than trying to open code it.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > ---
> > 
> > Hi Dave,
> > 
> > After testing, looks like this patch brings in a regression issue, it causes
> > case number aren't sorted in group.list, then the xfstests/new program can't
> > get a right new case number which is unused. For example:
> 
> Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> look into what's wrong with that.
> 
> Thanks,
> Zorro
> 
> > 
> > # make
> > # ./new generic
> > Next test id is 019
> > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > # ls tests/generic/019
> > tests/generic/019
> > 
> > Then comparing the tests/generic/group.list with old generic/group.list, found
> > lots of difference, due to the number in new group.list isn't sorted from
> > small to big (I haven't gotten chance to check if there're missing number).
> > 
> > Thanks,
> > Zorro
> > 
> > >  tools/mkgroupfile | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > index 24435898..414cb538 100755
> > > --- a/tools/mkgroupfile
> > > +++ b/tools/mkgroupfile
> > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > >  	exit 1
> > >  fi
> > >  
> > > +. ../../common/test_names
> > > +
> > >  cleanup()
> > >  {
> > >  	rm -f $new_groups.check
> > > @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > +		>> $new_groups

I think add a "sort -ug" [1] will help to fix this problem. I'm wondering
why we use ">> $new_groups" at here, why an appending write is needed. I think
there's not a 'loop running' for this code, right? If there's, please correct
me, then the 'sort -ug' have to be moved to other place.

Thanks,
Zorro

[1]
-               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
-               >> $new_groups
+               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
+               sort -ug > $new_groups

I'm wondering why we use

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

Re: [PATCH 04/12] fstests: fix group list generation for whacky test names

[Darrick J. Wong]

On Fri, May 20, 2022 at 05:25:15PM +0800, Zorro Lang wrote:
> On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> > On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > > On Tue, May 17, 2022 at 05:01:03PM +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
> > > > $
> > > > 
> > > > Use the regexes for matching test names defined in common/test_names
> > > > rather than trying to open code it.
> > > > 
> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > ---
> > > 
> > > Hi Dave,
> > > 
> > > After testing, looks like this patch brings in a regression issue, it causes
> > > case number aren't sorted in group.list, then the xfstests/new program can't
> > > get a right new case number which is unused. For example:
> > 
> > Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> > file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> > look into what's wrong with that.
> > 
> > Thanks,
> > Zorro
> > 
> > > 
> > > # make
> > > # ./new generic
> > > Next test id is 019
> > > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > > # ls tests/generic/019
> > > tests/generic/019
> > > 
> > > Then comparing the tests/generic/group.list with old generic/group.list, found
> > > lots of difference, due to the number in new group.list isn't sorted from
> > > small to big (I haven't gotten chance to check if there're missing number).
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > >  tools/mkgroupfile | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > index 24435898..414cb538 100755
> > > > --- a/tools/mkgroupfile
> > > > +++ b/tools/mkgroupfile
> > > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > > >  	exit 1
> > > >  fi
> > > >  
> > > > +. ../../common/test_names
> > > > +
> > > >  cleanup()
> > > >  {
> > > >  	rm -f $new_groups.check
> > > > @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > > +		>> $new_groups
> 
> I think add a "sort -ug" [1] will help to fix this problem.

Yes please, it's much easier for us humans to index the file if it's
already sorted numerically.

> I'm wondering
> why we use ">> $new_groups" at here, why an appending write is needed. I think
> there's not a 'loop running' for this code, right? If there's, please correct
> me, then the 'sort -ug' have to be moved to other place.

The function writes a header into the group list file warning readers
that it's an autogenerated file.

--D

> Thanks,
> Zorro
> 
> [1]
> -               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> -               >> $new_groups
> +               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
> +               sort -ug > $new_groups
> 
> I'm wondering why we use
> 
> > > >  
> > > >  	# Create the list of unique groups for existence checking
> > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > -- 
> > > > 2.35.1
> > > > 
> 

Re: [PATCH 04/12] fstests: fix group list generation for whacky test names

[Zorro Lang]

On Fri, May 20, 2022 at 09:23:10AM -0700, Darrick J. Wong wrote:
> On Fri, May 20, 2022 at 05:25:15PM +0800, Zorro Lang wrote:
> > On Fri, May 20, 2022 at 04:54:45PM +0800, Zorro Lang wrote:
> > > On Fri, May 20, 2022 at 04:36:47PM +0800, Zorro Lang wrote:
> > > > On Tue, May 17, 2022 at 05:01:03PM +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
> > > > > $
> > > > > 
> > > > > Use the regexes for matching test names defined in common/test_names
> > > > > rather than trying to open code it.
> > > > > 
> > > > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > > > > ---
> > > > 
> > > > Hi Dave,
> > > > 
> > > > After testing, looks like this patch brings in a regression issue, it causes
> > > > case number aren't sorted in group.list, then the xfstests/new program can't
> > > > get a right new case number which is unused. For example:
> > > 
> > > Oh, this issue isn't from this patch, it's from 441606d2 ("fstests: faster group
> > > file creation"). Hmm... I saw your used "sort -u" in that patch, I'm going to
> > > look into what's wrong with that.
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > > > 
> > > > # make
> > > > # ./new generic
> > > > Next test id is 019
> > > > Append a name to the ID? Test name will be 019-$name. y,[n]: ^C
> > > > # ls tests/generic/019
> > > > tests/generic/019
> > > > 
> > > > Then comparing the tests/generic/group.list with old generic/group.list, found
> > > > lots of difference, due to the number in new group.list isn't sorted from
> > > > small to big (I haven't gotten chance to check if there're missing number).
> > > > 
> > > > Thanks,
> > > > Zorro
> > > > 
> > > > >  tools/mkgroupfile | 5 ++++-
> > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/tools/mkgroupfile b/tools/mkgroupfile
> > > > > index 24435898..414cb538 100755
> > > > > --- a/tools/mkgroupfile
> > > > > +++ b/tools/mkgroupfile
> > > > > @@ -19,6 +19,8 @@ if [ ! -x ../../check ]; then
> > > > >  	exit 1
> > > > >  fi
> > > > >  
> > > > > +. ../../common/test_names
> > > > > +
> > > > >  cleanup()
> > > > >  {
> > > > >  	rm -f $new_groups.check
> > > > > @@ -60,7 +62,8 @@ 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/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > > > > +		>> $new_groups
> > 
> > I think add a "sort -ug" [1] will help to fix this problem.
> 
> Yes please, it's much easier for us humans to index the file if it's
> already sorted numerically.
> 
> > I'm wondering
> > why we use ">> $new_groups" at here, why an appending write is needed. I think
> > there's not a 'loop running' for this code, right? If there's, please correct
> > me, then the 'sort -ug' have to be moved to other place.
> 
> The function writes a header into the group list file warning readers
> that it's an autogenerated file.

Oh, you're right, there's a comment as a header, so we should change as below:

-               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
-               >> $new_groups
+               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
+               sort -ug >> $new_groups

Hi Dave, I'd like to fix this issue in this week. If you'd like to fix it
in this patch today, I'll wait half day. Or as it's a separated issue, I
can write another patch to get review, then merge with this patchset together.
Until now, I've merged below patches from this patchset:

38015b3d fstests: remove xfs deprecated test
59f840bf xfs/191: remove broken test
1de1ff66 xfs/148: fix failure from bad shortform size assumptions
df84ba04 xfs/148: make test debuggable
24ac437f xfs/348: golden output is not correct
e3a4d0e8 xfs/122: add attribute log formats to test output.
e121de4a fstests: filter quota warnings

Thanks,
Zorro

> 
> --D
> 
> > Thanks,
> > Zorro
> > 
> > [1]
> > -               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" \
> > -               >> $new_groups
> > +               sed -e "s/^.*\/\($VALID_TEST_NAME\):_begin_fstest/\1/" | \
> > +               sort -ug > $new_groups
> > 
> > I'm wondering why we use
> > 
> > > > >  
> > > > >  	# Create the list of unique groups for existence checking
> > > > >  	grep -I -R "^_begin_fstest" $test_dir/ | \
> > > > > -- 
> > > > > 2.35.1
> > > > > 
> > 
> 

Re: [PATCH 07/12] xfs/148: fix failure from bad shortform size assumptions

[Dave Chinner]

On Fri, May 20, 2022 at 03:34:38PM +0800, Zorro Lang wrote:
> On Tue, May 17, 2022 at 05:01:06PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > @@ -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
> 
> It's good to me, although I'm wondering why you didn't choose to use
> $SELINUX_MOUNT_OPTIONS [1]. Anyway, extending inode size can help too.

Because if we are running under any other context that adds xattrs
to the files by defaults (e.g. default acls, other LSMs, etc) will
cause it to fail the same way.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com