fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

include/linux/falloc.h +8 -0

@@ -3,4 +3,48 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;

include/linux/falloc.h line 23

Quoted 12 lines · On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <[email protected]> wrote:

include/linux/falloc.h +12 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};
+
+#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)

Are we sure that the aligment of l_start will be reliably the same across all compilers and versions thereof for all time?

Quoted 16 lines · Andrew Morton

include/linux/falloc.h line 23

Quoted 12 lines · On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <[email protected]> wrote:

include/linux/falloc.h +12 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};
+
+#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)

Are we sure that the aligment of l_start will be reliably the same across all compilers and versions thereof for all time?

On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.

XFS handles the conversion for compat_ioctl in fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl, the conversion code should be moved to fs/compat_ioctl.c.

Is this written in a standard somewhere?  Is it guaranteed?

If some (perhaps non-gcc) compiler were to lay this out differently (perhaps with suitable command-line options) then that's liveable with - as long as the kernel never changes the layout.  Of course it would be better to avoid this if poss.

The other potential issue with a structure like this is that there's a risk that it will lead us to copy four bytes of uninitialised kernel memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding for this sort of thing.

Quoted 1 line · Andrew Morton

Is this written in a standard somewhere? Is it guaranteed?

Alignment is defined in the architecture psABI documents. Unfortunately, many of them were written before the 'long long' type became part of the C standard, so it's not strictly guaranteed. AFAICT, the alignment of __u64 on x86 is the same as the alignment of 'double' by convention.

However, the problem is well-understood: x86 is the only one that has a problem in 32/64 bit compat mode. m68k has similar issues with 16/32 bit integers, but those don't apply here.

Quoted 4 lines

If some (perhaps non-gcc) compiler were to lay this out differently (perhaps with suitable command-line options) then that's liveable with - as long as the kernel never changes the layout. Of course it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of library interfaces would break. The kernel ABI is only a small part of the problem then.

Quoted 6 lines

The other potential issue with a structure like this is that there's a risk that it will lead us to copy four bytes of uninitialised kernel memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding for this sort of thing.

Agreed in general, but the whole point of this particular patch was to provide compatibility with an interface that has been part of XFS for many years.
Linux already has a better interface for new users (sys_fallocate), so changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure is only read, never written.

	Arnd <><

Quoted 39 lines · Arnd Bergmann

Quoted 1 line · Andrew Morton

Is this written in a standard somewhere? Is it guaranteed?

Alignment is defined in the architecture psABI documents. Unfortunately, many of them were written before the 'long long' type became part of the C standard, so it's not strictly guaranteed. AFAICT, the alignment of __u64 on x86 is the same as the alignment of 'double' by convention.

However, the problem is well-understood: x86 is the only one that has a problem in 32/64 bit compat mode. m68k has similar issues with 16/32 bit integers, but those don't apply here.

Quoted 4 lines

If some (perhaps non-gcc) compiler were to lay this out differently (perhaps with suitable command-line options) then that's liveable with - as long as the kernel never changes the layout. Of course it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of library interfaces would break. The kernel ABI is only a small part of the problem then.

Quoted 6 lines

The other potential issue with a structure like this is that there's a risk that it will lead us to copy four bytes of uninitialised kernel memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding for this sort of thing.

Agreed in general, but the whole point of this particular patch was to provide compatibility with an interface that has been part of XFS for many years.
Linux already has a better interface for new users (sys_fallocate), so changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure is only read, never written.

	Arnd <><

include/linux/falloc.h line 20

Quoted 9 lines · Arnd Bergmann

include/linux/falloc.h +9 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};

What about telling the compiler exactly what you said above, just to be sure we all mean the same thing. (And as documentation for new comers):

Quoted 51 lines · Boaz Harrosh

Quoted 36 lines · Arnd Bergmann

Quoted 1 line · Andrew Morton

Is this written in a standard somewhere? Is it guaranteed?

Alignment is defined in the architecture psABI documents. Unfortunately, many of them were written before the 'long long' type became part of the C standard, so it's not strictly guaranteed. AFAICT, the alignment of __u64 on x86 is the same as the alignment of 'double' by convention.

However, the problem is well-understood: x86 is the only one that has a problem in 32/64 bit compat mode. m68k has similar issues with 16/32 bit integers, but those don't apply here.

Quoted 4 lines

If some (perhaps non-gcc) compiler were to lay this out differently (perhaps with suitable command-line options) then that's liveable with - as long as the kernel never changes the layout. Of course it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of library interfaces would break. The kernel ABI is only a small part of the problem then.

Quoted 6 lines

The other potential issue with a structure like this is that there's a risk that it will lead us to copy four bytes of uninitialised kernel memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding for this sort of thing.

Agreed in general, but the whole point of this particular patch was to provide compatibility with an interface that has been part of XFS for many years.
Linux already has a better interface for new users (sys_fallocate), so changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure is only read, never written.

	Arnd <><

Review

Quoted 9 lines · Arnd Bergmann

(quoted diff) +9 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};

What about telling the compiler exactly what you said above, just to be sure we all mean the same thing. (And as documentation for new comers):

Review

Quoted 10 lines

(quoted diff) +10 -0

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

Because the compiler will assume all fields are always unaligned and will use very suboptimal code to access them?

Gr{oetje,eeting}s,

Geert

Quoted 15 lines · Geert Uytterhoeven

Quoted 14 lines · Boaz Harrosh

Review

Quoted 9 lines · Arnd Bergmann

(quoted diff) +9 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};

What about telling the compiler exactly what you said above, just to be sure we all mean the same thing. (And as documentation for new comers):

Quoted 13 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

Because the compiler will assume all fields are always unaligned and will use very suboptimal code to access them?

This discussion comes up every once in a while. I'm using an old FC7 compiler (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout of a structure is exactly the same the "__packed" on structure declarations does nothing. It only starts to affect when there are real differences in alignment. Also tests with gcc 3.4.x showed the same effect.

On previous discussions no one could come forward and say what compiler version breaks when __packed is applied on structure definition. I'm afraid your statement above is a myth.

Quoted 34 lines · Boaz Harrosh

Quoted 15 lines · Geert Uytterhoeven

Quoted 14 lines · Boaz Harrosh

Review

Quoted 9 lines · Arnd Bergmann

(quoted diff) +9 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};

What about telling the compiler exactly what you said above, just to be sure we all mean the same thing. (And as documentation for new comers):

Quoted 12 lines

Review

Quoted 10 lines

(quoted diff) +10 -0

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

Because the compiler will assume all fields are always unaligned and will use very suboptimal code to access them?

This discussion comes up every once in a while. I'm using an old FC7 compiler (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout of a structure is exactly the same the "__packed" on structure declarations does nothing. It only starts to affect when there are real differences in alignment. Also tests with gcc 3.4.x showed the same effect.

Quoted 3 lines

On previous discussions no one could come forward and say what compiler version breaks when __packed is applied on structure definition. I'm afraid your statement above is a myth.

FC7, targeting ia32? Sure, ia32 has no alignment restrictions. Try e.g. MIPS.

Gr{oetje,eeting}s,

Geert

I don't understand

if you have a structure like

struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change the layout of the structure. Are you saying the __attribute__((packed)) is an hint to the compiler that foo_packed might be used unaligned. This is just brain-dead, because I can use an unaligned pointer to foo just as I can to foo_packed. Otherwise there is no difference what-so-ever between the two. I have to see it to believe. It is totally the wrong hint in the wrong place taking away valuable meaning of saying "please don't use padding holes in this structure"

Sorry for been so slow, I just don't get it.
Boaz

Boaz Harrosh wrote:

...

Quoted 22 lines

I don't understand

if you have a structure like

struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change the layout of the structure. Are you saying the __attribute__((packed)) is an hint to the compiler that foo_packed might be used unaligned. This is just brain-dead, because I can use an unaligned pointer to foo just as I can to foo_packed. Otherwise there is no difference what-so-ever between the two. I have to see it to believe. It is totally the wrong hint in the wrong place taking away valuable meaning of saying "please don't use padding holes in this structure"

Quoted 2 lines

Sorry for been so slow, I just don't get it.
Boaz

While I'm no gcc guru, I can confirm that gratuitous use of the packed attribute is suboptimal; adding "packed" to every ondisk structure made obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

-Eric

Quoted 26 lines · Eric Sandeen

Boaz Harrosh wrote:

...

Quoted 22 lines

I don't understand

if you have a structure like

struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change the layout of the structure. Are you saying the __attribute__((packed)) is an hint to the compiler that foo_packed might be used unaligned. This is just brain-dead, because I can use an unaligned pointer to foo just as I can to foo_packed. Otherwise there is no difference what-so-ever between the two. I have to see it to believe. It is totally the wrong hint in the wrong place taking away valuable meaning of saying "please don't use padding holes in this structure"

Quoted 5 lines

Quoted 2 lines

Sorry for been so slow, I just don't get it.
Boaz

While I'm no gcc guru, I can confirm that gratuitous use of the packed attribute is suboptimal; adding "packed" to every ondisk structure made obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ? If not then clearly above is expected.

The structures have been defined exactly like that in XFS (and ocfs2) before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through the endless discussions about the pros and cons for it, but I think doing it in context of this patch is not helpful.

Quoted 9 lines · Arnd Bergmann

Review

Quoted 9 lines

(quoted diff) +9 -0

+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};

What about telling the compiler exactly what you said above, just to be sure we all mean the same thing. (And as documentation for new comers):

Quoted 6 lines · Christoph Hellwig

The structures have been defined exactly like that in XFS (and ocfs2) before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through the endless discussions about the pros and cons for it, but I think doing it in context of this patch is not helpful.

OK so ia64 gcc is broken in regard to __attribute__((packed(1))), and it should not be used.

But clearly the programmer, Like in this patch exactly, should spell out the hole created by padding and spell that hole out and call it __Padding. The porgrammer thought about it, identified there is an hole, please don't drop this information on the floor. Put it in the code so I don't have to break my head on it.

Quoted 7 lines · Boaz Harrosh

Quoted 6 lines · Christoph Hellwig

The structures have been defined exactly like that in XFS (and ocfs2) before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through the endless discussions about the pros and cons for it, but I think doing it in context of this patch is not helpful.

Quoted 2 lines

OK so ia64 gcc is broken in regard to __attribute__((packed(1))), and it should not be used.

No, the compiler is correct, it has to generate more complex code if it cannot assume that data is naturally aligned and the architecture does not support unaligned loads. If you don't understand this, please at least read the list archives about the last five times this came up before claiming that the compiler is broken.

Quoted 5 lines

But clearly the programmer, Like in this patch exactly, should spell out the hole created by padding and spell that hole out and call it __Padding. The porgrammer thought about it, identified there is an hole, please don't drop this information on the floor. Put it in the code so I don't have to break my head on it.

Again, you are missing the point. The patch has to leave out the padding and whatnot because the idea of the patch is to provide a backwards-compatible API for programs written against the legacy XFS API. The original definition of the interface was flawed, but unless you can travel back in time to complain about this in the initial submission of the XFS file system, you should not blame the entirely correct patch now.

	Arnd <><

Quoted 8 lines · Arnd Bergmann

Quoted 7 lines · Boaz Harrosh

Quoted 6 lines · Christoph Hellwig

The structures have been defined exactly like that in XFS (and ocfs2) before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through the endless discussions about the pros and cons for it, but I think doing it in context of this patch is not helpful.

Quoted 9 lines

Quoted 2 lines

OK so ia64 gcc is broken in regard to __attribute__((packed(1))), and it should not be used.

No, the compiler is correct, it has to generate more complex code if it cannot assume that data is naturally aligned and the architecture does not support unaligned loads. If you don't understand this, please at least read the list archives about the last five times this came up before claiming that the compiler is broken.

Wrong!! Sorry, you guys don't listen.
I'm talking of the case where the structures are EXACTLY the same anyway you look at them. sizeof(foo) == sizeof(foo_packed) and offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of the structure. foo && foo_packed are declared exactly the same but with __attribute__((packed(1))) applied to the later.

THEN in ia64 case the compiler is brain dead, because it relates "unalignment" to packed(1) which are two different things.

Quoted 14 lines · Boaz Harrosh

Quoted 6 lines · Arnd Bergmann

No, the compiler is correct, it has to generate more complex code if it cannot assume that data is naturally aligned and the architecture does not support unaligned loads. If you don't understand this, please at least read the list archives about the last five times this came up before claiming that the compiler is broken.

Wrong!! Sorry, you guys don't listen.
I'm talking of the case where the structures are EXACTLY the same anyway you look at them. sizeof(foo) == sizeof(foo_packed) and offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of the structure. foo && foo_packed are declared exactly the same but with __attribute__((packed(1))) applied to the later.

Quoted 2 lines

THEN in ia64 case the compiler is brain dead, because it relates "unalignment" to packed(1) which are two different things.

The natural alignment of a structure is max(alignment(member)), for all members. With __attribute__((packed)), the natural alignment of the structure is 1, so the compiler cannot assume anything.

While the ints in the structure may still be at offsets 0, 4, 8, and so on, this doesn't say anything about their actual memory addresses, as the struct base address itself may be unaligned.

Gr{oetje,eeting}s,

Geert

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.

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

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.

Quoted 4 lines · Boaz Harrosh

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.

Quoted 2 lines

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.

Quoted 4 lines

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

I would at least expect a big fat comment explaining what happened to the structure on what known ARCHs, and how it is expected to look in memory. And a BUILD_BUG_ON to make sure of that.

From http://wiki.debian.org/ArmEabiPort

Al, Could this be included in the vfs queue?

This patch adds ioctls to vfs for compatibility with legacy XFS pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation effectively invokes sys_fallocate for the new ioctls. Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

fs/compat_ioctl.c +5 -0

+/* on ia32 l_start is on a 32-bit boundary */
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;

fs/compat_ioctl.c +9 -0

+/* just account for different alignment */
+static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||

I'd just define this using compat_s64 instead of __s64 __packed so we can use the same code on all architectures, even at the small cost of extra text size on non-x86 architectures.