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

[Boaz Harrosh <bharrosh@panasas.com>]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> On Saturday 31 January 2009, Andrew Morton wrote:
>>>> 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.
>>>
>>>> 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.
>>>
>>>> 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 <><
>> Arnd Bergmann wrote:
>>> +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):
>>
>> +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
> 

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.

Boaz