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.