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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
> 




[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux