| Message ID | 20221213172935.680971-3-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fs-verity support for XFS | expand |
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. 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. 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. > Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> > --- > include/linux/pagemap.h | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h > index bbccb40442224..63ca600bdf8f7 100644 > --- a/include/linux/pagemap.h > +++ b/include/linux/pagemap.h > @@ -306,6 +306,11 @@ static inline void mapping_set_large_folios(struct address_space *mapping) > __set_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags); > } > > +static inline void mapping_clear_large_folios(struct address_space *mapping) > +{ > + __clear_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags); > +} > + > /* > * Large folio support currently depends on THP. These dependencies are > * being worked on but are not yet fixed. > -- > 2.31.1 >
On Tue, Dec 13, 2022 at 05:55:22PM +0000, Matthew Wilcox wrote: > 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. I'd be very interested to know what else is needed after commit 98dc08bae678 ("fsverity: stop using PG_error to track error status") which is upstream now, and https://lore.kernel.org/linux-fsdevel/20221028224539.171818-1-ebiggers@kernel.org/T/#u ("fsverity: support for non-4K pages") which is planned for 6.3. - Eric
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.
On Tue, Dec 13, 2022 at 11:33:54AM -0800, Eric Biggers wrote: > On Tue, Dec 13, 2022 at 05:55:22PM +0000, Matthew Wilcox wrote: > > 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. > > I'd be very interested to know what else is needed after commit 98dc08bae678 > ("fsverity: stop using PG_error to track error status") which is upstream now, > and > https://lore.kernel.org/linux-fsdevel/20221028224539.171818-1-ebiggers@kernel.org/T/#u > ("fsverity: support for non-4K pages") which is planned for 6.3. Did you change the bio interfaces to iterate a bvec full of variable sized folios, or does it still expect a bio to only have PAGE_SIZE pages attached to it? Cheers, Dave.
On Wed, Dec 14, 2022 at 08:10:10AM +1100, Dave Chinner wrote: > On Tue, Dec 13, 2022 at 11:33:54AM -0800, Eric Biggers wrote: > > On Tue, Dec 13, 2022 at 05:55:22PM +0000, Matthew Wilcox wrote: > > > 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. > > > > I'd be very interested to know what else is needed after commit 98dc08bae678 > > ("fsverity: stop using PG_error to track error status") which is upstream now, > > and > > https://lore.kernel.org/linux-fsdevel/20221028224539.171818-1-ebiggers@kernel.org/T/#u > > ("fsverity: support for non-4K pages") which is planned for 6.3. > > Did you change the bio interfaces to iterate a bvec full of > variable sized folios, or does it still expect a bio to only have > PAGE_SIZE pages attached to it? > You can take a look at fsverity_verify_bio() with https://lore.kernel.org/r/20221028224539.171818-2-ebiggers@kernel.org applied. It uses bio_for_each_segment_all() to iterate through the bio's segments. For each segment, it verifies each data block in the segment, assuming bv_len and bv_offset are multiples of the Merkle tree block size. The file position of each data block is computed as '(page->index << PAGE_SHIFT) + bv_offset'. I suppose the issue is going to be that only the first page of a folio actually has an index. Using bio_for_each_folio_all() would avoid this problem, I think? - Eric
On Tue, Dec 13, 2022 at 10:52:11PM -0800, Eric Biggers wrote: > On Wed, Dec 14, 2022 at 08:10:10AM +1100, Dave Chinner wrote: > > On Tue, Dec 13, 2022 at 11:33:54AM -0800, Eric Biggers wrote: > > > On Tue, Dec 13, 2022 at 05:55:22PM +0000, Matthew Wilcox wrote: > > > > 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. > > > > > > I'd be very interested to know what else is needed after commit 98dc08bae678 > > > ("fsverity: stop using PG_error to track error status") which is upstream now, > > > and > > > https://lore.kernel.org/linux-fsdevel/20221028224539.171818-1-ebiggers@kernel.org/T/#u > > > ("fsverity: support for non-4K pages") which is planned for 6.3. > > > > Did you change the bio interfaces to iterate a bvec full of > > variable sized folios, or does it still expect a bio to only have > > PAGE_SIZE pages attached to it? > > > > You can take a look at fsverity_verify_bio() with > https://lore.kernel.org/r/20221028224539.171818-2-ebiggers@kernel.org applied. > It uses bio_for_each_segment_all() to iterate through the bio's segments. For > each segment, it verifies each data block in the segment, assuming bv_len and > bv_offset are multiples of the Merkle tree block size. The file position of > each data block is computed as '(page->index << PAGE_SHIFT) + bv_offset'. Right, that still has the issue that it thinks each segment is a struct page, whereas the iomap code is putting a struct folio that contains a contiguous multipage folio into each segment. > I suppose the issue is going to be that only the first page of a folio actually > has an index. Using bio_for_each_folio_all() would avoid this problem, I think? Yes, using bio_for_each_folio_all() is large folio aware. But then you'll have to also pass the folio through the verification chain for getting data offsets into the folio, etc. Cheers, Dave.
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@fromorbit.com > Thanks Andrey
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h index bbccb40442224..63ca600bdf8f7 100644 --- a/include/linux/pagemap.h +++ b/include/linux/pagemap.h @@ -306,6 +306,11 @@ static inline void mapping_set_large_folios(struct address_space *mapping) __set_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags); } +static inline void mapping_clear_large_folios(struct address_space *mapping) +{ + __clear_bit(AS_LARGE_FOLIO_SUPPORT, &mapping->flags); +} + /* * Large folio support currently depends on THP. These dependencies are * being worked on but are not yet fixed.
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). Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- include/linux/pagemap.h | 5 +++++ 1 file changed, 5 insertions(+)