Linux Btrfs filesystem development
 help / color / mirror / Atom feed
* [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch
@ 2024-06-25  5:07 Qu Wenruo
  2024-06-25  5:07 ` [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map() Qu Wenruo
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25  5:07 UTC (permalink / raw)
  To: linux-btrfs

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 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 (4):
  btrfs: cleanup the bytenr usage inside
    btrfs_extent_item_to_extent_map()
  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    |  9 ++++-----
 fs/btrfs/inode.c        |  4 +---
 fs/btrfs/tree-checker.c | 19 +++++++++++++++++++
 4 files changed, 29 insertions(+), 8 deletions(-)

-- 
2.45.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map()
  2024-06-25  5:07 [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch Qu Wenruo
@ 2024-06-25  5:07 ` Qu Wenruo
  2024-06-25 10:19   ` Filipe Manana
  2024-06-25  5:07 ` [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch Qu Wenruo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25  5:07 UTC (permalink / raw)
  To: linux-btrfs

[PROBLEMS]
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.

Furthermore with recent enhanced btrfs-progs check for ram_bytes
mimsatch, it turns out that for truncated ordered extents, their
ram_bytes can be smaller than disk_num_bytes.

[ENHANCEMENT]
Thankfully all above problems are not really going to affect end users,
fix them by:

- Declare @bytenr only inside the if branch and make it const
  So we can remove the unnecessary advance of @bytenr.

- Manually override extent_map::ram_bytes using disk_num_bytes
  This is for non-compressed regular/preallocated extents.

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch
  2024-06-25  5:07 [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch Qu Wenruo
  2024-06-25  5:07 ` [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map() Qu Wenruo
@ 2024-06-25  5:07 ` Qu Wenruo
  2024-06-25 10:25   ` Filipe Manana
  2024-06-25  5:07 ` [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents Qu Wenruo
  2024-06-25  5:07 ` [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check Qu Wenruo
  3 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25  5:07 UTC (permalink / raw)
  To: linux-btrfs

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 would add 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 when reading from disk
  At btrfs_extent_item_to_extent_map() we override extent_map::ram_bytes
  to disk_num_bytes for non-compressed regular/preallocated extents.

So this enhanced sanity checks 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents
  2024-06-25  5:07 [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch Qu Wenruo
  2024-06-25  5:07 ` [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map() Qu Wenruo
  2024-06-25  5:07 ` [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch Qu Wenruo
@ 2024-06-25  5:07 ` Qu Wenruo
  2024-06-25 10:32   ` Filipe Manana
  2024-06-25  5:07 ` [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check Qu Wenruo
  3 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25  5:07 UTC (permalink / raw)
  To: linux-btrfs

[BUG]
After adding extra checks on btrfs_file_extent_item::ram_bytes to
tree-checker, running fsstress with multiple threads can lead to
tree-checker warning at write time, as we created file extent items with
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 would
catch such mismatch 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.

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check
  2024-06-25  5:07 [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch Qu Wenruo
                   ` (2 preceding siblings ...)
  2024-06-25  5:07 ` [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents Qu Wenruo
@ 2024-06-25  5:07 ` Qu Wenruo
  2024-06-25 10:37   ` Filipe Manana
  3 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25  5:07 UTC (permalink / raw)
  To: linux-btrfs

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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index a2c3651a3d8f..cddabd9a0e37 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -340,6 +340,25 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 		}
 	}
 
+	/*
+	 * For non-compressed data extents, ram_bytes should match its disk_bytenr.
+	 * However we do not really utilize ram_bytes in this case, so this check
+	 * is only optional for DEBUG+ASSERT builds for developers to catch the
+	 * unexpected behaviors.
+	 */
+	if (IS_ENABLED(CONFIG_BTRFS_DEBUG) && IS_ENABLED(CONFIG_BTRFS_ASSERT) &&
+	    btrfs_file_extent_compression(leaf, fi) == BTRFS_COMPRESS_NONE &&
+	    btrfs_file_extent_disk_bytenr(leaf, fi)) {
+		if (unlikely(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));
+			WARN_ON(1);
+		}
+	}
+
 	return 0;
 }
 
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map()
  2024-06-25  5:07 ` [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map() Qu Wenruo
@ 2024-06-25 10:19   ` Filipe Manana
  2024-06-25 10:48     ` Qu Wenruo
  0 siblings, 1 reply; 10+ messages in thread
From: Filipe Manana @ 2024-06-25 10:19 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [PROBLEMS]

I wouldn't call this "problems", there are no bugs here or anything harmful.

> 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.
>
> Furthermore with recent enhanced btrfs-progs check for ram_bytes
> mimsatch, it turns out that for truncated ordered extents, their

mimsatch -> mismatch

> ram_bytes can be smaller than disk_num_bytes.
>
> [ENHANCEMENT]
> Thankfully all above problems are not really going to affect end users,
> fix them by:
>
> - Declare @bytenr only inside the if branch and make it const
>   So we can remove the unnecessary advance of @bytenr.
>
> - Manually override extent_map::ram_bytes using disk_num_bytes
>   This is for non-compressed regular/preallocated extents.

I don't see anything in the patch changing ram_bytes.
Perhaps this is from an early patch version never submitted, or from
some other patch?

The code itself looks good.
Thanks.

>
> 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
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch
  2024-06-25  5:07 ` [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch Qu Wenruo
@ 2024-06-25 10:25   ` Filipe Manana
  0 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2024-06-25 10:25 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>
> 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 would add extra ram_bytes validation for extent maps.

would add -> adds

>
> Please note that, older filesystems with such mismatch won't trigger this error:
>
> - extent_map::ram_bytes is already fixed when reading from disk
>   At btrfs_extent_item_to_extent_map() we override extent_map::ram_bytes
>   to disk_num_bytes for non-compressed regular/preallocated extents.

At btrfs_extent_item_to_extent_map() we override ram_bytes for any
type of extent, it's not conditional on anything.
So this is confusing because it doesn't match the code.

>
> So this enhanced sanity checks should not affect end users.

checks -> check, otherwise "this" should be "these" (but there's only
one check).

The code looks good, thanks.

>
> 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
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents
  2024-06-25  5:07 ` [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents Qu Wenruo
@ 2024-06-25 10:32   ` Filipe Manana
  0 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2024-06-25 10:32 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> After adding extra checks on btrfs_file_extent_item::ram_bytes to
> tree-checker, running fsstress with multiple threads can lead to

It's irrelevant to mention multiple threads, that's not necessary to
cause the problem.

> tree-checker warning at write time, as we created file extent items with
> ram_bytes.

This last part of the sentence makes no sense "we created file extent
items with ram_bytes" - they must always have ram_bytes.
I think you meant to say "with an invalid ram_bytes value".

>
> 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 would
> catch such mismatch and report them as minor errors.

mismatch -> mismatches

>
> [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.
>
> 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;
> -       }

The code looks good, with those updates to the the changelog:

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

Thanks.

>         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
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check
  2024-06-25  5:07 ` [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check Qu Wenruo
@ 2024-06-25 10:37   ` Filipe Manana
  0 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2024-06-25 10:37 UTC (permalink / raw)
  To: Qu Wenruo; +Cc: linux-btrfs

On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>
> 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,

I would leave just for DEBUG.

> 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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index a2c3651a3d8f..cddabd9a0e37 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -340,6 +340,25 @@ static int check_extent_data_item(struct extent_buffer *leaf,
>                 }
>         }
>
> +       /*
> +        * For non-compressed data extents, ram_bytes should match its disk_bytenr.
> +        * However we do not really utilize ram_bytes in this case, so this check
> +        * is only optional for DEBUG+ASSERT builds for developers to catch the
> +        * unexpected behaviors.
> +        */
> +       if (IS_ENABLED(CONFIG_BTRFS_DEBUG) && IS_ENABLED(CONFIG_BTRFS_ASSERT) &&
> +           btrfs_file_extent_compression(leaf, fi) == BTRFS_COMPRESS_NONE &&
> +           btrfs_file_extent_disk_bytenr(leaf, fi)) {
> +               if (unlikely(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));
> +                       WARN_ON(1);

Instead of adding here a WARN_ON(1) and unlikely in the if condition,
just make the if condition use WARN_ON:

if (WARN_ON(btrfs_file_extent_ram_bytes(leaf, fi) !=
btrfs_file_extent_disk_num_bytes(leaf, fi)))

The WARN_ON includes the unlikely, and further this makes it conform
to our preference of having error messages after stack traces.

Thanks.

> +               }
> +       }
> +
>         return 0;
>  }
>
> --
> 2.45.2
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map()
  2024-06-25 10:19   ` Filipe Manana
@ 2024-06-25 10:48     ` Qu Wenruo
  0 siblings, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2024-06-25 10:48 UTC (permalink / raw)
  To: Filipe Manana, Qu Wenruo; +Cc: linux-btrfs



在 2024/6/25 19:49, Filipe Manana 写道:
> On Tue, Jun 25, 2024 at 6:08 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> [PROBLEMS]
>
> I wouldn't call this "problems", there are no bugs here or anything harmful.
>
>> 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.
>>
>> Furthermore with recent enhanced btrfs-progs check for ram_bytes
>> mimsatch, it turns out that for truncated ordered extents, their
>
> mimsatch -> mismatch
>
>> ram_bytes can be smaller than disk_num_bytes.
>>
>> [ENHANCEMENT]
>> Thankfully all above problems are not really going to affect end users,
>> fix them by:
>>
>> - Declare @bytenr only inside the if branch and make it const
>>    So we can remove the unnecessary advance of @bytenr.
>>
>> - Manually override extent_map::ram_bytes using disk_num_bytes
>>    This is for non-compressed regular/preallocated extents.
>
> I don't see anything in the patch changing ram_bytes.
> Perhaps this is from an early patch version never submitted, or from
> some other patch?

My bad, when re-editing the commit message, I got confused with later
patches.

I will remove the ram_bytes related part.

Thanks,
Qu
>
> The code itself looks good.
> Thanks.
>
>>
>> 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
>>
>>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-06-25 10:48 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-25  5:07 [PATCH 0/4] btrfs: detect and fix the ram_bytes mismatch Qu Wenruo
2024-06-25  5:07 ` [PATCH 1/4] btrfs: cleanup the bytenr usage inside btrfs_extent_item_to_extent_map() Qu Wenruo
2024-06-25 10:19   ` Filipe Manana
2024-06-25 10:48     ` Qu Wenruo
2024-06-25  5:07 ` [PATCH 2/4] btrfs: make validate_extent_map() to catch ram_bytes mismatch Qu Wenruo
2024-06-25 10:25   ` Filipe Manana
2024-06-25  5:07 ` [PATCH 3/4] btrfs: fix the ram_bytes assignment for truncated ordered extents Qu Wenruo
2024-06-25 10:32   ` Filipe Manana
2024-06-25  5:07 ` [PATCH 4/4] btrfs: tree-checker: add extra ram_bytes and disk_num_bytes check Qu Wenruo
2024-06-25 10:37   ` Filipe Manana

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox