* [PATCH] fix dir2 shortform structures on ARM old ABI
@ 2008-03-15 3:24 Eric Sandeen
2008-03-15 4:17 ` Josef 'Jeff' Sipek
` (3 more replies)
0 siblings, 4 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-15 3:24 UTC (permalink / raw)
To: xfs-oss; +Cc: patches
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 <sandeen@sandeen.net>
---
Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
===================================================================
--- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
+++ 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__ */
Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
===================================================================
--- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
+++ 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 */
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 3:24 [PATCH] fix dir2 shortform structures on ARM old ABI Eric Sandeen
@ 2008-03-15 4:17 ` Josef 'Jeff' Sipek
2008-03-15 4:23 ` Eric Sandeen
2008-03-20 3:02 ` Eric Sandeen
` (2 subsequent siblings)
3 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-15 4:17 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss, patches
On Fri, Mar 14, 2008 at 10:24:49PM -0500, Eric Sandeen wrote:
> 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
...
> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>
> ---
>
> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
> +++ 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
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?)
> +
> #endif /* __XFS_LINUX__ */
> Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
> +++ 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 */
A very simple patch! I like it (minus the condition vs. unconditional
packing - see above).
Josef 'Jeff' Sipek.
--
Penguin : Linux version 2.6.23.1 on an i386 machine (6135.23 BogoMips).
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 4:17 ` Josef 'Jeff' Sipek
@ 2008-03-15 4:23 ` Eric Sandeen
2008-03-15 4:27 ` Josef 'Jeff' Sipek
[not found] ` <20080315043622.GA11547@puku.stupidest.org>
0 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-15 4:23 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: xfs-oss
Josef 'Jeff' Sipek wrote:
> On Fri, Mar 14, 2008 at 10:24:49PM -0500, Eric Sandeen wrote:
>> 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
> ...
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
>>
>> ---
>>
>> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
>> ===================================================================
>> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
>> +++ 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
>
> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 4:23 ` Eric Sandeen
@ 2008-03-15 4:27 ` Josef 'Jeff' Sipek
2008-03-15 4:33 ` Eric Sandeen
[not found] ` <20080315043622.GA11547@puku.stupidest.org>
1 sibling, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-15 4:27 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Fri, Mar 14, 2008 at 11:23:43PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
> > On Fri, Mar 14, 2008 at 10:24:49PM -0500, Eric Sandeen wrote:
> >> 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
> > ...
> >> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> >>
> >> ---
> >>
> >> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
> >> ===================================================================
> >> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
> >> +++ 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
> >
> > 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.
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.
--
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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 4:27 ` Josef 'Jeff' Sipek
@ 2008-03-15 4:33 ` Eric Sandeen
2008-03-15 4:51 ` Josef 'Jeff' Sipek
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-03-15 4:33 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: xfs-oss
Josef 'Jeff' Sipek wrote:
> On Fri, Mar 14, 2008 at 11:23:43PM -0500, Eric Sandeen wrote:
>> Josef 'Jeff' Sipek wrote:
>>> 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.
>
> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
[not found] ` <20080315043622.GA11547@puku.stupidest.org>
@ 2008-03-15 4:45 ` Eric Sandeen
0 siblings, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-15 4:45 UTC (permalink / raw)
To: Chris Wedgwood; +Cc: Josef 'Jeff' Sipek, xfs-oss
Chris Wedgwood wrote:
> On Fri, Mar 14, 2008 at 11:23:43PM -0500, Eric Sandeen wrote:
>
>> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 4:33 ` Eric Sandeen
@ 2008-03-15 4:51 ` Josef 'Jeff' Sipek
2008-03-17 18:32 ` Eric Sandeen
0 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-15 4:51 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Fri, Mar 14, 2008 at 11:33:39PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
> > On Fri, Mar 14, 2008 at 11:23:43PM -0500, Eric Sandeen wrote:
> >> Josef 'Jeff' Sipek wrote:
>
> >>> 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.
> >
> > 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.
> 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 :)
--
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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 4:51 ` Josef 'Jeff' Sipek
@ 2008-03-17 18:32 ` Eric Sandeen
2008-03-17 19:53 ` Josef 'Jeff' Sipek
2008-03-17 23:35 ` [PATCH] fix dir2 shortform structures on ARM old ABI Timothy Shimmin
0 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-17 18:32 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: xfs-oss
Josef 'Jeff' Sipek wrote:
> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 18:32 ` Eric Sandeen
@ 2008-03-17 19:53 ` Josef 'Jeff' Sipek
2008-03-17 20:04 ` Eric Sandeen
2008-03-17 23:35 ` [PATCH] fix dir2 shortform structures on ARM old ABI Timothy Shimmin
1 sibling, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-17 19:53 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Mon, Mar 17, 2008 at 01:32:16PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
>
> > 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.
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.
--
A CRAY is the only computer that runs an endless loop in just 4 hours...
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 19:53 ` Josef 'Jeff' Sipek
@ 2008-03-17 20:04 ` Eric Sandeen
2008-03-17 20:28 ` Josef 'Jeff' Sipek
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-03-17 20:04 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: xfs-oss
Josef 'Jeff' Sipek wrote:
> On Mon, Mar 17, 2008 at 01:32:16PM -0500, Eric Sandeen wrote:
>> Josef 'Jeff' Sipek wrote:
>>
>>> 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.
>
> 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.
> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 20:04 ` Eric Sandeen
@ 2008-03-17 20:28 ` Josef 'Jeff' Sipek
2008-03-18 0:39 ` [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk Josef 'Jeff' Sipek
0 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-17 20:28 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Mon, Mar 17, 2008 at 03:04:13PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
...
> 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.
> > 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.
--
The box said "Windows XP or better required". So I installed Linux.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 18:32 ` Eric Sandeen
2008-03-17 19:53 ` Josef 'Jeff' Sipek
@ 2008-03-17 23:35 ` Timothy Shimmin
2008-03-17 23:42 ` Josef 'Jeff' Sipek
1 sibling, 1 reply; 51+ messages in thread
From: Timothy Shimmin @ 2008-03-17 23:35 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Josef 'Jeff' Sipek, xfs-oss
Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
>
>> 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/
>
Interesting.
So the problem there is that gcc is doing the wrong thing
on some arches (the example being ia64, sparc64).
--Tim
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 23:35 ` [PATCH] fix dir2 shortform structures on ARM old ABI Timothy Shimmin
@ 2008-03-17 23:42 ` Josef 'Jeff' Sipek
2008-03-18 4:31 ` Timothy Shimmin
0 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-17 23:42 UTC (permalink / raw)
To: Timothy Shimmin; +Cc: Eric Sandeen, xfs-oss
On Tue, Mar 18, 2008 at 10:35:32AM +1100, Timothy Shimmin wrote:
> Eric Sandeen wrote:
>> Josef 'Jeff' Sipek wrote:
>>> 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/
> 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.
--
Penguin : Linux version 2.6.23.1 on an i386 machine (6135.23 BogoMips).
^ permalink raw reply [flat|nested] 51+ messages in thread
* [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk
2008-03-17 20:28 ` Josef 'Jeff' Sipek
@ 2008-03-18 0:39 ` Josef 'Jeff' Sipek
2008-03-18 3:34 ` Eric Sandeen
0 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-18 0:39 UTC (permalink / raw)
To: sandeen, xfs, tes, dgc; +Cc: Josef 'Jeff' Sipek
Currently, the annotation just forces the structures to be packed, and
4-byte aligned.
Signed-off-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
---
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(-)
diff --git a/fs/xfs/linux-2.6/xfs_linux.h b/fs/xfs/linux-2.6/xfs_linux.h
index 284460f..f06199c 100644
--- a/fs/xfs/linux-2.6/xfs_linux.h
+++ b/fs/xfs/linux-2.6/xfs_linux.h
@@ -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 */
diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
index 61b292a..20f3291 100644
--- a/fs/xfs/xfs_ag.h
+++ b/fs/xfs/xfs_ag.h
@@ -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
diff --git a/fs/xfs/xfs_alloc_btree.h b/fs/xfs/xfs_alloc_btree.h
index 5bd1a2c..f7c5bba 100644
--- a/fs/xfs/xfs_alloc_btree.h
+++ b/fs/xfs/xfs_alloc_btree.h
@@ -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 */
diff --git a/fs/xfs/xfs_attr_leaf.h b/fs/xfs/xfs_attr_leaf.h
index 040f732..792d2a9 100644
--- a/fs/xfs/xfs_attr_leaf.h
+++ b/fs/xfs/xfs_attr_leaf.h
@@ -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.
diff --git a/fs/xfs/xfs_attr_sf.h b/fs/xfs/xfs_attr_sf.h
index f67f917..a13afb7 100644
--- a/fs/xfs/xfs_attr_sf.h
+++ b/fs/xfs/xfs_attr_sf.h
@@ -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;
diff --git a/fs/xfs/xfs_bmap_btree.h b/fs/xfs/xfs_bmap_btree.h
index cd0d4b4..3c749c8 100644
--- a/fs/xfs/xfs_bmap_btree.h
+++ b/fs/xfs/xfs_bmap_btree.h
@@ -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;
diff --git a/fs/xfs/xfs_btree.h b/fs/xfs/xfs_btree.h
index 7440b78..40ac5b8 100644
--- a/fs/xfs/xfs_btree.h
+++ b/fs/xfs/xfs_btree.h
@@ -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.
diff --git a/fs/xfs/xfs_da_btree.h b/fs/xfs/xfs_da_btree.h
index 7facf86..36901d7 100644
--- a/fs/xfs/xfs_da_btree.h
+++ b/fs/xfs/xfs_da_btree.h
@@ -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;
diff --git a/fs/xfs/xfs_dinode.h b/fs/xfs/xfs_dinode.h
index c9065ea..9a24755 100644
--- a/fs/xfs/xfs_dinode.h
+++ b/fs/xfs/xfs_dinode.h
@@ -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.
diff --git a/fs/xfs/xfs_dir2_block.h b/fs/xfs/xfs_dir2_block.h
index 10e6896..a85f98d 100644
--- a/fs/xfs/xfs_dir2_block.h
+++ b/fs/xfs/xfs_dir2_block.h
@@ -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)
diff --git a/fs/xfs/xfs_dir2_data.h b/fs/xfs/xfs_dir2_data.h
index b816e02..e7ae1db 100644
--- a/fs/xfs/xfs_dir2_data.h
+++ b/fs/xfs/xfs_dir2_data.h
@@ -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.
diff --git a/fs/xfs/xfs_dir2_leaf.h b/fs/xfs/xfs_dir2_leaf.h
index 6c9539f..01a6091 100644
--- a/fs/xfs/xfs_dir2_leaf.h
+++ b/fs/xfs/xfs_dir2_leaf.h
@@ -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.
diff --git a/fs/xfs/xfs_dir2_node.h b/fs/xfs/xfs_dir2_node.h
index dde72db..78ab236 100644
--- a/fs/xfs/xfs_dir2_node.h
+++ b/fs/xfs/xfs_dir2_node.h
@@ -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)) / \
diff --git a/fs/xfs/xfs_dir2_sf.h b/fs/xfs/xfs_dir2_sf.h
index 005629d..5229bf2 100644
--- a/fs/xfs/xfs_dir2_sf.h
+++ b/fs/xfs/xfs_dir2_sf.h
@@ -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)
{
diff --git a/fs/xfs/xfs_ialloc_btree.h b/fs/xfs/xfs_ialloc_btree.h
index 8efc4a5..036cdd0 100644
--- a/fs/xfs/xfs_ialloc_btree.h
+++ b/fs/xfs/xfs_ialloc_btree.h
@@ -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;
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index 01c63db..81c5a2d 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -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__
/*
diff --git a/fs/xfs/xfs_quota.h b/fs/xfs/xfs_quota.h
index 12c4ec7..f5b9c30 100644
--- a/fs/xfs/xfs_quota.h
+++ b/fs/xfs/xfs_quota.h
@@ -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.
diff --git a/fs/xfs/xfs_sb.h b/fs/xfs/xfs_sb.h
index b1a83f8..beee35e 100644
--- a/fs/xfs/xfs_sb.h
+++ b/fs/xfs/xfs_sb.h
@@ -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.
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 0804207..2fbe465 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -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 */
--
1.5.4.rc2.85.g9de45-dirty
^ permalink raw reply related [flat|nested] 51+ messages in thread
* Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk
2008-03-18 0:39 ` [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk Josef 'Jeff' Sipek
@ 2008-03-18 3:34 ` Eric Sandeen
2008-03-18 4:09 ` David Chinner
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-03-18 3:34 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: xfs, tes, dgc
Josef 'Jeff' Sipek wrote:
> 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
> Signed-off-by: Josef 'Jeff' Sipek <jeffpc@josefsipek.net>
>
> ---
> 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.
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk
2008-03-18 3:34 ` Eric Sandeen
@ 2008-03-18 4:09 ` David Chinner
2008-03-18 5:28 ` Josef 'Jeff' Sipek
0 siblings, 1 reply; 51+ messages in thread
From: David Chinner @ 2008-03-18 4:09 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Josef 'Jeff' Sipek, xfs, tes, dgc
On Mon, Mar 17, 2008 at 10:34:57PM -0500, Eric Sandeen wrote:
> Josef 'Jeff' Sipek wrote:
> > 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"....
> 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. :)
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.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-17 23:42 ` Josef 'Jeff' Sipek
@ 2008-03-18 4:31 ` Timothy Shimmin
0 siblings, 0 replies; 51+ messages in thread
From: Timothy Shimmin @ 2008-03-18 4:31 UTC (permalink / raw)
To: Josef 'Jeff' Sipek; +Cc: Eric Sandeen, xfs-oss
Josef 'Jeff' Sipek wrote:
> On Tue, Mar 18, 2008 at 10:35:32AM +1100, Timothy Shimmin wrote:
>> Eric Sandeen wrote:
>>> Josef 'Jeff' Sipek wrote:
>>>> 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/
>> 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
>
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
> 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.
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk
2008-03-18 4:09 ` David Chinner
@ 2008-03-18 5:28 ` Josef 'Jeff' Sipek
0 siblings, 0 replies; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-18 5:28 UTC (permalink / raw)
To: David Chinner; +Cc: Eric Sandeen, xfs, tes
On Tue, Mar 18, 2008 at 03:09:03PM +1100, David Chinner wrote:
> On Mon, Mar 17, 2008 at 10:34:57PM -0500, Eric Sandeen wrote:
> > Josef 'Jeff' Sipek wrote:
> > > 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"....
Ok. I'll redo the comment for the next version of the patch :)
...
> 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.)
> 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.
--
Intellectuals solve problems; geniuses prevent them
- Albert Einstein
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
@ 2008-03-18 23:31 Andre Draszik
2008-03-19 3:18 ` Eric Sandeen
0 siblings, 1 reply; 51+ messages in thread
From: Andre Draszik @ 2008-03-18 23:31 UTC (permalink / raw)
To: xfs
Hi,
(I just subscribed, so I can't reply correctly :-(
In fact,, the last two evenings I spent making XFS work on arm eabi,
where things are much better than with the old API, but still XFS
won't work out of the box.
So, Eric, if you go for the #if defined(__arm__) &&
!defined(__ARM_EABI__) approach, arm eabi will still be broken.
EABI basically behaves like other 'normal' arches/abis, but sometimes
structures get padded to have a size of a multiple of 8, i.e. padding
is added at the end of the struct, which as far as I can see for now
affects 5 structs: xfs_dir2_data_entry_t, xfs_dinode_t, xfs_sb_t,
xfs_dsb_t, and xfs_log_item_t
I must say, I like Jeff's approach of explicitly telling gcc about
alignment much better :-) It makes it a) much easier to find structs
that are in fact representations of on-disk data and thus might need
tweaking, and b) as somebody already said you fix such problems once
and forever.
E.g. for me as an absolute outsider, it was quite time consuming
finding out which structs are actually on-disk.
That said, Jeff, you mentioned that your changes don't work yet
completely - could this be because (at least from the comments) struct
xfs_sb needs to match struct xfs_dsb and you only change xfs_dsb?
Cheers,
Andre'
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
@ 2008-03-18 23:49 Andre Draszik
2008-03-19 0:23 ` Josef 'Jeff' Sipek
0 siblings, 1 reply; 51+ messages in thread
From: Andre Draszik @ 2008-03-18 23:49 UTC (permalink / raw)
To: xfs
Hi,
(I just subscribed, so I can't reply correctly :-(
In fact,, the last two evenings I spent making XFS work on arm eabi,
where things are much better than with the old API, but still XFS
won't work out of the box.
So, Eric, if you go for the #if defined(__arm__) &&
!defined(__ARM_EABI__) approach, arm eabi will still be broken.
EABI basically behaves like other 'normal' arches/abis, but sometimes
structures get padded to have a size of a multiple of 8, i.e. padding
is added at the end of the struct, which as far as I can see for now
affects 5 structs: xfs_dir2_data_entry_t, xfs_dinode_t, xfs_sb_t,
xfs_dsb_t, and xlog_rec_header_t
I must say, I like Jeff's approach of explicitly telling gcc about
alignment much better :-) It makes it a) much easier to find structs
that are in fact representations of on-disk data and thus might need
tweaking, and b) as somebody already said you fix such problems once
and forever.
E.g. for me as an absolute outsider, it was quite time consuming
finding out which structs are actually on-disk.
That said, Jeff, you mentioned that your changes don't work yet
completely - could this be because (at least from the comments) struct
xfs_sb needs to match struct xfs_dsb and you only change xfs_dsb?
Cheers,
Andre'
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-18 23:49 Andre Draszik
@ 2008-03-19 0:23 ` Josef 'Jeff' Sipek
2008-03-19 11:21 ` Luca Olivetti
0 siblings, 1 reply; 51+ messages in thread
From: Josef 'Jeff' Sipek @ 2008-03-19 0:23 UTC (permalink / raw)
To: Andre Draszik; +Cc: xfs
On Tue, Mar 18, 2008 at 11:49:11PM +0000, Andre Draszik wrote:
> Hi,
>
> (I just subscribed, so I can't reply correctly :-(
>
> In fact,, the last two evenings I spent making XFS work on arm eabi,
> where things are much better than with the old API, but still XFS
> won't work out of the box.
>
> So, Eric, if you go for the #if defined(__arm__) &&
> !defined(__ARM_EABI__) approach, arm eabi will still be broken.
Ouch.
> EABI basically behaves like other 'normal' arches/abis, but sometimes
> structures get padded to have a size of a multiple of 8, i.e. padding
> is added at the end of the struct, which as far as I can see for now
> affects 5 structs: xfs_dir2_data_entry_t, xfs_dinode_t, xfs_sb_t,
> xfs_dsb_t, and xlog_rec_header_t
>
> I must say, I like Jeff's approach of explicitly telling gcc about
> alignment much better :-) It makes it a) much easier to find structs
> that are in fact representations of on-disk data and thus might need
> tweaking, and b) as somebody already said you fix such problems once
> and forever.
Hopefully, there won't be any need for additional tweaking.
> E.g. for me as an absolute outsider, it was quite time consuming
> finding out which structs are actually on-disk.
Hey, I feel your pain. I just grepped the entire source tree for 'struct'
and went through them one by one.
> That said, Jeff, you mentioned that your changes don't work yet
> completely - could this be because (at least from the comments) struct
> xfs_sb needs to match struct xfs_dsb and you only change xfs_dsb?
As far as I know the patch I sent later last night does work. BUT, and I
mean *BUT*, I did not test it on anything other than x86. And even then, I
wouldn't trust it with my data just yet :) It is very possible that some
arches are still broken - it was a "request for comment", not a "please
apply".
I talked about it a bit in the IRC channel, and the SGI folks want (1) proof
that adding these attributes does not create any regressions on any of the
supported architectures (and fixes the one that's currently broken - arm),
and (2) need to make sure that the ~700 instructions (~0.3% increase) that
get added to ia64 do not cause a regression in performance. Both points are
valid, and it'll all happen sometime (hopefully) soon.
Thanks for the heads up wrt ARM EABI.
Josef 'Jeff' Sipek.
--
You measure democracy by the freedom it gives its dissidents, not the
freedom it gives its assimilated conformists.
- Abbie Hoffman
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-18 23:31 Andre Draszik
@ 2008-03-19 3:18 ` Eric Sandeen
2008-03-19 3:40 ` Eric Sandeen
2008-03-19 5:11 ` Eric Sandeen
0 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 3:18 UTC (permalink / raw)
To: Andre Draszik; +Cc: xfs
[-- Attachment #1: Type: text/plain, Size: 2985 bytes --]
Andre Draszik wrote:
> Hi,
>
> (I just subscribed, so I can't reply correctly :-(
>
> In fact,, the last two evenings I spent making XFS work on arm eabi,
> where things are much better than with the old API, but still XFS
> won't work out of the box.
How so?
> So, Eric, if you go for the #if defined(__arm__) &&
> !defined(__ARM_EABI__) approach, arm eabi will still be broken.
Details, please.
> EABI basically behaves like other 'normal' arches/abis, but sometimes
> structures get padded to have a size of a multiple of 8, i.e. padding
> is added at the end of the struct, which as far as I can see for now
> affects 5 structs: xfs_dir2_data_entry_t, xfs_dinode_t, xfs_sb_t,
> xfs_dsb_t, and xfs_log_item_t
I did pretty exhaustive testing on new abi and saw no failures that were
clearly unique to arm, although full qa gets enough failures that I
wouldn't swear to it. What testing did you do, and what failures did
you see? And what work did you need to do to "make XFS work" on eabi?
I've helpfully provided structure layouts for the structures you mention
in the attached files, for your diffing pleasure. I think you'll find
that it's not exactly as you described.
xfs_dinode_t has no extra padding at the end, though xfs_dir2_sf, a
member of one of its unions, does (though other union members are larger
though, so the struct offsets are not changed.)
There is one other cosmetic difference just because my arm tree doesn't
have Jeff's ail_entry list change. The rest of the structures seem to
be identical.
If you have structure differences that lead to demonstrable failures,
then by all means, provide the details.
> I must say, I like Jeff's approach of explicitly telling gcc about
> alignment much better :-) It makes it a) much easier to find structs
> that are in fact representations of on-disk data and thus might need
> tweaking, and b) as somebody already said you fix such problems once
> and forever.
> E.g. for me as an absolute outsider, it was quite time consuming
> finding out which structs are actually on-disk.
At some point here I'm just going to go quietly insane.
Yes, _annotating_ things as __ondisk is great, and I have no problems
with that, although it'd be nice if something actually made use of the
annotation. But don't confuse that with telling gcc to actually treat
each of these structures differently, which is great if done properly,
and requires a huge amount of diligence.
If you guys can take this exercise to the point where you've convinced
the sgi guys that the benefits outweigh the risks, then more power to
you. In the meantime, I hope my targeted, safe fix for a demonstrable
problem which has gone begging for 5 years or so doesn't get lost in the
noise.
-Eric
>
> That said, Jeff, you mentioned that your changes don't work yet
> completely - could this be because (at least from the comments) struct
> xfs_sb needs to match struct xfs_dsb and you only change xfs_dsb?
>
>
> Cheers,
> Andre'
>
>
[-- Attachment #2: arm.structs --]
[-- Type: text/plain, Size: 8926 bytes --]
struct xfs_dir2_data_entry {
__be64 inumber; /* 0 8 */
__u8 namelen; /* 8 1 */
__u8 name[1]; /* 9 1 */
__be16 tag; /* 10 2 */
/* size: 12, cachelines: 1 */
/* last cacheline: 12 bytes */
};
struct xfs_dinode {
xfs_dinode_core_t di_core; /* 0 96 */
/* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
__be32 di_next_unlinked; /* 96 4 */
union {
xfs_bmdr_block_t di_bmbt; /* 4 */
xfs_bmbt_rec_32_t di_bmx[1]; /* 16 */
xfs_dir2_sf_t di_dir2sf; /* 24 */
char di_c[1]; /* 1 */
__be32 di_dev; /* 4 */
uuid_t di_muuid; /* 16 */
char di_symlink[1]; /* 1 */
} di_u; /* 100 24 */
union {
xfs_bmdr_block_t di_abmbt; /* 4 */
xfs_bmbt_rec_32_t di_abmx[1]; /* 16 */
xfs_attr_shortform_t di_attrsf; /* 8 */
} di_a; /* 124 16 */
/* --- cacheline 2 boundary (128 bytes) was 12 bytes ago --- */
/* size: 140, cachelines: 3 */
/* last cacheline: 12 bytes */
};
struct xfs_sb {
__uint32_t sb_magicnum; /* 0 4 */
__uint32_t sb_blocksize; /* 4 4 */
xfs_drfsbno_t sb_dblocks; /* 8 8 */
xfs_drfsbno_t sb_rblocks; /* 16 8 */
xfs_drtbno_t sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
xfs_dfsbno_t sb_logstart; /* 48 8 */
xfs_ino_t sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
xfs_ino_t sb_rbmino; /* 64 8 */
xfs_ino_t sb_rsumino; /* 72 8 */
xfs_agblock_t sb_rextsize; /* 80 4 */
xfs_agblock_t sb_agblocks; /* 84 4 */
xfs_agnumber_t sb_agcount; /* 88 4 */
xfs_extlen_t sb_rbmblocks; /* 92 4 */
xfs_extlen_t sb_logblocks; /* 96 4 */
__uint16_t sb_versionnum; /* 100 2 */
__uint16_t sb_sectsize; /* 102 2 */
__uint16_t sb_inodesize; /* 104 2 */
__uint16_t sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__uint8_t sb_blocklog; /* 120 1 */
__uint8_t sb_sectlog; /* 121 1 */
__uint8_t sb_inodelog; /* 122 1 */
__uint8_t sb_inopblog; /* 123 1 */
__uint8_t sb_agblklog; /* 124 1 */
__uint8_t sb_rextslog; /* 125 1 */
__uint8_t sb_inprogress; /* 126 1 */
__uint8_t sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__uint64_t sb_icount; /* 128 8 */
__uint64_t sb_ifree; /* 136 8 */
__uint64_t sb_fdblocks; /* 144 8 */
__uint64_t sb_frextents; /* 152 8 */
xfs_ino_t sb_uquotino; /* 160 8 */
xfs_ino_t sb_gquotino; /* 168 8 */
__uint16_t sb_qflags; /* 176 2 */
__uint8_t sb_flags; /* 178 1 */
__uint8_t sb_shared_vn; /* 179 1 */
xfs_extlen_t sb_inoalignmt; /* 180 4 */
__uint32_t sb_unit; /* 184 4 */
__uint32_t sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__uint8_t sb_dirblklog; /* 192 1 */
__uint8_t sb_logsectlog; /* 193 1 */
__uint16_t sb_logsectsize; /* 194 2 */
__uint32_t sb_logsunit; /* 196 4 */
__uint32_t sb_features2; /* 200 4 */
__uint32_t sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_dsb {
__be32 sb_magicnum; /* 0 4 */
__be32 sb_blocksize; /* 4 4 */
__be64 sb_dblocks; /* 8 8 */
__be64 sb_rblocks; /* 16 8 */
__be64 sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
__be64 sb_logstart; /* 48 8 */
__be64 sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
__be64 sb_rbmino; /* 64 8 */
__be64 sb_rsumino; /* 72 8 */
__be32 sb_rextsize; /* 80 4 */
__be32 sb_agblocks; /* 84 4 */
__be32 sb_agcount; /* 88 4 */
__be32 sb_rbmblocks; /* 92 4 */
__be32 sb_logblocks; /* 96 4 */
__be16 sb_versionnum; /* 100 2 */
__be16 sb_sectsize; /* 102 2 */
__be16 sb_inodesize; /* 104 2 */
__be16 sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__u8 sb_blocklog; /* 120 1 */
__u8 sb_sectlog; /* 121 1 */
__u8 sb_inodelog; /* 122 1 */
__u8 sb_inopblog; /* 123 1 */
__u8 sb_agblklog; /* 124 1 */
__u8 sb_rextslog; /* 125 1 */
__u8 sb_inprogress; /* 126 1 */
__u8 sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__be64 sb_icount; /* 128 8 */
__be64 sb_ifree; /* 136 8 */
__be64 sb_fdblocks; /* 144 8 */
__be64 sb_frextents; /* 152 8 */
__be64 sb_uquotino; /* 160 8 */
__be64 sb_gquotino; /* 168 8 */
__be16 sb_qflags; /* 176 2 */
__u8 sb_flags; /* 178 1 */
__u8 sb_shared_vn; /* 179 1 */
__be32 sb_inoalignmt; /* 180 4 */
__be32 sb_unit; /* 184 4 */
__be32 sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__u8 sb_dirblklog; /* 192 1 */
__u8 sb_logsectlog; /* 193 1 */
__be16 sb_logsectsize; /* 194 2 */
__be32 sb_logsunit; /* 196 4 */
__be32 sb_features2; /* 200 4 */
__be32 sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_log_item {
xfs_ail_entry_t li_ail; /* 0 8 */
xfs_lsn_t li_lsn; /* 8 8 */
struct xfs_log_item_desc * li_desc; /* 16 4 */
struct xfs_mount * li_mountp; /* 20 4 */
uint li_type; /* 24 4 */
uint li_flags; /* 28 4 */
struct xfs_log_item * li_bio_list; /* 32 4 */
void (*li_cb)(struct xfs_buf *, struct xfs_log_item *); /* 36 4 */
struct xfs_item_ops * li_ops; /* 40 4 */
/* size: 44, cachelines: 1 */
/* last cacheline: 44 bytes */
};
[-- Attachment #3: x86.structs --]
[-- Type: text/plain, Size: 8926 bytes --]
struct xfs_dir2_data_entry {
__be64 inumber; /* 0 8 */
__u8 namelen; /* 8 1 */
__u8 name[1]; /* 9 1 */
__be16 tag; /* 10 2 */
/* size: 12, cachelines: 1 */
/* last cacheline: 12 bytes */
};
struct xfs_dinode {
xfs_dinode_core_t di_core; /* 0 96 */
/* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
__be32 di_next_unlinked; /* 96 4 */
union {
xfs_bmdr_block_t di_bmbt; /* 4 */
xfs_bmbt_rec_32_t di_bmx[1]; /* 16 */
xfs_dir2_sf_t di_dir2sf; /* 22 */
char di_c[1]; /* 1 */
__be32 di_dev; /* 4 */
uuid_t di_muuid; /* 16 */
char di_symlink[1]; /* 1 */
} di_u; /* 100 24 */
union {
xfs_bmdr_block_t di_abmbt; /* 4 */
xfs_bmbt_rec_32_t di_abmx[1]; /* 16 */
xfs_attr_shortform_t di_attrsf; /* 8 */
} di_a; /* 124 16 */
/* --- cacheline 2 boundary (128 bytes) was 12 bytes ago --- */
/* size: 140, cachelines: 3 */
/* last cacheline: 12 bytes */
};
struct xfs_sb {
__uint32_t sb_magicnum; /* 0 4 */
__uint32_t sb_blocksize; /* 4 4 */
xfs_drfsbno_t sb_dblocks; /* 8 8 */
xfs_drfsbno_t sb_rblocks; /* 16 8 */
xfs_drtbno_t sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
xfs_dfsbno_t sb_logstart; /* 48 8 */
xfs_ino_t sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
xfs_ino_t sb_rbmino; /* 64 8 */
xfs_ino_t sb_rsumino; /* 72 8 */
xfs_agblock_t sb_rextsize; /* 80 4 */
xfs_agblock_t sb_agblocks; /* 84 4 */
xfs_agnumber_t sb_agcount; /* 88 4 */
xfs_extlen_t sb_rbmblocks; /* 92 4 */
xfs_extlen_t sb_logblocks; /* 96 4 */
__uint16_t sb_versionnum; /* 100 2 */
__uint16_t sb_sectsize; /* 102 2 */
__uint16_t sb_inodesize; /* 104 2 */
__uint16_t sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__uint8_t sb_blocklog; /* 120 1 */
__uint8_t sb_sectlog; /* 121 1 */
__uint8_t sb_inodelog; /* 122 1 */
__uint8_t sb_inopblog; /* 123 1 */
__uint8_t sb_agblklog; /* 124 1 */
__uint8_t sb_rextslog; /* 125 1 */
__uint8_t sb_inprogress; /* 126 1 */
__uint8_t sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__uint64_t sb_icount; /* 128 8 */
__uint64_t sb_ifree; /* 136 8 */
__uint64_t sb_fdblocks; /* 144 8 */
__uint64_t sb_frextents; /* 152 8 */
xfs_ino_t sb_uquotino; /* 160 8 */
xfs_ino_t sb_gquotino; /* 168 8 */
__uint16_t sb_qflags; /* 176 2 */
__uint8_t sb_flags; /* 178 1 */
__uint8_t sb_shared_vn; /* 179 1 */
xfs_extlen_t sb_inoalignmt; /* 180 4 */
__uint32_t sb_unit; /* 184 4 */
__uint32_t sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__uint8_t sb_dirblklog; /* 192 1 */
__uint8_t sb_logsectlog; /* 193 1 */
__uint16_t sb_logsectsize; /* 194 2 */
__uint32_t sb_logsunit; /* 196 4 */
__uint32_t sb_features2; /* 200 4 */
__uint32_t sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_dsb {
__be32 sb_magicnum; /* 0 4 */
__be32 sb_blocksize; /* 4 4 */
__be64 sb_dblocks; /* 8 8 */
__be64 sb_rblocks; /* 16 8 */
__be64 sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
__be64 sb_logstart; /* 48 8 */
__be64 sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
__be64 sb_rbmino; /* 64 8 */
__be64 sb_rsumino; /* 72 8 */
__be32 sb_rextsize; /* 80 4 */
__be32 sb_agblocks; /* 84 4 */
__be32 sb_agcount; /* 88 4 */
__be32 sb_rbmblocks; /* 92 4 */
__be32 sb_logblocks; /* 96 4 */
__be16 sb_versionnum; /* 100 2 */
__be16 sb_sectsize; /* 102 2 */
__be16 sb_inodesize; /* 104 2 */
__be16 sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__u8 sb_blocklog; /* 120 1 */
__u8 sb_sectlog; /* 121 1 */
__u8 sb_inodelog; /* 122 1 */
__u8 sb_inopblog; /* 123 1 */
__u8 sb_agblklog; /* 124 1 */
__u8 sb_rextslog; /* 125 1 */
__u8 sb_inprogress; /* 126 1 */
__u8 sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__be64 sb_icount; /* 128 8 */
__be64 sb_ifree; /* 136 8 */
__be64 sb_fdblocks; /* 144 8 */
__be64 sb_frextents; /* 152 8 */
__be64 sb_uquotino; /* 160 8 */
__be64 sb_gquotino; /* 168 8 */
__be16 sb_qflags; /* 176 2 */
__u8 sb_flags; /* 178 1 */
__u8 sb_shared_vn; /* 179 1 */
__be32 sb_inoalignmt; /* 180 4 */
__be32 sb_unit; /* 184 4 */
__be32 sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__u8 sb_dirblklog; /* 192 1 */
__u8 sb_logsectlog; /* 193 1 */
__be16 sb_logsectsize; /* 194 2 */
__be32 sb_logsunit; /* 196 4 */
__be32 sb_features2; /* 200 4 */
__be32 sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_log_item {
struct list_head li_ail; /* 0 8 */
xfs_lsn_t li_lsn; /* 8 8 */
struct xfs_log_item_desc * li_desc; /* 16 4 */
struct xfs_mount * li_mountp; /* 20 4 */
uint li_type; /* 24 4 */
uint li_flags; /* 28 4 */
struct xfs_log_item * li_bio_list; /* 32 4 */
void (*li_cb)(struct xfs_buf *, struct xfs_log_item *); /* 36 4 */
struct xfs_item_ops * li_ops; /* 40 4 */
/* size: 44, cachelines: 1 */
/* last cacheline: 44 bytes */
};
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 3:18 ` Eric Sandeen
@ 2008-03-19 3:40 ` Eric Sandeen
2008-03-19 5:11 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 3:40 UTC (permalink / raw)
To: Andre Draszik; +Cc: xfs
Eric Sandeen wrote:
> xfs_dinode_t has no extra padding at the end, though xfs_dir2_sf, a
> member of one of its unions, does (though other union members are larger
> though, so the struct offsets are not changed.)
actually that's not quite right, but in any case it does not change the
size of the union or subsequent member offsets.
-Eric
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 3:18 ` Eric Sandeen
2008-03-19 3:40 ` Eric Sandeen
@ 2008-03-19 5:11 ` Eric Sandeen
2008-03-19 5:31 ` Eric Sandeen
2008-03-20 0:35 ` Timothy Shimmin
1 sibling, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 5:11 UTC (permalink / raw)
To: Andre Draszik; +Cc: xfs
[-- Attachment #1: Type: text/plain, Size: 658 bytes --]
Eric Sandeen wrote:
> I've helpfully provided structure layouts for the structures you mention
> in the attached files, for your diffing pleasure. I think you'll find
> that it's not exactly as you described.
Ah hell the arm structs I attached were for oldabi. It's what I get for
saving this fun work for late at night ;)
Attached are eabi structs; still only xfs_dir2_data_entry, xfs_dinode
and xfs_log_item seem to be affected by end-of-struct padding, of the
structures you mention. And xfs_log_item isn't a disk structure...
which brings me back to, what specific failures do you see as a result
of end-of-struct padding on these structs?
-Eric
[-- Attachment #2: arm-eabi.structs --]
[-- Type: text/plain, Size: 8980 bytes --]
struct xfs_dir2_data_entry {
__be64 inumber; /* 0 8 */
__u8 namelen; /* 8 1 */
__u8 name[1]; /* 9 1 */
__be16 tag; /* 10 2 */
/* size: 16, cachelines: 1 */
/* padding: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_dinode {
xfs_dinode_core_t di_core; /* 0 96 */
/* --- cacheline 1 boundary (64 bytes) was 32 bytes ago --- */
__be32 di_next_unlinked; /* 96 4 */
union {
xfs_bmdr_block_t di_bmbt; /* 4 */
xfs_bmbt_rec_32_t di_bmx[1]; /* 16 */
xfs_dir2_sf_t di_dir2sf; /* 22 */
char di_c[1]; /* 1 */
__be32 di_dev; /* 4 */
uuid_t di_muuid; /* 16 */
char di_symlink[1]; /* 1 */
} di_u; /* 100 24 */
union {
xfs_bmdr_block_t di_abmbt; /* 4 */
xfs_bmbt_rec_32_t di_abmx[1]; /* 16 */
xfs_attr_shortform_t di_attrsf; /* 8 */
} di_a; /* 124 16 */
/* --- cacheline 2 boundary (128 bytes) was 12 bytes ago --- */
/* size: 144, cachelines: 3 */
/* padding: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_sb {
__uint32_t sb_magicnum; /* 0 4 */
__uint32_t sb_blocksize; /* 4 4 */
xfs_drfsbno_t sb_dblocks; /* 8 8 */
xfs_drfsbno_t sb_rblocks; /* 16 8 */
xfs_drtbno_t sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
xfs_dfsbno_t sb_logstart; /* 48 8 */
xfs_ino_t sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
xfs_ino_t sb_rbmino; /* 64 8 */
xfs_ino_t sb_rsumino; /* 72 8 */
xfs_agblock_t sb_rextsize; /* 80 4 */
xfs_agblock_t sb_agblocks; /* 84 4 */
xfs_agnumber_t sb_agcount; /* 88 4 */
xfs_extlen_t sb_rbmblocks; /* 92 4 */
xfs_extlen_t sb_logblocks; /* 96 4 */
__uint16_t sb_versionnum; /* 100 2 */
__uint16_t sb_sectsize; /* 102 2 */
__uint16_t sb_inodesize; /* 104 2 */
__uint16_t sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__uint8_t sb_blocklog; /* 120 1 */
__uint8_t sb_sectlog; /* 121 1 */
__uint8_t sb_inodelog; /* 122 1 */
__uint8_t sb_inopblog; /* 123 1 */
__uint8_t sb_agblklog; /* 124 1 */
__uint8_t sb_rextslog; /* 125 1 */
__uint8_t sb_inprogress; /* 126 1 */
__uint8_t sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__uint64_t sb_icount; /* 128 8 */
__uint64_t sb_ifree; /* 136 8 */
__uint64_t sb_fdblocks; /* 144 8 */
__uint64_t sb_frextents; /* 152 8 */
xfs_ino_t sb_uquotino; /* 160 8 */
xfs_ino_t sb_gquotino; /* 168 8 */
__uint16_t sb_qflags; /* 176 2 */
__uint8_t sb_flags; /* 178 1 */
__uint8_t sb_shared_vn; /* 179 1 */
xfs_extlen_t sb_inoalignmt; /* 180 4 */
__uint32_t sb_unit; /* 184 4 */
__uint32_t sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__uint8_t sb_dirblklog; /* 192 1 */
__uint8_t sb_logsectlog; /* 193 1 */
__uint16_t sb_logsectsize; /* 194 2 */
__uint32_t sb_logsunit; /* 196 4 */
__uint32_t sb_features2; /* 200 4 */
__uint32_t sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_dsb {
__be32 sb_magicnum; /* 0 4 */
__be32 sb_blocksize; /* 4 4 */
__be64 sb_dblocks; /* 8 8 */
__be64 sb_rblocks; /* 16 8 */
__be64 sb_rextents; /* 24 8 */
uuid_t sb_uuid; /* 32 16 */
__be64 sb_logstart; /* 48 8 */
__be64 sb_rootino; /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
__be64 sb_rbmino; /* 64 8 */
__be64 sb_rsumino; /* 72 8 */
__be32 sb_rextsize; /* 80 4 */
__be32 sb_agblocks; /* 84 4 */
__be32 sb_agcount; /* 88 4 */
__be32 sb_rbmblocks; /* 92 4 */
__be32 sb_logblocks; /* 96 4 */
__be16 sb_versionnum; /* 100 2 */
__be16 sb_sectsize; /* 102 2 */
__be16 sb_inodesize; /* 104 2 */
__be16 sb_inopblock; /* 106 2 */
char sb_fname[12]; /* 108 12 */
__u8 sb_blocklog; /* 120 1 */
__u8 sb_sectlog; /* 121 1 */
__u8 sb_inodelog; /* 122 1 */
__u8 sb_inopblog; /* 123 1 */
__u8 sb_agblklog; /* 124 1 */
__u8 sb_rextslog; /* 125 1 */
__u8 sb_inprogress; /* 126 1 */
__u8 sb_imax_pct; /* 127 1 */
/* --- cacheline 2 boundary (128 bytes) --- */
__be64 sb_icount; /* 128 8 */
__be64 sb_ifree; /* 136 8 */
__be64 sb_fdblocks; /* 144 8 */
__be64 sb_frextents; /* 152 8 */
__be64 sb_uquotino; /* 160 8 */
__be64 sb_gquotino; /* 168 8 */
__be16 sb_qflags; /* 176 2 */
__u8 sb_flags; /* 178 1 */
__u8 sb_shared_vn; /* 179 1 */
__be32 sb_inoalignmt; /* 180 4 */
__be32 sb_unit; /* 184 4 */
__be32 sb_width; /* 188 4 */
/* --- cacheline 3 boundary (192 bytes) --- */
__u8 sb_dirblklog; /* 192 1 */
__u8 sb_logsectlog; /* 193 1 */
__be16 sb_logsectsize; /* 194 2 */
__be32 sb_logsunit; /* 196 4 */
__be32 sb_features2; /* 200 4 */
__be32 sb_bad_features2; /* 204 4 */
/* size: 208, cachelines: 4 */
/* last cacheline: 16 bytes */
};
struct xfs_log_item {
xfs_ail_entry_t li_ail; /* 0 8 */
xfs_lsn_t li_lsn; /* 8 8 */
struct xfs_log_item_desc * li_desc; /* 16 4 */
struct xfs_mount * li_mountp; /* 20 4 */
uint li_type; /* 24 4 */
uint li_flags; /* 28 4 */
struct xfs_log_item * li_bio_list; /* 32 4 */
void (*li_cb)(struct xfs_buf *, struct xfs_log_item *); /* 36 4 */
struct xfs_item_ops * li_ops; /* 40 4 */
/* size: 48, cachelines: 1 */
/* padding: 4 */
/* last cacheline: 48 bytes */
};
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 5:11 ` Eric Sandeen
@ 2008-03-19 5:31 ` Eric Sandeen
2008-03-20 0:35 ` Timothy Shimmin
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 5:31 UTC (permalink / raw)
To: Andre Draszik; +Cc: xfs
Eric Sandeen wrote:
> Eric Sandeen wrote:
>
>> I've helpfully provided structure layouts for the structures you mention
>> in the attached files, for your diffing pleasure. I think you'll find
>> that it's not exactly as you described.
>
> Ah hell the arm structs I attached were for oldabi. It's what I get for
> saving this fun work for late at night ;)
>
> Attached are eabi structs; still only xfs_dir2_data_entry, xfs_dinode
> and xfs_log_item seem to be affected by end-of-struct padding, of the
> structures you mention. And xfs_log_item isn't a disk structure...
>
> which brings me back to, what specific failures do you see as a result
> of end-of-struct padding on these structs?
... especially since the layout is identical to ia64, padding and all!
Ok must.. stop.. replying.. to.. self.
-Eric
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 0:23 ` Josef 'Jeff' Sipek
@ 2008-03-19 11:21 ` Luca Olivetti
2008-03-19 12:53 ` Eric Sandeen
0 siblings, 1 reply; 51+ messages in thread
From: Luca Olivetti @ 2008-03-19 11:21 UTC (permalink / raw)
To: xfs
En/na Josef 'Jeff' Sipek ha escrit:
> Thanks for the heads up wrt ARM EABI.
Well, I have a linkstation lspro, even if it's running now "freelink"
(i.e. debian, oabi, for the linkstation
http://buffalo.nas-central.org/index.php/FreeLink), it's using the
stock, eabi[*], kernel (Linux lspro 2.6.12.6-arm1 #2 Mon Jul 23 22:35:39
CEST 2007 armv5tejl GNU/Linux) and xfs works just fine.
[*] though I think, but I'm not sure, it was not the real thing but
something that marvell patched in.
However, any later kernel I tried "breaks" xfs (that's why I originally
subscribed to this list), i.e. I cannot see the contents of some directories
(more details here:
http://buffalo.nas-central.org/forums/viewtopic.php?p=35061#p35061)
The strange thing is that I can mount the failing image on i386, so it
probably has the correct structures on disk.
Maybe looking at what marvell/buffalo patched in that kernel could give
some insight on the xfs issues with arm.
Bye
--
Luca
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 11:21 ` Luca Olivetti
@ 2008-03-19 12:53 ` Eric Sandeen
2008-03-19 14:11 ` Luca Olivetti
[not found] ` <47E11F4A.3090205@ventoso.org>
0 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 12:53 UTC (permalink / raw)
To: Luca Olivetti; +Cc: xfs
Luca Olivetti wrote:
> En/na Josef 'Jeff' Sipek ha escrit:
>
>
>> Thanks for the heads up wrt ARM EABI.
>
> Well, I have a linkstation lspro, even if it's running now "freelink"
> (i.e. debian, oabi, for the linkstation
> http://buffalo.nas-central.org/index.php/FreeLink), it's using the
> stock, eabi[*], kernel (Linux lspro 2.6.12.6-arm1 #2 Mon Jul 23 22:35:39
> CEST 2007 armv5tejl GNU/Linux) and xfs works just fine.
yeah, it should. For my list of on-disk structures, arm eabi structure
layout is identical to ia64 - I don't think any heads up is needed.
> [*] though I think, but I'm not sure, it was not the real thing but
> something that marvell patched in.
>
> However, any later kernel I tried "breaks" xfs (that's why I originally
> subscribed to this list), i.e. I cannot see the contents of some directories
> (more details here:
> http://buffalo.nas-central.org/forums/viewtopic.php?p=35061#p35061)
do you still have that small fs image? I'll take a look. There was
also an arm get_unaligned issue which hit xfs in another spot,
http://marc.info/?l=git-commits-head&m=120433318323826&w=3
might be worth seeing if you still have trouble with that fix in place
(which is slated for 2.6.25)
-Eric
> The strange thing is that I can mount the failing image on i386, so it
> probably has the correct structures on disk.
>
> Maybe looking at what marvell/buffalo patched in that kernel could give
> some insight on the xfs issues with arm.
> Bye
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 12:53 ` Eric Sandeen
@ 2008-03-19 14:11 ` Luca Olivetti
[not found] ` <47E11F4A.3090205@ventoso.org>
1 sibling, 0 replies; 51+ messages in thread
From: Luca Olivetti @ 2008-03-19 14:11 UTC (permalink / raw)
To: xfs
En/na Eric Sandeen ha escrit:
>> However, any later kernel I tried "breaks" xfs (that's why I originally
>> subscribed to this list), i.e. I cannot see the contents of some directories
>> (more details here:
>> http://buffalo.nas-central.org/forums/viewtopic.php?p=35061#p35061)
>
> do you still have that small fs image? I'll take a look. There was
> also an arm get_unaligned issue which hit xfs in another spot,
> http://marc.info/?l=git-commits-head&m=120433318323826&w=3
Sure, I'll send you privately a link
> might be worth seeing if you still have trouble with that fix in place
> (which is slated for 2.6.25)
It's not that easy for me to test, since the linkstation is my home
server :-(
Bye
--
Luca
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
[not found] ` <47E12D20.4010901@sandeen.net>
@ 2008-03-19 15:34 ` Luca Olivetti
2008-03-19 15:40 ` Eric Sandeen
[not found] ` <47E14FF1.2020100@sandeen.net>
0 siblings, 2 replies; 51+ messages in thread
From: Luca Olivetti @ 2008-03-19 15:34 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
En/na Eric Sandeen ha escrit:
[posting also to the list since I think it may be interesting for
everybody, with sensitive information removed]
> Luca Olivetti wrote:
>> En/na Eric Sandeen ha escrit:
>>
>>> do you still have that small fs image?
>> Here's the link
>> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>>
>> Please tell me when you have downloaded it, as I'm not sure the image is
>> completely safe, so I don't like having it online.
>>
>> Bye
>
> So, made an xfs_metadump image of that (just smaller, easier to move
> around):
>
> xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
>
> (don't worry, it's not linked from anywhere, I can take it down when you
> grab it) and then:
>
> bunzip2 sda2-meta.img.bz2
> xfs_mdrestore sda2-meta.img sda2.img
> mkdir mnt
> mount -o loop sda2.img mnt/
> ls mnt
> ls mnt/sbin
>
> and this all worked fine for me on 2.6.25, eabi.
good to know!
>
> Linux fedora-arm 2.6.25-rc2 #2 Sat Feb 23 13:58:22 CST 2008 armv5tejl
> armv5tejl armv5tejl GNU/Linux
>
> Can you try the same steps on your box?
I'll report your result on the nas-central.org forum and I'll check
what's the status of 2.6.25-rc2 on the linkstation (though I see that
the orion.git repository is only updated to rc1).
If I find that it's possible to boot this kernel with no (or little ;-)
risk I'll try it.
Bye
--
Luca
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 15:34 ` Luca Olivetti
@ 2008-03-19 15:40 ` Eric Sandeen
[not found] ` <47E14FF1.2020100@sandeen.net>
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-19 15:40 UTC (permalink / raw)
To: Luca Olivetti; +Cc: xfs
Luca Olivetti wrote:
>> Can you try the same steps on your box?
>
> I'll report your result on the nas-central.org forum and I'll check
> what's the status of 2.6.25-rc2 on the linkstation (though I see that
> the orion.git repository is only updated to rc1).
> If I find that it's possible to boot this kernel with no (or little ;-)
> risk I'll try it.
You might also try that same image on your older kernel, to be sure the
test is valid (i.e., my test sequence still breaks for you on the old
kernel).
-Eric
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
[not found] ` <47E14FF1.2020100@sandeen.net>
@ 2008-03-19 18:24 ` Luca Olivetti
0 siblings, 0 replies; 51+ messages in thread
From: Luca Olivetti @ 2008-03-19 18:24 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs
El Wed, 19 Mar 2008 12:40:01 -0500
Eric Sandeen <sandeen@sandeen.net> escribió:
> Luca Olivetti wrote:
>
> > I'll report your result on the nas-central.org forum and I'll check
> > what's the status of 2.6.25-rc2 on the linkstation (though I see
> > that the orion.git repository is only updated to rc1).
> > If I find that it's possible to boot this kernel with no (or
> > little ;-) risk I'll try it.
> >
> > Bye
>
> FWIW... I think you guys would have much more luck if you bring these
> problems to the xfs list. Keeping them on the forums pretty much
> ensures that no xfs developer will ever see them. :)
Sure, but I'm not a developer, just a lurker
that reacted to the trigger word "arm" ;-) I'm just posting to the forum to know the status of the kernel for the
linkstation.
I think that the developers are subscribed to this list and exposed
some of the problem with the linkstation a while ago.
Bye
--
Luca
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-19 5:11 ` Eric Sandeen
2008-03-19 5:31 ` Eric Sandeen
@ 2008-03-20 0:35 ` Timothy Shimmin
1 sibling, 0 replies; 51+ messages in thread
From: Timothy Shimmin @ 2008-03-20 0:35 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Andre Draszik, xfs
Eric Sandeen wrote:
> Eric Sandeen wrote:
>
>> I've helpfully provided structure layouts for the structures you mention
>> in the attached files, for your diffing pleasure. I think you'll find
>> that it's not exactly as you described.
>
> Ah hell the arm structs I attached were for oldabi. It's what I get for
> saving this fun work for late at night ;)
>
> Attached are eabi structs; still only xfs_dir2_data_entry, xfs_dinode
> and xfs_log_item seem to be affected by end-of-struct padding, of the
> structures you mention. And xfs_log_item isn't a disk structure...
>
> which brings me back to, what specific failures do you see as a result
> of end-of-struct padding on these structs?
>
Which reminds me when writing 122 that I noticed with xfs_dinode
but didn't think the end of struct padding would affect things -
remember chatting to Nathan at the time IIRC.
Actually, now that I think about it (Tim waking up a bit :-),
the xfs_dinode_t is of limited value, because a lot of those
union fields at the end aren't actually used directly and just
give an indication of what the layout is like.
We are in the variable literal area there.
--Tim
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 3:24 [PATCH] fix dir2 shortform structures on ARM old ABI Eric Sandeen
2008-03-15 4:17 ` Josef 'Jeff' Sipek
@ 2008-03-20 3:02 ` Eric Sandeen
2008-05-05 7:38 ` Barry Naujok
2008-06-06 14:15 ` Eric Sandeen
2008-04-09 19:53 ` Eric Sandeen
2008-04-23 0:58 ` Eric Sandeen
3 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-03-20 3:02 UTC (permalink / raw)
To: xfs-oss
Here's the userspace side.
Jeff, I guess this means you have more work to do ;)
-Eric
Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
===================================================================
--- xfs-cmds.orig/xfsprogs/include/platform_defs.h.in
+++ 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__ */
Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
===================================================================
--- xfs-cmds.orig/xfsprogs/include/xfs_dir2_sf.h
+++ 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 */
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 3:24 [PATCH] fix dir2 shortform structures on ARM old ABI Eric Sandeen
2008-03-15 4:17 ` Josef 'Jeff' Sipek
2008-03-20 3:02 ` Eric Sandeen
@ 2008-04-09 19:53 ` Eric Sandeen
2008-04-23 0:58 ` Eric Sandeen
3 siblings, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-04-09 19:53 UTC (permalink / raw)
To: xfs-oss
Eric Sandeen wrote:
> 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
> 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 <sandeen@sandeen.net>
>
> ---
>
> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
> +++ 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__ */
> Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
> +++ 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 */
>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-15 3:24 [PATCH] fix dir2 shortform structures on ARM old ABI Eric Sandeen
` (2 preceding siblings ...)
2008-04-09 19:53 ` Eric Sandeen
@ 2008-04-23 0:58 ` Eric Sandeen
2008-05-02 20:55 ` Eric Sandeen
3 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-04-23 0:58 UTC (permalink / raw)
To: xfs-oss
Eric Sandeen wrote:
> 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
> 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 <sandeen@sandeen.net>
>
> ---
>
> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
> +++ 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__ */
> Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
> ===================================================================
> --- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
> +++ 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 */
>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-04-23 0:58 ` Eric Sandeen
@ 2008-05-02 20:55 ` Eric Sandeen
2008-05-05 7:08 ` David Chinner
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-05-02 20:55 UTC (permalink / raw)
To: xfs-oss
Eric Sandeen wrote:
> Eric Sandeen wrote:
>> 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...
> 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
>
>> 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 <sandeen@sandeen.net>
>>
>> ---
>>
>> Index: linux-2.6.24/fs/xfs/linux-2.6/xfs_linux.h
>> ===================================================================
>> --- linux-2.6.24.orig/fs/xfs/linux-2.6/xfs_linux.h
>> +++ 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__ */
>> Index: linux-2.6.24/fs/xfs/xfs_dir2_sf.h
>> ===================================================================
>> --- linux-2.6.24.orig/fs/xfs/xfs_dir2_sf.h
>> +++ 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 */
>>
>>
>>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-05-02 20:55 ` Eric Sandeen
@ 2008-05-05 7:08 ` David Chinner
2008-05-05 13:07 ` Eric Sandeen
2008-05-06 4:21 ` Timothy Shimmin
0 siblings, 2 replies; 51+ messages in thread
From: David Chinner @ 2008-05-05 7:08 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
> Eric Sandeen wrote:
> > Eric Sandeen wrote:
> >> 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...
<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.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-20 3:02 ` Eric Sandeen
@ 2008-05-05 7:38 ` Barry Naujok
2008-06-06 14:15 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Barry Naujok @ 2008-05-05 7:38 UTC (permalink / raw)
To: xfs-oss; +Cc: David Chinner
On Thu, 20 Mar 2008 14:02:27 +1100, Eric Sandeen <sandeen@sandeen.net>
wrote:
> Here's the userspace side.
Dave, yes he did!
> Jeff, I guess this means you have more work to do ;)
>
> -Eric
>
>
> Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
> ===================================================================
> --- xfs-cmds.orig/xfsprogs/include/platform_defs.h.in
> +++ 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__ */
> Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
> ===================================================================
> --- xfs-cmds.orig/xfsprogs/include/xfs_dir2_sf.h
> +++ 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 */
>
>
>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-05-05 7:08 ` David Chinner
@ 2008-05-05 13:07 ` Eric Sandeen
2008-05-06 4:21 ` Timothy Shimmin
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-05-05 13:07 UTC (permalink / raw)
To: David Chinner; +Cc: xfs-oss
David Chinner wrote:
> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>> Eric Sandeen wrote:
>>> Eric Sandeen wrote:
>>>> 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...
>
> <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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-05-05 7:08 ` David Chinner
2008-05-05 13:07 ` Eric Sandeen
@ 2008-05-06 4:21 ` Timothy Shimmin
2008-05-06 13:43 ` Eric Sandeen
1 sibling, 1 reply; 51+ messages in thread
From: Timothy Shimmin @ 2008-05-06 4:21 UTC (permalink / raw)
To: David Chinner; +Cc: Eric Sandeen, xfs-oss
David Chinner wrote:
> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>> Eric Sandeen wrote:
>>> Eric Sandeen wrote:
>>>> 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...
>
> <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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-05-06 4:21 ` Timothy Shimmin
@ 2008-05-06 13:43 ` Eric Sandeen
2008-06-05 5:38 ` Eric Sandeen
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-05-06 13:43 UTC (permalink / raw)
To: Timothy Shimmin; +Cc: David Chinner, xfs-oss
Timothy Shimmin wrote:
> David Chinner wrote:
>> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>>> Eric Sandeen wrote:
>>>> Eric Sandeen wrote:
>>>>> 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...
>> <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.
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
> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-05-06 13:43 ` Eric Sandeen
@ 2008-06-05 5:38 ` Eric Sandeen
2008-06-05 5:46 ` Mark Goodwin
2008-06-05 5:49 ` Chris Wedgwood
0 siblings, 2 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-06-05 5:38 UTC (permalink / raw)
To: Timothy Shimmin; +Cc: xfs-oss
Eric Sandeen wrote:
> Timothy Shimmin wrote:
>> David Chinner wrote:
>>> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>>>> Eric Sandeen wrote:
>>>>> Eric Sandeen wrote:
>>>>>> 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...
>>> <sigh>
Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
is it EVER going to get checked in?
-Eric
>>> 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.
>
> 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
>
>> 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
>>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:38 ` Eric Sandeen
@ 2008-06-05 5:46 ` Mark Goodwin
2008-06-05 5:49 ` Eric Sandeen
2008-06-05 5:49 ` Chris Wedgwood
1 sibling, 1 reply; 51+ messages in thread
From: Mark Goodwin @ 2008-06-05 5:46 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Timothy Shimmin, xfs-oss
Eric Sandeen wrote:
> Eric Sandeen wrote:
>> Timothy Shimmin wrote:
>>> David Chinner wrote:
>>>> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>>>>> Eric Sandeen wrote:
>>>>>> Eric Sandeen wrote:
>>>>>>> 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...
>>>> <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
>
> -Eric
>
>>>> 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.
>> 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
>>
>>> 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 markgw@sgi.com
Engineering Manager for XFS and PCP Phone: +61-3-99631937
SGI Australian Software Group Cell: +61-4-18969583
-------------------------------------------------------------
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:46 ` Mark Goodwin
@ 2008-06-05 5:49 ` Eric Sandeen
2008-06-05 6:02 ` Mark Goodwin
0 siblings, 1 reply; 51+ messages in thread
From: Eric Sandeen @ 2008-06-05 5:49 UTC (permalink / raw)
To: markgw; +Cc: Timothy Shimmin, xfs-oss
Mark Goodwin wrote:
>
> Eric Sandeen wrote:
>> Eric Sandeen wrote:
>>> Timothy Shimmin wrote:
>>>> David Chinner wrote:
>>>>> On Fri, May 02, 2008 at 03:55:45PM -0500, Eric Sandeen wrote:
>>>>>> Eric Sandeen wrote:
>>>>>>> Eric Sandeen wrote:
>>>>>>>> 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...
>>>>> <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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:38 ` Eric Sandeen
2008-06-05 5:46 ` Mark Goodwin
@ 2008-06-05 5:49 ` Chris Wedgwood
2008-06-05 5:52 ` Eric Sandeen
2008-06-05 6:34 ` Eric Sandeen
1 sibling, 2 replies; 51+ messages in thread
From: Chris Wedgwood @ 2008-06-05 5:49 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Timothy Shimmin, xfs-oss
On Thu, Jun 05, 2008 at 12:38:30AM -0500, Eric Sandeen wrote:
> Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
[...]
> >> 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?
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:49 ` Chris Wedgwood
@ 2008-06-05 5:52 ` Eric Sandeen
2008-06-05 6:34 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-06-05 5:52 UTC (permalink / raw)
To: Chris Wedgwood; +Cc: Timothy Shimmin, xfs-oss
Chris Wedgwood wrote:
> On Thu, Jun 05, 2008 at 12:38:30AM -0500, Eric Sandeen wrote:
>
>> Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
>
> [...]
>
>>>> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:49 ` Eric Sandeen
@ 2008-06-05 6:02 ` Mark Goodwin
2008-06-05 6:04 ` Mark Goodwin
2008-06-05 6:06 ` Eric Sandeen
0 siblings, 2 replies; 51+ messages in thread
From: Mark Goodwin @ 2008-06-05 6:02 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Timothy Shimmin, xfs-oss
Eric Sandeen wrote:
>
> 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 ..?
Timothy Shimmin wrote:
> 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
--
Mark Goodwin markgw@sgi.com
Engineering Manager for XFS and PCP Phone: +61-3-99631937
SGI Australian Software Group Cell: +61-4-18969583
-------------------------------------------------------------
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 6:02 ` Mark Goodwin
@ 2008-06-05 6:04 ` Mark Goodwin
2008-06-05 6:06 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Mark Goodwin @ 2008-06-05 6:04 UTC (permalink / raw)
To: markgw; +Cc: Eric Sandeen, Timothy Shimmin, xfs-oss
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
Mark Goodwin wrote:
>
>
> Eric Sandeen wrote:
>>
>> 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 ..?
>
> Timothy Shimmin wrote:
>> 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
>
--
Mark Goodwin markgw@sgi.com
Engineering Manager for XFS and PCP Phone: +61-3-99631937
SGI Australian Software Group Cell: +61-4-18969583
-------------------------------------------------------------
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 6:02 ` Mark Goodwin
2008-06-05 6:04 ` Mark Goodwin
@ 2008-06-05 6:06 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-06-05 6:06 UTC (permalink / raw)
To: markgw; +Cc: Timothy Shimmin, xfs-oss
Mark Goodwin wrote:
>
> Eric Sandeen wrote:
>> 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
> Timothy Shimmin wrote:
>> 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
>
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-06-05 5:49 ` Chris Wedgwood
2008-06-05 5:52 ` Eric Sandeen
@ 2008-06-05 6:34 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-06-05 6:34 UTC (permalink / raw)
To: Chris Wedgwood; +Cc: Timothy Shimmin, xfs-oss
Chris Wedgwood wrote:
> On Thu, Jun 05, 2008 at 12:38:30AM -0500, Eric Sandeen wrote:
>
>> Guys, this is SIMPLE, SAFE, and it fixes a CORRUPTION BUG.
>
> [...]
>
>>>> 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
^ permalink raw reply [flat|nested] 51+ messages in thread
* Re: [PATCH] fix dir2 shortform structures on ARM old ABI
2008-03-20 3:02 ` Eric Sandeen
2008-05-05 7:38 ` Barry Naujok
@ 2008-06-06 14:15 ` Eric Sandeen
1 sibling, 0 replies; 51+ messages in thread
From: Eric Sandeen @ 2008-06-06 14:15 UTC (permalink / raw)
To: xfs-oss; +Cc: Barry Naujok
Eric Sandeen wrote:
> Here's the userspace side.
>
Just a reminder after Dave's reminder. :)
-Eric
> -Eric
>
>
> Index: xfs-cmds/xfsprogs/include/platform_defs.h.in
> ===================================================================
> --- xfs-cmds.orig/xfsprogs/include/platform_defs.h.in
> +++ 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__ */
> Index: xfs-cmds/xfsprogs/include/xfs_dir2_sf.h
> ===================================================================
> --- xfs-cmds.orig/xfsprogs/include/xfs_dir2_sf.h
> +++ 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 */
>
>
>
>
>
^ permalink raw reply [flat|nested] 51+ messages in thread
end of thread, other threads:[~2008-06-06 14:15 UTC | newest]
Thread overview: 51+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-03-15 3:24 [PATCH] fix dir2 shortform structures on ARM old ABI Eric Sandeen
2008-03-15 4:17 ` Josef 'Jeff' Sipek
2008-03-15 4:23 ` Eric Sandeen
2008-03-15 4:27 ` Josef 'Jeff' Sipek
2008-03-15 4:33 ` Eric Sandeen
2008-03-15 4:51 ` Josef 'Jeff' Sipek
2008-03-17 18:32 ` Eric Sandeen
2008-03-17 19:53 ` Josef 'Jeff' Sipek
2008-03-17 20:04 ` Eric Sandeen
2008-03-17 20:28 ` Josef 'Jeff' Sipek
2008-03-18 0:39 ` [RFC][PATCH 1/1] XFS: annotate all on-disk structures with __ondisk Josef 'Jeff' Sipek
2008-03-18 3:34 ` Eric Sandeen
2008-03-18 4:09 ` David Chinner
2008-03-18 5:28 ` Josef 'Jeff' Sipek
2008-03-17 23:35 ` [PATCH] fix dir2 shortform structures on ARM old ABI Timothy Shimmin
2008-03-17 23:42 ` Josef 'Jeff' Sipek
2008-03-18 4:31 ` Timothy Shimmin
[not found] ` <20080315043622.GA11547@puku.stupidest.org>
2008-03-15 4:45 ` Eric Sandeen
2008-03-20 3:02 ` Eric Sandeen
2008-05-05 7:38 ` Barry Naujok
2008-06-06 14:15 ` Eric Sandeen
2008-04-09 19:53 ` Eric Sandeen
2008-04-23 0:58 ` Eric Sandeen
2008-05-02 20:55 ` Eric Sandeen
2008-05-05 7:08 ` David Chinner
2008-05-05 13:07 ` Eric Sandeen
2008-05-06 4:21 ` Timothy Shimmin
2008-05-06 13:43 ` Eric Sandeen
2008-06-05 5:38 ` Eric Sandeen
2008-06-05 5:46 ` Mark Goodwin
2008-06-05 5:49 ` Eric Sandeen
2008-06-05 6:02 ` Mark Goodwin
2008-06-05 6:04 ` Mark Goodwin
2008-06-05 6:06 ` Eric Sandeen
2008-06-05 5:49 ` Chris Wedgwood
2008-06-05 5:52 ` Eric Sandeen
2008-06-05 6:34 ` Eric Sandeen
-- strict thread matches above, loose matches on Subject: below --
2008-03-18 23:31 Andre Draszik
2008-03-19 3:18 ` Eric Sandeen
2008-03-19 3:40 ` Eric Sandeen
2008-03-19 5:11 ` Eric Sandeen
2008-03-19 5:31 ` Eric Sandeen
2008-03-20 0:35 ` Timothy Shimmin
2008-03-18 23:49 Andre Draszik
2008-03-19 0:23 ` Josef 'Jeff' Sipek
2008-03-19 11:21 ` Luca Olivetti
2008-03-19 12:53 ` Eric Sandeen
2008-03-19 14:11 ` Luca Olivetti
[not found] ` <47E11F4A.3090205@ventoso.org>
[not found] ` <47E12D20.4010901@sandeen.net>
2008-03-19 15:34 ` Luca Olivetti
2008-03-19 15:40 ` Eric Sandeen
[not found] ` <47E14FF1.2020100@sandeen.net>
2008-03-19 18:24 ` Luca Olivetti
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox