[PATCH v2 0/3] btrfs: apply strict alignment checks to extent maps

[PATCH v2 0/3] btrfs: apply strict alignment checks to extent maps

[Qu Wenruo]

[CHANGELOG]
v2:
- Grammar fixes

  Not only spelling fixes, but also extra new line after an item of a
  list.

- Remove unnecessary ASSERT()s inside inode tests

  There is already enough comments on each test file extent item.

- Remove some unnecessary commit messages from patch 3

  Which not only introduces grammar errors, but also duplicated.


Although we already have strict checks on file extent items from
tree-checker, we never do proper alignment checks for extent maps.

The reason is mostly due to the failure of self tests and how hard it is
to touch them, especially for the inode self tests.

I have to say the inode self test is really something, the extent maps
of the self test makes no sense, and would be rejected by the
tree-checker if they show up in the real world.
Thankfully only the first few file extents items are invalid, the
remaining ones are totally fine.

The comments are not any better, after the first line, there are no more
aligned number at all, and the numbers are not offset by 1, but 3 or
even more.
Considering we're using decimals for most of our dump-tree and comments,
no one is really going to note the wrong numbers until one throw them
into python or whatever calculator one prefers.

The series will mostly rework the inode self tests file extent item
layout so that they represent the real world better, and update the
comments and make the poor person who needs to update that selftest
suffer less in the future.

Then address the minor problem in the extent-map selftest where
fs_info->sectorsize is never properly populated for the 4K based self
test.

With all those done, we can finally put a proper alignment check into 
validate_extent_map() which is called for every new, merged or replaced
extent map.

Qu Wenruo (3):
  btrfs: tests: remove invalid file extent map tests
  btrfs: tests: prepare extent map tests for strict alignment checks
  btrfs: add strict extent map alignment checks

 fs/btrfs/extent_map.c             | 12 ++++
 fs/btrfs/tests/extent-map-tests.c | 16 ++++--
 fs/btrfs/tests/inode-tests.c      | 96 ++++++++++++++++---------------
 3 files changed, 72 insertions(+), 52 deletions(-)

-- 
2.52.0

[PATCH v2 1/3] btrfs: tests: remove invalid file extent map tests

[Qu Wenruo]

In the inode self tests, there are several problems:

- Invalid file extents

  E.g. hole range [4K, 4K + 4) is completely invalid.

  Only inlined extent maps can have an unaligned ram_bytes, and even for
  that case, the generated extent map will use sectorsize as em->len.

- Unaligned hole after inlined extent

  The kernel never does this by itself, the current btrfs_get_extent()
  will only return a single inlined extent map that covers the first
  block.

- Incorrect numbers in the comment

  E.g. 12291 no matter if you add or dec 1, is not aligned to 4K.
  The properly number for 12K is 12288, I don't know why there is even a
  diff of 3, and this completely doesn't match the extent map we
  inserted later.

- Hard-to-modify sequence in setup_file_extents()

  If some unfortunate person, just like me, needs to modify
  setup_file_extents(), good luck not screwing up the file offset.

Fix them by:

- Remove invalid unaligned extent maps

  This mostly means remove the [4K, 4K + 4) hole case.
  The remaining ones are already properly aligned.

  This slightly changes the on-disk data extent allocation, with that
  removed, the regular extents at [4K, 8K) and [8K , 12K) can be merged.

  So also add a 4K gap between those two data extents to prevent em
  merge.

- Remove the implied hole after an inlined extent

  Just like what the kernel is doing for inlined extents in the real
  world.

- Update the commit using proper numbers with 'K' suffixes

  Since there is no unaligned range except the first inlined one, we can
  always use numbers with 'K' suffixes, which is way more easier to read,
  and will always be aligned to 1024 at least.

- Add comments in setup_file_extents()

  So that we're clear about the file offset for each test file extent.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tests/inode-tests.c | 96 ++++++++++++++++++------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index a4c2b7748b95..077501b93215 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -81,17 +81,20 @@ static void insert_inode_item_key(struct btrfs_root *root)
  * diagram of how the extents will look though this may not be possible we still
  * want to make sure everything acts normally (the last number is not inclusive)
  *
- * [0  - 6][     6 - 4096     ][ 4096 - 4100][4100 - 8195][8195  -  12291]
- * [inline][hole but no extent][    hole    ][   regular ][regular1 split]
+ * The numbers are using 4K fs block size as an example, the real test will scale
+ * all the extent maps (except the inlined one) according to the block size.
  *
- * [12291 - 16387][16387 - 24579][24579 - 28675][ 28675 - 32771][32771 - 36867 ]
- * [    hole    ][regular1 split][   prealloc ][   prealloc1  ][prealloc1 written]
+ * [ 0  - 6 ][ 6 - 4K       ][ 4K - 8K ][ 8K -  12K      ]
+ * [ inline ][ implied hole ][ regular ][ regular1 split ]
  *
- * [36867 - 45059][45059 - 53251][53251 - 57347][57347 - 61443][61443- 69635]
- * [  prealloc1  ][ compressed  ][ compressed1 ][    regular  ][ compressed1]
+ * [ 12K - 16K ][ 16K - 24K      ][ 24K - 28K ][ 28K - 32K ][ 32K - 36K         ]
+ * [ hole      ][ regular1 split ][ prealloc  ][ prealloc1 ][ prealloc1 written ]
  *
- * [69635-73731][   73731 - 86019   ][86019-90115]
- * [  regular  ][ hole but no extent][  regular  ]
+ * [ 36K - 44K ][ 44K - 52K  ][ 52K - 56K   ][ 56K - 60K ][ 60K - 68 K  ]
+ * [ prealloc1 ][ compressed ][ compressed1 ][ regular   ][ compressed1 ]
+ *
+ * [ 68K - 72K ][ 72K - 84K          ][ 84K - 88K ]
+ * [  regular  ][ hole but no extent ][ regular   ]
  */
 static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 {
@@ -100,6 +103,8 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	u64 offset = 0;
 
 	/*
+	 * Start 0, length 6, inlined.
+	 *
 	 * Tree-checker has strict limits on inline extents that they can only
 	 * exist at file offset 0, thus we can only have one inline file extent
 	 * at most.
@@ -109,20 +114,18 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	slot++;
 	offset = sectorsize;
 
-	/* Now another hole */
-	insert_extent(root, offset, 4, 4, 0, 0, 0, BTRFS_FILE_EXTENT_REG, 0,
-		      slot);
+	/* Start 1 * blocksize, length 1 * blocksize, regular. */
+	insert_extent(root, offset, sectorsize, sectorsize, 0,
+		      disk_bytenr, sectorsize, BTRFS_FILE_EXTENT_REG, 0, slot);
 	slot++;
-	offset += 4;
 
-	/* Now for a regular extent */
-	insert_extent(root, offset, sectorsize - 1, sectorsize - 1, 0,
-		      disk_bytenr, sectorsize - 1, BTRFS_FILE_EXTENT_REG, 0, slot);
-	slot++;
-	disk_bytenr += sectorsize;
-	offset += sectorsize - 1;
+	/* We don't want the regular em merged with the next one. */
+	disk_bytenr += 2 * sectorsize;
+	offset += sectorsize;
 
 	/*
+	 * Start 2 * blocksize, length 1 * blocksize, regular.
+	 *
 	 * Now for 3 extents that were split from a hole punch so we test
 	 * offsets properly.
 	 */
@@ -130,10 +133,14 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 		      4 * sectorsize, BTRFS_FILE_EXTENT_REG, 0, slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 3 * blocksize, length 1 * blocksize, regular, explicit hole. */
 	insert_extent(root, offset, sectorsize, sectorsize, 0, 0, 0,
 		      BTRFS_FILE_EXTENT_REG, 0, slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 4 * blocksize, length 2 * blocksize, regular. */
 	insert_extent(root, offset, 2 * sectorsize, 4 * sectorsize,
 		      2 * sectorsize, disk_bytenr, 4 * sectorsize,
 		      BTRFS_FILE_EXTENT_REG, 0, slot);
@@ -141,7 +148,7 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	offset += 2 * sectorsize;
 	disk_bytenr += 4 * sectorsize;
 
-	/* Now for a unwritten prealloc extent */
+	/* Start 6 * blocksize, length 1 * blocksize, preallocated. */
 	insert_extent(root, offset, sectorsize, sectorsize, 0, disk_bytenr,
 		sectorsize, BTRFS_FILE_EXTENT_PREALLOC, 0, slot);
 	slot++;
@@ -154,6 +161,8 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	disk_bytenr += 2 * sectorsize;
 
 	/*
+	 * Start 7 * blocksize, length 1 * blocksize, prealloc.
+	 *
 	 * Now for a partially written prealloc extent, basically the same as
 	 * the hole punch example above.  Ram_bytes never changes when you mark
 	 * extents written btw.
@@ -162,11 +171,15 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 		      4 * sectorsize, BTRFS_FILE_EXTENT_PREALLOC, 0, slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 8 * blocksize, length 1 * blocksize, regular. */
 	insert_extent(root, offset, sectorsize, 4 * sectorsize, sectorsize,
 		      disk_bytenr, 4 * sectorsize, BTRFS_FILE_EXTENT_REG, 0,
 		      slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 9 * blocksize, length 2 * blocksize, prealloc. */
 	insert_extent(root, offset, 2 * sectorsize, 4 * sectorsize,
 		      2 * sectorsize, disk_bytenr, 4 * sectorsize,
 		      BTRFS_FILE_EXTENT_PREALLOC, 0, slot);
@@ -174,7 +187,7 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	offset += 2 * sectorsize;
 	disk_bytenr += 4 * sectorsize;
 
-	/* Now a normal compressed extent */
+	/* Start 11 * blocksize, length 2 * blocksize, regular. */
 	insert_extent(root, offset, 2 * sectorsize, 2 * sectorsize, 0,
 		      disk_bytenr, sectorsize, BTRFS_FILE_EXTENT_REG,
 		      BTRFS_COMPRESS_ZLIB, slot);
@@ -183,17 +196,21 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	/* No merges */
 	disk_bytenr += 2 * sectorsize;
 
-	/* Now a split compressed extent */
+	/* Start 13 * blocksize, length 1 * blocksize, regular. */
 	insert_extent(root, offset, sectorsize, 4 * sectorsize, 0, disk_bytenr,
 		      sectorsize, BTRFS_FILE_EXTENT_REG,
 		      BTRFS_COMPRESS_ZLIB, slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 14 * blocksize, length 1 * blocksize, regular. */
 	insert_extent(root, offset, sectorsize, sectorsize, 0,
 		      disk_bytenr + sectorsize, sectorsize,
 		      BTRFS_FILE_EXTENT_REG, 0, slot);
 	slot++;
 	offset += sectorsize;
+
+	/* Start 15 * blocksize, length 2 * blocksize, regular. */
 	insert_extent(root, offset, 2 * sectorsize, 4 * sectorsize,
 		      2 * sectorsize, disk_bytenr, sectorsize,
 		      BTRFS_FILE_EXTENT_REG, BTRFS_COMPRESS_ZLIB, slot);
@@ -201,12 +218,19 @@ static void setup_file_extents(struct btrfs_root *root, u32 sectorsize)
 	offset += 2 * sectorsize;
 	disk_bytenr += 2 * sectorsize;
 
-	/* Now extents that have a hole but no hole extent */
+	/* Start 17 * blocksize, length 1 * blocksize, regular. */
 	insert_extent(root, offset, sectorsize, sectorsize, 0, disk_bytenr,
 		      sectorsize, BTRFS_FILE_EXTENT_REG, 0, slot);
 	slot++;
 	offset += 4 * sectorsize;
 	disk_bytenr += sectorsize;
+
+	/*
+	 * Start 18 * blocksize, length 3 * blocksize, implied hole (aka no
+	 * file extent item).
+	 *
+	 * Start 21 * blocksize, length 1 * blocksize, regular.
+	 */
 	insert_extent(root, offset, sectorsize, sectorsize, 0, disk_bytenr,
 		      sectorsize, BTRFS_FILE_EXTENT_REG, 0, slot);
 }
@@ -316,28 +340,6 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 	offset = em->start + em->len;
 	btrfs_free_extent_map(em);
 
-	em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
-	if (IS_ERR(em)) {
-		test_err("got an error when we shouldn't have");
-		goto out;
-	}
-	if (em->disk_bytenr != EXTENT_MAP_HOLE) {
-		test_err("expected a hole, got %llu", em->disk_bytenr);
-		goto out;
-	}
-	if (em->start != offset || em->len != 4) {
-		test_err(
-	"unexpected extent wanted start %llu len 4, got start %llu len %llu",
-			offset, em->start, em->len);
-		goto out;
-	}
-	if (em->flags != 0) {
-		test_err("unexpected flags set, want 0 have %u", em->flags);
-		goto out;
-	}
-	offset = em->start + em->len;
-	btrfs_free_extent_map(em);
-
 	/* Regular extent */
 	em = btrfs_get_extent(BTRFS_I(inode), NULL, offset, sectorsize);
 	if (IS_ERR(em)) {
@@ -348,10 +350,10 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 		test_err("expected a real extent, got %llu", em->disk_bytenr);
 		goto out;
 	}
-	if (em->start != offset || em->len != sectorsize - 1) {
+	if (em->start != offset || em->len != sectorsize) {
 		test_err(
-	"unexpected extent wanted start %llu len 4095, got start %llu len %llu",
-			offset, em->start, em->len);
+	"unexpected extent wanted start %llu len %u, got start %llu len %llu",
+			offset, sectorsize, em->start, em->len);
 		goto out;
 	}
 	if (em->flags != 0) {
-- 
2.52.0

[PATCH v2 2/3] btrfs: tests: prepare extent map tests for strict alignment checks

[Qu Wenruo]

Currently the extent map self tests have the following points that will
cause false alerts for the incoming strict extent map alignment checks:

- Incorrect inlined extent map size

  Which is not following what the kernel is doing for inlined extents,
  as btrfs_extent_item_to_extent_map() always uses the fs block size as
  the length, not the ram_bytes.

  Fix it by using SZ_4K as extent map's length.

- Incorrect btrfs_fs_info::sectorsize

  As we always use PAGE_SIZE, which can be values larger than 4K.
  Meanwhile all the immediate numbers used are based on 4K fs block size
  in the test case.

  Fix it by using fixed SZ_4K fs block size when allocating the dummy
  btrfs_fs_info.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tests/extent-map-tests.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/tests/extent-map-tests.c b/fs/btrfs/tests/extent-map-tests.c
index aabf825e8d7b..811f36d41101 100644
--- a/fs/btrfs/tests/extent-map-tests.c
+++ b/fs/btrfs/tests/extent-map-tests.c
@@ -173,9 +173,12 @@ static int test_case_2(struct btrfs_fs_info *fs_info, struct btrfs_inode *inode)
 		return -ENOMEM;
 	}
 
-	/* Add [0, 1K) */
+	/*
+	 * Add [0, 1K) which is inlined. And the extent map length must
+	 * be one block.
+	 */
 	em->start = 0;
-	em->len = SZ_1K;
+	em->len = SZ_4K;
 	em->disk_bytenr = EXTENT_MAP_INLINE;
 	em->disk_num_bytes = 0;
 	em->ram_bytes = SZ_1K;
@@ -219,7 +222,7 @@ static int test_case_2(struct btrfs_fs_info *fs_info, struct btrfs_inode *inode)
 
 	/* Add [0, 1K) */
 	em->start = 0;
-	em->len = SZ_1K;
+	em->len = SZ_4K;
 	em->disk_bytenr = EXTENT_MAP_INLINE;
 	em->disk_num_bytes = 0;
 	em->ram_bytes = SZ_1K;
@@ -235,7 +238,7 @@ static int test_case_2(struct btrfs_fs_info *fs_info, struct btrfs_inode *inode)
 		ret = -ENOENT;
 		goto out;
 	}
-	if (em->start != 0 || btrfs_extent_map_end(em) != SZ_1K ||
+	if (em->start != 0 || btrfs_extent_map_end(em) != SZ_4K ||
 	    em->disk_bytenr != EXTENT_MAP_INLINE) {
 		test_err(
 "case2 [0 1K]: ret %d return a wrong em (start %llu len %llu disk_bytenr %llu",
@@ -1131,8 +1134,11 @@ int btrfs_test_extent_map(void)
 	/*
 	 * Note: the fs_info is not set up completely, we only need
 	 * fs_info::fsid for the tracepoint.
+	 *
+	 * And all the immediate numbers are based on 4K blocksize,
+	 * thus we have to use 4K as sectorsize no matter the page size.
 	 */
-	fs_info = btrfs_alloc_dummy_fs_info(PAGE_SIZE, PAGE_SIZE);
+	fs_info = btrfs_alloc_dummy_fs_info(SZ_4K, SZ_4K);
 	if (!fs_info) {
 		test_std_err(TEST_ALLOC_FS_INFO);
 		return -ENOMEM;
-- 
2.52.0

[PATCH v2 3/3] btrfs: add strict extent map alignment checks

[Qu Wenruo]

Currently we do not check the alignment of extent_map structure.

The reasons are the inode and extent-map tests use unaligned values
for start offsets and lengths.

Thankfully those legacy problems are properly addressed by previous
patches, now we can finally put the alignment checks into
validate_extent_map().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_map.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index 7e38c23a0c1c..095a561d733f 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -319,8 +319,15 @@ static void dump_extent_map(struct btrfs_fs_info *fs_info, const char *prefix,
 /* Internal sanity checks for btrfs debug builds. */
 static void validate_extent_map(struct btrfs_fs_info *fs_info, struct extent_map *em)
 {
+	const u32 blocksize = fs_info->sectorsize;
+
 	if (!IS_ENABLED(CONFIG_BTRFS_DEBUG))
 		return;
+
+	if (!IS_ALIGNED(em->start, blocksize) ||
+	    !IS_ALIGNED(em->len, blocksize))
+		dump_extent_map(fs_info, "unaligned start offset or length members", em);
+
 	if (em->disk_bytenr < EXTENT_MAP_LAST_BYTE) {
 		if (em->disk_num_bytes == 0)
 			dump_extent_map(fs_info, "zero disk_num_bytes", em);
@@ -334,6 +341,11 @@ static void validate_extent_map(struct btrfs_fs_info *fs_info, struct extent_map
 			dump_extent_map(fs_info,
 		"ram_bytes mismatch with disk_num_bytes for non-compressed em",
 					em);
+		if (!IS_ALIGNED(em->disk_bytenr, blocksize) ||
+		    !IS_ALIGNED(em->disk_num_bytes, blocksize) ||
+		    !IS_ALIGNED(em->offset, blocksize) ||
+		    !IS_ALIGNED(em->ram_bytes, blocksize))
+			dump_extent_map(fs_info, "unaligned members", em);
 	} else if (em->offset) {
 		dump_extent_map(fs_info, "non-zero offset for hole/inline", em);
 	}
-- 
2.52.0

Re: [PATCH v2 0/3] btrfs: apply strict alignment checks to extent maps

[Filipe Manana]

On Wed, Jan 21, 2026 at 11:15 PM Qu Wenruo <wqu@suse.com> wrote:
>
> [CHANGELOG]
> v2:
> - Grammar fixes
>
>   Not only spelling fixes, but also extra new line after an item of a
>   list.
>
> - Remove unnecessary ASSERT()s inside inode tests
>
>   There is already enough comments on each test file extent item.
>
> - Remove some unnecessary commit messages from patch 3
>
>   Which not only introduces grammar errors, but also duplicated.
>
>
> Although we already have strict checks on file extent items from
> tree-checker, we never do proper alignment checks for extent maps.
>
> The reason is mostly due to the failure of self tests and how hard it is
> to touch them, especially for the inode self tests.
>
> I have to say the inode self test is really something, the extent maps
> of the self test makes no sense, and would be rejected by the
> tree-checker if they show up in the real world.
> Thankfully only the first few file extents items are invalid, the
> remaining ones are totally fine.
>
> The comments are not any better, after the first line, there are no more
> aligned number at all, and the numbers are not offset by 1, but 3 or
> even more.
> Considering we're using decimals for most of our dump-tree and comments,
> no one is really going to note the wrong numbers until one throw them
> into python or whatever calculator one prefers.
>
> The series will mostly rework the inode self tests file extent item
> layout so that they represent the real world better, and update the
> comments and make the poor person who needs to update that selftest
> suffer less in the future.
>
> Then address the minor problem in the extent-map selftest where
> fs_info->sectorsize is never properly populated for the 4K based self
> test.
>
> With all those done, we can finally put a proper alignment check into
> validate_extent_map() which is called for every new, merged or replaced
> extent map.
>
> Qu Wenruo (3):
>   btrfs: tests: remove invalid file extent map tests
>   btrfs: tests: prepare extent map tests for strict alignment checks
>   btrfs: add strict extent map alignment checks

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

>
>  fs/btrfs/extent_map.c             | 12 ++++
>  fs/btrfs/tests/extent-map-tests.c | 16 ++++--
>  fs/btrfs/tests/inode-tests.c      | 96 ++++++++++++++++---------------
>  3 files changed, 72 insertions(+), 52 deletions(-)
>
> --
> 2.52.0
>
>