Re: [PATCH v5 7/9] iomap/xfs: Eliminate the iomap_valid handler

Andreas Gruenbacher
2023-01-08T18:50:59Z

On Sun, Jan 8, 2023 at 6:32 PM Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Jan 04, 2023 at 07:08:17PM +0000, Matthew Wilcox wrote:
> > On Wed, Jan 04, 2023 at 09:53:17AM -0800, Darrick J. Wong wrote:
> > > I wonder if this should be reworked a bit to reduce indenting:
> > >
> > > if (PTR_ERR(folio) == -ESTALE) {
> >
> > FYI this is a bad habit to be in. The compiler can optimise
> >
> > if (folio == ERR_PTR(-ESTALE))
> >
> > better than it can optimise the other way around.
>
> Yes. I think doing the recording that Darrick suggested combined
> with this style would be best:
>
> if (folio == ERR_PTR(-ESTALE)) {
> iter->iomap.flags |= IOMAP_F_STALE;
> return 0;
> }
> if (IS_ERR(folio))
> return PTR_ERR(folio);

Again, I've implemented this as a nested if because the -ESTALE case
should be pretty rare, and if we unnest, we end up with an additional
check on the main code path. To be specific, the "before" code here on
my current system is this:

------------------------------------
if (IS_ERR(folio)) {
22ad: 48 81 fd 00 f0 ff ff cmp $0xfffffffffffff000,%rbp
22b4: 0f 87 bf 03 00 00 ja 2679 <iomap_write_begin+0x499>
return 0;
}
return PTR_ERR(folio);
}
[...]
2679: 89 e8 mov %ebp,%eax
if (folio == ERR_PTR(-ESTALE)) {
267b: 48 83 fd 8c cmp $0xffffffffffffff8c,%rbp
267f: 0f 85 b7 fc ff ff jne 233c <iomap_write_begin+0x15c>
iter->iomap.flags |= IOMAP_F_STALE;
2685: 66 81 4b 42 00 02 orw $0x200,0x42(%rbx)
return 0;
268b: e9 aa fc ff ff jmp 233a <iomap_write_begin+0x15a>
------------------------------------

While the "after" code is this:

------------------------------------
if (folio == ERR_PTR(-ESTALE)) {
22ad: 48 83 fd 8c cmp $0xffffffffffffff8c,%rbp
22b1: 0f 84 bc 00 00 00 je 2373 <iomap_write_begin+0x193>
iter->iomap.flags |= IOMAP_F_STALE;
return 0;
}
if (IS_ERR(folio))
return PTR_ERR(folio);
22b7: 89 e8 mov %ebp,%eax
if (IS_ERR(folio))
22b9: 48 81 fd 00 f0 ff ff cmp $0xfffffffffffff000,%rbp
22c0: 0f 87 82 00 00 00 ja 2348 <iomap_write_begin+0x168>
------------------------------------

The compiler isn't smart enough to re-nest the ifs by recognizing that
folio == ERR_PTR(-ESTALE) is a subset of IS_ERR(folio).

So do you still insist on that un-nesting even though it produces worse code?

Thanks,
Andreas