On Thu, Mar 07, 2024 at 03:45:22PM -0800, Darrick J. Wong wrote:
> On Fri, Mar 08, 2024 at 10:38:06AM +1100, Dave Chinner 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.
> >
> > Honestly: I don't think we care about this.
> >
> > Indeed, if a system is configured with iomap and does not use XFS,
> > GFS2 or zonefs, it's not going to be using the iomap_ioend_bioset at
> > all, either. So by you definition that's just wasted memory, too, on
> > systems that don't use any of these three filesystems. But we
> > aren't going to make that one conditional, because the complexity
> > and overhead of checks that never trigger after the first IO doesn't
> > actually provide any return for the cost of ongoing maintenance.
>
> I've occasionally wondered if I shouldn't try harder to make iomap a
> module so that nobody pays the cost of having it until they load an fs
> driver that uses it.
Not sure it is worth that effort. e.g. the block device code is
already using iomap (when bufferheads are turned off) so the future
reality is going to be that iomap will always be required when
CONFIG_BLOCK=y.
> > > 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.
> >
> > That's not an iomap problem - lots of fs_initcall() functions return
> > errors because they failed things like memory allocation. If this is
> > actually problem, then fix the core init infrastructure to handle
> > errors properly, eh?
>
> Soooo ... the kernel crashes if iomap cannot allocate its bioset?
>
> I guess that's handling it, FSVO handle. :P
Yup, the kernel will crash if any fs_initcall() fails to allocate
the memory it needs and that subsystem is then used. So the
workaround for that is that a bunch of these initcalls simply use
panic() calls for error handling. :(
> (Is that why I can't boot with mem=60M anymore?)
Something else is probably panic()ing on ENOMEM... :/
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
