[PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

So it turns out that the free space tree bitmap handling has always been
broken on big-endian systems. Totally my bad.

Patch 1 fixes this. Technically, it's a disk format change for
big-endian systems, but it never could have worked before, so I won't go
through the trouble of any incompat bits. If you've somehow been using
space_cache=v2 on a big-endian system (I doubt anyone is), you're going
to want to mount with nospace_cache to clear it and wait for this to go
in.

Patch 2 fixes a similar error in the sanity tests (it's the same as the
v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
oversight that patch 1 fixes.

Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
pass on x86_64 and MIPS.

Thanks!

1: http://thread.gmane.org/gmane.comp.file-systems.btrfs/58329

Omar Sandoval (3):
  Btrfs: fix free space tree bitmaps on big-endian systems
  Btrfs: fix extent buffer bitmap tests on big-endian systems
  Btrfs: expand free space tree sanity tests to catch endianness bug

 fs/btrfs/extent_io.c                   |  64 +++++++++----
 fs/btrfs/extent_io.h                   |  22 +++++
 fs/btrfs/free-space-tree.c             |  17 ++--
 fs/btrfs/tests/extent-io-tests.c       |  87 +++++++++--------
 fs/btrfs/tests/free-space-tree-tests.c | 164 +++++++++++++++++++--------------
 5 files changed, 223 insertions(+), 131 deletions(-)

-- 
2.9.0

[PATCH 1/3] Btrfs: fix free space tree bitmaps on big-endian systems

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

In convert_free_space_to_{bitmaps,extents}(), we buffer the free space
bitmaps in memory and copy them directly to/from the extent buffers with
{read,write}_extent_buffer(). The extent buffer bitmap helpers use byte
granularity, which is equivalent to a little-endian bitmap. This means
that on big-endian systems, the in-memory bitmaps will be written to
disk byte-swapped. To fix this, use byte-granularity for the bitmaps in
memory.

Cc: stable@vger.kernel.org
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/extent_io.c       | 64 +++++++++++++++++++++++++++++++++-------------
 fs/btrfs/extent_io.h       | 22 ++++++++++++++++
 fs/btrfs/free-space-tree.c | 17 ++++++------
 3 files changed, 76 insertions(+), 27 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 75533adef998..bdb43882791a 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5514,17 +5514,45 @@ void copy_extent_buffer(struct extent_buffer *dst, struct extent_buffer *src,
 	}
 }
 
-/*
- * The extent buffer bitmap operations are done with byte granularity because
- * bitmap items are not guaranteed to be aligned to a word and therefore a
- * single word in a bitmap may straddle two pages in the extent buffer.
- */
-#define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
-#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
-#define BITMAP_FIRST_BYTE_MASK(start) \
-	((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
-#define BITMAP_LAST_BYTE_MASK(nbits) \
-	(BYTE_MASK >> (-(nbits) & (BITS_PER_BYTE - 1)))
+void le_bitmap_set(u8 *map, unsigned int start, int len)
+{
+	u8 *p = map + BIT_BYTE(start);
+	const unsigned int size = start + len;
+	int bits_to_set = BITS_PER_BYTE - (start % BITS_PER_BYTE);
+	u8 mask_to_set = BITMAP_FIRST_BYTE_MASK(start);
+
+	while (len - bits_to_set >= 0) {
+		*p |= mask_to_set;
+		len -= bits_to_set;
+		bits_to_set = BITS_PER_BYTE;
+		mask_to_set = ~(u8)0;
+		p++;
+	}
+	if (len) {
+		mask_to_set &= BITMAP_LAST_BYTE_MASK(size);
+		*p |= mask_to_set;
+	}
+}
+
+void le_bitmap_clear(u8 *map, unsigned int start, int len)
+{
+	u8 *p = map + BIT_BYTE(start);
+	const unsigned int size = start + len;
+	int bits_to_clear = BITS_PER_BYTE - (start % BITS_PER_BYTE);
+	u8 mask_to_clear = BITMAP_FIRST_BYTE_MASK(start);
+
+	while (len - bits_to_clear >= 0) {
+		*p &= ~mask_to_clear;
+		len -= bits_to_clear;
+		bits_to_clear = BITS_PER_BYTE;
+		mask_to_clear = ~(u8)0;
+		p++;
+	}
+	if (len) {
+		mask_to_clear &= BITMAP_LAST_BYTE_MASK(size);
+		*p &= ~mask_to_clear;
+	}
+}
 
 /*
  * eb_bitmap_offset() - calculate the page and offset of the byte containing the
@@ -5568,7 +5596,7 @@ static inline void eb_bitmap_offset(struct extent_buffer *eb,
 int extent_buffer_test_bit(struct extent_buffer *eb, unsigned long start,
 			   unsigned long nr)
 {
-	char *kaddr;
+	u8 *kaddr;
 	struct page *page;
 	unsigned long i;
 	size_t offset;
@@ -5590,13 +5618,13 @@ int extent_buffer_test_bit(struct extent_buffer *eb, unsigned long start,
 void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
 			      unsigned long pos, unsigned long len)
 {
-	char *kaddr;
+	u8 *kaddr;
 	struct page *page;
 	unsigned long i;
 	size_t offset;
 	const unsigned int size = pos + len;
 	int bits_to_set = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
-	unsigned int mask_to_set = BITMAP_FIRST_BYTE_MASK(pos);
+	u8 mask_to_set = BITMAP_FIRST_BYTE_MASK(pos);
 
 	eb_bitmap_offset(eb, start, pos, &i, &offset);
 	page = eb->pages[i];
@@ -5607,7 +5635,7 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
 		kaddr[offset] |= mask_to_set;
 		len -= bits_to_set;
 		bits_to_set = BITS_PER_BYTE;
-		mask_to_set = ~0U;
+		mask_to_set = ~(u8)0;
 		if (++offset >= PAGE_SIZE && len > 0) {
 			offset = 0;
 			page = eb->pages[++i];
@@ -5632,13 +5660,13 @@ void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
 void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
 				unsigned long pos, unsigned long len)
 {
-	char *kaddr;
+	u8 *kaddr;
 	struct page *page;
 	unsigned long i;
 	size_t offset;
 	const unsigned int size = pos + len;
 	int bits_to_clear = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
-	unsigned int mask_to_clear = BITMAP_FIRST_BYTE_MASK(pos);
+	u8 mask_to_clear = BITMAP_FIRST_BYTE_MASK(pos);
 
 	eb_bitmap_offset(eb, start, pos, &i, &offset);
 	page = eb->pages[i];
@@ -5649,7 +5677,7 @@ void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
 		kaddr[offset] &= ~mask_to_clear;
 		len -= bits_to_clear;
 		bits_to_clear = BITS_PER_BYTE;
-		mask_to_clear = ~0U;
+		mask_to_clear = ~(u8)0;
 		if (++offset >= PAGE_SIZE && len > 0) {
 			offset = 0;
 			page = eb->pages[++i];
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index c0c1c4fef6ce..d19010729468 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -58,6 +58,28 @@
  */
 #define EXTENT_PAGE_PRIVATE 1
 
+/*
+ * The extent buffer bitmap operations are done with byte granularity instead of
+ * word granularity for two reasons:
+ * 1. The bitmaps must be little-endian on disk.
+ * 2. Bitmap items are not guaranteed to be aligned to a word and therefore a
+ *    single word in a bitmap may straddle two pages in the extent buffer.
+ */
+#define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
+#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
+#define BITMAP_FIRST_BYTE_MASK(start) \
+	((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
+#define BITMAP_LAST_BYTE_MASK(nbits) \
+	(BYTE_MASK >> (-(nbits) & (BITS_PER_BYTE - 1)))
+
+static inline int le_test_bit(int nr, const u8 *addr)
+{
+	return 1U & (addr[BIT_BYTE(nr)] >> (nr & (BITS_PER_BYTE-1)));
+}
+
+extern void le_bitmap_set(u8 *map, unsigned int start, int len);
+extern void le_bitmap_clear(u8 *map, unsigned int start, int len);
+
 struct extent_state;
 struct btrfs_root;
 struct btrfs_io_bio;
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 53dbeaf6ce94..68ce45a2b763 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -151,7 +151,7 @@ static inline u32 free_space_bitmap_size(u64 size, u32 sectorsize)
 	return DIV_ROUND_UP((u32)div_u64(size, sectorsize), BITS_PER_BYTE);
 }
 
-static unsigned long *alloc_bitmap(u32 bitmap_size)
+static u8 *alloc_bitmap(u32 bitmap_size)
 {
 	void *mem;
 
@@ -180,8 +180,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 	struct btrfs_free_space_info *info;
 	struct btrfs_key key, found_key;
 	struct extent_buffer *leaf;
-	unsigned long *bitmap;
-	char *bitmap_cursor;
+	u8 *bitmap, *bitmap_cursor;
 	u64 start, end;
 	u64 bitmap_range, i;
 	u32 bitmap_size, flags, expected_extent_count;
@@ -231,7 +230,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 						block_group->sectorsize);
 				last = div_u64(found_key.objectid + found_key.offset - start,
 					       block_group->sectorsize);
-				bitmap_set(bitmap, first, last - first);
+				le_bitmap_set(bitmap, first, last - first);
 
 				extent_count++;
 				nr++;
@@ -269,7 +268,7 @@ int convert_free_space_to_bitmaps(struct btrfs_trans_handle *trans,
 		goto out;
 	}
 
-	bitmap_cursor = (char *)bitmap;
+	bitmap_cursor = bitmap;
 	bitmap_range = block_group->sectorsize * BTRFS_FREE_SPACE_BITMAP_BITS;
 	i = start;
 	while (i < end) {
@@ -318,7 +317,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 	struct btrfs_free_space_info *info;
 	struct btrfs_key key, found_key;
 	struct extent_buffer *leaf;
-	unsigned long *bitmap;
+	u8 *bitmap;
 	u64 start, end;
 	/* Initialize to silence GCC. */
 	u64 extent_start = 0;
@@ -362,7 +361,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 				break;
 			} else if (found_key.type == BTRFS_FREE_SPACE_BITMAP_KEY) {
 				unsigned long ptr;
-				char *bitmap_cursor;
+				u8 *bitmap_cursor;
 				u32 bitmap_pos, data_size;
 
 				ASSERT(found_key.objectid >= start);
@@ -372,7 +371,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 				bitmap_pos = div_u64(found_key.objectid - start,
 						     block_group->sectorsize *
 						     BITS_PER_BYTE);
-				bitmap_cursor = ((char *)bitmap) + bitmap_pos;
+				bitmap_cursor = bitmap + bitmap_pos;
 				data_size = free_space_bitmap_size(found_key.offset,
 								   block_group->sectorsize);
 
@@ -409,7 +408,7 @@ int convert_free_space_to_extents(struct btrfs_trans_handle *trans,
 	offset = start;
 	bitnr = 0;
 	while (offset < end) {
-		bit = !!test_bit(bitnr, bitmap);
+		bit = !!le_test_bit(bitnr, bitmap);
 		if (prev_bit == 0 && bit == 1) {
 			extent_start = offset;
 		} else if (prev_bit == 1 && bit == 0) {
-- 
2.9.0

[PATCH 2/3] 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>
Tested-by: Anatoly Pugachev <matorola@gmail.com>
Tested-by: Feifei Xu <xufeifei@linux.vnet.ibm.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/tests/extent-io-tests.c | 87 +++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 36 deletions(-)

diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index d19ab0317283..caad80bb9bd0 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -273,20 +273,37 @@ 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;
+	u32 x;
+	int ret;
 
 	memset(bitmap, 0, len);
 	memset_extent_buffer(eb, 0, 0, len);
@@ -297,16 +314,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 +335,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 +348,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 +360,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;
-	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;
+	bitmap_clear(bitmap, 0, len * BITS_PER_BYTE);
+	extent_buffer_bitmap_clear(eb, 0, 0, len * BITS_PER_BYTE);
+	for (i = 0; i < len * BITS_PER_BYTE / 32; i++) {
+		x = (0x19660dULL * (u64)x + 0x3c6ef35fULL) & 0xffffffffU;
+		for (j = 0; j < 32; j++) {
+			if (x & (1U << j)) {
+				bitmap_set(bitmap, i * 32 + j, 1);
+				extent_buffer_bitmap_set(eb, 0, i * 32 + 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

[PATCH 3/3] Btrfs: expand free space tree sanity tests to catch endianness bug

[Omar Sandoval]

From: Omar Sandoval <osandov@fb.com>

The free space tree format conversion functions were broken on
big-endian systems, but the sanity tests didn't catch it because all of
the operations were aligned to multiple words. This was meant to catch
any bugs in the extent buffer code's handling of high memory, but it
ended up hiding the endianness bug. Expand the tests to do both
sector-aligned and page-aligned operations.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 fs/btrfs/tests/free-space-tree-tests.c | 164 +++++++++++++++++++--------------
 1 file changed, 96 insertions(+), 68 deletions(-)

diff --git a/fs/btrfs/tests/free-space-tree-tests.c b/fs/btrfs/tests/free-space-tree-tests.c
index aac507085ab0..d837b9e422d8 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -27,12 +27,6 @@ struct free_space_extent {
 	u64 start, length;
 };
 
-/*
- * The test cases align their operations to this in order to hit some of the
- * edge cases in the bitmap code.
- */
-#define BITMAP_RANGE (BTRFS_FREE_SPACE_BITMAP_BITS * PAGE_SIZE)
-
 static int __check_free_space_extents(struct btrfs_trans_handle *trans,
 				      struct btrfs_fs_info *fs_info,
 				      struct btrfs_block_group_cache *cache,
@@ -168,7 +162,8 @@ static int check_free_space_extents(struct btrfs_trans_handle *trans,
 static int test_empty_block_group(struct btrfs_trans_handle *trans,
 				  struct btrfs_fs_info *fs_info,
 				  struct btrfs_block_group_cache *cache,
-				  struct btrfs_path *path)
+				  struct btrfs_path *path,
+				  u32 alignment)
 {
 	struct free_space_extent extents[] = {
 		{cache->key.objectid, cache->key.offset},
@@ -181,7 +176,8 @@ static int test_empty_block_group(struct btrfs_trans_handle *trans,
 static int test_remove_all(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {};
 	int ret;
@@ -201,16 +197,17 @@ static int test_remove_all(struct btrfs_trans_handle *trans,
 static int test_remove_beginning(struct btrfs_trans_handle *trans,
 				 struct btrfs_fs_info *fs_info,
 				 struct btrfs_block_group_cache *cache,
-				 struct btrfs_path *path)
+				 struct btrfs_path *path,
+				 u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid + BITMAP_RANGE,
-			cache->key.offset - BITMAP_RANGE},
+		{cache->key.objectid + alignment,
+			cache->key.offset - alignment},
 	};
 	int ret;
 
 	ret = __remove_from_free_space_tree(trans, fs_info, cache, path,
-					    cache->key.objectid, BITMAP_RANGE);
+					    cache->key.objectid, alignment);
 	if (ret) {
 		test_msg("Could not remove free space\n");
 		return ret;
@@ -224,17 +221,18 @@ static int test_remove_beginning(struct btrfs_trans_handle *trans,
 static int test_remove_end(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid, cache->key.offset - BITMAP_RANGE},
+		{cache->key.objectid, cache->key.offset - alignment},
 	};
 	int ret;
 
 	ret = __remove_from_free_space_tree(trans, fs_info, cache, path,
 					    cache->key.objectid +
-					    cache->key.offset - BITMAP_RANGE,
-					    BITMAP_RANGE);
+					    cache->key.offset - alignment,
+					    alignment);
 	if (ret) {
 		test_msg("Could not remove free space\n");
 		return ret;
@@ -247,18 +245,19 @@ static int test_remove_end(struct btrfs_trans_handle *trans,
 static int test_remove_middle(struct btrfs_trans_handle *trans,
 			      struct btrfs_fs_info *fs_info,
 			      struct btrfs_block_group_cache *cache,
-			      struct btrfs_path *path)
+			      struct btrfs_path *path,
+			      u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid, BITMAP_RANGE},
-		{cache->key.objectid + 2 * BITMAP_RANGE,
-			cache->key.offset - 2 * BITMAP_RANGE},
+		{cache->key.objectid, alignment},
+		{cache->key.objectid + 2 * alignment,
+			cache->key.offset - 2 * alignment},
 	};
 	int ret;
 
 	ret = __remove_from_free_space_tree(trans, fs_info, cache, path,
-					    cache->key.objectid + BITMAP_RANGE,
-					    BITMAP_RANGE);
+					    cache->key.objectid + alignment,
+					    alignment);
 	if (ret) {
 		test_msg("Could not remove free space\n");
 		return ret;
@@ -271,10 +270,11 @@ static int test_remove_middle(struct btrfs_trans_handle *trans,
 static int test_merge_left(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid, 2 * BITMAP_RANGE},
+		{cache->key.objectid, 2 * alignment},
 	};
 	int ret;
 
@@ -287,15 +287,15 @@ static int test_merge_left(struct btrfs_trans_handle *trans,
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid, BITMAP_RANGE);
+				       cache->key.objectid, alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
@@ -308,10 +308,11 @@ static int test_merge_left(struct btrfs_trans_handle *trans,
 static int test_merge_right(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid + BITMAP_RANGE, 2 * BITMAP_RANGE},
+		{cache->key.objectid + alignment, 2 * alignment},
 	};
 	int ret;
 
@@ -324,16 +325,16 @@ static int test_merge_right(struct btrfs_trans_handle *trans,
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + 2 * BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + 2 * alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
@@ -346,10 +347,11 @@ static int test_merge_right(struct btrfs_trans_handle *trans,
 static int test_merge_both(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid, 3 * BITMAP_RANGE},
+		{cache->key.objectid, 3 * alignment},
 	};
 	int ret;
 
@@ -362,23 +364,23 @@ static int test_merge_both(struct btrfs_trans_handle *trans,
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid, BITMAP_RANGE);
+				       cache->key.objectid, alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + 2 * BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + 2 * alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
@@ -391,12 +393,13 @@ static int test_merge_both(struct btrfs_trans_handle *trans,
 static int test_merge_none(struct btrfs_trans_handle *trans,
 			   struct btrfs_fs_info *fs_info,
 			   struct btrfs_block_group_cache *cache,
-			   struct btrfs_path *path)
+			   struct btrfs_path *path,
+			   u32 alignment)
 {
 	struct free_space_extent extents[] = {
-		{cache->key.objectid, BITMAP_RANGE},
-		{cache->key.objectid + 2 * BITMAP_RANGE, BITMAP_RANGE},
-		{cache->key.objectid + 4 * BITMAP_RANGE, BITMAP_RANGE},
+		{cache->key.objectid, alignment},
+		{cache->key.objectid + 2 * alignment, alignment},
+		{cache->key.objectid + 4 * alignment, alignment},
 	};
 	int ret;
 
@@ -409,23 +412,23 @@ static int test_merge_none(struct btrfs_trans_handle *trans,
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid, BITMAP_RANGE);
+				       cache->key.objectid, alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + 4 * BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + 4 * alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
 	}
 
 	ret = __add_to_free_space_tree(trans, fs_info, cache, path,
-				       cache->key.objectid + 2 * BITMAP_RANGE,
-				       BITMAP_RANGE);
+				       cache->key.objectid + 2 * alignment,
+				       alignment);
 	if (ret) {
 		test_msg("Could not add free space\n");
 		return ret;
@@ -438,10 +441,11 @@ static int test_merge_none(struct btrfs_trans_handle *trans,
 typedef int (*test_func_t)(struct btrfs_trans_handle *,
 			   struct btrfs_fs_info *,
 			   struct btrfs_block_group_cache *,
-			   struct btrfs_path *);
+			   struct btrfs_path *,
+			   u32 alignment);
 
-static int run_test(test_func_t test_func, int bitmaps,
-		u32 sectorsize, u32 nodesize)
+static int run_test(test_func_t test_func, int bitmaps, u32 sectorsize,
+		    u32 nodesize, u32 alignment)
 {
 	struct btrfs_root *root = NULL;
 	struct btrfs_block_group_cache *cache = NULL;
@@ -479,7 +483,7 @@ static int run_test(test_func_t test_func, int bitmaps,
 	btrfs_set_header_nritems(root->node, 0);
 	root->alloc_bytenr += 2 * nodesize;
 
-	cache = btrfs_alloc_dummy_block_group(8 * BITMAP_RANGE, sectorsize);
+	cache = btrfs_alloc_dummy_block_group(8 * alignment, sectorsize);
 	if (!cache) {
 		test_msg("Couldn't allocate dummy block group cache\n");
 		ret = -ENOMEM;
@@ -513,7 +517,7 @@ static int run_test(test_func_t test_func, int bitmaps,
 		}
 	}
 
-	ret = test_func(&trans, root->fs_info, cache, path);
+	ret = test_func(&trans, root->fs_info, cache, path, alignment);
 	if (ret)
 		goto out;
 
@@ -537,15 +541,27 @@ out:
 	return ret;
 }
 
-static int run_test_both_formats(test_func_t test_func,
-	u32 sectorsize, u32 nodesize)
+static int run_test_both_formats(test_func_t test_func, u32 sectorsize,
+				 u32 nodesize, u32 alignment)
 {
+	int test_ret = 0;
 	int ret;
 
-	ret = run_test(test_func, 0, sectorsize, nodesize);
-	if (ret)
-		return ret;
-	return run_test(test_func, 1, sectorsize, nodesize);
+	ret = run_test(test_func, 0, sectorsize, nodesize, alignment);
+	if (ret) {
+		test_msg("%pf failed with extents, sectorsize=%u, nodesize=%u, alignment=%u\n",
+			 test_func, sectorsize, nodesize, alignment);
+		test_ret = ret;
+	}
+
+	ret = run_test(test_func, 1, sectorsize, nodesize, alignment);
+	if (ret) {
+		test_msg("%pf failed with bitmaps, sectorsize=%u, nodesize=%u, alignment=%u\n",
+			 test_func, sectorsize, nodesize, alignment);
+		test_ret = ret;
+	}
+
+	return test_ret;
 }
 
 int btrfs_test_free_space_tree(u32 sectorsize, u32 nodesize)
@@ -561,18 +577,30 @@ int btrfs_test_free_space_tree(u32 sectorsize, u32 nodesize)
 		test_merge_both,
 		test_merge_none,
 	};
+	u32 bitmap_alignment;
+	int test_ret = 0;
 	int i;
 
+	/*
+	 * Align some operations to a page to flush out bugs in the extent
+	 * buffer bitmap handling of highmem.
+	 */
+	bitmap_alignment = BTRFS_FREE_SPACE_BITMAP_BITS * PAGE_SIZE;
+
 	test_msg("Running free space tree tests\n");
 	for (i = 0; i < ARRAY_SIZE(tests); i++) {
-		int ret = run_test_both_formats(tests[i], sectorsize,
-			nodesize);
-		if (ret) {
-			test_msg("%pf : sectorsize %u failed\n",
-				tests[i], sectorsize);
-			return ret;
-		}
+		int ret;
+
+		ret = run_test_both_formats(tests[i], sectorsize, nodesize,
+					    sectorsize);
+		if (ret)
+			test_ret = ret;
+
+		ret = run_test_both_formats(tests[i], sectorsize, nodesize,
+					    bitmap_alignment);
+		if (ret)
+			test_ret = ret;
 	}
 
-	return 0;
+	return test_ret;
 }
-- 
2.9.0

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chris Mason]

On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> So it turns out that the free space tree bitmap handling has always been
> broken on big-endian systems. Totally my bad.
>
> Patch 1 fixes this. Technically, it's a disk format change for
> big-endian systems, but it never could have worked before, so I won't go
> through the trouble of any incompat bits. If you've somehow been using
> space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> to want to mount with nospace_cache to clear it and wait for this to go
> in.
>
> Patch 2 fixes a similar error in the sanity tests (it's the same as the
> v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> oversight that patch 1 fixes.
>
> Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> pass on x86_64 and MIPS.

Thanks for fixing this up Omar.  Any big endian friends want to try this 
out in extended testing and make sure we've nailed it down?

-chris

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chandan Rajendra]

On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
> On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> >
> > So it turns out that the free space tree bitmap handling has always been
> > broken on big-endian systems. Totally my bad.
> >
> > Patch 1 fixes this. Technically, it's a disk format change for
> > big-endian systems, but it never could have worked before, so I won't go
> > through the trouble of any incompat bits. If you've somehow been using
> > space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> > to want to mount with nospace_cache to clear it and wait for this to go
> > in.
> >
> > Patch 2 fixes a similar error in the sanity tests (it's the same as the
> > v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> > oversight that patch 1 fixes.
> >
> > Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> > pass on x86_64 and MIPS.
> 
> Thanks for fixing this up Omar.  Any big endian friends want to try this 
> out in extended testing and make sure we've nailed it down?
>

Hi Omar & Chris,

I will run fstests with this patchset applied on ppc64 BE and inform you about
the results.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
chandan

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Fri, Jul 15, 2016 at 12:34:10PM +0530, Chandan Rajendra wrote:
> On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
> > On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > >
> > > So it turns out that the free space tree bitmap handling has always been
> > > broken on big-endian systems. Totally my bad.
> > >
> > > Patch 1 fixes this. Technically, it's a disk format change for
> > > big-endian systems, but it never could have worked before, so I won't go
> > > through the trouble of any incompat bits. If you've somehow been using
> > > space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> > > to want to mount with nospace_cache to clear it and wait for this to go
> > > in.
> > >
> > > Patch 2 fixes a similar error in the sanity tests (it's the same as the
> > > v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> > > oversight that patch 1 fixes.
> > >
> > > Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> > > pass on x86_64 and MIPS.
> > 
> > Thanks for fixing this up Omar.  Any big endian friends want to try this 
> > out in extended testing and make sure we've nailed it down?
> >
> 
> Hi Omar & Chris,
> 
> I will run fstests with this patchset applied on ppc64 BE and inform you about
> the results.
> 

Thanks, Chandan! I set up my xfstests for space_cache=v2 by doing:

    mkfs.btrfs "$TEST_DEV"
    mount -o space_cache=v2 "$TEST_DEV" "$TEST_DIR"
    umount "$TEST_DEV"

and adding

    export MOUNT_OPTIONS="-o space_cache=v2"

to local.config. btrfsck also needs the patch here [1].

Thanks again.

1: http://thread.gmane.org/gmane.comp.file-systems.btrfs/58382

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chandan Rajendra]

On Friday, July 15, 2016 12:15:15 PM Omar Sandoval wrote:
> On Fri, Jul 15, 2016 at 12:34:10PM +0530, Chandan Rajendra wrote:
> > On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
> > > On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > >
> > > > So it turns out that the free space tree bitmap handling has always been
> > > > broken on big-endian systems. Totally my bad.
> > > >
> > > > Patch 1 fixes this. Technically, it's a disk format change for
> > > > big-endian systems, but it never could have worked before, so I won't go
> > > > through the trouble of any incompat bits. If you've somehow been using
> > > > space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> > > > to want to mount with nospace_cache to clear it and wait for this to go
> > > > in.
> > > >
> > > > Patch 2 fixes a similar error in the sanity tests (it's the same as the
> > > > v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> > > > oversight that patch 1 fixes.
> > > >
> > > > Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> > > > pass on x86_64 and MIPS.
> > > 
> > > Thanks for fixing this up Omar.  Any big endian friends want to try this 
> > > out in extended testing and make sure we've nailed it down?
> > >
> > 
> > Hi Omar & Chris,
> > 
> > I will run fstests with this patchset applied on ppc64 BE and inform you about
> > the results.
> > 
> 
> Thanks, Chandan! I set up my xfstests for space_cache=v2 by doing:
> 
>     mkfs.btrfs "$TEST_DEV"
>     mount -o space_cache=v2 "$TEST_DEV" "$TEST_DIR"
>     umount "$TEST_DEV"
> 
> and adding
> 
>     export MOUNT_OPTIONS="-o space_cache=v2"
> 
> to local.config. btrfsck also needs the patch here [1].
> 
> 

Hi,

I did execute the fstests tests suite on ppc64 BE as per above configuration
and there were no new regressions. Also, I did execute fsx (via generic/127)
thrice on the same filesystem instance,
1. With the unpatched kernel and later
2. With the patched kernel and again
3. With the unpatched kernel
... there were no new regressions when executing the above steps.

Tested-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

-- 
chandan

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chris Mason]

On 07/17/2016 08:19 AM, Chandan Rajendra wrote:
> On Friday, July 15, 2016 12:15:15 PM Omar Sandoval wrote:
>> On Fri, Jul 15, 2016 at 12:34:10PM +0530, Chandan Rajendra wrote:
>>> On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
>>>> On 07/14/2016 07:31 PM, Omar Sandoval wrote:
>>>>> From: Omar Sandoval <osandov@fb.com>
>>>>>
>>>>> So it turns out that the free space tree bitmap handling has always been
>>>>> broken on big-endian systems. Totally my bad.
>>>>>
>>>>> Patch 1 fixes this. Technically, it's a disk format change for
>>>>> big-endian systems, but it never could have worked before, so I won't go
>>>>> through the trouble of any incompat bits. If you've somehow been using
>>>>> space_cache=v2 on a big-endian system (I doubt anyone is), you're going
>>>>> to want to mount with nospace_cache to clear it and wait for this to go
>>>>> in.
>>>>>
>>>>> Patch 2 fixes a similar error in the sanity tests (it's the same as the
>>>>> v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
>>>>> oversight that patch 1 fixes.
>>>>>
>>>>> Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
>>>>> pass on x86_64 and MIPS.
>>>>
>>>> Thanks for fixing this up Omar.  Any big endian friends want to try this
>>>> out in extended testing and make sure we've nailed it down?
>>>>
>>>
>>> Hi Omar & Chris,
>>>
>>> I will run fstests with this patchset applied on ppc64 BE and inform you about
>>> the results.
>>>
>>
>> Thanks, Chandan! I set up my xfstests for space_cache=v2 by doing:
>>
>>     mkfs.btrfs "$TEST_DEV"
>>     mount -o space_cache=v2 "$TEST_DEV" "$TEST_DIR"
>>     umount "$TEST_DEV"
>>
>> and adding
>>
>>     export MOUNT_OPTIONS="-o space_cache=v2"
>>
>> to local.config. btrfsck also needs the patch here [1].
>>
>>
>
> Hi,
>
> I did execute the fstests tests suite on ppc64 BE as per above configuration
> and there were no new regressions. Also, I did execute fsx (via generic/127)
> thrice on the same filesystem instance,
> 1. With the unpatched kernel and later
> 2. With the patched kernel and again
> 3. With the unpatched kernel
> ... there were no new regressions when executing the above steps.

Thanks Chandan!  But I'm a little confused.  If the patch is helping, we 
should be storing bitmaps wrong on disk unpatched.  There should be 
problems going back and forth.

-chris

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Mon, Jul 18, 2016 at 02:43:26PM -0400, Chris Mason wrote:
> 
> 
> On 07/17/2016 08:19 AM, Chandan Rajendra wrote:
> > On Friday, July 15, 2016 12:15:15 PM Omar Sandoval wrote:
> > > On Fri, Jul 15, 2016 at 12:34:10PM +0530, Chandan Rajendra wrote:
> > > > On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
> > > > > On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> > > > > > From: Omar Sandoval <osandov@fb.com>
> > > > > > 
> > > > > > So it turns out that the free space tree bitmap handling has always been
> > > > > > broken on big-endian systems. Totally my bad.
> > > > > > 
> > > > > > Patch 1 fixes this. Technically, it's a disk format change for
> > > > > > big-endian systems, but it never could have worked before, so I won't go
> > > > > > through the trouble of any incompat bits. If you've somehow been using
> > > > > > space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> > > > > > to want to mount with nospace_cache to clear it and wait for this to go
> > > > > > in.
> > > > > > 
> > > > > > Patch 2 fixes a similar error in the sanity tests (it's the same as the
> > > > > > v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> > > > > > oversight that patch 1 fixes.
> > > > > > 
> > > > > > Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> > > > > > pass on x86_64 and MIPS.
> > > > > 
> > > > > Thanks for fixing this up Omar.  Any big endian friends want to try this
> > > > > out in extended testing and make sure we've nailed it down?
> > > > > 
> > > > 
> > > > Hi Omar & Chris,
> > > > 
> > > > I will run fstests with this patchset applied on ppc64 BE and inform you about
> > > > the results.
> > > > 
> > > 
> > > Thanks, Chandan! I set up my xfstests for space_cache=v2 by doing:
> > > 
> > >     mkfs.btrfs "$TEST_DEV"
> > >     mount -o space_cache=v2 "$TEST_DEV" "$TEST_DIR"
> > >     umount "$TEST_DEV"
> > > 
> > > and adding
> > > 
> > >     export MOUNT_OPTIONS="-o space_cache=v2"
> > > 
> > > to local.config. btrfsck also needs the patch here [1].
> > > 
> > > 
> > 
> > Hi,
> > 
> > I did execute the fstests tests suite on ppc64 BE as per above configuration
> > and there were no new regressions. Also, I did execute fsx (via generic/127)
> > thrice on the same filesystem instance,
> > 1. With the unpatched kernel and later
> > 2. With the patched kernel and again
> > 3. With the unpatched kernel
> > ... there were no new regressions when executing the above steps.
> 
> Thanks Chandan!  But I'm a little confused.  If the patch is helping, we
> should be storing bitmaps wrong on disk unpatched.  There should be problems
> going back and forth.
> 
> -chris

Yeah, this should definitely not work. It's possible that things are
just silently failing and getting corrupted if the module isn't built
with CONFIG_BTRFS_ASSERT, but btrfsck v4.6.1 + my patch should catch
that.

Chandan, is fsx creating enough fragmentation to trigger the switch to
bitmaps? You can check with `btrfs inspect dump-tree`; there should be
FREE_SPACE_BITMAP items. If there are only FREE_SPACE_EXTENT items, then
it's not testing the right code path.

I have a script here [1] that I've been using to test the free space
tree. When I ran it with `--check` on MIPS, it failed on the old kernel
and passed with this series. If you stick a return after the call to
`unlink_every_other_file()`, you'll get a nice, fragmented filesystem to
feed to xfstests, as well.

1: https://github.com/osandov/osandov-linux/blob/master/scripts/fragment_free_space_tree.py

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chandan Rajendra]

On Monday, July 18, 2016 03:31:04 PM Omar Sandoval wrote:
> On Mon, Jul 18, 2016 at 02:43:26PM -0400, Chris Mason wrote:
> > 
> > 
> > On 07/17/2016 08:19 AM, Chandan Rajendra wrote:
> > > On Friday, July 15, 2016 12:15:15 PM Omar Sandoval wrote:
> > > > On Fri, Jul 15, 2016 at 12:34:10PM +0530, Chandan Rajendra wrote:
> > > > > On Thursday, July 14, 2016 07:47:04 PM Chris Mason wrote:
> > > > > > On 07/14/2016 07:31 PM, Omar Sandoval wrote:
> > > > > > > From: Omar Sandoval <osandov@fb.com>
> > > > > > > 
> > > > > > > So it turns out that the free space tree bitmap handling has always been
> > > > > > > broken on big-endian systems. Totally my bad.
> > > > > > > 
> > > > > > > Patch 1 fixes this. Technically, it's a disk format change for
> > > > > > > big-endian systems, but it never could have worked before, so I won't go
> > > > > > > through the trouble of any incompat bits. If you've somehow been using
> > > > > > > space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> > > > > > > to want to mount with nospace_cache to clear it and wait for this to go
> > > > > > > in.
> > > > > > > 
> > > > > > > Patch 2 fixes a similar error in the sanity tests (it's the same as the
> > > > > > > v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> > > > > > > oversight that patch 1 fixes.
> > > > > > > 
> > > > > > > Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> > > > > > > pass on x86_64 and MIPS.
> > > > > > 
> > > > > > Thanks for fixing this up Omar.  Any big endian friends want to try this
> > > > > > out in extended testing and make sure we've nailed it down?
> > > > > > 
> > > > > 
> > > > > Hi Omar & Chris,
> > > > > 
> > > > > I will run fstests with this patchset applied on ppc64 BE and inform you about
> > > > > the results.
> > > > > 
> > > > 
> > > > Thanks, Chandan! I set up my xfstests for space_cache=v2 by doing:
> > > > 
> > > >     mkfs.btrfs "$TEST_DEV"
> > > >     mount -o space_cache=v2 "$TEST_DEV" "$TEST_DIR"
> > > >     umount "$TEST_DEV"
> > > > 
> > > > and adding
> > > > 
> > > >     export MOUNT_OPTIONS="-o space_cache=v2"
> > > > 
> > > > to local.config. btrfsck also needs the patch here [1].
> > > > 
> > > > 
> > > 
> > > Hi,
> > > 
> > > I did execute the fstests tests suite on ppc64 BE as per above configuration
> > > and there were no new regressions. Also, I did execute fsx (via generic/127)
> > > thrice on the same filesystem instance,
> > > 1. With the unpatched kernel and later
> > > 2. With the patched kernel and again
> > > 3. With the unpatched kernel
> > > ... there were no new regressions when executing the above steps.
> > 
> > Thanks Chandan!  But I'm a little confused.  If the patch is helping, we
> > should be storing bitmaps wrong on disk unpatched.  There should be problems
> > going back and forth.
> > 
> > -chris
> 
> Yeah, this should definitely not work. It's possible that things are
> just silently failing and getting corrupted if the module isn't built
> with CONFIG_BTRFS_ASSERT, but btrfsck v4.6.1 + my patch should catch
> that.
> 
> Chandan, is fsx creating enough fragmentation to trigger the switch to
> bitmaps? You can check with `btrfs inspect dump-tree`; there should be
> FREE_SPACE_BITMAP items. If there are only FREE_SPACE_EXTENT items, then
> it's not testing the right code path.
> 
> I have a script here [1] that I've been using to test the free space
> tree. When I ran it with `--check` on MIPS, it failed on the old kernel
> and passed with this series. If you stick a return after the call to
> `unlink_every_other_file()`, you'll get a nice, fragmented filesystem to
> feed to xfstests, as well.

You are right, There were only FREE_SPACE_EXTENT items in the filesystem that
was operated on by fsx. I executed fragment_free_space_tree.py to create a
filesystem with FREE_SPACE_BITMAP items. When such a filesystem is created
with the unpatched kernel, later mounted on a patched kernel and fsx executed
on it, I see that we fail assertion statements in free-space-tree.c. For e.g.

BTRFS error (device loop0): incorrect extent count for 289406976; counted 8186, expected 8192
BTRFS: assertion failed: 0, file: /root/repos/linux/fs/btrfs/free-space-tree.c, line: 1485

-- 
chandan

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Chris Mason]

On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
> On Monday, July 18, 2016 03:31:04 PM Omar Sandoval wrote:
>> Yeah, this should definitely not work. It's possible that things are
>> just silently failing and getting corrupted if the module isn't built
>> with CONFIG_BTRFS_ASSERT, but btrfsck v4.6.1 + my patch should catch
>> that.
>>
>> Chandan, is fsx creating enough fragmentation to trigger the switch to
>> bitmaps? You can check with `btrfs inspect dump-tree`; there should be
>> FREE_SPACE_BITMAP items. If there are only FREE_SPACE_EXTENT items, then
>> it's not testing the right code path.
>>
>> I have a script here [1] that I've been using to test the free space
>> tree. When I ran it with `--check` on MIPS, it failed on the old kernel
>> and passed with this series. If you stick a return after the call to
>> `unlink_every_other_file()`, you'll get a nice, fragmented filesystem to
>> feed to xfstests, as well.
>
> You are right, There were only FREE_SPACE_EXTENT items in the filesystem that
> was operated on by fsx. I executed fragment_free_space_tree.py to create a
> filesystem with FREE_SPACE_BITMAP items. When such a filesystem is created
> with the unpatched kernel, later mounted on a patched kernel and fsx executed
> on it, I see that we fail assertion statements in free-space-tree.c. For e.g.
>
> BTRFS error (device loop0): incorrect extent count for 289406976; counted 8186, expected 8192
> BTRFS: assertion failed: 0, file: /root/repos/linux/fs/btrfs/free-space-tree.c, line: 1485
>

Omar, looks like we need to make the patched kernel refuse to mount free 
space trees without a new incompat bit set.  That way there won't be any 
surprises for the people that have managed to get a free space tree 
saved.  Can it please printk a message about clearing the tree and 
mounting again?

-chris

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Tue, Jul 19, 2016 at 03:25:16PM -0400, Chris Mason wrote:
> On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
> > On Monday, July 18, 2016 03:31:04 PM Omar Sandoval wrote:
> > > Yeah, this should definitely not work. It's possible that things are
> > > just silently failing and getting corrupted if the module isn't built
> > > with CONFIG_BTRFS_ASSERT, but btrfsck v4.6.1 + my patch should catch
> > > that.
> > > 
> > > Chandan, is fsx creating enough fragmentation to trigger the switch to
> > > bitmaps? You can check with `btrfs inspect dump-tree`; there should be
> > > FREE_SPACE_BITMAP items. If there are only FREE_SPACE_EXTENT items, then
> > > it's not testing the right code path.
> > > 
> > > I have a script here [1] that I've been using to test the free space
> > > tree. When I ran it with `--check` on MIPS, it failed on the old kernel
> > > and passed with this series. If you stick a return after the call to
> > > `unlink_every_other_file()`, you'll get a nice, fragmented filesystem to
> > > feed to xfstests, as well.
> > 
> > You are right, There were only FREE_SPACE_EXTENT items in the filesystem that
> > was operated on by fsx. I executed fragment_free_space_tree.py to create a
> > filesystem with FREE_SPACE_BITMAP items. When such a filesystem is created
> > with the unpatched kernel, later mounted on a patched kernel and fsx executed
> > on it, I see that we fail assertion statements in free-space-tree.c. For e.g.
> > 
> > BTRFS error (device loop0): incorrect extent count for 289406976; counted 8186, expected 8192
> > BTRFS: assertion failed: 0, file: /root/repos/linux/fs/btrfs/free-space-tree.c, line: 1485
> > 
> 
> Omar, looks like we need to make the patched kernel refuse to mount free
> space trees without a new incompat bit set.  That way there won't be any
> surprises for the people that have managed to get a free space tree saved.
> Can it please printk a message about clearing the tree and mounting again?
> 
> -chris

Sorry it took me a month to get around to this, I tried to implement
this a couple of ways but I really don't like it. Basically, when we see
that we're missing the compat bit, we have to assume that the free space
tree was created with the same endianness that we're running on now.
That could lead to a false positive if, say, we created the filesystem
on a little-endian machine with an old kernel but are using it on a
big-endian system, or a false negative if it was created on a big-endian
machine with an old kernel but we're using it on a little-endian
machine.

There's also the question of making it a compat bit vs an incompat bit.
An incompat bit makes sure that we don't break the filesystem by
mounting it on an old big-endian kernel, but needlessly breaks
backwards-compatibility for little-endian.

I'd be much happier if we could just pretend this never happened. Here's
the patch, anyways, for the sake of completeness. Chris, what do you
think?

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 2fe8f89..f50b3e0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -246,7 +246,8 @@ struct btrfs_super_block {
  * Compat flags that we support.  If any incompat flags are set other than the
  * ones specified below then we will fail to mount
  */
-#define BTRFS_FEATURE_COMPAT_SUPP		0ULL
+#define BTRFS_FEATURE_COMPAT_SUPP			\
+	(BTRFS_FEATURE_COMPAT_FREE_SPACE_TREE_LE)
 #define BTRFS_FEATURE_COMPAT_SAFE_SET		0ULL
 #define BTRFS_FEATURE_COMPAT_SAFE_CLEAR		0ULL
 
@@ -3538,6 +3539,62 @@ static inline int __btrfs_fs_compat_ro(struct btrfs_fs_info *fs_info, u64 flag)
 	return !!(btrfs_super_compat_ro_flags(disk_super) & flag);
 }
 
+#define btrfs_set_fs_compat(__fs_info, opt) \
+	__btrfs_set_fs_compat((__fs_info), BTRFS_FEATURE_COMPAT_##opt)
+
+static inline void __btrfs_set_fs_compat(struct btrfs_fs_info *fs_info,
+					 u64 flag)
+{
+	struct btrfs_super_block *disk_super;
+	u64 features;
+
+	disk_super = fs_info->super_copy;
+	features = btrfs_super_compat_flags(disk_super);
+	if (!(features & flag)) {
+		spin_lock(&fs_info->super_lock);
+		features = btrfs_super_compat_flags(disk_super);
+		if (!(features & flag)) {
+			features |= flag;
+			btrfs_set_super_compat_flags(disk_super, features);
+			btrfs_info(fs_info, "setting %llu feature flag", flag);
+		}
+		spin_unlock(&fs_info->super_lock);
+	}
+}
+
+#define btrfs_clear_fs_compat(__fs_info, opt) \
+	__btrfs_clear_fs_compat((__fs_info), BTRFS_FEATURE_COMPAT_##opt)
+
+static inline void __btrfs_clear_fs_compat(struct btrfs_fs_info *fs_info,
+					      u64 flag)
+{
+	struct btrfs_super_block *disk_super;
+	u64 features;
+
+	disk_super = fs_info->super_copy;
+	features = btrfs_super_compat_flags(disk_super);
+	if (features & flag) {
+		spin_lock(&fs_info->super_lock);
+		features = btrfs_super_compat_flags(disk_super);
+		if (features & flag) {
+			features &= ~flag;
+			btrfs_set_super_compat_flags(disk_super, features);
+			btrfs_info(fs_info, "clearing %llu feature flag", flag);
+		}
+		spin_unlock(&fs_info->super_lock);
+	}
+}
+
+#define btrfs_fs_compat(fs_info, opt) \
+	__btrfs_fs_compat((fs_info), BTRFS_FEATURE_COMPAT_##opt)
+
+static inline int __btrfs_fs_compat(struct btrfs_fs_info *fs_info, u64 flag)
+{
+	struct btrfs_super_block *disk_super;
+	disk_super = fs_info->super_copy;
+	return !!(btrfs_super_compat_flags(disk_super) & flag);
+}
+
 /* acl.c */
 #ifdef CONFIG_BTRFS_FS_POSIX_ACL
 struct posix_acl *btrfs_get_acl(struct inode *inode, int type);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 59febfb..467c364 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3090,6 +3090,15 @@ retry_root_backup:
 	if (sb->s_flags & MS_RDONLY)
 		return 0;
 
+	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE) &&
+	    !btrfs_test_opt(fs_info, CLEAR_CACHE)) {
+		ret = btrfs_check_free_space_tree_le(fs_info);
+		if (ret) {
+			close_ctree(tree_root);
+			return ret;
+		}
+	}
+
 	if (btrfs_test_opt(tree_root->fs_info, FREE_SPACE_TREE) &&
 	    !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
 		btrfs_info(fs_info, "creating free space tree");
diff --git a/fs/btrfs/free-space-tree.c b/fs/btrfs/free-space-tree.c
index 8fd85bf..ef84c1d 100644
--- a/fs/btrfs/free-space-tree.c
+++ b/fs/btrfs/free-space-tree.c
@@ -1182,6 +1182,7 @@ int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info)
 	}
 
 	btrfs_set_fs_compat_ro(fs_info, FREE_SPACE_TREE);
+	btrfs_set_fs_compat(fs_info, FREE_SPACE_TREE_LE);
 	fs_info->creating_free_space_tree = 0;
 
 	ret = btrfs_commit_transaction(trans, tree_root);
@@ -1250,6 +1251,7 @@ int btrfs_clear_free_space_tree(struct btrfs_fs_info *fs_info)
 		return PTR_ERR(trans);
 
 	btrfs_clear_fs_compat_ro(fs_info, FREE_SPACE_TREE);
+	btrfs_clear_fs_compat(fs_info, FREE_SPACE_TREE_LE);
 	fs_info->free_space_root = NULL;
 
 	ret = clear_free_space_tree(trans, free_space_root);
@@ -1284,6 +1286,40 @@ abort:
 	return ret;
 }
 
+#ifdef __LITTLE_ENDIAN
+static int __btrfs_set_free_space_tree_le(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *tree_root = fs_info->tree_root;
+
+	trans = btrfs_start_transaction(tree_root, 0);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+	btrfs_set_fs_compat(fs_info, FREE_SPACE_TREE_LE);
+	return btrfs_commit_transaction(trans, tree_root);
+}
+#endif
+
+/*
+ * The free space tree was broken on big-endian systems when it was introduced.
+ * This attempts to catch that with the FREE_SPACE_TREE_LE compat bit. If it's
+ * not set, then we assume that the filesystem being mounted was created/used on
+ * the same endianness, which kind of sucks.
+ */
+int btrfs_check_free_space_tree_le(struct btrfs_fs_info *fs_info)
+{
+	if (btrfs_fs_compat(fs_info, FREE_SPACE_TREE_LE))
+		return 0;
+
+#ifdef __LITTLE_ENDIAN
+	return __btrfs_set_free_space_tree_le(fs_info);
+#else
+	btrfs_err(fs_info, "free space tree created with broken big-endian kernel");
+	btrfs_err(fs_info, "please remount once with nospace_cache,clear_cache and then remount with space_cache=v2");
+	return -EINVAL;
+#endif
+}
+
 static int __add_block_group_free_space(struct btrfs_trans_handle *trans,
 					struct btrfs_fs_info *fs_info,
 					struct btrfs_block_group_cache *block_group,
diff --git a/fs/btrfs/free-space-tree.h b/fs/btrfs/free-space-tree.h
index 54ffced..505b13e 100644
--- a/fs/btrfs/free-space-tree.h
+++ b/fs/btrfs/free-space-tree.h
@@ -30,6 +30,7 @@
 void set_free_space_tree_thresholds(struct btrfs_block_group_cache *block_group);
 int btrfs_create_free_space_tree(struct btrfs_fs_info *fs_info);
 int btrfs_clear_free_space_tree(struct btrfs_fs_info *fs_info);
+int btrfs_check_free_space_tree_le(struct btrfs_fs_info *fs_info);
 int load_free_space_tree(struct btrfs_caching_control *caching_ctl);
 int add_block_group_free_space(struct btrfs_trans_handle *trans,
 			       struct btrfs_fs_info *fs_info,
diff --git a/include/uapi/linux/btrfs.h b/include/uapi/linux/btrfs.h
index ac5eacd..2695b3d 100644
--- a/include/uapi/linux/btrfs.h
+++ b/include/uapi/linux/btrfs.h
@@ -239,6 +239,8 @@ struct btrfs_ioctl_fs_info_args {
  * Used by:
  * struct btrfs_ioctl_feature_flags
  */
+#define BTRFS_FEATURE_COMPAT_FREE_SPACE_TREE_LE	(1ULL << 0)
+
 #define BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE	(1ULL << 0)
 
 #define BTRFS_FEATURE_INCOMPAT_MIXED_BACKREF	(1ULL << 0)

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Anatoly Pugachev]

On Thu, Aug 18, 2016 at 11:33 PM, Omar Sandoval <osandov@osandov.com> wrote:
> On Tue, Jul 19, 2016 at 03:25:16PM -0400, Chris Mason wrote:
>> On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
>>
>> Omar, looks like we need to make the patched kernel refuse to mount free
>> space trees without a new incompat bit set.  That way there won't be any
>> surprises for the people that have managed to get a free space tree saved.
>> Can it please printk a message about clearing the tree and mounting again?
>>
> Sorry it took me a month to get around to this, I tried to implement
> this a couple of ways but I really don't like it. Basically, when we see
> that we're missing the compat bit, we have to assume that the free space
> tree was created with the same endianness that we're running on now.
> That could lead to a false positive if, say, we created the filesystem
> on a little-endian machine with an old kernel but are using it on a
> big-endian system, or a false negative if it was created on a big-endian
> machine with an old kernel but we're using it on a little-endian
> machine.
>
> There's also the question of making it a compat bit vs an incompat bit.
> An incompat bit makes sure that we don't break the filesystem by
> mounting it on an old big-endian kernel, but needlessly breaks
> backwards-compatibility for little-endian.
>
> I'd be much happier if we could just pretend this never happened. Here's
> the patch, anyways, for the sake of completeness. Chris, what do you
> think?

Omar,

I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
v4.8-rc3-39-g61c0457)
on "modprobe btrfs" i'm getting the following in the logs and module
does not load:

Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
BTRFS: selftest: sectorsize: 8192  nodesize: 8192
BTRFS: selftest: Running btrfs free space cache tests
BTRFS: selftest: Running extent only tests
BTRFS: selftest: Running bitmap only tests
BTRFS: selftest: Running bitmap and extent tests
BTRFS: selftest: Running space stealing from bitmap to extent
BTRFS: selftest: Free space cache tests finished
BTRFS: selftest: Running extent buffer operation tests
BTRFS: selftest: Running btrfs_split_item tests
BTRFS: selftest: Running extent I/O tests
BTRFS: selftest: Running find delalloc tests
BTRFS: selftest: Running extent buffer bitmap tests
BTRFS: selftest: Setting straddling pages failed
BTRFS: selftest: Extent I/O tests finished

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Anatoly Pugachev]

On Thu, Aug 18, 2016 at 11:33 PM, Omar Sandoval <osandov@osandov.com> wrote:
> On Tue, Jul 19, 2016 at 03:25:16PM -0400, Chris Mason wrote:
>> On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
>>
>> Omar, looks like we need to make the patched kernel refuse to mount free
>> space trees without a new incompat bit set.  That way there won't be any
>> surprises for the people that have managed to get a free space tree saved.
>> Can it please printk a message about clearing the tree and mounting again?
>>
> Sorry it took me a month to get around to this, I tried to implement
> this a couple of ways but I really don't like it. Basically, when we see
> that we're missing the compat bit, we have to assume that the free space
> tree was created with the same endianness that we're running on now.
> That could lead to a false positive if, say, we created the filesystem
> on a little-endian machine with an old kernel but are using it on a
> big-endian system, or a false negative if it was created on a big-endian
> machine with an old kernel but we're using it on a little-endian
> machine.
>
> There's also the question of making it a compat bit vs an incompat bit.
> An incompat bit makes sure that we don't break the filesystem by
> mounting it on an old big-endian kernel, but needlessly breaks
> backwards-compatibility for little-endian.
>
> I'd be much happier if we could just pretend this never happened. Here's
> the patch, anyways, for the sake of completeness. Chris, what do you
> think?

Omar,

I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
v4.8-rc3-39-g61c0457)
on "modprobe btrfs" i'm getting the following in the logs and module
does not load:

Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
BTRFS: selftest: sectorsize: 8192  nodesize: 8192
BTRFS: selftest: Running btrfs free space cache tests
BTRFS: selftest: Running extent only tests
BTRFS: selftest: Running bitmap only tests
BTRFS: selftest: Running bitmap and extent tests
BTRFS: selftest: Running space stealing from bitmap to extent
BTRFS: selftest: Free space cache tests finished
BTRFS: selftest: Running extent buffer operation tests
BTRFS: selftest: Running btrfs_split_item tests
BTRFS: selftest: Running extent I/O tests
BTRFS: selftest: Running find delalloc tests
BTRFS: selftest: Running extent buffer bitmap tests
BTRFS: selftest: Setting straddling pages failed
BTRFS: selftest: Extent I/O tests finished

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Fri, Aug 26, 2016 at 02:06:29PM +0300, Anatoly Pugachev wrote:
> On Thu, Aug 18, 2016 at 11:33 PM, Omar Sandoval <osandov@osandov.com> wrote:
> > On Tue, Jul 19, 2016 at 03:25:16PM -0400, Chris Mason wrote:
> >> On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
> >>
> >> Omar, looks like we need to make the patched kernel refuse to mount free
> >> space trees without a new incompat bit set.  That way there won't be any
> >> surprises for the people that have managed to get a free space tree saved.
> >> Can it please printk a message about clearing the tree and mounting again?
> >>
> > Sorry it took me a month to get around to this, I tried to implement
> > this a couple of ways but I really don't like it. Basically, when we see
> > that we're missing the compat bit, we have to assume that the free space
> > tree was created with the same endianness that we're running on now.
> > That could lead to a false positive if, say, we created the filesystem
> > on a little-endian machine with an old kernel but are using it on a
> > big-endian system, or a false negative if it was created on a big-endian
> > machine with an old kernel but we're using it on a little-endian
> > machine.
> >
> > There's also the question of making it a compat bit vs an incompat bit.
> > An incompat bit makes sure that we don't break the filesystem by
> > mounting it on an old big-endian kernel, but needlessly breaks
> > backwards-compatibility for little-endian.
> >
> > I'd be much happier if we could just pretend this never happened. Here's
> > the patch, anyways, for the sake of completeness. Chris, what do you
> > think?
> 
> Omar,
> 
> I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
> v4.8-rc3-39-g61c0457)
> on "modprobe btrfs" i'm getting the following in the logs and module
> does not load:
> 
> Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
> BTRFS: selftest: sectorsize: 8192  nodesize: 8192
> BTRFS: selftest: Running btrfs free space cache tests
> BTRFS: selftest: Running extent only tests
> BTRFS: selftest: Running bitmap only tests
> BTRFS: selftest: Running bitmap and extent tests
> BTRFS: selftest: Running space stealing from bitmap to extent
> BTRFS: selftest: Free space cache tests finished
> BTRFS: selftest: Running extent buffer operation tests
> BTRFS: selftest: Running btrfs_split_item tests
> BTRFS: selftest: Running extent I/O tests
> BTRFS: selftest: Running find delalloc tests
> BTRFS: selftest: Running extent buffer bitmap tests
> BTRFS: selftest: Setting straddling pages failed
> BTRFS: selftest: Extent I/O tests finished

Is this with the whole patchset + this patch? You still need the patch
set for this to actually work, the extra patch is just some extra
checks.

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Fri, Aug 26, 2016 at 02:06:29PM +0300, Anatoly Pugachev wrote:
> On Thu, Aug 18, 2016 at 11:33 PM, Omar Sandoval <osandov@osandov.com> wrote:
> > On Tue, Jul 19, 2016 at 03:25:16PM -0400, Chris Mason wrote:
> >> On 07/19/2016 12:06 PM, Chandan Rajendra wrote:
> >>
> >> Omar, looks like we need to make the patched kernel refuse to mount free
> >> space trees without a new incompat bit set.  That way there won't be any
> >> surprises for the people that have managed to get a free space tree saved.
> >> Can it please printk a message about clearing the tree and mounting again?
> >>
> > Sorry it took me a month to get around to this, I tried to implement
> > this a couple of ways but I really don't like it. Basically, when we see
> > that we're missing the compat bit, we have to assume that the free space
> > tree was created with the same endianness that we're running on now.
> > That could lead to a false positive if, say, we created the filesystem
> > on a little-endian machine with an old kernel but are using it on a
> > big-endian system, or a false negative if it was created on a big-endian
> > machine with an old kernel but we're using it on a little-endian
> > machine.
> >
> > There's also the question of making it a compat bit vs an incompat bit.
> > An incompat bit makes sure that we don't break the filesystem by
> > mounting it on an old big-endian kernel, but needlessly breaks
> > backwards-compatibility for little-endian.
> >
> > I'd be much happier if we could just pretend this never happened. Here's
> > the patch, anyways, for the sake of completeness. Chris, what do you
> > think?
> 
> Omar,
> 
> I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
> v4.8-rc3-39-g61c0457)
> on "modprobe btrfs" i'm getting the following in the logs and module
> does not load:
> 
> Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
> BTRFS: selftest: sectorsize: 8192  nodesize: 8192
> BTRFS: selftest: Running btrfs free space cache tests
> BTRFS: selftest: Running extent only tests
> BTRFS: selftest: Running bitmap only tests
> BTRFS: selftest: Running bitmap and extent tests
> BTRFS: selftest: Running space stealing from bitmap to extent
> BTRFS: selftest: Free space cache tests finished
> BTRFS: selftest: Running extent buffer operation tests
> BTRFS: selftest: Running btrfs_split_item tests
> BTRFS: selftest: Running extent I/O tests
> BTRFS: selftest: Running find delalloc tests
> BTRFS: selftest: Running extent buffer bitmap tests
> BTRFS: selftest: Setting straddling pages failed
> BTRFS: selftest: Extent I/O tests finished

Is this with the whole patchset + this patch? You still need the patch
set for this to actually work, the extra patch is just some extra
checks.

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Anatoly Pugachev]

On Sat, Aug 27, 2016 at 3:56 AM, Omar Sandoval <osandov@osandov.com> wrote:
> On Fri, Aug 26, 2016 at 02:06:29PM +0300, Anatoly Pugachev wrote:
>>
>> I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
>> v4.8-rc3-39-g61c0457)
>> on "modprobe btrfs" i'm getting the following in the logs and module
>> does not load:
>>
>> Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
>> BTRFS: selftest: sectorsize: 8192  nodesize: 8192
>> BTRFS: selftest: Running btrfs free space cache tests
>> BTRFS: selftest: Running extent only tests
>> BTRFS: selftest: Running bitmap only tests
>> BTRFS: selftest: Running bitmap and extent tests
>> BTRFS: selftest: Running space stealing from bitmap to extent
>> BTRFS: selftest: Free space cache tests finished
>> BTRFS: selftest: Running extent buffer operation tests
>> BTRFS: selftest: Running btrfs_split_item tests
>> BTRFS: selftest: Running extent I/O tests
>> BTRFS: selftest: Running find delalloc tests
>> BTRFS: selftest: Running extent buffer bitmap tests
>> BTRFS: selftest: Setting straddling pages failed
>> BTRFS: selftest: Extent I/O tests finished
>
> Is this with the whole patchset + this patch? You still need the patch
> set for this to actually work, the extra patch is just some extra
> checks.

Omar,

sorry, I don't get what do you mean by the whole patchset ? I used git
kernel (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git)
and applied patch from your mail from August 18,
https://www.spinics.net/lists/linux-btrfs/msg58023.html

Tell me what patches do I need or point to btrfs git repo with all
patches included and I'll test it.

Thanks.

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Anatoly Pugachev]

On Sat, Aug 27, 2016 at 3:56 AM, Omar Sandoval <osandov@osandov.com> wrote:
> On Fri, Aug 26, 2016 at 02:06:29PM +0300, Anatoly Pugachev wrote:
>>
>> I can't load btrfs module with this patch applied to 4.8.0-rc3+ (git
>> v4.8-rc3-39-g61c0457)
>> on "modprobe btrfs" i'm getting the following in the logs and module
>> does not load:
>>
>> Btrfs loaded, crc32c=crc32c-generic, debug=on, assert=on
>> BTRFS: selftest: sectorsize: 8192  nodesize: 8192
>> BTRFS: selftest: Running btrfs free space cache tests
>> BTRFS: selftest: Running extent only tests
>> BTRFS: selftest: Running bitmap only tests
>> BTRFS: selftest: Running bitmap and extent tests
>> BTRFS: selftest: Running space stealing from bitmap to extent
>> BTRFS: selftest: Free space cache tests finished
>> BTRFS: selftest: Running extent buffer operation tests
>> BTRFS: selftest: Running btrfs_split_item tests
>> BTRFS: selftest: Running extent I/O tests
>> BTRFS: selftest: Running find delalloc tests
>> BTRFS: selftest: Running extent buffer bitmap tests
>> BTRFS: selftest: Setting straddling pages failed
>> BTRFS: selftest: Extent I/O tests finished
>
> Is this with the whole patchset + this patch? You still need the patch
> set for this to actually work, the extra patch is just some extra
> checks.

Omar,

sorry, I don't get what do you mean by the whole patchset ? I used git
kernel (git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git)
and applied patch from your mail from August 18,
https://www.spinics.net/lists/linux-btrfs/msg58023.html

Tell me what patches do I need or point to btrfs git repo with all
patches included and I'll test it.

Thanks.

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[David Sterba]

On Thu, Aug 18, 2016 at 01:33:27PM -0700, Omar Sandoval wrote:
> > Omar, looks like we need to make the patched kernel refuse to mount free
> > space trees without a new incompat bit set.  That way there won't be any
> > surprises for the people that have managed to get a free space tree saved.
> > Can it please printk a message about clearing the tree and mounting again?
> > 
> > -chris
> 
> Sorry it took me a month to get around to this, I tried to implement
> this a couple of ways but I really don't like it. Basically, when we see
> that we're missing the compat bit, we have to assume that the free space
> tree was created with the same endianness that we're running on now.
> That could lead to a false positive if, say, we created the filesystem
> on a little-endian machine with an old kernel but are using it on a
> big-endian system, or a false negative if it was created on a big-endian
> machine with an old kernel but we're using it on a little-endian
> machine.
> 
> There's also the question of making it a compat bit vs an incompat bit.
> An incompat bit makes sure that we don't break the filesystem by
> mounting it on an old big-endian kernel, but needlessly breaks
> backwards-compatibility for little-endian.
> 
> I'd be much happier if we could just pretend this never happened. Here's
> the patch, anyways, for the sake of completeness. Chris, what do you
> think?

Here's my proposal how to resolve this:

* we have reports from people using intel hw that FST works fine for
  them, so here I don't see any need to introduce incompat bits

* there are few users on bigendian machines, they need to update kernel
  to store the correct layout of FST bitmap

* as majority of user will never hit the BE/LE problem, I'd focus on
  advising how to reset the free space tree and let anybody update (ie.
  pretend this hasn't happened)

I don't think we absolutelly must introduce the incompat bit and prevent
a disaster, because there are very few users affected.

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Omar Sandoval]

On Wed, Sep 21, 2016 at 04:50:12PM +0200, David Sterba wrote:
> On Thu, Aug 18, 2016 at 01:33:27PM -0700, Omar Sandoval wrote:
> > > Omar, looks like we need to make the patched kernel refuse to mount free
> > > space trees without a new incompat bit set.  That way there won't be any
> > > surprises for the people that have managed to get a free space tree saved.
> > > Can it please printk a message about clearing the tree and mounting again?
> > > 
> > > -chris
> > 
> > Sorry it took me a month to get around to this, I tried to implement
> > this a couple of ways but I really don't like it. Basically, when we see
> > that we're missing the compat bit, we have to assume that the free space
> > tree was created with the same endianness that we're running on now.
> > That could lead to a false positive if, say, we created the filesystem
> > on a little-endian machine with an old kernel but are using it on a
> > big-endian system, or a false negative if it was created on a big-endian
> > machine with an old kernel but we're using it on a little-endian
> > machine.
> > 
> > There's also the question of making it a compat bit vs an incompat bit.
> > An incompat bit makes sure that we don't break the filesystem by
> > mounting it on an old big-endian kernel, but needlessly breaks
> > backwards-compatibility for little-endian.
> > 
> > I'd be much happier if we could just pretend this never happened. Here's
> > the patch, anyways, for the sake of completeness. Chris, what do you
> > think?
> 
> Here's my proposal how to resolve this:
> 
> * we have reports from people using intel hw that FST works fine for
>   them, so here I don't see any need to introduce incompat bits
> 
> * there are few users on bigendian machines, they need to update kernel
>   to store the correct layout of FST bitmap
> 
> * as majority of user will never hit the BE/LE problem, I'd focus on
>   advising how to reset the free space tree and let anybody update (ie.
>   pretend this hasn't happened)
> 
> I don't think we absolutelly must introduce the incompat bit and prevent
> a disaster, because there are very few users affected.

Thanks, Dave, that's what I was thinking, too. I'll rebase this series
and send it out as is. Hopefully we can get it in for 4.9, or 4.10 at
the latest.

-- 
Omar

Re: [PATCH 0/3] Btrfs: fix free space tree bitmaps+tests on big-endian systems

[Anatoly Pugachev]

On Fri, Jul 15, 2016 at 2:31 AM, Omar Sandoval <osandov@osandov.com> wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> So it turns out that the free space tree bitmap handling has always been
> broken on big-endian systems. Totally my bad.
>
> Patch 1 fixes this. Technically, it's a disk format change for
> big-endian systems, but it never could have worked before, so I won't go
> through the trouble of any incompat bits. If you've somehow been using
> space_cache=v2 on a big-endian system (I doubt anyone is), you're going
> to want to mount with nospace_cache to clear it and wait for this to go
> in.
>
> Patch 2 fixes a similar error in the sanity tests (it's the same as the
> v2 I posted here [1]) and patch 3 expands the sanity tests to catch the
> oversight that patch 1 fixes.
>
> Applies to v4.7-rc7. No regressions in xfstests, and the sanity tests
> pass on x86_64 and MIPS.

Omar,

can you please upstream this patch or update it for current git kernel ?
Thanks.