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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux