Re: [PATCH v3 07/28] fsverity: always use bitmap to track verified status

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Oct 11, 2023 at 03:03:55PM +0200, Andrey Albershteyn wrote:
> > How complicated would it be to keep supporting using the page bit when
> > merkle_tree_block_size == page_size and the filesystem supports it?  It's an
> > efficient solution, so it would be a shame to lose it.  Also it doesn't have the
> > max file size limit that the bitmap has.
> 
> Well, I think it's possible but my motivation was to step away from
> page manipulation as much as possible with intent to not affect other
> filesystems too much. I can probably add handling of this case to
> fsverity_read_merkle_tree_block() but fs/verity still will create
> bitmap and have a limit. The other way is basically revert changes
> done in patch 09, then, it probably will be quite a mix of page/block
> handling in fs/verity/verify.c

The page-based caching still has to be supported anyway, since that's what the
other filesystems that support fsverity use, and it seems you don't plan to
change that.  The question is where the "block verified" flags should be stored.
Currently there are two options: PG_checked and the separate bitmap.  I'm not
yet convinced that removing the support for the PG_checked method is a good
change.  PG_checked is a nice solution for the cases where it can be used; it
requires no extra memory, no locking, and has no max file size.  Also, this
change seems mostly orthogonal to what you're actually trying to accomplish.

> > > Also, this patch removes spinlock. The lock was used to reset bits
> > > in bitmap belonging to one page. This patch works only with one
> > > Merkle tree block and won't reset other blocks status.
> > 
> > The spinlock is needed when there are multiple Merkle tree blocks per page and
> > the filesystem is using the page-based caching.  So I don't think you can remove
> > it.  Can you elaborate on why you feel it can be removed?
> 
> With this patch is_hash_block_verified() doesn't reset bits for
> blocks belonging to the same page. Even if page is re-instantiated
> only one block is checked in this case. So, when other blocks are
> checked they are reset.
> 
> 	if (block_cached)
> 		return test_bit(hblock_idx, vi->hash_block_verified);

When part of the Merkle tree cache is evicted and re-instantiated, whether that
part is a "page" or something else, the verified status of all the blocks
contained in that part need to be invalidated so that they get re-verified.  The
existing code does that.  I don't see how your proposed code does that.

- Eric