Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer
From: Qu Wenruo <hidden>
Date: 2021-05-26 00:05:58
On 2021/5/26 上午1:08, David Sterba wrote:
The verification copies the calculated checksum bytes to a temporary buffer but this is not necessary. We can map the eb header on the first page and use the checksum bytes directly. This saves at least one function call and boundary checks so it could lead to a minor performance improvement. Signed-off-by: David Sterba <dsterba@suse.com>
The idea is good, and should be pretty simple to test. Reviewed-by: Qu Wenruo <redacted> But still a nitpick inlined below.
quoted hunk ↗ jump to hunk
--- fs/btrfs/disk-io.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 6dc137684899..868e358f6923 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c@@ -584,6 +584,7 @@ static int validate_extent_buffer(struct extent_buffer *eb) const u32 csum_size = fs_info->csum_size; u8 found_level; u8 result[BTRFS_CSUM_SIZE]; + const struct btrfs_header *header; int ret = 0; found_start = btrfs_header_bytenr(eb);@@ -608,15 +609,14 @@ static int validate_extent_buffer(struct extent_buffer *eb) } csum_tree_block(eb, result); + header = page_address(eb->pages[0]) + + get_eb_offset_in_page(eb, offsetof(struct btrfs_leaf, header));
It takes me near a minute to figure why it's not just "get_eb_offset_in_page(eb, 0)". I'm not sure if we really need that explicit way to just get 0, especially most of us (and even some advanced users) know that csum comes at the very beginning of a tree block. And the mention of btrfs_leave can sometimes be confusing, especially when we could have tree nodes passed in. Thanks, Qu
- if (memcmp_extent_buffer(eb, result, 0, csum_size)) {
- u8 val[BTRFS_CSUM_SIZE] = { 0 };
-
- read_extent_buffer(eb, &val, 0, csum_size);
+ if (memcmp(result, header->csum, csum_size) != 0) {
btrfs_warn_rl(fs_info,
"checksum verify failed on %llu wanted " CSUM_FMT " found " CSUM_FMT " level %d",
eb->start,
- CSUM_FMT_VALUE(csum_size, val),
+ CSUM_FMT_VALUE(csum_size, header->csum),
CSUM_FMT_VALUE(csum_size, result),
btrfs_header_level(eb));
ret = -EUCLEAN;