74 lines
2.8 KiB
Diff
74 lines
2.8 KiB
Diff
|
From f5ef812888a81be534466fa34df747c16bb65b7f Mon Sep 17 00:00:00 2001
|
||
|
|
From: Guo Xuenan <guoxuenan@huawei.com>
|
||
|
|
Date: Wed, 15 Mar 2023 15:57:35 +0100
|
||
|
|
Subject: [PATCH] xfs: get rid of assert from xfs_btree_islastblock
|
||
|
|
|
||
|
|
Source kernel commit: 8c25febf23963431686f04874b96321288504127
|
||
|
|
|
||
|
|
xfs_btree_check_block contains debugging knobs. With XFS_DEBUG setting up,
|
||
|
|
turn on the debugging knob can trigger the assert of xfs_btree_islastblock,
|
||
|
|
test script as follows:
|
||
|
|
|
||
|
|
while true
|
||
|
|
do
|
||
|
|
mount $disk $mountpoint
|
||
|
|
fsstress -d $testdir -l 0 -n 10000 -p 4 >/dev/null
|
||
|
|
echo 1 > /sys/fs/xfs/sda/errortag/btree_chk_sblk
|
||
|
|
sleep 10
|
||
|
|
umount $mountpoint
|
||
|
|
done
|
||
|
|
|
||
|
|
Kick off fsstress and only *then* turn on the debugging knob. If it
|
||
|
|
happens that the knob gets turned on after the cntbt lookup succeeds
|
||
|
|
but before the call to xfs_btree_islastblock, then we *can* end up in
|
||
|
|
the situation where a previously checked btree block suddenly starts
|
||
|
|
returning EFSCORRUPTED from xfs_btree_check_block. Kaboom.
|
||
|
|
|
||
|
|
Darrick give a very detailed explanation as follows:
|
||
|
|
Looking back at commit 27d9ee577dcce, I think the point of all this was
|
||
|
|
to make sure that the cursor has actually performed a lookup, and that
|
||
|
|
the btree block at whatever level we're asking about is ok.
|
||
|
|
|
||
|
|
If the caller hasn't ever done a lookup, the bc_levels array will be
|
||
|
|
empty, so cur->bc_levels[level].bp pointer will be NULL. The call to
|
||
|
|
xfs_btree_get_block will crash anyway, so the "ASSERT(block);" part is
|
||
|
|
pointless.
|
||
|
|
|
||
|
|
If the caller did a lookup but the lookup failed due to block
|
||
|
|
corruption, the corresponding cur->bc_levels[level].bp pointer will also
|
||
|
|
be NULL, and we'll still crash. The "ASSERT(xfs_btree_check_block);"
|
||
|
|
logic is also unnecessary.
|
||
|
|
|
||
|
|
If the cursor level points to an inode root, the block buffer will be
|
||
|
|
incore, so it had better always be consistent.
|
||
|
|
|
||
|
|
If the caller ignores a failed lookup after a successful one and calls
|
||
|
|
this function, the cursor state is garbage and the assert wouldn't have
|
||
|
|
tripped anyway. So get rid of the assert.
|
||
|
|
|
||
|
|
Fixes: 27d9ee577dcc ("xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock")
|
||
|
|
Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
|
||
|
|
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
|
||
|
|
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
|
||
|
|
Signed-off-by: Carlos Maiolino <cem@kernel.org>
|
||
|
|
Signed-off-by: Pavel Reichl <preichl@redhat.com>
|
||
|
|
---
|
||
|
|
libxfs/xfs_btree.h | 1 -
|
||
|
|
1 file changed, 1 deletion(-)
|
||
|
|
|
||
|
|
diff --git a/libxfs/xfs_btree.h b/libxfs/xfs_btree.h
|
||
|
|
index eef27858..29c4b4cc 100644
|
||
|
|
--- a/libxfs/xfs_btree.h
|
||
|
|
+++ b/libxfs/xfs_btree.h
|
||
|
|
@@ -556,7 +556,6 @@ xfs_btree_islastblock(
|
||
|
|
struct xfs_buf *bp;
|
||
|
|
|
||
|
|
block = xfs_btree_get_block(cur, level, &bp);
|
||
|
|
- ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);
|
||
|
|
|
||
|
|
if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
|
||
|
|
return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
|
||
|
|
--
|
||
|
|
2.40.0
|
||
|
|
|