[PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The in-memory bitmap code manipulates words and is therefore sensitive
to endianness, while the extent buffer bitmap code addresses bytes and
is byte-order agnostic. Because the byte addressing of the extent buffer
bitmaps is equivalent to a little-endian in-memory bitmap, the extent
buffer bitmap tests fail on big-endian systems.

34b3e6c92af1 ("Btrfs: self-tests: Fix extent buffer bitmap test fail on
BE system") worked around another endianness bug in the tests but missed
this one because ed9e4afdb055 ("Btrfs: self-tests: Execute page
straddling test only when nodesize < PAGE_SIZE") disables this part of
the test on ppc64. That change lost the original meaning of the test,
however. We really want to test that an equivalent series of operations
using the in-memory bitmap API and the extent buffer bitmap API produces
equivalent results.

To fix this, don't use memcmp_extent_buffer() or write_extent_buffer();
do everything bit-by-bit.

Reported-by: Anatoly Pugachev <matorola@gmail.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
This worked for me on a QEMU-emulated MIPS system. Anatoly, could you try this
out and let me know if this fixes your problem? Applies to v4.7-rc7.

 fs/btrfs/tests/extent-io-tests.c | 82 +++++++++++++++++++++++-----------------
 1 file changed, 48 insertions(+), 34 deletions(-)

diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index d19ab0317283..0207279a9fa9 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -273,20 +273,36 @@ out:
 	return ret;
 }
 
-/**
- * test_bit_in_byte - Determine whether a bit is set in a byte
- * @nr: bit number to test
- * @addr: Address to start counting from
- */
-static inline int test_bit_in_byte(int nr, const u8 *addr)
+static int check_eb_bitmap(unsigned long *bitmap, struct extent_buffer *eb,
+			   unsigned long len)
 {
-	return 1UL & (addr[nr / BITS_PER_BYTE] >> (nr & (BITS_PER_BYTE - 1)));
+	unsigned long i;
+
+	for (i = 0; i < len * BITS_PER_BYTE; i++) {
+		int bit, bit1;
+
+		bit = !!test_bit(i, bitmap);
+		bit1 = !!extent_buffer_test_bit(eb, 0, i);
+		if (bit1 != bit) {
+			test_msg("Bits do not match\n");
+			return -EINVAL;
+		}
+
+		bit1 = !!extent_buffer_test_bit(eb, i / BITS_PER_BYTE,
+						i % BITS_PER_BYTE);
+		if (bit1 != bit) {
+			test_msg("Offset bits do not match\n");
+			return -EINVAL;
+		}
+	}
+	return 0;
 }
 
 static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 			     unsigned long len)
 {
-	unsigned long i, x;
+	unsigned long i, j, x;
+	int ret;
 
 	memset(bitmap, 0, len);
 	memset_extent_buffer(eb, 0, 0, len);
@@ -297,16 +313,18 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 
 	bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
 	extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
-	if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
+	ret = check_eb_bitmap(bitmap, eb, len);
+	if (ret) {
 		test_msg("Setting all bits failed\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
 	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
-	if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
+	ret = check_eb_bitmap(bitmap, eb, len);
+	if (ret) {
 		test_msg("Clearing all bits failed\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	/* Straddling pages test */
@@ -316,9 +334,10 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 			sizeof(long) * BITS_PER_BYTE);
 		extent_buffer_bitmap_set(eb, PAGE_SIZE - sizeof(long) / 2, 0,
 					sizeof(long) * BITS_PER_BYTE);
-		if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
+		ret = check_eb_bitmap(bitmap, eb, len);
+		if (ret) {
 			test_msg("Setting straddling pages failed\n");
-			return -EINVAL;
+			return ret;
 		}
 
 		bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
@@ -328,9 +347,10 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 		extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
 		extent_buffer_bitmap_clear(eb, PAGE_SIZE - sizeof(long) / 2, 0,
 					sizeof(long) * BITS_PER_BYTE);
-		if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
+		ret = check_eb_bitmap(bitmap, eb, len);
+		if (ret) {
 			test_msg("Clearing straddling pages failed\n");
-			return -EINVAL;
+			return ret;
 		}
 	}
 
@@ -339,28 +359,22 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
 	 * something repetitive that could miss some hypothetical off-by-n bug.
 	 */
 	x = 0;
+	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
+	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
 	for (i = 0; i < len / sizeof(long); i++) {
 		x = (0x19660dULL * (u64)x + 0x3c6ef35fULL) & 0xffffffffUL;
-		bitmap[i] = x;
-	}
-	write_extent_buffer(eb, bitmap, 0, len);
-
-	for (i = 0; i < len * BITS_PER_BYTE; i++) {
-		int bit, bit1;
-
-		bit = !!test_bit_in_byte(i, (u8 *)bitmap);
-		bit1 = !!extent_buffer_test_bit(eb, 0, i);
-		if (bit1 != bit) {
-			test_msg("Testing bit pattern failed\n");
-			return -EINVAL;
+		for (j = 0; j < BITS_PER_LONG; j++) {
+			if (x & (1 << j)) {
+				bitmap_set(bitmap, i * sizeof(long) + j, 1);
+				extent_buffer_bitmap_set(eb, 0, i * sizeof(long) + j, 1);
+			}
 		}
+	}
 
-		bit1 = !!extent_buffer_test_bit(eb, i / BITS_PER_BYTE,
-						i % BITS_PER_BYTE);
-		if (bit1 != bit) {
-			test_msg("Testing bit pattern with offset failed\n");
-			return -EINVAL;
-		}
+	ret = check_eb_bitmap(bitmap, eb, len);
+	if (ret) {
+		test_msg("Random bit pattern failed\n");
+		return ret;
 	}
 
 	return 0;
-- 
2.9.0

Re: [PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[David Sterba]

On Tue, Jul 12, 2016 at 04:21:48PM -0700, Omar Sandoval wrote:
> +		for (j = 0; j < BITS_PER_LONG; j++) {
> +			if (x & (1 << j)) {

			if (x & (1UL << j)) {

Re: [PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[Feifei Xu]

On 2016/7/13 7:21, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> The in-memory bitmap code manipulates words and is therefore sensitive
> to endianness, while the extent buffer bitmap code addresses bytes and
> is byte-order agnostic. Because the byte addressing of the extent buffer
> bitmaps is equivalent to a little-endian in-memory bitmap, the extent
> buffer bitmap tests fail on big-endian systems.
>
> 34b3e6c92af1 ("Btrfs: self-tests: Fix extent buffer bitmap test fail on
> BE system") worked around another endianness bug in the tests but missed
> this one because ed9e4afdb055 ("Btrfs: self-tests: Execute page
> straddling test only when nodesize < PAGE_SIZE") disables this part of
> the test on ppc64. That change lost the original meaning of the test,
> however. We really want to test that an equivalent series of operations
> using the in-memory bitmap API and the extent buffer bitmap API produces
> equivalent results.
>
> To fix this, don't use memcmp_extent_buffer() or write_extent_buffer();
> do everything bit-by-bit.
>
> Reported-by: Anatoly Pugachev <matorola@gmail.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> This worked for me on a QEMU-emulated MIPS system. Anatoly, could you try this
> out and let me know if this fixes your problem? Applies to v4.7-rc7.
>
>   fs/btrfs/tests/extent-io-tests.c | 82 +++++++++++++++++++++++-----------------
>   1 file changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
> index d19ab0317283..0207279a9fa9 100644
> --- a/fs/btrfs/tests/extent-io-tests.c
> +++ b/fs/btrfs/tests/extent-io-tests.c
> @@ -273,20 +273,36 @@ out:
>   	return ret;
>   }
>   
> -/**
> - * test_bit_in_byte - Determine whether a bit is set in a byte
> - * @nr: bit number to test
> - * @addr: Address to start counting from
> - */
> -static inline int test_bit_in_byte(int nr, const u8 *addr)
> +static int check_eb_bitmap(unsigned long *bitmap, struct extent_buffer *eb,
> +			   unsigned long len)
>   {
> -	return 1UL & (addr[nr / BITS_PER_BYTE] >> (nr & (BITS_PER_BYTE - 1)));
> +	unsigned long i;
> +
> +	for (i = 0; i < len * BITS_PER_BYTE; i++) {
> +		int bit, bit1;
> +
> +		bit = !!test_bit(i, bitmap);
> +		bit1 = !!extent_buffer_test_bit(eb, 0, i);
> +		if (bit1 != bit) {
> +			test_msg("Bits do not match\n");
> +			return -EINVAL;
> +		}
> +
> +		bit1 = !!extent_buffer_test_bit(eb, i / BITS_PER_BYTE,
> +						i % BITS_PER_BYTE);
> +		if (bit1 != bit) {
> +			test_msg("Offset bits do not match\n");
> +			return -EINVAL;
> +		}
> +	}
> +	return 0;
>   }
>   
>   static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
>   			     unsigned long len)
>   {
> -	unsigned long i, x;
> +	unsigned long i, j, x;
> +	int ret;
>   
>   	memset(bitmap, 0, len);
>   	memset_extent_buffer(eb, 0, 0, len);
> @@ -297,16 +313,18 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
>   
>   	bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
>   	extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
> -	if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
> +	ret = check_eb_bitmap(bitmap, eb, len);
> +	if (ret) {
>   		test_msg("Setting all bits failed\n");
> -		return -EINVAL;
> +		return ret;
>   	}
>   
>   	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
>   	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
> -	if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
> +	ret = check_eb_bitmap(bitmap, eb, len);
> +	if (ret) {
>   		test_msg("Clearing all bits failed\n");
> -		return -EINVAL;
> +		return ret;
>   	}
>   
>   	/* Straddling pages test */
> @@ -316,9 +334,10 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
>   			sizeof(long) * BITS_PER_BYTE);
>   		extent_buffer_bitmap_set(eb, PAGE_SIZE - sizeof(long) / 2, 0,
>   					sizeof(long) * BITS_PER_BYTE);
> -		if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
> +		ret = check_eb_bitmap(bitmap, eb, len);
> +		if (ret) {
>   			test_msg("Setting straddling pages failed\n");
> -			return -EINVAL;
> +			return ret;
>   		}
>   
>   		bitmap_set(bitmap, 0, len * BITS_PER_BYTE);
> @@ -328,9 +347,10 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
>   		extent_buffer_bitmap_set(eb, 0, 0, len * BITS_PER_BYTE);
>   		extent_buffer_bitmap_clear(eb, PAGE_SIZE - sizeof(long) / 2, 0,
>   					sizeof(long) * BITS_PER_BYTE);
> -		if (memcmp_extent_buffer(eb, bitmap, 0, len) != 0) {
> +		ret = check_eb_bitmap(bitmap, eb, len);
> +		if (ret) {
>   			test_msg("Clearing straddling pages failed\n");
> -			return -EINVAL;
> +			return ret;
>   		}
>   	}
>   
> @@ -339,28 +359,22 @@ static int __test_eb_bitmaps(unsigned long *bitmap, struct extent_buffer *eb,
>   	 * something repetitive that could miss some hypothetical off-by-n bug.
>   	 */
>   	x = 0;
> +	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
> +	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
>   	for (i = 0; i < len / sizeof(long); i++) {
>   		x = (0x19660dULL * (u64)x + 0x3c6ef35fULL) & 0xffffffffUL;
> -		bitmap[i] = x;
> -	}
> -	write_extent_buffer(eb, bitmap, 0, len);
> -
> -	for (i = 0; i < len * BITS_PER_BYTE; i++) {
> -		int bit, bit1;
> -
> -		bit = !!test_bit_in_byte(i, (u8 *)bitmap);
> -		bit1 = !!extent_buffer_test_bit(eb, 0, i);
> -		if (bit1 != bit) {
> -			test_msg("Testing bit pattern failed\n");
> -			return -EINVAL;
> +		for (j = 0; j < BITS_PER_LONG; j++) {
> +			if (x & (1 << j)) {
> +				bitmap_set(bitmap, i * sizeof(long) + j, 1);
> +				extent_buffer_bitmap_set(eb, 0, i * sizeof(long) + j, 1);
> +			}
>   		}
> +	}
>   
> -		bit1 = !!extent_buffer_test_bit(eb, i / BITS_PER_BYTE,
> -						i % BITS_PER_BYTE);
> -		if (bit1 != bit) {
> -			test_msg("Testing bit pattern with offset failed\n");
> -			return -EINVAL;
> -		}
> +	ret = check_eb_bitmap(bitmap, eb, len);
> +	if (ret) {
> +		test_msg("Random bit pattern failed\n");
> +		return ret;
>   	}
>   
>   	return 0;
Thanks for fixing the straddling pages' extent buffer bitmaps tests on 
16K page big-endian system.
Also test pass on 64K page big-endian system (ppc64 BE).

Thanks
Fiona

Re: [PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[Anatoly Pugachev]

On Wed, Jul 13, 2016 at 2:21 AM, Omar Sandoval <osandov@osandov.com> wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> The in-memory bitmap code manipulates words and is therefore sensitive
> to endianness, while the extent buffer bitmap code addresses bytes and
> is byte-order agnostic. Because the byte addressing of the extent buffer
> bitmaps is equivalent to a little-endian in-memory bitmap, the extent
> buffer bitmap tests fail on big-endian systems.
>
> 34b3e6c92af1 ("Btrfs: self-tests: Fix extent buffer bitmap test fail on
> BE system") worked around another endianness bug in the tests but missed
> this one because ed9e4afdb055 ("Btrfs: self-tests: Execute page
> straddling test only when nodesize < PAGE_SIZE") disables this part of
> the test on ppc64. That change lost the original meaning of the test,
> however. We really want to test that an equivalent series of operations
> using the in-memory bitmap API and the extent buffer bitmap API produces
> equivalent results.
>
> To fix this, don't use memcmp_extent_buffer() or write_extent_buffer();
> do everything bit-by-bit.


Just tested patched kernel, able to load btrfs module and mount fs.
Thanks a lot!

Re: [PATCH] Btrfs: fix extent buffer bitmap tests on big-endian systems

[Anatoly Pugachev]

On Wed, Jul 13, 2016 at 2:21 AM, Omar Sandoval <osandov@osandov.com> wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> The in-memory bitmap code manipulates words and is therefore sensitive
> to endianness, while the extent buffer bitmap code addresses bytes and
> is byte-order agnostic. Because the byte addressing of the extent buffer
> bitmaps is equivalent to a little-endian in-memory bitmap, the extent
> buffer bitmap tests fail on big-endian systems.
>
> 34b3e6c92af1 ("Btrfs: self-tests: Fix extent buffer bitmap test fail on
> BE system") worked around another endianness bug in the tests but missed
> this one because ed9e4afdb055 ("Btrfs: self-tests: Execute page
> straddling test only when nodesize < PAGE_SIZE") disables this part of
> the test on ppc64. That change lost the original meaning of the test,
> however. We really want to test that an equivalent series of operations
> using the in-memory bitmap API and the extent buffer bitmap API produces
> equivalent results.
>
> To fix this, don't use memcmp_extent_buffer() or write_extent_buffer();
> do everything bit-by-bit.


Just tested patched kernel, able to load btrfs module and mount fs.
Thanks a lot!