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

[Boris Burkov <boris@bur.io>]

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.