On 05/07/2024 14:24, Pankaj Raghav (Samsung) wrote: >>> I suggest you handle it better than this. If the device is asking for a >>> blocksize > PMD_SIZE, you should fail to mount it. >> >> That's my point: we already do that. >> >> The largest block size we support is 64kB and that's way smaller >> than PMD_SIZE on all platforms and we always check for bs > ps >> support at mount time when the filesystem bs > ps. >> >> Hence we're never going to set the min value to anything unsupported >> unless someone makes a massive programming mistake. At which point, >> we want a *hard, immediate fail* so the developer notices their >> mistake immediately. All filesystems and block devices need to >> behave this way so the limits should be encoded as asserts in the >> function to trigger such behaviour. > > I agree, this kind of bug will be encountered only during developement > and not during actual production due to the limit we have fs block size > in XFS. > >> >>> If the device is >>> asking for a blocksize > PAGE_SIZE and CONFIG_TRANSPARENT_HUGEPAGE is >>> not set, you should also decline to mount the filesystem. >> >> What does CONFIG_TRANSPARENT_HUGEPAGE have to do with filesystems >> being able to use large folios? >> >> If that's an actual dependency of using large folios, then we're at >> the point where the mm side of large folios needs to be divorced >> from CONFIG_TRANSPARENT_HUGEPAGE and always supported. >> Alternatively, CONFIG_TRANSPARENT_HUGEPAGE needs to selected by the >> block layer and also every filesystem that wants to support >> sector/blocks sizes larger than PAGE_SIZE. IOWs, large folio >> support needs to *always* be enabled on systems that say >> CONFIG_BLOCK=y. > > Why CONFIG_BLOCK? I think it is enough if it comes from the FS side > right? And for now, the only FS that needs that sort of bs > ps > guarantee is XFS with this series. Other filesystems such as bcachefs > that call mapping_set_large_folios() only enable it as an optimization > and it is not needed for the filesystem to function. > > So this is my conclusion from the conversation: > - Add a dependency in Kconfig on THP for XFS until we fix the dependency > of large folios on THP THP isn't supported on some arches, so isn't this effectively saying XFS can no longer be used with those arches, even if the bs <= ps? I think while pagecache large folios depend on THP, you need to make this a mount-time check in the FS? But ideally, MAX_PAGECACHE_ORDER would be set to 0 for !CONFIG_TRANSPARENT_HUGEPAGE so you can just check against that and don't have to worry about THP availability directly. Willy; Why is MAX_PAGECACHE_ORDER set to 8 when THP is disabled currently? > - Add a BUILD_BUG_ON(XFS_MAX_BLOCKSIZE > MAX_PAGECACHE_ORDER) > - Add a WARN_ON_ONCE() and clamp the min and max value in > mapping_set_folio_order_range() ? > > Let me know what you all think @willy, @dave and @ryan. > > -- > Pankaj
