On Thu, Apr 25, 2024 at 12:46:26AM +0000, Eric Biggers wrote:
> 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) {'?
Nope.
> > I'm assuming that you /do/ want the error code in fsverity_read_merkle_tree?
>
> Yes, that's needed to log it.
Got 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.
/me slaps forehead
Yeah, I forgot that the ondisk format uses the byte position, it's only
the caching layer that cares about the block index. And it doesn't even
have to do that, the per-AG rhashtable is working fine and doesn't
impose a 16-byte expansion on every xfs_inode everywhere.
> (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.)
Yep.
--D
> - Eric
>
