Re: [PATCH V1.1] xfs: Use struct xfs_bmdr_block instead of struct xfs_btree_block to calculate root node size

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

On Fri, Apr 02, 2021 at 05:21:00PM +0530, Chandan Babu R wrote:
> The incore data fork of an inode stores the bmap btree root node as 'struct
> xfs_btree_block'. However, the ondisk version of the inode stores the bmap
> btree root node as a 'struct xfs_bmdr_block'.
> 
> xfs_bmap_add_attrfork_btree() checks if the btree root node fits inside the
> data fork of the inode. However, it incorrectly uses 'struct xfs_btree_block'
> to compute the size of the bmap btree root node. Since size of 'struct
> xfs_btree_block' is larger than that of 'struct xfs_bmdr_block',
> xfs_bmap_add_attrfork_btree() could end up unnecessarily demoting the current
> root node as the child of newly allocated root node.
> 
> This commit optimizes space usage by modifying xfs_bmap_add_attrfork_btree()
> to use 'struct xfs_bmdr_block' to check if the bmap btree root node fits
> inside the data fork of the inode.

Hmm.  This introduces a (compatible) change in the ondisk format, since
we no longer promote the data fork btree root block unnecessarily, right?

We've been writing out filesystems in that state for years, so I think
scrub is going to need patching to disable the "could the root block
contents fit in the inode root?" check on the data fork if there's an
attr fork.

Meanwhile, this fix looks decent.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

By the way, have you tried running xfs/{529-538} on a realtime
filesystem formatted with -d rtinherit=1 ?  There's something odd
causing them to fail, but it's realtime so who knows what that's
about. :)

I really like how these extent counter overflow checks are finding
longstanding bugs!

--D

> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> ---
> V1 -> V1.1
>   1. Initialize "block" variable during declaration.
>   
>  fs/xfs/libxfs/xfs_bmap.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 585f7e795023..006dd2150a6f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -927,13 +927,15 @@ xfs_bmap_add_attrfork_btree(
>  	xfs_inode_t		*ip,		/* incore inode pointer */
>  	int			*flags)		/* inode logging flags */
>  {
> +	struct xfs_btree_block	*block = ip->i_df.if_broot;
>  	xfs_btree_cur_t		*cur;		/* btree cursor */
>  	int			error;		/* error return value */
>  	xfs_mount_t		*mp;		/* file system mount struct */
>  	int			stat;		/* newroot status */
>  
>  	mp = ip->i_mount;
> -	if (ip->i_df.if_broot_bytes <= XFS_IFORK_DSIZE(ip))
> +
> +	if (XFS_BMAP_BMDR_SPACE(block) <= XFS_IFORK_DSIZE(ip))
>  		*flags |= XFS_ILOG_DBROOT;
>  	else {
>  		cur = xfs_bmbt_init_cursor(mp, tp, ip, XFS_DATA_FORK);
> -- 
> 2.29.2
>