On Wed, Dec 14, 2022 at 08:08:13AM +1100, Dave Chinner wrote: > On Tue, Dec 13, 2022 at 05:55:22PM +0000, Matthew Wilcox wrote: > > On Tue, Dec 13, 2022 at 06:29:26PM +0100, Andrey Albershteyn wrote: > > > Add wrapper to clear mapping's large folio flag. This is handy for > > > disabling large folios on already existing inodes (e.g. future XFS > > > integration of fs-verity). > > > > I have two problems with this. One is your use of __clear_bit(). > > We can use __set_bit() because it's done as part of initialisation. > > As far as I can tell from your patches, mapping_clear_large_folios() is > > called on a live inode, so you'd have to use clear_bit() to avoid races. > > I think we can do without mapping_clear_large_folios() - we > already have precedence for this sort of mapping state change with > the DAX inode feature flag. That is, we change the on-disk state in > the ioctl context, but we don't change the in-memory inode state. > Instead, we mark it I_DONTCACHEi to get it turfed from memory with > expediency. Then when it is re-instantiated, we see the on-disk > state and then don't enable large mappings on that inode. > > That will work just fine here, I think. Thanks for the suggestion, I will try to look into this. If it won't work out I will stick to large folio switch, if no other objections. In anyway I will remove the mapping_clear_large_folios() wrapper not to encourage further use of such approach. > > > The second is that verity should obviously be enhanced to support > > large folios (and for that matter, block sizes smaller than PAGE_SIZE). > > Without that, this is just a toy or a prototype. Disabling large folios > > is not an option. > > Disabling large folios is very much an option. Filesystems must opt > in to large mapping support, so they can also choose to opt out. > i.e. large mappings is a filesystem policy decision, not a core > infrastructure decision. Hence how we disable large mappings > for fsverity enabled inodes is open to discussion, but saying we > can't opt out of an optional feature is entirely non-sensical. > > > I'm happy to work with you to add support for large folios to verity. > > It hasn't been high priority for me, but I'm now working on folio support > > for bufferhead filesystems and this would probably fit in. > > Yes, we need fsverity to support multipage folios, but modifying > fsverity is outside the scope of initially enabling fsverity support > on XFS. This patch set is about sorting out the on-disk format > changes and interfacing with the fsverity infrastructure to enable > the feature to be tested and verified. > > Stuff like large mapping support in fsverity is a future concern, > not a show-stopper for initial feature support. We don't need every > bell and whistle in the initial merge.... > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx > Thanks Andrey
