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.
