Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier <jamie@shareable.org>]

Boaz Harrosh wrote:
> No the natural alignment is what it is, after the application of
> __attribute__((packed(1))). In a well defined structure that is a no-opt.
> But yes in ai64 the gcc programmers got lazy and did not make that analysis
> after laying out the structure.

No.  That's what you *want* packed to mean, but it doesn't mean that.

__attribute__ packed on a struct definition means to pack the
structure _and_ set its assumed alignment to 1.

This is what the packed attribute historically means, and it cannot be
changed without breaking existing code.

If you want to remove padding from a structure, but still keep its
natural alignment, you do it with two attributes together:
__attribute__((packed, aligned(4))).  You have to choose the alignment
you want in that case.

GCC manual:
     When used on a struct, or struct member, the `aligned' attribute
     can only increase the alignment; in order to decrease it, the
     `packed' attribute must be specified as well.  When used as part
     of a typedef, the `aligned' attribute can both increase and
     decrease alignment, and specifying the `packed' attribute will
     generate a warning.

It's a counterintuitive, because you must use
__attribute__((aligned(1))) when declaring a variable to reduce its
alignment, but you must use __attribute__((packed)) when declaring a
struct type.  Doing it at the end of a struct typedef is a weird mix
of semantics, so don't do that.

By the way, this discussion is why the "-Wpacked" and "-Wpadding"
options are available.

> The base address can be unaligned even if the structure is aligned. In that
> case you need the __atrubute__((aligned)) thingy.

No, because __attribute__((packed)) on a struct doesn't mean what you
want it to mean.  Use __attribute((packed,aligned(4))) if that's what
you want.

> Please note that I gave up on the compiler and understand that the
> use of __packed is dangerous in some cases, sigh. My standing point
> is to make sure there are no guesses left, and a BUILD_BUG_ON to
> make sure of that.

In this code, it's not a bug because it must be backward compatible
with existing binary code.  "Fixing" the padding breaks
compatibility, which is pointless for this patch.

-- Jamie