| Message ID | 20210402115100.13478-1-chandanrlinux@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | [V1.1] xfs: Use struct xfs_bmdr_block instead of struct xfs_btree_block to calculate root node size | expand |
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 >
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.
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);