xfs
[Top] [All Lists]

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

To: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
Subject: Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls
From: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
Date: Sun, 01 Feb 2009 12:39:39 +0200
Cc: Arnd Bergmann <arnd@xxxxxxxx>, Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>, Ankit Jain <me@xxxxxxxxxxxxx>, viro@xxxxxxxxxxxxxxxxxx, hch@xxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, mfasheh@xxxxxxxx, joel.becker@xxxxxxxxxx, ocfs2-devel@xxxxxxxxxxxxxx, linux-kernel@xxxxxxxxxxxxxxx, xfs-masters@xxxxxxxxxxx, xfs@xxxxxxxxxxx
In-reply-to: <Pine.LNX.4.64.0902011104320.20875@anakin>
References: <4980C71F.1010804@xxxxxxxxxxxxx> <200901310138.34164.arnd@xxxxxxxx> <20090130171423.f99c88d0.akpm@xxxxxxxxxxxxxxxxxxxx> <200901310248.42820.arnd@xxxxxxxx> <49856FE6.8020601@xxxxxxxxxxx> <Pine.LNX.4.64.0902011104320.20875@anakin>
User-agent: Thunderbird/3.0a2 (X11; 2008072418)
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

<Prev in Thread] Current Thread [Next in Thread>