Re: [PATCH v4 07/25] fsverity: support block-based Merkle tree caching

[Eric Biggers <ebiggers@kernel.org>]

On Fri, Feb 23, 2024 at 05:02:45PM +0100, Andrey Albershteyn wrote:
> > > +void fsverity_drop_block(struct inode *inode,
> > > +		struct fsverity_blockbuf *block)
> > > +{
> > > +	if (inode->i_sb->s_vop->drop_block)
> > > +		inode->i_sb->s_vop->drop_block(block);
> > > +	else {
> > > +		struct page *page = (struct page *)block->context;
> > > +
> > > +		/* Merkle tree block size == PAGE_SIZE; */
> > > +		if (block->verified)
> > > +			SetPageChecked(page);
> > > +
> > > +		kunmap_local(block->kaddr);
> > > +		put_page(page);
> > > +	}
> > > +}
> > 
> > I don't think this is the logical place for the call to SetPageChecked().
> > verity_data_block() currently does:
> > 
> >         if (vi->hash_block_verified)
> >                 set_bit(hblock_idx, vi->hash_block_verified);
> >         else
> >                 SetPageChecked(page);
> > 
> > You're proposing moving the SetPageChecked() to fsverity_drop_block().  Why?  We
> > should try to do things in a consistent place.
> > 
> > Similarly, I don't see why is_hash_block_verified() shouldn't keep the
> > PageChecked().
> > 
> > If we just keep PG_checked be get and set in the same places it currently is,
> > then adding fsverity_blockbuf::verified wouldn't be necessary.
> > 
> > Maybe you intended to move the awareness of PG_checked out of fs/verity/ and
> > into the filesystems?
> 
> yes
> 
> > Your change in how PG_checked is get and set is sort of a
> > step towards that, but it doesn't complete it.  It doesn't make sense to leave
> > in this half-finished state.
> 
> What do you think is missing? I didn't want to make too many changes
> to fs which already use fs-verity and completely change the
> interface, just to shift page handling stuff to middle layer
> functions. So yeah kinda "step towards" only :)

In your patchset, PG_checked is get and set by fsverity_drop_block() and
fsverity_read_merkle_tree_block(), which are located in fs/verity/ and called by
other code in fs/verity/.  I don't see this as being a separate layer from the
rest of fs/verity/.  If it was done by the individual filesystems (e.g.
fs/ext4/) that would be different, but it's not.  I think keeping fs/verity/
aware of PG_checked is the right call, and it's not necessary to do the half-way
move that sort of moves it to a different place in the stack but not really.

- Eric