Re: [PATCH 1/5] btrfs-progs: mkfs: Apply the sectorsize user specified on 64k page size system

Qu Wenruo
2019-07-05T08:39:09Z

On 2019/7/5 下午3:45, Nikolay Borisov wrote:
>
>
> On 5.07.19 г. 10:26 ч., Qu Wenruo wrote:
>> [BUG]
>> On aarch64 with 64k page size, mkfs.btrfs -s option doesn't work:
>> $ mkfs.btrfs -s 4096 ~/10G.img -f
>> btrfs-progs v5.1.1
>> See http://btrfs.wiki.kernel.org for more information.
>>
>> Label: (null)
>> UUID: c2a09334-aaca-4980-aefa-4b3e27390658
>> Node size: 65536
>> Sector size: 65536 << Still 64K, not 4K
>> Filesystem size: 10.00GiB
>> Block group profiles:
>> Data: single 8.00MiB
>> Metadata: DUP 256.00MiB
>> System: DUP 8.00MiB
>> SSD detected: no
>> Incompat features: extref, skinny-metadata
>> Number of devices: 1
>> Devices:
>> ID SIZE PATH
>> 1 10.00GiB /home/adam/10G.img
>>
>> [CAUSE]
>> This is because we automatically detect sectorsize based on current
>> system page size, then get the maxium number between user specified -s
>> parameter and system page size.
>>
>> It's fine for x86 as it has fixed page size 4K, also the minimium valid
>> sector size.
>>
>> But for system like aarch64 or ppc64le, where we can have 64K page size,
>> and it makes us unable to create a 4k sector sized btrfs.
>>
>> [FIX]
>> Only do auto detect when no -s|--sectorsize option is specified.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> mkfs/main.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/mkfs/main.c b/mkfs/main.c
>> index 8dbec0717b89..26d84e9dafc3 100644
>> --- a/mkfs/main.c
>> +++ b/mkfs/main.c
>> @@ -817,6 +817,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>> char *source_dir = NULL;
>> bool source_dir_set = false;
>> bool shrink_rootdir = false;
>> + bool sectorsize_set = false;
>> u64 source_dir_size = 0;
>> u64 min_dev_size;
>> u64 shrink_size;
>> @@ -906,6 +907,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>> }
>> case 's':
>> sectorsize = parse_size(optarg);
>> + sectorsize_set = true;
>> break;
>> case 'b':
>> block_count = parse_size(optarg);
>> @@ -943,7 +945,15 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>> printf("See %s for more information.\n\n", PACKAGE_URL);
>> }
>>
>> - sectorsize = max(sectorsize, (u32)sysconf(_SC_PAGESIZE));
>> + if (!sectorsize_set)
>> + sectorsize = max(sectorsize, (u32)sysconf(_SC_PAGESIZE));
>
> This means it's possible for the user to create a filesystem that is not
> mountable on his current machine, due to the presence of the following
> check in validate_super:

There is no problem for it as we can also do that on x86:
mkfs.btrfs -f -s 64k -n 64k

>
> if (sectorsize != PAGE_SIZE) {
> btrfs_err(..)
> }
>
> Perhaps the risk is not that big since if someone creates such a
> filesystem they will almost instantly realize it won't work and
> re-create it properly.

And I'd argue any user of -s option should be considered as experienced
user.

>
>
>
>> + if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
>> + sectorsize > SZ_64K) {
>
> nit: Perhaps this check should be modified so that it follows the kernel
> style :
> if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
> sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
>
> MAX_METADATA_BLOCKSIZE is defined as 64k but using the same defines
> seems more clear to me.

That's correct and looks cleaner.

I'll update this patch.

Thanks,
Qu

>
>
>> + error(
>> + "invalid sectorsize: %u, expect either 4k, 8k, 16k, 32k or 64k",
>> + sectorsize);
>> + goto error;
>> + }
>> stripesize = sectorsize;
>> saved_optind = optind;
>> dev_cnt = argc - optind;
>>