Re: [PATCH] bcachefs: Remove the handling of bch2_trans_iter_exit() in __bch2_bkey_get_iter()

[Hongbo Li]

On 2024/8/30 11:48, Hongbo Li wrote:
> On 2024/8/30 9:19, Youling Tang wrote:
> > On 2024/8/23 22:51, Kent Overstreet wrote:
> >
> > > On Fri, Aug 23, 2024 at 02:07:20PM GMT, Youling Tang wrote:
> > > > On 23/08/2024 11:55, Kent Overstreet wrote:
> > > > > On Fri, Aug 23, 2024 at 11:19:55AM GMT, Youling Tang wrote:
> > > > > > From: Youling Tang <tangyouling@xxxxxxxxxx>
> > > > > >
> > > > > > - Reduces bkey_err() calls.
> > > > > > - Avoid redundant calls to bch2_trans_iter_exit() in some functions.
> > > > > no, a function that returns an error should clean up after itself
> > > > Yes, functions should self-clean when they fail.
> > > >
> > > > However, there are repeated calls to bch2_trans_iter_exit in
> > > > some functions, take lookup_inode() as an example,
This is indeed the case. Personally, I think the bcachefs code is quite 
difficult to understand (such as too many macro, etc..), with many parts 
that could be simplified. I think may be only Kent himself can fully 
hold it.

> > > > When bkey_err(k) returns a non-zero, call bch2_trans_iter_exit()
> > > > once in bch2_bkey_get_iter(). It is then called again in
> > > > lookup_inode() via 'goto err'. (We can correct it by simply changing
> > > > it to 'return ret', but there are many similar cases.)
> > > I'm aware, but I'm not looking to microoptimize at the expense of making
> > > the code more fragile and less clear, especially right now when the
> > > priority is stabilizing and fixing bugs.
> > >
> > > If you were also doing performance testing and could show that it
> > > makes a measurable difference I'd consider it. Did you even look at the
> > > assembly output for any of these functions? CSE might be optimizing away
> > > the redundant calls.
> > I haven't performed the corresponding performance testing. Looking at
> > the assembly code, taking `lookup_inode()` as an example,
> I think his point is that your modification is error-free, but improving 
> readability is not a high priority at the moment unless it actually 
> affects performance. There might currently be a shortage of manpower, 
> with many features to be completed and bugs to be fixed.
> > Before the patch,
> >      142f:       74 96 je     13c7 <lookup_inode+0x117>
> >      1431:       48 8d b5 68 ff ff ff lea    -0x98(%rbp),%rsi
> >      1438:       4c 89 e7 mov    %r12,%rdi
> >      143b:       e8 00 00 00 00 call   1440 <lookup_inode+0x190>
> >      1440:       eb b4 jmp    13f6 <lookup_inode+0x146>
> >      1442:       e8 00 00 00 00 call   1447 <lookup_inode+0x197>
> >      1447:       66 0f 1f 84 00 00 00 nopw   0x0(%rax,%rax,1)
> >      144e:       00 00
> >
> > After,
> >      111f:       74 96 je     10b7 <lookup_inode+0x117>
> >      1121:       eb c3 jmp    10e6 <lookup_inode+0x146>
> >      1123:       e8 00 00 00 00 call   1128 <lookup_inode+0x188>
> >      1128:       0f 1f 84 00 00 00 00 nopl   0x0(%rax,%rax,1)
> >      112f:       00
> >
> > The following three assembly instructions have been reduced,
> >      1431:       48 8d b5 68 ff ff ff lea    -0x98(%rbp),%rsi
> >      1438:       4c 89 e7 mov    %r12,%rdi
> >      143b:       e8 00 00 00 00 call   1440 <lookup_inode+0x190>