| Message ID | 20211102152828.26895-2-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xfstests: Fix generic/643 on ext2 and ext3 | expand |
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 >
> > + # 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 > > >
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
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 >
> > + # 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 > > >
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 >
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.
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(-)