On Tuesday, April 7, 2020 6:19 AM Dave Chinner wrote:
> [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.
> */
Sure. I will change the comment. Thanks for the review.
--
chandan
