xfs
[Top] [All Lists]

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

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH 1/4] xfs: Add a new function xfs_buf_log_overhead() to replace the hard-code number of 128
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 13 Dec 2012 15:24:08 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <50C1DD53.5070304@xxxxxxxxxx>
References: <50C1DD53.5070304@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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

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