On 2023-10-10 20:15:43, Eric Biggers wrote:
> 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.
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
> > 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);
> > - 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.
If block was re-instantiated or was in cache. I can try to come up
with better name.
--
- Andrey
