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

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

[Chandan Babu R]

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.

Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 585f7e795023..63fcac13d29c 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -927,13 +927,16 @@ xfs_bmap_add_attrfork_btree(
 	xfs_inode_t		*ip,		/* incore inode pointer */
 	int			*flags)		/* inode logging flags */
 {
+	struct xfs_btree_block	*block;
 	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))
+	block = ip->i_df.if_broot;
+
+	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

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

[Christoph Hellwig]

On Thu, Apr 01, 2021 at 10:15:25PM +0530, Chandan Babu R wrote:
> @@ -927,13 +927,16 @@ xfs_bmap_add_attrfork_btree(
>  	xfs_inode_t		*ip,		/* incore inode pointer */
>  	int			*flags)		/* inode logging flags */
>  {
> +	struct xfs_btree_block	*block;
>  	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))
> +	block = ip->i_df.if_broot;

Just initializing block o nthe line it is declared would read a little
easier.  Other than that this looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

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

[Chandan Babu R]

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.

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

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

[Darrick J. Wong]

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
> 

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

[Chandan Babu R]

On 02 Apr 2021 at 21:09, Darrick J. Wong wrote:
> 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?

Yes, that is correct.

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

You are right. I will post the corresponding patch soon.

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

Thanks for reporting the bug. I will see what is going on there.

-- 
chandan