Re: [PATCH v2 04/11] btrfs: introduce extra sanity checks for extent maps

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/5/13 21:51, Filipe Manana 写道:
> On Fri, May 3, 2024 at 7:02 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> Since extent_map structure has the all the needed members to represent a
>> file extent directly, we can apply all the file extent sanity checks to an extent
>> map.
>>
>> The new sanity checks would cross check both the old members
>> (block_start/block_len/orig_start) and the new members
>> (disk_bytenr/disk_num_bytes/offset).
>>
>> There is a special case for offset/orig_start/start cross check, we only
>> do such sanity check for compressed extent:
>>
>> - Only compressed read/encoded write really utilize orig_start
>>    This can be proved by the cleanup patch of orig_start.
>>
>> - Merged data extents can lead to false alerts
>>    The problem is, with disk_bytenr/disk_num_bytes, if we're merging
>>    two extent maps like this:
>>
>>      |<- data extent A -->|<-- data extent B -->|
>>                |<- em 1 ->|<- em 2 ->|
>>
>>    Let's assume em2 has orig_offset of 0 and start of 0, and obvisouly
>>    offset 0.
>
> obvisouly -> obviously
>
> I'm confused. How can em2 have a "start" of 0?
> By "start" you mean file offset I suppose, since that's what it is
> before this patchset. That would mean em 1 would have a negative
> "start".

My bad, I screwed up the example numbers.

For "offset" I mean the the offset inside the data extent, aka,
"btrfs_file_extent_item::offset"
And for "start" I mean filepos, aka the key.offset.

And in above case, it's all dependent on the filepos of the em1/em2.

I would change the example to something like this:

em1: file pos 4k, len 4k, extent offset 4k, ram bytes 8k, disk_bytenr X,
disk_num_bytes 8K, compress none.

Then it's orig_start should be 0.

em2: file pos 8k, len 4k, extent offset 0, ram_bytes 8K,
disk_bytenr X + 8K, disk_num_bytes 8K, compress none.

And its orig_start should be 8K.

>
> And by "offset" you mean extent offset?
>
>>
>>    But after merging, the merged em would have offset of em1, screwing up
>
> But this suggests that by "offset" you mean file offset and not the
> offset within an extent's range.

No, I still mean "extent offset".

As the "merged" em would be:

em merged: filepos 4k, len 8K, extent offset 4K, ram_bytes 16K,
disk_bytenr X, disk_num_bytes 16K, compress none.

Thus the orig_offset would be still be 0, the same as the em1 before
merging.

> So did you mean "start" here?
>
>>    whatever the @orig_start cross check against @start.
>>
>> The checks happens at the following timing:
>>
>> - add_extent_mapping()
>>    This is for newly added extent map
>>
>> - replace_extent_mapping()
>>    This is for btrfs_drop_extent_map_range() and split_extent_map()
>>
>> - try_merge_map()
>>
>> Since the check is way more strict than before, the following code has
>> to be modified to pass the check:
>>
>> - extent-map-tests
>>    Previously the test case never populate ram_bytes, not to mention the
>>    newly introduced disk_bytenr/disk_num_bytes.
>>    Populate the involved numbers mostly to follow the existing
>>    block_start/block_len values.
>>
>>    There are two special cases worth mentioning:
>>    - test_case_3()
>>      The test case is already way too invalid that tree-checker will
>>      reject almost all extents.
>>
>>      And there is a special unaligned regular extent which has mismatch
>>      disk_num_bytes (4096) and ram_bytes (4096 - 1).
>>      Fix it by all assigned the disk_num_bytes and ram_bytes to 4096 - 1.
>
> Looking at the diff for test_case_3(), I don't see any assignment of
> 4096 - 1 anywhere.
> All the assignments I see have a value of SZ_4K or SZ_16K.

It's in fact setup_file_extents() from inode-tests.c, I got confused
with other fixes.

>
>>
>>    - test_case_7()
>>      An extent is inserted with 16K length, but on-disk extent size is
>>      only 4K.
>>      This means it must be a compressed extent, so set the compressed flag
>>      for it.
>>
>> - setup_relocation_extent_mapping()
>>    This is mostly utilized by relocation code to read the chunk like an
>>    inode.
>
> Not "mostly" - it's exclusively used by the relocation code.
>
>>    So populate the extent map using a regular non-compressed extent.
>
> Isn't that what we already do? I'm confused.
> We are already creating a regular non-compressed extent, and all the
> diff does is to initialize some fields unrelated to that.

It's more like initializing the remaining members, or those
uninitialized members would not pass cross-check.

I guess I should put the missing initialization where I introduced them?

>
>>
>> In fact, the new cross checks already exposed a bug in
>> btrfs_drop_extent_map_range(), and caught tons of bugs in the new
>> members assignment.
>
> I remember one bug in btrfs_drop_extent_map_range(), which fortunately
> didn't have any consequences.
> Those tons of bugs you mention are only in the code added by the
> patchset if I understand you correctly.

Isn't that why I mention is as "in the new members assignment"?

Or should I just drop the mention completely?

Thanks,
Qu