Re: [PATCH v5 10/24] iomap: integrate fs-verity verification into iomap's read path

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Mar 08, 2024 at 10:38:06AM +1100, Dave Chinner wrote:
> > 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.
> 
> 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.

"We're already wasting memory, therefore it's fine to waste more" isn't a great
argument.

iomap_ioend_bioset is indeed also problematic, though it's also a bit different
in that it's needed for basic filesystem functionality, not an optional feature.
Yes, ext4 and f2fs don't actually use iomap for buffered reads, but IIUC at
least there's a plan to do that.  Meanwhile there's no plan for fsverity to be
used on every system that may be using a filesystem that supports it.

We should take care not to over-estimate how many users use optional features.
Linux distros turn on most kconfig options just in case, but often the common
case is that the option is on but the feature is not used.

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

What does "properly" mean?  Even if init/main.c did something with the returned
error code, individual subsystems still need to know what behavior they want and
act accordingly.  Do they want to panic the kernel, or do they want to fall back
gracefully with degraded functionality.  If subsystems go with the panic (like
fsverity_init() does these days), then there is no need for doing any cleanup on
errors like this patchset does.

- Eric