All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS
@ 2021-05-25  4:46 Zorro Lang
  2021-05-25  4:46 ` [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks Zorro Lang
  2021-05-25 16:05 ` [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Darrick J. Wong
  0 siblings, 2 replies; 7+ messages in thread
From: Zorro Lang @ 2021-05-25  4:46 UTC (permalink / raw)
  To: fstests

xfstests set "-bsize=4k" to XFS_MKFS_OPTIONS by default, then give
it to MKFS_OPTIOPNS. So MKFS_OPTIOPNS always contains "-bsize=4k"
except we set XFS_MKFS_OPTIONS manually.

It's useless to set XFS_MKFS_OPTIONS to "-bsize=4096" by default,
especially that will cause all cases with _scratch_mkfs_blocksized()
always fail as "-b size option respecified", when test on XFS. For
exmaple: generic/222

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 common/config | 1 -
 1 file changed, 1 deletion(-)

diff --git a/common/config b/common/config
index a47e462c..ecd59fae 100644
--- a/common/config
+++ b/common/config
@@ -60,7 +60,6 @@ export EMAIL=root@localhost    # where auto-qa will send its status messages
 export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
 export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
 export BENCH_PASSES=${BENCH_PASSES:=5}
-export XFS_MKFS_OPTIONS=${XFS_MKFS_OPTIONS:=-bsize=4096}
 export TIME_FACTOR=${TIME_FACTOR:=1}
 export LOAD_FACTOR=${LOAD_FACTOR:=1}
 export DEBUGFS_MNT=${DEBUGFS_MNT:="/sys/kernel/debug"}
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks
  2021-05-25  4:46 [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Zorro Lang
@ 2021-05-25  4:46 ` Zorro Lang
  2021-05-25 16:09   ` Darrick J. Wong
  2021-05-25 16:05 ` [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Darrick J. Wong
  1 sibling, 1 reply; 7+ messages in thread
From: Zorro Lang @ 2021-05-25  4:46 UTC (permalink / raw)
  To: fstests

If a swapfile doesn't contain even a single page-aligned contiguous
range of blocks, it's an invalid swapfile, and might cause kernel
issue. This case covered commit 5808fecc5723 ("iomap: Fix negative
assignment to unsigned sis->pages in iomap_swapfile_activate").

Signed-off-by: Zorro Lang <zlang@redhat.com>
---
 tests/generic/638     | 79 +++++++++++++++++++++++++++++++++++++++++++
 tests/generic/638.out |  2 ++
 tests/generic/group   |  1 +
 3 files changed, 82 insertions(+)
 create mode 100755 tests/generic/638
 create mode 100644 tests/generic/638.out

diff --git a/tests/generic/638 b/tests/generic/638
new file mode 100755
index 00000000..74bc39cb
--- /dev/null
+++ b/tests/generic/638
@@ -0,0 +1,79 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2021 Red Hat Inc.  All Rights Reserved.
+#
+# FS QA Test 638
+#
+# Test small swapfile which doesn't contain even a single page-aligned contiguous
+# range of blocks. This case covered commit 5808fecc5723 ("iomap: Fix negative
+# assignment to unsigned sis->pages in iomap_swapfile_activate").
+#
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+. ./common/filter
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+# real QA test starts here
+_supported_fs generic
+_require_scratch
+_require_scratch_swapfile
+_require_test_program mkswap
+_require_test_program swapon
+
+make_unaligned_swapfile()
+{
+	local fname=$1
+	local n=$((psize / bsize - 1))
+
+	# Make sure the swapfile doesn't contain even a single page-aligned
+	# contiguous range of blocks. This's necessary to cover the bug
+	$XFS_IO_PROG -f -t -c "pwrite 0 $(((psize + bsize) * n))" $fname >> $seqres.full 2>&1
+	for((i=1; i<=n; i++));do
+		$XFS_IO_PROG -c "fcollapse $(((psize - bsize) * i)) $bsize" $fname
+	done
+	chmod 0600 $fname
+	$CHATTR_PROG +C $fname > /dev/null 2>&1
+	$here/src/mkswap $fname
+}
+
+_scratch_mkfs >> $seqres.full 2>&1
+_scratch_mount
+psize=`get_page_size`
+bsize=`_get_block_size $SCRATCH_MNT`
+# Due to we need page-unaligned blocks, so blocksize < pagesize is necessary.
+# If not, try to make a smaller enough block size
+if [ $bsize -ge $psize ];then
+	_scratch_unmount
+	_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1
+	if [ $? -ne 0 ];then
+		_notrun "Can't make filesystem block size < page size"
+	fi
+	_scratch_mount
+	bsize=1024
+fi
+swapfile=$SCRATCH_MNT/$seq.swapfile
+make_unaligned_swapfile $swapfile
+$here/src/swapon $swapfile
+swapoff $swapfile
+
+echo "Silence is golden"
+# success, all done
+status=0
+exit
diff --git a/tests/generic/638.out b/tests/generic/638.out
new file mode 100644
index 00000000..3113b1e3
--- /dev/null
+++ b/tests/generic/638.out
@@ -0,0 +1,2 @@
+QA output created by 638
+Silence is golden
diff --git a/tests/generic/group b/tests/generic/group
index b9614c69..5b9b3d1b 100644
--- a/tests/generic/group
+++ b/tests/generic/group
@@ -640,3 +640,4 @@
 635 auto quick atime bigtime shutdown
 636 auto quick swap
 637 auto quick rw
+638 auto quick swap
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS
  2021-05-25  4:46 [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Zorro Lang
  2021-05-25  4:46 ` [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks Zorro Lang
@ 2021-05-25 16:05 ` Darrick J. Wong
  2021-05-30 12:23   ` Eryu Guan
  1 sibling, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2021-05-25 16:05 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests

On Tue, May 25, 2021 at 12:46:41PM +0800, Zorro Lang wrote:
> xfstests set "-bsize=4k" to XFS_MKFS_OPTIONS by default, then give
> it to MKFS_OPTIOPNS. So MKFS_OPTIOPNS always contains "-bsize=4k"
> except we set XFS_MKFS_OPTIONS manually.
> 
> It's useless to set XFS_MKFS_OPTIONS to "-bsize=4096" by default,
> especially that will cause all cases with _scratch_mkfs_blocksized()
> always fail as "-b size option respecified", when test on XFS. For
> exmaple: generic/222
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>

/me wonders what the historical context is (a) for having an
$FSTYP-specific MKFS_OPTIONS variable, and (b) for setting bsize=4096
here.  All I can say is that I've tripped over this numerous times and
wouldn't really mind if it went away. :P

Acked-by: Darrick J. Wong <djwong@kernel.org>

(acked in the sense that I'm not /opposed/ but really really wants to
know the historical reasons)

--D

> ---
>  common/config | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/common/config b/common/config
> index a47e462c..ecd59fae 100644
> --- a/common/config
> +++ b/common/config
> @@ -60,7 +60,6 @@ export EMAIL=root@localhost    # where auto-qa will send its status messages
>  export HOST_OPTIONS=${HOST_OPTIONS:=local.config}
>  export CHECK_OPTIONS=${CHECK_OPTIONS:="-g auto"}
>  export BENCH_PASSES=${BENCH_PASSES:=5}
> -export XFS_MKFS_OPTIONS=${XFS_MKFS_OPTIONS:=-bsize=4096}
>  export TIME_FACTOR=${TIME_FACTOR:=1}
>  export LOAD_FACTOR=${LOAD_FACTOR:=1}
>  export DEBUGFS_MNT=${DEBUGFS_MNT:="/sys/kernel/debug"}
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks
  2021-05-25  4:46 ` [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks Zorro Lang
@ 2021-05-25 16:09   ` Darrick J. Wong
  2021-05-26  4:13     ` Zorro Lang
  0 siblings, 1 reply; 7+ messages in thread
From: Darrick J. Wong @ 2021-05-25 16:09 UTC (permalink / raw)
  To: Zorro Lang; +Cc: fstests

On Tue, May 25, 2021 at 12:46:42PM +0800, Zorro Lang wrote:
> If a swapfile doesn't contain even a single page-aligned contiguous
> range of blocks, it's an invalid swapfile, and might cause kernel
> issue. This case covered commit 5808fecc5723 ("iomap: Fix negative
> assignment to unsigned sis->pages in iomap_swapfile_activate").
> 
> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
>  tests/generic/638     | 79 +++++++++++++++++++++++++++++++++++++++++++
>  tests/generic/638.out |  2 ++
>  tests/generic/group   |  1 +
>  3 files changed, 82 insertions(+)
>  create mode 100755 tests/generic/638
>  create mode 100644 tests/generic/638.out
> 
> diff --git a/tests/generic/638 b/tests/generic/638
> new file mode 100755
> index 00000000..74bc39cb
> --- /dev/null
> +++ b/tests/generic/638
> @@ -0,0 +1,79 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Red Hat Inc.  All Rights Reserved.
> +#
> +# FS QA Test 638
> +#
> +# Test small swapfile which doesn't contain even a single page-aligned contiguous
> +# range of blocks. This case covered commit 5808fecc5723 ("iomap: Fix negative
> +# assignment to unsigned sis->pages in iomap_swapfile_activate").
> +#
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# real QA test starts here
> +_supported_fs generic
> +_require_scratch
> +_require_scratch_swapfile
> +_require_test_program mkswap
> +_require_test_program swapon
> +
> +make_unaligned_swapfile()
> +{
> +	local fname=$1
> +	local n=$((psize / bsize - 1))
> +
> +	# Make sure the swapfile doesn't contain even a single page-aligned
> +	# contiguous range of blocks. This's necessary to cover the bug
> +	$XFS_IO_PROG -f -t -c "pwrite 0 $(((psize + bsize) * n))" $fname >> $seqres.full 2>&1
> +	for((i=1; i<=n; i++));do
> +		$XFS_IO_PROG -c "fcollapse $(((psize - bsize) * i)) $bsize" $fname
> +	done
> +	chmod 0600 $fname
> +	$CHATTR_PROG +C $fname > /dev/null 2>&1
> +	$here/src/mkswap $fname
> +}
> +
> +_scratch_mkfs >> $seqres.full 2>&1
> +_scratch_mount
> +psize=`get_page_size`
> +bsize=`_get_block_size $SCRATCH_MNT`

_get_file_block_size, since fcollapse requires arguments aligned to the
file's allocation unit, which isn't necessarily the same as the fs block
size (bigalloc, rt, etc.)...

> +# Due to we need page-unaligned blocks, so blocksize < pagesize is necessary.
> +# If not, try to make a smaller enough block size
> +if [ $bsize -ge $psize ];then
> +	_scratch_unmount
> +	_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1

...which leads me to the next question, which is: Should the above
helper override mkfs options as necessary to enforce that the allocation
unit is 1024 bytes?  Or is it time for a new helper?

> +	if [ $? -ne 0 ];then
> +		_notrun "Can't make filesystem block size < page size"
> +	fi
> +	_scratch_mount
> +	bsize=1024

You probably want to re-check _get_file_block_size to make sure that you
actually got blocksize < pagesize in the format attempt.

--D

> +fi
> +swapfile=$SCRATCH_MNT/$seq.swapfile
> +make_unaligned_swapfile $swapfile
> +$here/src/swapon $swapfile
> +swapoff $swapfile
> +
> +echo "Silence is golden"
> +# success, all done
> +status=0
> +exit
> diff --git a/tests/generic/638.out b/tests/generic/638.out
> new file mode 100644
> index 00000000..3113b1e3
> --- /dev/null
> +++ b/tests/generic/638.out
> @@ -0,0 +1,2 @@
> +QA output created by 638
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index b9614c69..5b9b3d1b 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -640,3 +640,4 @@
>  635 auto quick atime bigtime shutdown
>  636 auto quick swap
>  637 auto quick rw
> +638 auto quick swap
> -- 
> 2.31.1
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks
  2021-05-25 16:09   ` Darrick J. Wong
@ 2021-05-26  4:13     ` Zorro Lang
  2021-05-27  9:18       ` riteshh
  0 siblings, 1 reply; 7+ messages in thread
From: Zorro Lang @ 2021-05-26  4:13 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: fstests

On Tue, May 25, 2021 at 09:09:07AM -0700, Darrick J. Wong wrote:
> On Tue, May 25, 2021 at 12:46:42PM +0800, Zorro Lang wrote:
> > If a swapfile doesn't contain even a single page-aligned contiguous
> > range of blocks, it's an invalid swapfile, and might cause kernel
> > issue. This case covered commit 5808fecc5723 ("iomap: Fix negative
> > assignment to unsigned sis->pages in iomap_swapfile_activate").
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > ---
> >  tests/generic/638     | 79 +++++++++++++++++++++++++++++++++++++++++++
> >  tests/generic/638.out |  2 ++
> >  tests/generic/group   |  1 +
> >  3 files changed, 82 insertions(+)
> >  create mode 100755 tests/generic/638
> >  create mode 100644 tests/generic/638.out
> > 
> > diff --git a/tests/generic/638 b/tests/generic/638
> > new file mode 100755
> > index 00000000..74bc39cb
> > --- /dev/null
> > +++ b/tests/generic/638
> > @@ -0,0 +1,79 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2021 Red Hat Inc.  All Rights Reserved.
> > +#
> > +# FS QA Test 638
> > +#
> > +# Test small swapfile which doesn't contain even a single page-aligned contiguous
> > +# range of blocks. This case covered commit 5808fecc5723 ("iomap: Fix negative
> > +# assignment to unsigned sis->pages in iomap_swapfile_activate").
> > +#
> > +seq=`basename $0`
> > +seqres=$RESULT_DIR/$seq
> > +echo "QA output created by $seq"
> > +
> > +here=`pwd`
> > +tmp=/tmp/$$
> > +status=1	# failure is the default!
> > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > +
> > +_cleanup()
> > +{
> > +	cd /
> > +	rm -f $tmp.*
> > +}
> > +
> > +# get standard environment, filters and checks
> > +. ./common/rc
> > +. ./common/filter
> > +
> > +# remove previous $seqres.full before test
> > +rm -f $seqres.full
> > +
> > +# real QA test starts here
> > +_supported_fs generic
> > +_require_scratch
> > +_require_scratch_swapfile
> > +_require_test_program mkswap
> > +_require_test_program swapon
> > +
> > +make_unaligned_swapfile()
> > +{
> > +	local fname=$1
> > +	local n=$((psize / bsize - 1))
> > +
> > +	# Make sure the swapfile doesn't contain even a single page-aligned
> > +	# contiguous range of blocks. This's necessary to cover the bug
> > +	$XFS_IO_PROG -f -t -c "pwrite 0 $(((psize + bsize) * n))" $fname >> $seqres.full 2>&1
> > +	for((i=1; i<=n; i++));do
> > +		$XFS_IO_PROG -c "fcollapse $(((psize - bsize) * i)) $bsize" $fname
> > +	done
> > +	chmod 0600 $fname
> > +	$CHATTR_PROG +C $fname > /dev/null 2>&1
> > +	$here/src/mkswap $fname
> > +}
> > +
> > +_scratch_mkfs >> $seqres.full 2>&1
> > +_scratch_mount
> > +psize=`get_page_size`
> > +bsize=`_get_block_size $SCRATCH_MNT`
> 
> _get_file_block_size, since fcollapse requires arguments aligned to the
> file's allocation unit, which isn't necessarily the same as the fs block
> size (bigalloc, rt, etc.)...

You're right.

> 
> > +# Due to we need page-unaligned blocks, so blocksize < pagesize is necessary.
> > +# If not, try to make a smaller enough block size
> > +if [ $bsize -ge $psize ];then
> > +	_scratch_unmount
> > +	_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1
> 
> ...which leads me to the next question, which is: Should the above
> helper override mkfs options as necessary to enforce that the allocation
> unit is 1024 bytes?  Or is it time for a new helper?

Hmm... the _scratch_mkfs_blocksized() does:

    xfs)
        _scratch_mkfs_xfs $MKFS_OPTIONS -b size=$blocksize

_scratch_mkfs_xfs() already use $MKFS_OPTIONS internally, don't need to give
$MKFS_OPTIONS to it again. Although _scratch_mkfs_xfs deals with $MKFS_OPTIONS,
it just drops whole $MKFS_OPTIONS if there're conflicts.

The _scratch_mkfs_geom() does overwrite, but _scratch_mkfs_sized doesn't (sized
filesystem is rarely seen in local.config). We might let _scratch_mkfs_blocksized
learn _scratch_mkfs_geom.

> 
> > +	if [ $? -ne 0 ];then
> > +		_notrun "Can't make filesystem block size < page size"
> > +	fi
> > +	_scratch_mount
> > +	bsize=1024
> 
> You probably want to re-check _get_file_block_size to make sure that you
> actually got blocksize < pagesize in the format attempt.

Yes, you're right. _scratch_mkfs_blocksized can't make sure to get that blocksize.

Thanks,
Zorro

> 
> --D
> 
> > +fi
> > +swapfile=$SCRATCH_MNT/$seq.swapfile
> > +make_unaligned_swapfile $swapfile
> > +$here/src/swapon $swapfile
> > +swapoff $swapfile
> > +
> > +echo "Silence is golden"
> > +# success, all done
> > +status=0
> > +exit
> > diff --git a/tests/generic/638.out b/tests/generic/638.out
> > new file mode 100644
> > index 00000000..3113b1e3
> > --- /dev/null
> > +++ b/tests/generic/638.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 638
> > +Silence is golden
> > diff --git a/tests/generic/group b/tests/generic/group
> > index b9614c69..5b9b3d1b 100644
> > --- a/tests/generic/group
> > +++ b/tests/generic/group
> > @@ -640,3 +640,4 @@
> >  635 auto quick atime bigtime shutdown
> >  636 auto quick swap
> >  637 auto quick rw
> > +638 auto quick swap
> > -- 
> > 2.31.1
> > 
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks
  2021-05-26  4:13     ` Zorro Lang
@ 2021-05-27  9:18       ` riteshh
  0 siblings, 0 replies; 7+ messages in thread
From: riteshh @ 2021-05-27  9:18 UTC (permalink / raw)
  To: Darrick J. Wong, fstests

On 21/05/26 12:13PM, Zorro Lang wrote:
> On Tue, May 25, 2021 at 09:09:07AM -0700, Darrick J. Wong wrote:
> > On Tue, May 25, 2021 at 12:46:42PM +0800, Zorro Lang wrote:
> > > If a swapfile doesn't contain even a single page-aligned contiguous
> > > range of blocks, it's an invalid swapfile, and might cause kernel
> > > issue. This case covered commit 5808fecc5723 ("iomap: Fix negative
> > > assignment to unsigned sis->pages in iomap_swapfile_activate").
> > >
> > > Signed-off-by: Zorro Lang <zlang@redhat.com>
> > > ---
> > >  tests/generic/638     | 79 +++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/638.out |  2 ++
> > >  tests/generic/group   |  1 +
> > >  3 files changed, 82 insertions(+)
> > >  create mode 100755 tests/generic/638
> > >  create mode 100644 tests/generic/638.out
> > >
> > > diff --git a/tests/generic/638 b/tests/generic/638
> > > new file mode 100755
> > > index 00000000..74bc39cb
> > > --- /dev/null
> > > +++ b/tests/generic/638
> > > @@ -0,0 +1,79 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2021 Red Hat Inc.  All Rights Reserved.
> > > +#
> > > +# FS QA Test 638
> > > +#
> > > +# Test small swapfile which doesn't contain even a single page-aligned contiguous
> > > +# range of blocks. This case covered commit 5808fecc5723 ("iomap: Fix negative
> > > +# assignment to unsigned sis->pages in iomap_swapfile_activate").
> > > +#
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=1	# failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > +	cd /
> > > +	rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# real QA test starts here
> > > +_supported_fs generic
> > > +_require_scratch
> > > +_require_scratch_swapfile
> > > +_require_test_program mkswap
> > > +_require_test_program swapon
> > > +
> > > +make_unaligned_swapfile()
> > > +{
> > > +	local fname=$1
> > > +	local n=$((psize / bsize - 1))
> > > +
> > > +	# Make sure the swapfile doesn't contain even a single page-aligned
> > > +	# contiguous range of blocks. This's necessary to cover the bug
> > > +	$XFS_IO_PROG -f -t -c "pwrite 0 $(((psize + bsize) * n))" $fname >> $seqres.full 2>&1
> > > +	for((i=1; i<=n; i++));do
> > > +		$XFS_IO_PROG -c "fcollapse $(((psize - bsize) * i)) $bsize" $fname
> > > +	done
> > > +	chmod 0600 $fname
> > > +	$CHATTR_PROG +C $fname > /dev/null 2>&1
> > > +	$here/src/mkswap $fname
> > > +}
> > > +
> > > +_scratch_mkfs >> $seqres.full 2>&1
> > > +_scratch_mount
> > > +psize=`get_page_size`
> > > +bsize=`_get_block_size $SCRATCH_MNT`
> >
> > _get_file_block_size, since fcollapse requires arguments aligned to the
> > file's allocation unit, which isn't necessarily the same as the fs block
> > size (bigalloc, rt, etc.)...
>
> You're right.
>
> >
> > > +# Due to we need page-unaligned blocks, so blocksize < pagesize is necessary.
> > > +# If not, try to make a smaller enough block size
> > > +if [ $bsize -ge $psize ];then
> > > +	_scratch_unmount
> > > +	_scratch_mkfs_blocksized 1024 >> $seqres.full 2>&1
> >
> > ...which leads me to the next question, which is: Should the above
> > helper override mkfs options as necessary to enforce that the allocation
> > unit is 1024 bytes?  Or is it time for a new helper?
>
> Hmm... the _scratch_mkfs_blocksized() does:
>
>     xfs)
>         _scratch_mkfs_xfs $MKFS_OPTIONS -b size=$blocksize
>
> _scratch_mkfs_xfs() already use $MKFS_OPTIONS internally, don't need to give
> $MKFS_OPTIONS to it again. Although _scratch_mkfs_xfs deals with $MKFS_OPTIONS,
> it just drops whole $MKFS_OPTIONS if there're conflicts.
>
> The _scratch_mkfs_geom() does overwrite, but _scratch_mkfs_sized doesn't (sized
> filesystem is rarely seen in local.config). We might let _scratch_mkfs_blocksized
> learn _scratch_mkfs_geom.
>
> >
> > > +	if [ $? -ne 0 ];then
> > > +		_notrun "Can't make filesystem block size < page size"
> > > +	fi
> > > +	_scratch_mount
> > > +	bsize=1024
> >
> > You probably want to re-check _get_file_block_size to make sure that you
> > actually got blocksize < pagesize in the format attempt.
>
> Yes, you're right. _scratch_mkfs_blocksized can't make sure to get that blocksize.
>

Sorry, about delay in getting a test ready for this kernel bug. Also I only now
noticed this patch of yours, after I had sent a patch generic/639 to cover this
bug. (sheer coincidence).

I have tested [1] on both x86 and Power platform for all filesystems config.
(ext4-1k, ext4-4k. ext4-64k, ext2, xfs-1k, xfs-4k, xfs-64k, btrfs) (64k if for Power)
But I am fine if you would like to push your patch upstream.

[1]: https://patchwork.kernel.org/project/fstests/patch/e1f9798462ef60648db24b6291e1b149b114f2f2.1622105066.git.riteshh@linux.ibm.com/

-ritesh

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS
  2021-05-25 16:05 ` [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Darrick J. Wong
@ 2021-05-30 12:23   ` Eryu Guan
  0 siblings, 0 replies; 7+ messages in thread
From: Eryu Guan @ 2021-05-30 12:23 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Zorro Lang, fstests

On Tue, May 25, 2021 at 09:05:07AM -0700, Darrick J. Wong wrote:
> On Tue, May 25, 2021 at 12:46:41PM +0800, Zorro Lang wrote:
> > xfstests set "-bsize=4k" to XFS_MKFS_OPTIONS by default, then give
> > it to MKFS_OPTIOPNS. So MKFS_OPTIOPNS always contains "-bsize=4k"
> > except we set XFS_MKFS_OPTIONS manually.
> > 
> > It's useless to set XFS_MKFS_OPTIONS to "-bsize=4096" by default,
> > especially that will cause all cases with _scratch_mkfs_blocksized()
> > always fail as "-b size option respecified", when test on XFS. For
> > exmaple: generic/222
> > 
> > Signed-off-by: Zorro Lang <zlang@redhat.com>
> 
> /me wonders what the historical context is (a) for having an
> $FSTYP-specific MKFS_OPTIONS variable, and (b) for setting bsize=4096
> here.  All I can say is that I've tripped over this numerous times and
> wouldn't really mind if it went away. :P
> 
> Acked-by: Darrick J. Wong <djwong@kernel.org>
> 
> (acked in the sense that I'm not /opposed/ but really really wants to
> know the historical reasons)

Looked at the git log, and it was added in 2004 without any
explaination. I'll let the patch pending for another week, to see if
anyone knows the history behind this.

commit cb6beb975950121f0569c5dede6136fb8ae0b8b8
Author: ptools <ptools>
Date:   Tue Jun 15 07:32:36 2004 +0000

    Fixed merge problems

Thanks,
Eryu

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-30 12:23 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25  4:46 [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Zorro Lang
2021-05-25  4:46 ` [PATCH 2/2] generic: test small swapfile without page-aligned contiguous blocks Zorro Lang
2021-05-25 16:09   ` Darrick J. Wong
2021-05-26  4:13     ` Zorro Lang
2021-05-27  9:18       ` riteshh
2021-05-25 16:05 ` [PATCH 1/2] common/config: remove default 4k blocksize from XFS_MKFS_OPTIONS Darrick J. Wong
2021-05-30 12:23   ` Eryu Guan

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.