xfs
[Top] [All Lists]

Re: [PATCH 1/4] xfs: Add a new function xfs_buf_log_overhead() to replac

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: Add a new function xfs_buf_log_overhead() to replace the hard-code number of 128
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Thu, 13 Dec 2012 18:04:20 +0800
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20121213042408.GH16353@dastard>
References: <50C1DD53.5070304@xxxxxxxxxx> <20121213042408.GH16353@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121028 Thunderbird/16.0.2
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.
> 

<Prev in Thread] Current Thread [Next in Thread>