[chopped bits out of the diff to get the whole reservation in one
obvious piece of code.]
On Sat, Apr 04, 2020 at 02:22:02PM +0530, Chandan Rajendra wrote:
> @@ -698,42 +699,36 @@ xfs_calc_attrinval_reservation(
> }
>
> /*
> + * Setting an attribute.
> * the inode getting the attribute
> * the superblock for allocations
> + * the agf extents are allocated from
> * the attribute btree * max depth
> + * the bmbt entries for da btree blocks
> + * the bmbt entries for remote blocks (if any)
> + * the allocation btrees.
> */
> STATIC uint
> -xfs_calc_attrsetm_reservation(
> +xfs_calc_attrset_reservation(
> struct xfs_mount *mp)
> {
> + int max_rmt_blks;
> + int da_blks;
> + int bmbt_blks;
> +
> + da_blks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
#define XFS_DAENTER_BLOCKS(mp,w) \
(XFS_DAENTER_1B(mp,w) * XFS_DAENTER_DBS(mp,w))
#define XFS_DAENTER_1B(mp,w) \
((w) == XFS_DATA_FORK ? (mp)->m_dir_geo->fsbcount : 1)
#define XFS_DAENTER_DBS(mp,w) \
(XFS_DA_NODE_MAXDEPTH + (((w) == XFS_DATA_FORK) ? 2 : 0))
So, da_blks contains the full da btree split depth * 1 block. i.e.
da_blks = XFS_DA_NODE_MAXDEPTH;
> + bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
#define XFS_DAENTER_BMAPS(mp,w) \
(XFS_DAENTER_DBS(mp,w) * XFS_DAENTER_BMAP1B(mp,w))
#define XFS_DAENTER_BMAP1B(mp,w) \
XFS_NEXTENTADD_SPACE_RES(mp, XFS_DAENTER_1B(mp, w), w)
So, bmbt_blks contains the full da btree split depth * the BMBT
overhead for a single block allocation:
#define XFS_EXTENTADD_SPACE_RES(mp,w) (XFS_BM_MAXLEVELS(mp,w) - 1)
#define XFS_NEXTENTADD_SPACE_RES(mp,b,w)\
(((b + XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp) - 1) / \
XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp)) * \
XFS_EXTENTADD_SPACE_RES(mp,w))
XFS_NEXTENTADD_SPACE_RES(1) = ((1 + N - 1) / N) * (XFS_BM_MAXLEVELS - 1)
= (XFS_BM_MAXLEVELS - 1)
So, bmbt_blks = XFS_DA_NODE_MAXDEPTH * (XFS_BM_MAXLEVELS - 1)
IOWs, this bmbt reservation is assuming a full height BMBT
modification on *every* dabtree node allocation. IOWs, we're
reserving multiple times the log space for potential bmbt
modifications than we are for the entire dabtree modification.
That's why the individual dabtree reservations are so big....
> + max_rmt_blks = xfs_attr3_rmt_blocks(mp, XATTR_SIZE_MAX);
> + bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, max_rmt_blks, XFS_ATTR_FORK);
And this assumes we are going to log at least another full bmbt
modification.
IT seems to me that the worst case here is one full split and then
every other allocation inserts at the start of an existing block and
so updates pointers all the way up to the root. The impact is
limited, though, because XFS_DA_NODE_MAXDEPTH = 5 and so the attr
fork BMBT tree is not likely to reach anywhere near it's max depth
on large filesystems.....
> return XFS_DQUOT_LOGRES(mp) +
> xfs_calc_inode_res(mp, 1) +
> xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> + xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> + xfs_calc_buf_res(da_blks, XFS_FSB_TO_B(mp, 1)) +
> + xfs_calc_buf_res(bmbt_blks, XFS_FSB_TO_B(mp, 1)) +
> + xfs_calc_buf_res(xfs_allocfree_log_count(mp, da_blks),
> XFS_FSB_TO_B(mp, 1));
Given the above, this looks OK. Worst case BMBT usage looks
excessive, but there is a chance it could be required...
> diff --git a/fs/xfs/libxfs/xfs_trans_space.h b/fs/xfs/libxfs/xfs_trans_space.h
> index 88221c7a04ccf..6a22ad11b3825 100644
> --- a/fs/xfs/libxfs/xfs_trans_space.h
> +++ b/fs/xfs/libxfs/xfs_trans_space.h
> @@ -38,8 +38,14 @@
>
> #define XFS_DAENTER_1B(mp,w) \
> ((w) == XFS_DATA_FORK ? (mp)->m_dir_geo->fsbcount : 1)
> +/*
> + * xattr set operation can cause the da btree to split twice in the
> + * worst case. The double split is actually an extra leaf node rather
> + * than a complete split of blocks in the path from root to a
> + * leaf. The '1' in the macro below accounts for the extra leaf node.
It's not a double tree split, so don't describe it that way and then
have to explain that it's not a double tree split!
/*
* When inserting a large local record into the dabtree leaf, we may
* need to split the leaf twice to make room to fit the new record
* into the new leaf. This double leaf split still only requires a
* single datree path update as the inserted leaves are at adjacent
* indexes. Hence we only need to account for an the extra leaf
* block in the attribute fork here.
*/
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
