Re: [RFC PATCH 02/11] pagemap: add mapping_clear_large_folios() wrapper

[Dave Chinner <david@fromorbit.com>]

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.

> 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@fromorbit.com