Re: [PATCH 2/2] xfs: inode log reservations are still too small

[Brian Foster <bfoster@redhat.com>]

On Wed, Mar 05, 2014 at 12:11:33PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Back in commit 23956703 ("xfs: inode log reservations are too
> small"), the reservation size was increased to take into account the
> difference in size between the in-memory BMBT block headers and the
> on-disk BMDR headers. This solved a transaction overrun when logging
> the inode size.
> 
> Recently, however, we've seen a number of these same overruns on
> kernels with the above fix in it. All of them have been by 4 bytes,
> so we must still not be accounting for something correctly.
> 
> Through inspection it turns out the above commit didn't take into
> account everything it should have. That is, it only accounts for a
> single log op_hdr structure, when it can actually require up to four
> op_hdrs - one for each region (log iovec) that is formatted. These
> regions are the inode log format header, the inode core, and the two
> forks that can be held in the literal area of the inode.
> 
> This means we are not accounting for 36 bytes of log space that the
> transaction can use, and hence when we get inodes in certain formats
> with particular fragmentation patterns we can overrun the
> transaction. Fix this by adding the correct accounting for log
> op_headers in the transaction.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---

You can add:

Tested-by: Brian Foster <bfoster@redhat.com>

... to Eric's review as well. I'm still throwing a workload at it, but
this has survived a few hours of a reproducer that consistently caused
the overrun in 5-10 seconds. Thanks for catching this.

Brian

>  fs/xfs/xfs_trans_resv.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/xfs/xfs_trans_resv.c b/fs/xfs/xfs_trans_resv.c
> index 8515b04..d2c8e4a 100644
> --- a/fs/xfs/xfs_trans_resv.c
> +++ b/fs/xfs/xfs_trans_resv.c
> @@ -81,20 +81,28 @@ xfs_calc_buf_res(
>   * on disk. Hence we need an inode reservation function that calculates all this
>   * correctly. So, we log:
>   *
> - * - log op headers for object
> + * - 4 log op headers for object
> + *	- for the ilf, the inode core and 2 forks
>   * - inode log format object
> - * - the entire inode contents (core + 2 forks)
> - * - two bmap btree block headers
> + * - the inode core
> + * - two inode forks containing bmap btree root blocks.
> + *	- the btree data contained by both forks will fit into the inode size,
> + *	  hence when combined with the inode core above, we have a total of the
> + *	  actual inode size.
> + *	- the BMBT headers need to be accounted separately, as they are
> + *	  additional to the records and pointers that fit inside the inode
> + *	  forks.
>   */
>  STATIC uint
>  xfs_calc_inode_res(
>  	struct xfs_mount	*mp,
>  	uint			ninodes)
>  {
> -	return ninodes * (sizeof(struct xlog_op_header) +
> -			  sizeof(struct xfs_inode_log_format) +
> -			  mp->m_sb.sb_inodesize +
> -			  2 * XFS_BMBT_BLOCK_LEN(mp));
> +	return ninodes *
> +		(4 * sizeof(struct xlog_op_header) +
> +		 sizeof(struct xfs_inode_log_format) +
> +		 mp->m_sb.sb_inodesize +
> +		 2 * XFS_BMBT_BLOCK_LEN(mp));
>  }
>  
>  /*
> -- 
> 1.9.0
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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