Received-SPF: pass (google.com: domain of linux-crypto+bounces-4921-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 12 Jun 2024 12:14:06 -0700
From: Eric Biggers <ebiggers@kernel.org>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: linux-crypto@vger.kernel.org, fsverity@lists.linux.dev,
	dm-devel@lists.linux.dev, x86@kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Ard Biesheuvel <ardb@kernel.org>,
	Sami Tolvanen <samitolvanen@google.com>,
	Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH v5 15/15] dm-verity: improve performance by using
 multibuffer hashing
Message-ID: <20240612191406.GA1298@quark.localdomain>
References: <20240611034822.36603-1-ebiggers@kernel.org>
 <20240611034822.36603-16-ebiggers@kernel.org>
 <Zmlq5Y5MgEAVF42C@gondor.apana.org.au>
 <20240612153829.GC1170@sol.localdomain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240612153829.GC1170@sol.localdomain>

On Wed, Jun 12, 2024 at 08:38:29AM -0700, Eric Biggers wrote:
> On Wed, Jun 12, 2024 at 05:31:17PM +0800, Herbert Xu wrote:
> > On Mon, Jun 10, 2024 at 08:48:22PM -0700, Eric Biggers wrote:
> > >
> > > +		if (++io->num_pending == v->mb_max_msgs) {
> > > +			r = verity_verify_pending_blocks(v, io, bio);
> > > +			if (unlikely(r))
> > > +				goto error;
> > >  		}
> > 
> > What is the overhead if you just let it accumulate as large a
> > request as possible? We should let the underlying algorithm decide
> > how to divide this up in the most optimal fashion.
> > 
> 
> The queue adds 144*num_messages bytes to each bio.  It's desirable to keep this
> memory overhead down.  So it makes sense to limit the queue length to the
> multibuffer hashing interleaving factor.  Yes we could build something where you
> could get a marginal performance benefit from amounts higher than that by saving
> indirect calls, but I think it wouldn't be worth bloating the per-IO memory.
> 
> Another thing to keep in mind is that with how the dm-verity code is currently
> structured, for each data block it gets the wanted hash from the Merkle tree
> (which it prefetched earlier) before hashing the data block.  So I also worry
> that if we wait too long before starting to hash the data blocks, dm-verity will
> spend more time unnecessarily blocked on waiting for Merkle tree I/O.

To clarify, 144*num_messages bytes is for the current version that doesn't have
a redundant shash_desc (or ahash_request which is even bigger) for each block.
If you force those to be included, then that would of course go up.

Also, we can't really swap the order of "hashing a data block" and "getting the
wanted hash of the block" in order to avoid the second issue I mentioned,
because that would require removing the optimization where dm-verity zero-fills
blocks whose hashes are the hash of the zero block instead of verifying them.

So it seems like the downsides of queueing up too many blocks outweigh the
potential benefits.

- Eric