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
