Re: [PATCH 03/13] fsverity: support block-based Merkle tree caching

[Eric Biggers <ebiggers@kernel.org>]

On Wed, Apr 24, 2024 at 05:27:06PM -0700, Darrick J. Wong wrote:
> > > > > +		err = fsverity_read_merkle_tree_block(inode, &vi->tree_params,
> > > > > +				pos - offs_in_block, ra_bytes, &block);
> > > > > +		if (err) {
> > > > >  			fsverity_err(inode,
> > > > > -				     "Error %d reading Merkle tree page %lu",
> > > > > -				     err, index);
> > > > > +				     "Error %d reading Merkle tree block %llu",
> > > > > +				     err, pos);
> > > > >  			break;
> > > > 
> > > > The error message should go into fsverity_read_merkle_tree_block() so that it
> > > > does not need to be duplicated in its two callers.  This would, additionally,
> > > > eliminate the need to introduce the 'err' variable in verify_data_block().
> > > 
> > > Do you want that to be a separate cleanup patch?
> > 
> > I think it makes sense to do it in this patch, since this patch introduces
> > fsverity_read_merkle_tree_block().  This patch also adds the 'err' variable back
> > to verify_data_block(), which may mislead people into thinking it's the actual
> > error code that users see (this has happened before) --- that could be avoided.
> 
> Let's rename it "bad" in verify.c then.

Anything wrong with just 'if (fsverity_read_merkle_tree_block(...) != 0) {'?

> I'm assuming that you /do/ want the error code in fsverity_read_merkle_tree?

Yes, that's needed to log it.

> > It makes sense to cache the blocks by index regardless of whether you get passed
> > the index directly.  So this is just a question of what the
> > ->read_merkle_tree_block function prototype should be.
> > 
> > Currently the other fsverity_operations functions are just aligned with what the
> > filesystems actually need: ->read_merkle_tree_page takes a page index, whereas
> > ->write_merkle_tree_block takes a pos and size.  xfs_fsverity_read_merkle()
> > needs pos, index, and size, and fsverity_read_merkle_tree_block() computes the
> > index already.  So that's why I'd probably go with pos, index, and size instead
> > of pos, log2_blocksize, and size.  There's no right answer though.
> 
> How about just index and size?  I think XFS will work just fine without
> the byte position since it's using that as a key for the cache and the
> ondisk xattrs anyway.  Even if we can't decide on which data structure
> to use for caching. ;)

Currently XFS is using the byte position as the key for the ondisk xattrs.  That
could be changed if you want it to be the block index instead, though.

(I also notice you're using big endian, which isn't a great choice these days.
But I assume it's for consistency with the rest of XFS.)

- Eric