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.
--D
> BTW, errors from builtin initcalls such as iomap_init() get ignored. So the
> error handling logic above does not really work as may have been intended.
>
> - Eric
>
