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

From: Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To: Naohiro Aota <Naohiro.Aota@wdc.com>
Cc: Eryu Guan <guan@eryu.me>,
	"fstests@vger.kernel.org" <fstests@vger.kernel.org>,
	"linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
	"linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
	"linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
	Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
	Damien Le Moal <damien.lemoal@opensource.wdc.com>
Subject: Re: [PATCH v3 1/6] common/rc: fix btrfs mixed mode usage in _scratch_mkfs_sized
Date: Mon, 21 Feb 2022 11:02:55 +0000	[thread overview]
Message-ID: <20220221110254.y2yb7xdlf22ahh7k@shindev> (raw)
In-Reply-To: <20220221070229.2hs45fqk7fbfbgpk@naota-xeon>

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