[Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 09/04/2019 13:48, Andrew Price wrote:
>
>
> On 09/04/2019 13:21, Steven Whitehouse wrote:
>> Hi,
>>
>> On 09/04/2019 13:18, Andrew Price wrote:
>>> On 09/04/2019 13:03, Christoph Hellwig wrote:
>>>> On Tue, Apr 09, 2019 at 10:41:53AM +0100, Andrew Price wrote:
>>>>> Give gfs2-utils its own copy of gfs2_ondisk.h which uses userspace
>>>>> types. This allows us to always support the latest ondisk 
>>>>> structures and
>>>>> obsoletes a lot of #ifdef GFS2_HAS_<FEATURE> blocks and configure.ac
>>>>> checks.
>>>>>
>>>>> gfs2_ondisk.h was changed simply by search-and-replace of the 
>>>>> kernel int
>>>>> types with the uintN_t, i.e.:
>>>>>
>>>>> :%s/__u\(8\|16\|32\|64\)/uint\1_t/g
>>>>> :%s/__be\(64\|32\|16\|8\)/uint\1_t/g
>>>>>
>>>>> and the linux/types.h include replaced with stdint.h
>>>>
>>>> Why?
>>>
>>> Because I'd like to be able to build gfs2-utils on FreeBSD one day. 
>>> Plus we get the handy stuff in inttypes.h to use, Linux doesn't have 
>>> that.
>>>
>>>> At least the be types give you really useful type checking with
>>>> sparse, which can be trivially wired up in userspace as well.
>>>
>>> If you mean the bitwise annotations that only sparse checks, we're 
>>> fairly safe in gfs2-utils in that anything represented by a struct 
>>> is going to have been parsed through one of the libgfs2/ondisk.c 
>>> functions so will be the right endianness. I run sparse over this 
>>> code very rarely anyway.
>>
>> Those conversion functions are not sensible, thats why we got rid of 
>> them from the kernel code. 
>
> Is it the functions that aren't sensible or the use of the 
> gfs2_ondisk.h structs as the containers for the native endian data? 
> I'm not sure I get why the kernel functions like gfs2_dinode_in() are 
> considered sensible and gfs2-utils' gfs2_dinode_in(), which does a 
> similar thing but with a different struct, isn't sensible.
>
Well in general we don't want to convert lots of fields in what is 
basically a copy. The inode, when it is read in is an exception to that 
mainly because we have to in order to make sure that the vfs level data 
is all up to date. Keeping the structs as containers is useful, so yes 
we want to retain that. In many cases though we only need a few fields 
from what can be quite large data structures, so in those cases we 
should read/update the fields that we care about for that particular 
operation, rather than converting the whole data structure each time. We 
got a fair speed up when we made that change in the kernel.

So generally I'd like to discourage the blanket conversion functions, 
though it is likely we'll need to retain a few of them, in favour of 
converting just the required fields at the point of use. This should be 
safe to do given that we have the ability to do compile time type 
checking - and lets try and include that in the tests that are always 
run before check in, to make sure that we don't land up with any 
mistakes. That would be a good addition to the tests I think,

Steve.