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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux