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 <w...@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
