-------- Original Message --------
Subject: RE: attr2
Date: Fri, 15 Dec 2006 16:33:04 +1100
From: Barry Naujok <bnaujok@xxxxxxxxxxxxxxxxx>
To: 'Russell Cattelan' <cattelan@xxxxxxxxxxx>
CC: 'Timothy Shimmin' <timothy.shimmin@xxxxxxxxx>,
<chatz@xxxxxxxxxxxxxxxxx>, 'Timothy Shimmin' <tes@xxxxxxx>,
<sandeen@xxxxxxxxxxx>, 'David Chinner' <dgc@xxxxxxx>
I think I have it cracked now :)
I've cleaned up a bit more as per Russell's comments.
I also found another case of btree corruption when the last attribute is
deleted (an attr2 optimization is to completely remove the attr fork
when the last attribute is deleted).
Also, the root btree is not modified anymore when an attribute goes just
beyond the space the rest of the code thinks it needs
(XFS_BMAP_ROOT_SPACE macro).
Barry.
> -----Original Message-----
> From: Russell Cattelan [mailto:cattelan@xxxxxxxxxxx]
> Sent: Friday, 15 December 2006 2:31 PM
> To: Barry Naujok
> Cc: 'Timothy Shimmin'; chatz@xxxxxxxxxxxxxxxxx; 'Timothy
> Shimmin'; sandeen@xxxxxxxxxxx; 'David Chinner'
> Subject: Re: attr2
>
> Barry Naujok wrote:
>
>
> sorry just getting time to look at this again
>
>
>
> So looking over the latest patch I'm trying to understand
> what is different from the original patch I sent out.
>
> I guess there is the addition of the calculation of number
> of btree recs in the case of a the inode being converted
> to an attr capable format.
> I know we had talked about optimizing that space but it
> leads into the guessing game we were talking about.
> Will giving more space to attr be better or will leaving more
> space for btree block be better.
>
> I would lean on the side of attrs are infrequent updates/additions
> but data blocks are not. So opting for an attr1 half and half
> split once
> the inode goes btree for either fork seems simpler to me.
> And keeps the code out of the guessing game of which fork
> is better optimized by having more space.
>
>
> My style point comment:
> The current patch is a bit busy and has more calls to MAX,
> the maxforkoff is a bit confusing since it contains bytes during
> the calculation then converted back to the >> 3 value.
> My orginal patch has the dsize variable to help clarify bytes vs
> forkoff.
> Hmm looking closer I also think there is replicated checks for
> does the attr fit, the case statement doesn't need to deal
> with "does the attr fit" since that is handled later.
> Nothing really wrong with it but I just like the less line
> of code approach.
>
> >
> >
> >
> >
> >>-----Original Message-----
> >>From: Timothy Shimmin [mailto:timothy.shimmin@xxxxxxxxx]
> >>Sent: Friday, 15 December 2006 1:08 PM
> >>To: Barry Naujok
> >>Cc: Russell Cattelan; chatz@xxxxxxxxxxxxxxxxx; Timothy
> >>Shimmin; sandeen@xxxxxxxxxxx; David Chinner
> >>Subject: Re: attr2
> >>
> >>Hi Barry,
> >>
> >>I don't have the tree locally (can't do an xdiff) but
> >>if we have data btree and existing forkoff then
> >>you just set the minforkoff to the existing forkoff which
> >>won't guarantee that the forkoff is not moved - previously
> >>I was returning the forkoff in that case.
> >>
> >>
> >
> >Yes, good point. I was only thinking of the case of attrs
> trying to get
> >enlarged. I don't believe forkoff can be increased with the existing
> >code, but with the attached change, it's now guaranteed.
> >
> >
> >
> >>Also comments could be fixed up a bit.
> >>
> >>The XFS_DINODE_FMT_EXTENTS comment has problems:
> >>"in in the extents form will migrate to btree"
> >>maybe you mean:
> >>"in the data extents form migrating to btree"
> >>
> >>The XFS_DINODE_FMT_BTREE comment talks about setting the
> >>forkoff but it is actually setting minforkoff.
> >>
> >>
> >
> >Hehe, that comment is unchanged from the previous patch, but
> I've fixed
> >it.
> >
> >There still seems to be an issue with a non-attribute
> data-btree case,
> >where adding a big-enough attribute will still cause the
> btree to grow
> >another level (not corrupted though), so it seems minforkoff in that
> >case is not quite right.
> >
> >
> >
> >>--Tim
> >>
> >>On 12/15/06, Barry Naujok <bnaujok@xxxxxxxxxxxxxxxxx> wrote:
> >>
> >>
> >>>
> >>>
> >>>>-----Original Message-----
> >>>>From: Timothy Shimmin [mailto:timothy.shimmin@xxxxxxxxx]
> >>>>Sent: Thursday, 14 December 2006 11:57 PM
> >>>>To: Barry Naujok
> >>>>Cc: Russell Cattelan; chatz@xxxxxxxxxxxxxxxxx; Timothy
> >>>>Shimmin; sandeen@xxxxxxxxxxx; David Chinner
> >>>>Subject: Re: attr2
> >>>>
> >>>>Hi,
> >>>>
> >>>>On 12/14/06, Barry Naujok <bnaujok@xxxxxxxxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>>Made one interesting observation with the attr2 with a
> >>>>>
> >>>>>
> >>inode full of
> >>
> >>
> >>>>>extents that must be migrated to btree format, if the
> >>>>>
> >>>>>
> >>xattr is big
> >>
> >>
> >>>>>enough to go beyond the default forkoff (ie. < 15 in 256
> >>>>>
> >>>>>
> >>>>byte inode) but
> >>>>
> >>>>
> >>>>>still stay inline, it won't, but go to extents.
> >>>>>
> >>>>>Eg:
> >>>>>
> >>>>> makeextents -b4096 -n 9 <foo>
> >>>>> setfattr -n user.<32 chars> <foo>
> >>>>>
> >>>>>forkoff stays at 15, the data extents are btree as
> >>>>>
> >>>>>
> >>expected, and the
> >>
> >>
> >>>>>xattr is extents rather than local (shortform).
> >>>>>
> >>>>>I think a test is needed when creating the initial attr fork:
> >>>>>
> >>>>>xfs_bmap_add_attrfork() calls xfs_attr_shortform_bytesfit()
> >>>>>
> >>>>>
> >>>>to determine
> >>>>
> >>>>
> >>>>>the desired forkoff. As the inode is full at this stage
> >>>>>
> >>>>>
> >>>>without an attr
> >>>>
> >>>>
> >>>>>fork, it will definitely go btree.
> >>>>>
> >>>>> minforkoff = MAX(extents used, XFS_BMDR_SPACE_CALC());
> >>>>>
> >>>>>I'm thinking if the inode doesn't have attrs at this stage,
> >>>>>
> >>>>>
> >>>>it should
> >>>>
> >>>>
> >>>>>base the minimum on XFS_BMDR_SPACE_CALC, rather than the
> >>>>>
> >>>>>
> >>>>space currently
> >>>>
> >>>>
> >>>>>used by extents.
> >>>>>
> >>>>>eg:
> >>>>>
> >>>>> minforkoff = XFS_BMDR_SPACE_CALC(MINDBTPTRS);
> >>>>> if (dp->i_d.di_forkoff && xfs_ifork_dsize_used(dp)
> >>>>>minforkoff)
> >>>>> minforkoff = xfs_ifork_dsize_used(dp);
> >>>>>
> >>>>>Thoughts?
> >>>>>
> >>>>>
> >>>>>
> >>>>Did you mean this just if the data extents are in btree format?
> >>>>You don't seem to be codifying the notion of
> >>>>"full of extents that must be migrated to btree format".
> >>>>No comparison of requested attr offset with the data space used
> >>>>which would tell if we were going to have to force data
> >>>>extents to btree.
> >>>>
> >>>>I guess there is no point in both going out of line if they
> >>>>don't have to.
> >>>>However, should we favour one over the other (EA over data).
> >>>>I thought we were doing 1st come 1st served.
> >>>>If data extents fit inline currently and then they still
> >>>>
> >>>>
> >>fit inline
> >>
> >>
> >>>>after EA added but
> >>>>with EA out of line, then data wins.
> >>>>But if data extents have to go out of line and we can
> >>>>have attr inline then keep attr inline.
> >>>>
> >>>>It's late so I'm probably not thinking clearly ;-)
> >>>>
> >>>>--Tim
> >>>>
> >>>>
> >>>Ok, I have implemented and tested the attached patch. I've
> >>>
> >>>
> >>merged the
> >>
> >>
> >>>function back in, distilling the core requirements.
> >>>
> >>>So, the main change from the previous patch is with data
> >>>
> >>>
> >>extents format
> >>
> >>
> >>>and no attr, it checks to see if adding the default attr
> >>>
> >>>
> >>size (forkoff =
> >>
> >>
> >>>15 for 256 byte inodes) will cause the data fork to be converted to
> >>>btree format. If so, it will then set the minimum offset
> to the base
> >>>btree root size.
> >>>
> >>>So, with 7 extents, forkoff is allowed to go down to 14.
> After that,
> >>>forkoff will be back at 15 and the attrs go out-of-line.
> >>>
> >>>With 8 extents, adding any attr at all will cause the data
> >>>
> >>>
> >>extents to go
> >>
> >>
> >>>btree. This allows the attr to use up all the inode space
> >>>
> >>>
> >>it can up to
> >>
> >>
> >>>the btree root.
> >>>
> >>>
> >>>
> >>>
> >>>
>
--
David Chatterton
XFS Engineering Manager
SGI Australia
attr2_new.diff
Description: Binary data
|