Re: [PATCH v2] btrfs-progs: mkfs: only output the warning if the sectorsize is not supported

[Boris Burkov <boris@bur.io>]

On Mon, Apr 19, 2021 at 02:45:12PM +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.
> 
> This patch will also introduce a new helper,
> sysfs_open_global_feature_file() to make it more obvious which global
> feature file we're opening.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> changelog:
> v2:
> - Introduce new helper to open global feature file
> - Extra the supported sectorsize check into its own function
> - Do proper token check other than strstr()
> - Fix the bug that we're passing @page_size to check
> ---
>  common/fsfeatures.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-
>  common/utils.c      | 15 ++++++++++++++
>  common/utils.h      |  1 +
>  3 files changed, 64 insertions(+), 1 deletion(-)
> 
> diff --git a/common/fsfeatures.c b/common/fsfeatures.c
> index 569208a9e5b1..6641c44dfa45 100644
> --- a/common/fsfeatures.c
> +++ b/common/fsfeatures.c
> @@ -327,8 +327,50 @@ u32 get_running_kernel_version(void)
>  
>  	return version;
>  }
> +
> +/*
> + * The buffer size should be strlen("4096 8192 16384 32768 65536"),
> + * which is 28, then we just round it up to 32.
> + */
> +#define SUPPORTED_SECTORSIZE_BUF_SIZE	32
> +
> +/*
> + * Check if the current kernel supports given sectorsize.
> + *
> + * Return true if the sectorsize is supported.
> + * Return false otherwise.
> + */
> +static bool check_supported_sectorsize(u32 sectorsize)
> +{
> +	char supported_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 };
> +	char sectorsize_buf[SUPPORTED_SECTORSIZE_BUF_SIZE] = { 0 };
> +	char *this_char;
> +	char *save_ptr = NULL;
> +	int fd;
> +	int ret;
> +
> +	fd = sysfs_open_global_feature_file("supported_sectorsizes");
> +	if (fd < 0)
> +		return false;
> +	ret = sysfs_read_file(fd, supported_buf, SUPPORTED_SECTORSIZE_BUF_SIZE);
> +	close(fd);
> +	if (ret < 0)
> +		return false;
> +	snprintf(sectorsize_buf, SUPPORTED_SECTORSIZE_BUF_SIZE,
> +		 "%u", sectorsize);
> +
> +	for (this_char = strtok_r(supported_buf, " ", &save_ptr);
> +	     this_char != NULL;
> +	     this_char = strtok_r(NULL, ",", &save_ptr)) {

Based on the example file contents in the comment, I would expect " " as
the delimeter for looping through the supported sizes, not ",".

> +		if (!strncmp(this_char, sectorsize_buf, strlen(sectorsize_buf)))
> +			return true;
> +	}
> +	return false;
> +}
> +
>  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 +382,12 @@ int btrfs_check_sectorsize(u32 sectorsize)
>  		      sectorsize);
>  		return -EINVAL;
>  	}
> -	if (page_size != sectorsize)
> +	if (page_size == sectorsize)
> +		sectorsize_checked = true;
> +	else
> +		sectorsize_checked = check_supported_sectorsize(sectorsize);
> +
> +	if (!sectorsize_checked)
>  		warning(
>  "the filesystem may not be mountable, sectorsize %u doesn't match page size %u",
>  			sectorsize, page_size);
> diff --git a/common/utils.c b/common/utils.c
> index 57e41432c8fb..e8b35879f19f 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -2205,6 +2205,21 @@ int sysfs_open_fsid_file(int fd, const char *filename)
>  	return open(sysfs_file, O_RDONLY);
>  }
>  
> +/*
> + * Open a file in global btrfs features directory and return the file
> + * descriptor or error.
> + */
> +int sysfs_open_global_feature_file(const char *feature_name)
> +{
> +	char path[PATH_MAX];
> +	int ret;
> +
> +	ret = path_cat_out(path, "/sys/fs/btrfs/features", feature_name);
> +	if (ret < 0)
> +		return ret;
> +	return open(path, O_RDONLY);
> +}
> +
>  /*
>   * Read up to @size bytes to @buf from @fd
>   */
> diff --git a/common/utils.h b/common/utils.h
> index c38bdb08077c..d2f6416a9b5a 100644
> --- a/common/utils.h
> +++ b/common/utils.h
> @@ -169,6 +169,7 @@ char *btrfs_test_for_multiple_profiles(int fd);
>  int btrfs_warn_multiple_profiles(int fd);
>  
>  int sysfs_open_fsid_file(int fd, const char *filename);
> +int sysfs_open_global_feature_file(const char *feature_name);
>  int sysfs_read_file(int fd, char *buf, size_t size);
>  
>  #endif
> -- 
> 2.31.1
>