On 2023-10-12 00:27:46, Eric Biggers wrote: > 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. Yes, I was trying to combine PG_checked and bitmap in the way it can be also used by XFS. I will try change this patch to not drop merkle_tree_block_size == page_size case. > > > > 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. Oh, I see the problem now, I will revert it back. -- - Andrey
