xfs
[Top] [All Lists]

Re: [PATCH v2 02/12] xfs: make use of xfs_calc_buf_res() in xfs_trans.c

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH v2 02/12] xfs: make use of xfs_calc_buf_res() in xfs_trans.c
From: Mark Tinguely <tinguely@xxxxxxx>
Date: Sat, 19 Jan 2013 13:08:47 -0600
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <50EEC68F.6070309@xxxxxxxxxx>
References: <50EEC68F.6070309@xxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:9.0) Gecko/20120122 Thunderbird/9.0
On 01/10/13 07:47, Jeff Liu wrote:
Start to make use of the new helper to figure out space log reservations for 
those
transactions which are pre-calculated at mount time in xfs_trans.c.

Signed-off-by: Jie Liu<jeff.liu@xxxxxxxxxx>
---
  fs/xfs/xfs_trans.c |  244 ++++++++++++++++++++++++----------------------------
  1 file changed, 113 insertions(+), 131 deletions(-)


Wow! Reading this patch makes me appreciate the work you did here and gets my eyes in shape for Dave's UBER user sync patch.

A question for you, or anyone. When these reservations are made, the comments talk about specify number of agf/agfl (usually 2 or 3) that will be dirty in the command.

There are other comments that seem to imply an agf/agfl is reserved for all AGs and then use the multiplier of 4. Is a specific number of AGs can be involved in the operation or does it want something like sb_agcount?

I think there a couple error (may be more):

  /*
@@ -148,18 +145,18 @@ xfs_calc_itruncate_reservation(
        struct xfs_mount        *mp)
  {
        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))),
-                   (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_ALLOCFREE_LOG_RES(mp, 1) +
-                    128 * (2 + XFS_IALLOC_BLOCKS(mp) + mp->m_in_maxlevels +
-                           XFS_ALLOCFREE_LOG_COUNT(mp, 1))));
+               MAX((xfs_calc_buf_res(1, mp->m_sb.sb_inodesize) +
+                    xfs_calc_buf_res(XFS_BM_MAXLEVELS(mp, XFS_DATA_FORK) + 1,
+                                     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)) +
+                   xfs_calc_buf_res(5, 0) +
+                   xfs_calc_buf_res(XFS_ALLOCFREE_LOG_COUNT(mp, 1),
+                                    XFS_FSB_TO_B(mp, 1)) +
+                   xfs_calc_buf_res(2 + XFS_IALLOC_BLOCKS(mp) +
+                                    mp->m_in_maxlevels,
+                                    XFS_FSB_TO_B(mp, 1))));
                                     ^^^^^^^^^^^^^^
                                I think this should be 0. In the
                                original code, I see the headers being
                                reserved but not the data.


>   /*
> @@ -298,18 +287,18 @@ xfs_calc_create_reservation(
>    struct xfs_mount        *mp)
>   {
>    return XFS_DQUOT_LOGRES(mp) +
> -          MAX((mp->m_sb.sb_inodesize +
> -               mp->m_sb.sb_inodesize +
> -               mp->m_sb.sb_sectsize +
> -               XFS_FSB_TO_B(mp, 1) +
> -               XFS_DIROP_LOG_RES(mp) +
> -               128 * (3 + XFS_DIROP_LOG_COUNT(mp))),
> -              (3 * mp->m_sb.sb_sectsize +
> -               XFS_FSB_TO_B(mp, XFS_IALLOC_BLOCKS(mp)) +
> -               XFS_FSB_TO_B(mp, mp->m_in_maxlevels) +
> -               XFS_ALLOCFREE_LOG_RES(mp, 1) +
> -               128 * (2 + XFS_IALLOC_BLOCKS(mp) + mp->m_in_maxlevels +
> -                      XFS_ALLOCFREE_LOG_COUNT(mp, 1))));
> +          MAX((xfs_calc_buf_res(2, mp->m_sb.sb_inodesize) +
> +               xfs_calc_buf_res(1, mp->m_sb.sb_sectsize) +
> +               xfs_calc_buf_res(0, XFS_FSB_TO_B(mp, 1)) +
                                      ^^^^
                                    0 * (128+XFS_FSB_TO_B(mp, 1))?
                        from the header counts, it appears you meant
                        no headers, so it would be just:
                                        XFS_FSB_TO_B(mp, 1) +

> +               xfs_calc_buf_res(XFS_DIROP_LOG_COUNT(mp),
> +                                XFS_FSB_TO_B(mp, 1))),
> +              (xfs_calc_buf_res(3, mp->m_sb.sb_sectsize) +
> +               xfs_calc_buf_res(XFS_IALLOC_BLOCKS(mp),
> +                                XFS_FSB_TO_B(mp, 1)) +
> +               xfs_calc_buf_res(mp->m_in_maxlevels,
> +                                XFS_FSB_TO_B(mp, 1)) +
> +               xfs_calc_buf_res(XFS_ALLOCFREE_LOG_COUNT(mp, 1),
> +                                XFS_FSB_TO_B(mp, 1))));
>   }


>   /*
> @@ -337,16 +326,15 @@ xfs_calc_ifree_reservation(
>    struct xfs_mount        *mp)
>   {
>    return XFS_DQUOT_LOGRES(mp) +
> -          mp->m_sb.sb_inodesize +
> -          mp->m_sb.sb_sectsize +
> -          mp->m_sb.sb_sectsize +
> -          XFS_FSB_TO_B(mp, 1) +
> -          MAX((__uint16_t)XFS_FSB_TO_B(mp, 1),
> -              XFS_INODE_CLUSTER_SIZE(mp)) +

                        ^^^ end of MAX() 5th header is
                        is the single item in MAX

> -          128 * 5 +
> -          XFS_ALLOCFREE_LOG_RES(mp, 1) +
> -          128 * (2 + XFS_IALLOC_BLOCKS(mp) + mp->m_in_maxlevels +
> -                 XFS_ALLOCFREE_LOG_COUNT(mp, 1));
> +          xfs_calc_buf_res(1, mp->m_sb.sb_inodesize) +
> +          xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
> +          xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
> +          MAX(xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)),
> +              XFS_INODE_CLUSTER_SIZE(mp) +
                   ^^^^^ MAX should end here ^^

> +              xfs_calc_buf_res(2 + XFS_IALLOC_BLOCKS(mp) +
> +                               mp->m_in_maxlevels, 0) +
> +              xfs_calc_buf_res(XFS_ALLOCFREE_LOG_COUNT(mp, 1),
> +                               XFS_FSB_TO_B(mp, 1)));
>   }
>

  /*
@@ -337,16 +326,15 @@ xfs_calc_ifree_reservation(
        struct xfs_mount        *mp)
  {
        return XFS_DQUOT_LOGRES(mp) +
-               mp->m_sb.sb_inodesize +
-               mp->m_sb.sb_sectsize +
-               mp->m_sb.sb_sectsize +
-               XFS_FSB_TO_B(mp, 1) +
-               MAX((__uint16_t)XFS_FSB_TO_B(mp, 1),
                        ^^^^

-                   XFS_INODE_CLUSTER_SIZE(mp)) +
-               128 * 5 +
-               XFS_ALLOCFREE_LOG_RES(mp, 1) +
-               128 * (2 + XFS_IALLOC_BLOCKS(mp) + mp->m_in_maxlevels +
-                      XFS_ALLOCFREE_LOG_COUNT(mp, 1));
+               xfs_calc_buf_res(1, mp->m_sb.sb_inodesize) +
+               xfs_calc_buf_res(2, mp->m_sb.sb_sectsize) +
+               xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)) +
+               MAX(xfs_calc_buf_res(1, XFS_FSB_TO_B(mp, 1)),
                        ^^^ has extra header added it.
+                   XFS_INODE_CLUSTER_SIZE(mp) +
+                   xfs_calc_buf_res(2 + XFS_IALLOC_BLOCKS(mp) +
+                                    mp->m_in_maxlevels, 0) +
+                   xfs_calc_buf_res(XFS_ALLOCFREE_LOG_COUNT(mp, 1),
+                                    XFS_FSB_TO_B(mp, 1)));
  }

  /*

I will have to go through this patch again and also test prints before and after the patch.

Before the patch:
write 108216 itrnc 219064 renam 305976 link 153144 remov 153144 symlk 158520 creat 157880 mkdir 157880 ifree 57912 ichng 1592 grwdt 44160 swrit 384 wrtid 384 addfk 69560 atriv 174720 attst 22456 attrr 87992 clagi 640 gwall 65024 grezr 4224 gwrfr 5760


After the patch:
write 108216 itrnc 255928 renam 305976 link 153144 remov 153144 symlk 158520 creat 153784 mkdir 153784 ifree 57784 ichng 1592 grwdt 44160 swrit 384 wrtid 384 addfk 69560 atriv 174720 attst 22456 attrr 87992 clagi 640 gwall 65024 grezr 4224 gwrfr 5760


I plan to test on both sides of the routine's MAX() command too.

                ----

BTW, besides reusing the same superblock reservation and the commit widths, the rest of the series looks good to me.

--Mark.


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