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
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")
Signed-off-by: Ritesh Harjani <[email protected]>
---
common/rc | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/common/rc b/common/rc
index b0c353c4c107..4c59968a6bd3 100644
--- a/common/rc
+++ b/common/rc
@@ -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)
--
2.26.2
Currently 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.
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
diff --git a/tests/shared/001 b/tests/shared/001
new file mode 100755
index 000000000000..ad7285bdb709
--- /dev/null
+++ b/tests/shared/001
@@ -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
diff --git a/tests/shared/001.out b/tests/shared/001.out
new file mode 100644
index 000000000000..2d7655e19422
--- /dev/null
+++ b/tests/shared/001.out
@@ -0,0 +1,6 @@
+QA output created by 001
+regular swap
+too long swap
+tiny swap
+fallocate swap
+mixed swap
diff --git a/tests/shared/group b/tests/shared/group
index a8b926d877d2..5a41b23c7010 100644
--- a/tests/shared/group
+++ b/tests/shared/group
@@ -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
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 options
Signed-off-by: Ritesh Harjani <[email protected]>
---
common/rc | 2 ++
1 file changed, 2 insertions(+)
diff --git a/common/rc b/common/rc
index 4c59968a6bd3..e9ba1b6e8265 100644
--- a/common/rc
+++ b/common/rc
@@ -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=
--
2.26.2
On Fri, Oct 30, 2020 at 01:22:53AM +0530, Ritesh Harjani wrote:
> 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 options
>
> Signed-off-by: Ritesh Harjani <[email protected]>
> ---
> common/rc | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/common/rc b/common/rc
> index 4c59968a6bd3..e9ba1b6e8265 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -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
>
> BC=$(which bc 2> /dev/null) || BC=
>
> --
> 2.26.2
>
On Fri, Oct 30, 2020 at 01:22:51AM +0530, Ritesh Harjani wrote:
> 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")
>
> Signed-off-by: Ritesh Harjani <[email protected]>
> ---
> common/rc | 18 ++++++++++--------
> 1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/common/rc b/common/rc
> index b0c353c4c107..4c59968a6bd3 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -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
> }
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
>
> # Check that a fs has enough free space (in 1024b blocks)
> --
> 2.26.2
>
On Fri, Oct 30, 2020 at 01:22:52AM +0530, Ritesh Harjani wrote:
> Currently 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.
>
> 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..?
> diff --git a/tests/shared/001 b/tests/shared/001
> new file mode 100755
> index 000000000000..ad7285bdb709
> --- /dev/null
> +++ b/tests/shared/001
> @@ -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
It might be a little more readable to define and use per-cmd variables
like $MKSWAP, etc., if we don't have those already.
> + 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
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
> +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
> diff --git a/tests/shared/001.out b/tests/shared/001.out
> new file mode 100644
> index 000000000000..2d7655e19422
> --- /dev/null
> +++ b/tests/shared/001.out
> @@ -0,0 +1,6 @@
> +QA output created by 001
> +regular swap
> +too long swap
> +tiny swap
> +fallocate swap
> +mixed swap
> diff --git a/tests/shared/group b/tests/shared/group
> index a8b926d877d2..5a41b23c7010 100644
> --- a/tests/shared/group
> +++ b/tests/shared/group
> @@ -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
>
On Fri, Oct 30, 2020 at 01:22:50AM +0530, Ritesh Harjani wrote:
> 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.
> 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.
> 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
>
> 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
On 11/1/20 9:33 PM, Eryu Guan wrote:
> On Fri, Oct 30, 2020 at 01:22:50AM +0530, Ritesh Harjani wrote:
>> 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.
>
>> 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.
Nice idea. Let me check that part.
>
>> 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