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

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

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
>>