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: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Sun, 20 Jan 2013 10:09:43 +1100
Cc: Jeff Liu <jeff.liu@xxxxxxxxxx>, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <50FAEF3F.7090607@xxxxxxx>
References: <50EEC68F.6070309@xxxxxxxxxx> <50FAEF3F.7090607@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Sat, Jan 19, 2013 at 01:08:47PM -0600, 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?

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....

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

At a quick glance, it's hard to verify if there are errors or not.
I was about to suggest that you do this:

> 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

But it looks like you've already thought of that :)

I'd also suggest that different geometries need to be checked, because
things like block size affect the result. I'd be looking at checking
these geometries:

        -b size=512 -n size=512
        -b size=512 -n size=4096
        -b size=512 -n size=65536
        -b size=4096 -n size=4096
        -b size=4096 -n size=65536
        -b size=4096 -n size=4096 -i size=2048


Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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