Re: [PATCH V4 RESEND 3/7] xfs: xfs_attr_calc_size: Calculate Bmbt blks only once

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

On Tuesday, February 25, 2020 9:41 PM Brian Foster wrote: 
> On Mon, Feb 24, 2020 at 09:30:40AM +0530, Chandan Rajendra wrote:
> > The number of Bmbt blocks that is required can be calculated only once by
> > passing the sum of total number of dabtree blocks and remote blocks to
> > XFS_NEXTENTADD_SPACE_RES() macro.
> > 
> > Signed-off-by: Chandan Rajendra <chandanrlinux@gmail.com>
> > ---
> 
> According to the cover letter this is fixing a reservation calculation
> issue, though the commit log kind of gives the impression it's a
> refactor. Can you elaborate on what this fixes in the commit log?
> 

XFS_NEXTENTADD_SPACE_RES() first figures out the number of Bmbt leaf blocks
needed for mapping the 'block count' passed to it as the second argument.
When calculating the number of leaf blocks, it accommodates the 'block count'
argument in groups of XFS_MAX_CONTIG_EXTENTS_PER_BLOCK(mp). For each such
group it decides that a bmbt leaf block is required. For each of the leaf
blocks that needs to be allocated, it assumes that there will be a split of
the bmbt tree from root to leaf. Hence it multiplies the number of leaf blocks
with the maximum height of the tree.

With two individual calls to XFS_NEXTENTADD_SPACE_RES() (one is indirectly
through the call to XFS_DAENTER_BMAPS() => XFS_DAENTER_BMAP1B() and the other
is for remote attr blocks) we miss out on the opportunity to group the bmbt
leaf blocks and hence overcompensate on the bmbt blocks calculation.

Please let me know if my understanding is incorrect.

> 
> >  fs/xfs/libxfs/xfs_attr.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
> > index 942ba552e0bdd..a708b142f69b6 100644
> > --- a/fs/xfs/libxfs/xfs_attr.c
> > +++ b/fs/xfs/libxfs/xfs_attr.c
> > @@ -154,12 +154,10 @@ xfs_attr_calc_size(
> >  	size = xfs_attr_leaf_newentsize(args->geo, args->namelen,
> >  			args->valuelen, local);
> >  	total_dablks = XFS_DAENTER_BLOCKS(mp, XFS_ATTR_FORK);
> > -	bmbt_blks = XFS_DAENTER_BMAPS(mp, XFS_ATTR_FORK);
> >  	if (*local) {
> >  		if (size > (args->geo->blksize / 2)) {
> >  			/* Double split possible */
> >  			total_dablks *= 2;
> > -			bmbt_blks *= 2;
> >  		}
> >  		rmt_blks = 0;
> >  	} else {
> > @@ -168,10 +166,11 @@ xfs_attr_calc_size(
> >  		 * make room for the attribute value itself.
> >  		 */
> >  		rmt_blks = xfs_attr3_rmt_blocks(mp, args->valuelen);
> > -		bmbt_blks += XFS_NEXTENTADD_SPACE_RES(mp, rmt_blks,
> > -				XFS_ATTR_FORK);
> >  	}
> >  
> > +	bmbt_blks = XFS_NEXTENTADD_SPACE_RES(mp, total_dablks + rmt_blks,
> > +			XFS_ATTR_FORK);
> > +
> >  	return total_dablks + rmt_blks + bmbt_blks;
> >  }
> >  
> 
> 

-- 
chandan