Re: [PATCH 2/7] btrfs: accessors: use type sizeof constants directly

Boris Burkov
2025-07-02T17:57:20Z

On Tue, Jul 01, 2025 at 07:23:49PM +0200, David Sterba wrote:
> Now unit_size is used only once, so use it directly in 'part'
> calculation. Don't cache sizeof(type) in a variable. While this is a
> compile-time constant, forcing the type 'int' generates worse code as it
> leads to additional conversion from 32 to 64 bit type on x86_64.
>
> The sizeof() is used only a few times and it does not make the code that
> harder to read, so use it directly and let the compiler utilize the
> immediate constants in the context it needs. The .ko code size slightly
> increases (+50) but further patches will reduce that again.
>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
> fs/btrfs/accessors.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/accessors.c b/fs/btrfs/accessors.c
> index b54c8abe467a06..2e90b9b14e73f4 100644
> --- a/fs/btrfs/accessors.c
> +++ b/fs/btrfs/accessors.c
> @@ -52,19 +52,17 @@ u##bits btrfs_get_##bits(const struct extent_buffer *eb, \
> const unsigned long idx = get_eb_folio_index(eb, member_offset);\
> const unsigned long oil = get_eb_offset_in_folio(eb, \
> member_offset);\
> - const int unit_size = eb->folio_size; \
> char *kaddr = folio_address(eb->folios[idx]); \
> - const int size = sizeof(u##bits); \
> - const int part = unit_size - oil; \
> + const int part = eb->folio_size - oil; \

nit: the names oil and part are pretty non-sensical to me. Oil used to
be oip for Offset In Page. Is it Offset In foLio?

I can't figure out what part should mean.

So while I see why you're doing all the changes, I can' help but notice
that you removed the two named variables with logical names and left the
confusing ones. :)

> u8 lebytes[sizeof(u##bits)]; \
> \
> - ASSERT(check_setget_bounds(eb, ptr, off, size)); \
> + ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits))); \
> if (INLINE_EXTENT_BUFFER_PAGES == 1 || likely(sizeof(u##bits) <= part)) \
> return get_unaligned_le##bits(kaddr + oil); \
> \
> memcpy(lebytes, kaddr + oil, part); \
> kaddr = folio_address(eb->folios[idx + 1]); \
> - memcpy(lebytes + part, kaddr, size - part); \
> + memcpy(lebytes + part, kaddr, sizeof(u##bits) - part); \
> return get_unaligned_le##bits(lebytes); \
> } \
> void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> @@ -74,13 +72,11 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> const unsigned long idx = get_eb_folio_index(eb, member_offset);\
> const unsigned long oil = get_eb_offset_in_folio(eb, \
> member_offset);\
> - const int unit_size = eb->folio_size; \
> char *kaddr = folio_address(eb->folios[idx]); \
> - const int size = sizeof(u##bits); \
> - const int part = unit_size - oil; \
> + const int part = eb->folio_size - oil; \
> u8 lebytes[sizeof(u##bits)]; \
> \
> - ASSERT(check_setget_bounds(eb, ptr, off, size)); \
> + ASSERT(check_setget_bounds(eb, ptr, off, sizeof(u##bits))); \
> if (INLINE_EXTENT_BUFFER_PAGES == 1 || \
> likely(sizeof(u##bits) <= part)) { \
> put_unaligned_le##bits(val, kaddr + oil); \
> @@ -90,7 +86,7 @@ void btrfs_set_##bits(const struct extent_buffer *eb, void *ptr, \
> put_unaligned_le##bits(val, lebytes); \
> memcpy(kaddr + oil, lebytes, part); \
> kaddr = folio_address(eb->folios[idx + 1]); \
> - memcpy(kaddr, lebytes + part, size - part); \
> + memcpy(kaddr, lebytes + part, sizeof(u##bits) - part); \
> }
>
> DEFINE_BTRFS_SETGET_BITS(8)
> --
> 2.49.0
>