[PATCH V2 0/2] xfstests: Fix generic/643 on ext2 and ext3

[PATCH V2 0/2] xfstests: Fix generic/643 on ext2 and ext3

[Carlos Maiolino]

Hello.

This is the 2nd version of this series to fix test g/643.

This version ensures _format_swapfile helper returns the swap size in bytes
every time (previous version returned it in a different unit, depending on the
swap's size) and updates remaining users of _format_swapfile() to discard its
return value.

This also slightly change patch 2, making g/643 to use swapon to retrieve swap
size (more details in the patch's description).

Original cover-letter below.


Currently, generic/643 test fails on ext2 and ext3 filesystems when using 1k
block sizes.
The failure itself happens due the mapping of indirect blocks to iomap extents
and the aligment constraints imposed by iomap. Specific details are described in
patch 2.

To fix the test, I modified it to use the swapfile size described in the swap
header (patch 2), and to retrieve such information, at mkswap time, I modified
the _format_swapfile() function, and I believe such modification requires its own
patch (patch 1).

These changes have also been tested on 64k pages (both on arm and PPC) to ensure
it doesn't break the test on such architectures.

Carlos Maiolino (2):
  common/rc: Enable _format_swapfile to return the swap size
  generic/643: Fix for 1k block sizes for ext2 and ext3

 common/rc         | 10 +++++++---
 tests/btrfs/173   |  2 +-
 tests/btrfs/174   |  2 +-
 tests/btrfs/175   |  4 ++--
 tests/btrfs/176   |  2 +-
 tests/btrfs/177   |  2 +-
 tests/generic/356 |  2 +-
 tests/generic/357 |  2 +-
 tests/generic/493 |  2 +-
 tests/generic/494 |  2 +-
 tests/generic/554 |  2 +-
 tests/generic/569 |  2 +-
 tests/generic/643 | 27 +++++++++------------------
 13 files changed, 28 insertions(+), 33 deletions(-)

-- 
2.31.1

[PATCH V2 1/2] common/rc: Enable _format_swapfile to return the swap size

[Carlos Maiolino]

Once the kernel is free to not map the full swap file during a swapon call,
it can be useful to know the exact size of the swap area created during
_format_swapfile().

To achieve this, it is needed to change other _format_swapfile() callers to drop
the return value if not required, otherwise, it will be printed to stdout making
such tests to fail.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- return swap size in bytes
	- add a function description to _format_swapfile
	- update other _format_swapfile() callers to discard its
	  return value
	- rephrase patch's description

 common/rc         | 10 +++++++---
 tests/btrfs/173   |  2 +-
 tests/btrfs/174   |  2 +-
 tests/btrfs/175   |  4 ++--
 tests/btrfs/176   |  2 +-
 tests/btrfs/177   |  2 +-
 tests/generic/356 |  2 +-
 tests/generic/357 |  2 +-
 tests/generic/493 |  2 +-
 tests/generic/494 |  2 +-
 tests/generic/554 |  2 +-
 tests/generic/569 |  2 +-
 12 files changed, 19 insertions(+), 15 deletions(-)

diff --git a/common/rc b/common/rc
index 7f693d39..c3f05f76 100644
--- a/common/rc
+++ b/common/rc
@@ -2584,9 +2584,11 @@ _require_odirect()
 	rm -f $testfile 2>&1 > /dev/null
 }
 
+# Format a swapfile and return its size in bytes
 _format_swapfile() {
 	local fname="$1"
 	local sz="$2"
+	local swap_log=""
 
 	rm -f "$fname"
 	touch "$fname"
@@ -2595,8 +2597,10 @@ _format_swapfile() {
 	$CHATTR_PROG +C "$fname" > /dev/null 2>&1
 	_pwrite_byte 0x61 0 "$sz" "$fname" >> $seqres.full
 	# Ignore permission complaints on filesystems that don't support perms
-	$MKSWAP_PROG "$fname" 2>&1 >> $seqres.full | \
-		grep -v "insecure permission"
+	swap_log=$($MKSWAP_PROG "$fname" 2>&1 | grep -v "insecure permission")
+	echo $swap_log >> $seqres.full
+
+	echo $swap_log | grep -oP '\w+(?= bytes)'
 }
 
 _swapon_file() {
@@ -2628,7 +2632,7 @@ _require_scratch_swapfile()
 	_scratch_mount
 
 	# Minimum size for mkswap is 10 pages
-	_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
+	_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) > /dev/null
 
 	# ext* has supported all variants of swap files since their
 	# introduction, so swapon should not fail.
diff --git a/tests/btrfs/173 b/tests/btrfs/173
index 1252587c..9f53143e 100755
--- a/tests/btrfs/173
+++ b/tests/btrfs/173
@@ -31,7 +31,7 @@ swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1
 
 echo "Compressed file"
 rm -f "$SCRATCH_MNT/swap"
-_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
+_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) > /dev/null
 $CHATTR_PROG +c "$SCRATCH_MNT/swap" 2>&1 | grep -o "Invalid argument while setting flags"
 
 status=0
diff --git a/tests/btrfs/174 b/tests/btrfs/174
index 8bf856ae..3bb5e7f9 100755
--- a/tests/btrfs/174
+++ b/tests/btrfs/174
@@ -20,7 +20,7 @@ _scratch_mount
 
 $BTRFS_UTIL_PROG subvolume create "$SCRATCH_MNT/swapvol" >> $seqres.full
 swapfile="$SCRATCH_MNT/swapvol/swap"
-_format_swapfile "$swapfile" $(($(get_page_size) * 10))
+_format_swapfile "$swapfile" $(($(get_page_size) * 10)) > /dev/null
 swapon "$swapfile"
 
 # Turning off nocow doesn't do anything because the file is not empty, not
diff --git a/tests/btrfs/175 b/tests/btrfs/175
index bf0ede25..6f7832a5 100755
--- a/tests/btrfs/175
+++ b/tests/btrfs/175
@@ -18,7 +18,7 @@ _check_minimal_fs_size $((1024 * 1024 * 1024))
 
 cycle_swapfile() {
 	local sz=${1:-$(($(get_page_size) * 10))}
-	_format_swapfile "$SCRATCH_MNT/swap" "$sz"
+	_format_swapfile "$SCRATCH_MNT/swap" "$sz" > /dev/null
 	swapon "$SCRATCH_MNT/swap" 2>&1 | _filter_scratch
 	swapoff "$SCRATCH_MNT/swap" > /dev/null 2>&1
 }
@@ -47,7 +47,7 @@ _scratch_mkfs >> $seqres.full 2>&1
 _scratch_mount
 # Create the swap file, then add the device. That way we know it's all on one
 # device.
-_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
+_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) > /dev/null
 scratch_dev2="$(echo "${SCRATCH_DEV_POOL}" | awk '{ print $2 }')"
 $BTRFS_UTIL_PROG device add -f "$scratch_dev2" "$SCRATCH_MNT"
 swapon "$SCRATCH_MNT/swap" 2>&1 | _filter_scratch
diff --git a/tests/btrfs/176 b/tests/btrfs/176
index 41b02eb6..8d624d5a 100755
--- a/tests/btrfs/176
+++ b/tests/btrfs/176
@@ -29,7 +29,7 @@ scratch_dev3="$(echo "${SCRATCH_DEV_POOL}" | awk '{ print $3 }')"
 echo "Remove device"
 _scratch_mkfs >> $seqres.full 2>&1
 _scratch_mount
-_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
+_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10)) > /dev/null
 $BTRFS_UTIL_PROG device add -f "$scratch_dev2" "$SCRATCH_MNT"
 swapon "$SCRATCH_MNT/swap" 2>&1 | _filter_scratch
 # We know the swap file is on device 1 because we added device 2 after it was
diff --git a/tests/btrfs/177 b/tests/btrfs/177
index b2050236..2fd11e89 100755
--- a/tests/btrfs/177
+++ b/tests/btrfs/177
@@ -63,7 +63,7 @@ dd if=/dev/zero of="$SCRATCH_MNT/refill" bs=4096 >> $seqres.full 2>&1
 # of the filesystem was used, so the swap file must be in the new part of the
 # filesystem.
 $BTRFS_UTIL_PROG filesystem resize $((3 * fssize)) "$SCRATCH_MNT" | convert_resize_output
-_format_swapfile "$swapfile" $((32 * 1024 * 1024))
+_format_swapfile "$swapfile" $((32 * 1024 * 1024)) > /dev/null
 swapon "$swapfile"
 
 # Free up the first 1GB of the filesystem.
diff --git a/tests/generic/356 b/tests/generic/356
index fa6c0585..ffc7bed5 100755
--- a/tests/generic/356
+++ b/tests/generic/356
@@ -39,7 +39,7 @@ blocks=160
 blksz=65536
 
 echo "Initialize file"
-_format_swapfile "$testdir/file1" $((blocks * blksz))
+_format_swapfile "$testdir/file1" $((blocks * blksz)) > /dev/null
 swapon $testdir/file1
 
 touch "$testdir/file2"
diff --git a/tests/generic/357 b/tests/generic/357
index dc9a91a4..ce748f85 100755
--- a/tests/generic/357
+++ b/tests/generic/357
@@ -39,7 +39,7 @@ blocks=160
 blksz=65536
 
 echo "Initialize file"
-_format_swapfile "$testdir/file1" $((blocks * blksz))
+_format_swapfile "$testdir/file1" $((blocks * blksz)) > /dev/null
 touch "$testdir/file2"
 $CHATTR_PROG +C "$testdir/file2" >/dev/null 2>&1
 _cp_reflink $testdir/file1 $testdir/file2 2>&1 | _filter_scratch
diff --git a/tests/generic/493 b/tests/generic/493
index a26a908d..c2fd72f6 100755
--- a/tests/generic/493
+++ b/tests/generic/493
@@ -27,7 +27,7 @@ blocks=160
 blksz=65536
 
 echo "Initialize file"
-_format_swapfile "$testdir/file1" $((blocks * blksz))
+_format_swapfile "$testdir/file1" $((blocks * blksz)) > /dev/null
 swapon "$testdir/file1"
 
 touch "$testdir/file2"
diff --git a/tests/generic/494 b/tests/generic/494
index 2e14fab8..b41c938d 100755
--- a/tests/generic/494
+++ b/tests/generic/494
@@ -26,7 +26,7 @@ blocks=160
 blksz=65536
 
 echo "Initialize file"
-_format_swapfile "$testdir/file1" $((blocks * blksz))
+_format_swapfile "$testdir/file1" $((blocks * blksz)) > /dev/null
 swapon "$testdir/file1"
 
 echo "Try to truncate"
diff --git a/tests/generic/554 b/tests/generic/554
index c1984285..b9efee0d 100755
--- a/tests/generic/554
+++ b/tests/generic/554
@@ -30,7 +30,7 @@ _scratch_mount
 $XFS_IO_PROG -f -c "pwrite -S 0x61 0 128k" $SCRATCH_MNT/file >> $seqres.full 2>&1
 
 echo swap files return ETXTBUSY
-_format_swapfile $SCRATCH_MNT/swapfile 16m
+_format_swapfile $SCRATCH_MNT/swapfile 16m > /dev/null
 _swapon_file $SCRATCH_MNT/swapfile
 
 $XFS_IO_PROG -f -c "copy_range -l 32k $SCRATCH_MNT/file" $SCRATCH_MNT/swapfile
diff --git a/tests/generic/569 b/tests/generic/569
index 1dc54da8..c5dcfefe 100755
--- a/tests/generic/569
+++ b/tests/generic/569
@@ -31,7 +31,7 @@ _scratch_mount >> $seqres.full 2>&1
 
 testfile=$SCRATCH_MNT/$seq.swap
 
-_format_swapfile $testfile 20m
+_format_swapfile $testfile 20m > /dev/null
 
 # Can you modify the swapfile via previously open file descriptors?
 for verb in 1 2 3 4; do
-- 
2.31.1

[PATCH V2 2/2] generic/643: Fix for 1k block sizes for ext2 and ext3

[Carlos Maiolino]

Currently this test fails on ext2 and ext3 filesystems using 1k block sizes,
because we set the maximum swap size the kernel is allowed to map according to
the mapping kernel created when enabling the original swap file.

But the translation from indirect block mapping to iomap extents associated with
the page alignment requirements imposed by iomap_swapfile_add_extent(), causes
this test to fail. The kernel end up mapping way less pages than the file
actually has.
After the file is extended by the test, the page alignment is not a problem
anymore and the kernel can use the whole space available in the swapfile,
written in its header, and this creates a variance bigger than what the current
test allows, making the tolerance check within the test to fail.

Fix this by using the swap size recorded in the swapfile header (reported by
mkswap), as the maximum swap size the kernel is allowed to map, instead of
reading the swap size mapped by the kernel from /proc. This also makes
the first swap{on/off} cycle unnecessary, so remove it.

Since the size hardcoded in the swapfile header is the limit allowed for the
kernel to map as swap area, this is the real limit the kernel can't map beyond,
and what this test should be checking for.

This patch also slightly changes the way the test check the swap size
'after' the swap file is extended. Instead of retrieving the information
from /proc/swaps directly, the test now relies on 'swapon' tool. This
enables the test to retrieve the swap size in bytes directly, same unit
returned by _format_swapfile. This avoid possible miscalculations caused by
retrieving swap size in different units.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
V2:
	- use swapon to retrieve swap size instead of reading
	  /proc/swaps
	- rephrase patch's description

 tests/generic/643 | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/tests/generic/643 b/tests/generic/643
index 7a1d3ec7..9a0ec2c3 100755
--- a/tests/generic/643
+++ b/tests/generic/643
@@ -36,34 +36,25 @@ _scratch_mount >> $seqres.full
 # Assuming we're not borrowing a FAT16 partition from Windows 3.1, we need an
 # unlikely enough name that we can grep /proc/swaps for this.
 swapfile=$SCRATCH_MNT/386spart.par
-_format_swapfile $swapfile 1m >> $seqres.full
+before_blocks=$(_format_swapfile $swapfile 1m)
 
 page_size=$(getconf PAGE_SIZE)
 
-swapfile_blocks() {
-	local swapfile="$1"
-
-	grep "$swapfile" /proc/swaps | awk '{print $3}'
-}
-
-_swapon_file $swapfile
-before_blocks=$(swapfile_blocks "$swapfile")
-swapoff $swapfile
-
 # Extend the length of the swapfile but do not rewrite the header.
 # The subsequent swapon should set up 1MB worth of blocks, not 2MB.
 $XFS_IO_PROG -f -c 'pwrite 1m 1m' $swapfile >> $seqres.full
 
 _swapon_file $swapfile
-after_blocks=$(swapfile_blocks "$swapfile")
+after_blocks=$(swapon --show --bytes |grep $swapfile | awk '{print $3}')
 swapoff $swapfile
 
-# Both swapon attempts should have found approximately the same number of
-# blocks.  Unfortunately, mkswap and the kernel are a little odd -- the number
-# of pages that mkswap writes into the swapfile header is one page less than
-# the file size, and then the kernel itself doesn't always grab all the pages
-# advertised in the header.  Hence we let the number of swap pages increase by
-# two pages.  I'm looking at you, Mr. 64k pages on arm64...
+# The swapon attempt should have found approximately the same number of blocks
+# originally created by the mkswap.
+# Unfortunately, mkswap and the kernel are a little odd -- the number of pages
+# that mkswap writes into the swapfile header is one page less than the file
+# size, and then the kernel itself doesn't always grab all the pages advertised
+# in the header. Such cases include ext2 and ext3 with 1k block size and arm64
+# with its 64k pages. Hence we let the number of swap pages increase by two pages.
 page_variance=$(( page_size / 512 ))
 _within_tolerance "swap blocks" $after_blocks $before_blocks 0 $page_variance -v
 
-- 
2.31.1

Re: [PATCH V2 0/2] xfstests: Fix generic/643 on ext2 and ext3

[Carlos Maiolino]

Ping.

> 
> This is the 2nd version of this series to fix test g/643.
> 
> This version ensures _format_swapfile helper returns the swap size in bytes
> every time (previous version returned it in a different unit, depending on the
> swap's size) and updates remaining users of _format_swapfile() to discard its
> return value.
> 
> This also slightly change patch 2, making g/643 to use swapon to retrieve swap
> size (more details in the patch's description).
> 
> Original cover-letter below.
> 
> 
> Currently, generic/643 test fails on ext2 and ext3 filesystems when using 1k
> block sizes.
> The failure itself happens due the mapping of indirect blocks to iomap extents
> and the aligment constraints imposed by iomap. Specific details are described in
> patch 2.
> 
> To fix the test, I modified it to use the swapfile size described in the swap
> header (patch 2), and to retrieve such information, at mkswap time, I modified
> the _format_swapfile() function, and I believe such modification requires its own
> patch (patch 1).
> 
> These changes have also been tested on 64k pages (both on arm and PPC) to ensure
> it doesn't break the test on such architectures.
> 
> Carlos Maiolino (2):
>   common/rc: Enable _format_swapfile to return the swap size
>   generic/643: Fix for 1k block sizes for ext2 and ext3
> 
>  common/rc         | 10 +++++++---
>  tests/btrfs/173   |  2 +-
>  tests/btrfs/174   |  2 +-
>  tests/btrfs/175   |  4 ++--
>  tests/btrfs/176   |  2 +-
>  tests/btrfs/177   |  2 +-
>  tests/generic/356 |  2 +-
>  tests/generic/357 |  2 +-
>  tests/generic/493 |  2 +-
>  tests/generic/494 |  2 +-
>  tests/generic/554 |  2 +-
>  tests/generic/569 |  2 +-
>  tests/generic/643 | 27 +++++++++------------------
>  13 files changed, 28 insertions(+), 33 deletions(-)
> 
> -- 
> 2.31.1
> 

-- 
Carlos

Re: [PATCH V2 0/2] xfstests: Fix generic/643 on ext2 and ext3

[Eryu Guan]

On Mon, Nov 22, 2021 at 09:49:17AM +0100, Carlos Maiolino wrote:
> Ping.

I think this patchset has been applied and pushed to upstream in last
week's update. Would you please check if you could see them after
pulling the latest update?

Thanks,
Eryu

> 
> > 
> > This is the 2nd version of this series to fix test g/643.
> > 
> > This version ensures _format_swapfile helper returns the swap size in bytes
> > every time (previous version returned it in a different unit, depending on the
> > swap's size) and updates remaining users of _format_swapfile() to discard its
> > return value.
> > 
> > This also slightly change patch 2, making g/643 to use swapon to retrieve swap
> > size (more details in the patch's description).
> > 
> > Original cover-letter below.
> > 
> > 
> > Currently, generic/643 test fails on ext2 and ext3 filesystems when using 1k
> > block sizes.
> > The failure itself happens due the mapping of indirect blocks to iomap extents
> > and the aligment constraints imposed by iomap. Specific details are described in
> > patch 2.
> > 
> > To fix the test, I modified it to use the swapfile size described in the swap
> > header (patch 2), and to retrieve such information, at mkswap time, I modified
> > the _format_swapfile() function, and I believe such modification requires its own
> > patch (patch 1).
> > 
> > These changes have also been tested on 64k pages (both on arm and PPC) to ensure
> > it doesn't break the test on such architectures.
> > 
> > Carlos Maiolino (2):
> >   common/rc: Enable _format_swapfile to return the swap size
> >   generic/643: Fix for 1k block sizes for ext2 and ext3
> > 
> >  common/rc         | 10 +++++++---
> >  tests/btrfs/173   |  2 +-
> >  tests/btrfs/174   |  2 +-
> >  tests/btrfs/175   |  4 ++--
> >  tests/btrfs/176   |  2 +-
> >  tests/btrfs/177   |  2 +-
> >  tests/generic/356 |  2 +-
> >  tests/generic/357 |  2 +-
> >  tests/generic/493 |  2 +-
> >  tests/generic/494 |  2 +-
> >  tests/generic/554 |  2 +-
> >  tests/generic/569 |  2 +-
> >  tests/generic/643 | 27 +++++++++------------------
> >  13 files changed, 28 insertions(+), 33 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 
> 
> -- 
> Carlos

Re: [PATCH V2 0/2] xfstests: Fix generic/643 on ext2 and ext3

[Carlos Maiolino]

Hi Eryu.

On Mon, Nov 22, 2021 at 06:18:45PM +0800, Eryu Guan wrote:
> On Mon, Nov 22, 2021 at 09:49:17AM +0100, Carlos Maiolino wrote:
> > Ping.
> 
> I think this patchset has been applied and pushed to upstream in last
> week's update. Would you please check if you could see them after
> pulling the latest update?

Yeah, I can see them. Thanks!

> 
> Thanks,
> Eryu
> 
> > 
> > > 
> > > This is the 2nd version of this series to fix test g/643.
> > > 
> > > This version ensures _format_swapfile helper returns the swap size in bytes
> > > every time (previous version returned it in a different unit, depending on the
> > > swap's size) and updates remaining users of _format_swapfile() to discard its
> > > return value.
> > > 
> > > This also slightly change patch 2, making g/643 to use swapon to retrieve swap
> > > size (more details in the patch's description).
> > > 
> > > Original cover-letter below.
> > > 
> > > 
> > > Currently, generic/643 test fails on ext2 and ext3 filesystems when using 1k
> > > block sizes.
> > > The failure itself happens due the mapping of indirect blocks to iomap extents
> > > and the aligment constraints imposed by iomap. Specific details are described in
> > > patch 2.
> > > 
> > > To fix the test, I modified it to use the swapfile size described in the swap
> > > header (patch 2), and to retrieve such information, at mkswap time, I modified
> > > the _format_swapfile() function, and I believe such modification requires its own
> > > patch (patch 1).
> > > 
> > > These changes have also been tested on 64k pages (both on arm and PPC) to ensure
> > > it doesn't break the test on such architectures.
> > > 
> > > Carlos Maiolino (2):
> > >   common/rc: Enable _format_swapfile to return the swap size
> > >   generic/643: Fix for 1k block sizes for ext2 and ext3
> > > 
> > >  common/rc         | 10 +++++++---
> > >  tests/btrfs/173   |  2 +-
> > >  tests/btrfs/174   |  2 +-
> > >  tests/btrfs/175   |  4 ++--
> > >  tests/btrfs/176   |  2 +-
> > >  tests/btrfs/177   |  2 +-
> > >  tests/generic/356 |  2 +-
> > >  tests/generic/357 |  2 +-
> > >  tests/generic/493 |  2 +-
> > >  tests/generic/494 |  2 +-
> > >  tests/generic/554 |  2 +-
> > >  tests/generic/569 |  2 +-
> > >  tests/generic/643 | 27 +++++++++------------------
> > >  13 files changed, 28 insertions(+), 33 deletions(-)
> > > 
> > > -- 
> > > 2.31.1
> > > 
> > 
> > -- 
> > Carlos
> 

-- 
Carlos