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