| Message ID | 6828072ccda5d55b9d130f48b750455ea728781b.1621961965.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc fixups and cleanups | expand |
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. 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; >
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.
On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote: > > > + 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. --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -584,7 +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; + const u8 *header_csum; int ret = 0; found_start = btrfs_header_bytenr(eb); @@ -609,14 +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)); + header_csum = page_address(eb->pages[0]) + + get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum)); - if (memcmp(result, header->csum, csum_size) != 0) { + 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, header->csum), + CSUM_FMT_VALUE(csum_size, header_csum), CSUM_FMT_VALUE(csum_size, result), btrfs_header_level(eb)); ret = -EUCLEAN;
On Thu, May 27, 2021 at 07:13:24AM +0800, Qu Wenruo wrote: > > > On 2021/5/27 上午12:58, David Sterba wrote: > > On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote: > >>>> + 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. > > > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -584,7 +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; > > + const u8 *header_csum; > > int ret = 0; > > > > found_start = btrfs_header_bytenr(eb); > > @@ -609,14 +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)); > > + header_csum = page_address(eb->pages[0]) + > > + get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum)); > > This version looks better to me. Thanks, I've preemptively squashed it to the commit, now in misc-next.
On 2021/5/27 上午12:58, David Sterba wrote: > On Wed, May 26, 2021 at 06:31:39PM +0200, David Sterba wrote: >>>> + 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. > > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -584,7 +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; > + const u8 *header_csum; > int ret = 0; > > found_start = btrfs_header_bytenr(eb); > @@ -609,14 +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)); > + header_csum = page_address(eb->pages[0]) + > + get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum)); This version looks better to me. THanks, Qu > > - if (memcmp(result, header->csum, csum_size) != 0) { > + 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, header->csum), > + CSUM_FMT_VALUE(csum_size, header_csum), > CSUM_FMT_VALUE(csum_size, result), > btrfs_header_level(eb)); > ret = -EUCLEAN; >
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)); - 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;
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> --- fs/btrfs/disk-io.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)