* [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() @ 2020-12-16 5:17 Ritesh Harjani 2020-12-16 5:17 ` [PATCHv2 2/2] generic/496: Add whitelisted FS support for swapon test Ritesh Harjani 2020-12-16 5:23 ` [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani 0 siblings, 2 replies; 6+ messages in thread From: Ritesh Harjani @ 2020-12-16 5:17 UTC (permalink / raw) To: fstests; +Cc: Eryu Guan, linux-ext4, linux-xfs, anju, Ritesh Harjani Filesystems e.g. ext4 and XFS supports swapon by default and an error returned with swapon should be treated as a failure. Hence add ext4/xfs as whitelisted fstype in _require_scratch_swapfile() Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- v1->v2: Addressed comments from Eryu @[1] [1]: https://patchwork.kernel.org/project/fstests/cover/cover.1604000570.git.riteshh@linux.ibm.com/ common/rc | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/common/rc b/common/rc index 33b5b598a198..635b77a005c6 100644 --- a/common/rc +++ b/common/rc @@ -2380,6 +2380,7 @@ _format_swapfile() { # Check that the filesystem supports swapfiles _require_scratch_swapfile() { + local fstyp=$FSTYP _require_scratch _require_command "$MKSWAP_PROG" "mkswap" @@ -2401,10 +2402,21 @@ _require_scratch_swapfile() # 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 + # For whitelisted fstyp swapon should not fail. + case "$fstyp" in + ext4|xfs) + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + _scratch_unmount + _fail "swapon failed for $fstyp" + fi + ;; + *) + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then + _scratch_unmount + _notrun "swapfiles are not supported" + fi + ;; + esac swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 _scratch_unmount -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCHv2 2/2] generic/496: Add whitelisted FS support for swapon test 2020-12-16 5:17 [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani @ 2020-12-16 5:17 ` Ritesh Harjani 2020-12-16 5:23 ` [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani 1 sibling, 0 replies; 6+ messages in thread From: Ritesh Harjani @ 2020-12-16 5:17 UTC (permalink / raw) To: fstests; +Cc: Eryu Guan, linux-ext4, linux-xfs, anju, Ritesh Harjani ext4, xfs should not fail swapon on fallocated file. Currently if this fails the fstst was not returning a failure. Fix those for whitelisted FS (for now added ext4/xfs). There were some regressions which went unnoticed due to this in ext4 tree, which later got fixed as part of this patch [1] [1]: https://patchwork.ozlabs.org/patch/1357275 Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> --- tests/generic/496 | 16 +++++++++++++--- tests/generic/group | 2 +- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/generic/496 b/tests/generic/496 index 805c6ac1c0ea..0546d8455491 100755 --- a/tests/generic/496 +++ b/tests/generic/496 @@ -5,7 +5,7 @@ # FS QA Test No. 496 # # Test various swapfile activation oddities on filesystems that support -# fallocated swapfiles. +# fallocated swapfiles (for whitelisted fs) # seq=`basename $0` seqres=$RESULT_DIR/$seq @@ -61,8 +61,18 @@ touch $swapfile $CHATTR_PROG +C $swapfile >> $seqres.full 2>&1 $XFS_IO_PROG -f -c "falloc 0 $len" $swapfile >> $seqres.full "$here/src/mkswap" $swapfile -"$here/src/swapon" $swapfile >> $seqres.full 2>&1 || \ - _notrun "fallocated swap not supported here" + +# ext4/xfs should not fail for swapon on fallocated files +case $FSTYP in +ext4|xfs) + "$here/src/swapon" $swapfile >> $seqres.full 2>&1 || \ + _fail "swapon failed on fallocated file" + ;; +*) + "$here/src/swapon" $swapfile >> $seqres.full 2>&1 || \ + _notrun "fallocated swap not supported here" + ;; +esac swapoff $swapfile # Create a fallocated swap file and touch every other $PAGE_SIZE to create diff --git a/tests/generic/group b/tests/generic/group index d8758d7f6a5f..7a7388d92ec6 100644 --- a/tests/generic/group +++ b/tests/generic/group @@ -498,7 +498,7 @@ 493 auto quick swap dedupe 494 auto quick swap punch 495 auto quick swap -496 auto quick swap +496 auto quick swap prealloc 497 auto quick swap collapse 498 auto quick log 499 auto quick rw collapse zero -- 2.26.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() 2020-12-16 5:17 [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani 2020-12-16 5:17 ` [PATCHv2 2/2] generic/496: Add whitelisted FS support for swapon test Ritesh Harjani @ 2020-12-16 5:23 ` Ritesh Harjani 2020-12-20 15:39 ` Eryu Guan 1 sibling, 1 reply; 6+ messages in thread From: Ritesh Harjani @ 2020-12-16 5:23 UTC (permalink / raw) To: fstests, Eryu Guan; +Cc: linux-ext4, linux-xfs, anju On 12/16/20 10:47 AM, Ritesh Harjani wrote: > Filesystems e.g. ext4 and XFS supports swapon by default and an error > returned with swapon should be treated as a failure. Hence > add ext4/xfs as whitelisted fstype in _require_scratch_swapfile() > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > --- > v1->v2: Addressed comments from Eryu @[1] > [1]: https://patchwork.kernel.org/project/fstests/cover/cover.1604000570.git.riteshh@linux.ibm.com/ > > common/rc | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/common/rc b/common/rc > index 33b5b598a198..635b77a005c6 100644 > --- a/common/rc > +++ b/common/rc > @@ -2380,6 +2380,7 @@ _format_swapfile() { > # Check that the filesystem supports swapfiles > _require_scratch_swapfile() > { > + local fstyp=$FSTYP > _require_scratch > _require_command "$MKSWAP_PROG" "mkswap" > > @@ -2401,10 +2402,21 @@ _require_scratch_swapfile() > # 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 > + # For whitelisted fstyp swapon should not fail. > + case "$fstyp" in > + ext4|xfs) > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > + _scratch_unmount > + _fail "swapon failed for $fstyp" @Eryu, As of now I added _fail() if swapon failed for given whitelisting fstype. Do you think this is alright, or should I just ignore the error in case of such FS? > + fi > + ;; > + *) > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > + _scratch_unmount > + _notrun "swapfiles are not supported" > + fi > + ;; > + esac > > swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 > _scratch_unmount > -- > 2.26.2 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() 2020-12-16 5:23 ` [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani @ 2020-12-20 15:39 ` Eryu Guan 2021-01-04 18:25 ` Darrick J. Wong 0 siblings, 1 reply; 6+ messages in thread From: Eryu Guan @ 2020-12-20 15:39 UTC (permalink / raw) To: Ritesh Harjani; +Cc: fstests, linux-ext4, linux-xfs, anju On Wed, Dec 16, 2020 at 10:53:45AM +0530, Ritesh Harjani wrote: > > > On 12/16/20 10:47 AM, Ritesh Harjani wrote: > > Filesystems e.g. ext4 and XFS supports swapon by default and an error > > returned with swapon should be treated as a failure. Hence > > add ext4/xfs as whitelisted fstype in _require_scratch_swapfile() > > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > > --- > > v1->v2: Addressed comments from Eryu @[1] > > [1]: https://patchwork.kernel.org/project/fstests/cover/cover.1604000570.git.riteshh@linux.ibm.com/ > > > > common/rc | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/common/rc b/common/rc > > index 33b5b598a198..635b77a005c6 100644 > > --- a/common/rc > > +++ b/common/rc > > @@ -2380,6 +2380,7 @@ _format_swapfile() { > > # Check that the filesystem supports swapfiles > > _require_scratch_swapfile() > > { > > + local fstyp=$FSTYP > > _require_scratch > > _require_command "$MKSWAP_PROG" "mkswap" > > > > @@ -2401,10 +2402,21 @@ _require_scratch_swapfile() > > # 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 > > + # For whitelisted fstyp swapon should not fail. > > + case "$fstyp" in > > + ext4|xfs) > > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > > + _scratch_unmount > > + _fail "swapon failed for $fstyp" > > @Eryu, > As of now I added _fail() if swapon failed for given whitelisting fstype. > Do you think this is alright, or should I just ignore the error in I think it's reasonable. But I'd like to leave the patchset on the list for review for another week, to see if ext4 and/or xfs folks will chime in and have different thoughts. Thanks, Eryu > case of such FS? > > > > > + fi > > + ;; > > + *) > > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > > + _scratch_unmount > > + _notrun "swapfiles are not supported" > > + fi > > + ;; > > + esac > > > > swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 > > _scratch_unmount > > -- > > 2.26.2 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() 2020-12-20 15:39 ` Eryu Guan @ 2021-01-04 18:25 ` Darrick J. Wong 2021-01-05 11:00 ` Ritesh Harjani 0 siblings, 1 reply; 6+ messages in thread From: Darrick J. Wong @ 2021-01-04 18:25 UTC (permalink / raw) To: Eryu Guan; +Cc: Ritesh Harjani, fstests, linux-ext4, linux-xfs, anju On Sun, Dec 20, 2020 at 11:39:06PM +0800, Eryu Guan wrote: > On Wed, Dec 16, 2020 at 10:53:45AM +0530, Ritesh Harjani wrote: > > > > > > On 12/16/20 10:47 AM, Ritesh Harjani wrote: > > > Filesystems e.g. ext4 and XFS supports swapon by default and an error > > > returned with swapon should be treated as a failure. Hence > > > add ext4/xfs as whitelisted fstype in _require_scratch_swapfile() > > > > > > Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> > > > --- > > > v1->v2: Addressed comments from Eryu @[1] > > > [1]: https://patchwork.kernel.org/project/fstests/cover/cover.1604000570.git.riteshh@linux.ibm.com/ > > > > > > common/rc | 20 ++++++++++++++++---- > > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > > > diff --git a/common/rc b/common/rc > > > index 33b5b598a198..635b77a005c6 100644 > > > --- a/common/rc > > > +++ b/common/rc > > > @@ -2380,6 +2380,7 @@ _format_swapfile() { > > > # Check that the filesystem supports swapfiles > > > _require_scratch_swapfile() > > > { > > > + local fstyp=$FSTYP > > > _require_scratch > > > _require_command "$MKSWAP_PROG" "mkswap" > > > > > > @@ -2401,10 +2402,21 @@ _require_scratch_swapfile() > > > # 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 > > > + # For whitelisted fstyp swapon should not fail. I would use a different phase than 'whitelisted', since that doesn't tell us why ext4 and xfs are special: # ext* and xfs have supported all variants of swap files since their # introduction, so swapon should not fail. > > > + case "$fstyp" in $FSTYP, not $fstyp > > > + ext4|xfs) I would also add a few more FSTYPs here, since at least ext2 and ext3 supported swap files. Are there other old fses that do? --D > > > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > > > + _scratch_unmount > > > + _fail "swapon failed for $fstyp" > > > > @Eryu, > > As of now I added _fail() if swapon failed for given whitelisting fstype. > > Do you think this is alright, or should I just ignore the error in > > I think it's reasonable. > > But I'd like to leave the patchset on the list for review for another > week, to see if ext4 and/or xfs folks will chime in and have different > thoughts. > > Thanks, > Eryu > > > case of such FS? > > > > > > > > > + fi > > > + ;; > > > + *) > > > + if ! swapon "$SCRATCH_MNT/swap" >/dev/null 2>&1; then > > > + _scratch_unmount > > > + _notrun "swapfiles are not supported" > > > + fi > > > + ;; > > > + esac > > > > > > swapoff "$SCRATCH_MNT/swap" >/dev/null 2>&1 > > > _scratch_unmount > > > -- > > > 2.26.2 > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() 2021-01-04 18:25 ` Darrick J. Wong @ 2021-01-05 11:00 ` Ritesh Harjani 0 siblings, 0 replies; 6+ messages in thread From: Ritesh Harjani @ 2021-01-05 11:00 UTC (permalink / raw) To: Darrick J. Wong, Eryu Guan; +Cc: fstests, linux-ext4, linux-xfs, anju On 1/4/21 11:55 PM, Darrick J. Wong wrote: > On Sun, Dec 20, 2020 at 11:39:06PM +0800, Eryu Guan wrote: >> On Wed, Dec 16, 2020 at 10:53:45AM +0530, Ritesh Harjani wrote: >>> >>> >>> On 12/16/20 10:47 AM, Ritesh Harjani wrote: >>>> Filesystems e.g. ext4 and XFS supports swapon by default and an error >>>> returned with swapon should be treated as a failure. Hence >>>> add ext4/xfs as whitelisted fstype in _require_scratch_swapfile() >>>> >>>> Signed-off-by: Ritesh Harjani <riteshh@linux.ibm.com> >>>> --- >>>> v1->v2: Addressed comments from Eryu @[1] >>>> [1]: https://patchwork.kernel.org/project/fstests/cover/cover.1604000570.git.riteshh@linux.ibm.com/ >>>> >>>> common/rc | 20 ++++++++++++++++---- >>>> 1 file changed, 16 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/common/rc b/common/rc >>>> index 33b5b598a198..635b77a005c6 100644 >>>> --- a/common/rc >>>> +++ b/common/rc >>>> @@ -2380,6 +2380,7 @@ _format_swapfile() { >>>> # Check that the filesystem supports swapfiles >>>> _require_scratch_swapfile() >>>> { >>>> + local fstyp=$FSTYP >>>> _require_scratch >>>> _require_command "$MKSWAP_PROG" "mkswap" >>>> >>>> @@ -2401,10 +2402,21 @@ _require_scratch_swapfile() >>>> # 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 >>>> + # For whitelisted fstyp swapon should not fail. > > I would use a different phase than 'whitelisted', since that doesn't > tell us why ext4 and xfs are special: > > # ext* and xfs have supported all variants of swap files since their > # introduction, so swapon should not fail. Sounds ok to me. > >>>> + case "$fstyp" in > > $FSTYP, not $fstyp sure I will use $FSTYP directly and remove local fstyp variable. > >>>> + ext4|xfs) > > I would also add a few more FSTYPs here, since at least ext2 and ext3 > supported swap files. Are there other old fses that do? Sure, agreed. I will add ext2 & ext3 too. -ritesh ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-05 11:02 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-16 5:17 [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani 2020-12-16 5:17 ` [PATCHv2 2/2] generic/496: Add whitelisted FS support for swapon test Ritesh Harjani 2020-12-16 5:23 ` [PATCHv2 1/2] common/rc: Add whitelisted FS support in _require_scratch_swapfile() Ritesh Harjani 2020-12-20 15:39 ` Eryu Guan 2021-01-04 18:25 ` Darrick J. Wong 2021-01-05 11:00 ` Ritesh Harjani
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).