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

[Boris Burkov <boris@bur.io>]

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
>