* [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported @ 2021-04-15 5:30 Qu Wenruo 2021-04-16 17:59 ` David Sterba 2021-04-16 18:14 ` Boris Burkov 0 siblings, 2 replies; 6+ messages in thread From: Qu Wenruo @ 2021-04-15 5:30 UTC (permalink / raw) To: linux-btrfs Currently mkfs.btrfs will output a warning message if the sectorsize is not the same as page size: WARNING: the filesystem may not be mountable, sectorsize 4096 doesn't match page size 65536 But since btrfs subpage support for 64K page size is comming, this output is populating the golden output of fstests, causing tons of false alerts. This patch will make teach mkfs.btrfs to check /sys/fs/btrfs/features/supported_sectorsizes, and compare if the sector size is supported. Then only output above warning message if the sector size is not supported. Signed-off-by: Qu Wenruo <wqu@suse.com> --- common/fsfeatures.c | 36 +++++++++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/common/fsfeatures.c b/common/fsfeatures.c index 569208a9e5b1..13b775da9c72 100644 --- a/common/fsfeatures.c +++ b/common/fsfeatures.c @@ -16,6 +16,8 @@ #include "kerncompat.h" #include <sys/utsname.h> +#include <sys/stat.h> +#include <fcntl.h> #include <linux/version.h> #include <unistd.h> #include "common/fsfeatures.h" @@ -327,8 +329,15 @@ u32 get_running_kernel_version(void) return version; } + +/* + * The buffer size if strlen("4096 8192 16384 32768 65536"), + * which is 28, then round up to 32. + */ +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 int btrfs_check_sectorsize(u32 sectorsize) { + bool sectorsize_checked = false; u32 page_size = (u32)sysconf(_SC_PAGESIZE); if (!is_power_of_2(sectorsize)) { @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) sectorsize); return -EINVAL; } - if (page_size != sectorsize) + if (page_size == sectorsize) { + sectorsize_checked = true; + } else { + /* + * Check if the sector size is supported + */ + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; + int fd; + int ret; + + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", + O_RDONLY); + if (fd < 0) + goto out; + ret = read(fd, supported_buf, sizeof(supported_buf)); + close(fd); + if (ret < 0) + goto out; + snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, + "%u", page_size); + if (strstr(supported_buf, sectorsize_buf)) + sectorsize_checked = true; + } +out: + if (!sectorsize_checked) warning( "the filesystem may not be mountable, sectorsize %u doesn't match page size %u", sectorsize, page_size); -- 2.31.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported 2021-04-15 5:30 [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported Qu Wenruo @ 2021-04-16 17:59 ` David Sterba 2021-04-16 18:14 ` Boris Burkov 1 sibling, 0 replies; 6+ messages in thread From: David Sterba @ 2021-04-16 17:59 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Thu, Apr 15, 2021 at 01:30:11PM +0800, Qu Wenruo wrote: > Currently mkfs.btrfs will output a warning message if the sectorsize is > not the same as page size: > WARNING: the filesystem may not be mountable, sectorsize 4096 doesn't match page size 65536 > > But since btrfs subpage support for 64K page size is comming, this > output is populating the golden output of fstests, causing tons of false > alerts. > > This patch will make teach mkfs.btrfs to check > /sys/fs/btrfs/features/supported_sectorsizes, and compare if the sector > size is supported. > > Then only output above warning message if the sector size is not > supported. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > common/fsfeatures.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/common/fsfeatures.c b/common/fsfeatures.c > index 569208a9e5b1..13b775da9c72 100644 > --- a/common/fsfeatures.c > +++ b/common/fsfeatures.c > @@ -16,6 +16,8 @@ > > #include "kerncompat.h" > #include <sys/utsname.h> > +#include <sys/stat.h> > +#include <fcntl.h> > #include <linux/version.h> > #include <unistd.h> > #include "common/fsfeatures.h" > @@ -327,8 +329,15 @@ u32 get_running_kernel_version(void) > > return version; > } > + > +/* > + * The buffer size if strlen("4096 8192 16384 32768 65536"), > + * which is 28, then round up to 32. > + */ > +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 > int btrfs_check_sectorsize(u32 sectorsize) > { > + bool sectorsize_checked = false; > u32 page_size = (u32)sysconf(_SC_PAGESIZE); > > if (!is_power_of_2(sectorsize)) { > @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) > sectorsize); > return -EINVAL; > } > - if (page_size != sectorsize) > + if (page_size == sectorsize) { > + sectorsize_checked = true; > + } else { > + /* > + * Check if the sector size is supported > + */ > + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > + int fd; > + int ret; > + > + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", > + O_RDONLY); > + if (fd < 0) > + goto out; > + ret = read(fd, supported_buf, sizeof(supported_buf)); > + close(fd); There are some sysfs helpers, like sysfs_read_file and sysfs_open_fsid_file that would avoid the boilerplate code. We don't have a helper for the toplevel sysfs directory so that would need to be added first. Now that I look at it, the sysfs_read_file could actually do that in one go, including open and close. I'll probably do that as a cleanup later and apply your patch as it's a fix. Thanks. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported 2021-04-15 5:30 [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported Qu Wenruo 2021-04-16 17:59 ` David Sterba @ 2021-04-16 18:14 ` Boris Burkov 2021-04-16 20:07 ` David Sterba 2021-04-17 0:23 ` Qu Wenruo 1 sibling, 2 replies; 6+ messages in thread From: Boris Burkov @ 2021-04-16 18:14 UTC (permalink / raw) To: Qu Wenruo; +Cc: linux-btrfs On Thu, Apr 15, 2021 at 01:30:11PM +0800, Qu Wenruo wrote: > Currently mkfs.btrfs will output a warning message if the sectorsize is > not the same as page size: > WARNING: the filesystem may not be mountable, sectorsize 4096 doesn't match page size 65536 > > But since btrfs subpage support for 64K page size is comming, this > output is populating the golden output of fstests, causing tons of false > alerts. > > This patch will make teach mkfs.btrfs to check > /sys/fs/btrfs/features/supported_sectorsizes, and compare if the sector > size is supported. > > Then only output above warning message if the sector size is not > supported. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > common/fsfeatures.c | 36 +++++++++++++++++++++++++++++++++++- > 1 file changed, 35 insertions(+), 1 deletion(-) > > diff --git a/common/fsfeatures.c b/common/fsfeatures.c > index 569208a9e5b1..13b775da9c72 100644 > --- a/common/fsfeatures.c > +++ b/common/fsfeatures.c > @@ -16,6 +16,8 @@ > > #include "kerncompat.h" > #include <sys/utsname.h> > +#include <sys/stat.h> > +#include <fcntl.h> > #include <linux/version.h> > #include <unistd.h> > #include "common/fsfeatures.h" > @@ -327,8 +329,15 @@ u32 get_running_kernel_version(void) > > return version; > } > + > +/* > + * The buffer size if strlen("4096 8192 16384 32768 65536"), > + * which is 28, then round up to 32. I think there is a typo in this comment, because it doesn't quite parse. > + */ > +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 > int btrfs_check_sectorsize(u32 sectorsize) > { > + bool sectorsize_checked = false; > u32 page_size = (u32)sysconf(_SC_PAGESIZE); > > if (!is_power_of_2(sectorsize)) { > @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) > sectorsize); > return -EINVAL; > } > - if (page_size != sectorsize) > + if (page_size == sectorsize) { > + sectorsize_checked = true; > + } else { > + /* > + * Check if the sector size is supported > + */ > + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > + int fd; > + int ret; > + > + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", > + O_RDONLY); > + if (fd < 0) > + goto out; > + ret = read(fd, supported_buf, sizeof(supported_buf)); > + close(fd); > + if (ret < 0) > + goto out; > + snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, > + "%u", page_size); > + if (strstr(supported_buf, sectorsize_buf)) > + sectorsize_checked = true; Two comments here. 1: I think we should be checking sectorsize against the file rather than page_size. 2: strstr seems too permissive, since it doesn't have a notion of tokens. If not for the power_of_2 check above, we would admit all kinds of silly things like 409. But even with it, we would permit "4" now and with your example from the comment, "8", "16", and "32". > + } > +out: > + if (!sectorsize_checked) > warning( > "the filesystem may not be mountable, sectorsize %u doesn't match page size %u", > sectorsize, page_size); Do you have plans to change the contents of this string to match the new meaning of the check, or is that too harmful to testing/automation? > -- > 2.31.1 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported 2021-04-16 18:14 ` Boris Burkov @ 2021-04-16 20:07 ` David Sterba 2021-04-16 20:46 ` Boris Burkov 2021-04-17 0:23 ` Qu Wenruo 1 sibling, 1 reply; 6+ messages in thread From: David Sterba @ 2021-04-16 20:07 UTC (permalink / raw) To: Boris Burkov; +Cc: Qu Wenruo, linux-btrfs On Fri, Apr 16, 2021 at 11:14:08AM -0700, Boris Burkov wrote: > On Thu, Apr 15, 2021 at 01:30:11PM +0800, Qu Wenruo wrote: > > +/* > > + * The buffer size if strlen("4096 8192 16384 32768 65536"), > > + * which is 28, then round up to 32. > > I think there is a typo in this comment, because it doesn't quite parse. Typo fixed. > > + */ > > +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 > > int btrfs_check_sectorsize(u32 sectorsize) > > { > > + bool sectorsize_checked = false; > > u32 page_size = (u32)sysconf(_SC_PAGESIZE); > > > > if (!is_power_of_2(sectorsize)) { > > @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) > > sectorsize); > > return -EINVAL; > > } > > - if (page_size != sectorsize) > > + if (page_size == sectorsize) { > > + sectorsize_checked = true; > > + } else { > > + /* > > + * Check if the sector size is supported > > + */ > > + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > > + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > > + int fd; > > + int ret; > > + > > + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", > > + O_RDONLY); > > + if (fd < 0) > > + goto out; > > + ret = read(fd, supported_buf, sizeof(supported_buf)); > > + close(fd); > > + if (ret < 0) > > + goto out; > > + snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, > > + "%u", page_size); > > + if (strstr(supported_buf, sectorsize_buf)) > > + sectorsize_checked = true; > > Two comments here. > 1: I think we should be checking sectorsize against the file rather than > page_size. What do you mean by 'against the file'? I read it as comparing what system reports as page size, converted to string and looked up in the sysfs file. > 2: strstr seems too permissive, since it doesn't have a notion of > tokens. If not for the power_of_2 check above, we would admit all kinds > of silly things like 409. But even with it, we would permit "4" now and > with your example from the comment, "8", "16", and "32". In general you're right. Practically speaking, this will work. We know what the kernel module puts to that file and if getconf returns some absurd number for PAGE_SIZE other things will break. The code assumes perfect match, in any other case it prints the warning. So even if there are some funny values either in getconf or the sysfs file, it leads to something noticealbe to the user. A silent error would be worse and worth fixing, but that way around it works. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported 2021-04-16 20:07 ` David Sterba @ 2021-04-16 20:46 ` Boris Burkov 0 siblings, 0 replies; 6+ messages in thread From: Boris Burkov @ 2021-04-16 20:46 UTC (permalink / raw) To: dsterba, Qu Wenruo, linux-btrfs On Fri, Apr 16, 2021 at 10:07:09PM +0200, David Sterba wrote: > On Fri, Apr 16, 2021 at 11:14:08AM -0700, Boris Burkov wrote: > > On Thu, Apr 15, 2021 at 01:30:11PM +0800, Qu Wenruo wrote: > > > +/* > > > + * The buffer size if strlen("4096 8192 16384 32768 65536"), > > > + * which is 28, then round up to 32. > > > > I think there is a typo in this comment, because it doesn't quite parse. > > Typo fixed. > > > > + */ > > > +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 > > > int btrfs_check_sectorsize(u32 sectorsize) > > > { > > > + bool sectorsize_checked = false; > > > u32 page_size = (u32)sysconf(_SC_PAGESIZE); > > > > > > if (!is_power_of_2(sectorsize)) { > > > @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) > > > sectorsize); > > > return -EINVAL; > > > } > > > - if (page_size != sectorsize) > > > + if (page_size == sectorsize) { > > > + sectorsize_checked = true; > > > + } else { > > > + /* > > > + * Check if the sector size is supported > > > + */ > > > + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > > > + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; > > > + int fd; > > > + int ret; > > > + > > > + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", > > > + O_RDONLY); > > > + if (fd < 0) > > > + goto out; > > > + ret = read(fd, supported_buf, sizeof(supported_buf)); > > > + close(fd); > > > + if (ret < 0) > > > + goto out; > > > + snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, > > > + "%u", page_size); > > > + if (strstr(supported_buf, sectorsize_buf)) > > > + sectorsize_checked = true; > > > > Two comments here. > > 1: I think we should be checking sectorsize against the file rather than > > page_size. > > What do you mean by 'against the file'? > I am saying we should do `snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, "%u", sectorsize);` so that we lookup sectorsize, not page_size, in supported_sectorsizes. Sorry for the confusing wording. > I read it as comparing what system reports as page size, converted to > string and looked up in the sysfs file. Apologies if I am misunderstanding the purpose of this patch, but the way I understand it is: Today, on a system with 64K pages, we must use a 64K sectorsize. As part of supporting a 4K sectorsize, we want to relax this check to allow sectorsize=4K. If the user runs mkfs.btrfs -s 4096, how would checking whether 64K is present in "supported_sectorsizes" help validate the input? As long page_size is in "supported_sectorsizes", any power of 2 between 4k and 64k is legit. I suppose that could be the logic, but it seems kind of bizarre to me. If that is really the intent, I would argue the filename "supported_sectorsizes" doesn't make sense. > > > 2: strstr seems too permissive, since it doesn't have a notion of > > tokens. If not for the power_of_2 check above, we would admit all kinds > > of silly things like 409. But even with it, we would permit "4" now and > > with your example from the comment, "8", "16", and "32". > > In general you're right. Practically speaking, this will work. We know > what the kernel module puts to that file and if getconf returns some > absurd number for PAGE_SIZE other things will break. The code assumes > perfect match, in any other case it prints the warning. So even if > there are some funny values either in getconf or the sysfs file, it > leads to something noticealbe to the user. A silent error would be worse > and worth fixing, but that way around it works. Looking more closely, I think that the [4k,64k] check mostly covers my concern anyway. I also agree that we should be pragmatic and not over-complicate the check. However, I do think if my point above holds, then the strstr input could be different than just the one fixed page size. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported 2021-04-16 18:14 ` Boris Burkov 2021-04-16 20:07 ` David Sterba @ 2021-04-17 0:23 ` Qu Wenruo 1 sibling, 0 replies; 6+ messages in thread From: Qu Wenruo @ 2021-04-17 0:23 UTC (permalink / raw) To: Boris Burkov, Qu Wenruo; +Cc: linux-btrfs On 2021/4/17 上午2:14, Boris Burkov wrote: > On Thu, Apr 15, 2021 at 01:30:11PM +0800, Qu Wenruo wrote: >> Currently mkfs.btrfs will output a warning message if the sectorsize is >> not the same as page size: >> WARNING: the filesystem may not be mountable, sectorsize 4096 doesn't match page size 65536 >> >> But since btrfs subpage support for 64K page size is comming, this >> output is populating the golden output of fstests, causing tons of false >> alerts. >> >> This patch will make teach mkfs.btrfs to check >> /sys/fs/btrfs/features/supported_sectorsizes, and compare if the sector >> size is supported. >> >> Then only output above warning message if the sector size is not >> supported. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> common/fsfeatures.c | 36 +++++++++++++++++++++++++++++++++++- >> 1 file changed, 35 insertions(+), 1 deletion(-) >> >> diff --git a/common/fsfeatures.c b/common/fsfeatures.c >> index 569208a9e5b1..13b775da9c72 100644 >> --- a/common/fsfeatures.c >> +++ b/common/fsfeatures.c >> @@ -16,6 +16,8 @@ >> >> #include "kerncompat.h" >> #include <sys/utsname.h> >> +#include <sys/stat.h> >> +#include <fcntl.h> >> #include <linux/version.h> >> #include <unistd.h> >> #include "common/fsfeatures.h" >> @@ -327,8 +329,15 @@ u32 get_running_kernel_version(void) >> >> return version; >> } >> + >> +/* >> + * The buffer size if strlen("4096 8192 16384 32768 65536"), >> + * which is 28, then round up to 32. > > I think there is a typo in this comment, because it doesn't quite parse. I mean the strlen() == 28, then round the value to 32. Any better alternative? > >> + */ >> +#define SUPPORTED_SECTORSIZE_BUF_SIZE 32 >> int btrfs_check_sectorsize(u32 sectorsize) >> { >> + bool sectorsize_checked = false; >> u32 page_size = (u32)sysconf(_SC_PAGESIZE); >> >> if (!is_power_of_2(sectorsize)) { >> @@ -340,7 +349,32 @@ int btrfs_check_sectorsize(u32 sectorsize) >> sectorsize); >> return -EINVAL; >> } >> - if (page_size != sectorsize) >> + if (page_size == sectorsize) { >> + sectorsize_checked = true; >> + } else { >> + /* >> + * Check if the sector size is supported >> + */ >> + char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; >> + char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 }; >> + int fd; >> + int ret; >> + >> + fd = open("/sys/fs/btrfs/features/supported_sectorsizes", >> + O_RDONLY); >> + if (fd < 0) >> + goto out; >> + ret = read(fd, supported_buf, sizeof(supported_buf)); >> + close(fd); >> + if (ret < 0) >> + goto out; >> + snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE, >> + "%u", page_size); >> + if (strstr(supported_buf, sectorsize_buf)) >> + sectorsize_checked = true; > > Two comments here. > 1: I think we should be checking sectorsize against the file rather than > page_size. Damn it, all my bad. > 2: strstr seems too permissive, since it doesn't have a notion of > tokens. If not for the power_of_2 check above, we would admit all kinds > of silly things like 409. But even with it, we would permit "4" now and > with your example from the comment, "8", "16", and "32". Indeed I took a shortcut here. It's indeed not elegant, I'll change it to use " " as token to analyse each value and compare to sector size. Thanks, Qu > >> + } >> +out: >> + if (!sectorsize_checked) >> warning( >> "the filesystem may not be mountable, sectorsize %u doesn't match page size %u", >> sectorsize, page_size); > > Do you have plans to change the contents of this string to match the new > meaning of the check, or is that too harmful to testing/automation? > >> -- >> 2.31.1 >> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-17 0:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-15 5:30 [PATCH] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported Qu Wenruo 2021-04-16 17:59 ` David Sterba 2021-04-16 18:14 ` Boris Burkov 2021-04-16 20:07 ` David Sterba 2021-04-16 20:46 ` Boris Burkov 2021-04-17 0:23 ` Qu Wenruo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.