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

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 02, 2025 at 10:58:54AM -0700, Boris Burkov wrote:
> 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.

It confused me the whole time I was looking at the code, it snuck in
with the folio changes, it should have been 'oif' follwing the previous
naming pattern.

> 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. :)

So I can sneak in a patch renaming it at the beginning or at the end,
naming it 'oif' or 'foff' (there's the eb related offset as parameter so
we need some kind of distinction).