fix dir2 shortform structures on ARM old ABI

PATCH In review Eric Sandeen 7 participants 37 messages Mar 15, 2008 – Jun 6, 2008 1 message not in the archive XFS filesystem

lore

Share & tools

lore

Eric Sandeen

author

2008-03-15 03:25 UTC #0 lore raw

PATCH 1/1 fix dir2 shortform structures on ARM old ABI

This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

Old ABI ARM has interesting packing & padding; for example on ARM old abi:

struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_sf_off_t          offset;       /*     4     4 */
	__uint8_t                  name[1];      /*     8     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_inou_t            inumber;      /*    12     8 */

	/* size: 20, cachelines: 1 */
	/* sum members: 14, holes: 2, sum holes: 6 */
	/* last cacheline: 20 bytes */
};

but on x86:

struct xfs_dir2_sf_entry {

	__uint8_t                  namelen;      /*     0     1 */
	xfs_dir2_sf_off_t          offset;       /*     1     2 */
	__uint8_t                  name[1];      /*     3     1 */
	xfs_dir2_inou_t            inumber;      /*     4     8 */

	/* size: 12, cachelines: 1 */
	/* last cacheline: 12 bytes */
};

... this sort of discrepancy leads to problems.

I've verified this patch by comparing the on-disk structure layouts using pahole from the dwarves package, as well as running through a bit of xfsqa under qemu-arm, modified so that the check/repair phase after each test actually executes check/repair from the x86 host, on the filesystem populated by the arm emulator. Thus far it all looks good.

There are 2 other structures with extra padding at the end, but they don't seem to cause trouble. I suppose they could be packed as well: xfs_dir2_data_unused_t and xfs_dir2_sf_t.

Note that userspace needs a similar treatment, and any filesystems which were running with the previous rogue "fix" will now see corruption (either in the kernel, or during xfs_repair) with this fix properly in place; it may be worth teaching xfs_repair to identify and fix that specific issue.

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

---

fs/xfs/linux-2.6/xfs_linux.h +7 -0

Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif /* __XFS_LINUX__ */

fs/xfs/xfs_dir2_sf.h +3 -3

Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================
@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must

@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t; 
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Josef 'Jeff' Sipek

2008-03-15 04:17 UTC #1 lore raw

Quoted 7 lines · Eric Sandeen

This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

...

Quoted 9 lines

Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================

fs/xfs/linux-2.6/xfs_linux.h static inline __uint64_t howmany_64(__ui · line 308

Quoted 9 lines

fs/xfs/linux-2.6/xfs_linux.h +6 -0

@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif

Shouldn't this be unconditional? Just because it ends up being ok on x86 doesn't mean that it won't break some time later on...(do we want another bad_features2 incident?)

Quoted 22 lines

fs/xfs/linux-2.6/xfs_linux.h +1 -0

+
 #endif /* __XFS_LINUX__ */

Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================

fs/xfs/xfs_dir2_sf.h +2 -2

@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must

@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;

fs/xfs/xfs_dir2_sf.h typedef struct xfs_dir2_sf_hdr { · line 89

Quoted 10 lines

fs/xfs/xfs_dir2_sf.h +1 -1

 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t; 
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

A very simple patch! I like it (minus the condition vs. unconditional packing - see above).

Josef 'Jeff' Sipek.

Signature · 1 line

Penguin : Linux version 2.6.23.1 on an i386 machine (6135.23 BogoMips).

Eric Sandeen

author

2008-03-15 04:23 UTC #2 lore raw

Quoted 21 lines · Josef 'Jeff' Sipek

Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
...
Quoted 12 lines
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
fs/xfs/linux-2.6/xfs_linux.h +0 -0
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 

Quoted 10 lines

fs/xfs/linux-2.6/xfs_linux.h static inline __uint64_t howmany_64(__ui · line 308
Quoted 6 lines
fs/xfs/linux-2.6/xfs_linux.h +6 -0
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
Shouldn't this be unconditional? Just because it ends up being ok on x86 doesn't mean that it won't break some time later on...(do we want another bad_features2 incident?)

I think that packing structures when they don't need to be can actually be harmful, efficiency-wise. I read a nice explanation of this.... which I can't find now.

-Eric

Josef 'Jeff' Sipek

2008-03-15 05:08 UTC #3 lore raw

Quoted 33 lines · Eric Sandeen

Quoted 21 lines · Josef 'Jeff' Sipek
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
...
Quoted 12 lines
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
fs/xfs/linux-2.6/xfs_linux.h +0 -0
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
Quoted 10 lines
fs/xfs/linux-2.6/xfs_linux.h static inline __uint64_t howmany_64(__ui · line 308
Quoted 6 lines
fs/xfs/linux-2.6/xfs_linux.h +6 -0
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
Shouldn't this be unconditional? Just because it ends up being ok on x86 doesn't mean that it won't break some time later on...(do we want another bad_features2 incident?)

Quoted 3 lines

I think that packing structures when they don't need to be can actually be harmful, efficiency-wise. I read a nice explanation of this.... which I can't find now.

Agreed. For in-memory only structures it makes sense to let the compiler do whatever is the best, but for structures that are on-disk, you really have no choice, you have to have the same layout in memory - which frequently means packed. Unless I missed it, the structs you modified are on-disk, and therefore _must_ be the way the docs say - which happens to be packed.

Josef 'Jeff' Sipek.

Signature · 4 lines

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like
that.
		- Linus Torvalds

Eric Sandeen

author

2008-03-15 04:33 UTC #4 lore raw

Quoted 10 lines · Josef 'Jeff' Sipek

Quoted 8 lines · Eric Sandeen
Quoted 3 lines · Josef 'Jeff' Sipek
Shouldn't this be unconditional? Just because it ends up being ok on x86 doesn't mean that it won't break some time later on...(do we want another bad_features2 incident?)
I think that packing structures when they don't need to be can actually be harmful, efficiency-wise. I read a nice explanation of this.... which I can't find now.

Quoted 5 lines

Agreed. For in-memory only structures it makes sense to let the compiler do whatever is the best, but for structures that are on-disk, you really have no choice, you have to have the same layout in memory - which frequently means packed. Unless I missed it, the structs you modified are on-disk, and therefore _must_ be the way the docs say - which happens to be packed.

Well, the docs probably don't actually say "packed" :) ... they just assume standard/predictable layout of the structures.

So on almost all architectures they _don't_ need to be explicitly packed, and it's my understanding that doing so when it's not neccessary is harmful. But these 3 cases, on the odd arm abi, do need it.

A QA test to run pahole on all disk structures to look for proper sizes / no holes might be good... though it require debug xfs (well, xfs built with -g).

-Eric

Josef 'Jeff' Sipek

2008-03-15 04:51 UTC #5 lore raw

Quoted 11 lines · Eric Sandeen

Quoted 10 lines · Josef 'Jeff' Sipek
Quoted 8 lines · Eric Sandeen
Quoted 3 lines · Josef 'Jeff' Sipek
Shouldn't this be unconditional? Just because it ends up being ok on x86 doesn't mean that it won't break some time later on...(do we want another bad_features2 incident?)
I think that packing structures when they don't need to be can actually be harmful, efficiency-wise. I read a nice explanation of this.... which I can't find now.

Quoted 8 lines

Quoted 5 lines
Agreed. For in-memory only structures it makes sense to let the compiler do whatever is the best, but for structures that are on-disk, you really have no choice, you have to have the same layout in memory - which frequently means packed. Unless I missed it, the structs you modified are on-disk, and therefore _must_ be the way the docs say - which happens to be packed.
Well, the docs probably don't actually say "packed" :) ... they just assume standard/predictable layout of the structures.

Ok, nitpicking, eh? Well, you started ;)

Yes, it is true that they don't say packed, but at the same time they don't define any holes, and the best way to force the compiler to not make holes is to pack the structure.

Quoted 3 lines

So on almost all architectures they _don't_ need to be explicitly packed, and it's my understanding that doing so when it's not neccessary is harmful. But these 3 cases, on the odd arm abi, do need it.

My understanding of the "harmful" case is that of unaligned word access, but the compiler takes care of that.

All in all, all the ABIs that get the right alignment without packing will not suffer performance-wise, and the old arm ABI needs to be packed anyway. Now, next time Linux gets ported to an architecture - if that arch has a "bad" alignment ABI rules, XFS will break, and someone (I nominate you :-P ) will have to augment the #if...or just use packed and forget about the whole issue. :)

Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)

Signature · 6 lines

Research, n.:
  Consider Columbus:
    He didn't know where he was going.
    When he got there he didn't know where he was.
    When he got back he didn't know where he had been.
    And he did it all on someone else's money.

Eric Sandeen

author

2008-03-17 18:32 UTC #6 lore raw

Quoted 2 lines · Josef 'Jeff' Sipek

Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)

Very. ;)

Tossing packed at all the ondisk stuctures bloats things badly on ia64.

cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis

That's +15606 lines.

http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

Please, don't do this.

_Annotating_ ondisk structures sounds good to me, assuming something can be done with it (i.e., testing for holes - I'd thought of this a while ago too, but never came up with anything to make use of it) but don't pack stuff just for fun.

-Eric

Josef 'Jeff' Sipek

2008-03-17 19:52 UTC #7 lore raw

Quoted 7 lines · Eric Sandeen

Quoted 2 lines · Josef 'Jeff' Sipek
Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)
Very. ;)

Quoted 8 lines

Tossing packed at all the ondisk stuctures bloats things badly on ia64.
cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis
That's +15606 lines.

I'm not done yet! :-P

First of all, the patch I showed you actually breaks a few things that I still need to fix.

Second, I need to find out whether all the affected structures are always aligned on some boundary (probably 4 or 8 byte). If there indeed is some alignment, there might be a way to reduce those 15k extra lines to something a whole lot less - I hope.

Josef 'Jeff' Sipek.

Signature · 1 line

A CRAY is the only computer that runs an endless loop in just 4 hours...

Eric Sandeen

author

2008-03-17 20:03 UTC #8 lore raw

Quoted 14 lines · Josef 'Jeff' Sipek

Quoted 13 lines · Eric Sandeen
Quoted 2 lines · Josef 'Jeff' Sipek
Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)
Very. ;)
Tossing packed at all the ondisk stuctures bloats things badly on ia64.
cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis

Quoted 6 lines

Quoted 1 line
That's +15606 lines.
I'm not done yet! :-P
First of all, the patch I showed you actually breaks a few things that I still need to fix.

Oh, I wasn't trying to blame you or our patch specifically, just wanted to highlight what I consider to be the bad idea of giving gcc a bunch of directives that IMHO we don't need.

Quoted 4 lines

Second, I need to find out whether all the affected structures are always aligned on some boundary (probably 4 or 8 byte). If there indeed is some alignment, there might be a way to reduce those 15k extra lines to something a whole lot less - I hope.

To what end? What are you trying to fix? If it's not reduced to 0 then your change is introducing regressions, IMHO.

Respectfully, ;)
-Eric

Josef 'Jeff' Sipek

2008-03-17 23:43 UTC #9 lore raw

Quoted 1 line · Eric Sandeen

Josef 'Jeff' Sipek wrote:

...

Quoted 3 lines

Oh, I wasn't trying to blame you or our patch specifically, just wanted to highlight what I consider to be the bad idea of giving gcc a bunch of directives that IMHO we don't need.

Right. And yes, just plopping __attribute__((packed)) to the end of each on-disk structure is a really bad idea - it actually make xfsqa fail :)

But living on the edge, without telling gcc what exactly we want from it is an even worse idea! Take sb_bad_features2...that fiasco that's going to stay with the XFS on-disk format for a long time to come, would have never happened if the structures were properly packed/padded to begin with.

Quoted 6 lines

Quoted 4 lines
Second, I need to find out whether all the affected structures are always aligned on some boundary (probably 4 or 8 byte). If there indeed is some alignment, there might be a way to reduce those 15k extra lines to something a whole lot less - I hope.
To what end? What are you trying to fix? If it's not reduced to 0 then

Not packing the structures is all fine if you have one compiler, one OS, and one architecture to care about. That worked fine when XFS ran on IRIX on MIPS, but Linux runs on so many different ABIs that you are asking for trouble by not packing. I can't imagine the number of supported ABIs not growing. Packing on as-needed basis (which you suggested with your patch) is rather messy...

1) new ABIs have to checked

2) you'll end up with a rat's nest of #ifdefs to make __arch_pack do the right thing

Really, we need a way to tell gcc: "hey, gcc, we know what we're doing - just trust us; don't pad, and don't worry about the alignment". packed gets you the no-padding, and as I mentioned in my previous reply, align(X) will hopefully take care of the alignment. Then, gcc should generate nice code to access members that are on proper boundaries (AFAIK virtually all, if not all, the struct members in XFS fall into this category), and slightly worse code for few places that might exist.

The problem you saw in ia64, is because gcc generated the "worst" case code for all the struct accesses. I _think_ that can be fixed to near-0, if not 0 on all but the few ABIs (e.g., old arm).

Josef 'Jeff' Sipek.

Signature · 1 line

The box said "Windows XP or better required". So I installed Linux.

Josef 'Jeff' Sipek

2008-03-18 00:39 UTC #10 lore raw

RFC PATCH 1/1 XFS: annotate all on-disk structures with __ondisk

Currently, the annotation just forces the structures to be packed, and 4-byte aligned.

Signed-off-by Josef 'Jeff' Sipek <[email protected]>

---
This is just an RFC, and the alignment needs to be verified against the offsets within the pages read from disk, and more xfsqa runs on various architectures are needed. (I don't want to be responsible for something like the bitops regression on ppc!)

The .text segment shrinks on x86 and s390x, but grows in ia64 (3776 bytes == 0.3%).

   text    data     bss     dec     hex filename
 542054    3171    3084  548309   85dd5 xfs-x86-original.ko
 542026    3171    3084  548281   85db9 xfs-x86-packed-aligned4.ko
1244057   70858    2480 1317395  141a13 xfs-ia64-original.ko
1247833   70858    2480 1321171  1428d3 xfs-ia64-packed-aligned4.ko
 679901   19374    3112  702387   ab7b3 xfs-s390x-original.ko
 679781   19374    3112  702267   ab73b xfs-s390x-packed-aligned4.ko

The approximate number of instructions effectively stays the same on x86 (goes up by 2), s390x gets smaller (by 12 instructions), but ia64 bloats by 708 instructions (0.34%).

$ for x in *.ko; do objdump -d $x > `basename $x .ko`.dis ; done
$ wc -l *.dis
  141494 xfs-x86-original.dis
  141496 xfs-x86-packed-aligned4.dis
  208514 xfs-ia64-original.dis
  209222 xfs-ia64-packed-aligned4.dis
  121544 xfs-s390x-original.dis
  121532 xfs-s390x-packed-aligned4.dis

I could try to compile things on a sparc64, mips, and x86_64, but that's for another day - and depending on where this thread will lead.

The patch keeps xfsqa happy on x86 - well, it dies in the 100-range, but I haven't had the time to check if that happens without this patch. Someone (not it!) should nurse xfsqa back to health :)

Jeff.

---
 fs/xfs/linux-2.6/xfs_linux.h |    1 +
 fs/xfs/xfs_ag.h              |    6 +++---
 fs/xfs/xfs_alloc_btree.h     |    2 +-
 fs/xfs/xfs_attr_leaf.h       |   14 +++++++-------
 fs/xfs/xfs_attr_sf.h         |    9 +++++----
 fs/xfs/xfs_bmap_btree.h      |    8 ++++----
 fs/xfs/xfs_btree.h           |   12 ++++++------
 fs/xfs/xfs_da_btree.h        |    8 ++++----
 fs/xfs/xfs_dinode.h          |    6 +++---
 fs/xfs/xfs_dir2_block.h      |    4 ++--
 fs/xfs/xfs_dir2_data.h       |   10 +++++-----
 fs/xfs/xfs_dir2_leaf.h       |    9 +++++----
 fs/xfs/xfs_dir2_node.h       |    5 +++--
 fs/xfs/xfs_dir2_sf.h         |   14 +++++++-------
 fs/xfs/xfs_ialloc_btree.h    |    4 ++--
 fs/xfs/xfs_log_priv.h        |    6 +++---
 fs/xfs/xfs_quota.h           |    4 ++--
 fs/xfs/xfs_sb.h              |    2 +-
 fs/xfs/xfs_trans.h           |    2 +-
 19 files changed, 65 insertions(+), 61 deletions(-)

fs/xfs/linux-2.6/xfs_linux.h +1 -0

index 284460f..f06199c 100644
@@ -186,6 +186,7 @@
 #define xfs_itruncate_data(ip, off)	\
 	(-vmtruncate(vn_to_inode(XFS_ITOV(ip)), (off)))
 
+#define __ondisk	__attribute__((packed,aligned(4)))
 
 /* Move the kernel do_div definition off to one side */

fs/xfs/xfs_ag.h +3 -3

index 61b292a..20f3291 100644
@@ -69,7 +69,7 @@ typedef struct xfs_agf {
 	__be32		agf_freeblks;	/* total free blocks */
 	__be32		agf_longest;	/* longest free space */
 	__be32		agf_btreeblks;	/* # of blocks held in AGF btrees */
-} xfs_agf_t;
+} __ondisk xfs_agf_t;
 
 #define	XFS_AGF_MAGICNUM	0x00000001
 #define	XFS_AGF_VERSIONNUM	0x00000002

@@ -121,7 +121,7 @@ typedef struct xfs_agi {
 	 * still being referenced.
 	 */
 	__be32		agi_unlinked[XFS_AGI_UNLINKED_BUCKETS];
-} xfs_agi_t;
+} __ondisk xfs_agi_t;
 
 #define	XFS_AGI_MAGICNUM	0x00000001
 #define	XFS_AGI_VERSIONNUM	0x00000002

@@ -153,7 +153,7 @@ typedef struct xfs_agi {
 
 typedef struct xfs_agfl {
 	__be32		agfl_bno[1];	/* actually XFS_AGFL_SIZE(mp) */
-} xfs_agfl_t;
+} __ondisk xfs_agfl_t;
 
 /*
  * Busy block/extent entry.  Used in perag to mark blocks that have been freed

fs/xfs/xfs_alloc_btree.h +1 -1

index 5bd1a2c..f7c5bba 100644
@@ -41,7 +41,7 @@ struct xfs_mount;
 typedef struct xfs_alloc_rec {
 	__be32		ar_startblock;	/* starting block number */
 	__be32		ar_blockcount;	/* count of free blocks */
-} xfs_alloc_rec_t, xfs_alloc_key_t;
+} __ondisk xfs_alloc_rec_t, xfs_alloc_key_t;
 
 typedef struct xfs_alloc_rec_incore {
 	xfs_agblock_t	ar_startblock;	/* starting block number */

fs/xfs/xfs_attr_leaf.h +7 -7

index 040f732..792d2a9 100644
@@ -75,7 +75,7 @@ struct xfs_trans;
 typedef struct xfs_attr_leaf_map {	/* RLE map of free bytes */
 	__be16	base;			  /* base of free region */
 	__be16	size;			  /* length of free region */
-} xfs_attr_leaf_map_t;
+} __ondisk xfs_attr_leaf_map_t;
 
 typedef struct xfs_attr_leaf_hdr {	/* constant-structure header block */
 	xfs_da_blkinfo_t info;		/* block type, links, etc. */

@@ -86,34 +86,34 @@ typedef struct xfs_attr_leaf_hdr {	/* constant-structure header block */
 	__u8	pad1;
 	xfs_attr_leaf_map_t freemap[XFS_ATTR_LEAF_MAPSIZE];
 					/* N largest free regions */
-} xfs_attr_leaf_hdr_t;
+} __ondisk xfs_attr_leaf_hdr_t;
 
 typedef struct xfs_attr_leaf_entry {	/* sorted on key, not name */
 	__be32	hashval;		/* hash value of name */
  	__be16	nameidx;		/* index into buffer of name/value */
 	__u8	flags;			/* LOCAL/ROOT/SECURE/INCOMPLETE flag */
 	__u8	pad2;			/* unused pad byte */
-} xfs_attr_leaf_entry_t;
+} __ondisk xfs_attr_leaf_entry_t;
 
 typedef struct xfs_attr_leaf_name_local {
 	__be16	valuelen;		/* number of bytes in value */
 	__u8	namelen;		/* length of name bytes */
 	__u8	nameval[1];		/* name/value bytes */
-} xfs_attr_leaf_name_local_t;
+} __ondisk xfs_attr_leaf_name_local_t;
 
 typedef struct xfs_attr_leaf_name_remote {
 	__be32	valueblk;		/* block number of value bytes */
 	__be32	valuelen;		/* number of bytes in value */
 	__u8	namelen;		/* length of name bytes */
-	__u8	name[1];		/* name bytes */
-} xfs_attr_leaf_name_remote_t;
+	__u8	name[3];		/* name bytes */
+} __ondisk xfs_attr_leaf_name_remote_t;
 
 typedef struct xfs_attr_leafblock {
 	xfs_attr_leaf_hdr_t	hdr;	/* constant-structure header block */
 	xfs_attr_leaf_entry_t	entries[1];	/* sorted on key, not name */
 	xfs_attr_leaf_name_local_t namelist;	/* grows from bottom of buf */
 	xfs_attr_leaf_name_remote_t valuelist;	/* grows from bottom of buf */
-} xfs_attr_leafblock_t;
+} __ondisk xfs_attr_leafblock_t;
 
 /*
  * Flags used in the leaf_entry[i].flags field.

fs/xfs/xfs_attr_sf.h +5 -4

index f67f917..a13afb7 100644
@@ -33,15 +33,16 @@ struct xfs_inode;
 typedef struct xfs_attr_shortform {
 	struct xfs_attr_sf_hdr {	/* constant-structure header block */
 		__be16	totsize;	/* total bytes in shortform list */
-		__u8	count;	/* count of active entries */
-	} hdr;
+		__u8	count;		/* count of active entries */
+		__u8	pad;
+	} __ondisk hdr;
 	struct xfs_attr_sf_entry {
 		__uint8_t namelen;	/* actual length of name (no NULL) */
 		__uint8_t valuelen;	/* actual length of value (no NULL) */
 		__uint8_t flags;	/* flags bits (see xfs_attr_leaf.h) */
 		__uint8_t nameval[1];	/* name & value bytes concatenated */
-	} list[1];			/* variable sized array */
-} xfs_attr_shortform_t;
+	} __ondisk list[1];			/* variable sized array */
+} __ondisk xfs_attr_shortform_t;
 typedef struct xfs_attr_sf_hdr xfs_attr_sf_hdr_t;
 typedef struct xfs_attr_sf_entry xfs_attr_sf_entry_t;

fs/xfs/xfs_bmap_btree.h +4 -4

index cd0d4b4..3c749c8 100644
@@ -31,7 +31,7 @@ struct xfs_inode;
 typedef struct xfs_bmdr_block {
 	__be16		bb_level;	/* 0 is a leaf */
 	__be16		bb_numrecs;	/* current # of data records */
-} xfs_bmdr_block_t;
+} __ondisk xfs_bmdr_block_t;
 
 /*
  * Bmap btree record and extent descriptor.

@@ -51,11 +51,11 @@ typedef struct xfs_bmdr_block {
 typedef struct xfs_bmbt_rec_32
 {
 	__uint32_t		l0, l1, l2, l3;
-} xfs_bmbt_rec_32_t;
+} __ondisk xfs_bmbt_rec_32_t;
 typedef struct xfs_bmbt_rec_64
 {
 	__be64			l0, l1;
-} xfs_bmbt_rec_64_t;
+} __ondisk xfs_bmbt_rec_64_t;
 
 typedef __uint64_t	xfs_bmbt_rec_base_t;	/* use this for casts */
 typedef xfs_bmbt_rec_64_t xfs_bmbt_rec_t, xfs_bmdr_rec_t;

@@ -140,7 +140,7 @@ typedef struct xfs_bmbt_irec
  */
 typedef struct xfs_bmbt_key {
 	__be64		br_startoff;	/* starting file offset */
-} xfs_bmbt_key_t, xfs_bmdr_key_t;
+} __ondisk xfs_bmbt_key_t, xfs_bmdr_key_t;
 
 /* btree pointer type */
 typedef __be64 xfs_bmbt_ptr_t, xfs_bmdr_ptr_t;

fs/xfs/xfs_btree.h +6 -6

index 7440b78..40ac5b8 100644
@@ -47,7 +47,7 @@ typedef struct xfs_btree_sblock {
 	__be16		bb_numrecs;	/* current # of data records */
 	__be32		bb_leftsib;	/* left sibling block or NULLAGBLOCK */
 	__be32		bb_rightsib;	/* right sibling block or NULLAGBLOCK */
-} xfs_btree_sblock_t;
+} __ondisk xfs_btree_sblock_t;
 
 /*
  * Long form header: bmap btrees.

@@ -58,7 +58,7 @@ typedef struct xfs_btree_lblock {
 	__be16		bb_numrecs;	/* current # of data records */
 	__be64		bb_leftsib;	/* left sibling block or NULLDFSBNO */
 	__be64		bb_rightsib;	/* right sibling block or NULLDFSBNO */
-} xfs_btree_lblock_t;
+} __ondisk xfs_btree_lblock_t;
 
 /*
  * Combined header and structure, used by common code.

@@ -68,7 +68,7 @@ typedef struct xfs_btree_hdr
 	__be32		bb_magic;	/* magic number for block type */
 	__be16		bb_level;	/* 0 is a leaf */
 	__be16		bb_numrecs;	/* current # of data records */
-} xfs_btree_hdr_t;
+} __ondisk xfs_btree_hdr_t;
 
 typedef struct xfs_btree_block {
 	xfs_btree_hdr_t	bb_h;		/* header */

@@ -76,13 +76,13 @@ typedef struct xfs_btree_block {
 		struct {
 			__be32		bb_leftsib;
 			__be32		bb_rightsib;
-		} s;			/* short form pointers */
+		} __ondisk s;		/* short form pointers */
 		struct	{
 			__be64		bb_leftsib;
 			__be64		bb_rightsib;
-		} l;			/* long form pointers */
+		} __ondisk l;		/* long form pointers */
 	} bb_u;				/* rest */
-} xfs_btree_block_t;
+} __ondisk xfs_btree_block_t;
 
 /*
  * For logging record fields.

fs/xfs/xfs_da_btree.h +4 -4

index 7facf86..36901d7 100644
@@ -45,7 +45,7 @@ typedef struct xfs_da_blkinfo {
 	__be32		back;			/* following block in list */
 	__be16		magic;			/* validity check on block */
 	__be16		pad;			/* unused */
-} xfs_da_blkinfo_t;
+} __ondisk xfs_da_blkinfo_t;
 
 /*
  * This is the structure of the root and intermediate nodes in the Btree.

@@ -63,12 +63,12 @@ typedef struct xfs_da_intnode {
 		xfs_da_blkinfo_t info;	/* block type, links, etc. */
 		__be16	count;		/* count of active entries */
 		__be16	level;		/* level above leaves (leaf == 0) */
-	} hdr;
+	} __ondisk hdr;
 	struct xfs_da_node_entry {
 		__be32	hashval;	/* hash value for this descendant */
 		__be32	before;		/* Btree block before this key */
-	} btree[1];			/* variable sized array of keys */
-} xfs_da_intnode_t;
+	} __ondisk btree[1];		/* variable sized array of keys */
+} __ondisk xfs_da_intnode_t;
 typedef struct xfs_da_node_hdr xfs_da_node_hdr_t;
 typedef struct xfs_da_node_entry xfs_da_node_entry_t;

fs/xfs/xfs_dinode.h +3 -3

index c9065ea..9a24755 100644
@@ -36,7 +36,7 @@ struct xfs_mount;
 typedef struct xfs_timestamp {
 	__be32		t_sec;		/* timestamp seconds */
 	__be32		t_nsec;		/* timestamp nanoseconds */
-} xfs_timestamp_t;
+} __ondisk xfs_timestamp_t;
 
 /*
  * Note: Coordinate changes to this structure with the XFS_DI_* #defines

@@ -69,7 +69,7 @@ typedef struct xfs_dinode_core {
 	__be16		di_dmstate;	/* DMIG state info */
 	__be16		di_flags;	/* random flags, XFS_DIFLAG_... */
 	__be32		di_gen;		/* generation number */
-} xfs_dinode_core_t;
+} __ondisk xfs_dinode_core_t;
 
 #define DI_MAX_FLUSH 0xffff

@@ -96,7 +96,7 @@ typedef struct xfs_dinode
 		xfs_bmbt_rec_32_t di_abmx[1];	/* extent list */
 		xfs_attr_shortform_t di_attrsf;	/* shortform attribute list */
 	}		di_a;
-} xfs_dinode_t;
+} __ondisk xfs_dinode_t;
 
 /*
  * The 32 bit link count in the inode theoretically maxes out at UINT_MAX.

fs/xfs/xfs_dir2_block.h +2 -2

index 10e6896..a85f98d 100644
@@ -45,7 +45,7 @@ struct xfs_trans;
 typedef struct xfs_dir2_block_tail {
 	__be32		count;			/* count of leaf entries */
 	__be32		stale;			/* count of stale lf entries */
-} xfs_dir2_block_tail_t;
+} __ondisk xfs_dir2_block_tail_t;
 
 /*
  * Generic single-block structure, for xfs_db.

@@ -55,7 +55,7 @@ typedef struct xfs_dir2_block {
 	xfs_dir2_data_union_t	u[1];
 	xfs_dir2_leaf_entry_t	leaf[1];
 	xfs_dir2_block_tail_t	tail;
-} xfs_dir2_block_t;
+} __ondisk xfs_dir2_block_t;
 
 /*
  * Pointer to the leaf header embedded in a data block (1-block format)

fs/xfs/xfs_dir2_data.h +5 -5

index b816e02..e7ae1db 100644
@@ -67,7 +67,7 @@ struct xfs_trans;
 typedef struct xfs_dir2_data_free {
 	__be16			offset;		/* start of freespace */
 	__be16			length;		/* length of freespace */
-} xfs_dir2_data_free_t;
+} __ondisk xfs_dir2_data_free_t;
 
 /*
  * Header for the data blocks.

@@ -78,7 +78,7 @@ typedef struct xfs_dir2_data_hdr {
 	__be32			magic;		/* XFS_DIR2_DATA_MAGIC */
 						/* or XFS_DIR2_BLOCK_MAGIC */
 	xfs_dir2_data_free_t	bestfree[XFS_DIR2_DATA_FD_COUNT];
-} xfs_dir2_data_hdr_t;
+} __ondisk xfs_dir2_data_hdr_t;
 
 /*
  * Active entry in a data block.  Aligned to 8 bytes.

@@ -90,7 +90,7 @@ typedef struct xfs_dir2_data_entry {
 	__u8			name[1];	/* name bytes, no null */
 						/* variable offset */
 	__be16			tag;		/* starting offset of us */
-} xfs_dir2_data_entry_t;
+} __ondisk xfs_dir2_data_entry_t;
 
 /*
  * Unused entry in a data block.  Aligned to 8 bytes.

@@ -101,7 +101,7 @@ typedef struct xfs_dir2_data_unused {
 	__be16			length;		/* total free length */
 						/* variable offset */
 	__be16			tag;		/* starting offset of us */
-} xfs_dir2_data_unused_t;
+} __ondisk xfs_dir2_data_unused_t;
 
 typedef union {
 	xfs_dir2_data_entry_t	entry;

@@ -114,7 +114,7 @@ typedef union {
 typedef struct xfs_dir2_data {
 	xfs_dir2_data_hdr_t	hdr;		/* magic XFS_DIR2_DATA_MAGIC */
 	xfs_dir2_data_union_t	u[1];
-} xfs_dir2_data_t;
+} __ondisk xfs_dir2_data_t;
 
 /*
  * Macros.

fs/xfs/xfs_dir2_leaf.h +5 -4

index 6c9539f..01a6091 100644
@@ -48,7 +48,7 @@ typedef struct xfs_dir2_leaf_hdr {
 	xfs_da_blkinfo_t	info;		/* header for da routines */
 	__be16			count;		/* count of entries */
 	__be16			stale;		/* count of stale entries */
-} xfs_dir2_leaf_hdr_t;
+} __ondisk xfs_dir2_leaf_hdr_t;
 
 /*
  * Leaf block entry.

@@ -56,14 +56,14 @@ typedef struct xfs_dir2_leaf_hdr {
 typedef struct xfs_dir2_leaf_entry {
 	__be32			hashval;	/* hash value of name */
 	__be32			address;	/* address of data entry */
-} xfs_dir2_leaf_entry_t;
+} __ondisk xfs_dir2_leaf_entry_t;
 
 /*
  * Leaf block tail.
  */
 typedef struct xfs_dir2_leaf_tail {
 	__be32			bestcount;
-} xfs_dir2_leaf_tail_t;
+} __ondisk xfs_dir2_leaf_tail_t;
 
 /*
  * Leaf block.

@@ -75,8 +75,9 @@ typedef struct xfs_dir2_leaf {
 	xfs_dir2_leaf_entry_t	ents[1];	/* entries */
 						/* ... */
 	xfs_dir2_data_off_t	bests[1];	/* best free counts */
+	__u8			pad[2];
 	xfs_dir2_leaf_tail_t	tail;		/* leaf tail */
-} xfs_dir2_leaf_t;
+} __ondisk xfs_dir2_leaf_t;
 
 /*
  * DB blocks here are logical directory block numbers, not filesystem blocks.

fs/xfs/xfs_dir2_node.h +3 -2

index dde72db..78ab236 100644
@@ -45,13 +45,14 @@ typedef	struct xfs_dir2_free_hdr {
 	__be32			firstdb;	/* db of first entry */
 	__be32			nvalid;		/* count of valid entries */
 	__be32			nused;		/* count of used entries */
-} xfs_dir2_free_hdr_t;
+} __ondisk xfs_dir2_free_hdr_t;
 
 typedef struct xfs_dir2_free {
 	xfs_dir2_free_hdr_t	hdr;		/* block header */
 	__be16			bests[1];	/* best free counts */
 						/* unused entries are -1 */
-} xfs_dir2_free_t;
+	__u8			pad[2];
+} __ondisk xfs_dir2_free_t;
 
 #define	XFS_DIR2_MAX_FREE_BESTS(mp)	\
 	(((mp)->m_dirblksize - (uint)sizeof(xfs_dir2_free_hdr_t)) / \

fs/xfs/xfs_dir2_sf.h +7 -7

index 005629d..5229bf2 100644
@@ -43,26 +43,26 @@ struct xfs_trans;
 /*
  * Inode number stored as 8 8-bit values.
  */
-typedef	struct { __uint8_t i[8]; } xfs_dir2_ino8_t;
+typedef	struct { __uint8_t i[8]; } __ondisk xfs_dir2_ino8_t;
 
 /*
  * Inode number stored as 4 8-bit values.
  * Works a lot of the time, when all the inode numbers in a directory
  * fit in 32 bits.
  */
-typedef struct { __uint8_t i[4]; } xfs_dir2_ino4_t;
+typedef struct { __uint8_t i[4]; } __ondisk xfs_dir2_ino4_t;
 
 typedef union {
 	xfs_dir2_ino8_t	i8;
 	xfs_dir2_ino4_t	i4;
-} xfs_dir2_inou_t;
+} __ondisk xfs_dir2_inou_t;
 #define	XFS_DIR2_MAX_SHORT_INUM	((xfs_ino_t)0xffffffffULL)
 
 /*
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __ondisk xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must

@@ -76,19 +76,19 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __ondisk xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __ondisk xfs_dir2_sf_entry_t;
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */
 	xfs_dir2_sf_entry_t	list[1];	/* shortform entries */
-} xfs_dir2_sf_t;
+} __ondisk xfs_dir2_sf_t;
 
 static inline int xfs_dir2_sf_hdr_size(int i8count)
 {

fs/xfs/xfs_ialloc_btree.h +2 -2

index 8efc4a5..036cdd0 100644
@@ -51,7 +51,7 @@ typedef struct xfs_inobt_rec {
 	__be32		ir_startino;	/* starting inode number */
 	__be32		ir_freecount;	/* count of free inodes (set bits) */
 	__be64		ir_free;	/* free inode mask */
-} xfs_inobt_rec_t;
+} __ondisk xfs_inobt_rec_t;
 
 typedef struct xfs_inobt_rec_incore {
 	xfs_agino_t	ir_startino;	/* starting inode number */

@@ -65,7 +65,7 @@ typedef struct xfs_inobt_rec_incore {
  */
 typedef struct xfs_inobt_key {
 	__be32		ir_startino;	/* starting inode number */
-} xfs_inobt_key_t;
+} __ondisk xfs_inobt_key_t;
 
 /* btree pointer type */
 typedef __be32 xfs_inobt_ptr_t;

fs/xfs/xfs_log_priv.h +3 -3

index 01c63db..81c5a2d 100644
@@ -270,7 +270,7 @@ typedef struct xlog_op_header {
 	__u8	   oh_clientid;	/* who sent me this		:  1 b */
 	__u8	   oh_flags;	/*				:  1 b */
 	__u16	   oh_res2;	/* 32 bit align			:  2 b */
-} xlog_op_header_t;
+} __ondisk xlog_op_header_t;
 
 
 /* valid values for h_fmt */

@@ -301,12 +301,12 @@ typedef struct xlog_rec_header {
 	__be32    h_fmt;        /* format of log record                 :  4 */
 	uuid_t	  h_fs_uuid;    /* uuid of FS                           : 16 */
 	__be32	  h_size;	/* iclog size				:  4 */
-} xlog_rec_header_t;
+} __ondisk xlog_rec_header_t;
 
 typedef struct xlog_rec_ext_header {
 	__be32	  xh_cycle;	/* write cycle of log			: 4 */
 	__be32	  xh_cycle_data[XLOG_HEADER_CYCLE_SIZE / BBSIZE]; /*	: 256 */
-} xlog_rec_ext_header_t;
+} __ondisk xlog_rec_ext_header_t;
 
 #ifdef __KERNEL__
 /*

fs/xfs/xfs_quota.h +2 -2

index 12c4ec7..f5b9c30 100644
@@ -67,7 +67,7 @@ typedef struct	xfs_disk_dquot {
 	__be32		d_rtbtimer;	/* similar to above; for RT disk blocks */
 	__be16		d_rtbwarns;	/* warnings issued wrt RT disk blocks */
 	__be16		d_pad;
-} xfs_disk_dquot_t;
+} __ondisk xfs_disk_dquot_t;
 
 /*
  * This is what goes on disk. This is separated from the xfs_disk_dquot because

@@ -76,7 +76,7 @@ typedef struct	xfs_disk_dquot {
 typedef struct xfs_dqblk {
 	xfs_disk_dquot_t  dd_diskdq;	/* portion that lives incore as well */
 	char		  dd_fill[32];	/* filling for posterity */
-} xfs_dqblk_t;
+} __ondisk xfs_dqblk_t;
 
 /*
  * flags for q_flags field in the dquot.

fs/xfs/xfs_sb.h +1 -1

index b1a83f8..beee35e 100644
@@ -226,7 +226,7 @@ typedef struct xfs_dsb {
 	__be32	sb_bad_features2;
 
 	/* must be padded to 64 bit alignment */
-} xfs_dsb_t;
+} __ondisk xfs_dsb_t;
 
 /*
  * Sequence number values for the fields.

fs/xfs/xfs_trans.h +1 -1

index 0804207..2fbe465 100644
@@ -32,7 +32,7 @@ typedef struct xfs_trans_header {
 	uint		th_type;		/* transaction type */
 	__int32_t	th_tid;			/* transaction id (unused) */
 	uint		th_num_items;		/* num items logged by trans */
-} xfs_trans_header_t;
+} __ondisk xfs_trans_header_t;
 
 #define	XFS_TRANS_HEADER_MAGIC	0x5452414e	/* TRAN */

Signature · 1 line

1.5.4.rc2.85.g9de45-dirty

Eric Sandeen

author

2008-03-18 03:34 UTC #11 lore raw

Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk

Quoted 2 lines · Josef 'Jeff' Sipek

Currently, the annotation just forces the structures to be packed, and 4-byte aligned.

Semantic nitpick: in my definition of "annotation" this is more than just an annotation.

An "__ondisk" annotation, to me, would allow something like sparse to verify properly laid out on-disk structures, but would not affect the actual runtime code - I think that would be quite useful. However, this
change actually impacts the bytecode; it is a functional change.

So I really do understand what you're trying to do, despite my protestations. If there is some magical instruction to gcc which:

a) leaves all current "non-broken" ABIs and gcc implementations' bytecode untouched (or at the very least, minimally/trivially modified), and

b) fixes all possible future ABIs so they will always have things perfectly and properly aligned, again w/o undue molestation of the resulting bytecode

then I could probably be convinced. :) But this seems like a tall order, and would require much scrutiny.

I'm just very shy of a sweeping change like this, which has a material impact on the most common architectures, and does not actually provide, as far as I can see, any benefit to them - only risk.

And for now I'll shut up and let the sgi guys chime in eventually. :)

-Eric

Quoted 40 lines

Signed-off-by Josef 'Jeff' Sipek <[email protected]>
---
This is just an RFC, and the alignment needs to be verified against the offsets within the pages read from disk, and more xfsqa runs on various architectures are needed. (I don't want to be responsible for something like the bitops regression on ppc!)
The .text segment shrinks on x86 and s390x, but grows in ia64 (3776 bytes == 0.3%).
   text    data     bss     dec     hex filename
 542054    3171    3084  548309   85dd5 xfs-x86-original.ko
 542026    3171    3084  548281   85db9 xfs-x86-packed-aligned4.ko
1244057   70858    2480 1317395  141a13 xfs-ia64-original.ko
1247833   70858    2480 1321171  1428d3 xfs-ia64-packed-aligned4.ko
 679901   19374    3112  702387   ab7b3 xfs-s390x-original.ko
 679781   19374    3112  702267   ab73b xfs-s390x-packed-aligned4.ko
The approximate number of instructions effectively stays the same on x86 (goes up by 2), s390x gets smaller (by 12 instructions), but ia64 bloats by 708 instructions (0.34%).
$ for x in *.ko; do objdump -d $x > `basename $x .ko`.dis ; done
$ wc -l *.dis
  141494 xfs-x86-original.dis
  141496 xfs-x86-packed-aligned4.dis
  208514 xfs-ia64-original.dis
  209222 xfs-ia64-packed-aligned4.dis
  121544 xfs-s390x-original.dis
  121532 xfs-s390x-packed-aligned4.dis
I could try to compile things on a sparc64, mips, and x86_64, but that's for another day - and depending on where this thread will lead.
The patch keeps xfsqa happy on x86 - well, it dies in the 100-range, but I haven't had the time to check if that happens without this patch. Someone (not it!) should nurse xfsqa back to health :)
Jeff.

David Chinner

2008-03-18 04:08 UTC #12 lore raw

Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk

Quoted 11 lines · Eric Sandeen

Quoted 2 lines · Josef 'Jeff' Sipek
Currently, the annotation just forces the structures to be packed, and 4-byte aligned.
Semantic nitpick: in my definition of "annotation" this is more than just an annotation.
An "__ondisk" annotation, to me, would allow something like sparse to verify properly laid out on-disk structures, but would not affect the actual runtime code - I think that would be quite useful. However, this
change actually impacts the bytecode; it is a functional change.

Yup - this isn't "annotation"....

Quoted 10 lines

So I really do understand what you're trying to do, despite my protestations. If there is some magical instruction to gcc which:
a) leaves all current "non-broken" ABIs and gcc implementations' bytecode untouched (or at the very least, minimally/trivially modified), and
b) fixes all possible future ABIs so they will always have things perfectly and properly aligned, again w/o undue molestation of the resulting bytecode

Quoted 8 lines

then I could probably be convinced. :) But this seems like a tall order, and would require much scrutiny.
I'm just very shy of a sweeping change like this, which has a material impact on the most common architectures, and does not actually provide, as far as I can see, any benefit to them - only risk.
And for now I'll shut up and let the sgi guys chime in eventually. :)

I think you iterated my concerns quite well, Eric.

The thing I want to see for any sort of change like this is output off all the structures and their alignment before the change and their alignment after the change. On all supported arches. 'pahole' is the tool you used for that, wasn't it, Eric?

The only arch I would expect to see a change in the structures is on ARM; if there's anything other than that there there's something wrong. This is going to require a lot of validation to ensure that it is correct.....

Not to mention performance testing on ia64 given the added overhead in critical paths.....

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Josef 'Jeff' Sipek

2008-03-18 05:27 UTC #13 lore raw

Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk

Quoted 8 lines · David Chinner

Quoted 7 lines · Eric Sandeen
Quoted 2 lines · Josef 'Jeff' Sipek
Currently, the annotation just forces the structures to be packed, and 4-byte aligned.
Semantic nitpick: in my definition of "annotation" this is more than just an annotation.

Quoted 6 lines

Quoted 4 lines
An "__ondisk" annotation, to me, would allow something like sparse to verify properly laid out on-disk structures, but would not affect the actual runtime code - I think that would be quite useful. However, this
change actually impacts the bytecode; it is a functional change.
Yup - this isn't "annotation"....

Ok. I'll redo the comment for the next version of the patch :)

...

Quoted 6 lines

I think you iterated my concerns quite well, Eric.
The thing I want to see for any sort of change like this is output off all the structures and their alignment before the change and their alignment after the change. On all supported arches. 'pahole' is the tool you used for that, wasn't it, Eric?

Ok, next one will include pahole output. (And yes, pahole is the tool Eric used.)

Quoted 6 lines

The only arch I would expect to see a change in the structures is on ARM; if there's anything other than that there there's something wrong. This is going to require a lot of validation to ensure that it is correct.....
Not to mention performance testing on ia64 given the added overhead in critical paths.....

Agreed on both accounts.

Josef 'Jeff' Sipek.

Signature · 2 lines

Intellectuals solve problems; geniuses prevent them
		- Albert Einstein

Timothy Shimmin

2008-03-17 23:35 UTC #14 lore raw

In reply to Eric Sandeen

Quoted 9 lines · Eric Sandeen

Quoted 2 lines · Josef 'Jeff' Sipek
Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)
Very. ;)
Tossing packed at all the ondisk stuctures bloats things badly on ia64.

Quoted 9 lines

cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis
That's +15606 lines.
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

Interesting.
So the problem there is that gcc is doing the wrong thing on some arches (the example being ia64, sparc64).

--Tim

Josef 'Jeff' Sipek

2008-03-17 23:43 UTC #15 lore raw

Quoted 12 lines · Timothy Shimmin

Quoted 11 lines · Eric Sandeen
Quoted 2 lines · Josef 'Jeff' Sipek
Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)
Very. ;)
Tossing packed at all the ondisk stuctures bloats things badly on ia64. cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis
That's +15606 lines.
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

Quoted 3 lines

Interesting.
So the problem there is that gcc is doing the wrong thing on some arches (the example being ia64, sparc64).

Actually, it's not doing the wrong thing...

__attribute__((packed)) means:

1) condense the members of the struct leaving NO padding bytes

2) do NOT assume the entire structure is aligned on any boundary

This means, that even if you have a member that'd be nicely aligned without the packed attribute (see below), the compiler will generate worst case alignment code.

struct foo {
	u64 a;
} __attribute__((packed));

You can put struct foo anywhere in memory, and the code accessing ->a will _always_ work.

Using __attribute((packed,aligned(4))), tells it that the structure as a whole will be aligned on a 4-byte boundary, but there should be no padding bytes inserted.

Josef 'Jeff' Sipek.

Signature · 1 line

Penguin : Linux version 2.6.23.1 on an i386 machine (6135.23 BogoMips).

Timothy Shimmin

2008-03-18 04:30 UTC #16 lore raw

Quoted 17 lines · Josef 'Jeff' Sipek

Quoted 12 lines · Timothy Shimmin
Quoted 11 lines · Eric Sandeen
Quoted 2 lines · Josef 'Jeff' Sipek
Josef 'Jeff' Sipek, wondering exactly how passionate one can get about structure member alignment :)
Very. ;)
Tossing packed at all the ondisk stuctures bloats things badly on ia64. cvs/linux-2.6-xfs> wc -l before.dis
166688 before.dis
cvs/linux-2.6-xfs> wc -l after.dis
182294 after.dis
That's +15606 lines.
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/
Quoted 3 lines
Interesting.
So the problem there is that gcc is doing the wrong thing on some arches (the example being ia64, sparc64).

Quoted 8 lines

Actually, it's not doing the wrong thing...
__attribute__((packed)) means:
1) condense the members of the struct leaving NO padding bytes
2) do NOT assume the entire structure is aligned on any boundary

Okay I only knew about (1) - cause that sounds more like "pack"ing to me. So you can't assume alignment for the start of the variable without aligned() if you use packed - Ok.

Thanks,
--Tim

Quoted 17 lines

This means, that even if you have a member that'd be nicely aligned without the packed attribute (see below), the compiler will generate worst case alignment code.
struct foo {
	u64 a;
} __attribute__((packed));
You can put struct foo anywhere in memory, and the code accessing ->a will _always_ work.
Using __attribute((packed,aligned(4))), tells it that the structure as a whole will be aligned on a 4-byte boundary, but there should be no padding bytes inserted.
Josef 'Jeff' Sipek.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Eric Sandeen

author

2008-03-15 04:45 UTC #18 lore raw

Quoted 7 lines · Chris Wedgwood

Quoted 3 lines · Eric Sandeen
I think that packing structures when they don't need to be can actually be harmful, efficiency-wise. I read a nice explanation of this.... which I can't find now.
objdump -d would show if that was the case...

it would show if that was the case for the particular compiler you test I guess.

-Eric

Eric Sandeen

author

2008-03-20 03:02 UTC #19 lore raw

In reply to Eric Sandeen

Here's the userspace side.

Jeff, I guess this means you have more work to do ;)

-Eric

xfsprogs/include/platform_defs.h.in +7 -0

Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
===================================================================
@@ -147,4 +147,11 @@ typedef unsigned long long __psunsigned_
 					| (minor&IRIX_DEV_MAXMIN)))
 #define IRIX_DEV_TO_KDEVT(dev)	makedev(IRIX_DEV_MAJOR(dev),IRIX_DEV_MINOR(dev))
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif	/* __XFS_PLATFORM_DEFS_H__ */

xfsprogs/include/xfs_dir2_sf.h +3 -3

Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
===================================================================
@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must

@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t;
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Barry Naujok

2008-05-05 07:35 UTC #20 lore raw

Quoted 1 line · Eric Sandeen

Here's the userspace side.

Dave, yes he did!

Quoted 55 lines

Jeff, I guess this means you have more work to do ;)

-Eric

Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
===================================================================

xfsprogs/include/platform_defs.h.in +7 -0

@@ -147,4 +147,11 @@ typedef unsigned long long __psunsigned_
 					| (minor&IRIX_DEV_MAXMIN)))
 #define  
 IRIX_DEV_TO_KDEVT(dev)	makedev(IRIX_DEV_MAJOR(dev),IRIX_DEV_MINOR(dev))
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif	/* __XFS_PLATFORM_DEFS_H__ */

Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
===================================================================

xfsprogs/include/xfs_dir2_sf.h +0 -0

@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really

xfs_dir2_data_off_t.
* Only need 16 bits, this is the byte offset into the single block
form.
*/

xfsprogs/include/xfs_dir2_sf.h +1 -1

-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;

/*
* The parent directory has a dedicated field, and the self-pointer must

xfsprogs/include/xfs_dir2_sf.h +2 -2

@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t;
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Eric Sandeen

author

2008-06-06 14:15 UTC #21 lore raw

In reply to Eric Sandeen

Quoted 2 lines · Eric Sandeen

Here's the userspace side.

Just a reminder after Dave's reminder. :)

-Eric

Quoted 54 lines

-Eric

Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
===================================================================

xfsprogs/include/platform_defs.h.in +7 -0

@@ -147,4 +147,11 @@ typedef unsigned long long __psunsigned_
 					| (minor&IRIX_DEV_MAXMIN)))
 #define IRIX_DEV_TO_KDEVT(dev)	makedev(IRIX_DEV_MAJOR(dev),IRIX_DEV_MINOR(dev))
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif	/* __XFS_PLATFORM_DEFS_H__ */

Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
===================================================================

xfsprogs/include/xfs_dir2_sf.h +3 -3

@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must

@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t;
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Eric Sandeen

author

2008-04-09 20:40 UTC #22 lore raw

In reply to Eric Sandeen

Quoted 7 lines · Eric Sandeen

This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

So, I'm wondering what the sgi guys think of this. I know Jeff wants an all-singing, all-dancing pack-and-align patch, and maybe when it's ready it could replace this, but I wonder if maybe now this could go in just, you know, to fix the bug? :)

Thanks,
-Eric

Quoted 105 lines

Old ABI ARM has interesting packing & padding; for example on ARM old abi:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_sf_off_t          offset;       /*     4     4 */
	__uint8_t                  name[1];      /*     8     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_inou_t            inumber;      /*    12     8 */

	/* size: 20, cachelines: 1 */
	/* sum members: 14, holes: 2, sum holes: 6 */
	/* last cacheline: 20 bytes */
};
but on x86:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */
	xfs_dir2_sf_off_t          offset;       /*     1     2 */
	__uint8_t                  name[1];      /*     3     1 */
	xfs_dir2_inou_t            inumber;      /*     4     8 */
	/* size: 12, cachelines: 1 */
	/* last cacheline: 12 bytes */
};
... this sort of discrepancy leads to problems.
I've verified this patch by comparing the on-disk structure layouts using pahole from the dwarves package, as well as running through a bit of xfsqa under qemu-arm, modified so that the check/repair phase after each test actually executes check/repair from the x86 host, on the filesystem populated by the arm emulator. Thus far it all looks good.
There are 2 other structures with extra padding at the end, but they don't seem to cause trouble. I suppose they could be packed as well: xfs_dir2_data_unused_t and xfs_dir2_sf_t.
Note that userspace needs a similar treatment, and any filesystems which were running with the previous rogue "fix" will now see corruption (either in the kernel, or during xfs_repair) with this fix properly in place; it may be worth teaching xfs_repair to identify and fix that specific issue.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
fs/xfs/linux-2.6/xfs_linux.h +7 -0
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif /* __XFS_LINUX__ */
Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================
fs/xfs/xfs_dir2_sf.h +3 -3
@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must
@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t; 
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Eric Sandeen

author

2008-04-23 00:58 UTC #23 lore raw

In reply to Eric Sandeen

Quoted 7 lines · Eric Sandeen

This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

ping again...

There is still the cache flushing issue, I guess, but I think the on-disk alignment is still worth fixing.

Oh, I'm sorry Jeff - I meant "fixing." :)

Thanks,
-Eric

Quoted 105 lines

Old ABI ARM has interesting packing & padding; for example on ARM old abi:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_sf_off_t          offset;       /*     4     4 */
	__uint8_t                  name[1];      /*     8     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_inou_t            inumber;      /*    12     8 */

	/* size: 20, cachelines: 1 */
	/* sum members: 14, holes: 2, sum holes: 6 */
	/* last cacheline: 20 bytes */
};
but on x86:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */
	xfs_dir2_sf_off_t          offset;       /*     1     2 */
	__uint8_t                  name[1];      /*     3     1 */
	xfs_dir2_inou_t            inumber;      /*     4     8 */
	/* size: 12, cachelines: 1 */
	/* last cacheline: 12 bytes */
};
... this sort of discrepancy leads to problems.
I've verified this patch by comparing the on-disk structure layouts using pahole from the dwarves package, as well as running through a bit of xfsqa under qemu-arm, modified so that the check/repair phase after each test actually executes check/repair from the x86 host, on the filesystem populated by the arm emulator. Thus far it all looks good.
There are 2 other structures with extra padding at the end, but they don't seem to cause trouble. I suppose they could be packed as well: xfs_dir2_data_unused_t and xfs_dir2_sf_t.
Note that userspace needs a similar treatment, and any filesystems which were running with the previous rogue "fix" will now see corruption (either in the kernel, or during xfs_repair) with this fix properly in place; it may be worth teaching xfs_repair to identify and fix that specific issue.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
fs/xfs/linux-2.6/xfs_linux.h +7 -0
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif /* __XFS_LINUX__ */
Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================
fs/xfs/xfs_dir2_sf.h +3 -3
@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must
@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t; 
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

Eric Sandeen

author

2008-05-02 20:55 UTC #24 lore raw

Quoted 10 lines · Eric Sandeen

Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
ping again...

ping #3...

Quoted 115 lines

There is still the cache flushing issue, I guess, but I think the on-disk alignment is still worth fixing.
Oh, I'm sorry Jeff - I meant "fixing." :)
Thanks,
-Eric
Quoted 105 lines
Old ABI ARM has interesting packing & padding; for example on ARM old abi:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_sf_off_t          offset;       /*     4     4 */
	__uint8_t                  name[1];      /*     8     1 */

	/* XXX 3 bytes hole, try to pack */

	xfs_dir2_inou_t            inumber;      /*    12     8 */

	/* size: 20, cachelines: 1 */
	/* sum members: 14, holes: 2, sum holes: 6 */
	/* last cacheline: 20 bytes */
};
but on x86:
struct xfs_dir2_sf_entry {
	__uint8_t                  namelen;      /*     0     1 */
	xfs_dir2_sf_off_t          offset;       /*     1     2 */
	__uint8_t                  name[1];      /*     3     1 */
	xfs_dir2_inou_t            inumber;      /*     4     8 */
	/* size: 12, cachelines: 1 */
	/* last cacheline: 12 bytes */
};
... this sort of discrepancy leads to problems.
I've verified this patch by comparing the on-disk structure layouts using pahole from the dwarves package, as well as running through a bit of xfsqa under qemu-arm, modified so that the check/repair phase after each test actually executes check/repair from the x86 host, on the filesystem populated by the arm emulator. Thus far it all looks good.
There are 2 other structures with extra padding at the end, but they don't seem to cause trouble. I suppose they could be packed as well: xfs_dir2_data_unused_t and xfs_dir2_sf_t.
Note that userspace needs a similar treatment, and any filesystems which were running with the previous rogue "fix" will now see corruption (either in the kernel, or during xfs_repair) with this fix properly in place; it may be worth teaching xfs_repair to identify and fix that specific issue.
Signed-off-by Eric Sandeen <[email protected]>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
fs/xfs/linux-2.6/xfs_linux.h +7 -0
@@ -300,4 +300,11 @@ static inline __uint64_t howmany_64(__ui
 	return x;
 }
 
+/* ARM old ABI has some weird alignment/padding */
+#if defined(__arm__) && !defined(__ARM_EABI__)
+#define __arch_pack __attribute__((packed))
+#else
+#define __arch_pack
+#endif
+
 #endif /* __XFS_LINUX__ */
Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================
fs/xfs/xfs_dir2_sf.h +3 -3
@@ -62,7 +62,7 @@ typedef union {
  * Normalized offset (in a data block) of the entry, really xfs_dir2_data_off_t.
  * Only need 16 bits, this is the byte offset into the single block form.
  */
-typedef struct { __uint8_t i[2]; } xfs_dir2_sf_off_t;
+typedef struct { __uint8_t i[2]; } __arch_pack xfs_dir2_sf_off_t;
 
 /*
  * The parent directory has a dedicated field, and the self-pointer must
@@ -76,14 +76,14 @@ typedef struct xfs_dir2_sf_hdr {
 	__uint8_t		count;		/* count of entries */
 	__uint8_t		i8count;	/* count of 8-byte inode #s */
 	xfs_dir2_inou_t		parent;		/* parent dir inode number */
-} xfs_dir2_sf_hdr_t;
+} __arch_pack xfs_dir2_sf_hdr_t;
 
 typedef struct xfs_dir2_sf_entry {
 	__uint8_t		namelen;	/* actual name length */
 	xfs_dir2_sf_off_t	offset;		/* saved offset */
 	__uint8_t		name[1];	/* name, variable size */
 	xfs_dir2_inou_t		inumber;	/* inode number, var. offset */
-} xfs_dir2_sf_entry_t;
+} __arch_pack xfs_dir2_sf_entry_t; 
 
 typedef struct xfs_dir2_sf {
 	xfs_dir2_sf_hdr_t	hdr;		/* shortform header */

David Chinner

2008-05-05 07:08 UTC #25 lore raw

Quoted 10 lines · Eric Sandeen

Quoted 9 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

Quoted 3 lines

Quoted 1 line
ping again...
ping #3...

<sigh>

Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.

Did you ever send an equivalent userspace patch?

Cheers,

Dave.

Signature · 3 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

Eric Sandeen

author

2008-05-05 13:07 UTC #26 lore raw

Quoted 13 lines · David Chinner

Quoted 11 lines · Eric Sandeen
Quoted 9 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
ping again...
ping #3...

Quoted 7 lines

<sigh>
Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.
Did you ever send an equivalent userspace patch?

I did ... I can resend too.

-Eric

Timothy Shimmin

2008-05-06 04:21 UTC #27 lore raw

In reply to David Chinner

Quoted 10 lines · David Chinner

Quoted 9 lines · Eric Sandeen
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

Quoted 9 lines

Quoted 2 lines
Quoted 1 line
ping again...
ping #3...
<sigh>
Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.

As it only defines __arch_pack for __arm__,
I literally can't see how on earth it won't pass for ia64 and x86-64, though I realise (I guess) we need to test to be sure :)

So Eric tested this on qemu-arm with success.
And there was a little debate over whether ARM-EABI would work currently in XFS,
with Luca Olivetti saying in one kernel he has success and in another he doesn't. And Andre Draszik saying that for ARM-EABI it wouldn't work.
That aside, Eric has tried out on ARM without EABI (old ABI) and has had success, so it is at least useful for this case.
I don't see us doing any arm testing for this ourselves :)

--Tim

Eric Sandeen

author

2008-05-06 13:43 UTC #28 lore raw

Quoted 23 lines · Timothy Shimmin

Quoted 10 lines · David Chinner
Quoted 9 lines · Eric Sandeen
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
ping again...
ping #3...
<sigh>
Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.
As it only defines __arch_pack for __arm__,
I literally can't see how on earth it won't pass for ia64 and x86-64, though I realise (I guess) we need to test to be sure :)

Quoted 6 lines

So Eric tested this on qemu-arm with success.
And there was a little debate over whether ARM-EABI would work currently in XFS,
with Luca Olivetti saying in one kernel he has success and in another he doesn't. And Andre Draszik saying that for ARM-EABI it wouldn't work.

The patch should only affect behavior on *old* abi:

+#if defined(__arm__) && !defined(__ARM_EABI__)

it is the only one with the unique alignment that matters here.

There *is* still another issue on some arm chips related to processor cache flushing; I didn't see the problem in qemu because it the emulator does not have this behavior.

But, it's a separate issue from the structure alignment this patch addresses.

One thing at a time. :)

Thanks,

-Eric

Quoted 6 lines

That aside, Eric has tried out on ARM without EABI (old ABI) and has had success, so it is at least useful for this case.
I don't see us doing any arm testing for this ourselves :)
--Tim

Eric Sandeen

author

2008-06-05 05:37 UTC #29 lore raw

Quoted 12 lines · Eric Sandeen

Quoted 11 lines · Timothy Shimmin
Quoted 10 lines · David Chinner
Quoted 9 lines · Eric Sandeen
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
ping again...
ping #3...
<sigh>

Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.

is it EVER going to get checked in?

-Eric

Quoted 42 lines

Quoted 8 lines
Quoted 4 lines
Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.
As it only defines __arch_pack for __arm__,
I literally can't see how on earth it won't pass for ia64 and x86-64, though I realise (I guess) we need to test to be sure :)
Quoted 6 lines
So Eric tested this on qemu-arm with success.
And there was a little debate over whether ARM-EABI would work currently in XFS,
with Luca Olivetti saying in one kernel he has success and in another he doesn't. And Andre Draszik saying that for ARM-EABI it wouldn't work.
The patch should only affect behavior on *old* abi:
+#if defined(__arm__) && !defined(__ARM_EABI__)
it is the only one with the unique alignment that matters here.
There *is* still another issue on some arm chips related to processor cache flushing; I didn't see the problem in qemu because it the emulator does not have this behavior.
But, it's a separate issue from the structure alignment this patch addresses.
One thing at a time. :)
Thanks,
-Eric
Quoted 6 lines
That aside, Eric has tried out on ARM without EABI (old ABI) and has had success, so it is at least useful for this case.
I don't see us doing any arm testing for this ourselves :)
--Tim

Mark Goodwin

2008-06-05 05:45 UTC #30 lore raw

Quoted 13 lines · Eric Sandeen

Quoted 12 lines · Eric Sandeen
Quoted 11 lines · Timothy Shimmin
Quoted 10 lines · David Chinner
Quoted 9 lines · Eric Sandeen
Quoted 8 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
Quoted 1 line
ping again...
ping #3...
<sigh>
Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG. is it EVER going to get checked in?

Please take it in Tim, that nasty CORRUPTION word caught my attention :)

Cheers

Quoted 44 lines

-Eric
Quoted 39 lines
Quoted 8 lines
Quoted 4 lines
Looks like if I don't pick it up then nobody is going to answer. I'll run it through my ia64 and x86_64 test boxes and if it's ok then I'll commit it.
As it only defines __arch_pack for __arm__,
I literally can't see how on earth it won't pass for ia64 and x86-64, though I realise (I guess) we need to test to be sure :)
Quoted 5 lines
So Eric tested this on qemu-arm with success.
And there was a little debate over whether ARM-EABI would work
currently in XFS, with Luca Olivetti saying in one kernel he has success and in another he doesn't. And Andre Draszik saying that for ARM-EABI it wouldn't
work.
The patch should only affect behavior on *old* abi:
+#if defined(__arm__) && !defined(__ARM_EABI__)
it is the only one with the unique alignment that matters here.
There *is* still another issue on some arm chips related to processor cache flushing; I didn't see the problem in qemu because it the emulator does not have this behavior.
But, it's a separate issue from the structure alignment this patch addresses.
One thing at a time. :)
Thanks,
-Eric
Quoted 6 lines
That aside, Eric has tried out on ARM without EABI (old ABI) and has had success, so it is at least useful for this case.
I don't see us doing any arm testing for this ourselves :)
--Tim

Signature · 4 lines

 Mark Goodwin                                  [email protected]
 Engineering Manager for XFS and PCP    Phone: +61-3-99631937
 SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Eric Sandeen

author

2008-06-05 05:48 UTC #31 lore raw

Quoted 16 lines · Mark Goodwin

Quoted 14 lines · Eric Sandeen
Quoted 13 lines · Eric Sandeen
Quoted 12 lines · Timothy Shimmin
Quoted 11 lines · David Chinner
Quoted 10 lines · Eric Sandeen
Quoted 9 lines · Eric Sandeen
Quoted 7 lines · Eric Sandeen
This should fix the longstanding issues with xfs and old ABI arm boxes, which lead to various asserts and xfs shutdowns, and for which an (incorrect) patch has been floating around for years. (Said patch made ARM internally consistent, but altered the normal xfs on-disk format such that it looked corrupted on other architectures):
http://lists.arm.linux.org.uk/lurker/message/20040311.002034.5ecf21a2.html
ping again...

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
ping #3...
<sigh>
Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG. is it EVER going to get checked in?
Please take it in Tim, that nasty CORRUPTION word caught my attention :)
Cheers

heh, thanks Mark. Even though it's only CORRUPTION ON ARM I guess the shouting worked. :)

Seriously though if you guys have any problems with it please let me know but I don't want it to just get dropped.

-Eric

Mark Goodwin

2008-06-05 06:02 UTC #32 lore raw

Quoted 3 lines · Eric Sandeen

heh, thanks Mark. Even though it's only CORRUPTION ON ARM I guess the shouting worked. :)

Well, we're dealing with apparent insidious corruption after failed btree AG allocations (which aren't supposed to fail). This seems to stem from the fixes for the ENOSPC deadlock bugs (two years ago, see below). you and Nathan probably remember this ..?

Quoted 24 lines · Timothy Shimmin

Okay some history in xfs-dev archives on this bug... plenty of reading here :)
Reverse chronological order with initial report from guy at NEC:
May 2006
Review thread b/w Nathan and Ying Ping:
http://ils.corp.sgi.com/Archives/xfs-dev/200605/msg00128.html
April 2006
Thread - dgc, Ying, nathan
http://ils.corp.sgi.com/Archives/xfs-dev/200604/msg00005.html
March 2006 - 2 threads
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00002.html
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00140.html
Feb 2006
http://ils.corp.sgi.com/Archives/xfs-dev/200602/msg00062.html
Initial report and suggested test case by Asano Masahiro - NEC (fwd'ed by Eric).
Plenty of commentary by Asano.
Thread - including comments by Steve Lord:
http://thread.gmane.org/gmane.comp.file-systems.xfs.general/13268

Signature · 4 lines

 Mark Goodwin                                  [email protected]
 Engineering Manager for XFS and PCP    Phone: +61-3-99631937
 SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Mark Goodwin

2008-06-05 06:04 UTC #33 lore raw

Just realized you can't read the ils threads. But you can read this one:
http://thread.gmane.org/gmane.comp.file-systems.xfs.general/13268

-- Mark

Quoted 39 lines · Mark Goodwin

Quoted 3 lines · Eric Sandeen
heh, thanks Mark. Even though it's only CORRUPTION ON ARM I guess the shouting worked. :)
Well, we're dealing with apparent insidious corruption after failed btree AG allocations (which aren't supposed to fail). This seems to stem from the fixes for the ENOSPC deadlock bugs (two years ago, see below). you and Nathan probably remember this ..?
Quoted 24 lines · Timothy Shimmin
Okay some history in xfs-dev archives on this bug... plenty of reading here :)
Reverse chronological order with initial report from guy at NEC:
May 2006
Review thread b/w Nathan and Ying Ping:
http://ils.corp.sgi.com/Archives/xfs-dev/200605/msg00128.html
April 2006
Thread - dgc, Ying, nathan
http://ils.corp.sgi.com/Archives/xfs-dev/200604/msg00005.html
March 2006 - 2 threads
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00002.html
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00140.html
Feb 2006
http://ils.corp.sgi.com/Archives/xfs-dev/200602/msg00062.html
Initial report and suggested test case by Asano Masahiro - NEC (fwd'ed by Eric).
Plenty of commentary by Asano.
Thread - including comments by Steve Lord:
http://thread.gmane.org/gmane.comp.file-systems.xfs.general/13268

Signature · 4 lines

 Mark Goodwin                                  [email protected]
 Engineering Manager for XFS and PCP    Phone: +61-3-99631937
 SGI Australian Software Group           Cell: +61-4-18969583
-------------------------------------------------------------

Eric Sandeen

author

2008-06-05 06:05 UTC #34 lore raw

In reply to Mark Goodwin

Quoted 10 lines · Mark Goodwin

Quoted 2 lines · Eric Sandeen
heh, thanks Mark. Even though it's only CORRUPTION ON ARM I guess the shouting worked. :)
Well, we're dealing with apparent insidious corruption after failed btree AG allocations (which aren't supposed to fail). This seems to stem from the fixes for the ENOSPC deadlock bugs (two years ago, see below). you and Nathan probably remember this ..?

nah it's easier to hit than that. Just run xfs-qa on arm with old abi and you'll hit it plenty quickly. there are other calculations around that assume no padding.

Even if it doesn't look corrupted on arm, the filesystem isn't portable.

-Eric

Quoted 26 lines

Quoted 24 lines · Timothy Shimmin
Okay some history in xfs-dev archives on this bug... plenty of reading here :)
Reverse chronological order with initial report from guy at NEC:
May 2006
Review thread b/w Nathan and Ying Ping:
http://ils.corp.sgi.com/Archives/xfs-dev/200605/msg00128.html
April 2006
Thread - dgc, Ying, nathan
http://ils.corp.sgi.com/Archives/xfs-dev/200604/msg00005.html
March 2006 - 2 threads
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00002.html
http://ils.corp.sgi.com/Archives/xfs-dev/200603/msg00140.html
Feb 2006
http://ils.corp.sgi.com/Archives/xfs-dev/200602/msg00062.html
Initial report and suggested test case by Asano Masahiro - NEC (fwd'ed by Eric).
Plenty of commentary by Asano.
Thread - including comments by Steve Lord:
http://thread.gmane.org/gmane.comp.file-systems.xfs.general/13268

Chris Wedgwood

2008-06-05 05:48 UTC #35 lore raw

In reply to Eric Sandeen

Quoted 1 line · Eric Sandeen

Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.

[...]

Quoted 1 line

Quoted 1 line
Quoted 1 line
As it only defines __arch_pack for __arm__,

__arch_pack is a horrible name and not very intuitive, what's wrong with __on_disk or something else?

Eric Sandeen

author

2008-06-05 05:51 UTC #36 lore raw

Quoted 11 lines · Chris Wedgwood

Quoted 1 line · Eric Sandeen
Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
As it only defines __arch_pack for __arm__,
__arch_pack is a horrible name and not very intuitive, what's wrong with __on_disk or something else?

because __on_disk implies that it's on disk.

__arch_pack means that it's packed on some arches.

Not the same thing.

If anyone wants to change the name to __purple_ponies or whatever that's fine, but the intent is to pack these 3(?) and only these 3 structs on this arch and only this arch, at least for now.

'til Jeff gets his all-singing all-dancing no-regression gcc annotation in place anyway.

-Eric

Eric Sandeen

author

2008-06-05 06:33 UTC #37 lore raw

In reply to Chris Wedgwood

Quoted 12 lines · Chris Wedgwood

Quoted 1 line · Eric Sandeen
Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
[...]
Quoted 1 line
Quoted 1 line
Quoted 1 line
As it only defines __arch_pack for __arm__,
__arch_pack is a horrible name and not very intuitive, what's wrong with __on_disk or something else?

seriously, if you don't like the name or the style of the fix that's fine, we can fix that up, but I went to enough trouble to track down the issue and test the fix it seems worth actually... fixing it.

If you want to __on_disk annotate everything and only pack it on arm OABI that might be less hacky. cw almost convinced me of this.

I was just going for "least invasive" here.

-Eric