All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] Annonate fstests with possible reasons for failure
@ 2022-04-17 17:40 Amir Goldstein
  2022-04-17 17:40 ` [PATCH 1/3] common: support black listing fs in _supported_fs() Amir Goldstein
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Amir Goldstein @ 2022-04-17 17:40 UTC (permalink / raw)
  To: Eryu Guan, Zorro Lang; +Cc: Darrick J . Wong, Luis Chamberlain, fstests

In a perfect worlds, tests are expected to pass, but we do not live in a
perfect world.

In reality, fstests are often merged before a fix is merged to Linus'
tree or before a fix to all fs is available [1][2].

We all know that day when we pull fstests upstream branch and get a
bunch of new test failures.

This series [3] adds several new ways to annotate tests to provide some
hints for testers about which tests are expected to pass on which fs
and or which kernel versions:

 _fixed_by_kernel_commit
 _known_issue_before_kernel
 _known_issue_on_fs

And a new run option SKIP_KNOWN_ISSUES=yes.

These annotation and run option are inspired by a similar feature in LTP.

These annotation could be used by testers to auto-generate expunge list
for testing stable kernels from the information in _fixed_by_kernel_commit
and _known_issue_before_kernel. This trivial task is left is an excercise.

Thanks,
Amir.

[1] https://lore.kernel.org/fstests/20220412172853.GG16799@magnolia/
[2] https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
[3] https://github.com/amir73il/xfstests/commits/hints

Amir Goldstein (3):
  common: support black listing fs in _supported_fs()
  common: print hints for reasons of test failures
  common: add run option to skip tests for known issues

 README            |  2 ++
 check             | 10 ++++++++-
 common/preamble   |  2 +-
 common/rc         | 54 ++++++++++++++++++++++++++++++++++++++++-------
 tests/generic/500 |  3 +--
 tests/generic/631 |  3 +--
 tests/generic/679 |  3 +--
 tests/overlay/009 |  3 +++
 tests/overlay/010 |  3 +++
 tests/overlay/014 |  2 ++
 tests/overlay/016 |  4 +++-
 tests/overlay/017 |  4 +++-
 tests/overlay/018 |  4 +++-
 tests/overlay/020 |  3 +++
 tests/overlay/022 |  2 ++
 tests/overlay/029 |  2 ++
 tests/overlay/038 |  2 ++
 tests/overlay/041 |  2 ++
 tests/overlay/042 |  3 +++
 tests/overlay/043 |  4 +++-
 tests/overlay/044 |  4 +++-
 tests/overlay/054 |  3 +++
 tests/overlay/055 |  3 +++
 tests/overlay/061 |  2 ++
 tests/overlay/063 |  3 +++
 tests/overlay/065 |  4 ++++
 tests/overlay/067 |  3 +++
 tests/overlay/070 |  4 +++-
 tests/overlay/071 |  4 +++-
 tests/overlay/072 |  2 ++
 tests/overlay/074 |  5 +++++
 tests/overlay/077 |  5 +++++
 tests/overlay/078 |  1 +
 33 files changed, 135 insertions(+), 23 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 13+ messages in thread
* [PATCH 1/3] common: support black listing fs in _supported_fs()
  2022-04-17 17:40 [PATCH 0/3] Annonate fstests with possible reasons for failure Amir Goldstein
@ 2022-04-17 17:40 ` Amir Goldstein
  2022-04-18  4:33   ` Zorro Lang
  2022-04-17 17:40 ` [PATCH 2/3] common: print hints for reasons of test failures Amir Goldstein
  2022-04-17 17:40 ` [PATCH 3/3] common: add run option to skip tests for known issues Amir Goldstein
  2 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2022-04-17 17:40 UTC (permalink / raw)
  To: Eryu Guan, Zorro Lang; +Cc: Darrick J . Wong, Luis Chamberlain, fstests

For example:
_supported_fs ^xfs

There is no need to specify "generic" when using a block list
"all other fs are supported" is implied.

Converted some generic tests that open code this condition without
emitting a meaningful reason.

More generic test like generic/186,187 could use a block list, but
were not converted because they emit some meaningful reason:
  _notrun "Can't fragment free space on btrfs."

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 common/rc         | 30 ++++++++++++++++++++----------
 tests/generic/500 |  3 +--
 tests/generic/631 |  3 +--
 tests/generic/679 |  3 +--
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/common/rc b/common/rc
index 17629801..1d37dcbd 100644
--- a/common/rc
+++ b/common/rc
@@ -1572,19 +1572,29 @@ _fail()
 
 # tests whether $FSTYP is one of the supported filesystems for a test
 #
-_supported_fs()
+_check_supported_fs()
 {
-    local f
+	local res=1
+	local f
 
-    for f
-    do
-	if [ "$f" = "$FSTYP" -o "$f" = "generic" ]
-	then
-	    return
-	fi
-    done
+	for f; do
+		# ^FS means black listed fs
+		if [ "$f" = "^$FSTYP" ]; then
+			return 1
+		elif [ "$f" = "generic" ] || [[ "$f" == "^"* ]]; then
+			# ^FS implies "generic ^FS"
+			res=0
+		elif [ "$f" = "$FSTYP" ]; then
+			return 0
+		fi
+	done
+	return $res
+}
 
-    _notrun "not suitable for this filesystem type: $FSTYP"
+_supported_fs()
+{
+	_check_supported_fs $* || \
+		_notrun "not suitable for this filesystem type: $FSTYP"
 }
 
 # check if a FS on a device is mounted
diff --git a/tests/generic/500 b/tests/generic/500
index 0be59934..bc84d219 100755
--- a/tests/generic/500
+++ b/tests/generic/500
@@ -35,7 +35,6 @@ _cleanup()
 . ./common/dmthin
 
 # real QA test starts here
-_supported_fs generic
 _require_scratch_nocheck
 _require_dm_target thin-pool
 
@@ -43,7 +42,7 @@ _require_dm_target thin-pool
 # and since we've filled the thinp device it'll return EIO, which will make
 # btrfs flip read only, making it fail this test when it just won't work right
 # for us in the first place.
-test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"
+_supported_fs ^btrfs
 
 # Require underlying device support discard
 _scratch_mkfs >>$seqres.full 2>&1
diff --git a/tests/generic/631 b/tests/generic/631
index 4996bce7..219f7a05 100755
--- a/tests/generic/631
+++ b/tests/generic/631
@@ -36,10 +36,9 @@ _cleanup()
 . ./common/attr
 
 # real QA test starts here
-_supported_fs generic
 _require_scratch
 _require_attrs trusted
-test "$FSTYP" = "overlay" && _notrun "Test does not apply to overlayfs."
+_supported_fs ^overlay
 _require_extra_fs overlay
 
 _scratch_mkfs >> $seqres.full
diff --git a/tests/generic/679 b/tests/generic/679
index 440f0c08..c32d42b9 100755
--- a/tests/generic/679
+++ b/tests/generic/679
@@ -17,7 +17,6 @@ _begin_fstest auto quick prealloc
 
 # real QA test starts here
 
-_supported_fs generic
 _require_scratch
 _require_xfs_io_command "falloc"
 _require_xfs_io_command "fiemap"
@@ -26,7 +25,7 @@ _require_xfs_io_command "fiemap"
 #
 #   https://lore.kernel.org/linux-btrfs/20220315164011.GF8241@magnolia/
 #
-[ $FSTYP == "xfs" ] && _notrun "test not valid for xfs"
+_supported_fs ^xfs
 
 rm -f $seqres.full
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 2/3] common: print hints for reasons of test failures
  2022-04-17 17:40 [PATCH 0/3] Annonate fstests with possible reasons for failure Amir Goldstein
  2022-04-17 17:40 ` [PATCH 1/3] common: support black listing fs in _supported_fs() Amir Goldstein
@ 2022-04-17 17:40 ` Amir Goldstein
  2022-04-18  4:52   ` Zorro Lang
  2022-04-17 17:40 ` [PATCH 3/3] common: add run option to skip tests for known issues Amir Goldstein
  2 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2022-04-17 17:40 UTC (permalink / raw)
  To: Eryu Guan, Zorro Lang; +Cc: Darrick J . Wong, Luis Chamberlain, fstests

Introduce helpers _known_issue_before_kernel() and
_fixed_by_kernel_commit() that can be used to hint testers why a test
might be failing and aid in auto-generating of expunge lists for
downstream kernel testing.

Annotate fix kernel commits for some overlayfs tests and fix kernel
version for some overlayfs tests testing for legacy behavior whose
fixes are not likely to be backported to stable kernels.

This is modeled after LTP's 'make filter-known-fails' and
print_failure_hints() using struct tst_tag annotations.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 check             | 10 +++++++++-
 common/preamble   |  2 +-
 common/rc         | 14 ++++++++++++++
 tests/overlay/009 |  3 +++
 tests/overlay/010 |  3 +++
 tests/overlay/014 |  2 ++
 tests/overlay/016 |  4 +++-
 tests/overlay/017 |  4 +++-
 tests/overlay/018 |  4 +++-
 tests/overlay/020 |  3 +++
 tests/overlay/022 |  2 ++
 tests/overlay/029 |  2 ++
 tests/overlay/038 |  2 ++
 tests/overlay/041 |  2 ++
 tests/overlay/042 |  3 +++
 tests/overlay/043 |  4 +++-
 tests/overlay/044 |  4 +++-
 tests/overlay/054 |  3 +++
 tests/overlay/055 |  3 +++
 tests/overlay/063 |  3 +++
 tests/overlay/065 |  4 ++++
 tests/overlay/067 |  3 +++
 tests/overlay/070 |  4 +++-
 tests/overlay/071 |  4 +++-
 tests/overlay/072 |  2 ++
 tests/overlay/074 |  5 +++++
 tests/overlay/077 |  5 +++++
 tests/overlay/078 |  1 +
 28 files changed, 96 insertions(+), 9 deletions(-)

diff --git a/check b/check
index a0863121..de11b37e 100755
--- a/check
+++ b/check
@@ -808,7 +808,7 @@ function run_section()
 		fi
 
 		# really going to try and run this one
-		rm -f $seqres.out.bad
+		rm -f $seqres.out.bad $seqres.hints
 
 		# check if we really should run it
 		_expunge_test $seqnum
@@ -942,6 +942,14 @@ function run_section()
 			fi; } | sed -e 's/^\(.\)/    \1/'
 			err=true
 		fi
+		if [ -f $seqres.hints ]; then
+			if $err; then
+				echo
+				cat $seqres.hints
+			else
+				rm -f $seqres.hints
+			fi
+		fi
 	done
 
 	# make sure we record the status of the last test we ran.
diff --git a/common/preamble b/common/preamble
index 64d79385..68219660 100644
--- a/common/preamble
+++ b/common/preamble
@@ -79,6 +79,6 @@ _begin_fstest()
 	. ./common/rc
 
 	# remove previous $seqres.full before test
-	rm -f $seqres.full
+	rm -f $seqres.full $seqres.hints
 
 }
diff --git a/common/rc b/common/rc
index 1d37dcbd..2e9dc408 100644
--- a/common/rc
+++ b/common/rc
@@ -1597,6 +1597,19 @@ _supported_fs()
 		_notrun "not suitable for this filesystem type: $FSTYP"
 }
 
+_known_issue_before_kernel()
+{
+	echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
+	echo >> $seqres.hints
+}
+
+_fixed_by_kernel_commit()
+{
+	echo "HINT: You _MAY_ be missing kernel fix:" >> $seqres.hints
+	echo "      $*" >> $seqres.hints
+	echo >> $seqres.hints
+}
+
 # check if a FS on a device is mounted
 # if so, verify that it is mounted on mount point
 # if fstype is given as argument, verify that it is also
@@ -4916,6 +4929,7 @@ _require_kernel_config()
 	_has_kernel_config $1 || _notrun "Installed kernel not built with $1"
 }
 
+
 init_rc
 
 ################################################################################
diff --git a/tests/overlay/009 b/tests/overlay/009
index 94bd1b66..d85ef16e 100755
--- a/tests/overlay/009
+++ b/tests/overlay/009
@@ -17,6 +17,9 @@ _begin_fstest auto quick
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit a4859d75944a \
+	"ovl: fix dentry leak for default_permissions"
+
 _require_scratch
 
 # Remove all files from previous tests
diff --git a/tests/overlay/010 b/tests/overlay/010
index 71ef6ec1..af22b2b4 100755
--- a/tests/overlay/010
+++ b/tests/overlay/010
@@ -17,6 +17,9 @@ _begin_fstest auto quick whiteout
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 84889d493356 \
+	"ovl: check dentry positiveness in ovl_cleanup_whiteouts()"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/014 b/tests/overlay/014
index 83295ccc..2d6c11d9 100755
--- a/tests/overlay/014
+++ b/tests/overlay/014
@@ -21,6 +21,8 @@ _begin_fstest auto quick copyup
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 0956254a2d5b "ovl: don't copy up opaqueness"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/016 b/tests/overlay/016
index 8a091615..d160ff49 100755
--- a/tests/overlay/016
+++ b/tests/overlay/016
@@ -6,7 +6,7 @@
 #
 # Test ro/rw fd data inconsistecies
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.19:
 # - process A opens file F for read
 # - process B writes new data to file F
 # - process A reads old data from file F
@@ -19,6 +19,8 @@ _begin_fstest auto quick copyup
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.19"
+
 _require_scratch
 _require_xfs_io_command "open"
 
diff --git a/tests/overlay/017 b/tests/overlay/017
index 15b0d613..00ca126c 100755
--- a/tests/overlay/017
+++ b/tests/overlay/017
@@ -6,7 +6,7 @@
 #
 # Test constant inode numbers
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.14:
 # - stat file A shows inode number X
 # - modify A to trigger copy up
 # - stat file A shows inode number Y != X
@@ -23,6 +23,8 @@ _begin_fstest auto quick copyup redirect
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.14"
+
 _require_scratch
 _require_test_program "af_unix"
 _require_test_program "t_dir_type"
diff --git a/tests/overlay/018 b/tests/overlay/018
index b09bca9c..0d1cbfa5 100755
--- a/tests/overlay/018
+++ b/tests/overlay/018
@@ -6,7 +6,7 @@
 #
 # Test hardlink breakage
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.13:
 # - file A and B are hardlinked in lower
 # - modify A to trigger copy up
 # - file A is no longer a hardlink of file B
@@ -19,6 +19,8 @@ _begin_fstest auto quick copyup hardlink
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.13"
+
 _require_scratch
 _require_scratch_feature index
 _require_test_program "t_dir_type"
diff --git a/tests/overlay/020 b/tests/overlay/020
index 51f97ccd..98a33aec 100755
--- a/tests/overlay/020
+++ b/tests/overlay/020
@@ -23,6 +23,9 @@ require_unshare() {
 
 # Modify as appropriate.
 _supported_fs overlay
+_fixed_by_kernel_commit 3fe6e52f0626 \
+	"ovl: override creds with the ones from the superblock mounter"
+
 _require_scratch
 require_unshare -m -p -U
 
diff --git a/tests/overlay/022 b/tests/overlay/022
index 1a11805d..09af6500 100755
--- a/tests/overlay/022
+++ b/tests/overlay/022
@@ -29,6 +29,8 @@ _cleanup()
 
 # Modify as appropriate.
 _supported_fs overlay
+_fixed_by_kernel_commit 76bc8e2843b6 "ovl: disallow overlayfs as upperdir"
+
 _require_scratch
 
 # Remove all files from previous tests
diff --git a/tests/overlay/029 b/tests/overlay/029
index 1a532c2a..c4c8eed7 100755
--- a/tests/overlay/029
+++ b/tests/overlay/029
@@ -34,6 +34,8 @@ _cleanup()
 
 # Modify as appropriate.
 _supported_fs overlay
+_fixed_by_kernel_commit c4fcfc1619ea "ovl: fix d_real() for stacked fs"
+
 _require_scratch
 
 # Remove all files from previous tests
diff --git a/tests/overlay/038 b/tests/overlay/038
index 145b4b34..7bde49ee 100755
--- a/tests/overlay/038
+++ b/tests/overlay/038
@@ -15,6 +15,8 @@ _begin_fstest auto quick copyup
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.14"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/041 b/tests/overlay/041
index 762e7389..084aaad6 100755
--- a/tests/overlay/041
+++ b/tests/overlay/041
@@ -17,6 +17,8 @@ _begin_fstest auto quick copyup nonsamefs
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.17"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/042 b/tests/overlay/042
index 60f4b477..0715066f 100755
--- a/tests/overlay/042
+++ b/tests/overlay/042
@@ -26,6 +26,9 @@ _begin_fstest auto quick copyup hardlink
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 6eaf011144af \
+	"ovl: fix EIO from lookup of non-indexed upper"
+
 _require_scratch
 # Without overlay index feature hardlinks are broken on copy up
 _require_scratch_feature index
diff --git a/tests/overlay/043 b/tests/overlay/043
index 383151dd..e6510ae5 100755
--- a/tests/overlay/043
+++ b/tests/overlay/043
@@ -8,7 +8,7 @@
 # This is a variant of overlay/017 to test constant st_ino numbers for
 # non-samefs setup.
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
 # - stat file A shows inode number X
 # - modify A to trigger copy up
 # - stat file A shows inode number Y != X
@@ -25,6 +25,8 @@ _begin_fstest auto quick copyup nonsamefs
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.17"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/044 b/tests/overlay/044
index 3f54b7ea..b11f4359 100755
--- a/tests/overlay/044
+++ b/tests/overlay/044
@@ -7,7 +7,7 @@
 # Test hardlink breakage on non-samefs setup
 # This is a variant of overlay/018 to test.
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
 # - file A and B are hardlinked in lower
 # - modify A to trigger copy up
 # - file A is no longer a hardlink of file B
@@ -20,6 +20,8 @@ _begin_fstest auto quick copyup hardlink nonsamefs
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.17"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/054 b/tests/overlay/054
index d95427b6..ba20a7fc 100755
--- a/tests/overlay/054
+++ b/tests/overlay/054
@@ -37,6 +37,9 @@ _begin_fstest auto quick copyup redirect exportfs
 # real QA test starts here
 
 _supported_fs overlay
+_fixed_by_kernel_commit 2ca3c148a062 \
+	"ovl: check lower ancestry on encode of lower dir file handle"
+
 _require_scratch
 _require_test_program "open_by_handle"
 # We need to require all features together, because nfs_export cannot
diff --git a/tests/overlay/055 b/tests/overlay/055
index 45a3c107..367f038b 100755
--- a/tests/overlay/055
+++ b/tests/overlay/055
@@ -46,6 +46,9 @@ _cleanup()
 # real QA test starts here
 
 _supported_fs overlay
+_fixed_by_kernel_commit 2ca3c148a062 \
+	"ovl: check lower ancestry on encode of lower dir file handle"
+
 _require_test
 _require_test_program "open_by_handle"
 # Use non-default scratch underlying overlay dirs, we need to check
diff --git a/tests/overlay/063 b/tests/overlay/063
index 94726000..f7bd46e4 100755
--- a/tests/overlay/063
+++ b/tests/overlay/063
@@ -17,6 +17,9 @@ _begin_fstest auto quick whiteout
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 5e1275808630 \
+	"ovl: check whiteout in ovl_create_over_whiteout()"
+
 _require_scratch
 
 # Remove all files from previous tests
diff --git a/tests/overlay/065 b/tests/overlay/065
index 5f3fe097..38430f86 100755
--- a/tests/overlay/065
+++ b/tests/overlay/065
@@ -38,6 +38,10 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v5.2"
+_fixed_by_kernel_commit 0be0bfd2de9d \
+	"ovl: fix regression caused by overlapping layers detection"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/067 b/tests/overlay/067
index 7dddb265..3f54a418 100755
--- a/tests/overlay/067
+++ b/tests/overlay/067
@@ -20,6 +20,9 @@ _begin_fstest auto quick copyup nonsamefs
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 9c6d8f13e9da \
+	"ovl: fix corner case of non-unique st_dev;st_ino"
+
 # Use non-default scratch underlying overlay dirs, we need to check
 # them explicity after test.
 _require_scratch_nocheck
diff --git a/tests/overlay/070 b/tests/overlay/070
index d433279a..f7f926f9 100755
--- a/tests/overlay/070
+++ b/tests/overlay/070
@@ -8,7 +8,7 @@
 # nested overlay setup, where all layers of both overlays are on the
 # same fs.
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
 # - stat file A shows inode number X
 # - modify A to trigger copy up
 # - stat file A shows inode number Y != X
@@ -35,6 +35,8 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.17"
+
 _require_scratch_nocheck
 _require_test_program "af_unix"
 _require_test_program "t_dir_type"
diff --git a/tests/overlay/071 b/tests/overlay/071
index 2ef28369..4f35a8d9 100755
--- a/tests/overlay/071
+++ b/tests/overlay/071
@@ -7,7 +7,7 @@
 # This is a variant of overlay/017 to test constant st_ino numbers for
 # nested overlay setup, where lower overlay layers are not on the same fs.
 #
-# This simple test demonstrates a known issue with overlayfs:
+# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
 # - stat file A shows inode number X
 # - modify A to trigger copy up
 # - stat file A shows inode number Y != X
@@ -38,6 +38,8 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v4.17"
+
 _require_test
 _require_scratch_nocheck
 _require_test_program "af_unix"
diff --git a/tests/overlay/072 b/tests/overlay/072
index bdb608ff..6f5e77df 100755
--- a/tests/overlay/072
+++ b/tests/overlay/072
@@ -28,6 +28,8 @@ _begin_fstest auto quick copyup hardlink
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 83552eacdfc0 "ovl: fix WARN_ON nlink drop to zero"
+
 _require_scratch
 
 upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
diff --git a/tests/overlay/074 b/tests/overlay/074
index 833e7919..d3738649 100755
--- a/tests/overlay/074
+++ b/tests/overlay/074
@@ -22,6 +22,11 @@ _begin_fstest auto quick exportfs dangerous
 # real QA test starts here
 
 _supported_fs overlay
+_fixed_by_kernel_commit 144da23beab8 \
+	"ovl: return required buffer size for file handles"
+_fixed_by_kernel_commit 9aafc1b01873 \
+	"ovl: potential crash in ovl_fid_to_fh()"
+
 _require_scratch
 _require_test_program "open_by_handle"
 # We need to require all features together, because nfs_export cannot
diff --git a/tests/overlay/077 b/tests/overlay/077
index d22a1a94..702ff54c 100755
--- a/tests/overlay/077
+++ b/tests/overlay/077
@@ -18,6 +18,11 @@ _begin_fstest auto quick dir
 
 # real QA test starts here
 _supported_fs overlay
+_fixed_by_kernel_commit 65cd913ec9d9 \
+	"ovl: invalidate readdir cache on changes to dir with origin"
+_fixed_by_kernel_commit 9011c2791e63 \
+	"ovl: skip stale entries in merge dir cache iteration"
+
 _require_scratch_nocheck
 
 # Use small getdents bufsize to fit less than 10 entries
diff --git a/tests/overlay/078 b/tests/overlay/078
index 9e9be03f..d71f09bf 100755
--- a/tests/overlay/078
+++ b/tests/overlay/078
@@ -33,6 +33,7 @@ _cleanup()
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_before_kernel "v5.13"
 
 _require_command "$LSATTR_PROG" lasttr
 _require_command "$CHATTR_PROG" chattr
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* [PATCH 3/3] common: add run option to skip tests for known issues
  2022-04-17 17:40 [PATCH 0/3] Annonate fstests with possible reasons for failure Amir Goldstein
  2022-04-17 17:40 ` [PATCH 1/3] common: support black listing fs in _supported_fs() Amir Goldstein
  2022-04-17 17:40 ` [PATCH 2/3] common: print hints for reasons of test failures Amir Goldstein
@ 2022-04-17 17:40 ` Amir Goldstein
  2022-04-18  5:05   ` Zorro Lang
  2 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2022-04-17 17:40 UTC (permalink / raw)
  To: Eryu Guan, Zorro Lang; +Cc: Darrick J . Wong, Luis Chamberlain, fstests

Some tests are written before a fix is merged upstream and some tests
are written without a known available fix at all. Such is the case with
test overlay/061.

Introduce a helper _known_issues_on_fs() which can be used to document
this situation and print a hint on failure.
The helper supports specifying specifc fs with the known issue for
generic tests as well as the notation ^FS1 ^FS2 to indicate a known issue
on all filesystems expect for FS1 FS2.

Setting the variable SKIP_KNOWN_ISSUES=yes, will cause all tests
annotated as known issues for the tested fs to be skipped.

A future improvement may provide a run option to skip tests based on
_known_issues_before_kernel when running the tests on an older kernel
version.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 README            |  2 ++
 common/rc         | 16 +++++++++++++++-
 tests/overlay/061 |  2 ++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/README b/README
index 7da66cb6..8abc3840 100644
--- a/README
+++ b/README
@@ -241,6 +241,8 @@ Misc:
    this option is supported for all filesystems currently only -overlay is
    expected to run without issues. For other filesystems additional patches
    and fixes to the test suite might be needed.
+ - set SKIP_KNOWN_ISSUES=yes to skip tests for bug without a known fix.
+   Those tests are annotated with _known_issue_on_fs helper.
 
 ______________________
 USING THE FSQA SUITE
diff --git a/common/rc b/common/rc
index 2e9dc408..3cf60a7e 100644
--- a/common/rc
+++ b/common/rc
@@ -1597,8 +1597,23 @@ _supported_fs()
 		_notrun "not suitable for this filesystem type: $FSTYP"
 }
 
+_known_issue_on_fs()
+{
+	# Only "supported" fs have the known issue
+	_check_supported_fs $* || return
+
+	if [ "$SKIP_KNOWN_ISSUES" = "yes" ]; then
+		_notrun "known issue for this filesystem type: $FSTYP"
+	fi
+
+	echo "HINT: You _MAY_ be hit by a known issue for filesystem type $FSTYP." >> $seqres.hints
+	echo >> $seqres.hints
+}
+
 _known_issue_before_kernel()
 {
+	# TODO: don't run if $SKIP_KNOWN_ISSUES and kernel version < $1
+
 	echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
 	echo >> $seqres.hints
 }
@@ -4929,7 +4944,6 @@ _require_kernel_config()
 	_has_kernel_config $1 || _notrun "Installed kernel not built with $1"
 }
 
-
 init_rc
 
 ################################################################################
diff --git a/tests/overlay/061 b/tests/overlay/061
index b80cf5a0..36be3391 100755
--- a/tests/overlay/061
+++ b/tests/overlay/061
@@ -22,6 +22,8 @@ _begin_fstest posix copyup
 
 # real QA test starts here
 _supported_fs overlay
+_known_issue_on_fs overlay
+
 _require_scratch
 _require_xfs_io_command "open"
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] common: support black listing fs in _supported_fs()
  2022-04-17 17:40 ` [PATCH 1/3] common: support black listing fs in _supported_fs() Amir Goldstein
@ 2022-04-18  4:33   ` Zorro Lang
  0 siblings, 0 replies; 13+ messages in thread
From: Zorro Lang @ 2022-04-18  4:33 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Sun, Apr 17, 2022 at 08:40:21PM +0300, Amir Goldstein wrote:
> For example:
> _supported_fs ^xfs
> 
> There is no need to specify "generic" when using a block list
> "all other fs are supported" is implied.
> 
> Converted some generic tests that open code this condition without
> emitting a meaningful reason.
> 
> More generic test like generic/186,187 could use a block list, but
> were not converted because they emit some meaningful reason:
>   _notrun "Can't fragment free space on btrfs."
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

At first, I'd like to take more time to hear discussion from others. I'll
leave personal review point to each patch, welcome more opinions :)
...

This feature is good to me. But I'd strongly recommend that don't *abuse*
it, except we have a definite reason to count a fs out entirely.

The best way to _notrun a case is by detecting a specified feature/behavior
is supported or not. Judge by kernel/package version is worse. Skipping a
fs entirely without proper reason is the worst.

The generic cases trys to open for all known/unknown filesystems at first,
so before a fs maintainer/devel clearly make sure they don't/no way to accept
this test, we'd better to give them chance to have a try, not count them out
from beginning.

Thanks,
Zorro

>  common/rc         | 30 ++++++++++++++++++++----------
>  tests/generic/500 |  3 +--
>  tests/generic/631 |  3 +--
>  tests/generic/679 |  3 +--
>  4 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 17629801..1d37dcbd 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1572,19 +1572,29 @@ _fail()
>  
>  # tests whether $FSTYP is one of the supported filesystems for a test
>  #
> -_supported_fs()
> +_check_supported_fs()
>  {
> -    local f
> +	local res=1
> +	local f
>  
> -    for f
> -    do
> -	if [ "$f" = "$FSTYP" -o "$f" = "generic" ]
> -	then
> -	    return
> -	fi
> -    done
> +	for f; do
> +		# ^FS means black listed fs
> +		if [ "$f" = "^$FSTYP" ]; then
> +			return 1
> +		elif [ "$f" = "generic" ] || [[ "$f" == "^"* ]]; then
> +			# ^FS implies "generic ^FS"
> +			res=0
> +		elif [ "$f" = "$FSTYP" ]; then
> +			return 0
> +		fi
> +	done
> +	return $res
> +}
>  
> -    _notrun "not suitable for this filesystem type: $FSTYP"
> +_supported_fs()
> +{
> +	_check_supported_fs $* || \
> +		_notrun "not suitable for this filesystem type: $FSTYP"
>  }
>  
>  # check if a FS on a device is mounted
> diff --git a/tests/generic/500 b/tests/generic/500
> index 0be59934..bc84d219 100755
> --- a/tests/generic/500
> +++ b/tests/generic/500
> @@ -35,7 +35,6 @@ _cleanup()
>  . ./common/dmthin
>  
>  # real QA test starts here
> -_supported_fs generic
>  _require_scratch_nocheck
>  _require_dm_target thin-pool
>  
> @@ -43,7 +42,7 @@ _require_dm_target thin-pool
>  # and since we've filled the thinp device it'll return EIO, which will make
>  # btrfs flip read only, making it fail this test when it just won't work right
>  # for us in the first place.
> -test $FSTYP == "btrfs"  && _notrun "btrfs doesn't work that way lol"
> +_supported_fs ^btrfs
>  
>  # Require underlying device support discard
>  _scratch_mkfs >>$seqres.full 2>&1
> diff --git a/tests/generic/631 b/tests/generic/631
> index 4996bce7..219f7a05 100755
> --- a/tests/generic/631
> +++ b/tests/generic/631
> @@ -36,10 +36,9 @@ _cleanup()
>  . ./common/attr
>  
>  # real QA test starts here
> -_supported_fs generic
>  _require_scratch
>  _require_attrs trusted
> -test "$FSTYP" = "overlay" && _notrun "Test does not apply to overlayfs."
> +_supported_fs ^overlay
>  _require_extra_fs overlay
>  
>  _scratch_mkfs >> $seqres.full
> diff --git a/tests/generic/679 b/tests/generic/679
> index 440f0c08..c32d42b9 100755
> --- a/tests/generic/679
> +++ b/tests/generic/679
> @@ -17,7 +17,6 @@ _begin_fstest auto quick prealloc
>  
>  # real QA test starts here
>  
> -_supported_fs generic
>  _require_scratch
>  _require_xfs_io_command "falloc"
>  _require_xfs_io_command "fiemap"
> @@ -26,7 +25,7 @@ _require_xfs_io_command "fiemap"
>  #
>  #   https://lore.kernel.org/linux-btrfs/20220315164011.GF8241@magnolia/
>  #
> -[ $FSTYP == "xfs" ] && _notrun "test not valid for xfs"
> +_supported_fs ^xfs
>  
>  rm -f $seqres.full
>  
> -- 
> 2.35.1
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] common: print hints for reasons of test failures
  2022-04-17 17:40 ` [PATCH 2/3] common: print hints for reasons of test failures Amir Goldstein
@ 2022-04-18  4:52   ` Zorro Lang
  2022-04-18  5:26     ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2022-04-18  4:52 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Sun, Apr 17, 2022 at 08:40:22PM +0300, Amir Goldstein wrote:
> Introduce helpers _known_issue_before_kernel() and
> _fixed_by_kernel_commit() that can be used to hint testers why a test
> might be failing and aid in auto-generating of expunge lists for
> downstream kernel testing.
> 
> Annotate fix kernel commits for some overlayfs tests and fix kernel
> version for some overlayfs tests testing for legacy behavior whose
> fixes are not likely to be backported to stable kernels.
> 
> This is modeled after LTP's 'make filter-known-fails' and
> print_failure_hints() using struct tst_tag annotations.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

The _fixed_by_kernel_commit is fine, just not all cases for kernel patch,
some of them cover patch from userspace packages. So you might change it
to _fixed_by_upstream_commit, then let its arguments point out the commit
from which upstream project.

The _known_issue_before_kernel() is a little ... hard to say. If it's only
for one base line of a project (e.g. mainline), then it make sense. But if
for lots of downstream projects, the version number will not totally
different. Some downstream projects might backport lots of upstream patches,
without change its base verion.

If it's just used as a hint output, then it more likes a weak version of
_fixed_by_kernel_commit. When we can't make sure a case cover which specified
commits, then we use a big version number directly.

If it doesn't aim to be a hint output simply, then we face the problem I
said above, it just can be used for versions from one base line of a project.

Thanks,
Zorro

>  check             | 10 +++++++++-
>  common/preamble   |  2 +-
>  common/rc         | 14 ++++++++++++++
>  tests/overlay/009 |  3 +++
>  tests/overlay/010 |  3 +++
>  tests/overlay/014 |  2 ++
>  tests/overlay/016 |  4 +++-
>  tests/overlay/017 |  4 +++-
>  tests/overlay/018 |  4 +++-
>  tests/overlay/020 |  3 +++
>  tests/overlay/022 |  2 ++
>  tests/overlay/029 |  2 ++
>  tests/overlay/038 |  2 ++
>  tests/overlay/041 |  2 ++
>  tests/overlay/042 |  3 +++
>  tests/overlay/043 |  4 +++-
>  tests/overlay/044 |  4 +++-
>  tests/overlay/054 |  3 +++
>  tests/overlay/055 |  3 +++
>  tests/overlay/063 |  3 +++
>  tests/overlay/065 |  4 ++++
>  tests/overlay/067 |  3 +++
>  tests/overlay/070 |  4 +++-
>  tests/overlay/071 |  4 +++-
>  tests/overlay/072 |  2 ++
>  tests/overlay/074 |  5 +++++
>  tests/overlay/077 |  5 +++++
>  tests/overlay/078 |  1 +
>  28 files changed, 96 insertions(+), 9 deletions(-)
> 
> diff --git a/check b/check
> index a0863121..de11b37e 100755
> --- a/check
> +++ b/check
> @@ -808,7 +808,7 @@ function run_section()
>  		fi
>  
>  		# really going to try and run this one
> -		rm -f $seqres.out.bad
> +		rm -f $seqres.out.bad $seqres.hints
>  
>  		# check if we really should run it
>  		_expunge_test $seqnum
> @@ -942,6 +942,14 @@ function run_section()
>  			fi; } | sed -e 's/^\(.\)/    \1/'
>  			err=true
>  		fi
> +		if [ -f $seqres.hints ]; then
> +			if $err; then
> +				echo
> +				cat $seqres.hints
> +			else
> +				rm -f $seqres.hints
> +			fi
> +		fi
>  	done
>  
>  	# make sure we record the status of the last test we ran.
> diff --git a/common/preamble b/common/preamble
> index 64d79385..68219660 100644
> --- a/common/preamble
> +++ b/common/preamble
> @@ -79,6 +79,6 @@ _begin_fstest()
>  	. ./common/rc
>  
>  	# remove previous $seqres.full before test
> -	rm -f $seqres.full
> +	rm -f $seqres.full $seqres.hints
>  
>  }
> diff --git a/common/rc b/common/rc
> index 1d37dcbd..2e9dc408 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1597,6 +1597,19 @@ _supported_fs()
>  		_notrun "not suitable for this filesystem type: $FSTYP"
>  }
>  
> +_known_issue_before_kernel()
> +{
> +	echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
> +	echo >> $seqres.hints
> +}
> +
> +_fixed_by_kernel_commit()
> +{
> +	echo "HINT: You _MAY_ be missing kernel fix:" >> $seqres.hints
> +	echo "      $*" >> $seqres.hints
> +	echo >> $seqres.hints
> +}
> +
>  # check if a FS on a device is mounted
>  # if so, verify that it is mounted on mount point
>  # if fstype is given as argument, verify that it is also
> @@ -4916,6 +4929,7 @@ _require_kernel_config()
>  	_has_kernel_config $1 || _notrun "Installed kernel not built with $1"
>  }
>  
> +
>  init_rc
>  
>  ################################################################################
> diff --git a/tests/overlay/009 b/tests/overlay/009
> index 94bd1b66..d85ef16e 100755
> --- a/tests/overlay/009
> +++ b/tests/overlay/009
> @@ -17,6 +17,9 @@ _begin_fstest auto quick
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit a4859d75944a \
> +	"ovl: fix dentry leak for default_permissions"
> +
>  _require_scratch
>  
>  # Remove all files from previous tests
> diff --git a/tests/overlay/010 b/tests/overlay/010
> index 71ef6ec1..af22b2b4 100755
> --- a/tests/overlay/010
> +++ b/tests/overlay/010
> @@ -17,6 +17,9 @@ _begin_fstest auto quick whiteout
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 84889d493356 \
> +	"ovl: check dentry positiveness in ovl_cleanup_whiteouts()"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/014 b/tests/overlay/014
> index 83295ccc..2d6c11d9 100755
> --- a/tests/overlay/014
> +++ b/tests/overlay/014
> @@ -21,6 +21,8 @@ _begin_fstest auto quick copyup
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 0956254a2d5b "ovl: don't copy up opaqueness"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/016 b/tests/overlay/016
> index 8a091615..d160ff49 100755
> --- a/tests/overlay/016
> +++ b/tests/overlay/016
> @@ -6,7 +6,7 @@
>  #
>  # Test ro/rw fd data inconsistecies
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.19:
>  # - process A opens file F for read
>  # - process B writes new data to file F
>  # - process A reads old data from file F
> @@ -19,6 +19,8 @@ _begin_fstest auto quick copyup
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.19"
> +
>  _require_scratch
>  _require_xfs_io_command "open"
>  
> diff --git a/tests/overlay/017 b/tests/overlay/017
> index 15b0d613..00ca126c 100755
> --- a/tests/overlay/017
> +++ b/tests/overlay/017
> @@ -6,7 +6,7 @@
>  #
>  # Test constant inode numbers
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.14:
>  # - stat file A shows inode number X
>  # - modify A to trigger copy up
>  # - stat file A shows inode number Y != X
> @@ -23,6 +23,8 @@ _begin_fstest auto quick copyup redirect
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.14"
> +
>  _require_scratch
>  _require_test_program "af_unix"
>  _require_test_program "t_dir_type"
> diff --git a/tests/overlay/018 b/tests/overlay/018
> index b09bca9c..0d1cbfa5 100755
> --- a/tests/overlay/018
> +++ b/tests/overlay/018
> @@ -6,7 +6,7 @@
>  #
>  # Test hardlink breakage
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.13:
>  # - file A and B are hardlinked in lower
>  # - modify A to trigger copy up
>  # - file A is no longer a hardlink of file B
> @@ -19,6 +19,8 @@ _begin_fstest auto quick copyup hardlink
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.13"
> +
>  _require_scratch
>  _require_scratch_feature index
>  _require_test_program "t_dir_type"
> diff --git a/tests/overlay/020 b/tests/overlay/020
> index 51f97ccd..98a33aec 100755
> --- a/tests/overlay/020
> +++ b/tests/overlay/020
> @@ -23,6 +23,9 @@ require_unshare() {
>  
>  # Modify as appropriate.
>  _supported_fs overlay
> +_fixed_by_kernel_commit 3fe6e52f0626 \
> +	"ovl: override creds with the ones from the superblock mounter"
> +
>  _require_scratch
>  require_unshare -m -p -U
>  
> diff --git a/tests/overlay/022 b/tests/overlay/022
> index 1a11805d..09af6500 100755
> --- a/tests/overlay/022
> +++ b/tests/overlay/022
> @@ -29,6 +29,8 @@ _cleanup()
>  
>  # Modify as appropriate.
>  _supported_fs overlay
> +_fixed_by_kernel_commit 76bc8e2843b6 "ovl: disallow overlayfs as upperdir"
> +
>  _require_scratch
>  
>  # Remove all files from previous tests
> diff --git a/tests/overlay/029 b/tests/overlay/029
> index 1a532c2a..c4c8eed7 100755
> --- a/tests/overlay/029
> +++ b/tests/overlay/029
> @@ -34,6 +34,8 @@ _cleanup()
>  
>  # Modify as appropriate.
>  _supported_fs overlay
> +_fixed_by_kernel_commit c4fcfc1619ea "ovl: fix d_real() for stacked fs"
> +
>  _require_scratch
>  
>  # Remove all files from previous tests
> diff --git a/tests/overlay/038 b/tests/overlay/038
> index 145b4b34..7bde49ee 100755
> --- a/tests/overlay/038
> +++ b/tests/overlay/038
> @@ -15,6 +15,8 @@ _begin_fstest auto quick copyup
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.14"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/041 b/tests/overlay/041
> index 762e7389..084aaad6 100755
> --- a/tests/overlay/041
> +++ b/tests/overlay/041
> @@ -17,6 +17,8 @@ _begin_fstest auto quick copyup nonsamefs
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.17"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/042 b/tests/overlay/042
> index 60f4b477..0715066f 100755
> --- a/tests/overlay/042
> +++ b/tests/overlay/042
> @@ -26,6 +26,9 @@ _begin_fstest auto quick copyup hardlink
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 6eaf011144af \
> +	"ovl: fix EIO from lookup of non-indexed upper"
> +
>  _require_scratch
>  # Without overlay index feature hardlinks are broken on copy up
>  _require_scratch_feature index
> diff --git a/tests/overlay/043 b/tests/overlay/043
> index 383151dd..e6510ae5 100755
> --- a/tests/overlay/043
> +++ b/tests/overlay/043
> @@ -8,7 +8,7 @@
>  # This is a variant of overlay/017 to test constant st_ino numbers for
>  # non-samefs setup.
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
>  # - stat file A shows inode number X
>  # - modify A to trigger copy up
>  # - stat file A shows inode number Y != X
> @@ -25,6 +25,8 @@ _begin_fstest auto quick copyup nonsamefs
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.17"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/044 b/tests/overlay/044
> index 3f54b7ea..b11f4359 100755
> --- a/tests/overlay/044
> +++ b/tests/overlay/044
> @@ -7,7 +7,7 @@
>  # Test hardlink breakage on non-samefs setup
>  # This is a variant of overlay/018 to test.
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
>  # - file A and B are hardlinked in lower
>  # - modify A to trigger copy up
>  # - file A is no longer a hardlink of file B
> @@ -20,6 +20,8 @@ _begin_fstest auto quick copyup hardlink nonsamefs
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.17"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/054 b/tests/overlay/054
> index d95427b6..ba20a7fc 100755
> --- a/tests/overlay/054
> +++ b/tests/overlay/054
> @@ -37,6 +37,9 @@ _begin_fstest auto quick copyup redirect exportfs
>  # real QA test starts here
>  
>  _supported_fs overlay
> +_fixed_by_kernel_commit 2ca3c148a062 \
> +	"ovl: check lower ancestry on encode of lower dir file handle"
> +
>  _require_scratch
>  _require_test_program "open_by_handle"
>  # We need to require all features together, because nfs_export cannot
> diff --git a/tests/overlay/055 b/tests/overlay/055
> index 45a3c107..367f038b 100755
> --- a/tests/overlay/055
> +++ b/tests/overlay/055
> @@ -46,6 +46,9 @@ _cleanup()
>  # real QA test starts here
>  
>  _supported_fs overlay
> +_fixed_by_kernel_commit 2ca3c148a062 \
> +	"ovl: check lower ancestry on encode of lower dir file handle"
> +
>  _require_test
>  _require_test_program "open_by_handle"
>  # Use non-default scratch underlying overlay dirs, we need to check
> diff --git a/tests/overlay/063 b/tests/overlay/063
> index 94726000..f7bd46e4 100755
> --- a/tests/overlay/063
> +++ b/tests/overlay/063
> @@ -17,6 +17,9 @@ _begin_fstest auto quick whiteout
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 5e1275808630 \
> +	"ovl: check whiteout in ovl_create_over_whiteout()"
> +
>  _require_scratch
>  
>  # Remove all files from previous tests
> diff --git a/tests/overlay/065 b/tests/overlay/065
> index 5f3fe097..38430f86 100755
> --- a/tests/overlay/065
> +++ b/tests/overlay/065
> @@ -38,6 +38,10 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v5.2"
> +_fixed_by_kernel_commit 0be0bfd2de9d \
> +	"ovl: fix regression caused by overlapping layers detection"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/067 b/tests/overlay/067
> index 7dddb265..3f54a418 100755
> --- a/tests/overlay/067
> +++ b/tests/overlay/067
> @@ -20,6 +20,9 @@ _begin_fstest auto quick copyup nonsamefs
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 9c6d8f13e9da \
> +	"ovl: fix corner case of non-unique st_dev;st_ino"
> +
>  # Use non-default scratch underlying overlay dirs, we need to check
>  # them explicity after test.
>  _require_scratch_nocheck
> diff --git a/tests/overlay/070 b/tests/overlay/070
> index d433279a..f7f926f9 100755
> --- a/tests/overlay/070
> +++ b/tests/overlay/070
> @@ -8,7 +8,7 @@
>  # nested overlay setup, where all layers of both overlays are on the
>  # same fs.
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
>  # - stat file A shows inode number X
>  # - modify A to trigger copy up
>  # - stat file A shows inode number Y != X
> @@ -35,6 +35,8 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.17"
> +
>  _require_scratch_nocheck
>  _require_test_program "af_unix"
>  _require_test_program "t_dir_type"
> diff --git a/tests/overlay/071 b/tests/overlay/071
> index 2ef28369..4f35a8d9 100755
> --- a/tests/overlay/071
> +++ b/tests/overlay/071
> @@ -7,7 +7,7 @@
>  # This is a variant of overlay/017 to test constant st_ino numbers for
>  # nested overlay setup, where lower overlay layers are not on the same fs.
>  #
> -# This simple test demonstrates a known issue with overlayfs:
> +# This simple test demonstrates an issue with overlayfs on kernel < v4.17:
>  # - stat file A shows inode number X
>  # - modify A to trigger copy up
>  # - stat file A shows inode number Y != X
> @@ -38,6 +38,8 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v4.17"
> +
>  _require_test
>  _require_scratch_nocheck
>  _require_test_program "af_unix"
> diff --git a/tests/overlay/072 b/tests/overlay/072
> index bdb608ff..6f5e77df 100755
> --- a/tests/overlay/072
> +++ b/tests/overlay/072
> @@ -28,6 +28,8 @@ _begin_fstest auto quick copyup hardlink
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 83552eacdfc0 "ovl: fix WARN_ON nlink drop to zero"
> +
>  _require_scratch
>  
>  upperdir=$OVL_BASE_SCRATCH_MNT/$OVL_UPPER
> diff --git a/tests/overlay/074 b/tests/overlay/074
> index 833e7919..d3738649 100755
> --- a/tests/overlay/074
> +++ b/tests/overlay/074
> @@ -22,6 +22,11 @@ _begin_fstest auto quick exportfs dangerous
>  # real QA test starts here
>  
>  _supported_fs overlay
> +_fixed_by_kernel_commit 144da23beab8 \
> +	"ovl: return required buffer size for file handles"
> +_fixed_by_kernel_commit 9aafc1b01873 \
> +	"ovl: potential crash in ovl_fid_to_fh()"
> +
>  _require_scratch
>  _require_test_program "open_by_handle"
>  # We need to require all features together, because nfs_export cannot
> diff --git a/tests/overlay/077 b/tests/overlay/077
> index d22a1a94..702ff54c 100755
> --- a/tests/overlay/077
> +++ b/tests/overlay/077
> @@ -18,6 +18,11 @@ _begin_fstest auto quick dir
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_fixed_by_kernel_commit 65cd913ec9d9 \
> +	"ovl: invalidate readdir cache on changes to dir with origin"
> +_fixed_by_kernel_commit 9011c2791e63 \
> +	"ovl: skip stale entries in merge dir cache iteration"
> +
>  _require_scratch_nocheck
>  
>  # Use small getdents bufsize to fit less than 10 entries
> diff --git a/tests/overlay/078 b/tests/overlay/078
> index 9e9be03f..d71f09bf 100755
> --- a/tests/overlay/078
> +++ b/tests/overlay/078
> @@ -33,6 +33,7 @@ _cleanup()
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_before_kernel "v5.13"
>  
>  _require_command "$LSATTR_PROG" lasttr
>  _require_command "$CHATTR_PROG" chattr
> -- 
> 2.35.1
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] common: add run option to skip tests for known issues
  2022-04-17 17:40 ` [PATCH 3/3] common: add run option to skip tests for known issues Amir Goldstein
@ 2022-04-18  5:05   ` Zorro Lang
  2022-04-18  5:56     ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2022-04-18  5:05 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Sun, Apr 17, 2022 at 08:40:23PM +0300, Amir Goldstein wrote:
> Some tests are written before a fix is merged upstream and some tests
> are written without a known available fix at all. Such is the case with
> test overlay/061.
> 
> Introduce a helper _known_issues_on_fs() which can be used to document
> this situation and print a hint on failure.
> The helper supports specifying specifc fs with the known issue for
> generic tests as well as the notation ^FS1 ^FS2 to indicate a known issue
> on all filesystems expect for FS1 FS2.
> 
> Setting the variable SKIP_KNOWN_ISSUES=yes, will cause all tests
> annotated as known issues for the tested fs to be skipped.
> 
> A future improvement may provide a run option to skip tests based on
> _known_issues_before_kernel when running the tests on an older kernel
> version.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  README            |  2 ++
>  common/rc         | 16 +++++++++++++++-
>  tests/overlay/061 |  2 ++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/README b/README
> index 7da66cb6..8abc3840 100644
> --- a/README
> +++ b/README
> @@ -241,6 +241,8 @@ Misc:
>     this option is supported for all filesystems currently only -overlay is
>     expected to run without issues. For other filesystems additional patches
>     and fixes to the test suite might be needed.
> + - set SKIP_KNOWN_ISSUES=yes to skip tests for bug without a known fix.
> +   Those tests are annotated with _known_issue_on_fs helper.
>  
>  ______________________
>  USING THE FSQA SUITE
> diff --git a/common/rc b/common/rc
> index 2e9dc408..3cf60a7e 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -1597,8 +1597,23 @@ _supported_fs()
>  		_notrun "not suitable for this filesystem type: $FSTYP"
>  }
>  
> +_known_issue_on_fs()
> +{
> +	# Only "supported" fs have the known issue
> +	_check_supported_fs $* || return
> +
> +	if [ "$SKIP_KNOWN_ISSUES" = "yes" ]; then
> +		_notrun "known issue for this filesystem type: $FSTYP"
> +	fi
> +
> +	echo "HINT: You _MAY_ be hit by a known issue for filesystem type $FSTYP." >> $seqres.hints
> +	echo >> $seqres.hints
> +}
> +
>  _known_issue_before_kernel()
>  {
> +	# TODO: don't run if $SKIP_KNOWN_ISSUES and kernel version < $1
> +

As I said in last patch review, it's only for one kernel base line, it might cause
downstream kernel testing can't run a case, even if it had backported the patches.

>  	echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
>  	echo >> $seqres.hints
>  }
> @@ -4929,7 +4944,6 @@ _require_kernel_config()
>  	_has_kernel_config $1 || _notrun "Installed kernel not built with $1"
>  }
>  
> -
>  init_rc
>  
>  ################################################################################
> diff --git a/tests/overlay/061 b/tests/overlay/061
> index b80cf5a0..36be3391 100755
> --- a/tests/overlay/061
> +++ b/tests/overlay/061
> @@ -22,6 +22,8 @@ _begin_fstest posix copyup
>  
>  # real QA test starts here
>  _supported_fs overlay
> +_known_issue_on_fs overlay

If it's a overlay case, it's absolutely cover an overlay known issue at first.
Even if it hit a mm or underlying fs bug, the _known_issue_on_fs is helpless
for that.

I can understand above 2 patches, but this patch looks weird to me. We can
give a hint to downstream testing (from upstream commit side). But deal with
known issues, that's another story. I'd like to let testers from different
downstream kernels to deal with their known issues checking by themselves,
don't depend on upstream fstests for all.

Thanks,
Zorro

> +
>  _require_scratch
>  _require_xfs_io_command "open"
>  
> -- 
> 2.35.1
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] common: print hints for reasons of test failures
  2022-04-18  4:52   ` Zorro Lang
@ 2022-04-18  5:26     ` Amir Goldstein
  2022-04-18 15:18       ` Theodore Ts'o
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2022-04-18  5:26 UTC (permalink / raw)
  To: Amir Goldstein, Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Mon, Apr 18, 2022 at 7:52 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Sun, Apr 17, 2022 at 08:40:22PM +0300, Amir Goldstein wrote:
> > Introduce helpers _known_issue_before_kernel() and
> > _fixed_by_kernel_commit() that can be used to hint testers why a test
> > might be failing and aid in auto-generating of expunge lists for
> > downstream kernel testing.
> >
> > Annotate fix kernel commits for some overlayfs tests and fix kernel
> > version for some overlayfs tests testing for legacy behavior whose
> > fixes are not likely to be backported to stable kernels.
> >
> > This is modeled after LTP's 'make filter-known-fails' and
> > print_failure_hints() using struct tst_tag annotations.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
>
> The _fixed_by_kernel_commit is fine, just not all cases for kernel patch,
> some of them cover patch from userspace packages. So you might change it
> to _fixed_by_upstream_commit, then let its arguments point out the commit
> from which upstream project.
>

I thought about it too and I agree we should have _fixed_by_upstream_commit
In fact, LTP tests are annotated like this: {"linux-git", "8edc6e1688fc"}.

But I decided that even if and when we add _fixed_by_upstream_commit
we had better leave the wrapper _fixed_by_kernel_commit for brevity
because it is by far going to be the most used annotation.
I can add this helper now or it could be added later.

> The _known_issue_before_kernel() is a little ... hard to say. If it's only
> for one base line of a project (e.g. mainline), then it make sense. But if
> for lots of downstream projects, the version number will not totally
> different. Some downstream projects might backport lots of upstream patches,
> without change its base verion.
>
> If it's just used as a hint output, then it more likes a weak version of
> _fixed_by_kernel_commit. When we can't make sure a case cover which specified
> commits, then we use a big version number directly.
>
> If it doesn't aim to be a hint output simply, then we face the problem I
> said above, it just can be used for versions from one base line of a project.

That's exactly what it is. A hint.
Note that I used the release tag as the argument,
so the only difference from _fixed_by_kernel_commit is the hint text.
could have also used:
  _fixed_by_kernel_commit v4.19 "overlayfs stacked file operations"
Anyway, if one is testing a downstream kernel and sees a hint like this:

You _MAY_ be missing a kernel fix...
You _MAY_ be hit by a known issue on kernel < ...

All it is is a hint. Same as in LTP, which is used to test downstream kernels.

And if downstream kernel did backport the fix or the entire feature,
then there is a good chance that the test will pass and hint would not be
displayed at all.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] common: add run option to skip tests for known issues
  2022-04-18  5:05   ` Zorro Lang
@ 2022-04-18  5:56     ` Amir Goldstein
  2022-04-18  9:06       ` Zorro Lang
  0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2022-04-18  5:56 UTC (permalink / raw)
  To: Amir Goldstein, Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Mon, Apr 18, 2022 at 8:05 AM Zorro Lang <zlang@redhat.com> wrote:
>
> On Sun, Apr 17, 2022 at 08:40:23PM +0300, Amir Goldstein wrote:
> > Some tests are written before a fix is merged upstream and some tests
> > are written without a known available fix at all. Such is the case with
> > test overlay/061.
> >
> > Introduce a helper _known_issues_on_fs() which can be used to document
> > this situation and print a hint on failure.
> > The helper supports specifying specifc fs with the known issue for
> > generic tests as well as the notation ^FS1 ^FS2 to indicate a known issue
> > on all filesystems expect for FS1 FS2.
> >
> > Setting the variable SKIP_KNOWN_ISSUES=yes, will cause all tests
> > annotated as known issues for the tested fs to be skipped.
> >
> > A future improvement may provide a run option to skip tests based on
> > _known_issues_before_kernel when running the tests on an older kernel
> > version.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  README            |  2 ++
> >  common/rc         | 16 +++++++++++++++-
> >  tests/overlay/061 |  2 ++
> >  3 files changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/README b/README
> > index 7da66cb6..8abc3840 100644
> > --- a/README
> > +++ b/README
> > @@ -241,6 +241,8 @@ Misc:
> >     this option is supported for all filesystems currently only -overlay is
> >     expected to run without issues. For other filesystems additional patches
> >     and fixes to the test suite might be needed.
> > + - set SKIP_KNOWN_ISSUES=yes to skip tests for bug without a known fix.
> > +   Those tests are annotated with _known_issue_on_fs helper.
> >
> >  ______________________
> >  USING THE FSQA SUITE
> > diff --git a/common/rc b/common/rc
> > index 2e9dc408..3cf60a7e 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1597,8 +1597,23 @@ _supported_fs()
> >               _notrun "not suitable for this filesystem type: $FSTYP"
> >  }
> >
> > +_known_issue_on_fs()
> > +{
> > +     # Only "supported" fs have the known issue
> > +     _check_supported_fs $* || return
> > +
> > +     if [ "$SKIP_KNOWN_ISSUES" = "yes" ]; then
> > +             _notrun "known issue for this filesystem type: $FSTYP"
> > +     fi
> > +
> > +     echo "HINT: You _MAY_ be hit by a known issue for filesystem type $FSTYP." >> $seqres.hints
> > +     echo >> $seqres.hints
> > +}
> > +
> >  _known_issue_before_kernel()
> >  {
> > +     # TODO: don't run if $SKIP_KNOWN_ISSUES and kernel version < $1
> > +
>
> As I said in last patch review, it's only for one kernel base line, it might cause
> downstream kernel testing can't run a case, even if it had backported the patches.
>

I agree. This may be useful to some but a bit controversial.
Anyway, I shall remove this TODO.


> >       echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
> >       echo >> $seqres.hints
> >  }
> > @@ -4929,7 +4944,6 @@ _require_kernel_config()
> >       _has_kernel_config $1 || _notrun "Installed kernel not built with $1"
> >  }
> >
> > -
> >  init_rc
> >
> >  ################################################################################
> > diff --git a/tests/overlay/061 b/tests/overlay/061
> > index b80cf5a0..36be3391 100755
> > --- a/tests/overlay/061
> > +++ b/tests/overlay/061
> > @@ -22,6 +22,8 @@ _begin_fstest posix copyup
> >
> >  # real QA test starts here
> >  _supported_fs overlay
> > +_known_issue_on_fs overlay
>
> If it's a overlay case, it's absolutely cover an overlay known issue at first.
> Even if it hit a mm or underlying fs bug, the _known_issue_on_fs is helpless
> for that.

I disagree.
There is a subtle difference.
Most of the tests in fstests are regression tests meaning that they
cover an issue that is supposed to be fixed in the tested kernel.

In case of overlayfs, I took "test driven development" a bit further and
wrote fstests to cover non-standard behavior that we want to fix and then
wrote a development roadmap [*] that took more that 10 kernel releases
to complete.
Test overlay/061 is the last of those tests, whose issue was never addressed
and there is no prospect as to when it is going to be addressed.

This is the reason that previous patch removed all those lines from comments:
-# This simple test demonstrates a known issue with overlayfs:

[*] https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO#Compliance

As a tester, how would you know if something is wrong with your kernel
when a test is failing unless you had an annotation that tells you that the
test is expected to fail?

SKIP_KNOWN_ISSUES takes this one step further and says don't bother
me with tests that are expected to fail.

>
> I can understand above 2 patches, but this patch looks weird to me. We can
> give a hint to downstream testing (from upstream commit side). But deal with
> known issues, that's another story. I'd like to let testers from different
> downstream kernels to deal with their known issues checking by themselves,
> don't depend on upstream fstests for all.
>

Not sure I understand your point.

The intention of this helper was to address the debate over two of Darrick's
new tests. I mentioned them in the cover letter but forgot to mention here:

[1] https://lore.kernel.org/fstests/20220412172853.GG16799@magnolia/
[2] https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/

There is a difference between:
_supported_fs ext4 xfs btrfs

And:

_supported_fs generic
# https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
_known_issue_on_fs ^ext4 ^xfs ^btrfs

The main difference is that the test WILL run and fail on all other fs
and both me and Eryu indicated that should happen.

The difference from only using _supported_fs generic
is that giving more information causes less surprises and less confusion
to other fs developers that get a surprise new failure when pulling fstests
upstream updates.

In the end, that what this series is all about - being more considerate
of other testers and making the information known to test author known
to a much wider audience that does not need to dig in test comments
and long mailing list discussions or overlayfs roadmap wikis to find that
information.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] common: add run option to skip tests for known issues
  2022-04-18  5:56     ` Amir Goldstein
@ 2022-04-18  9:06       ` Zorro Lang
  2022-04-18 10:03         ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Zorro Lang @ 2022-04-18  9:06 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Mon, Apr 18, 2022 at 08:56:43AM +0300, Amir Goldstein wrote:
> On Mon, Apr 18, 2022 at 8:05 AM Zorro Lang <zlang@redhat.com> wrote:
> >
> > On Sun, Apr 17, 2022 at 08:40:23PM +0300, Amir Goldstein wrote:
> > > Some tests are written before a fix is merged upstream and some tests
> > > are written without a known available fix at all. Such is the case with
> > > test overlay/061.
> > >
> > > Introduce a helper _known_issues_on_fs() which can be used to document
> > > this situation and print a hint on failure.
> > > The helper supports specifying specifc fs with the known issue for
> > > generic tests as well as the notation ^FS1 ^FS2 to indicate a known issue
> > > on all filesystems expect for FS1 FS2.
> > >
> > > Setting the variable SKIP_KNOWN_ISSUES=yes, will cause all tests
> > > annotated as known issues for the tested fs to be skipped.
> > >
> > > A future improvement may provide a run option to skip tests based on
> > > _known_issues_before_kernel when running the tests on an older kernel
> > > version.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  README            |  2 ++
> > >  common/rc         | 16 +++++++++++++++-
> > >  tests/overlay/061 |  2 ++
> > >  3 files changed, 19 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/README b/README
> > > index 7da66cb6..8abc3840 100644
> > > --- a/README
> > > +++ b/README
> > > @@ -241,6 +241,8 @@ Misc:
> > >     this option is supported for all filesystems currently only -overlay is
> > >     expected to run without issues. For other filesystems additional patches
> > >     and fixes to the test suite might be needed.
> > > + - set SKIP_KNOWN_ISSUES=yes to skip tests for bug without a known fix.
> > > +   Those tests are annotated with _known_issue_on_fs helper.
> > >
> > >  ______________________
> > >  USING THE FSQA SUITE
> > > diff --git a/common/rc b/common/rc
> > > index 2e9dc408..3cf60a7e 100644
> > > --- a/common/rc
> > > +++ b/common/rc
> > > @@ -1597,8 +1597,23 @@ _supported_fs()
> > >               _notrun "not suitable for this filesystem type: $FSTYP"
> > >  }
> > >
> > > +_known_issue_on_fs()
> > > +{
> > > +     # Only "supported" fs have the known issue
> > > +     _check_supported_fs $* || return
> > > +
> > > +     if [ "$SKIP_KNOWN_ISSUES" = "yes" ]; then
> > > +             _notrun "known issue for this filesystem type: $FSTYP"
> > > +     fi
> > > +
> > > +     echo "HINT: You _MAY_ be hit by a known issue for filesystem type $FSTYP." >> $seqres.hints
> > > +     echo >> $seqres.hints
> > > +}
> > > +
> > >  _known_issue_before_kernel()
> > >  {
> > > +     # TODO: don't run if $SKIP_KNOWN_ISSUES and kernel version < $1
> > > +
> >
> > As I said in last patch review, it's only for one kernel base line, it might cause
> > downstream kernel testing can't run a case, even if it had backported the patches.
> >
> 
> I agree. This may be useful to some but a bit controversial.
> Anyway, I shall remove this TODO.
> 
> 
> > >       echo "HINT: You _MAY_ be hit by a known issue on kernel version < $1." >> $seqres.hints
> > >       echo >> $seqres.hints
> > >  }
> > > @@ -4929,7 +4944,6 @@ _require_kernel_config()
> > >       _has_kernel_config $1 || _notrun "Installed kernel not built with $1"
> > >  }
> > >
> > > -
> > >  init_rc
> > >
> > >  ################################################################################
> > > diff --git a/tests/overlay/061 b/tests/overlay/061
> > > index b80cf5a0..36be3391 100755
> > > --- a/tests/overlay/061
> > > +++ b/tests/overlay/061
> > > @@ -22,6 +22,8 @@ _begin_fstest posix copyup
> > >
> > >  # real QA test starts here
> > >  _supported_fs overlay
> > > +_known_issue_on_fs overlay
> >
> > If it's a overlay case, it's absolutely cover an overlay known issue at first.
> > Even if it hit a mm or underlying fs bug, the _known_issue_on_fs is helpless
> > for that.
> 
> I disagree.
> There is a subtle difference.
> Most of the tests in fstests are regression tests meaning that they
> cover an issue that is supposed to be fixed in the tested kernel.
> 
> In case of overlayfs, I took "test driven development" a bit further and
> wrote fstests to cover non-standard behavior that we want to fix and then
> wrote a development roadmap [*] that took more that 10 kernel releases
> to complete.
> Test overlay/061 is the last of those tests, whose issue was never addressed
> and there is no prospect as to when it is going to be addressed.
> 
> This is the reason that previous patch removed all those lines from comments:
> -# This simple test demonstrates a known issue with overlayfs:
> 
> [*] https://github.com/amir73il/overlayfs/wiki/Overlayfs-TODO#Compliance
> 
> As a tester, how would you know if something is wrong with your kernel
> when a test is failing unless you had an annotation that tells you that the
> test is expected to fail?
> 
> SKIP_KNOWN_ISSUES takes this one step further and says don't bother
> me with tests that are expected to fail.
> 
> >
> > I can understand above 2 patches, but this patch looks weird to me. We can
> > give a hint to downstream testing (from upstream commit side). But deal with
> > known issues, that's another story. I'd like to let testers from different
> > downstream kernels to deal with their known issues checking by themselves,
> > don't depend on upstream fstests for all.
> >
> 
> Not sure I understand your point.

Oh, sorry I didn't explain that clearly. I mean patch [1/3] and [2/3] can be improved
by more talking, but [3/3] is not reasonable for me.

Let's see what's the purpose of _known_issue_on_fs():

1) If it trys to work for upstream mainline:
As a hint, it can be replaced by known commit output [2/3]. As a forcibly "_notrun" to a fs,
it can be replaced by _supported_fs ^$someonefs with a proper comment [1/3]. As a test driven
tool... no, I don't like to make xfstests to be place which developers used to record/update/
remind their "planning to do/fix", they can record them in other place :)

2) If it trys to help downstream testing:
It's really helpless for downstream testing, except downstream testers maintain their
own xfstests, and change `_known_issue_on_fs xxx` for themselves. But as a downstream
tester, I'd like to maintain our known issues (known failures and skip list) by
ourselves in another place, not in xfstests inside.

Anyway, let's see reviewpoint from others too :)

Thanks,
Zorro

> 
> The intention of this helper was to address the debate over two of Darrick's
> new tests. I mentioned them in the cover letter but forgot to mention here:
> 
> [1] https://lore.kernel.org/fstests/20220412172853.GG16799@magnolia/
> [2] https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
> 
> There is a difference between:
> _supported_fs ext4 xfs btrfs
> 
> And:
> 
> _supported_fs generic
> # https://lore.kernel.org/fstests/20220414155007.GC17014@magnolia/
> _known_issue_on_fs ^ext4 ^xfs ^btrfs
> 
> The main difference is that the test WILL run and fail on all other fs
> and both me and Eryu indicated that should happen.
> 
> The difference from only using _supported_fs generic
> is that giving more information causes less surprises and less confusion
> to other fs developers that get a surprise new failure when pulling fstests
> upstream updates.
> 
> In the end, that what this series is all about - being more considerate
> of other testers and making the information known to test author known
> to a much wider audience that does not need to dig in test comments
> and long mailing list discussions or overlayfs roadmap wikis to find that
> information.
> 
> Thanks,
> Amir.
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] common: add run option to skip tests for known issues
  2022-04-18  9:06       ` Zorro Lang
@ 2022-04-18 10:03         ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2022-04-18 10:03 UTC (permalink / raw)
  To: Amir Goldstein, Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

> > > I can understand above 2 patches, but this patch looks weird to me. We can
> > > give a hint to downstream testing (from upstream commit side). But deal with
> > > known issues, that's another story. I'd like to let testers from different
> > > downstream kernels to deal with their known issues checking by themselves,
> > > don't depend on upstream fstests for all.
> > >
> >
> > Not sure I understand your point.
>
> Oh, sorry I didn't explain that clearly. I mean patch [1/3] and [2/3] can be improved
> by more talking, but [3/3] is not reasonable for me.
>
> Let's see what's the purpose of _known_issue_on_fs():
>
> 1) If it trys to work for upstream mainline:
> As a hint, it can be replaced by known commit output [2/3]. As a forcibly "_notrun" to a fs,
> it can be replaced by _supported_fs ^$someonefs with a proper comment [1/3]. As a test driven
> tool... no, I don't like to make xfstests to be place which developers used to record/update/
> remind their "planning to do/fix", they can record them in other place :)
>
> 2) If it trys to help downstream testing:
> It's really helpless for downstream testing, except downstream testers maintain their
> own xfstests, and change `_known_issue_on_fs xxx` for themselves. But as a downstream
> tester, I'd like to maintain our known issues (known failures and skip list) by
> ourselves in another place, not in xfstests inside.
>
> Anyway, let's see reviewpoint from others too :)
>

I understand your POV and agree that the value of _known_issue_on_fs
is questionable, so I don't mind dropping patch 3/3.

Regarding overlay/061, it is not really a problem, I was just using it as
an example. I had already addressed the issue of this test by commit
fdb69864 overlay/061: remove from auto and quick groups

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] common: print hints for reasons of test failures
  2022-04-18  5:26     ` Amir Goldstein
@ 2022-04-18 15:18       ` Theodore Ts'o
  2022-04-18 15:50         ` Amir Goldstein
  0 siblings, 1 reply; 13+ messages in thread
From: Theodore Ts'o @ 2022-04-18 15:18 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Mon, Apr 18, 2022 at 08:26:33AM +0300, Amir Goldstein wrote:
> >
> > The _fixed_by_kernel_commit is fine, just not all cases for kernel patch,
> > some of them cover patch from userspace packages. So you might change it
> > to _fixed_by_upstream_commit, then let its arguments point out the commit
> > from which upstream project.
> 
> I thought about it too and I agree we should have _fixed_by_upstream_commit
> In fact, LTP tests are annotated like this: {"linux-git", "8edc6e1688fc"}.

Sometimes there will be a different fix for stable kernels because the
"proper" fix in upstream is too risky / dangerous / involved.  So it
might be useful to specify multiple commits.

> But I decided that even if and when we add _fixed_by_upstream_commit
> we had better leave the wrapper _fixed_by_kernel_commit for brevity
> because it is by far going to be the most used annotation.
> I can add this helper now or it could be added later.

Maybe allow multiple _fixed_by_kernel_commit where the default is
upstream, and then other git trees can be specified?  e.g.:

_fixed_by_kernel_commit 8edc6e1688fc
_fixed_by_kernel_commit deadbeef1234 linux-5.4
_fixed_by_kernel_commit f00dfeed5678 linux-5.10

					- Ted

^ permalink raw reply	[flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] common: print hints for reasons of test failures
  2022-04-18 15:18       ` Theodore Ts'o
@ 2022-04-18 15:50         ` Amir Goldstein
  0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2022-04-18 15:50 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: Eryu Guan, Darrick J . Wong, Luis Chamberlain, fstests

On Mon, Apr 18, 2022 at 6:19 PM Theodore Ts'o <tytso@mit.edu> wrote:
>
> On Mon, Apr 18, 2022 at 08:26:33AM +0300, Amir Goldstein wrote:
> > >
> > > The _fixed_by_kernel_commit is fine, just not all cases for kernel patch,
> > > some of them cover patch from userspace packages. So you might change it
> > > to _fixed_by_upstream_commit, then let its arguments point out the commit
> > > from which upstream project.
> >
> > I thought about it too and I agree we should have _fixed_by_upstream_commit
> > In fact, LTP tests are annotated like this: {"linux-git", "8edc6e1688fc"}.
>
> Sometimes there will be a different fix for stable kernels because the
> "proper" fix in upstream is too risky / dangerous / involved.  So it
> might be useful to specify multiple commits.
>

multiple hints are supported, see:

tests/overlay/074:_fixed_by_kernel_commit 144da23beab8 \
tests/overlay/074      "ovl: return required buffer size for file handles"
tests/overlay/074:_fixed_by_kernel_commit 9aafc1b01873 \
tests/overlay/074      "ovl: potential crash in ovl_fid_to_fh()"

> > But I decided that even if and when we add _fixed_by_upstream_commit
> > we had better leave the wrapper _fixed_by_kernel_commit for brevity
> > because it is by far going to be the most used annotation.
> > I can add this helper now or it could be added later.
>
> Maybe allow multiple _fixed_by_kernel_commit where the default is
> upstream, and then other git trees can be specified?  e.g.:
>
> _fixed_by_kernel_commit 8edc6e1688fc
> _fixed_by_kernel_commit deadbeef1234 linux-5.4
> _fixed_by_kernel_commit f00dfeed5678 linux-5.10
>

All the args are just free text printed as hint, so you could
write it as:

_fixed_by_kernel_commit 8edc6e1688fc \
       "fanotify: fix notification of groups with inode & mount marks"
_fixed_by_kernel_commit deadbeef1234 linux-5.4 \
       "[5.4 backport] fanotify: fix notification of groups with inode
& mount marks"

Which results in:

HINT: You _MAY_ be missing kernel fix:
    8edc6e1688fc fanotify: fix notification of groups with inode & mount marks

HINT: You _MAY_ be missing kernel fix:
    deadbeef1234 [5.4 backport] fanotify: fix notification of groups
with inode & mount marks

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-04-18 15:56 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-17 17:40 [PATCH 0/3] Annonate fstests with possible reasons for failure Amir Goldstein
2022-04-17 17:40 ` [PATCH 1/3] common: support black listing fs in _supported_fs() Amir Goldstein
2022-04-18  4:33   ` Zorro Lang
2022-04-17 17:40 ` [PATCH 2/3] common: print hints for reasons of test failures Amir Goldstein
2022-04-18  4:52   ` Zorro Lang
2022-04-18  5:26     ` Amir Goldstein
2022-04-18 15:18       ` Theodore Ts'o
2022-04-18 15:50         ` Amir Goldstein
2022-04-17 17:40 ` [PATCH 3/3] common: add run option to skip tests for known issues Amir Goldstein
2022-04-18  5:05   ` Zorro Lang
2022-04-18  5:56     ` Amir Goldstein
2022-04-18  9:06       ` Zorro Lang
2022-04-18 10:03         ` Amir Goldstein

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.