Re: [PATCH] btrfs: add a test for btrfs fsverity

[Boris Burkov <boris@bur.io>]

Thu, Feb 04, 2021 at 10:04:07PM -0800, Eric Biggers wrote:
> Thanks for writing a test for this!
> 
> On Thu, Feb 04, 2021 at 03:24:26PM -0800, Boris Burkov wrote:
> > There are some btrfs specific fsverity scenarios that don't map
> > neatly onto the tests in generic/574, like holes, inline extents,
> > and preallocated extents. Cover those in a btrfs specific test.
> > 
> > That test relies on assumptions about how the Merkle tree is stored
> > by ext4/f2fs which don't apply to btrfs, so we also test Merkle tree
> > corruption here. This could be merged by some generic abstraction.
> 
> The only part of generic/574 that cares where the Merkle tree is stored is
> _fsv_scratch_corrupt_merkle_tree().  Couldn't that be updated to handle btrfs?
> 

I agree that would be an easy enough fix, I'll make it in this patch.
Until I get 574 fully passing, I think I ought to leave the Merkle
corruption here as well, though, right?

> > Finally, that test relies extensively on fiemap, which is currently
> > broken on btrfs for offsets and sizes that don't align to PAGE_SIZE,
> > so put a simple regular file case in this test for now, while we fix
> > fiemap or generalize extent lookup.
> 
> fiemap is only used by _fsv_scratch_corrupt_bytes().  It just wants to know the
> list of extents that intersect the requested byte range.  Does that really not
> work on btrfs if the range isn't page-aligned?
> 

Unfortunately, fiemap is in fact broken in btrfs in that case, and prints
silly stuff like [K..K-1]. I wrote up a fix for it, but am still figuring
out how to test it, and decided to get the verity stuff out ahead of it.

However, even if I did get that fix in, it would still not be quite
right. Btrfs fiemap is in terms of logical block addresses, so
an additional translation with btrfs-map-logical is needed, and I
couldn't figure out how to elegantly inject that into
_fsv_scratch_corrupt_bytes. I do hope to get btrfs to pass generic/574
soon, though.

> - Eric 

Thanks for the review,
Boris