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
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
