[PATCH 1/4] xfs: Add a new function xfs_buf_log_overhead() to replace the hard-code number of 128

[Dave Chinner]

On Fri, Dec 07, 2012 at 08:13:07PM +0800, Jeff Liu wrote:
> Introduce a helper to calculate the size of log buffer overhead for log format
> structures.  With that, we can kill the old magic number "128" so that we don't
> change the historical reservation that has been used for this overhead.
> 
> Signed-off-by: Jie Liu <jeff.liu at oracle.com>
> Cc: Dave Chinner <dchinner at redhat.com>
.....
> @@ -119,16 +137,19 @@ xfs_calc_itruncate_reservation(
>  	return XFS_DQUOT_LOGRES(mp) +
>  		MAX((mp->m_sb.sb_inodesize +
>  		     XFS_FSB_TO_B(mp, XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) + 1) +
> -		     128 * (2 + XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK))),
> +		     xfs_buf_log_overhead() *
> +		     (2 + XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK))),
>  		    (4 * mp->m_sb.sb_sectsize +
>  		     4 * mp->m_sb.sb_sectsize +
>  		     mp->m_sb.sb_sectsize +
>  		     XFS_ALLOCFREE_LOG_RES(mp, 4) +
> -		     128 * (9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
> -		     128 * 5 +
> +		     xfs_buf_log_overhead() *
> +		     (9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
> +		     xfs_buf_log_overhead() * 5 +

It would be nice to maintain some indent here to indicate single
expressions. i.e: This is easy to see these as single expressions:

-		128 * (9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
-		128 * 5 +

But this is much harder to see these:

+		xfs_buf_log_overhead() *
+		(9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
+		xfs_buf_log_overhead() * 5 +

I think it would be nicer to make long expressions like this have
the second and subseqeunt lines indented further like:

+		xfs_buf_log_overhead() *
+			(9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
+		xfs_buf_log_overhead() * 5 +


Hmm, looking at this, should we just pass the value into
xfs_buf_log_overhead() and do the multiplication there? i.e

		xfs_buf_log_overhead(9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4)) +
		xfs_buf_log_overhead(5) +

and so we end up with:

xfs_buf_log_overhead(
	int	nbufs)
{
	return nbufs * (sizeof.....)
}


This would simplify the complex macros, and make it obvious what is
actually being calculated.

I see this repeated throughout the patch....

>  xfs_calc_writeid_reservation(xfs_mount_t *mp)
>  {
> -	return mp->m_sb.sb_inodesize + 128;
> +	return mp->m_sb.sb_inodesize + xfs_buf_log_overhead();
>  }

And when I see this sort fo thing repeated, something like

xfs_calc_buf_res(
	int	nbufs,
	int	size)
{
	return nbufs * (size + (sizeof.....));
}

and the above reservation becomes:

 xfs_calc_writeid_reservation(xfs_mount_t *mp)
 {
-	return mp->m_sb.sb_inodesize + 128;
+	return xfs_calc_buf_res(1, mp->m_sb.sb_inodesize);
 }

because most of the reservations have magic numbers in the buf log
overhead count to include individual buffers that are just accounted
by size earlier in the caldulation.

For example:

> @@ -463,12 +493,14 @@ xfs_calc_attrinval_reservation(
>  {
>  	return MAX((mp->m_sb.sb_inodesize +
>  		    XFS_FSB_TO_B(mp, XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK)) +
> -		    128 * (1 + XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK))),
> +		    xfs_buf_log_overhead() *
> +		    (1 + XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK))),
>  		   (4 * mp->m_sb.sb_sectsize +
>  		    4 * mp->m_sb.sb_sectsize +
>  		    mp->m_sb.sb_sectsize +
>  		    XFS_ALLOCFREE_LOG_RES(mp, 4) +
> -		    128 * (9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4))));
> +		    xfs_buf_log_overhead() *
> +		    (9 + XFS_ALLOCFREE_LOG_COUNT(mp, 4))));
>  }

This translates as:

	MAX(	(inode being logged in buffer +
		size in bytes of BMAP btree modification +
		overhead of (1 inode buffer + BMAP btree buffers)),

		(4 sectors in buffers +
		 4 sectors in buffers +
		 another sector +
		 4 AG freespace tree modifications in bytes +
		 overhead of (9 buffers + AG freespace buffer count)
	)

So effectively, this could be written as:

	return MAX((xfs_calc_buf_res(1, mp->m_sb.sb_inodesize) +
		    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))));

That, to me, us a much cleaner result, and one that encapsulates the
overhead of logging the buffer with the size of the buffer being
logged. This would also remove one layer of macros - the above gets
rid of the need for the XFS_ALLOCFREE_LOG_RES() macro (and others)
in these calculations...

> +++ b/fs/xfs/xfs_trans.h
> @@ -259,7 +259,8 @@ struct xfs_log_item_desc {
>  	((mp)->m_reservations.tr_attrset + \
>  	 (ext * (mp)->m_sb.sb_sectsize) + \
>  	 (ext * XFS_FSB_TO_B((mp), XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK))) + \
> -	 (128 * (ext + (ext * XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK)))))
> +	 (xfs_buf_log_overhead() * \
> +	  (ext + (ext * XFS_BM_MAXLEVELS(mp, XFS_ATTR_FORK)))))

That's the XFS_ATTRSET_LOG_RES() macro. That should be added to the
mp->m_reservations structure and calculated at mount time like
everything else.

>  #define	XFS_ATTRRM_LOG_RES(mp)	((mp)->m_reservations.tr_attrrm)
>  #define	XFS_CLEAR_AGI_BUCKET_LOG_RES(mp)  ((mp)->m_reservations.tr_clearagi)
>  
> @@ -535,5 +536,6 @@ extern kmem_zone_t	*xfs_log_item_desc_zone;
>  
>  void		xfs_trans_init(struct xfs_mount *);
>  int		xfs_trans_roll(struct xfs_trans **, struct xfs_inode *);
> +uint		xfs_buf_log_overhead(void);

And then this doesn't need to be externally visible.

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com