Re: [PATCH 5/6] xfs: factor free space tree transaciton reservations

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, May 27, 2021 at 02:52:01PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Convert all the open coded free space tree modification reservations
> to use the new xfs_allocfree_extent_res() function.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_trans_resv.c | 122 ++++++++++++---------------------
>  1 file changed, 45 insertions(+), 77 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_trans_resv.c b/fs/xfs/libxfs/xfs_trans_resv.c
> index 6363cacb790f..02079f55ef20 100644
> --- a/fs/xfs/libxfs/xfs_trans_resv.c
> +++ b/fs/xfs/libxfs/xfs_trans_resv.c
> @@ -143,18 +143,16 @@ xfs_calc_inode_res(
>   * reservation:
>   *
>   * the inode btree: max depth * blocksize
> - * the allocation btrees: 2 trees * (max depth - 1) * block size
> + * one extent allocfree reservation for the AG.
>   *
> - * The caller must account for SB and AG header modifications, etc.
>   */
>  STATIC uint
>  xfs_calc_inobt_res(
>  	struct xfs_mount	*mp)
>  {
>  	return xfs_calc_buf_res(M_IGEO(mp)->inobt_maxlevels,
> -			XFS_FSB_TO_B(mp, 1)) +
> -				xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -			XFS_FSB_TO_B(mp, 1));
> +				XFS_FSB_TO_B(mp, 1)) +
> +		xfs_allocfree_extent_res(mp);

Hm.  xfs_allocfree_extent_res computes the space to handle full splits
of both free space btrees and updates to the AGFL and AGF.

This function did not previously account for AG header updates -- it
only computes the space needed to log a split of an inode btree and
splits of both free space btrees.  It looks like xfs_calc_inobt_res's
callers themselves add reservation space for AG headers.

So, doesn't this introduce double-counting of the AGF for inode
operations?  It's probably not a big deal to allocate more space, but
this doesn't look like a straight refactoring.

>  }
>  
>  /*
> @@ -182,7 +180,7 @@ xfs_calc_finobt_res(
>   * Calculate the reservation required to allocate or free an inode chunk. This
>   * includes:
>   *
> - * the allocation btrees: 2 trees * (max depth - 1) * block size
> + * one extent allocfree reservation for the AG.
>   * the inode chunk: m_ino_geo.ialloc_blks * N
>   *
>   * The size N of the inode chunk reservation depends on whether it is for
> @@ -200,8 +198,7 @@ xfs_calc_inode_chunk_res(
>  {
>  	uint			res, size = 0;
>  
> -	res = xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -			       XFS_FSB_TO_B(mp, 1));
> +	res = xfs_allocfree_extent_res(mp);

Same question here.

>  	if (alloc) {
>  		/* icreate tx uses ordered buffers */
>  		if (xfs_sb_version_has_v3inode(&mp->m_sb))
> @@ -256,22 +253,18 @@ xfs_rtalloc_log_count(
>   * extents.  This gives (t1):
>   *    the inode getting the new extents: inode size
>   *    the inode's bmap btree: max depth * block size
> - *    the agfs of the ags from which the extents are allocated: 2 * sector
>   *    the superblock free block counter: sector size
> - *    the allocation btrees: 2 exts * 2 trees * (2 * max depth - 1) * block size
> + *    two extent allocfree reservations for the AG.
>   * Or, if we're writing to a realtime file (t2):
>   *    the inode getting the new extents: inode size
>   *    the inode's bmap btree: max depth * block size
> - *    the agfs of the ags from which the extents are allocated: 2 * sector
> + *    one extent allocfree reservation for the AG.
>   *    the superblock free block counter: sector size
>   *    the realtime bitmap: ((MAXEXTLEN / rtextsize) / NBBY) bytes
>   *    the realtime summary: 1 block
> - *    the allocation btrees: 2 trees * (2 * max depth - 1) * block size
>   * And the bmap_finish transaction can free bmap blocks in a join (t3):
> - *    the agfs of the ags containing the blocks: 2 * sector size
> - *    the agfls of the ags containing the blocks: 2 * sector size
>   *    the super block free block counter: sector size
> - *    the allocation btrees: 2 exts * 2 trees * (2 * max depth - 1) * block size
> + *    two extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_write_reservation(
> @@ -282,8 +275,8 @@ xfs_calc_write_reservation(
>  
>  	t1 = xfs_calc_inode_res(mp, 1) +
>  	     xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK), blksz) +
> -	     xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> -	     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 2), blksz);
> +	     xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +	     xfs_allocfree_extent_res(mp) * 2;

This conversion looks ok.

>  
>  	if (xfs_sb_version_hasrealtime(&mp->m_sb)) {
>  		t2 = xfs_calc_inode_res(mp, 1) +
> @@ -291,13 +284,13 @@ xfs_calc_write_reservation(
>  				     blksz) +
>  		     xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
>  		     xfs_calc_buf_res(xfs_rtalloc_log_count(mp, 1), blksz) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1), blksz);
> +		     xfs_allocfree_extent_res(mp);

Shouldn't this change the 3 above to 1?

Same questions for the rest of this patch.

--D

>  	} else {
>  		t2 = 0;
>  	}
>  
> -	t3 = xfs_calc_buf_res(5, mp->m_sb.sb_sectsize) +
> -	     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 2), blksz);
> +	t3 = xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +	     xfs_allocfree_extent_res(mp) * 2;
>  
>  	return XFS_DQUOT_LOGRES(mp) + max3(t1, t2, t3);
>  }
> @@ -307,19 +300,13 @@ xfs_calc_write_reservation(
>   *    the inode being truncated: inode size
>   *    the inode's bmap btree: (max depth + 1) * block size
>   * And the bmap_finish transaction can free the blocks and bmap blocks (t2):
> - *    the agf for each of the ags: 4 * sector size
> - *    the agfl for each of the ags: 4 * sector size
>   *    the super block to reflect the freed blocks: sector size
> - *    worst case split in allocation btrees per extent assuming 4 extents:
> - *		4 exts * 2 trees * (2 * max depth - 1) * block size
> + *    four extent allocfree reservations for the AG.
>   * Or, if it's a realtime file (t3):
> - *    the agf for each of the ags: 2 * sector size
> - *    the agfl for each of the ags: 2 * sector size
>   *    the super block to reflect the freed blocks: sector size
>   *    the realtime bitmap: 2 exts * ((MAXEXTLEN / rtextsize) / NBBY) bytes
>   *    the realtime summary: 2 exts * 1 block
> - *    worst case split in allocation btrees per extent assuming 2 extents:
> - *		2 exts * 2 trees * (2 * max depth - 1) * block size
> + *    two extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_itruncate_reservation(
> @@ -331,13 +318,13 @@ xfs_calc_itruncate_reservation(
>  	t1 = xfs_calc_inode_res(mp, 1) +
>  	     xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) + 1, blksz);
>  
> -	t2 = xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
> -	     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4), blksz);
> +	t2 = xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +	     xfs_allocfree_extent_res(mp) * 4;
>  
>  	if (xfs_sb_version_hasrealtime(&mp->m_sb)) {
> -		t3 = xfs_calc_buf_res(5, mp->m_sb.sb_sectsize) +
> +		t3 = xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
>  		     xfs_calc_buf_res(xfs_rtalloc_log_count(mp, 2), blksz) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 2), blksz);
> +		     xfs_allocfree_extent_res(mp) * 2;
>  	} else {
>  		t3 = 0;
>  	}
> @@ -352,10 +339,8 @@ xfs_calc_itruncate_reservation(
>   *    the two directory bmap btrees: 2 * max depth * block size
>   * And the bmap_finish transaction can free dir and bmap blocks (two sets
>   *	of bmap blocks) giving:
> - *    the agf for the ags in which the blocks live: 3 * sector size
> - *    the agfl for the ags in which the blocks live: 3 * sector size
>   *    the superblock for the free block count: sector size
> - *    the allocation btrees: 3 exts * 2 trees * (2 * max depth - 1) * block size
> + *    three extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_rename_reservation(
> @@ -365,9 +350,8 @@ xfs_calc_rename_reservation(
>  		max((xfs_calc_inode_res(mp, 4) +
>  		     xfs_calc_buf_res(2 * XFS_DIROP_LOG_COUNT(mp),
>  				      XFS_FSB_TO_B(mp, 1))),
> -		    (xfs_calc_buf_res(7, mp->m_sb.sb_sectsize) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 3),
> -				      XFS_FSB_TO_B(mp, 1))));
> +		    (xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +		     xfs_allocfree_extent_res(mp) * 3));
>  }
>  
>  /*
> @@ -381,20 +365,19 @@ xfs_calc_iunlink_remove_reservation(
>  	struct xfs_mount        *mp)
>  {
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -	       2 * M_IGEO(mp)->inode_cluster_size;
> +	       xfs_calc_buf_res(2, M_IGEO(mp)->inode_cluster_size);
>  }
>  
>  /*
>   * For creating a link to an inode:
> + *    the inode is removed from the iunlink list (O_TMPFILE)
>   *    the parent directory inode: inode size
>   *    the linked inode: inode size
>   *    the directory btree could split: (max depth + v2) * dir block size
>   *    the directory bmap btree could join or split: (max depth + v2) * blocksize
>   * And the bmap_finish transaction can free some bmap blocks giving:
> - *    the agf for the ag in which the blocks live: sector size
> - *    the agfl for the ag in which the blocks live: sector size
>   *    the superblock for the free block count: sector size
> - *    the allocation btrees: 2 trees * (2 * max depth - 1) * block size
> + *    one extent allocfree reservation for the AG.
>   */
>  STATIC uint
>  xfs_calc_link_reservation(
> @@ -405,9 +388,8 @@ xfs_calc_link_reservation(
>  		max((xfs_calc_inode_res(mp, 2) +
>  		     xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp),
>  				      XFS_FSB_TO_B(mp, 1))),
> -		    (xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				      XFS_FSB_TO_B(mp, 1))));
> +		    (xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +		     xfs_allocfree_extent_res(mp)));
>  }
>  
>  /*
> @@ -419,20 +401,19 @@ STATIC uint
>  xfs_calc_iunlink_add_reservation(xfs_mount_t *mp)
>  {
>  	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> -			M_IGEO(mp)->inode_cluster_size;
> +	       xfs_calc_buf_res(1, M_IGEO(mp)->inode_cluster_size);
>  }
>  
>  /*
>   * For removing a directory entry we can modify:
> + *    the inode is added to the agi unlinked list
>   *    the parent directory inode: inode size
>   *    the removed inode: inode size
>   *    the directory btree could join: (max depth + v2) * dir block size
>   *    the directory bmap btree could join or split: (max depth + v2) * blocksize
>   * And the bmap_finish transaction can free the dir and bmap blocks giving:
> - *    the agf for the ag in which the blocks live: 2 * sector size
> - *    the agfl for the ag in which the blocks live: 2 * sector size
>   *    the superblock for the free block count: sector size
> - *    the allocation btrees: 2 exts * 2 trees * (2 * max depth - 1) * block size
> + *    two extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_remove_reservation(
> @@ -443,9 +424,8 @@ xfs_calc_remove_reservation(
>  		max((xfs_calc_inode_res(mp, 1) +
>  		     xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp),
>  				      XFS_FSB_TO_B(mp, 1))),
> -		    (xfs_calc_buf_res(4, mp->m_sb.sb_sectsize) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 2),
> -				      XFS_FSB_TO_B(mp, 1))));
> +		    (xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +		     xfs_allocfree_extent_res(mp) * 2));
>  }
>  
>  /*
> @@ -581,16 +561,14 @@ xfs_calc_ichange_reservation(
>  /*
>   * Growing the data section of the filesystem.
>   *	superblock
> - *	agi and agf
> - *	allocation btrees
> + *      one extent allocfree reservation for the AG.
>   */
>  STATIC uint
>  xfs_calc_growdata_reservation(
>  	struct xfs_mount	*mp)
>  {
> -	return xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1));
> +	return xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +	       xfs_allocfree_extent_res(mp);
>  }
>  
>  /*
> @@ -598,10 +576,9 @@ xfs_calc_growdata_reservation(
>   * In the first set of transactions (ALLOC) we allocate space to the
>   * bitmap or summary files.
>   *	superblock: sector size
> - *	agf of the ag from which the extent is allocated: sector size
>   *	bmap btree for bitmap/summary inode: max depth * blocksize
>   *	bitmap/summary inode: inode size
> - *	allocation btrees for 1 block alloc: 2 * (2 * maxdepth - 1) * blocksize
> + *      one extent allocfree reservation for the AG.
>   */
>  STATIC uint
>  xfs_calc_growrtalloc_reservation(
> @@ -611,8 +588,7 @@ xfs_calc_growrtalloc_reservation(
>  		xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK),
>  				 XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_inode_res(mp, 1) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1));
> +		xfs_allocfree_extent_res(mp);
>  }
>  
>  /*
> @@ -675,7 +651,7 @@ xfs_calc_writeid_reservation(
>   *	agf block and superblock (for block allocation)
>   *	the new block (directory sized)
>   *	bmap blocks for the new directory block
> - *	allocation btrees
> + *      one extent allocfree reservation for the AG.
>   */
>  STATIC uint
>  xfs_calc_addafork_reservation(
> @@ -687,8 +663,7 @@ xfs_calc_addafork_reservation(
>  		xfs_calc_buf_res(1, mp->m_dir_geo->blksize) +
>  		xfs_calc_buf_res(XFS_DAENTER_BMAP1B(mp, XFS_DATA_FORK) + 1,
>  				 XFS_FSB_TO_B(mp, 1)) +
> -		xfs_calc_buf_res(xfs_allocfree_log_count(mp, 1),
> -				 XFS_FSB_TO_B(mp, 1));
> +		xfs_allocfree_extent_res(mp);
>  }
>  
>  /*
> @@ -696,11 +671,8 @@ xfs_calc_addafork_reservation(
>   *    the inode being truncated: inode size
>   *    the inode's bmap btree: max depth * block size
>   * And the bmap_finish transaction can free the blocks and bmap blocks:
> - *    the agf for each of the ags: 4 * sector size
> - *    the agfl for each of the ags: 4 * sector size
>   *    the super block to reflect the freed blocks: sector size
> - *    worst case split in allocation btrees per extent assuming 4 extents:
> - *		4 exts * 2 trees * (2 * max depth - 1) * block size
> + *    four extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_attrinval_reservation(
> @@ -709,9 +681,8 @@ xfs_calc_attrinval_reservation(
>  	return max((xfs_calc_inode_res(mp, 1) +
>  		    xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK),
>  				     XFS_FSB_TO_B(mp, 1))),
> -		   (xfs_calc_buf_res(9, mp->m_sb.sb_sectsize) +
> -		    xfs_calc_buf_res(xfs_allocfree_log_count(mp, 4),
> -				     XFS_FSB_TO_B(mp, 1))));
> +		   (xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +		    xfs_allocfree_extent_res(mp) * 4));
>  }
>  
>  /*
> @@ -760,10 +731,8 @@ xfs_calc_attrsetrt_reservation(
>   *    the attribute btree could join: max depth * block size
>   *    the inode bmap btree could join or split: max depth * block size
>   * And the bmap_finish transaction can free the attr blocks freed giving:
> - *    the agf for the ag in which the blocks live: 2 * sector size
> - *    the agfl for the ag in which the blocks live: 2 * sector size
>   *    the superblock for the free block count: sector size
> - *    the allocation btrees: 2 exts * 2 trees * (2 * max depth - 1) * block size
> + *    two extent allocfree reservations for the AG.
>   */
>  STATIC uint
>  xfs_calc_attrrm_reservation(
> @@ -776,9 +745,8 @@ xfs_calc_attrrm_reservation(
>  		     (uint)XFS_FSB_TO_B(mp,
>  					XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK)) +
>  		     xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK), 0)),
> -		    (xfs_calc_buf_res(5, mp->m_sb.sb_sectsize) +
> -		     xfs_calc_buf_res(xfs_allocfree_log_count(mp, 2),
> -				      XFS_FSB_TO_B(mp, 1))));
> +		    (xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +		     xfs_allocfree_extent_res(mp) * 2));
>  }
>  
>  /*
> -- 
> 2.31.1
>