* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-02 15:28 ` [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size Carlos Maiolino
@ 2021-11-04 12:52 ` Lukas Czerner
2021-11-04 13:01 ` Carlos Maiolino
2021-11-07 13:14 ` Eryu Guan
2021-11-08 4:26 ` Zorro Lang
2 siblings, 1 reply; 9+ messages in thread
From: Lukas Czerner @ 2021-11-04 12:52 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: fstests
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-04 12:52 ` Lukas Czerner
@ 2021-11-04 13:01 ` Carlos Maiolino
0 siblings, 0 replies; 9+ messages in thread
From: Carlos Maiolino @ 2021-11-04 13:01 UTC (permalink / raw)
To: Lukas Czerner; +Cc: fstests
> > + # 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-02 15:28 ` [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size Carlos Maiolino
2021-11-04 12:52 ` Lukas Czerner
@ 2021-11-07 13:14 ` Eryu Guan
2021-11-09 9:02 ` Carlos Maiolino
2021-11-08 4:26 ` Zorro Lang
2 siblings, 1 reply; 9+ messages in thread
From: Eryu Guan @ 2021-11-07 13:14 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: fstests
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-07 13:14 ` Eryu Guan
@ 2021-11-09 9:02 ` Carlos Maiolino
0 siblings, 0 replies; 9+ messages in thread
From: Carlos Maiolino @ 2021-11-09 9:02 UTC (permalink / raw)
To: Eryu Guan; +Cc: fstests
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-02 15:28 ` [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size Carlos Maiolino
2021-11-04 12:52 ` Lukas Czerner
2021-11-07 13:14 ` Eryu Guan
@ 2021-11-08 4:26 ` Zorro Lang
2021-11-09 8:59 ` Carlos Maiolino
2 siblings, 1 reply; 9+ messages in thread
From: Zorro Lang @ 2021-11-08 4:26 UTC (permalink / raw)
To: Carlos Maiolino; +Cc: fstests
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] common/rc: Enable _format_swapfile to return the swap size
2021-11-08 4:26 ` Zorro Lang
@ 2021-11-09 8:59 ` Carlos Maiolino
0 siblings, 0 replies; 9+ messages in thread
From: Carlos Maiolino @ 2021-11-09 8:59 UTC (permalink / raw)
To: fstests
> > + # 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
^ permalink raw reply [flat|nested] 9+ messages in thread