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

[PATCH v3 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 v2:
* Added Reviewed-by tags

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 v3 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>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 common/rc | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/rc b/common/rc
index de60fb7b..74d2d8bd 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 v3 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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 v3 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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 v3 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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 v3 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 v3 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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
 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 v3 1/6] common/rc: fix btrfs mixed mode usage in _scratch_mkfs_sized

[Eryu Guan]

On Fri, Feb 18, 2022 at 04:31:51PM +0900, Shin'ichiro Kawasaki wrote:
> 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

I'm wondering if this 256M -> 109M change was made just recently or was
made on old kernel.

If it was changed just recently, say 5.14 kernel, I suspect that tests
will fail on kernels prior to that change.

But if this change was made on some acient kernels, say 4.10, then I
think we're fine with this patch.

Thanks,
Eryu

> 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>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  common/rc | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/rc b/common/rc
> index de60fb7b..74d2d8bd 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

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

[Eryu Guan]

On Fri, Feb 18, 2022 at 04:31:50PM +0900, Shin'ichiro Kawasaki wrote:
> 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 v2:
> * Added Reviewed-by tags
> 
> 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

Thanks a lot for all the changes! I've applied all patches except the
first one. Also many thanks to Darrick for reviewing them!

Thanks,
Eryu

> 
>  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

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

[Naohiro Aota]

On Mon, Feb 21, 2022 at 01:00:23AM +0800, Eryu Guan wrote:
> On Fri, Feb 18, 2022 at 04:31:51PM +0900, Shin'ichiro Kawasaki wrote:
> > 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
> 
> I'm wondering if this 256M -> 109M change was made just recently or was
> made on old kernel.

The check is imposed from the userland tool btrfs-progs. The value is
calculated from a code in 31d228a2eb98 ("btrfs-progs: mkfs: Enhance
minimal device size calculation to fix mkfs failure on small file"),
which is released around v4.14.

But, after rechecking the code, the size part of the patch looks
invalid to me. My bad.

https://github.com/kdave/btrfs-progs/blob/master/mkfs/common.c#L651

As said in 50c1905c2795 ("btrfs: _scratch_mkfs_sized fix min size
without mixed option"), we need to consider every possible profile to
decide the minimal value.

That gives me:

- reserved += BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
	    BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
  --> reserved = 1M + 4M + 8M * 2 = 21M

- meta_size = SZ_8M + SZ_32M;
- meta_size *= 2;
- reserved += meta_size;
  --> reserved = 21M + (8M + 32M) * 2 = 101M

- data_size = 64M;
- data_size *= 2;
- reserved += data_size;
  --> reserved = 101M + 64M * 2 = 229M

We can also confirm the calculation with a zero size file:

   $ mkfs.btrfs -f -d DUP -m DUP btrfs.img
   btrfs-progs v5.16 
   See http://btrfs.wiki.kernel.org for more information.
   
   ERROR: 'btrfs.img' is too small to make a usable filesystem
   ERROR: minimum size for each btrfs device is 240123904

So, the original 256MB is roughly correct.

> If it was changed just recently, say 5.14 kernel, I suspect that tests
> will fail on kernels prior to that change.
> 
> But if this change was made on some acient kernels, say 4.10, then I
> think we're fine with this patch.
> 
> Thanks,
> Eryu
> 
> > 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>
> > Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> > ---
> >  common/rc | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index de60fb7b..74d2d8bd 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

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

[Shinichiro Kawasaki]

On Feb 21, 2022 / 07:02, Naohiro Aota wrote:
> On Mon, Feb 21, 2022 at 01:00:23AM +0800, Eryu Guan wrote:
> > On Fri, Feb 18, 2022 at 04:31:51PM +0900, Shin'ichiro Kawasaki wrote:
> > > 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
> > 
> > I'm wondering if this 256M -> 109M change was made just recently or was
> > made on old kernel.
> 
> The check is imposed from the userland tool btrfs-progs. The value is
> calculated from a code in 31d228a2eb98 ("btrfs-progs: mkfs: Enhance
> minimal device size calculation to fix mkfs failure on small file"),
> which is released around v4.14.
> 
> But, after rechecking the code, the size part of the patch looks
> invalid to me. My bad.
> 
> https://github.com/kdave/btrfs-progs/blob/master/mkfs/common.c#L651
> 
> As said in 50c1905c2795 ("btrfs: _scratch_mkfs_sized fix min size
> without mixed option"), we need to consider every possible profile to
> decide the minimal value.
> 
> That gives me:
> 
> - reserved += BTRFS_BLOCK_RESERVED_1M_FOR_SUPER +
> 	    BTRFS_MKFS_SYSTEM_GROUP_SIZE + SZ_8M * 2;
>   --> reserved = 1M + 4M + 8M * 2 = 21M
> 
> - meta_size = SZ_8M + SZ_32M;
> - meta_size *= 2;
> - reserved += meta_size;
>   --> reserved = 21M + (8M + 32M) * 2 = 101M
> 
> - data_size = 64M;
> - data_size *= 2;
> - reserved += data_size;
>   --> reserved = 101M + 64M * 2 = 229M
> 
> We can also confirm the calculation with a zero size file:
> 
>    $ mkfs.btrfs -f -d DUP -m DUP btrfs.img
>    btrfs-progs v5.16 
>    See http://btrfs.wiki.kernel.org for more information.
>    
>    ERROR: 'btrfs.img' is too small to make a usable filesystem
>    ERROR: minimum size for each btrfs device is 240123904
> 
> So, the original 256MB is roughly correct.

Naohiro, thank you for checking the detail. I agree that we should keep the
number 256MB. I will drop that part and repost the patch.

-- 
Best Regards,
Shin'ichiro Kawasaki