(and bad attr2 bug) - pack xfs_sb_t for 64-bit arches

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

I turned on attr2 in FC6 at nathan's suggestion, for selinux goodness with more efficient xattr space usage.

But, many reports that this was totally broken in fc6, on x86_64.

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

The filesystem was also found to be marked w/ attr1, not attr2....

if you dd/hexdump the superblock, you will find the attr2 flag, but at the wrong offset.

This is because the xfs_sb_t struct is padded out to 64 bits on 64-bit arches, and the xfs_xlatesb() routine and xfs_sb_info[] array take this padding to mean that the last item is 4 bytes bigger than it is, and treats sb_features2 as 8 bytes not four. This then gets endian-flipped out...

I can't quite figure out how this winds up causing problems if you stay on the x86_64 arch, as I'd expect that if the offset is wrong, it should at least be consistently wrong. And in fact if you do mkfs,mount,xfs_info, it will tell you that you do have attr2. But somewhere along the line thing go wrong, and post-install, post-reboot, the filesystem thinks it is attr1, and is therefore corrupt.

I think that maybe some accesses are post-xfs_xlatesb, while others may access the un-flipped sb directly? Or maybe this is sb logging code that has messed things up? Not sure... needs more investigation.

The real bug here was whoever removed the dummy field and did not replace that with a comment ot say that the xfs_sb strucutre needs to be padded to 64 bits to ensure translation worked properly on 64 bit systems.

I'd prefer explicit padding (with warning comments) over packing the structure. Thoughts?

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

I turned on attr2 in FC6 at nathan's suggestion, for selinux goodness with more efficient xattr space usage.

But, many reports that this was totally broken in fc6, on x86_64.

Install went ok, but on reboot the filesystem was found to be corrupt.

The filesystem was also found to be marked w/ attr1, not attr2....

If you do a fresh mkfs.xfs on x86_64, with -i attr=2, and dump out the superblock (or look at it with xfs_db) you will find that although the versionnum says that there is a morebits bit, the features2 flag is 0.

if you dd/hexdump the superblock, you will find the attr2 flag, but at the wrong offset.

This is because the xfs_sb_t struct is padded out to 64 bits on 64-bit arches,

and the xfs_xlatesb() routine and xfs_sb_info[] array take this padding to mean that the last item is 4 bytes bigger than it is, and treats sb_features2 as 8 bytes not four. This then gets endian-flipped out...

Hi Eric,

Well there is a bug in the sb endian translation code then or its setup. All the field accesses should be correct, no?

Quoted 5 lines · Timothy Shimmin

Hi Eric,

Well there is a bug in the sb endian translation code then or its setup. All the field accesses should be correct, no?

The problem is the size of the variable translated in xfs_xlatesb() is the offset of the next field minus the offset of the current field.

With the last filed of the structure, it ends up being the size of the structure minus the offset of the field. On a 32 bit machine, the structure is 4 bytes larger that the offset of the feature2 field. On 64 bit machine, the strucutre is 8 bytes larger than the offset of the features2 field,

so it translates is as though it was a 64 bit field, not a 32 bit field.....

Quoted 3 lines

so it translates is as though it was a 64 bit field, not a 32 bit field.....

So why not change xfs_sb_info to give the real offset of where the next field should go (if there was one), instead of giving the sizeof the
structure which is not where say a 32 bit field would go and is wrong IMHO.

i.e.

===========================================================================
Index: fs/xfs/xfs_mount.c
===========================================================================

Whenever you add to the table, you now need to modify both the new entry and the terminator to get it right.

Nor (IMO) is it obvious that it is a terminator or why it is different to all the other entries in the structure. A field such as sb_dummy or sb_pad before the terminator is fairly obvious, and it means that you don't need to modify the table terminator every time the superblock gets extended.

That way the code stays more consistent over time, diffs are smaller and neater, and you can see at a simple diff just how the features have been added over time (like I did this morning).....

Quoted 11 lines · Timothy Shimmin

Whenever you add to the table, you now need to modify both the new entry and the terminator to get it right.

Nor (IMO) is it obvious that it is a terminator or why it is different to all the other entries in the structure. A field such as sb_dummy or sb_pad before the terminator is fairly obvious, and it means that you don't need to modify the table terminator every time the superblock gets extended.

Quoted 3 lines

That way the code stays more consistent over time, diffs are smaller and neater, and you can see at a simple diff just how the features have been added over time (like I did this morning).....

nothing in the code is terribly obvious.. please add comments however you decide to fix it :)

and really, now that this is out in the wild, maybe sb_features3 instead of padding is appropriate, and check both for the attr2 bit...? :(

i'm trying to figure out what the kernel upgrade path is for fc6 users who have an extra-padded-flipped features2/attr2 filesystem. :(

and really, now that this is out in the wild, maybe sb_features3 instead of padding is appropriate, and check both for the attr2 bit...? :(

Quoted 3 lines

and really, now that this is out in the wild, maybe sb_features3 instead of padding is appropriate, and check both for the attr2 bit...? :(

Thats not going to work, theres three or four other feature2 bits preceding attr2 as well.

The "take a 32 bit systems fs to a 64 bit system" is relatively uncommon, so I suppose its just something we live with (as we did with the log recovery issues in that situation for several years).

(quoted diff) +5 -2

@@ -135,9 +136,12 @@
       __uint8_t       sb_shared_vn;   /* shared version number */
       xfs_extlen_t    sb_inoalignmt;  /* inode chunk alignment, fsblocks */
       __uint32_t      sb_unit;        /* stripe or raid unit */
-       __uint32_t      sb_width;       /* stripe or raid width */
+       __uint32_t      sb_width;       /* stripe or raid width */
       __uint8_t       sb_dirblklog;   /* log2 of dir block size (fsbs) */
-        __uint8_t       sb_dummy[7];    /* padding */
+       __uint8_t       sb_logsectlog;  /* log2 of the log sector size */
+       __uint16_t      sb_logsectsize; /* sector size for the log, bytes */
+       __uint32_t      sb_logsunit;    /* stripe unit size for the log */
+       __uint32_t      sb_features2;   /* additional feature bits */
 } xfs_sb_t;
 

So before the sector size > 512 bytes code, there was padding to push the superblock out to 64bit alignement so that sb_dirblklog was correctly aligned. The xfs_sb_info structure:

    { offsetof(xfs_sb_t, sb_unit),      0 },
    { offsetof(xfs_sb_t, sb_width),     0 },
    { offsetof(xfs_sb_t, sb_dirblklog),         0 },

(quoted diff) +3 -1

-    { offsetof(xfs_sb_t, sb_dummy),     1 },
+    { offsetof(xfs_sb_t, sb_logsectlog), 0 },
+    { offsetof(xfs_sb_t, sb_logsectsize),0 },
    { offsetof(xfs_sb_t, sb_logsunit),  0 },
+    { offsetof(xfs_sb_t, sb_features2),         0 },
    { sizeof(xfs_sb_t),                 0 }

};

had the sb_dummy field as "no translate" so it effectively ignored it but it ensured that sb_dirblklog was sized correctly.

The real bug here was whoever removed the dummy field and did not replace that with a comment ot say that the xfs_sb strucutre needs to be padded to 64 bits to ensure translation worked properly on 64 bit systems.

I'd prefer explicit padding (with warning comments) over packing the structure. Thoughts?

Cheers,

Dave.

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Quoted 5 lines · On Fri, 2006-11-17 at 00:34 -0600, [email protected] wrote:

Quoted 3 lines

and really, now that this is out in the wild, maybe sb_features3 instead of padding is appropriate, and check both for the attr2 bit...? :(

Quoted 6 lines

Thats not going to work, theres three or four other feature2 bits preceding attr2 as well.

The "take a 32 bit systems fs to a 64 bit system" is relatively uncommon, so I suppose its just something we live with (as we did with the log recovery issues in that situation for several years).

So you think this should not be fixed, then? Because if it -is- fixed

then it's not an fs transfer problem; suddenly 64-bit attr2 filesystems will think they have attr1 if proper padding is added.

Quoted 7 lines · On Fri, 2006-11-17 at 09:20 -0600, [email protected] wrote:

Quoted 7 lines · On Fri, 2006-11-17 at 00:34 -0600, [email protected] wrote:

Quoted 3 lines

and really, now that this is out in the wild, maybe sb_features3 instead of padding is appropriate, and check both for the attr2 bit...? :(

Thats not going to work, theres three or four other feature2 bits preceding attr2 as well.

Quoted 4 lines

Quoted 3 lines

The "take a 32 bit systems fs to a 64 bit system" is relatively uncommon, so I suppose its just something we live with (as we did with the log recovery issues in that situation for several years).

So you think this should not be fixed, then? Because if it -is- fixed

I didn't say that. It should be fixed. Noone will notice though, as its not actually biting anyone... (the attr2 problem will not be related to this, its gonna be something else).

...
but it can't just be properly padded in the kernel and leave it at that, can it?

If so won't attr2 filesystems on x86_64 suddenly start
appearing to be attr2?

Quoted 2 lines · Eric Sandeen

...
but it can't just be properly padded in the kernel and leave it at that, can it?

I think it can.

Quoted 2 lines

If so won't attr2 filesystems on x86_64 suddenly start
appearing to be attr2?

What problem do you see resulting from that though?

Quoted 7 lines · Nathan Scott

Quoted 3 lines · Eric Sandeen

...
but it can't just be properly padded in the kernel and leave it at that, can it?

I think it can.

Quoted 2 lines

Quoted 2 lines

If so won't attr2 filesystems on x86_64 suddenly start
appearing to be attr2?

ugh typo... "as attr1" I meant...

Quoted 1 line

What problem do you see resulting from that though?

is an attr2 filesystem mounted as attr1 safe?

Quoted 4 lines · Eric Sandeen

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

Quoted 3 lines

I turned on attr2 in FC6 at nathan's suggestion, for selinux goodness with more efficient xattr space usage.

But, many reports that this was totally broken in fc6, on x86_64.

ugh. it's broken on x86 too, so it's not just the alignment/padding, although that should be fixed for cross-arch mounts.

-Eric

Quoted 7 lines · Eric Sandeen

ugh. it's broken on x86 too, so it's not just the alignment/padding,

although that should be fixed for cross-arch mounts.

-Eric

here's a testcase to corrupt it FWIW.

Quoted 12 lines · Eric Sandeen

Quoted 7 lines · Eric Sandeen

ugh. it's broken on x86 too, so it's not just the alignment/padding,

although that should be fixed for cross-arch mounts.

-Eric

here's a testcase to corrupt it FWIW.

Ok, with expert collaboration from Russell, Barry, Tim, Nathan, David, et al, how about this:

For btree dirs, we need a different calculation for the space used in di_u, to set the minimum threshold for the fork offset...

This fixes my testcase, but as Tim points out -now- we need to compact the btree ptrs, if we return (and use) an offset < current forkoff...

whee....

-Eric

Quoted 20 lines · Eric Sandeen

Quoted 12 lines · Eric Sandeen

Quoted 7 lines · Eric Sandeen

ugh. it's broken on x86 too, so it's not just the alignment/padding,

although that should be fixed for cross-arch mounts.

-Eric

here's a testcase to corrupt it FWIW.

Ok, with expert collaboration from Russell, Barry, Tim, Nathan, David, et al, how about this:

For btree dirs, we need a different calculation for the space used in di_u, to set the minimum threshold for the fork offset...

Quoted 7 lines

This fixes my testcase, but as Tim points out -now- we need to compact the btree ptrs, if we return (and use) an offset < current forkoff...

whee....

-Eric

It turns out this only fixes one of the problems it is still quite easy to corrupt indoes with attr2.

The following patch is a short term fix that address the problem of forkoff
moving without re-factoring the root inode btree root block.

Once the inode has be flipped to BTREE for the data space the forkoff is fixed
to the that size, currently due to the way attr1 worked (fixed size forkoff) the code is not handling the size to the root btree node due to size changes in the attr portion of the inode.

The optimal solution is to adjust the data portion of the inode root btree block down if space exists.

One easy fix that was resulting all attr add being pushed out of line is added
the header size to the initial split of the inode, at least the first attr add
should go inline now. Which should be a win the big attr user right now SElinux.

Including the 2 test script that have been used.

--
Russell Cattelan <[email protected]>

Thanks, Russell.

I've been going thru the irc and just started looking at the patch. I'll get back to you about it tomorrow.

I agree it would be good to have the fixed forkoff for data btree roots as the first fix. And look into redoing the btree root for a later change.

Quoted 7 lines · Timothy Shimmin

Thanks, Russell.

I've been going thru the irc and just started looking at the patch. I'll get back to you about it tomorrow.

I agree it would be good to have the fixed forkoff for data btree roots as the first fix. And look into redoing the btree root for a later change.

My only question is, how much does this defeat the purpose of attr2?

-Eric

Quoted 8 lines · Eric Sandeen

Quoted 7 lines · Timothy Shimmin

Thanks, Russell.

I've been going thru the irc and just started looking at the patch. I'll get back to you about it tomorrow.

I agree it would be good to have the fixed forkoff for data btree roots as the first fix. And look into redoing the btree root for a later change.

Quoted 1 line

My only question is, how much does this defeat the purpose of attr2?

Well from the standpoint that attr2 currently corrupts inodes anything to prevent that is good, since currently attr2 can't be used at all. When the di_u is extent based the attr2 code works as expected, giving space to which ever segment gets there first.The attr2 code should still be a big win for most file/dir inodes since they are probably able to do their block mapping with local or extent mode.

The number of inodes that get pushed to btree mode should be a small % of the
total number of inodes, especially on a root file system. So while attr2 is
not as efficient as it could be for that segment of the inodes the rest of inodes
do benefit from attr2

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.

Quoted 3 lines

-Eric

Quoted 4 lines

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.

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

(quoted diff) +14 -3

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

Quoted 10 lines · <[email protected]> wrote:

Quoted 4 lines

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.

Of course the setting of the forkoff is a bit of a guessing game since we can't predict the future usage but I think the plan is to set it to the minimum to fit on a first come first served basis.

So I'm thinking that we should set it based on the size of shortform if that is how it will be stored or to the size taken up by the EA extents - I was initially thinking that this would be 1 extent but with a remote value block of up to 64K this could in theory be an extent for each fsb of the value I guess.
Have to think about this some more.

--Tim

Quoted 9 lines · Eric Sandeen

Quoted 7 lines · Timothy Shimmin

Thanks, Russell.

I've been going thru the irc and just started looking at the patch. I'll get back to you about it tomorrow.

I agree it would be good to have the fixed forkoff for data btree roots as the first fix. And look into redoing the btree root for a later change.

Quoted 1 line

My only question is, how much does this defeat the purpose of attr2?

Well from the standpoint that attr2 currently corrupts inodes anything to prevent that is good, since currently attr2 can't be used at all. When the di_u is extent based the attr2 code works as expected, giving space to which ever segment gets there first.The attr2 code should still be a big win for most file/dir inodes since they are probably able to do their block mapping with local or extent mode.

Quoted 70 lines · Timothy Shimmin

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

(quoted diff) +14 -3

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

Quoted 7 lines · <[email protected]> wrote:

Quoted 5 lines

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.

Quoted 6 lines

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.

Quoted 13 lines

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.

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.

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.

Quoted 61 lines · Timothy Shimmin

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

(quoted diff) +14 -3

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

Quoted 10 lines · <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 10 lines

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

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.

Quoted 14 lines

Of course the setting of the forkoff is a bit of a guessing game since we can't predict the future usage but I think the plan is to set it to the minimum to fit on a first come first served basis.

So I'm thinking that we should set it based on the size of shortform if that is how it will be stored or to the size taken up by the EA extents - I was initially thinking that this would be 1 extent but with a remote value block of up to 64K this could in theory be an extent for each fsb of the value I guess.
Have to think about this some more.

--Tim

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

I turned on attr2 in FC6 at nathan's suggestion, for selinux goodness with more efficient xattr space usage.

But, many reports that this was totally broken in fc6, on x86_64.

Install went ok, but on reboot the filesystem was found to be corrupt.

The filesystem was also found to be marked w/ attr1, not attr2....

If you do a fresh mkfs.xfs on x86_64, with -i attr=2, and dump out the superblock (or look at it with xfs_db) you will find that although the versionnum says that there is a morebits bit, the features2 flag is 0.

if you dd/hexdump the superblock, you will find the attr2 flag, but at the wrong offset.

This is because the xfs_sb_t struct is padded out to 64 bits on 64-bit arches, and the xfs_xlatesb() routine and xfs_sb_info[] array take this padding to mean that the last item is 4 bytes bigger than it is, and treats sb_features2 as 8 bytes not four. This then gets endian-flipped out...

I can't quite figure out how this winds up causing problems if you stay on the x86_64 arch, as I'd expect that if the offset is wrong, it should at least be consistently wrong. And in fact if you do mkfs,mount,xfs_info, it will tell you that you do have attr2. But somewhere along the line thing go wrong, and post-install, post-reboot, the filesystem thinks it is attr1, and is therefore corrupt.

I think that maybe some accesses are post-xfs_xlatesb, while others may access the un-flipped sb directly? Or maybe this is sb logging code that has messed things up? Not sure... needs more investigation.

In any case, dd does not lie, and this patch for the kernel, and a corresponding one for userspace, at least make "mkfs.xfs -i attr=2" puts the features2 flag in the right place, as shown by inspection via dd.

Signed-off-by Eric Sandeen <[email protected]>

Index: linux-2.6.18/fs/xfs/xfs_sb.h
===================================================================

fs/xfs/xfs_sb.h +1 -1

@@ -149,7 +149,7 @@ typedef struct xfs_sb
 	__uint16_t	sb_logsectsize;	/* sector size for the log, bytes */
 	__uint32_t	sb_logsunit;	/* stripe unit size for the log */
 	__uint32_t	sb_features2;	/* additional feature bits */
-} xfs_sb_t;
+} __attribute__ ((packed)) xfs_sb_t;
 
 /*
  * Sequence number values for the fields.

Quoted 4 lines · Eric Sandeen

see also https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=212201

Bugzilla Bug 212201: Cannot build sysem with XFS file system.

Quoted 4 lines

I turned on attr2 in FC6 at nathan's suggestion, for selinux goodness with more efficient xattr space usage.

But, many reports that this was totally broken in fc6, on x86_64.

Although it turned out to be a different issue, not the packing issue, is the packing/alignment (below) still something that needs to be fixed...?