Re: [PATCH v3 04/11] xfs: update inode allocation/free transaction reservations for finobt

[Dave Chinner <david@fromorbit.com>]

On Tue, Feb 04, 2014 at 12:49:35PM -0500, Brian Foster wrote:
> Create the xfs_calc_finobt_res() helper to calculate the finobt log
> reservation for inode allocation and free. Update
> XFS_IALLOC_SPACE_RES() to reserve blocks for the additional finobt
> insertion on inode allocation. Create XFS_IFREE_SPACE_RES() to
> reserve blocks for the potential finobt record insertion on inode
> free (i.e., if an inode chunk was previously fully allocated).
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_inode.c       |  4 +++-
>  fs/xfs/xfs_trans_resv.c  | 47 +++++++++++++++++++++++++++++++++++++++++++----
>  fs/xfs/xfs_trans_space.h |  7 ++++++-
>  3 files changed, 52 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 001aa89..57c77ed 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1730,7 +1730,9 @@ xfs_inactive_ifree(
>  	int			error;
>  
>  	tp = xfs_trans_alloc(mp, XFS_TRANS_INACTIVE);
> -	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree, 0, 0);
> +	tp->t_flags |= XFS_TRANS_RESERVE;
> +	error = xfs_trans_reserve(tp, &M_RES(mp)->tr_ifree,
> +				  XFS_IFREE_SPACE_RES(mp), 0);

Can you add a comment explaining why the XFS_TRANS_RESERVE flag is
used here, and why it's use won't lead to accelerated reserve pool
depletion?

>  	if (error) {
>  		ASSERT(XFS_FORCED_SHUTDOWN(mp));
>  		xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES);
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index 2fd59c0..32f35c1 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -98,6 +98,37 @@ xfs_calc_inode_res(
>  }
>  
>  /*
> + * The free inode btree is a conditional feature and the log reservation
> + * requirements differ slightly from that of the traditional inode allocation
> + * btree. The finobt tracks records for inode chunks with at least one free inode.
> + * Therefore, a record can be removed from the tree for an inode allocation or
> + * free and the associated merge reservation is unconditional. This also covers
> + * the possibility of a split on record insertion.

Slightly wider than 80 columns here. FWIW, if you use vim, add this
rule to have it add a red line at the textwidth you have set:

" highlight textwidth
set cc=+1

And that will point out lines that are too long quite obviously ;)

> + *
> + * the free inode btree: max depth * block size
> + * the free inode btree entry: block size
> + *
> + * TODO: is the modify res really necessary? covered by the merge/split res?
> + * This seems to be the pattern of ifree, but not create_resv_alloc. Why?

The modify case is for an allocation that only updates an inobt
record (i.e. chunk already allocated, free inodes in it). Because
we can remove a finobt record when "modifying" the last free inode
record in a chunk, "modify" can cause a redcord removal and hence a
tree merge. In which case it's no different of any of the other
finobt reservations....

> @@ -267,6 +298,7 @@ xfs_calc_remove_reservation(
>   *    the superblock for the nlink flag: sector size
>   *    the directory btree: (max depth + v2) * dir block size
>   *    the directory inode's bmap btree: (max depth + v2) * block size
> + *    the finobt
>   */
>  STATIC uint
>  xfs_calc_create_resv_modify(
> @@ -275,7 +307,8 @@ xfs_calc_create_resv_modify(
>  	return xfs_calc_inode_res(mp, 2) +
>  		xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
>  		(uint)XFS_FSB_TO_B(mp, 1) +
> -		xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp), XFS_FSB_TO_B(mp, 1));
> +		xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp), XFS_FSB_TO_B(mp, 1)) +
> +		xfs_calc_finobt_res(mp, 1);
>  }

And this is where is starts to get complex. The modify operation can
now cause a finobt merge, when means blocks will be allocated/freed.
That means we now need to take into account:

 *    the allocation btrees: 2 trees * (max depth - 1) * block size

and anything else freeing an extent requires.

>  /*
> @@ -285,6 +318,7 @@ xfs_calc_create_resv_modify(
>   *    the inode blocks allocated: XFS_IALLOC_BLOCKS * blocksize
>   *    the inode btree: max depth * blocksize
>   *    the allocation btrees: 2 trees * (max depth - 1) * block size
> + *    the finobt
>   */
>  STATIC uint
>  xfs_calc_create_resv_alloc(
> @@ -295,7 +329,8 @@ xfs_calc_create_resv_alloc(
>  		xfs_calc_buf_res(XFS_IALLOC_BLOCKS(mp), XFS_FSB_TO_B(mp, 1)) +
>  		xfs_calc_buf_res(mp->m_in_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_calc_finobt_res(mp, 0);
>  }

This reservation is only for v4 superblocks - the icreate
transaction reservation is used for v5 superblocks, so that's the
only one you need to modify.

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs