Re: [PATCH 1/2] xfs: Fix log reservation calculation for xattr insert operation

[Chandan Rajendra <chandan@linux.ibm.com>]

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