On Tue 14 Sep 2021 at 16:36, David Sterba [off-list ref] wrote:

On Tue, Sep 14, 2021 at 10:08:36PM +0800, Su Yue wrote:

quoted

On Tue 14 Sep 2021 at 15:12, David Sterba [off-list ref] 
wrote:

quoted

On Tue, Sep 14, 2021 at 06:53:35PM +0800, Su Yue wrote:

quoted

It looks strange that btrfs_node_key is in struct-funcs.c.
So move it to ctree.h and make it static inline.

"looks strange" is not a sufficient reason. Inlining a 
function
means
that the body will be expanded at each call site, bloating 
the
binary
code. Have you measured the impact of that?

Fair enough.

Before:
   text    data     bss     dec     hex filename
1202418  123105   19384 1344907  14858b fs/btrfs/btrfs.ko
After:
   text    data     bss     dec     hex filename
1202620  123105   19384 1345109  148655 fs/btrfs/btrfs.ko

+202

quoted

There's some performance cost of an non-inline function due 
to
the call
overhead but it does not make sense to inline a function 
that's
called
rarely and not in a tight loop. If you grep for the function
you'd see
that it's called eg. once per function or in a loop that's 
not
performance critical on first sight (eg. in 
reada_for_search).

Right, the patch won't impact performance obviously at the cost 
of
+202 binary size. So I'd drop the patch.

It does increase the size a bit but from what I've seen in the 
assembly
it's not that bad and still probably worth doing the inline. 
There's one
more extra call to read_extent_buffer (hidden behind 
read_eb_member
macro).

Thanks for taking a look. I just noticed the lonely function then 
post
the patch without deeper thinking.

Cleaning up the node key helpers would be useful too, adding 
some
more helpers and not calling read_eb_member in the end. I have a 
WIP
patchset for that but had to leave it as there were bugs I did 
not find.
I can bounce it to you if you're interested.

Thanks a lot. But I can't take it due to some reasons. Since you 
have
a better WIP patchset, I don't mind forgoing the patch.

--
Su