On Thu, Mar 07, 2024 at 02:06:08PM -0800, Darrick J. Wong wrote:
> On Mon, Mar 04, 2024 at 03:39:27PM -0800, Eric Biggers wrote:
> > On Mon, Mar 04, 2024 at 08:10:33PM +0100, Andrey Albershteyn wrote:
> > > +#ifdef CONFIG_FS_VERITY
> > > +struct iomap_fsverity_bio {
> > > + struct work_struct work;
> > > + struct bio bio;
> > > +};
> >
> > Maybe leave a comment above that mentions that bio must be the last field.
> >
> > > @@ -471,6 +529,7 @@ static loff_t iomap_readahead_iter(const struct iomap_iter *iter,
> > > * iomap_readahead - Attempt to read pages from a file.
> > > * @rac: Describes the pages to be read.
> > > * @ops: The operations vector for the filesystem.
> > > + * @wq: Workqueue for post-I/O processing (only need for fsverity)
> >
> > This should not be here.
> >
> > > +#define IOMAP_POOL_SIZE (4 * (PAGE_SIZE / SECTOR_SIZE))
> > > +
> > > static int __init iomap_init(void)
> > > {
> > > - return bioset_init(&iomap_ioend_bioset, 4 * (PAGE_SIZE / SECTOR_SIZE),
> > > - offsetof(struct iomap_ioend, io_inline_bio),
> > > - BIOSET_NEED_BVECS);
> > > + int error;
> > > +
> > > + error = bioset_init(&iomap_ioend_bioset, IOMAP_POOL_SIZE,
> > > + offsetof(struct iomap_ioend, io_inline_bio),
> > > + BIOSET_NEED_BVECS);
> > > +#ifdef CONFIG_FS_VERITY
> > > + if (error)
> > > + return error;
> > > +
> > > + error = bioset_init(&iomap_fsverity_bioset, IOMAP_POOL_SIZE,
> > > + offsetof(struct iomap_fsverity_bio, bio),
> > > + BIOSET_NEED_BVECS);
> > > + if (error)
> > > + bioset_exit(&iomap_ioend_bioset);
> > > +#endif
> > > + return error;
> > > }
> > > fs_initcall(iomap_init);
> >
> > This makes all kernels with CONFIG_FS_VERITY enabled start preallocating memory
> > for these bios, regardless of whether they end up being used or not. When
> > PAGE_SIZE==4096 it comes out to about 134 KiB of memory total (32 bios at just
> > over 4 KiB per bio, most of which is used for the BIO_MAX_VECS bvecs), and it
> > scales up with PAGE_SIZE such that with PAGE_SIZE==65536 it's about 2144 KiB.
> >
> > How about allocating the pool when it's known it's actually going to be used,
> > similar to what fs/crypto/ does for fscrypt_bounce_page_pool? For example,
> > there could be a flag in struct fsverity_operations that says whether filesystem
> > wants the iomap fsverity bioset, and when fs/verity/ sets up the fsverity_info
> > for any file for the first time since boot, it could call into fs/iomap/ to
> > initialize the iomap fsverity bioset if needed.
>
> I was thinking the same thing.
>
> Though I wondered if you want to have a CONFIG_IOMAP_FS_VERITY as well?
> But that might be tinykernel tetrapyloctomy.
>
I'd prefer just doing the alloc-on-first-use optimization, and not do
CONFIG_IOMAP_FS_VERITY. CONFIG_IOMAP_FS_VERITY would serve a similar purpose,
but only if XFS is not built into the kernel, and only before the other
filesystems that support fsverity convert their buffered reads to use iomap.
For tiny kernels there's always the option of CONFIG_FS_VERITY=n.
- Eric
