Re: [PATCH] xfs: scrub: Disable check for non-optimized data fork bmbt node

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

On Mon, Apr 05, 2021 at 01:17:42PM +0530, Chandan Babu R wrote:
> xchk_btree_check_minrecs() checks if the contents of the immediate child of a
> bmbt root block can fit within the root block. This check could fail on inodes
> with an attr fork since xfs_bmap_add_attrfork_btree() used to demote the
> current root node of the data fork as the child of a newly allocated root node
> if it found that the size of "struct xfs_btree_block" along with the space
> required for records exceeded that of space available in the data fork.
> 
> xfs_bmap_add_attrfork_btree() should have used "struct xfs_bmdr_block" instead
> of "struct xfs_btree_block" for the above mentioned space requirement
> calculation. This commit disables the check for non-optimized (in terms of
> disk space usage) data fork bmbt trees since there could be filesystems
> in use that already have such a layout.
> 
> Reported-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
>  fs/xfs/scrub/btree.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> index debf392e0515..79e0aa484b4a 100644
> --- a/fs/xfs/scrub/btree.c
> +++ b/fs/xfs/scrub/btree.c
> @@ -9,6 +9,7 @@
>  #include "xfs_format.h"
>  #include "xfs_trans_resv.h"
>  #include "xfs_mount.h"
> +#include "xfs_inode.h"
>  #include "xfs_btree.h"
>  #include "scrub/scrub.h"
>  #include "scrub/common.h"
> @@ -469,14 +470,17 @@ xchk_btree_check_minrecs(
>  	 */
>  	if ((cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) &&
>  	    level == cur->bc_nlevels - 2) {
> +		struct xfs_inode 	*ip = bs->sc->ip;
>  		struct xfs_btree_block	*root_block;
>  		struct xfs_buf		*root_bp;
>  		int			root_maxrecs;
> +		int			whichfork = cur->bc_ino.whichfork;
>  
>  		root_block = xfs_btree_get_block(cur, root_level, &root_bp);
>  		root_maxrecs = cur->bc_ops->get_dmaxrecs(cur, root_level);
>  		if (be16_to_cpu(root_block->bb_numrecs) != 1 ||
> -		    numrecs <= root_maxrecs)
> +		    (!(whichfork == XFS_DATA_FORK && XFS_IFORK_Q(ip)) &&

This isn't an issue now, but it will become one when inodes gain the
ability to host roots of btrees that are not just the block mapping
btrees.  (Specifically, realtime reflink and refcounting, which are
buried deep in djwong-dev...)  It might be a good idea to check the
btree cursor type so that we don't leave a logic bomb.

I think I'd like a code level comment capturing what happened and why we
occasionally skip checks.  Something like this?

/* Decide if we want to check minrecs of a btree block in the inode root. */
static inline bool
xchk_btree_check_iroot_minrecs(
	struct xchk_btree	*bs)
{
	/*
	 * xfs_bmap_add_attrfork_btree had an implementation bug wherein it
	 * would miscalculate the space required for the data fork bmbt root
	 * when adding an attr fork, and promote the iroot contents to an
	 * external block unnecessarily.  This went unnoticed for many years
	 * until scrub found filesystems in this state.  Inode rooted btrees
	 * are not supposed to have immediate child blocks that are small
	 * enough that the contents could fit in the inode root, but we can't
	 * fail existing filesystems, so instead we disable the check for data
	 * fork bmap btrees when there's an attr fork.
	 */
	if (bs->cur->bc_btnum == XFS_BTNUM_BMAP &&
	    bs->cur->bc_ino.whichfork == XFS_DATA_FORK &&
	    XFS_IFORK_Q(ip))
		return false;

	return false;
}

		if (xchk_btree_check_iroot_minrecs(bs) &&
		    (be16_to_cpu(root_block->bb_numrecs) != 1 ||
		     numrecs <= root_maxrecs))
			xchk_btree_set_corrupt(...);

Code-wise, this patch looks very similar to what I was drafting in my head. :)

--D


> +		     numrecs <= root_maxrecs))
>  			xchk_btree_set_corrupt(bs->sc, cur, level);
>  		return;
>  	}
> -- 
> 2.29.2
>