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@xxxxxxxxxx>
> Cc: Dave Chinner <dchinner@xxxxxxxxxx>
.....
> @@ -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@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
