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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/4/20 上午12:31, Boris Burkov wrote:
> 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 ",".

What am I doing, (facepalm...

Thanks for pointing this out,
Qu
>
>> +		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
>>