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
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 <[email protected]>
Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
Reviewed-by: Darrick J. Wong <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
Reviewed-by: Johannes Thumshirn <[email protected]>
---
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
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
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 <[email protected]>
> Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
> Reviewed-by: Johannes Thumshirn <[email protected]>
> ---
> 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
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 <[email protected]>
> > Signed-off-by: Shin'ichiro Kawasaki <[email protected]>
> > Reviewed-by: Johannes Thumshirn <[email protected]>
> > ---
> > 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
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