| Message ID | f679cbd1b09fff494b58f37520a9ab727c3ff313.1707716170.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: mkfs: do not default to 4K sectorsize if the fs is zoned | expand |
On Mon, Feb 12, 2024 at 04:06:30PM +1030, Qu Wenruo wrote: > With some help from a reporter using loongson (with 16K page size), and > a zoned device, it turns out that, zoned code is not compatible with > subpage's requirement. > > Mostly conflicts on the multiple re-entry into the same @locked_page > using extent_write_locked_range(). > > The function is only utilized by compression for non-zoned btrfs, but > would be the default entry for all writes for zoned btrfs. > > So we can not default to 4K for subpage zoned combination. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > mkfs/main.c | 32 +++++++++++++++++++++++--------- > 1 file changed, 23 insertions(+), 9 deletions(-) > > diff --git a/mkfs/main.c b/mkfs/main.c > index b50b78b5665a..f54c1a055ae2 100644 > --- a/mkfs/main.c > +++ b/mkfs/main.c > @@ -1383,15 +1383,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv) > printf("See %s for more information.\n\n", PACKAGE_URL); > } > > - if (!sectorsize) > - sectorsize = (u32)SZ_4K; > - if (btrfs_check_sectorsize(sectorsize)) > - goto error; > - > - if (!nodesize) > - nodesize = max_t(u32, sectorsize, BTRFS_MKFS_DEFAULT_NODE_SIZE); > - > - stripesize = sectorsize; > saved_optind = optind; > device_count = argc - optind; > if (device_count == 0) > @@ -1470,6 +1461,29 @@ int BOX_MAIN(mkfs)(int argc, char **argv) > features.incompat_flags |= BTRFS_FEATURE_INCOMPAT_ZONED; > } > > + if (!sectorsize) { > + /* > + * Zoned btrfs utilize extent_write_locked_range(), which can not > + * handle multiple entries into the same locked page. > + * > + * For non-zoned btrfs, subpage workaround the problem by requiring > + * full page alignment for compression (the only path utilizing > + * that path). > + * But since zoned btrfs always goes that path, kernel can not support > + * writes into subpage zoned btrfs. > + */ > + if (!opt_zoned) > + sectorsize = (u32)SZ_4K; > + else > + sectorsize = (u32)getpagesize(); > + } 6.7 did the change to 4k instead of the auto detection of sectorsize, yet this adds the logic back. I'd rather not do that. We had compatibility issues with zoned when some of the profiles were not supported initially and collided with mkfs defaults, so I'd rather exit mkfs with a message what works with subpage and zoned.
On Mon, Feb 12, 2024 at 7:37 AM David Sterba <dsterba@suse.cz> wrote: > > On Mon, Feb 12, 2024 at 04:06:30PM +1030, Qu Wenruo wrote: > > With some help from a reporter using loongson (with 16K page size), and > > a zoned device, it turns out that, zoned code is not compatible with > > subpage's requirement. > > > > Mostly conflicts on the multiple re-entry into the same @locked_page > > using extent_write_locked_range(). > > > > The function is only utilized by compression for non-zoned btrfs, but > > would be the default entry for all writes for zoned btrfs. > > > > So we can not default to 4K for subpage zoned combination. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > --- > > mkfs/main.c | 32 +++++++++++++++++++++++--------- > > 1 file changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/mkfs/main.c b/mkfs/main.c > > index b50b78b5665a..f54c1a055ae2 100644 > > --- a/mkfs/main.c > > +++ b/mkfs/main.c > > @@ -1383,15 +1383,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv) > > printf("See %s for more information.\n\n", PACKAGE_URL); > > } > > > > - if (!sectorsize) > > - sectorsize = (u32)SZ_4K; > > - if (btrfs_check_sectorsize(sectorsize)) > > - goto error; > > - > > - if (!nodesize) > > - nodesize = max_t(u32, sectorsize, BTRFS_MKFS_DEFAULT_NODE_SIZE); > > - > > - stripesize = sectorsize; > > saved_optind = optind; > > device_count = argc - optind; > > if (device_count == 0) > > @@ -1470,6 +1461,29 @@ int BOX_MAIN(mkfs)(int argc, char **argv) > > features.incompat_flags |= BTRFS_FEATURE_INCOMPAT_ZONED; > > } > > > > + if (!sectorsize) { > > + /* > > + * Zoned btrfs utilize extent_write_locked_range(), which can not > > + * handle multiple entries into the same locked page. > > + * > > + * For non-zoned btrfs, subpage workaround the problem by requiring > > + * full page alignment for compression (the only path utilizing > > + * that path). > > + * But since zoned btrfs always goes that path, kernel can not support > > + * writes into subpage zoned btrfs. > > + */ > > + if (!opt_zoned) > > + sectorsize = (u32)SZ_4K; > > + else > > + sectorsize = (u32)getpagesize(); > > + } > > 6.7 did the change to 4k instead of the auto detection of sectorsize, > yet this adds the logic back. I'd rather not do that. We had > compatibility issues with zoned when some of the profiles were not > supported initially and collided with mkfs defaults, so I'd rather > exit mkfs with a message what works with subpage and zoned. > This should also be an impetus for zoned to get subpage support working, which throwing an error will hopefully do. -- 真実はいつも一つ！/ Always, there's only one truth!
On 2024/2/13 01:25, Neal Gompa wrote: > On Mon, Feb 12, 2024 at 7:37 AM David Sterba <dsterba@suse.cz> wrote: >> >> On Mon, Feb 12, 2024 at 04:06:30PM +1030, Qu Wenruo wrote: >>> With some help from a reporter using loongson (with 16K page size), and >>> a zoned device, it turns out that, zoned code is not compatible with >>> subpage's requirement. >>> >>> Mostly conflicts on the multiple re-entry into the same @locked_page >>> using extent_write_locked_range(). >>> >>> The function is only utilized by compression for non-zoned btrfs, but >>> would be the default entry for all writes for zoned btrfs. >>> >>> So we can not default to 4K for subpage zoned combination. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> mkfs/main.c | 32 +++++++++++++++++++++++--------- >>> 1 file changed, 23 insertions(+), 9 deletions(-) >>> >>> diff --git a/mkfs/main.c b/mkfs/main.c >>> index b50b78b5665a..f54c1a055ae2 100644 >>> --- a/mkfs/main.c >>> +++ b/mkfs/main.c >>> @@ -1383,15 +1383,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv) >>> printf("See %s for more information.\n\n", PACKAGE_URL); >>> } >>> >>> - if (!sectorsize) >>> - sectorsize = (u32)SZ_4K; >>> - if (btrfs_check_sectorsize(sectorsize)) >>> - goto error; >>> - >>> - if (!nodesize) >>> - nodesize = max_t(u32, sectorsize, BTRFS_MKFS_DEFAULT_NODE_SIZE); >>> - >>> - stripesize = sectorsize; >>> saved_optind = optind; >>> device_count = argc - optind; >>> if (device_count == 0) >>> @@ -1470,6 +1461,29 @@ int BOX_MAIN(mkfs)(int argc, char **argv) >>> features.incompat_flags |= BTRFS_FEATURE_INCOMPAT_ZONED; >>> } >>> >>> + if (!sectorsize) { >>> + /* >>> + * Zoned btrfs utilize extent_write_locked_range(), which can not >>> + * handle multiple entries into the same locked page. >>> + * >>> + * For non-zoned btrfs, subpage workaround the problem by requiring >>> + * full page alignment for compression (the only path utilizing >>> + * that path). >>> + * But since zoned btrfs always goes that path, kernel can not support >>> + * writes into subpage zoned btrfs. >>> + */ >>> + if (!opt_zoned) >>> + sectorsize = (u32)SZ_4K; >>> + else >>> + sectorsize = (u32)getpagesize(); >>> + } >> >> 6.7 did the change to 4k instead of the auto detection of sectorsize, >> yet this adds the logic back. I'd rather not do that. We had >> compatibility issues with zoned when some of the profiles were not >> supported initially and collided with mkfs defaults, so I'd rather >> exit mkfs with a message what works with subpage and zoned. >> > > This should also be an impetus for zoned to get subpage support > working, which throwing an error will hopefully do. It's too hard to reliably test if subpage + zoned is properly supported. We only have a simple supported_sectorsize from sysfs, which can not provide good enough granularity to distinguish zoned and non-zoned. Of course we can unconditionally throw out a warning, but if we add such zoned + subpage support in the future, we still need to determine if we need to throw that warning again. Any better solution? Thanks, Qu > > > > -- > 真実はいつも一つ！/ Always, there's only one truth! >
diff --git a/mkfs/main.c b/mkfs/main.c index b50b78b5665a..f54c1a055ae2 100644 --- a/mkfs/main.c +++ b/mkfs/main.c @@ -1383,15 +1383,6 @@ int BOX_MAIN(mkfs)(int argc, char **argv) printf("See %s for more information.\n\n", PACKAGE_URL); } - if (!sectorsize) - sectorsize = (u32)SZ_4K; - if (btrfs_check_sectorsize(sectorsize)) - goto error; - - if (!nodesize) - nodesize = max_t(u32, sectorsize, BTRFS_MKFS_DEFAULT_NODE_SIZE); - - stripesize = sectorsize; saved_optind = optind; device_count = argc - optind; if (device_count == 0) @@ -1470,6 +1461,29 @@ int BOX_MAIN(mkfs)(int argc, char **argv) features.incompat_flags |= BTRFS_FEATURE_INCOMPAT_ZONED; } + if (!sectorsize) { + /* + * Zoned btrfs utilize extent_write_locked_range(), which can not + * handle multiple entries into the same locked page. + * + * For non-zoned btrfs, subpage workaround the problem by requiring + * full page alignment for compression (the only path utilizing + * that path). + * But since zoned btrfs always goes that path, kernel can not support + * writes into subpage zoned btrfs. + */ + if (!opt_zoned) + sectorsize = (u32)SZ_4K; + else + sectorsize = (u32)getpagesize(); + } + if (btrfs_check_sectorsize(sectorsize)) + goto error; + + if (!nodesize) + nodesize = max_t(u32, sectorsize, BTRFS_MKFS_DEFAULT_NODE_SIZE); + + stripesize = sectorsize; /* * Set default profiles according to number of added devices. * For mixed groups defaults are single/single.
With some help from a reporter using loongson (with 16K page size), and a zoned device, it turns out that, zoned code is not compatible with subpage's requirement. Mostly conflicts on the multiple re-entry into the same @locked_page using extent_write_locked_range(). The function is only utilized by compression for non-zoned btrfs, but would be the default entry for all writes for zoned btrfs. So we can not default to 4K for subpage zoned combination. Signed-off-by: Qu Wenruo <wqu@suse.com> --- mkfs/main.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-)