fstests: Fix tests which checks for swapfile support
cover fstests: Fix tests which checks for swapfile support
For more details, pls refer commit msg of each patch.
Patch-1: modifies _require_scratch_swapfile() to check swapon only for btrfs
Patch-2: adds a swapfile test for fallocate files for ext4, xfs (assuming both FS supports and thus should pass).
Patch-3: added to support tests to run when multiple config section present in local.config file.
Ritesh Harjani (3):
common/rc: Make swapon check in _require_scratch_swapfile() specific to btrfs shared/001: Verify swapon on fallocated files for supported FS common/rc: source common/xfs and common/btrfs
common/rc | 20 +++++---- tests/shared/001 | 97 ++++++++++++++++++++++++++++++++++++++++++++ tests/shared/001.out | 6 +++ tests/shared/group | 1 + 4 files changed, 116 insertions(+), 8 deletions(-) create mode 100755 tests/shared/001 create mode 100644 tests/shared/001.out
--
2.26.2
PATCH 1/3 common/rc: Make swapon check in _require_scratch_swapfile() specific to btrfs
swapon/off check in _require_scratch_swapfile() was specifically added for btrfs[1]/[2] since in previous kernels, swapfile was not supported on btrfs. This rather masks the issue sometimes with swapon system call in case if it fails.
for e.g. in latest ext4 upstream tree when "-g quick" (which ran swap tests too) was tested, all swap tests resulted into "_notrun" since swapon failed inside _require_scratch_swapfile() itself.
Whereas this failure on ext4 was actually due to a regression with latest fast-commit patch, which went un-noticed.
Hence make this swapon/off check only specific to btrfs.
Tested default config of xfs/btrfs/ext4/f2fs with this patch.
With this change on buggy kernel, we could clearly catch the swap failures. e.g.
generic/472 17s ...
<...>
@@ -1,4 +1,7 @@
QA output created by 472
regular swap
+swapon: Invalid argument
too long swap
+swapon: Invalid argument
tiny swap
+swapon: Invalid argument
...
(Run 'diff -u /home/qemu/src/tools-work/xfstests-dev/tests/generic/472.out \/home/qemu/src/tools-work/xfstests-dev/results//ext4/generic/472.out.bad' \
to see the entire diff)
[1]: 8c96cfbfe530 ("generic/35[67]: disable swapfile tests on Btrfs") [2]: bd6d67ee598e ("generic: enable swapfile tests on Btrfs")
--- common/rc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)
common/rc +10 -8
@@ -2358,18 +2358,20 @@ _require_scratch_swapfile() [ -n "$SELINUX_MOUNT_OPTIONS" ] && export \ SELINUX_MOUNT_OPTIONS="-o context=system_u:object_r:swapfile_t:s0" - _scratch_mount + if [ "$FSTYP" = "btrfs" ]; then + _scratch_mount + + # Minimum size for mkswap is 10 pages + _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) - # Minimum size for mkswap is 10 pages - _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + _scratch_unmount + _notrun "swapfiles are not supported" + fi - if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 _scratch_unmount - _notrun "swapfiles are not supported" fi - - swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 - _scratch_unmount } # Check that a fs has enough free space (in 1024b blocks)
Signature · 1 line
2.26.2
Re: [PATCH 1/3] common/rc: Make swapon check in _require_scratch_swapfile() specific to btrfs
Quoted 58 lines · Ritesh Harjani
swapon/off check in
_require_scratch_swapfile()was specifically added for btrfs[1]/[2] since in previous kernels, swapfile was not supported on btrfs. This rather masks the issue sometimes with swapon system call in case if it fails.
for e.g. in latest ext4 upstream tree when "-g quick" (which ran swap tests too) was tested, all swap tests resulted into "_notrun" since swapon failed inside_require_scratch_swapfile()itself.
Whereas this failure on ext4 was actually due to a regression with latest fast-commit patch, which went un-noticed.
Hence make this swapon/off check only specific to btrfs.
Tested default config of xfs/btrfs/ext4/f2fs with this patch.With this change on buggy kernel, we could clearly catch the swap failures. e.g.
generic/472 17s ...
<...>(quoted diff) +0
-0@@ -1,4 +1,7 @@QA output created by 472
regular swap
+swapon: Invalid argument
too long swap
+swapon: Invalid argument
tiny swap
+swapon: Invalid argument
...
(Run 'diff -u/home/qemu/src/tools-work/xfstests-dev/tests/generic/472.out\/home/qemu/src/tools-work/xfstests-dev/results//ext4/generic/472.out.bad' \
to see the entire diff)[1]:
8c96cfbfe530("generic/35[67]: disable swapfile tests on Btrfs") [2]:bd6d67ee598e("generic: enable swapfile tests on Btrfs")Signed-off-by Ritesh Harjani <[email protected]>--- common/rc | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)common/rc +9
-3@@ -2358,18 +2358,20 @@ _require_scratch_swapfile() [ -n "$SELINUX_MOUNT_OPTIONS" ] && export \ SELINUX_MOUNT_OPTIONS="-o context=system_u:object_r:swapfile_t:s0" - _scratch_mount + if [ "$FSTYP" = "btrfs" ]; then + _scratch_mount + + # Minimum size for mkswap is 10 pages + _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) - # Minimum size for mkswap is 10 pages - _format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + _scratch_unmount + _notrun "swapfiles are not supported" + fi
common/rc _require_scratch_swapfile() · line 2375
Quoted 10 lines
common/rc +1
-5- if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 _scratch_unmount - _notrun "swapfiles are not supported" fi - - swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 - _scratch_unmount }
This factors out the majority of this function for !btrfs cases to the point where it doesn't do anything swap related. Perhaps it would be more clear to do something like '[ $FSTYP = "btrfs" ] && _require_scratch_swapfile()' in the actual tests that require filtering out on btrfs..?
Brian
Quoted 5 lines
common/rc +0
-0# Check that a fs has enough free space (in 1024b blocks)Signature · 1 line
2.26.2
PATCH 2/3 shared/001: Verify swapon on fallocated files for supported FS
In reply to Ritesh HarjaniCurrently generic/496 tests if swapon works with fallocated files or not on the given FS and bails out with _not_run if it doesn't. Due to this 2 of the regressions went unnoticed in ext4.
Hence this test is created specifically for FS (ext4, xfs) which does support swap functionality on unwritten extents to report an error if it swapon fails with fallocated fils.
--- tests/shared/001 | 97 ++++++++++++++++++++++++++++++++++++++++++++ tests/shared/001.out | 6 +++ tests/shared/group | 1 + 3 files changed, 104 insertions(+) create mode 100755 tests/shared/001 create mode 100644 tests/shared/001.out
tests/shared/001 new file +97 -0
@@ -0,0 +1,97 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Oracle. All Rights Reserved. +# +# FS QA Test 001 +# +# This is similar to generic/472 and generic/496. +# Except that generic/496 is modified to focefully verify if +# swap works on fallocate files for given supported filesystems +# or not instead of bailing out with _not_run, if swapon cmd fails. +# Modified by Ritesh Harjani <[email protected]> +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + swapoff $swapfile 2> /dev/null + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# Modify as appropriate. +_supported_fs ext4 xfs +_require_scratch_swapfile +_require_test_program mkswap +_require_test_program swapon +_require_xfs_io_command "falloc" + +_scratch_mkfs >>$seqres.full 2>&1 +_scratch_mount >>$seqres.full 2>&1 + +swapfile=$SCRATCH_MNT/swap +len=$((2 * 1048576)) +page_size=$(get_page_size) + +swapfile_cycle() { + local swapfile="$1" + + if [ $# -eq 2 ]; then + local len="$2" + _pwrite_byte 0x58 0 $len $swapfile >> $seqres.full + fi + "$here/src/mkswap" $swapfile >> $seqres.full + "$here/src/swapon" $swapfile 2>&1 | _filter_scratch + swapoff $swapfile 2>> $seqres.full + rm -f $swapfile +} + +# from here similar to generic/472 +touch $swapfile + +# Create a regular swap file +echo "regular swap" | tee -a $seqres.full +swapfile_cycle $swapfile $len + +# Create a swap file with a little too much junk on the end +echo "too long swap" | tee -a $seqres.full +swapfile_cycle $swapfile $((len + 3)) + +# Create a ridiculously small swap file. Each swap file must have at least +# two pages after the header page. +echo "tiny swap" | tee -a $seqres.full +swapfile_cycle $swapfile $(($(get_page_size) * 3)) + +# From here similar to generic/496 +echo "fallocate swap" | tee -a $seqres.full +$XFS_IO_PROG -f -c "falloc 0 $len" $swapfile >> $seqres.full +"$here/src/mkswap" $swapfile +"$here/src/swapon" $swapfile 2>&1 | _filter_scratch +swapoff $swapfile + +# Create a fallocated swap file and touch every other $PAGE_SIZE to create +# a mess of written/unwritten extent records +echo "mixed swap" | tee -a $seqres.full +$XFS_IO_PROG -f -c "falloc 0 $len" $swapfile >> $seqres.full +seq $page_size $((page_size * 2)) $len | while read offset; do + _pwrite_byte 0x58 $offset 1 $swapfile >> $seqres.full +done +swapfile_cycle $swapfile + +# success, all done +status=0 +exit
tests/shared/001.out new file +6 -0
@@ -0,0 +1,6 @@ +QA output created by 001 +regular swap +too long swap +tiny swap +fallocate swap +mixed swap
tests/shared/group +1 -0
@@ -3,6 +3,7 @@ # - do not start group names with a digit # - comment line before each group is "new" description # +001 auto swap quick 002 auto metadata quick log 032 mkfs auto quick 298 auto trim
--
2.26.2
Re: [PATCH 2/3] shared/001: Verify swapon on fallocated files for supported FS
Quoted 7 lines · Ritesh Harjani
Currently generic/496 tests if swapon works with fallocated files or not on the given FS and bails out with
_not_runif it doesn't. Due to this 2 of the regressions went unnoticed in ext4.
Hence this test is created specifically for FS (ext4, xfs) which does support swap functionality on unwritten extents to report an error if it swapon fails with fallocated fils.
Quoted 9 lines
Signed-off-by Ritesh Harjani <[email protected]>--- tests/shared/001 | 97 ++++++++++++++++++++++++++++++++++++++++++++ tests/shared/001.out | 6 +++ tests/shared/group | 1 + 3 files changed, 104 insertions(+) create mode 100755 tests/shared/001 create mode 100644 tests/shared/001.out
I could be mistaken, but I thought we were phasing out shared tests in favor of generic tests..?
Quoted 55 lines
tests/shared/001 new file +49
-0@@ -0,0 +1,97 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2018 Oracle. All Rights Reserved. +# +# FS QA Test 001 +# +# This is similar to generic/472 and generic/496. +# Except that generic/496 is modified to focefully verify if +# swap works on fallocate files for given supported filesystems +# or not instead of bailing out with _not_run, if swapon cmd fails. +# Modified by Ritesh Harjani <[email protected]> +# +seq=`basename $0` +seqres=$RESULT_DIR/$seq +echo "QA output created by $seq" + +here=`pwd` +tmp=/tmp/$$ +status=1 # failure is the default! +trap "_cleanup; exit \$status" 0 1 2 3 15 + +_cleanup() +{ + cd / + swapoff $swapfile 2> /dev/null + rm -f $tmp.* +} + +# get standard environment, filters and checks +. ./common/rc +. ./common/filter + +# remove previous $seqres.full before test +rm -f $seqres.full + +# Modify as appropriate. +_supported_fs ext4 xfs +_require_scratch_swapfile +_require_test_program mkswap +_require_test_program swapon +_require_xfs_io_command "falloc" + +_scratch_mkfs >>$seqres.full 2>&1 +_scratch_mount >>$seqres.full 2>&1 + +swapfile=$SCRATCH_MNT/swap +len=$((2 * 1048576)) +page_size=$(get_page_size) +
tests/shared/001 line 59
Quoted 10 lines
tests/shared/001 new file +10
-0+swapfile_cycle() { + local swapfile="$1" + + if [ $# -eq 2 ]; then + local len="$2" + _pwrite_byte 0x58 0 $len $swapfile >> $seqres.full + fi + "$here/src/mkswap" $swapfile >> $seqres.full + "$here/src/swapon" $swapfile 2>&1 | _filter_scratch + swapoff $swapfile 2>> $seqres.full
It might be a little more readable to define and use per-cmd variables like $MKSWAP, etc., if we don't have those already.
Quoted 20 lines
tests/shared/001 new file +20
-0+ rm -f $swapfile +} + +# from here similar to generic/472 +touch $swapfile + +# Create a regular swap file +echo "regular swap" | tee -a $seqres.full +swapfile_cycle $swapfile $len + +# Create a swap file with a little too much junk on the end +echo "too long swap" | tee -a $seqres.full +swapfile_cycle $swapfile $((len + 3)) + +# Create a ridiculously small swap file. Each swap file must have at least +# two pages after the header page. +echo "tiny swap" | tee -a $seqres.full +swapfile_cycle $swapfile $(($(get_page_size) * 3)) + +# From here similar to generic/496
tests/shared/001 line 89
Quoted 10 lines
tests/shared/001 new file +10
-0+echo "fallocate swap" | tee -a $seqres.full +$XFS_IO_PROG -f -c "falloc 0 $len" $swapfile >> $seqres.full +"$here/src/mkswap" $swapfile +"$here/src/swapon" $swapfile 2>&1 | _filter_scratch +swapoff $swapfile + +# Create a fallocated swap file and touch every other $PAGE_SIZE to create +# a mess of written/unwritten extent records +echo "mixed swap" | tee -a $seqres.full +$XFS_IO_PROG -f -c "falloc 0 $len" $swapfile >> $seqres.full
We probably want to recreate the file here. It looks like we reuse the file from the previous test, since it didn't call swapfile_cycle().
I also wonder whether it's really worth duplicating previous test code just to prevent incorrect _notrun cases that should be considered test failures. One could argue that's a bug in the original test that could be fixed. Could we modify those tests to only filter out the btrfs case?
Brian
Quoted 35 lines
tests/shared/001 new file +8
-0+seq $page_size $((page_size * 2)) $len | while read offset; do + _pwrite_byte 0x58 $offset 1 $swapfile >> $seqres.full +done +swapfile_cycle $swapfile + +# success, all done +status=0 +exittests/shared/001.out new file +6
-0@@ -0,0 +1,6 @@ +QA output created by 001 +regular swap +too long swap +tiny swap +fallocate swap +mixed swaptests/shared/group +1
-1@@ -3,6 +3,7 @@ # - do not start group names with a digit # - comment line before each group is "new" description # +001 auto swap quick 002 auto metadata quick log 032 mkfs auto quick 298 auto trim --2.26.2
PATCH 3/3 common/rc: source common/xfs and common/btrfs
In reply to Ritesh HarjaniWithout this patch I am unable to test for multiple different filesystem sections for the same tests. Since we anyway have only function definitions in these files, so it should be ok to source it by default too.
e.g. when I run ./check -s btrfs tests/generic/613 with 3 different [***_fs] sections from local.config file, I see below failures.
./common/rc: line 2801: _check_btrfs_filesystem: command not found
./check -s xfs_4k -g swap (for XFS this fails like below) ./common/rc: line 749: _scratch_mkfs_xfs: command not found
check: failed to mkfs $SCRATCH_DEV using specified options
--- common/rc | 2 ++ 1 file changed, 2 insertions(+)
common/rc +2 -0
@@ -3,6 +3,8 @@ # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. . common/config +. ./common/xfs +. ./common/btrfs BC=$(which bc 2> /dev/null) || BC=
Signature · 1 line
2.26.2
Re: [PATCH 3/3] common/rc: source common/xfs and common/btrfs
Quoted 23 lines · Ritesh Harjani
Without this patch I am unable to test for multiple different filesystem sections for the same tests. Since we anyway have only function definitions in these files, so it should be ok to source it by default too.
e.g. when I run ./check -s btrfs tests/generic/613 with 3 different [***_fs] sections from local.config file, I see below failures../common/rc: line 2801:
_check_btrfs_filesystem: command not found./check -s
xfs_4k-g swap (for XFS this fails like below) ./common/rc: line 749:_scratch_mkfs_xfs: command not found
check: failed to mkfs $SCRATCH_DEV using specified optionsSigned-off-by Ritesh Harjani <[email protected]>--- common/rc | 2 ++ 1 file changed, 2 insertions(+)common/rc +0
-0
common/rc line 7
Quoted 5 lines
common/rc +2
-0@@ -3,6 +3,8 @@ # Copyright (c) 2000-2006 Silicon Graphics, Inc. All Rights Reserved. . common/config +. ./common/xfs +. ./common/btrfs
Uhh, what happens if you run xfs and nfs one after the other?
--D
Quoted 6 lines
common/rc +0
-0BC=$(which bc 2> /dev/null) || BC=Signature · 1 line
2.26.2
Quoted 3 lines · Ritesh Harjani
For more details, pls refer commit msg of each patch.
Patch-1: modifies
_require_scratch_swapfile()to check swapon only for btrfs
I don't think it's a good idea, if a new fs without swapfile support is tested by fstests, test would get false failure, where it should _notrun. And making a generic requirement check to fs-specific doesn't seem quite right either.
Quoted 2 lines
Patch-2: adds a swapfile test for fallocate files for ext4, xfs (assuming both FS supports and thus should pass).
As Brian mentioned in his review, we're in the process to convert all shared tests to generic or fs-specific tests (very slow though), that said we don't want new shared tests.
I think we could whitelist fs types in _require_scratch_swapfile() and don't _notrun for such filesystems, something like what we did in _fstyp_has_non_default_seek_data_hole(), so that we won't miss silent regressions on sucn filesystems, and we'll do sanity check as well on other filesystems.
Quoted 2 lines
Patch-3: added to support tests to run when multiple config section present in local.config file.
I have a patch[1] that should fix the issue 3 years ago, but it never got reviewed, would you please check and see if it fixed the bug for you?
[1] https://patchwork.kernel.org/project/fstests/patch/[email protected]/
Thanks,
Eryu
Quoted 16 lines
Ritesh Harjani (3):
common/rc: Make swapon check in_require_scratch_swapfile()specific to btrfs shared/001: Verify swapon on fallocated files for supported FS common/rc: source common/xfs and common/btrfscommon/rc | 20 +++++---- tests/shared/001 | 97 ++++++++++++++++++++++++++++++++++++++++++++ tests/shared/001.out | 6 +++ tests/shared/group | 1 + 4 files changed, 116 insertions(+), 8 deletions(-) create mode 100755 tests/shared/001 create mode 100644 tests/shared/001.out--
2.26.2
Quoted 17 lines · Eryu Guan
Quoted 3 lines · Ritesh Harjani
For more details, pls refer commit msg of each patch.
Patch-1: modifies
_require_scratch_swapfile()to check swapon only for btrfsI don't think it's a good idea, if a new fs without swapfile support is tested by fstests, test would get false failure, where it should
_notrun. And making a generic requirement check to fs-specific doesn't seem quite right either.Quoted 2 lines
Patch-2: adds a swapfile test for fallocate files for ext4, xfs (assuming both FS supports and thus should pass).
As Brian mentioned in his review, we're in the process to convert all shared tests to generic or fs-specific tests (very slow though), that said we don't want new shared tests.
Quoted 5 lines
I think we could whitelist fs types in
_require_scratch_swapfile()and don't_notrunfor such filesystems, something like what we did in_fstyp_has_non_default_seek_data_hole(), so that we won't miss silent regressions on sucn filesystems, and we'll do sanity check as well on other filesystems.
Nice idea. Let me check that part.
Quoted 8 lines
Quoted 3 lines
Patch-3: added to support tests to run when multiple config section present in local.config file.
I have a patch[1] that should fix the issue 3 years ago, but it never got reviewed, would you please check and see if it fixed the bug for you?
[1] https://patchwork.kernel.org/project/fstests/patch/[email protected]/
Sure, will test it and get back.
Sorry about the delay from my end on this patch series. I got pulled into something else. Let me work on your suggestions.
-ritesh