[PATCH v2 0/5] btrfs: detect and fix the ram_bytes mismatch

[PATCH v2 0/5] btrfs: detect and fix the ram_bytes mismatch

[Qu Wenruo]

[CHANGELOG]
v2:
- Add the missing patch fixing ram_bytes
  Now the 2nd patch would ignore the incorrect value and use a correct
  one from btrfs_file_extent_item::disk_num_bytes.

- Update the commit messages to fix my usual "would" and other grammar
  errors

There is a long existing mismatch between ram_bytes and disk_num_bytes
for regular non-compressed data extents.

It turns out to be caused by truncated ordered extents, which modified
ram_bytes unnecessarily.

Thankfully this is not going to cause any data corruption or whatever,
kernel can handle it correctly without any extra problem.
It's only a small violation on the on-disk format.

This series would fix by:

- Cleanup the @bytenr usage inside btrfs_extent_item_to_extent_map()
- Override the ram_bytes when reading file extent items from disk
  So that we always get correct extent maps even if the on-disk one is
  incorrect.
- Add an extra check on extent_map members
- Add the proper fix for the ram_bytes mismatch
- Add a tree-checker for the ram_bytes mismatch
  Since we can have on-disk ram_bytes incorrect already, this check is
  only for DEBUG and ASSERT builds, and it won't report error but only
  does a kernel warning for us to catch.

Qu Wenruo (5):
  btrfs: cleanup the bytenr usage inside
    btrfs_extent_item_to_extent_map()
  btrfs: ignore incorrect btrfs_file_extent_item::ram_bytes
  btrfs: make validate_extent_map() to catch ram_bytes mismatch
  btrfs: fix the ram_bytes assignment for truncated ordered extents
  btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check

 fs/btrfs/extent_map.c   |  5 +++++
 fs/btrfs/file-item.c    | 16 +++++++++++-----
 fs/btrfs/inode.c        |  4 +---
 fs/btrfs/tree-checker.c | 18 ++++++++++++++++++
 4 files changed, 35 insertions(+), 8 deletions(-)

-- 
2.45.2

[PATCH v2 1/5] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map()

[Qu Wenruo]

[HICCUP]
Before commit 85de2be7129c ("btrfs: remove extent_map::block_start
member"), we utilized @bytenr variable inside
btrfs_extent_item_to_extent_map() to calculate block_start.

But that commit removed block_start completely, we have no need to
advance @bytenr at all.

[ENHANCEMENT]
- Rename @bytenr as @disk_bytenr
- Only declare @disk_bytenr inside the if branch
- Make @disk_bytenr const and remove the modification on it

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file-item.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 55703c833f3d..2cc61c792ee6 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -1281,7 +1281,6 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 	const int slot = path->slots[0];
 	struct btrfs_key key;
 	u64 extent_start;
-	u64 bytenr;
 	u8 type = btrfs_file_extent_type(leaf, fi);
 	int compress_type = btrfs_file_extent_compression(leaf, fi);
 
@@ -1291,22 +1290,22 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 	em->generation = btrfs_file_extent_generation(leaf, fi);
 	if (type == BTRFS_FILE_EXTENT_REG ||
 	    type == BTRFS_FILE_EXTENT_PREALLOC) {
+		const u64 disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
+
 		em->start = extent_start;
 		em->len = btrfs_file_extent_end(path) - extent_start;
-		bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
-		if (bytenr == 0) {
+		if (disk_bytenr == 0) {
 			em->disk_bytenr = EXTENT_MAP_HOLE;
 			em->disk_num_bytes = 0;
 			em->offset = 0;
 			return;
 		}
-		em->disk_bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
+		em->disk_bytenr = disk_bytenr;
 		em->disk_num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
 		em->offset = btrfs_file_extent_offset(leaf, fi);
 		if (compress_type != BTRFS_COMPRESS_NONE) {
 			extent_map_set_compression(em, compress_type);
 		} else {
-			bytenr += btrfs_file_extent_offset(leaf, fi);
 			if (type == BTRFS_FILE_EXTENT_PREALLOC)
 				em->flags |= EXTENT_FLAG_PREALLOC;
 		}
-- 
2.45.2

[PATCH v2 2/5] btrfs: ignore incorrect btrfs_file_extent_item::ram_bytes

[Qu Wenruo]

[HICCUP]
Kernels can create file extent items with incorrect ram_bytes like this:

	item 6 key (257 EXTENT_DATA 0) itemoff 15816 itemsize 53
		generation 7 type 1 (regular)
		extent data disk byte 13631488 nr 32768
		extent data offset 0 nr 4096 ram 4096
		extent compression 0 (none)

Thankfully kernel can handle them properly, as in that case ram_bytes is
not utilized at all.

[ENHANCEMENT]
Since the hiccup is not going to cause any data-loss and is only a minor
violation of on-disk format, here we only need to ignore the incorrect
ram_bytes value, and use the correct one from
btrfs_file_extent_item::disk_num_bytes.

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

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 2cc61c792ee6..e815fefaffe1 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -1306,6 +1306,13 @@ void btrfs_extent_item_to_extent_map(struct btrfs_inode *inode,
 		if (compress_type != BTRFS_COMPRESS_NONE) {
 			extent_map_set_compression(em, compress_type);
 		} else {
+			/*
+			 * Older kernels can create regular non-hole data
+			 * extents with ram_bytes smaller than disk_num_bytes.
+			 * Not a big deal, just always use disk_num_bytes
+			 * for ram_bytes.
+			 */
+			em->ram_bytes = em->disk_num_bytes;
 			if (type == BTRFS_FILE_EXTENT_PREALLOC)
 				em->flags |= EXTENT_FLAG_PREALLOC;
 		}
-- 
2.45.2

[PATCH v2 3/5] btrfs: make validate_extent_map() to catch ram_bytes mismatch

[Qu Wenruo]

Previously validate_extent_map() is only to catch bugs related to
extent_map member cleanups.

But with recent btrfs-check enhancement to catch ram_bytes mismatch with
disk_num_bytes, it would be much better to catch such extent maps
earlier.

So this patch adds extra ram_bytes validation for extent maps.

Please note that, older filesystems with such mismatch won't trigger this error:

- extent_map::ram_bytes is already fixed
  Previous patch has already fixed the ram_bytes for affected file
  extents.

So this enhanced sanity check should not affect end users.

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

diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
index b869a0ee24d2..6961cc73fe3f 100644
--- a/fs/btrfs/extent_map.c
+++ b/fs/btrfs/extent_map.c
@@ -317,6 +317,11 @@ static void validate_extent_map(struct btrfs_fs_info *fs_info, struct extent_map
 		if (em->offset + em->len > em->disk_num_bytes &&
 		    !extent_map_is_compressed(em))
 			dump_extent_map(fs_info, "disk_num_bytes too small", em);
+		if (!extent_map_is_compressed(em) &&
+		    em->ram_bytes != em->disk_num_bytes)
+			dump_extent_map(fs_info,
+		"ram_bytes mismatch with disk_num_bytes for non-compressed em",
+					em);
 	} else if (em->offset) {
 		dump_extent_map(fs_info, "non-zero offset for hole/inline", em);
 	}
-- 
2.45.2

[PATCH v2 4/5] btrfs: fix the ram_bytes assignment for truncated ordered extents

[Qu Wenruo]

[HICCUP]
After adding extra checks on btrfs_file_extent_item::ram_bytes to
tree-checker, running fsstress leads to tree-checker warning at write time,
as we created file extent items with an invalid ram_bytes.

All those offending file extents have offset 0, and ram_bytes matching
num_bytes, and smaller than disk_num_bytes.

This would also trigger the recently enhanced btrfs-check, which catches
such mismatches and report them as minor errors.

[CAUSE]
When a folio/page is invalidated and it is part of a submitted OE, we
mark the OE truncated just to the beginning of the folio/page.

And for truncated OE, we insert the file extent item with incorrect
value for ram_bytes (using num_bytes instead of the usual value).

This is not a big deal for end users, as we do not utilize the ram_bytes
field for regular non-compressed extents.
This mismatch is just a small violation against on-disk format.

[FIX]
Fix it by removing the override on btrfs_file_extent_item::ram_bytes.

Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index d6c43120c5d3..45f77ae8963f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -3018,10 +3018,8 @@ static int insert_ordered_extent_file_extent(struct btrfs_trans_handle *trans,
 	btrfs_set_stack_file_extent_disk_num_bytes(&stack_fi,
 						   oe->disk_num_bytes);
 	btrfs_set_stack_file_extent_offset(&stack_fi, oe->offset);
-	if (test_bit(BTRFS_ORDERED_TRUNCATED, &oe->flags)) {
+	if (test_bit(BTRFS_ORDERED_TRUNCATED, &oe->flags))
 		num_bytes = oe->truncated_len;
-		ram_bytes = num_bytes;
-	}
 	btrfs_set_stack_file_extent_num_bytes(&stack_fi, num_bytes);
 	btrfs_set_stack_file_extent_ram_bytes(&stack_fi, ram_bytes);
 	btrfs_set_stack_file_extent_compression(&stack_fi, oe->compress_type);
-- 
2.45.2

[PATCH v2 5/5] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check

[Qu Wenruo]

This is to ensure non-compressed file extents (both regular and
prealloc) should have matching ram_bytes and disk_num_bytes.

This is only for CONFIG_BTRFS_DEBUG and CONFIG_BTRFS_ASSERT case,
furthermore this will not return error, but just a kernel warning to
inform developers.

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index a2c3651a3d8f..32ca692d9e20 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -340,6 +340,24 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 		}
 	}
 
+	/*
+	 * For non-compressed data extents, ram_bytes should match its
+	 * disk_num_bytes.
+	 * However we do not really utilize ram_bytes in this case, so this check
+	 * is only optional for DEBUG builds for developers to catch the
+	 * unexpected behaviors.
+	 */
+	if (IS_ENABLED(CONFIG_BTRFS_DEBUG) &&
+	    btrfs_file_extent_compression(leaf, fi) == BTRFS_COMPRESS_NONE &&
+	    btrfs_file_extent_disk_bytenr(leaf, fi)) {
+		if (WARN_ON(btrfs_file_extent_ram_bytes(leaf, fi) !=
+			    btrfs_file_extent_disk_num_bytes(leaf, fi)))
+			file_extent_err(leaf, slot,
+"mismatch ram_bytes (%llu) and disk_num_bytes (%llu) for non-compressed extent",
+					btrfs_file_extent_ram_bytes(leaf, fi),
+					btrfs_file_extent_disk_num_bytes(leaf, fi));
+	}
+
 	return 0;
 }
 
-- 
2.45.2

Re: [PATCH v2 0/5] btrfs: detect and fix the ram_bytes mismatch

[Filipe Manana]

On Wed, Jun 26, 2024 at 2:48 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [CHANGELOG]
> v2:
> - Add the missing patch fixing ram_bytes
>   Now the 2nd patch would ignore the incorrect value and use a correct
>   one from btrfs_file_extent_item::disk_num_bytes.
>
> - Update the commit messages to fix my usual "would" and other grammar
>   errors
>
> There is a long existing mismatch between ram_bytes and disk_num_bytes
> for regular non-compressed data extents.
>
> It turns out to be caused by truncated ordered extents, which modified
> ram_bytes unnecessarily.
>
> Thankfully this is not going to cause any data corruption or whatever,
> kernel can handle it correctly without any extra problem.
> It's only a small violation on the on-disk format.
>
> This series would fix by:
>
> - Cleanup the @bytenr usage inside btrfs_extent_item_to_extent_map()
> - Override the ram_bytes when reading file extent items from disk
>   So that we always get correct extent maps even if the on-disk one is
>   incorrect.
> - Add an extra check on extent_map members
> - Add the proper fix for the ram_bytes mismatch
> - Add a tree-checker for the ram_bytes mismatch
>   Since we can have on-disk ram_bytes incorrect already, this check is
>   only for DEBUG and ASSERT builds, and it won't report error but only
>   does a kernel warning for us to catch.
>
> Qu Wenruo (5):
>   btrfs: cleanup the bytenr usage inside
>     btrfs_extent_item_to_extent_map()
>   btrfs: ignore incorrect btrfs_file_extent_item::ram_bytes
>   btrfs: make validate_extent_map() to catch ram_bytes mismatch
>   btrfs: fix the ram_bytes assignment for truncated ordered extents
>   btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check
>
>  fs/btrfs/extent_map.c   |  5 +++++
>  fs/btrfs/file-item.c    | 16 +++++++++++-----
>  fs/btrfs/inode.c        |  4 +---
>  fs/btrfs/tree-checker.c | 18 ++++++++++++++++++
>  4 files changed, 35 insertions(+), 8 deletions(-)

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

Looks good, thanks.

>
> --
> 2.45.2
>
>