xfs
[Top] [All Lists]

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

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH v2 02/12] xfs: make use of xfs_calc_buf_res() in xfs_trans.c
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Tue, 22 Jan 2013 11:27:54 +0800
Cc: xfs@xxxxxxxxxxx, Dave Chinner <david@xxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <50FAEF3F.7090607@xxxxxxx>
References: <50EEC68F.6070309@xxxxxxxxxx> <50FAEF3F.7090607@xxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121028 Thunderbird/16.0.2
Hi Mark and Dave,

Thanks for your review and sorry for my late response!

On 01/20/2013 03:08 AM, Mark Tinguely wrote:
> 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?
With Dave's comments in another email for this question:

"""
Do you mean comments like this about
xfs_calc_itruncate_reservation()?

 * And the bmap_finish transaction can free the blocks and bmap
 * blocks:
 *    the agf for each of the ags: 4 * sector size
 *    the agfl for each of the ags: 4 * sector size

This assumes the transaction can free 4 extents before a commit, and
all 4 extents can be in a different AG.

You'll find all the other cases documented like this indicate how
many extents can be freed or allocated in a single transaction....
"""

Does it sounds make sense if we improve those comments(anything might
confuse the reader) with more detailed info like above?

> 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.
Yep, will fix it.
> 
> 
>  >   /*
>  > @@ -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) +
Ah, You're right.
> 
>  > +               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 ^^
Yes. :)
> 
>  > +              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 +
                ^^^^^^^^^
I have not idea why we have to log 5 headers here by looking at the
comments of this transaction, I think I must missed something...

>> -            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
> 
In previous tests, I have only ran xfstests to make sure nothing was
broke.   But obviously, I should test it according to yours and combine
with Dave's comments(i.e. enlarge the test coverage with different
sector size/block size).

Thanks,
-Jeff

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