| Message ID | 20230404145319.2057051-10-aalbersh@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | fs-verity support for XFS | expand |
> if (iomap_block_needs_zeroing(iter, pos)) { > folio_zero_range(folio, poff, plen); > + if (iomap->flags & IOMAP_F_READ_VERITY) { Wju do we need the new flag vs just testing that folio_ops and folio_ops->verify_folio is non-NULL? > - ctx->bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs), > - REQ_OP_READ, gfp); > + ctx->bio = bio_alloc_bioset(iomap->bdev, bio_max_segs(nr_vecs), > + REQ_OP_READ, GFP_NOFS, &iomap_read_ioend_bioset); All other callers don't really need the larger bioset, so I'd avoid the unconditional allocation here, but more on that later. > + ioend = container_of(ctx->bio, struct iomap_read_ioend, > + read_inline_bio); > + ioend->io_inode = iter->inode; > + if (ctx->ops && ctx->ops->prepare_ioend) > + ctx->ops->prepare_ioend(ioend); > + So what we're doing in writeback and direct I/O, is to: a) have a submit_bio hook b) allow the file system to then hook the bi_end_io caller c) (only in direct O/O for now) allow the file system to provide a bio_set to allocate from I wonder if that also makes sense and keep all the deferral in the file system. We'll need that for the btrfs iomap conversion anyway, and it seems more flexible. The ioend processing would then move into XFS. > @@ -156,6 +160,11 @@ struct iomap_folio_ops { > * locked by the iomap code. > */ > bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); > + > + /* > + * Verify folio when successfully read > + */ > + bool (*verify_folio)(struct folio *folio, loff_t pos, unsigned int len); Why isn't this in iomap_readpage_ops?
Hi Christoph, On Tue, Apr 04, 2023 at 08:37:02AM -0700, Christoph Hellwig wrote: > > if (iomap_block_needs_zeroing(iter, pos)) { > > folio_zero_range(folio, poff, plen); > > + if (iomap->flags & IOMAP_F_READ_VERITY) { > > Wju do we need the new flag vs just testing that folio_ops and > folio_ops->verify_folio is non-NULL? Yes, it can be just test, haven't noticed that it's used only here, initially I used it in several places. > > > - ctx->bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs), > > - REQ_OP_READ, gfp); > > + ctx->bio = bio_alloc_bioset(iomap->bdev, bio_max_segs(nr_vecs), > > + REQ_OP_READ, GFP_NOFS, &iomap_read_ioend_bioset); > > All other callers don't really need the larger bioset, so I'd avoid > the unconditional allocation here, but more on that later. Ok, make sense. > > > + ioend = container_of(ctx->bio, struct iomap_read_ioend, > > + read_inline_bio); > > + ioend->io_inode = iter->inode; > > + if (ctx->ops && ctx->ops->prepare_ioend) > > + ctx->ops->prepare_ioend(ioend); > > + > > So what we're doing in writeback and direct I/O, is to: > > a) have a submit_bio hook > b) allow the file system to then hook the bi_end_io caller > c) (only in direct O/O for now) allow the file system to provide > a bio_set to allocate from I see. > > I wonder if that also makes sense and keep all the deferral in the > file system. We'll need that for the btrfs iomap conversion anyway, > and it seems more flexible. The ioend processing would then move into > XFS. > Not sure what you mean here. > > @@ -156,6 +160,11 @@ struct iomap_folio_ops { > > * locked by the iomap code. > > */ > > bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); > > + > > + /* > > + * Verify folio when successfully read > > + */ > > + bool (*verify_folio)(struct folio *folio, loff_t pos, unsigned int len); > > Why isn't this in iomap_readpage_ops? > Yes, it can be. But it appears to me to be more relevant to _folio_ops, any particular reason to move it there? Don't mind moving it to iomap_readpage_ops.
On Wed, Apr 05, 2023 at 01:01:16PM +0200, Andrey Albershteyn wrote: > Hi Christoph, > > On Tue, Apr 04, 2023 at 08:37:02AM -0700, Christoph Hellwig wrote: > > > if (iomap_block_needs_zeroing(iter, pos)) { > > > folio_zero_range(folio, poff, plen); > > > + if (iomap->flags & IOMAP_F_READ_VERITY) { > > > > Wju do we need the new flag vs just testing that folio_ops and > > folio_ops->verify_folio is non-NULL? > > Yes, it can be just test, haven't noticed that it's used only here, > initially I used it in several places. > > > > > > - ctx->bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs), > > > - REQ_OP_READ, gfp); > > > + ctx->bio = bio_alloc_bioset(iomap->bdev, bio_max_segs(nr_vecs), > > > + REQ_OP_READ, GFP_NOFS, &iomap_read_ioend_bioset); > > > > All other callers don't really need the larger bioset, so I'd avoid > > the unconditional allocation here, but more on that later. > > Ok, make sense. > > > > > > + ioend = container_of(ctx->bio, struct iomap_read_ioend, > > > + read_inline_bio); > > > + ioend->io_inode = iter->inode; > > > + if (ctx->ops && ctx->ops->prepare_ioend) > > > + ctx->ops->prepare_ioend(ioend); > > > + > > > > So what we're doing in writeback and direct I/O, is to: > > > > a) have a submit_bio hook > > b) allow the file system to then hook the bi_end_io caller > > c) (only in direct O/O for now) allow the file system to provide > > a bio_set to allocate from > > I see. > > > > > I wonder if that also makes sense and keep all the deferral in the > > file system. We'll need that for the btrfs iomap conversion anyway, > > and it seems more flexible. The ioend processing would then move into > > XFS. > > > > Not sure what you mean here. I /think/ Christoph is talking about allowing callers of iomap pagecache operations to supply a custom submit_bio function and a bio_set so that filesystems can add in their own post-IO processing and appropriately sized (read: minimum you can get away with) bios. I imagine btrfs has quite a lot of (read) ioend processing they need to do, as will xfs now that you're adding fsverity. > > > @@ -156,6 +160,11 @@ struct iomap_folio_ops { > > > * locked by the iomap code. > > > */ > > > bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); > > > + > > > + /* > > > + * Verify folio when successfully read > > > + */ > > > + bool (*verify_folio)(struct folio *folio, loff_t pos, unsigned int len); Any reason why we shouldn't return the usual negative errno? > > Why isn't this in iomap_readpage_ops? > > > > Yes, it can be. But it appears to me to be more relevant to > _folio_ops, any particular reason to move it there? Don't mind > moving it to iomap_readpage_ops. I think the point is that this is a general "check what we just read" hook, so it could be in readpage_ops since we're never going to need to re-validate verity contents, right? Hence it could be in readpage_ops instead of the general iomap_folio_ops. <shrug> Is there a use case for ->verify_folio that isn't a read post- processing step? --D > -- > - Andrey >
On Wed, Apr 05, 2023 at 08:06:27AM -0700, Darrick J. Wong wrote: > > > I wonder if that also makes sense and keep all the deferral in the > > > file system. We'll need that for the btrfs iomap conversion anyway, > > > and it seems more flexible. The ioend processing would then move into > > > XFS. > > > > > > > Not sure what you mean here. > > I /think/ Christoph is talking about allowing callers of iomap pagecache > operations to supply a custom submit_bio function and a bio_set so that > filesystems can add in their own post-IO processing and appropriately > sized (read: minimum you can get away with) bios. I imagine btrfs has > quite a lot of (read) ioend processing they need to do, as will xfs now > that you're adding fsverity. Exactly. > I think the point is that this is a general "check what we just read" > hook, so it could be in readpage_ops since we're never going to need to > re-validate verity contents, right? Hence it could be in readpage_ops > instead of the general iomap_folio_ops. > > <shrug> Is there a use case for ->verify_folio that isn't a read post- > processing step? Yes. In fact I wonder if the verification might actually be done in the per-bio end_io handler in the file system. But I'll need to find some more time to read through the XFS parts of series to come up with a more intelligent suggestion on that.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index d39be64b1da9..7e59c299c496 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -42,6 +42,7 @@ static inline struct iomap_page *to_iomap_page(struct folio *folio) } static struct bio_set iomap_ioend_bioset; +static struct bio_set iomap_read_ioend_bioset; static struct iomap_page * iomap_page_create(struct inode *inode, struct folio *folio, unsigned int flags) @@ -184,7 +185,7 @@ static void iomap_finish_folio_read(struct folio *folio, size_t offset, folio_unlock(folio); } -static void iomap_read_end_io(struct bio *bio) +void iomap_read_end_io(struct bio *bio) { int error = blk_status_to_errno(bio->bi_status); struct folio_iter fi; @@ -193,6 +194,7 @@ static void iomap_read_end_io(struct bio *bio) iomap_finish_folio_read(fi.folio, fi.offset, fi.length, error); bio_put(bio); } +EXPORT_SYMBOL_GPL(iomap_read_end_io); /** * iomap_read_inline_data - copy inline data into the page cache @@ -257,6 +259,7 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, loff_t orig_pos = pos; size_t poff, plen; sector_t sector; + struct iomap_read_ioend *ioend; if (iomap->type == IOMAP_INLINE) return iomap_read_inline_data(iter, folio); @@ -269,6 +272,13 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, if (iomap_block_needs_zeroing(iter, pos)) { folio_zero_range(folio, poff, plen); + if (iomap->flags & IOMAP_F_READ_VERITY) { + if (!iomap->folio_ops->verify_folio(folio, poff, plen)) { + folio_set_error(folio); + goto done; + } + } + iomap_set_range_uptodate(folio, iop, poff, plen); goto done; } @@ -290,8 +300,8 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, if (ctx->rac) /* same as readahead_gfp_mask */ gfp |= __GFP_NORETRY | __GFP_NOWARN; - ctx->bio = bio_alloc(iomap->bdev, bio_max_segs(nr_vecs), - REQ_OP_READ, gfp); + ctx->bio = bio_alloc_bioset(iomap->bdev, bio_max_segs(nr_vecs), + REQ_OP_READ, GFP_NOFS, &iomap_read_ioend_bioset); /* * If the bio_alloc fails, try it again for a single page to * avoid having to deal with partial page reads. This emulates @@ -305,6 +315,13 @@ static loff_t iomap_readpage_iter(const struct iomap_iter *iter, ctx->bio->bi_opf |= REQ_RAHEAD; ctx->bio->bi_iter.bi_sector = sector; ctx->bio->bi_end_io = iomap_read_end_io; + + ioend = container_of(ctx->bio, struct iomap_read_ioend, + read_inline_bio); + ioend->io_inode = iter->inode; + if (ctx->ops && ctx->ops->prepare_ioend) + ctx->ops->prepare_ioend(ioend); + bio_add_folio(ctx->bio, folio, plen, poff); } @@ -1813,6 +1830,15 @@ EXPORT_SYMBOL_GPL(iomap_writepages); static int __init iomap_init(void) { + int error = 0; + + error = bioset_init(&iomap_read_ioend_bioset, + 4 * (PAGE_SIZE / SECTOR_SIZE), + offsetof(struct iomap_read_ioend, read_inline_bio), + BIOSET_NEED_BVECS); + if (error) + return error; + return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE), offsetof(struct iomap_ioend, io_inline_bio), BIOSET_NEED_BVECS); diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 0fbce375265d..9a17b53309c9 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -53,6 +53,9 @@ struct vm_fault; * * IOMAP_F_XATTR indicates that the iomap is for an extended attribute extent * rather than a file data extent. + * + * IOMAP_F_READ_VERITY indicates that the iomap needs verification of read + * folios */ #define IOMAP_F_NEW (1U << 0) #define IOMAP_F_DIRTY (1U << 1) @@ -60,6 +63,7 @@ struct vm_fault; #define IOMAP_F_MERGED (1U << 3) #define IOMAP_F_BUFFER_HEAD (1U << 4) #define IOMAP_F_XATTR (1U << 5) +#define IOMAP_F_READ_VERITY (1U << 6) /* * Flags set by the core iomap code during operations: @@ -156,6 +160,11 @@ struct iomap_folio_ops { * locked by the iomap code. */ bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); + + /* + * Verify folio when successfully read + */ + bool (*verify_folio)(struct folio *folio, loff_t pos, unsigned int len); }; /* @@ -258,13 +267,30 @@ int iomap_file_buffered_write_punch_delalloc(struct inode *inode, struct iomap *iomap, loff_t pos, loff_t length, ssize_t written, int (*punch)(struct inode *inode, loff_t pos, loff_t length)); +struct iomap_read_ioend { + struct inode *io_inode; /* file being read from */ + struct work_struct work; /* post read work (e.g. fs-verity) */ + struct bio read_inline_bio;/* MUST BE LAST! */ +}; + +struct iomap_readpage_ops { + /* + * Optional, allows the file systems to perform actions just before + * submitting the bio and/or override the bio bi_end_io handler for + * additional verification after bio is processed + */ + void (*prepare_ioend)(struct iomap_read_ioend *ioend); +}; + struct iomap_readpage_ctx { struct folio *cur_folio; bool cur_folio_in_bio; struct bio *bio; struct readahead_control *rac; + const struct iomap_readpage_ops *ops; }; +void iomap_read_end_io(struct bio *bio); int iomap_read_folio(struct iomap_readpage_ctx *ctx, const struct iomap_ops *ops); void iomap_readahead(struct iomap_readpage_ctx *ctx,
Add IOMAP_F_READ_VERITY which indicates that iomap need to verify BIO (e.g. fs-verity) after I/O is completed. Add iomap_readpage_ops with only optional ->prepare_ioend() to allow filesystem to add callout used for configuring read path ioend. Mainly for setting ->bi_end_io() callout. Add iomap_folio_ops->verify_folio() for direct folio verification. The verification itself is suppose to happen on filesystem side. The verification is done when the BIO is processed by calling out ->bi_end_io(). Make iomap_read_end_io() exportable, so, it can be called back from filesystem callout after verification is done. The read path ioend are stored side by side with BIOs allocated from iomap_read_ioend_bioset. Signed-off-by: Andrey Albershteyn <aalbersh@redhat.com> --- fs/iomap/buffered-io.c | 32 +++++++++++++++++++++++++++++--- include/linux/iomap.h | 26 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-)