Re: [PATCH 8/9] btrfs: simplify eb checksum verification in btrfs_validate_metadata_buffer

[David Sterba <dsterba@suse.cz>]

On Wed, May 26, 2021 at 08:05:51AM +0800, Qu Wenruo wrote:
> 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 <wqu@suse.com>
> 
> But still a nitpick inlined below.
> > ---
> >   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.

Ah right, I wanted to use the offsetof for clarity but that it could be
used with nodes makes it confusing again. What if it's replaced by

	get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));

This makes it clear that it's the checksum and from the experience we
know it's at offset 0. I'd rather avoid magic constants and offsets but
you're right that everybody knows that the checksum is at the beginning
of btree block.