Skip to content

Commit 5672225

Browse files
Dave Chinnerdchinner
authored andcommitted
xfs: avoid unnecessary runtime sibling pointer endian conversions
Commit dc04db2 has caused a small aim7 regression, showing a small increase in CPU usage in __xfs_btree_check_sblock() as a result of the extra checking. This is likely due to the endian conversion of the sibling poitners being unconditional instead of relying on the compiler to endian convert the NULL pointer at compile time and avoiding the runtime conversion for this common case. Rework the checks so that endian conversion of the sibling pointers is only done if they are not null as the original code did. .... and these need to be "inline" because the compiler completely fails to inline them automatically like it should be doing. $ size fs/xfs/libxfs/xfs_btree.o* text data bss dec hex filename 51874 240 0 52114 cb92 fs/xfs/libxfs/xfs_btree.o.orig 51562 240 0 51802 ca5a fs/xfs/libxfs/xfs_btree.o.inline Just when you think the tools have advanced sufficiently we don't have to care about stuff like this anymore, along comes a reminder that *our tools still suck*. Fixes: dc04db2 ("xfs: detect self referencing btree sibling pointers") Reported-by: kernel test robot <oliver.sang@intel.com> Signed-off-by: Dave Chinner <dchinner@redhat.com> Reviewed-by: Darrick J. Wong <djwong@kernel.org> Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Dave Chinner <david@fromorbit.com>
1 parent ab6a8d3 commit 5672225

File tree

1 file changed

+33
-14
lines changed

1 file changed

+33
-14
lines changed

fs/xfs/libxfs/xfs_btree.c

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,31 @@ xfs_btree_magic(
5151
return magic;
5252
}
5353

54-
static xfs_failaddr_t
54+
/*
55+
* These sibling pointer checks are optimised for null sibling pointers. This
56+
* happens a lot, and we don't need to byte swap at runtime if the sibling
57+
* pointer is NULL.
58+
*
59+
* These are explicitly marked at inline because the cost of calling them as
60+
* functions instead of inlining them is about 36 bytes extra code per call site
61+
* on x86-64. Yes, gcc-11 fails to inline them, and explicit inlining of these
62+
* two sibling check functions reduces the compiled code size by over 300
63+
* bytes.
64+
*/
65+
static inline xfs_failaddr_t
5566
xfs_btree_check_lblock_siblings(
5667
struct xfs_mount *mp,
5768
struct xfs_btree_cur *cur,
5869
int level,
5970
xfs_fsblock_t fsb,
60-
xfs_fsblock_t sibling)
71+
__be64 dsibling)
6172
{
62-
if (sibling == NULLFSBLOCK)
73+
xfs_fsblock_t sibling;
74+
75+
if (dsibling == cpu_to_be64(NULLFSBLOCK))
6376
return NULL;
77+
78+
sibling = be64_to_cpu(dsibling);
6479
if (sibling == fsb)
6580
return __this_address;
6681
if (level >= 0) {
@@ -74,17 +89,21 @@ xfs_btree_check_lblock_siblings(
7489
return NULL;
7590
}
7691

77-
static xfs_failaddr_t
92+
static inline xfs_failaddr_t
7893
xfs_btree_check_sblock_siblings(
7994
struct xfs_mount *mp,
8095
struct xfs_btree_cur *cur,
8196
int level,
8297
xfs_agnumber_t agno,
8398
xfs_agblock_t agbno,
84-
xfs_agblock_t sibling)
99+
__be32 dsibling)
85100
{
86-
if (sibling == NULLAGBLOCK)
101+
xfs_agblock_t sibling;
102+
103+
if (dsibling == cpu_to_be32(NULLAGBLOCK))
87104
return NULL;
105+
106+
sibling = be32_to_cpu(dsibling);
88107
if (sibling == agbno)
89108
return __this_address;
90109
if (level >= 0) {
@@ -136,10 +155,10 @@ __xfs_btree_check_lblock(
136155
fsb = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
137156

138157
fa = xfs_btree_check_lblock_siblings(mp, cur, level, fsb,
139-
be64_to_cpu(block->bb_u.l.bb_leftsib));
158+
block->bb_u.l.bb_leftsib);
140159
if (!fa)
141160
fa = xfs_btree_check_lblock_siblings(mp, cur, level, fsb,
142-
be64_to_cpu(block->bb_u.l.bb_rightsib));
161+
block->bb_u.l.bb_rightsib);
143162
return fa;
144163
}
145164

@@ -204,10 +223,10 @@ __xfs_btree_check_sblock(
204223
}
205224

206225
fa = xfs_btree_check_sblock_siblings(mp, cur, level, agno, agbno,
207-
be32_to_cpu(block->bb_u.s.bb_leftsib));
226+
block->bb_u.s.bb_leftsib);
208227
if (!fa)
209228
fa = xfs_btree_check_sblock_siblings(mp, cur, level, agno,
210-
agbno, be32_to_cpu(block->bb_u.s.bb_rightsib));
229+
agbno, block->bb_u.s.bb_rightsib);
211230
return fa;
212231
}
213232

@@ -4523,10 +4542,10 @@ xfs_btree_lblock_verify(
45234542
/* sibling pointer verification */
45244543
fsb = XFS_DADDR_TO_FSB(mp, xfs_buf_daddr(bp));
45254544
fa = xfs_btree_check_lblock_siblings(mp, NULL, -1, fsb,
4526-
be64_to_cpu(block->bb_u.l.bb_leftsib));
4545+
block->bb_u.l.bb_leftsib);
45274546
if (!fa)
45284547
fa = xfs_btree_check_lblock_siblings(mp, NULL, -1, fsb,
4529-
be64_to_cpu(block->bb_u.l.bb_rightsib));
4548+
block->bb_u.l.bb_rightsib);
45304549
return fa;
45314550
}
45324551

@@ -4580,10 +4599,10 @@ xfs_btree_sblock_verify(
45804599
agno = xfs_daddr_to_agno(mp, xfs_buf_daddr(bp));
45814600
agbno = xfs_daddr_to_agbno(mp, xfs_buf_daddr(bp));
45824601
fa = xfs_btree_check_sblock_siblings(mp, NULL, -1, agno, agbno,
4583-
be32_to_cpu(block->bb_u.s.bb_leftsib));
4602+
block->bb_u.s.bb_leftsib);
45844603
if (!fa)
45854604
fa = xfs_btree_check_sblock_siblings(mp, NULL, -1, agno, agbno,
4586-
be32_to_cpu(block->bb_u.s.bb_rightsib));
4605+
block->bb_u.s.bb_rightsib);
45874606
return fa;
45884607
}
45894608

0 commit comments

Comments
 (0)