diff mbox series

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

Message ID 20220218073156.2179803-2-shinichiro.kawasaki@wdc.com (mailing list archive)
State New, archived
Headers show
Series fstests: fix _scratch_mkfs_sized failure handling | expand

Commit Message

Shinichiro Kawasaki Feb. 18, 2022, 7:31 a.m. UTC 
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(-)

Comments

Eryu Guan Feb. 20, 2022, 5 p.m. UTC | #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 <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
Naohiro Aota Feb. 21, 2022, 7:02 a.m. UTC | #2 
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
Shinichiro Kawasaki Feb. 21, 2022, 11:02 a.m. UTC | #3 
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.
diff mbox series

Patch

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)