On Fri, Oct 06, 2023 at 08:49:01PM +0200, Andrey Albershteyn wrote:
> The bitmap is used to track verified status of the Merkle tree
> blocks which are smaller than a PAGE. Blocks which fits exactly in a
> page - use PageChecked() for tracking "verified" status.
>
> This patch switches to always use bitmap to track verified status.
> This is needed to move fs-verity away from page management and work
> only with Merkle tree blocks.
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.
> 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?
> /*
> - * Returns true if the hash block with index @hblock_idx in the tree, located in
> - * @hpage, has already been verified.
> + * Returns true if the hash block with index @hblock_idx in the tree has
> + * already been verified.
> */
> -static bool is_hash_block_verified(struct fsverity_info *vi, struct page *hpage,
> - unsigned long hblock_idx)
> +static bool is_hash_block_verified(struct fsverity_info *vi,
> + unsigned long hblock_idx,
> + bool block_cached)
> {
> - bool verified;
> - unsigned int blocks_per_page;
> - unsigned int i;
> -
> - /*
> - * When the Merkle tree block size and page size are the same, then the
> - * ->hash_block_verified bitmap isn't allocated, and we use PG_checked
> - * to directly indicate whether the page's block has been verified.
> - *
> - * Using PG_checked also guarantees that we re-verify hash pages that
> - * get evicted and re-instantiated from the backing storage, as new
> - * pages always start out with PG_checked cleared.
> - */
> - if (!vi->hash_block_verified)
> - return PageChecked(hpage);
> -
> - /*
> - * When the Merkle tree block size and page size differ, we use a bitmap
> - * to indicate whether each hash block has been verified.
> - *
> - * However, we still need to ensure that hash pages that get evicted and
> - * re-instantiated from the backing storage are re-verified. To do
> - * this, we use PG_checked again, but now it doesn't really mean
> - * "checked". Instead, now it just serves as an indicator for whether
> - * the hash page is newly instantiated or not.
> - *
> - * The first thread that sees PG_checked=0 must clear the corresponding
> - * bitmap bits, then set PG_checked=1. This requires a spinlock. To
> - * avoid having to take this spinlock in the common case of
> - * PG_checked=1, we start with an opportunistic lockless read.
> - */
Note that the above comment explains why the spinlock is needed.
> - if (PageChecked(hpage)) {
> - /*
> - * A read memory barrier is needed here to give ACQUIRE
> - * semantics to the above PageChecked() test.
> - */
> - smp_rmb();
> + if (block_cached)
> return test_bit(hblock_idx, vi->hash_block_verified);
It's not clear what block_cached is supposed to mean here.
- Eric
