On 2024-02-23 10:07:32, Eric Biggers wrote:
> 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
>
I see, thanks! I will move back
--
- Andrey
