xfs: fix unaligned access to rmapbt high keys in scrub
Subsystems (2)
PATCH v2 1/1 xfs: fix unaligned access to rmapbt high keys in scrub
An rmapbt node stores each key as a low/high pair of packed 20-byte struct xfs_rmap_key, so every high key is only 4-byte aligned. xchk_rmapbt_check_unwritten_in_keyflags() reads rm_offset through union xfs_btree_key, which is 8-byte aligned, so the compiler may use an 8-byte load. On sparc64 every high key it checks then takes an unaligned access trap:
Kernel unaligned access at TPC[105a54d8] xchk_rmapbt_check_unwritten_in_keyflags+0xb8/0x160 [xfs]
Read both keys through struct xfs_rmap_key and convert rm_offset with be64_to_cpu(), like the other rmapbt key accessors, instead of comparing against a byte-swapped flag.
---
Tested on sparc64 (T4, 7.3.3) by scrubbing the rmapbt of a loop-mounted
image: alignment faults per scrub went from about 4000 to 0.
v2: read both keys through struct xfs_rmap_key with be64_to_cpu() and drop the byte-swapped badflag (Dave). be64_to_cpu() on the union alone still traps, as it is a no-op on big-endian.
v1: https://lore.kernel.org/all/[email protected]/
fs/xfs/scrub/rmap.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
fs/xfs/scrub/rmap.c +5 -6
@@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags(struct xfs_scrub *sc = bs->sc;struct xfs_btree_cur *cur = bs->cur;struct xfs_btree_block *keyblock; -union xfs_btree_key *lkey, *hkey; -__be64 badflag =cpu_to_be64 (XFS_RMAP_OFF_UNWRITTEN ); +const struct xfs_rmap_key *lkey, *hkey;unsigned int level;if (sc->sm->sm_flags &XFS_SCRUB_OFLAG_PREEN )
@@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags( keyblock =xfs_btree_get_block (cur, level, &bp);for (ptr =1 ; ptr <=be16_to_cpu (keyblock->bb_numrecs); ptr++) { - lkey =xfs_btree_key_addr (cur, ptr, keyblock); + lkey = &xfs_btree_key_addr (cur, ptr, keyblock)->rmap; -if (lkey->rmap.rm_offset & badflag) { +if (be64_to_cpu (lkey->rm_offset) &XFS_RMAP_OFF_UNWRITTEN ) {xchk_btree_set_preen (sc, cur, level);break ; } - hkey =xfs_btree_high_key_addr (cur, ptr, keyblock); -if (hkey->rmap.rm_offset & badflag) { + hkey = &xfs_btree_high_key_addr (cur, ptr, keyblock)->rmap; +if (be64_to_cpu (hkey->rm_offset) &XFS_RMAP_OFF_UNWRITTEN ) {xchk_btree_set_preen (sc, cur, level);break ; }
Signature · 1 line
2.43.0
Quoted 5 lines · Stian Halseth
An rmapbt node stores each key as a low/high pair of packed 20-byte struct
xfs_rmap_key, so every high key is only 4-byte aligned.xchk_rmapbt_check_unwritten_in_keyflags()readsrm_offsetthrough unionxfs_btree_key, which is 8-byte aligned, so the compiler may use an 8-byte load.
Ok, this trips my 'written by an LLM' filter. i.e. long run-on sentences, strung together with commas and "so ...". I mention this because....
Quoted 15 lines
fs/xfs/scrub/rmap.c +1
-2@@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags(struct xfs_scrub *sc = bs->sc;struct xfs_btree_cur *cur = bs->cur;struct xfs_btree_block *keyblock; -union xfs_btree_key *lkey, *hkey; -__be64 badflag =cpu_to_be64 (XFS_RMAP_OFF_UNWRITTEN ); +const struct xfs_rmap_key *lkey, *hkey;unsigned int level;if (sc->sm->sm_flags &XFS_SCRUB_OFLAG_PREEN )
fs/xfs/scrub/rmap.c xchk_rmapbt_check_unwritten_in_keyflags( · line 168
Quoted 5 lines
fs/xfs/scrub/rmap.c +1
-1@@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags( keyblock =xfs_btree_get_block (cur, level, &bp);for (ptr =1 ; ptr <=be16_to_cpu (keyblock->bb_numrecs); ptr++) { - lkey =xfs_btree_key_addr (cur, ptr, keyblock); + lkey = &xfs_btree_key_addr (cur, ptr, keyblock)->rmap;
The code fix smells of LLM, too: technically correct, but unreadable and unmaintainable.
Regardless of where it came from, it's objectively -poor code- because:
- it uses a pattern that is not found anywhere else in the XFS code.
- it costs nothing to use two variables here which would make the code much more readable for a maintainer.
- no comment to explain the issue the code has to avoid.
At minimum, a comment explaining why the code needs to be structured this way is needed. Humans like me are going to forget why the code needed to be structured this way in short order. Hence a comment is needed to remind me why the code was written this way next time I look at it.
IOWs, the code should look something like this:
/*
* The on-disk high key in the rmap block may not be naturally
* aligned, so we have to tell the compiler that we are
* decoding rmap keys so it treats them correctly as an
* unaligned packed structure.
*/
union xfs_btree_key *key;
const struct xfs_rmap_key *rkey;
key = xfs_btree_key_addr(cur, ptr, keyblock);
rkey = &key->rmap;
if (be64_to_cpu(rkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
xchk_btree_set_preen(sc, cur, level);
break;
}
key = xfs_btree_high_key_addr(cur, ptr, keyblock);
rkey = &key->rmap;
if (be64_to_cpu(rkey->rm_offset) & XFS_RMAP_OFF_UNWRITTEN) {
xchk_btree_set_preen(sc, cur, level);
break;
}-Dave.
Signature · 2 lines
Dave Chinner [email protected]
Hi Dave,
Quoted 10 lines · Dave Chinner
Quoted 5 lines · Stian Halseth
An rmapbt node stores each key as a low/high pair of packed 20-byte struct
xfs_rmap_key, so every high key is only 4-byte aligned.xchk_rmapbt_check_unwritten_in_keyflags()readsrm_offsetthrough unionxfs_btree_key, which is 8-byte aligned, so the compiler may use an 8-byte load.Ok, this trips my 'written by an LLM' filter. i.e. long run-on sentences, strung together with commas and "so ...". I mention this because....
The changelog was drafted with LLM help, and I'll rewrite it for v3.
Quoted 36 lines
Quoted 23 lines
fs/xfs/scrub/rmap.c +1
-2@@ -149,8 +149,7 @@ xchk_rmapbt_check_unwritten_in_keyflags( struct xfs_scrub *sc = bs->sc; struct xfs_btree_cur *cur = bs->cur; struct xfs_btree_block *keyblock; -union xfs_btree_key *lkey, *hkey; -__be64 badflag =cpu_to_be64 (XFS_RMAP_OFF_UNWRITTEN ); +const struct xfs_rmap_key *lkey, *hkey; unsigned int level;
if (sc->sm->sm_flags&XFS_SCRUB_OFLAG_PREEN)fs/xfs/scrub/rmap.c +0
-0@@ -166,15 +165,15 @@ xchk_rmapbt_check_unwritten_in_keyflags(
keyblock =xfs_btree_get_block(cur, level, &bp); for (ptr = 1; ptr <=be16_to_cpu(keyblock->bb_numrecs); ptr++) {fs/xfs/scrub/rmap.c +1
-1- lkey =xfs_btree_key_addr (cur, ptr, keyblock); + lkey = &xfs_btree_key_addr(cur, ptr, keyblock)-Quoted 1 line
Quoted 1 line
rmap;
The code fix smells of LLM, too: technically correct, but unreadable and unmaintainable.
Regardless of where it came from, it's objectively -poor code- because:
- it uses a pattern that is not found anywhere else in the XFS code.
- it costs nothing to use two variables here which would make the code much more readable for a maintainer.
- no comment to explain the issue the code has to avoid.
Quoted 5 lines
At minimum, a comment explaining why the code needs to be structured this way is needed. Humans like me are going to forget why the code needed to be structured this way in short order. Hence a comment is needed to remind me why the code was written this way next time I look at it.
Thank you for the feedback, and no argument from me on those three points.
You're correct in your assumption that LLM assistance was used for the final code. Earlier iterations were more readable, and I should have kept them. I try to keep my patches small, and this time it came at the cost of readability. I stand by the correctness and testing, but will send a v3 following your suggestion, which reads much better.
I take this feedback to heart, and it's truly useful for my personal development. My goal is to learn and improve. I will pay closer attention to the existing code patterns, and try to adhere more closely to them. I'm very sceptical of the output I get from the LLM models, so I test everything to the best of my abilities before posting.
/Stian