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.
> Similarly, once XFS has fsverity enabled, it's going to get used all
> over the place in the container and VM world. So we are *always*
> going to want this bioset to be initialised on these production
> systems, so it falls into the same category as the
> iomap_ioend_bioset. That is, if you don't want that overhead, turn
> the functionality off via CONFIG file options.
<nod> If someone really cares I guess we could employ that weird
cmpxchg trick that the dio workqueue setup function uses. But... let's
let someone complain first, eh? ;)
> > 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
(Is that why I can't boot with mem=60M anymore?)
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
