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

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

[Carlos Maiolino]

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/generic/643 | 27 +++++++++------------------
 2 files changed, 16 insertions(+), 21 deletions(-)

-- 
2.31.1

[PATCH 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 required to change _require_scratch_swapfile(), to drop
the _format_swapfile() return value, otherwise, it will also have a return value
that will end up in tests outputs causing tests to fail.

Tests using _format_swapfile() that do not require the swap file size do not
need to be modified, as the return value will be simply ignored.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 common/rc | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/common/rc b/common/rc
index 7f693d39..fb1f32e0 100644
--- a/common/rc
+++ b/common/rc
@@ -2587,6 +2587,7 @@ _require_odirect()
 _format_swapfile() {
 	local fname="$1"
 	local sz="$2"
+	local swap_log=""
 
 	rm -f "$fname"
 	touch "$fname"
@@ -2595,8 +2596,11 @@ _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
+
+	# return created swap size
+	echo $swap_log | grep -oP '(?<=size = )\w+'
 }
 
 _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.
-- 
2.31.1

[PATCH 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.
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.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 tests/generic/643 | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/tests/generic/643 b/tests/generic/643
index 7a1d3ec7..b7627743 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=$(grep "$swapfile" /proc/swaps | 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 1/2] common/rc: Enable _format_swapfile to return the swap size

[Lukas Czerner]

On Tue, Nov 02, 2021 at 04:28:27PM +0100, Carlos Maiolino wrote:
> 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 required to change _require_scratch_swapfile(), to drop
> the _format_swapfile() return value, otherwise, it will also have a return value
> that will end up in tests outputs causing tests to fail.
> 
> Tests using _format_swapfile() that do not require the swap file size do not
> need to be modified, as the return value will be simply ignored.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  common/rc | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f693d39..fb1f32e0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2587,6 +2587,7 @@ _require_odirect()
>  _format_swapfile() {
>  	local fname="$1"
>  	local sz="$2"
> +	local swap_log=""
>  
>  	rm -f "$fname"
>  	touch "$fname"
> @@ -2595,8 +2596,11 @@ _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
> +
> +	# return created swap size
> +	echo $swap_log | grep -oP '(?<=size = )\w+'

I think you need take into account the fact that the units of the mkswap
output will change depedning on the actual size. It might be better to
grab the size in bytes which is in the output as well.

-Lukas

>  }
>  
>  _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.
> -- 
> 2.31.1
> 

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

[Carlos Maiolino]

> > +	# return created swap size
> > +	echo $swap_log | grep -oP '(?<=size = )\w+'
> 
> I think you need take into account the fact that the units of the mkswap
> output will change depedning on the actual size. It might be better to
> grab the size in bytes which is in the output as well.

Sounds fair. I'll wait a bit more to see if some more people comment it, and
I'll update it.

> 
> -Lukas
> 
> >  }
> >  
> >  _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.
> > -- 
> > 2.31.1
> > 
> 

-- 
Carlos

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

[Eryu Guan]

On Tue, Nov 02, 2021 at 04:28:27PM +0100, Carlos Maiolino wrote:
> 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 required to change _require_scratch_swapfile(), to drop
> the _format_swapfile() return value, otherwise, it will also have a return value
> that will end up in tests outputs causing tests to fail.
> 
> Tests using _format_swapfile() that do not require the swap file size do not
> need to be modified, as the return value will be simply ignored.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  common/rc | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f693d39..fb1f32e0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2587,6 +2587,7 @@ _require_odirect()
>  _format_swapfile() {
>  	local fname="$1"
>  	local sz="$2"
> +	local swap_log=""
>  
>  	rm -f "$fname"
>  	touch "$fname"
> @@ -2595,8 +2596,11 @@ _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
> +
> +	# return created swap size
> +	echo $swap_log | grep -oP '(?<=size = )\w+'

It's now returning the size of the swap space, it'd be better to add
some comments before the helper to describe/explain the behavior.

Thanks,
Eryu

>  }
>  
>  _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.
> -- 
> 2.31.1

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

[Zorro Lang]

On Tue, Nov 02, 2021 at 04:28:27PM +0100, Carlos Maiolino wrote:
> 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 required to change _require_scratch_swapfile(), to drop
> the _format_swapfile() return value, otherwise, it will also have a return value
> that will end up in tests outputs causing tests to fail.
> 
> Tests using _format_swapfile() that do not require the swap file size do not
> need to be modified, as the return value will be simply ignored.
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  common/rc | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index 7f693d39..fb1f32e0 100644
> --- a/common/rc
> +++ b/common/rc
> @@ -2587,6 +2587,7 @@ _require_odirect()
>  _format_swapfile() {
>  	local fname="$1"
>  	local sz="$2"
> +	local swap_log=""
>  
>  	rm -f "$fname"
>  	touch "$fname"
> @@ -2595,8 +2596,11 @@ _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
> +
> +	# return created swap size
> +	echo $swap_log | grep -oP '(?<=size = )\w+'

I saw you've changed generic/643 to suit this change, but there're many cases
call _format_swapfile() [1], now it print something to stdout by default, did
you make sure it won't break other cases' golden image?

Thanks,
Zorro

[1]
$ grep -rsn _format_swapfile tests/
tests/btrfs/173:22:# We can't use _format_swapfile because we don't want chattr +C, and we can't
tests/btrfs/173:34:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
tests/btrfs/177:66:_format_swapfile "$swapfile" $((32 * 1024 * 1024))
tests/btrfs/174:23:_format_swapfile "$swapfile" $(($(get_page_size) * 10))
tests/btrfs/175:21:     _format_swapfile "$SCRATCH_MNT/swap" "$sz"
tests/btrfs/175:50:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
tests/btrfs/176:32:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
tests/btrfs/176:50:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
tests/generic/643:39:_format_swapfile $swapfile 1m >> $seqres.full
tests/generic/357:42:_format_swapfile "$testdir/file1" $((blocks * blksz))
tests/generic/493:30:_format_swapfile "$testdir/file1" $((blocks * blksz))
tests/generic/494:29:_format_swapfile "$testdir/file1" $((blocks * blksz))
tests/generic/356:42:_format_swapfile "$testdir/file1" $((blocks * blksz))
tests/generic/569:34:_format_swapfile $testfile 20m
tests/generic/495:24:# We can't use _format_swapfile because we're using our custom mkswap and
tests/generic/554:33:_format_swapfile $SCRATCH_MNT/swapfile 16m

>  }
>  
>  _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.
> -- 
> 2.31.1
> 

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

[Carlos Maiolino]

> > +	# return created swap size
> > +	echo $swap_log | grep -oP '(?<=size = )\w+'
> 

Hi Zorro.

> I saw you've changed generic/643 to suit this change, but there're many cases
> call _format_swapfile() [1], now it print something to stdout by default, did
> you make sure it won't break other cases' golden image?

Yes, I did change generic/643 to use the value returned by _format_swapfile(). I
did test a few other tests which use _format_swapfile(), just in case, and they
passed normally for me, that's why I added the following comment to my patch's
description:

--
Tests using _format_swapfile() that do not require the swap file size do not
need to be modified, as the return value will be simply ignored.
--

Did you find something failing that wasn't supposed to? If not, I'll try to
rephrase that above on my V2, to be a bit more clear.

Thanks for the review!
Cheers.

> 
> Thanks,
> Zorro
> 
> [1]
> $ grep -rsn _format_swapfile tests/
> tests/btrfs/173:22:# We can't use _format_swapfile because we don't want chattr +C, and we can't
> tests/btrfs/173:34:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> tests/btrfs/177:66:_format_swapfile "$swapfile" $((32 * 1024 * 1024))
> tests/btrfs/174:23:_format_swapfile "$swapfile" $(($(get_page_size) * 10))
> tests/btrfs/175:21:     _format_swapfile "$SCRATCH_MNT/swap" "$sz"
> tests/btrfs/175:50:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> tests/btrfs/176:32:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> tests/btrfs/176:50:_format_swapfile "$SCRATCH_MNT/swap" $(($(get_page_size) * 10))
> tests/generic/643:39:_format_swapfile $swapfile 1m >> $seqres.full
> tests/generic/357:42:_format_swapfile "$testdir/file1" $((blocks * blksz))
> tests/generic/493:30:_format_swapfile "$testdir/file1" $((blocks * blksz))
> tests/generic/494:29:_format_swapfile "$testdir/file1" $((blocks * blksz))
> tests/generic/356:42:_format_swapfile "$testdir/file1" $((blocks * blksz))
> tests/generic/569:34:_format_swapfile $testfile 20m
> tests/generic/495:24:# We can't use _format_swapfile because we're using our custom mkswap and
> tests/generic/554:33:_format_swapfile $SCRATCH_MNT/swapfile 16m
> 
> >  }
> >  
> >  _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.
> > -- 
> > 2.31.1
> > 
> 

-- 
Carlos

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

[Carlos Maiolino]

Hi Eryu.

> > @@ -2595,8 +2596,11 @@ _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
> > +
> > +	# return created swap size
> > +	echo $swap_log | grep -oP '(?<=size = )\w+'
> 
> It's now returning the size of the swap space, it'd be better to add
> some comments before the helper to describe/explain the behavior.

Thanks for the review. You mean another comment on the top of function
definition, despite the one I added inlined? I think it would just make it
redundant, but I'm not against moving my comment from above the echo call, to
the top of the function definition.

Cheers.

> 
> Thanks,
> Eryu
> 
> >  }
> >  
> >  _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.
> > -- 
> > 2.31.1
> 

-- 
Carlos