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

["Darrick J. Wong" <djwong@kernel.org>]

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.

> 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.

--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
>