Re: [PATCH] xfs: fix incorrect usage of xfs_btree_check_block

[Guo Xuenan <guoxuenan@huawei.com>]

On 2022/11/8 0:58, Darrick J. Wong wrote:
> On Thu, Nov 03, 2022 at 07:37:09PM +0800, Guo Xuenan wrote:
>> xfs_btree_check_block contains a tag XFS_ERRTAG_BTREE_CHECK_{L,S}BLOCK,
>> it is a fault injection tag, better not use it in the macro ASSERT.
>>
>> Since with XFS_DEBUG setting up, we can always trigger assert by `echo 1
>>> /sys/fs/xfs/${disk}/errortag/btree_chk_{s,l}blk`.
>> It's confusing and strange.
> Please be more specific about how this is confusing or strange.
I meant in current code, the ASSERT will alway happen,when we
`echo 1 > /sys/fs/xfs/${disk}/errortag/btree_chk_{s,l}blk`.
xfs_btree_islastblock
   ->ASSERT(block && xfs_btree_check_block(cur, block, level, bp) == 0);
     ->xfs_btree_check_{l/s}block
       ->XFS_TEST_ERROR(false, mp, XFS_ERRTAG_BTREE_CHECK_{S,L}BLOCK)
we can use error injection to trigger this ASSERT.
I think ASERRT macro and error injection are to find some effective 
problems,
not to create some kernel panic. So, putting the error injection 
function in
ASSERT is a little strange.

>> Instead of using it in ASSERT, replace it with
>> xfs_warn.
>>
>> Fixes: 27d9ee577dcc ("xfs: actually check xfs_btree_check_block return in xfs_btree_islastblock")
>> Signed-off-by: Guo Xuenan <guoxuenan@huawei.com>
>> ---
>>   fs/xfs/libxfs/xfs_btree.h | 7 +++++--
>>   1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_btree.h b/fs/xfs/libxfs/xfs_btree.h
>> index eef27858a013..637513087c18 100644
>> --- a/fs/xfs/libxfs/xfs_btree.h
>> +++ b/fs/xfs/libxfs/xfs_btree.h
>> @@ -556,8 +556,11 @@ 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);
>> -
>> +	ASSERT(block);
>> +#if defined(DEBUG) || defined(XFS_WARN)
>> +	if (xfs_btree_check_block(cur, block, level, bp))
>> +		xfs_warn(cur->bc_mp, "%s: xfs_btree_check_block() error.", __func__);
>> +#endif
> ...because this seems like open-coding ASSERT, possibly without the
> panic on errors part.
yes，exactly！I also think it can be deleted, but i have no idea if this 
is necessary,
I just retain it in this fix patch; looking forward to your decision :)
> --D
>
>>   	if (cur->bc_flags & XFS_BTREE_LONG_PTRS)
>>   		return block->bb_u.l.bb_rightsib == cpu_to_be64(NULLFSBLOCK);
>>   	return block->bb_u.s.bb_rightsib == cpu_to_be32(NULLAGBLOCK);
>> -- 
>> 2.31.1
>>
> .

-- 
Guo Xuenan [OS Kernel Lab]
-----------------------------
Email: guoxuenan@huawei.com