ext4 build errors

Message ID 1506954181.985.9.camel@infinera.com
State Rejected
Headers show
Series
  • ext4 build errors
Related show

Commit Message

Joakim Tjernlund Oct. 2, 2017, 2:23 p.m.
Hi ext4 devs

Adding the patch last in this mail cause lots of build errors in ext4, here is a few:

fs/ext4/balloc.c: In function ‘ext4_init_block_bitmap’:
fs/ext4/balloc.c:215:21: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   ext4_set_bit(bit, bh->b_data);
                     ^
In file included from ./arch/x86/include/asm/bitops.h:517:0,
                 from ./include/linux/bitops.h:36,
                 from ./include/linux/kernel.h:10,
                 from ./include/linux/list.h:8,
                 from ./include/linux/preempt.h:10,
                 from ./include/linux/spinlock.h:50,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from fs/ext4/balloc.c:14:
./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’
 static inline void __set_bit_le(int nr, unsigned long *addr)
                    ^
fs/ext4/balloc.c:225:44: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data);
                                            ^
In file included from ./arch/x86/include/asm/bitops.h:517:0,
                 from ./include/linux/bitops.h:36,
                 from ./include/linux/kernel.h:10,
                 from ./include/linux/list.h:8,
                 from ./include/linux/preempt.h:10,
                 from ./include/linux/spinlock.h:50,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from fs/ext4/balloc.c:14:
./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’
 static inline void __set_bit_le(int nr, unsigned long *addr)
                    ^
fs/ext4/balloc.c:229:44: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types]
   ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data);
                                            ^
In file included from ./arch/x86/include/asm/bitops.h:517:0,
                 from ./include/linux/bitops.h:36,
                 from ./include/linux/kernel.h:10,
                 from ./include/linux/list.h:8,
                 from ./include/linux/preempt.h:10,
                 from ./include/linux/spinlock.h:50,
                 from ./include/linux/seqlock.h:35,
                 from ./include/linux/time.h:5,
                 from fs/ext4/balloc.c:14:
./include/asm-generic/bitops/le.h:71:20: note: expected ‘long unsigned int *’ but argument is of type ‘char *’
 static inline void __set_bit_le(int nr, unsigned long *addr)
                    ^
fs/ext4/balloc.c:235:45: error: passing argument 2 of ‘__set_bit_le’ from incompatible pointer type [-Werror=incompatible-pointer-types]
    ext4_set_bit(EXT4_B2C(sbi, tmp - start), bh->b_data);

I think ext4 needs some cleanup before my patch can be applied, what do you think?

 Jocke

From 18c2d4659a33ac047b31a0e33c524811a2a8f642 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>

Date: Thu, 9 Mar 2017 14:19:41 +0100
Subject: [PATCH 1/4] x86: Match bitops prototypes

Adjust bitops function prototype in arch/x86/include/asm/le.h
to match the generic ones in asm-generic/bitops/le.h
That is, replace void* with unsigned long*
---
 include/asm-generic/bitops/le.h | 42 ++++++++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 19 deletions(-)

-- 
2.10.2

Comments

Theodore Ts'o Oct. 2, 2017, 2:55 p.m. | #1
On Mon, Oct 02, 2017 at 02:23:02PM +0000, Joakim Tjernlund wrote:
> Hi ext4 devs
> 
> Adding the patch last in this mail cause lots of build errors in ext4, here is a few:

Why did you need this patch to fix problems in VirtualBox?

Cleaning this up is going to be a little tricky, because one of the
implications the void * declaration in the __set_bit_le() declaration
is that there isn't any particular alignment requirement with the __le
functions.  But the long * declaration implies that the bitmaps have
to be aligned to sizeof(long).

For the ext4 bitmap, we use it on bh->b_data, for which we can safely
assume is long-aligned.  But the mballoc buddy bitmaps use
mb_set_bit() in ways that are _not_ guaranteed to be long aligned.

So fixing this is going to be a bit painful, and will likely result in
a performance regression for ext4.  We can make our own version that
open codes it as C functions --- but then we lose all of the
architecture optimized bitop functions.

I believe the reason why the standard bitop functions are made long *
aligned is that on some BE architectures --- I suspect it was PowerPC
but I'm not 100% sure about that --- the native bitop functions
required a long * alignment.  Fortunately all of the little endian
architectures didn't have these alignment restrictions, so we could
keep the __set_bit_le functions to not have any long alignment
restrictions.

The fact that bitop and the bitop_le functions are not the same
is... inelegant, but if it represents a practical optimization that is
possible on LE systems but not on BE systems (where bitop_le gets open
coded in C, in an inefficient way, but oh, well, BE systems aren't for
the cool kids anyway :-), I have to ask whether it's really worth it
to do the cleanup.

Cheers,

					- Ted
Joakim Tjernlund Oct. 2, 2017, 3:15 p.m. | #2
On Mon, 2017-10-02 at 10:55 -0400, Theodore Ts'o wrote:
> On Mon, Oct 02, 2017 at 02:23:02PM +0000, Joakim Tjernlund wrote:
> > Hi ext4 devs
> > 
> > Adding the patch last in this mail cause lots of build errors in ext4, here is a few:
> 
> Why did you need this patch to fix problems in VirtualBox?

Earlier VirtualBox had some c++ code that included kernel header and g++ is
much pickier about these things so I had to do some cleanups to build
some of the VirtualBox modules. I think it is better now but I haven't really checked.

> 
> Cleaning this up is going to be a little tricky, because one of the
> implications the void * declaration in the __set_bit_le() declaration
> is that there isn't any particular alignment requirement with the __le
> functions.  But the long * declaration implies that the bitmaps have
> to be aligned to sizeof(long).
> 
> For the ext4 bitmap, we use it on bh->b_data, for which we can safely
> assume is long-aligned.  But the mballoc buddy bitmaps use
> mb_set_bit() in ways that are _not_ guaranteed to be long aligned.
> 
> So fixing this is going to be a bit painful, and will likely result in
> a performance regression for ext4.  We can make our own version that
> open codes it as C functions --- but then we lose all of the
> architecture optimized bitop functions.
> 
> I believe the reason why the standard bitop functions are made long *
> aligned is that on some BE architectures --- I suspect it was PowerPC
> but I'm not 100% sure about that --- the native bitop functions
> required a long * alignment.  Fortunately all of the little endian
> architectures didn't have these alignment restrictions, so we could
> keep the __set_bit_le functions to not have any long alignment
> restrictions.

If this is a special case for ext4, can you not just do an explicit
type cast in ext4 code? 

> 
> The fact that bitop and the bitop_le functions are not the same
> is... inelegant, but if it represents a practical optimization that is
> possible on LE systems but not on BE systems (where bitop_le gets open
> coded in C, in an inefficient way, but oh, well, BE systems aren't for
> the cool kids anyway :-), I have to ask whether it's really worth it
> to do the cleanup.

I see, but by using void * you also loose type checking w.r.t size so
if you by mistake use an u32, you will not notice.

 Jocke
Theodore Ts'o Oct. 2, 2017, 4:54 p.m. | #3
On Mon, Oct 02, 2017 at 03:15:33PM +0000, Joakim Tjernlund wrote:
> > I believe the reason why the standard bitop functions are made long *
> > aligned is that on some BE architectures --- I suspect it was PowerPC
> > but I'm not 100% sure about that --- the native bitop functions
> > required a long * alignment.  Fortunately all of the little endian
> > architectures didn't have these alignment restrictions, so we could
> > keep the __set_bit_le functions to not have any long alignment
> > restrictions.
> 
> If this is a special case for ext4, can you not just do an explicit
> type cast in ext4 code?

Sure, it would be safe *today*, but then in the future someone might
change an implementation of the bitop_le* functions for some
architecture which would not tolerate unaligned pointers (since using
a long * would imply this is allowed), and then things would break.

> > The fact that bitop and the bitop_le functions are not the same
> > is... inelegant, but if it represents a practical optimization that is
> > possible on LE systems but not on BE systems (where bitop_le gets open
> > coded in C, in an inefficient way, but oh, well, BE systems aren't for
> > the cool kids anyway :-), I have to ask whether it's really worth it
> > to do the cleanup.
> 
> I see, but by using void * you also loose type checking w.r.t size so
> if you by mistake use an u32, you will not notice.

Um, we're never using a u32.  We're using a pointer into a bit array
which is often far larger than 32 or 64 bits.  For example, when we
use a 4k block size, then bh->b_data is a bit array which is 4096*8 ==
32,768 bits.

This is why void * is the right thing --- it's not a u32 or a long.
It's a bit array.  And in the case of the mb buddy bitmap, it's not
necessarily going to start on a a byte boundary which is a multiple of
4 or 8.

					- Ted
Joakim Tjernlund Oct. 2, 2017, 5:27 p.m. | #4
On Mon, 2017-10-02 at 12:54 -0400, Theodore Ts'o wrote:
> On Mon, Oct 02, 2017 at 03:15:33PM +0000, Joakim Tjernlund wrote:
> > > I believe the reason why the standard bitop functions are made long *
> > > aligned is that on some BE architectures --- I suspect it was PowerPC
> > > but I'm not 100% sure about that --- the native bitop functions
> > > required a long * alignment.  Fortunately all of the little endian
> > > architectures didn't have these alignment restrictions, so we could
> > > keep the __set_bit_le functions to not have any long alignment
> > > restrictions.
> > 
> > If this is a special case for ext4, can you not just do an explicit
> > type cast in ext4 code?
> 
> Sure, it would be safe *today*, but then in the future someone might
> change an implementation of the bitop_le* functions for some
> architecture which would not tolerate unaligned pointers (since using
> a long * would imply this is allowed), and then things would break.

I guess I am missing something, the __set_bit_le calls __set_bit(nr ^ BITOP_LE_SWIZZLE, addr):
static inline void __set_bit_le(int nr, void *addr)
{
	__set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
}

and __set_bit assumes it is working on a long, why is is safe in __set_bit_le to cast
the void * into a long * but not in ext4 code?

> 
> > > The fact that bitop and the bitop_le functions are not the same
> > > is... inelegant, but if it represents a practical optimization that is
> > > possible on LE systems but not on BE systems (where bitop_le gets open
> > > coded in C, in an inefficient way, but oh, well, BE systems aren't for
> > > the cool kids anyway :-), I have to ask whether it's really worth it
> > > to do the cleanup.
> > 
> > I see, but by using void * you also loose type checking w.r.t size so
> > if you by mistake use an u32, you will not notice.
> 
> Um, we're never using a u32.  We're using a pointer into a bit array
> which is often far larger than 32 or 64 bits.  For example, when we
> use a 4k block size, then bh->b_data is a bit array which is 4096*8 ==
> 32,768 bits.
> 
> This is why void * is the right thing --- it's not a u32 or a long.
> It's a bit array.  And in the case of the mb buddy bitmap, it's not
> necessarily going to start on a a byte boundary which is a multiple of
> 4 or 8.

For ext4 it might be right but I was using "you" in a wider scope,
the rest of kernel src.

 Jocke
Theodore Ts'o Oct. 2, 2017, 6:40 p.m. | #5
On Mon, Oct 02, 2017 at 05:27:12PM +0000, Joakim Tjernlund wrote:
> > This is why void * is the right thing --- it's not a u32 or a long.
> > It's a bit array.  And in the case of the mb buddy bitmap, it's not
> > necessarily going to start on a a byte boundary which is a multiple of
> > 4 or 8.
> 
> For ext4 it might be right but I was using "you" in a wider scope,
> the rest of kernel src.

So let's take this up to a high level of the problem.  For the bitops
functions, there are two sorts of users.  Those that want to operate
on an integer type (either a u32 or a long), and those that operate on
bitarrays.  Most of the integer users are using the bitops for
in-memory state flags.  Most of the bitarray use cases are for things
like ext4's allocation bitmaps, where the on-disk format needs to be
portable across architectures --- and that's where bitops_le tends to
be used.

Taking a quick look at the output of "git grep set_bit_le", most of
the users are just like ext4, where it's being used for a bitarray.
So my argument I think *does* apply much more widely.

      	       	       	      	    	 - Ted
Joakim Tjernlund Oct. 2, 2017, 8:12 p.m. | #6
On Mon, 2017-10-02 at 14:40 -0400, Theodore Ts'o wrote:
> On Mon, Oct 02, 2017 at 05:27:12PM +0000, Joakim Tjernlund wrote:
> > > This is why void * is the right thing --- it's not a u32 or a long.
> > > It's a bit array.  And in the case of the mb buddy bitmap, it's not
> > > necessarily going to start on a a byte boundary which is a multiple of
> > > 4 or 8.
> > 
> > For ext4 it might be right but I was using "you" in a wider scope,
> > the rest of kernel src.
> 
> So let's take this up to a high level of the problem.  For the bitops
> functions, there are two sorts of users.  Those that want to operate
> on an integer type (either a u32 or a long), and those that operate on
> bitarrays.  Most of the integer users are using the bitops for
> in-memory state flags.  Most of the bitarray use cases are for things
> like ext4's allocation bitmaps, where the on-disk format needs to be
> portable across architectures --- and that's where bitops_le tends to
> be used.
> 
> Taking a quick look at the output of "git grep set_bit_le", most of
> the users are just like ext4, where it's being used for a bitarray.
> So my argument I think *does* apply much more widely.

OK, I can see that. It is just that __set_bit_le is impl. using 
__set_bit(long nr, volatile unsigned long *addr) and that implies, to me,
that the memory should be long aligned. 

 Jocke

Patch

diff --git a/include/asm-generic/bitops/le.h b/include/asm-generic/bitops/le.h
index 6173154..92a3afa 100644
--- a/include/asm-generic/bitops/le.h
+++ b/include/asm-generic/bitops/le.h
@@ -8,20 +8,22 @@ 
 
 #define BITOP_LE_SWIZZLE	0
 
-static inline unsigned long find_next_zero_bit_le(const void *addr,
-		unsigned long size, unsigned long offset)
+static inline unsigned long find_next_zero_bit_le(const unsigned long *addr,
+                                                  unsigned long size,
+                                                  unsigned long offset)
 {
 	return find_next_zero_bit(addr, size, offset);
 }
 
-static inline unsigned long find_next_bit_le(const void *addr,
-		unsigned long size, unsigned long offset)
+static inline unsigned long find_next_bit_le(const unsigned long *addr,
+                                             unsigned long size,
+                                             unsigned long offset)
 {
 	return find_next_bit(addr, size, offset);
 }
 
-static inline unsigned long find_first_zero_bit_le(const void *addr,
-		unsigned long size)
+static inline unsigned long find_first_zero_bit_le(const unsigned long *addr,
+                                                   unsigned long size)
 {
 	return find_first_zero_bit(addr, size);
 }
@@ -31,13 +33,15 @@  static inline unsigned long find_first_zero_bit_le(const void *addr,
 #define BITOP_LE_SWIZZLE	((BITS_PER_LONG-1) & ~0x7)
 
 #ifndef find_next_zero_bit_le
-extern unsigned long find_next_zero_bit_le(const void *addr,
-		unsigned long size, unsigned long offset);
+extern unsigned long find_next_zero_bit_le(const unsigned long *addr,
+                                           unsigned long size,
+                                           unsigned long offset);
 #endif
 
 #ifndef find_next_bit_le
-extern unsigned long find_next_bit_le(const void *addr,
-		unsigned long size, unsigned long offset);
+extern unsigned long find_next_bit_le(const unsigned long *addr,
+                                      unsigned long size,
+                                      unsigned long offset);
 #endif
 
 #ifndef find_first_zero_bit_le
@@ -49,47 +53,47 @@  extern unsigned long find_next_bit_le(const void *addr,
 #error "Please fix <asm/byteorder.h>"
 #endif
 
-static inline int test_bit_le(int nr, const void *addr)
+static inline int test_bit_le(int nr, const unsigned long *addr)
 {
 	return test_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline void set_bit_le(int nr, void *addr)
+static inline void set_bit_le(int nr, unsigned long *addr)
 {
 	set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline void clear_bit_le(int nr, void *addr)
+static inline void clear_bit_le(int nr, unsigned long *addr)
 {
 	clear_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline void __set_bit_le(int nr, void *addr)
+static inline void __set_bit_le(int nr, unsigned long *addr)
 {
 	__set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline void __clear_bit_le(int nr, void *addr)
+static inline void __clear_bit_le(int nr, unsigned long *addr)
 {
 	__clear_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline int test_and_set_bit_le(int nr, void *addr)
+static inline int test_and_set_bit_le(int nr, unsigned long *addr)
 {
 	return test_and_set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline int test_and_clear_bit_le(int nr, void *addr)
+static inline int test_and_clear_bit_le(int nr, unsigned long *addr)
 {
 	return test_and_clear_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline int __test_and_set_bit_le(int nr, void *addr)
+static inline int __test_and_set_bit_le(int nr, unsigned long *addr)
 {
 	return __test_and_set_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }
 
-static inline int __test_and_clear_bit_le(int nr, void *addr)
+static inline int __test_and_clear_bit_le(int nr, unsigned long *addr)
 {
 	return __test_and_clear_bit(nr ^ BITOP_LE_SWIZZLE, addr);
 }