Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
Revisiting this... On 09/04/2019 13:35, Christoph Hellwig wrote: On Tue, Apr 09, 2019 at 01:21:23PM +0100, Steven Whitehouse wrote: Those conversion functions are not sensible, thats why we got rid of them from the kernel code. It is better to have a set of types that have the endianess specified so that we can use sparse. Compile time checking is always a good plan where it is possible. Yeah. And vs inttypes.h is no argument either, you can define the __be types based on the inttypes.h types (which really are stdint.h ones anyway). Linux's __be* type definitions are pulled in by standard headers (e.g. sys/stat.h via bits/statx.h -> linux/stat.h -> linux/types.h) so they can't be redefined in terms of stdint.h types, unfortunately. Even if that did work it could easily be broken outside of our control in the future, so that's another reason to leave the __be* types alone. So I think the best we can do is define some new bitwise-annotated types based on stdint.h and change the imported gfs2_ondisk.h to use them. Andy
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
On 09/04/2019 14:03, Steven Whitehouse wrote: On 09/04/2019 13:48, Andrew Price wrote: On 09/04/2019 13:21, Steven Whitehouse wrote: 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, Ah ok, that makes sense to me, thanks for explaining. I'm sure we could speed up bits of gfs2-utils by taking that approach too. Andy
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
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_ 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.
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
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_ 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. It is better to have a set of types that have the endianess specified so that we can use sparse. Compile time checking is always a good plan where it is possible. Okay, I'll add back the bitwise annotations through typedefs to stdint.h types in a new header but I don't want to name it linux/types.h to avoid picking up the wrong one, so I'll just change that #include in gfs2_ondisk.h. Andy
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
On Tue, Apr 09, 2019 at 01:21:23PM +0100, Steven Whitehouse wrote: > Those conversion functions are not sensible, thats why we got rid of them > from the kernel code. It is better to have a set of types that have the > endianess specified so that we can use sparse. Compile time checking is > always a good plan where it is possible. Yeah. And vs inttypes.h is no argument either, you can define the __be types based on the inttypes.h types (which really are stdint.h ones anyway).
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
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_ 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. It is better to have a set of types that have the endianess specified so that we can use sparse. Compile time checking is always a good plan where it is possible. Also keeping the file 1:1 the same is going to make your life much easier in the future.. It's really no difficulty to run the above substitutions the next time the file changes, but gfs2_ondisk.h changes once in a blue moon anyway so the maintenance overhead is going to be tiny. Andy Thats true, but lets keep the ability to do endianess checks, Steve.
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
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_ 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. Also keeping the file 1:1 the same is going to make your life much easier in the future.. It's really no difficulty to run the above substitutions the next time the file changes, but gfs2_ondisk.h changes once in a blue moon anyway so the maintenance overhead is going to be tiny. Andy
Re: [Cluster-devel] [PATCH] libgfs2: Import gfs2_ondisk.h
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_ 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? At least the be types give you really useful type checking with sparse, which can be trivially wired up in userspace as well. Also keeping the file 1:1 the same is going to make your life much easier in the future..
