Re: [PATCH] (and bad attr2 bug) - pack xfs_sb_t for 64-bit arches

Timothy Shimmin
2006-11-24T04:46:26Z

--On 23 November 2006 11:37:46 AM -0600 Russell Cattelan <cattelan@thebarn.com> wrote:

> Timothy Shimmin wrote:
>
>> Hi Guys,
>>
>> So just looking at the first part, which as Eric suggested can be
>> considered
>> on its own.
>>
>> Index: work_gfs/fs/xfs/xfs_attr.c
>> ===================================================================
>> --- work_gfs.orig/fs/xfs/xfs_attr.c 2006-11-21 18:38:27.572949303
>> -0600
>> +++ work_gfs/fs/xfs/xfs_attr.c 2006-11-21 18:44:51.666033422 -0600
>> @@ -210,8 +210,20 @@ xfs_attr_set_int(xfs_inode_t *dp, const
>> * (inode must not be locked when we call this routine)
>> */
>> if (XFS_IFORK_Q(dp) == 0) {
>> - if ((error = xfs_bmap_add_attrfork(dp, size, rsvd)))
>> - return(error);
>> + if ((dp->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) ||
>> + ((dp->i_d.di_aformat == XFS_DINODE_FMT_EXTENTS) &&
>> + (dp->i_d.di_anextents == 0))) {
>> + /* xfs_bmap_add_attrfork will set the forkoffset based on
>> + * the size needed, the local attr case needs the size
>> + * attr plus the size of the hdr, if the size of
>> + * header is not accounted for initially the forkoffset
>> + * won't allow enough space, the actually attr add will
>> + * then be forced out out line to extents
>> + */
>> + size += sizeof(xfs_attr_sf_hdr_t);
>> + if ((error = xfs_bmap_add_attrfork(dp, size, rsvd)))
>> + return(error);
>> + }
>> }
>>
>> --On 22 November 2006 10:38:16 AM -0600 Eric Sandeen
>> <sandeen@sandeen.net> wrote:
>>
>>>> By fixing the initial size calculation at least things like SElinux
>>>> which is adding one attr won't cause the attr segment to flip to
>>>> extents
>>>> immediately.
>>>> The second attr will cause the flip but not the first one.
>>>
>>>
>>> I'd say this part (fixing up proper space for the initial attr fork
>>> setup) should probably go in
>>> soon if it gets good reviews (with the removal of the extra tests, as
>>> we discussed on irc last
>>> night). I think this proper change stands on its own just fine.
>>>
>>
>> So yeah, as you said in IRC, the brace is in the wrong spot and
>> the di_aformat tests don't make any sense here.
>> Basically, we know that fork offset is zero and therefore that the
>> di_aformat should be
>> set at XFS_DINODE_FMT_EXTENTS and di_anetents will be zero.
>> As this is the state before we add in an attribute fork.
>> Why we have this initial state as extents, I'm not too sure and
>> wondered in the
>> past. Maybe because this state is one which doesn't occupy any space
>> in the literal area.
>> A shortform EA has a header at least.
>>
>> My next concern is that the size that is calculated is presumably
>> trying to accomodate
>> the shortform EA. However, the calculation is for the sf header and
>> the space for a
>> a xfs_attr_leaf_name_local with given namelen and valuelen.
>> It would be better to base it on an xfs_attr_sf_entry type.
>> So I think we need to rework this calculation.
>>
>> Which leads me on to the next issue.
>> We don't know what EA form we are going to take,
>> so we can't really assume that it will be shortform.
>> If the EA name or value is big then the EA will go into extents and
>> could occupy very
>> little room in the inode.
>> With the current & proposed test this could make the bytesfit function
>> return 0
>> (the offset calculated in bytesfit could also go negative) and
>> then we would set the forkoff back at the old attr1 default.
>> So we might have 1 EA extent in the inode taking little space and yet
>> setting the forkoff
>> in the middle.
>
> Yes I agree worst cast scenario is that the inode has reverted to an attr1 split and
> that space is being wasted in the attr portion. By the time an inode has flipped to
> btree mode for di_u how much of a performance hit is really going to be noticed?
> mapping the blocks for that inode is going to take multiple reads.
> Attr2 seems most effective at space optimization for the local and extent
> versions of di_u and probably not so much for btree.
>
> At least by fixing the size calculation the shortforms that do fit into di_a will now
> be added inline. What is happening now the btree is being re factored
> which is probably expensive and the attr is being added as extents since the
> original size used for the btree refactoring wasn't enough.
>
> So the change to add in the header size will at least make single case
> attrs more efficient since they will now be inline.

No argument there, I just want to make sure the SF size calculation is correct.
I think we need a new macro here.
Currently we just accumulate the size using the totsize field in the header
and so on an EA add, just add in the space for 1 entry: i.e.
newsize = XFS_ATTR_SF_TOTSIZE(args->dp);
newsize += XFS_ATTR_SF_ENTSIZE_BYNAME(args->namelen, args->valuelen);
We need a macro, at least, for the total size for 1 given EA.
xfs_attr_leaf_newentsize is not necessarily the space for a shortform (SF) EA -
the structs do have different field sizes.

> If the attr does not fit inline then worst case the forkoff flips to attr1
> default or a half and half split.
>
> Given the cost of refactoring a btree it might be better to have attr1 behavior?
> Since di_a will have extra space additional attr adds won't cause forkoff to
> move and thus won't cause a rebalance of di_u.
>
If the SF EA can't fit in the literal space, and goes to extents then adding
further EAs probably isn't going to move the forkoff at all either, since they'll
probably just go into the EA leaf block.

> So in thinking about this more does it make sense to actually not
> try to optimize the space needed for di_u when it is a btree?
> Maybe the first time an attr is added simply split the inode
> space doing the rebalance once?
> That would allow for more attrs to be added without rebalancing
> the data btree.
> The other scheme of space optimzation if the root btree node
> is sparse would say sure give more space to di_a but at
> the expense of a reblanace.
>
> So ya it's a bit of a guessing game.
>
Yeah I'm inclined not to worry about compressing the btree data root but
just working out where it really finishes in the literal area and not
pushing it.

(All this would be a lot easier to discuss in person with a whiteboard,
as I think Eric was kind of suggesting in irc :)

Back to the 1st part.
I'm thinking that if I can't fit the SF EA in the available space,
then we might just assume that we should leave EA space for the
MAX(2 extents, min-root-size) i.e. try for 2 extents (bytesfit will make
sure we atleast have the min-root-size). The forkoff is just a good
guess after all - it's a tradeoff.

Okay, need to do some coding...

--Tim