[PATCH v2 0/6] fstests: fix _scratch_mkfs_sized failure handling

[PATCH v2 0/6] fstests: fix _scratch_mkfs_sized failure handling

[Shin'ichiro Kawasaki]

When generic/204 is run for btrfs-zoned filesystem on zoned block devices with
GB size order, it takes very long time to complete. The test case creates 115MiB
filesystem on the scratch device and fills files in it within decent run time.
However, with btrfs-zoned condition, the test case creates filesystem as large
as the device size and it takes very long time to fill it all. Three causes were
identified for the long run time, and this series addresses them.

The first cause is mixed mode option that _scratch_mkfs_sized helper function
adds to mkfs.btrfs. This option was added for both regular btrfs and
zoned-btrfs. However, zoned-btrfs does not support mixed mode. The mkfs with
mixed mode fails and results in _scratch_mkfs_sized failure. The mixed mode
shall not be specified for btrfs-zoned filesystem.

The second cause is unnecessary call of the _scratch_mkfs helper function in the
test case generic/204. This helper function is called to obtain data block size
and i-node size. However, these numbers can be obtained from _scratch_mkfs_sized
call. The _scratch_mkfs function call shall be removed.

The third cause is no check of return code from _scratch_mkfs_sized. The test
case generic/204 calls both _scratch_mkfs and _scratch_mkfs_sized, and does not
check return code from them. If _scratch_mkfs succeeds and _scratch_mkfs_sized
fails, the scratch device still has valid filesystem created by _scratch_mkfs.
Following test workload can be executed without failure, but the filesystem
does not have the size specified for _scratch_mkfs_sized. The return code of
_scratch_mkfs_sized shall be checked to catch the mkfs failure. This problem
exists not only in generic/204, but also in other test cases which call both
_scratch_mkfs and _scratch_mkfs_sized.

In this series, the first patch addresses the first cause, and the second patch
addresses the second cause. These two patches fix the test case generic/204.
Following three patches address the third cause, and fix other test cases than
generic/204.

The last patch is an additional clean up of the helper function _filter_mkfs.
During this fix work, it was misunderstood that this function were xfs unique.
To clarify it can be extended to other filesystems, factor out xfs unique part.

Changes from v1:
* Added 2nd patch which removes _scratch_mkfs call from generic/204
* Added 6th patch which factors out xfs unique part from _filter_mkfs
* Dropped 3 patches which had renamed _filter_mkfs to _xfs_filter_mkfs
* Dropped generic/204 hunk from the 3rd patch

Shin'ichiro Kawasaki (6):
  common/rc: fix btrfs mixed mode usage in _scratch_mkfs_sized
  generic/204: remove unnecessary _scratch_mkfs call
  generic/{171,172,173,174}: check _scratch_mkfs_sized return code
  ext4/021: check _scratch_mkfs_sized return code
  xfs/015: check _scratch_mkfs_sized return code
  common: factor out xfs unique part from _filter_mkfs

 common/filter     | 40 +---------------------------------------
 common/rc         |  8 ++++----
 common/xfs        | 41 +++++++++++++++++++++++++++++++++++++++++
 tests/ext4/021    |  2 +-
 tests/generic/171 |  2 +-
 tests/generic/172 |  2 +-
 tests/generic/173 |  2 +-
 tests/generic/174 |  2 +-
 tests/generic/204 |  6 +-----
 tests/xfs/015     |  2 +-
 10 files changed, 53 insertions(+), 54 deletions(-)

-- 
2.34.1

[PATCH v2 1/6] common/rc: fix btrfs mixed mode usage in _scratch_mkfs_sized

[Shin'ichiro Kawasaki]

The helper function _scratch_mkfs_sized needs a couple of improvements
for btrfs. At first, the function adds --mixed option to mkfs.btrfs when
the filesystem size is smaller then 256MiB, but this threshold is no
longer correct and it should be 109MiB. Secondly, the --mixed option
shall not be specified to mkfs.btrfs for zoned devices, since zoned
devices does not allow mixing metadata blocks and data blocks.

Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/rc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index b3289de9..eb2493d1 100644
--- a/common/rc
+++ b/common/rc
@@ -1075,10 +1075,10 @@ _scratch_mkfs_sized()
 		;;
 	btrfs)
 		local mixed_opt=
-		# minimum size that's needed without the mixed option.
-		# Ref: btrfs-prog: btrfs_min_dev_size()
-		# Non mixed mode is also the default option.
-		(( fssize < $((256 * 1024 *1024)) )) && mixed_opt='--mixed'
+		# Mixed option is required when the filesystem size is small and
+		# the device is not zoned. Ref: btrfs-progs: btrfs_min_dev_size()
+		(( fssize < $((109 * 1024 * 1024)) )) &&
+			! _scratch_btrfs_is_zoned && mixed_opt='--mixed'
 		$MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
 		;;
 	jfs)
-- 
2.34.1

[PATCH v2 2/6] generic/204: remove unnecessary _scratch_mkfs call

[Shin'ichiro Kawasaki]

The test case generic/204 calls _scratch_mkfs to get data block size and
i-node size of the filesystem and obtained data block size is passed to
the following _scratch_mfks_sized call as an option. However, the
_scratch_mkfs call is unnecessary since the sizes can be obtained by
_scratch_mkfs_sized call without the data block size option.

Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds
and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs
leaves valid working filesystem on scratch device then following mount
and IO operations can not detect the failure of _scratch_mkfs_sized.
This results in the test case run with unexpected test condition.

Hence, remove the _scratch_mkfs call and the data block size option for
_scratch_mkfs_sized call.

Suggested-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/generic/204 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/tests/generic/204 b/tests/generic/204
index a3dabb71..a33a090f 100755
--- a/tests/generic/204
+++ b/tests/generic/204
@@ -24,10 +24,6 @@ _supported_fs generic
 
 _require_scratch
 
-# get the block size first
-_scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null
-. $tmp.mkfs
-
 # For xfs, we need to handle the different default log sizes that different
 # versions of mkfs create. All should be valid with a 16MB log, so use that.
 # And v4/512 v5/1k xfs don't have enough free inodes, set imaxpct=50 at mkfs
@@ -35,7 +31,7 @@ _scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null
 [ $FSTYP = "xfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -l size=16m -i maxpct=50"
 
 SIZE=`expr 115 \* 1024 \* 1024`
-_scratch_mkfs_sized $SIZE $dbsize 2> /dev/null > $tmp.mkfs.raw
+_scratch_mkfs_sized $SIZE 2> /dev/null > $tmp.mkfs.raw
 cat $tmp.mkfs.raw | _filter_mkfs 2> $tmp.mkfs > /dev/null
 _scratch_mount
 
-- 
2.34.1

[PATCH v2 3/6] generic/{171,172,173,174}: check _scratch_mkfs_sized return code

[Shin'ichiro Kawasaki]

The test cases generic/{171,172,173,174} call _scratch_mkfs before
_scratch_mkfs_sized, and they do not check return code of
_scratch_mkfs_sized. Even if _scratch_mkfs_sized failed, _scratch_mount
after it cannot detect the sized mkfs failure because _scratch_mkfs
already created a file system on the device. This results in unexpected
test condition of the test cases.

To avoid the unexpected test condition, check return code of
_scratch_mkfs_sized in the test cases.

Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/generic/171 | 2 +-
 tests/generic/172 | 2 +-
 tests/generic/173 | 2 +-
 tests/generic/174 | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/generic/171 b/tests/generic/171
index fb2a6f14..f823a454 100755
--- a/tests/generic/171
+++ b/tests/generic/171
@@ -42,7 +42,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
 if [ $sz_bytes -lt $((32 * 1048576)) ]; then
 	sz_bytes=$((32 * 1048576))
 fi
-_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
+_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
 _scratch_mount >> $seqres.full 2>&1
 rm -rf $testdir
 mkdir $testdir
diff --git a/tests/generic/172 b/tests/generic/172
index ab5122fa..383824b9 100755
--- a/tests/generic/172
+++ b/tests/generic/172
@@ -40,7 +40,7 @@ umount $SCRATCH_MNT
 
 file_size=$((768 * 1024 * 1024))
 fs_size=$((1024 * 1024 * 1024))
-_scratch_mkfs_sized $fs_size >> $seqres.full 2>&1
+_scratch_mkfs_sized $fs_size >> $seqres.full 2>&1 || _fail "mkfs failed"
 _scratch_mount >> $seqres.full 2>&1
 rm -rf $testdir
 mkdir $testdir
diff --git a/tests/generic/173 b/tests/generic/173
index 0eb313e2..e1493278 100755
--- a/tests/generic/173
+++ b/tests/generic/173
@@ -42,7 +42,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
 if [ $sz_bytes -lt $((32 * 1048576)) ]; then
 	sz_bytes=$((32 * 1048576))
 fi
-_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
+_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
 _scratch_mount >> $seqres.full 2>&1
 rm -rf $testdir
 mkdir $testdir
diff --git a/tests/generic/174 b/tests/generic/174
index 1505453e..c7a177b8 100755
--- a/tests/generic/174
+++ b/tests/generic/174
@@ -43,7 +43,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
 if [ $sz_bytes -lt $((32 * 1048576)) ]; then
 	sz_bytes=$((32 * 1048576))
 fi
-_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
+_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
 _scratch_mount >> $seqres.full 2>&1
 rm -rf $testdir
 mkdir $testdir
-- 
2.34.1

[PATCH v2 4/6] ext4/021: check _scratch_mkfs_sized return code

[Shin'ichiro Kawasaki]

The test cases ext4/021 calls _scratch_mkfs before _scratch_mkfs_sized,
and does not check return code of _scratch_mkfs_sized. Even if
_scratch_mkfs_sized failed, _scratch_mount after it cannot detect the
sized mkfs failure because _scratch_mkfs already created a file system
on the device. This results in unexpected test condition.

To avoid the unexpected test condition, check return code of
_scratch_mkfs_sized.

Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 tests/ext4/021 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/ext4/021 b/tests/ext4/021
index 62768c60..a9277abf 100755
--- a/tests/ext4/021
+++ b/tests/ext4/021
@@ -24,7 +24,7 @@ _scratch_unmount
 
 # With 4k block size, this amounts to 10M FS instance.
 fssize=$((2560 * $blocksize))
-_scratch_mkfs_sized $fssize >> $seqres.full 2>&1
+_scratch_mkfs_sized $fssize >> $seqres.full 2>&1 || _fail "mkfs failed"
 _require_metadata_journaling $SCRATCH_DEV
 
 offset=0
-- 
2.34.1

[PATCH v2 5/6] xfs/015: check _scratch_mkfs_sized return code

[Shin'ichiro Kawasaki]

The test cases xfs/015 calls _scratch_mkfs before _scratch_mkfs_sized,
and does not check return code of _scratch_mkfs_sized. Even if
_scratch_mkfs_sized failed, _scratch_mount after it cannot detect the
sized mkfs failure because _scratch_mkfs already created a file system
on the device. This results in unexpected test condition.

To avoid the unexpected test condition, check return code of
_scratch_mkfs_sized.

Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/015 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/xfs/015 b/tests/xfs/015
index 86fa6336..2bb7b8d5 100755
--- a/tests/xfs/015
+++ b/tests/xfs/015
@@ -43,7 +43,7 @@ _scratch_mount
 _require_fs_space $SCRATCH_MNT 131072
 _scratch_unmount
 
-_scratch_mkfs_sized $((32 * 1024 * 1024)) > $tmp.mkfs.raw
+_scratch_mkfs_sized $((32 * 1024 * 1024)) > $tmp.mkfs.raw || _fail "mkfs failed"
 cat $tmp.mkfs.raw | _filter_mkfs >$seqres.full 2>$tmp.mkfs
 # get original data blocks number and agcount
 . $tmp.mkfs
-- 
2.34.1

[PATCH v2 6/6] common: factor out xfs unique part from _filter_mkfs

[Shin'ichiro Kawasaki]

Most of the code in the function _filter_mkfs is xfs unique. This is
misleading that the function would be dedicated for xfs. Clean up the
function by factoring out xfs unique part to _xfs_filter_mkfs in
common/xfs. While at the same time, fix indent in _xfs_filter_mkfs to be
consistent with other functions in common/xfs.

Suggested-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
---
 common/filter | 40 +---------------------------------------
 common/xfs    | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 39 deletions(-)

diff --git a/common/filter b/common/filter
index c3db7a56..257227c2 100644
--- a/common/filter
+++ b/common/filter
@@ -121,53 +121,15 @@ _filter_mkfs()
 {
     case $FSTYP in
     xfs)
+	_xfs_filter_mkfs "$@"
 	;;
     *)
 	cat - >/dev/null
 	perl -e 'print STDERR "dbsize=4096\nisize=256\n"'
 	return ;;
     esac
-
-    echo "_fs_has_crcs=0" >&2
-    set -
-    perl -ne '
-    if (/^meta-data=([\w,|\/.-]+)\s+isize=(\d+)\s+agcount=(\d+), agsize=(\d+) blks/) {
-	print STDERR "ddev=$1\nisize=$2\nagcount=$3\nagsize=$4\n";
-	print STDOUT "meta-data=DDEV isize=XXX agcount=N, agsize=XXX blks\n";
-    }
-    if (/^\s+=\s+sectsz=(\d+)\s+attr=(\d+)/) {
-        print STDERR "sectsz=$1\nattr=$2\n";
-    }
-    if (/^\s+=\s+crc=(\d)/) {
-        print STDERR "_fs_has_crcs=$1\n";
-    }
-    if (/^data\s+=\s+bsize=(\d+)\s+blocks=(\d+), imaxpct=(\d+)/) {
-	print STDERR "dbsize=$1\ndblocks=$2\nimaxpct=$3\n";
-	print STDOUT "data     = bsize=XXX blocks=XXX, imaxpct=PCT\n";
-    }
-    if (/^\s+=\s+sunit=(\d+)\s+swidth=(\d+) blks/) {
-        print STDERR "sunit=$1\nswidth=$2\nunwritten=1\n";
-	print STDOUT "         = sunit=XXX swidth=XXX, unwritten=X\n";
-    }
-    if (/^naming\s+=version\s+(\d+)\s+bsize=(\d+)/) {
-	print STDERR "dirversion=$1\ndirbsize=$2\n";
-	print STDOUT "naming   =VERN bsize=XXX\n";
-    }
-    if (/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+),\s+version=(\d+)/ ||
-	/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+)/) {
-	print STDERR "ldev=\"$1\"\nlbsize=$2\nlblocks=$3\nlversion=$4\n";
-	print STDOUT "log      =LDEV bsize=XXX blocks=XXX\n";
-    }
-    if (/^\s+=\s+sectsz=(\d+)\s+sunit=(\d+) blks/) {
-	print STDERR "logsectsz=$1\nlogsunit=$2\n\n";
-    }
-    if (/^realtime\s+=([\w|\/.-]+)\s+extsz=(\d+)\s+blocks=(\d+), rtextents=(\d+)/) {
-	print STDERR "rtdev=$1\nrtextsz=$2\nrtblocks=$3\nrtextents=$4\n";
-	print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
-    }'
 }
 
-
 # prints the bits we care about in growfs
 # 
 _filter_growfs()
diff --git a/common/xfs b/common/xfs
index 713e9fe7..053b6189 100644
--- a/common/xfs
+++ b/common/xfs
@@ -1275,3 +1275,44 @@ _require_scratch_xfs_bigtime()
 		_notrun "bigtime feature not advertised on mount?"
 	_scratch_unmount
 }
+
+_xfs_filter_mkfs()
+{
+	echo "_fs_has_crcs=0" >&2
+	set -
+	perl -ne '
+	if (/^meta-data=([\w,|\/.-]+)\s+isize=(\d+)\s+agcount=(\d+), agsize=(\d+) blks/) {
+		print STDERR "ddev=$1\nisize=$2\nagcount=$3\nagsize=$4\n";
+		print STDOUT "meta-data=DDEV isize=XXX agcount=N, agsize=XXX blks\n";
+	}
+	if (/^\s+=\s+sectsz=(\d+)\s+attr=(\d+)/) {
+		print STDERR "sectsz=$1\nattr=$2\n";
+	}
+	if (/^\s+=\s+crc=(\d)/) {
+		print STDERR "_fs_has_crcs=$1\n";
+	}
+	if (/^data\s+=\s+bsize=(\d+)\s+blocks=(\d+), imaxpct=(\d+)/) {
+		print STDERR "dbsize=$1\ndblocks=$2\nimaxpct=$3\n";
+		print STDOUT "data     = bsize=XXX blocks=XXX, imaxpct=PCT\n";
+	}
+	if (/^\s+=\s+sunit=(\d+)\s+swidth=(\d+) blks/) {
+		print STDERR "sunit=$1\nswidth=$2\nunwritten=1\n";
+		print STDOUT "         = sunit=XXX swidth=XXX, unwritten=X\n";
+	}
+	if (/^naming\s+=version\s+(\d+)\s+bsize=(\d+)/) {
+		print STDERR "dirversion=$1\ndirbsize=$2\n";
+		print STDOUT "naming   =VERN bsize=XXX\n";
+	}
+	if (/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+),\s+version=(\d+)/ ||
+		/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+)/) {
+		print STDERR "ldev=\"$1\"\nlbsize=$2\nlblocks=$3\nlversion=$4\n";
+		print STDOUT "log      =LDEV bsize=XXX blocks=XXX\n";
+	}
+	if (/^\s+=\s+sectsz=(\d+)\s+sunit=(\d+) blks/) {
+		print STDERR "logsectsz=$1\nlogsunit=$2\n\n";
+	}
+	if (/^realtime\s+=([\w|\/.-]+)\s+extsz=(\d+)\s+blocks=(\d+), rtextents=(\d+)/) {
+		print STDERR "rtdev=$1\nrtextsz=$2\nrtblocks=$3\nrtextents=$4\n";
+		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
+	}'
+}
-- 
2.34.1

Re: [PATCH v2 2/6] generic/204: remove unnecessary _scratch_mkfs call

[Darrick J. Wong]

On Wed, Feb 09, 2022 at 09:33:01PM +0900, Shin'ichiro Kawasaki wrote:
> The test case generic/204 calls _scratch_mkfs to get data block size and
> i-node size of the filesystem and obtained data block size is passed to
> the following _scratch_mfks_sized call as an option. However, the
> _scratch_mkfs call is unnecessary since the sizes can be obtained by
> _scratch_mkfs_sized call without the data block size option.
> 
> Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds
> and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs
> leaves valid working filesystem on scratch device then following mount
> and IO operations can not detect the failure of _scratch_mkfs_sized.
> This results in the test case run with unexpected test condition.
> 
> Hence, remove the _scratch_mkfs call and the data block size option for
> _scratch_mkfs_sized call.
> 
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Looks ok, assuming you've verified that fstests with FSTYP=xfs doesn't
regress...

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

--D

> ---
>  tests/generic/204 | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/tests/generic/204 b/tests/generic/204
> index a3dabb71..a33a090f 100755
> --- a/tests/generic/204
> +++ b/tests/generic/204
> @@ -24,10 +24,6 @@ _supported_fs generic
>  
>  _require_scratch
>  
> -# get the block size first
> -_scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null
> -. $tmp.mkfs
> -
>  # For xfs, we need to handle the different default log sizes that different
>  # versions of mkfs create. All should be valid with a 16MB log, so use that.
>  # And v4/512 v5/1k xfs don't have enough free inodes, set imaxpct=50 at mkfs
> @@ -35,7 +31,7 @@ _scratch_mkfs 2> /dev/null | _filter_mkfs 2> $tmp.mkfs > /dev/null
>  [ $FSTYP = "xfs" ] && MKFS_OPTIONS="$MKFS_OPTIONS -l size=16m -i maxpct=50"
>  
>  SIZE=`expr 115 \* 1024 \* 1024`
> -_scratch_mkfs_sized $SIZE $dbsize 2> /dev/null > $tmp.mkfs.raw
> +_scratch_mkfs_sized $SIZE 2> /dev/null > $tmp.mkfs.raw
>  cat $tmp.mkfs.raw | _filter_mkfs 2> $tmp.mkfs > /dev/null
>  _scratch_mount
>  
> -- 
> 2.34.1
> 

Re: [PATCH v2 3/6] generic/{171,172,173,174}: check _scratch_mkfs_sized return code

[Darrick J. Wong]

On Wed, Feb 09, 2022 at 09:33:02PM +0900, Shin'ichiro Kawasaki wrote:
> The test cases generic/{171,172,173,174} call _scratch_mkfs before
> _scratch_mkfs_sized, and they do not check return code of
> _scratch_mkfs_sized. Even if _scratch_mkfs_sized failed, _scratch_mount
> after it cannot detect the sized mkfs failure because _scratch_mkfs
> already created a file system on the device. This results in unexpected
> test condition of the test cases.
> 
> To avoid the unexpected test condition, check return code of
> _scratch_mkfs_sized in the test cases.
> 
> Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/generic/171 | 2 +-
>  tests/generic/172 | 2 +-
>  tests/generic/173 | 2 +-
>  tests/generic/174 | 2 +-
>  4 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/generic/171 b/tests/generic/171
> index fb2a6f14..f823a454 100755
> --- a/tests/generic/171
> +++ b/tests/generic/171
> @@ -42,7 +42,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
>  if [ $sz_bytes -lt $((32 * 1048576)) ]; then
>  	sz_bytes=$((32 * 1048576))
>  fi
> -_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
> +_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
>  _scratch_mount >> $seqres.full 2>&1
>  rm -rf $testdir
>  mkdir $testdir
> diff --git a/tests/generic/172 b/tests/generic/172
> index ab5122fa..383824b9 100755
> --- a/tests/generic/172
> +++ b/tests/generic/172
> @@ -40,7 +40,7 @@ umount $SCRATCH_MNT
>  
>  file_size=$((768 * 1024 * 1024))
>  fs_size=$((1024 * 1024 * 1024))
> -_scratch_mkfs_sized $fs_size >> $seqres.full 2>&1
> +_scratch_mkfs_sized $fs_size >> $seqres.full 2>&1 || _fail "mkfs failed"
>  _scratch_mount >> $seqres.full 2>&1
>  rm -rf $testdir
>  mkdir $testdir
> diff --git a/tests/generic/173 b/tests/generic/173
> index 0eb313e2..e1493278 100755
> --- a/tests/generic/173
> +++ b/tests/generic/173
> @@ -42,7 +42,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
>  if [ $sz_bytes -lt $((32 * 1048576)) ]; then
>  	sz_bytes=$((32 * 1048576))
>  fi
> -_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
> +_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
>  _scratch_mount >> $seqres.full 2>&1
>  rm -rf $testdir
>  mkdir $testdir
> diff --git a/tests/generic/174 b/tests/generic/174
> index 1505453e..c7a177b8 100755
> --- a/tests/generic/174
> +++ b/tests/generic/174
> @@ -43,7 +43,7 @@ sz_bytes=$((nr_blks * 8 * blksz))
>  if [ $sz_bytes -lt $((32 * 1048576)) ]; then
>  	sz_bytes=$((32 * 1048576))
>  fi
> -_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1
> +_scratch_mkfs_sized $sz_bytes >> $seqres.full 2>&1 || _fail "mkfs failed"
>  _scratch_mount >> $seqres.full 2>&1
>  rm -rf $testdir
>  mkdir $testdir
> -- 
> 2.34.1
> 

Re: [PATCH v2 4/6] ext4/021: check _scratch_mkfs_sized return code

[Darrick J. Wong]

On Wed, Feb 09, 2022 at 09:33:03PM +0900, Shin'ichiro Kawasaki wrote:
> The test cases ext4/021 calls _scratch_mkfs before _scratch_mkfs_sized,
> and does not check return code of _scratch_mkfs_sized. Even if
> _scratch_mkfs_sized failed, _scratch_mount after it cannot detect the
> sized mkfs failure because _scratch_mkfs already created a file system
> on the device. This results in unexpected test condition.
> 
> To avoid the unexpected test condition, check return code of
> _scratch_mkfs_sized.
> 
> Suggested-by: Naohiro Aota <naohiro.aota@wdc.com>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

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

--D

> ---
>  tests/ext4/021 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/ext4/021 b/tests/ext4/021
> index 62768c60..a9277abf 100755
> --- a/tests/ext4/021
> +++ b/tests/ext4/021
> @@ -24,7 +24,7 @@ _scratch_unmount
>  
>  # With 4k block size, this amounts to 10M FS instance.
>  fssize=$((2560 * $blocksize))
> -_scratch_mkfs_sized $fssize >> $seqres.full 2>&1
> +_scratch_mkfs_sized $fssize >> $seqres.full 2>&1 || _fail "mkfs failed"
>  _require_metadata_journaling $SCRATCH_DEV
>  
>  offset=0
> -- 
> 2.34.1
> 

Re: [PATCH v2 6/6] common: factor out xfs unique part from _filter_mkfs

[Darrick J. Wong]

On Wed, Feb 09, 2022 at 09:33:05PM +0900, Shin'ichiro Kawasaki wrote:
> Most of the code in the function _filter_mkfs is xfs unique. This is
> misleading that the function would be dedicated for xfs. Clean up the
> function by factoring out xfs unique part to _xfs_filter_mkfs in
> common/xfs. While at the same time, fix indent in _xfs_filter_mkfs to be
> consistent with other functions in common/xfs.
> 
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>

Thanks!!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/filter | 40 +---------------------------------------
>  common/xfs    | 41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 39 deletions(-)
> 
> diff --git a/common/filter b/common/filter
> index c3db7a56..257227c2 100644
> --- a/common/filter
> +++ b/common/filter
> @@ -121,53 +121,15 @@ _filter_mkfs()
>  {
>      case $FSTYP in
>      xfs)
> +	_xfs_filter_mkfs "$@"
>  	;;
>      *)
>  	cat - >/dev/null
>  	perl -e 'print STDERR "dbsize=4096\nisize=256\n"'
>  	return ;;
>      esac
> -
> -    echo "_fs_has_crcs=0" >&2
> -    set -
> -    perl -ne '
> -    if (/^meta-data=([\w,|\/.-]+)\s+isize=(\d+)\s+agcount=(\d+), agsize=(\d+) blks/) {
> -	print STDERR "ddev=$1\nisize=$2\nagcount=$3\nagsize=$4\n";
> -	print STDOUT "meta-data=DDEV isize=XXX agcount=N, agsize=XXX blks\n";
> -    }
> -    if (/^\s+=\s+sectsz=(\d+)\s+attr=(\d+)/) {
> -        print STDERR "sectsz=$1\nattr=$2\n";
> -    }
> -    if (/^\s+=\s+crc=(\d)/) {
> -        print STDERR "_fs_has_crcs=$1\n";
> -    }
> -    if (/^data\s+=\s+bsize=(\d+)\s+blocks=(\d+), imaxpct=(\d+)/) {
> -	print STDERR "dbsize=$1\ndblocks=$2\nimaxpct=$3\n";
> -	print STDOUT "data     = bsize=XXX blocks=XXX, imaxpct=PCT\n";
> -    }
> -    if (/^\s+=\s+sunit=(\d+)\s+swidth=(\d+) blks/) {
> -        print STDERR "sunit=$1\nswidth=$2\nunwritten=1\n";
> -	print STDOUT "         = sunit=XXX swidth=XXX, unwritten=X\n";
> -    }
> -    if (/^naming\s+=version\s+(\d+)\s+bsize=(\d+)/) {
> -	print STDERR "dirversion=$1\ndirbsize=$2\n";
> -	print STDOUT "naming   =VERN bsize=XXX\n";
> -    }
> -    if (/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+),\s+version=(\d+)/ ||
> -	/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+)/) {
> -	print STDERR "ldev=\"$1\"\nlbsize=$2\nlblocks=$3\nlversion=$4\n";
> -	print STDOUT "log      =LDEV bsize=XXX blocks=XXX\n";
> -    }
> -    if (/^\s+=\s+sectsz=(\d+)\s+sunit=(\d+) blks/) {
> -	print STDERR "logsectsz=$1\nlogsunit=$2\n\n";
> -    }
> -    if (/^realtime\s+=([\w|\/.-]+)\s+extsz=(\d+)\s+blocks=(\d+), rtextents=(\d+)/) {
> -	print STDERR "rtdev=$1\nrtextsz=$2\nrtblocks=$3\nrtextents=$4\n";
> -	print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
> -    }'
>  }
>  
> -
>  # prints the bits we care about in growfs
>  # 
>  _filter_growfs()
> diff --git a/common/xfs b/common/xfs
> index 713e9fe7..053b6189 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -1275,3 +1275,44 @@ _require_scratch_xfs_bigtime()
>  		_notrun "bigtime feature not advertised on mount?"
>  	_scratch_unmount
>  }
> +
> +_xfs_filter_mkfs()
> +{
> +	echo "_fs_has_crcs=0" >&2
> +	set -
> +	perl -ne '
> +	if (/^meta-data=([\w,|\/.-]+)\s+isize=(\d+)\s+agcount=(\d+), agsize=(\d+) blks/) {
> +		print STDERR "ddev=$1\nisize=$2\nagcount=$3\nagsize=$4\n";
> +		print STDOUT "meta-data=DDEV isize=XXX agcount=N, agsize=XXX blks\n";
> +	}
> +	if (/^\s+=\s+sectsz=(\d+)\s+attr=(\d+)/) {
> +		print STDERR "sectsz=$1\nattr=$2\n";
> +	}
> +	if (/^\s+=\s+crc=(\d)/) {
> +		print STDERR "_fs_has_crcs=$1\n";
> +	}
> +	if (/^data\s+=\s+bsize=(\d+)\s+blocks=(\d+), imaxpct=(\d+)/) {
> +		print STDERR "dbsize=$1\ndblocks=$2\nimaxpct=$3\n";
> +		print STDOUT "data     = bsize=XXX blocks=XXX, imaxpct=PCT\n";
> +	}
> +	if (/^\s+=\s+sunit=(\d+)\s+swidth=(\d+) blks/) {
> +		print STDERR "sunit=$1\nswidth=$2\nunwritten=1\n";
> +		print STDOUT "         = sunit=XXX swidth=XXX, unwritten=X\n";
> +	}
> +	if (/^naming\s+=version\s+(\d+)\s+bsize=(\d+)/) {
> +		print STDERR "dirversion=$1\ndirbsize=$2\n";
> +		print STDOUT "naming   =VERN bsize=XXX\n";
> +	}
> +	if (/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+),\s+version=(\d+)/ ||
> +		/^log\s+=(internal log|[\w|\/.-]+)\s+bsize=(\d+)\s+blocks=(\d+)/) {
> +		print STDERR "ldev=\"$1\"\nlbsize=$2\nlblocks=$3\nlversion=$4\n";
> +		print STDOUT "log      =LDEV bsize=XXX blocks=XXX\n";
> +	}
> +	if (/^\s+=\s+sectsz=(\d+)\s+sunit=(\d+) blks/) {
> +		print STDERR "logsectsz=$1\nlogsunit=$2\n\n";
> +	}
> +	if (/^realtime\s+=([\w|\/.-]+)\s+extsz=(\d+)\s+blocks=(\d+), rtextents=(\d+)/) {
> +		print STDERR "rtdev=$1\nrtextsz=$2\nrtblocks=$3\nrtextents=$4\n";
> +		print STDOUT "realtime =RDEV extsz=XXX blocks=XXX, rtextents=XXX\n";
> +	}'
> +}
> -- 
> 2.34.1
> 

Re: [PATCH v2 2/6] generic/204: remove unnecessary _scratch_mkfs call

[Shinichiro Kawasaki]

On Feb 09, 2022 / 14:31, Darrick J. Wong wrote:
> On Wed, Feb 09, 2022 at 09:33:01PM +0900, Shin'ichiro Kawasaki wrote:
> > The test case generic/204 calls _scratch_mkfs to get data block size and
> > i-node size of the filesystem and obtained data block size is passed to
> > the following _scratch_mfks_sized call as an option. However, the
> > _scratch_mkfs call is unnecessary since the sizes can be obtained by
> > _scratch_mkfs_sized call without the data block size option.
> > 
> > Also the _scratch_mkfs call is harmful when the _scratch_mkfs succeeds
> > and the _scratch_mkfs_sized fails. In this case, the _scratch_mkfs
> > leaves valid working filesystem on scratch device then following mount
> > and IO operations can not detect the failure of _scratch_mkfs_sized.
> > This results in the test case run with unexpected test condition.
> > 
> > Hence, remove the _scratch_mkfs call and the data block size option for
> > _scratch_mkfs_sized call.
> > 
> > Suggested-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Shin'ichiro Kawasaki <shinichiro.kawasaki@wdc.com>
> 
> Looks ok, assuming you've verified that fstests with FSTYP=xfs doesn't
> regress...
> 
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>

Thanks for reviewing. I tested the test case with FSTYP=xfs on a few devices and
3 variety of MKFS_OPTIONS (no option, "-b size=1024 -i size=512" and
"-b size=4096 -i size=2048") and all passed. Also I ran whole fstests with
FSTYP=xfs, and confirmed that this change does not cause additional failure.

-- 
Best Regards,
Shin'ichiro Kawasaki

Re: [PATCH v2 1/6] common/rc: fix btrfs mixed mode usage in _scratch_mkfs_sized

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>