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@xxxxxxxx> >> --- >> 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; >>
