Hi Dave,
On 12/13/2012 12:24 PM, Dave Chinner wrote:
> 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 +
Yes, the patch of mine even make the code style looks a bit ugly than the original.
>
> 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.
Definitely, this looks so much better than mine.
>
> 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.
Coool.
>
> 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...
Pretty cool, will fix it according to your comments.
>
>> +++ 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.
ok, this change will reflected in next round of submit.
>
>> #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.
Exactly, thank you so much for your detailed review comments!
Cheers,
-Jeff
>
> Cheers,
>
> Dave.
>
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
