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.
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
