| Message ID | 20190705072651.25150-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: tests: Make 64K page size system happier | expand |
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: 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. > + 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. > + error( > + "invalid sectorsize: %u, expect either 4k, 8k, 16k, 32k or 64k", > + sectorsize); > + goto error; > + } > stripesize = sectorsize; > saved_optind = optind; > dev_cnt = argc - optind; >
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; >>
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)); + if (!is_power_of_2(sectorsize) || sectorsize < 4096 || + sectorsize > SZ_64K) { + error( + "invalid sectorsize: %u, expect either 4k, 8k, 16k, 32k or 64k", + sectorsize); + goto error; + } stripesize = sectorsize; saved_optind = optind; dev_cnt = argc - optind;
[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(-)