btrfs: fix inlined file extent items in data reloc
Subsystems (2)
v2 cover btrfs: fix inlined file extent items in data reloc
[CHANGELOG]
v2:
- Add more explanation on why we can created inlined extents even if relocation is preallocating space
- Add the fixes: tag since the behavior is caused by a specific commit
There is a syzbot report that an inlined file extent item in a data reloc inode triggered a sanity check in get_new_location().
It turns out that we can create inlined file extents for data reloc inodes in the first place after commit 3eaf5f082c4c ("btrfs: extract inlined creation into a dedicated delalloc helper").
So the first patch will avoid compression for data reloc inodes first, then the second patch to reject inlined file extent items in get_new_location(), making the checks more robust.
Qu Wenruo (2):
btrfs: do not try compression for data reloc inodes
btrfs: reject inline file extent item in get_new_location()
fs/btrfs/btrfs_inode.h | 2 ++ fs/btrfs/relocation.c | 7 +++++++ 2 files changed, 9 insertions(+)
Signature · 1 line
2.54.0
PATCH v2 1/2 btrfs: do not try compression for data reloc inodes
[BUG]
There is a syzbot report that the check inside get_new_location() triggered:
BTRFS info (device loop0): found 31 extents, stage: move data extents
BTRFS info (device loop0): leaf 8908800 gen 16 total ptrs 28 free space 1676 owner 18446744073709551607
item 0 key (256 INODE_ITEM 0) itemoff 3835 itemsize 160
inode generation 5 transid 0 size 0 nbytes 0
block group 0 mode 40755 links 1 uid 0 gid 0
rdev 0 sequence 0 flags 0x0
atime 1669132761.0
ctime 1669132761.0
mtime 1669132761.0
otime 0.0
item 1 key (256 INODE_REF 256) itemoff 3823 itemsize 12
index 0 name_len 2
item 2 key (258 INODE_ITEM 0) itemoff 3663 itemsize 160
inode generation 1 transid 16 size 733184 nbytes 106496
block group 0 mode 100600 links 0 uid 0 gid 0
rdev 0 sequence 24 flags 0x18
item 3 key (258 EXTENT_DATA 0) itemoff 3595 itemsize 68
generation 16 type 0
inline extent data size 47 ram_bytes 4096 compression 1
[...]
item 27 key (18446744073709551611 ORPHAN_ITEM 258) itemoff 2376 itemsize 0
BTRFS error (device loop0): unexpected non-zero offset in file extent item for data reloc inode 258 key offset 0 offset 9277520992061368337
------------[ cut here ]------------
btrfs_abort_should_print_stack(__error)[CAUSE]
The above dump tree shows the first file extent item is inlined, which should make no sense for data reloc inodes, as such inodes are just representing where the data extents are in the relocation destination chunk.
However the relocation path is just preallocate space for each block, then dirty them, cluster by cluster.
It's possible to have a single block at the beginning of the block group, and no other block in the same cluster.
Then relocation will preallocate a file extent for that block, dirty the first block. Then memory pressure forces the data reloc inode to be written back, before any other blocks being dirtied/allocated.
Finally commit 3eaf5f082c4c ("btrfs: extract inlined creation into a dedicated delalloc helper") changed the timing of delalloc, before that commit we always try NOCOW first, so that dirtied block will be written back into the preallocated space.
But with that commit, we always try inline first, and since compression is forced, we try compressing the first block, and then inline the compressed data, resulting in the above inlined file extent in data reloc tree.
Then the check in get_new_location() will check the file offset, without checking if the file extent is inlined or not, resulting the above failure.
[FIX]
Do not allow compression for data reloc inodes in the first place.
--- fs/btrfs/btrfs_inode.h | 2 ++ 1 file changed, 2 insertions(+)
fs/btrfs/btrfs_inode.h +2 -0
@@ -476,6 +476,8 @@ static inline bool btrfs_inode_can_compress(const struct btrfs_inode *inode)if (inode->flags &BTRFS_INODE_NODATACOW || inode->flags &BTRFS_INODE_NODATASUM )return false; +if (btrfs_root_id (inode->root) ==BTRFS_DATA_RELOC_TREE_OBJECTID ) +return false;return true; }
Signature · 1 line
2.54.0
Re: [PATCH v2 1/2] btrfs: do not try compression for data reloc inodes
Quoted 30 lines · Qu Wenruo
[BUG]
There is a syzbot report that the check insideget_new_location()triggered:BTRFS info (device loop0): found 31 extents, stage: move data extents
BTRFS info (device loop0): leaf 8908800 gen 16 total ptrs 28 free space 1676 owner 18446744073709551607item 0 key (256 INODE_ITEM 0) itemoff 3835 itemsize 160 inode generation 5 transid 0 size 0 nbytes 0 block group 0 mode 40755 links 1 uid 0 gid 0 rdev 0 sequence 0 flags 0x0 atime 1669132761.0 ctime 1669132761.0 mtime 1669132761.0 otime 0.0 item 1 key (256 INODE_REF 256) itemoff 3823 itemsize 12 index 0 name_len 2 item 2 key (258 INODE_ITEM 0) itemoff 3663 itemsize 160 inode generation 1 transid 16 size 733184 nbytes 106496 block group 0 mode 100600 links 0 uid 0 gid 0 rdev 0 sequence 24 flags 0x18 item 3 key (258 EXTENT_DATA 0) itemoff 3595 itemsize 68 generation 16 type 0 inline extent data size 47 ram_bytes 4096 compression 1 [...] item 27 key (18446744073709551611 ORPHAN_ITEM 258) itemoff 2376 itemsize 0 BTRFS error (device loop0): unexpected non-zero offset in file extent item for data reloc inode 258 key offset 0 offset 9277520992061368337 ------------[ cut here ]------------ btrfs_abort_should_print_stack(__error)
Quoted 6 lines
[CAUSE]
The above dump tree shows the first file extent item is inlined, which should make no sense for data reloc inodes, as such inodes are just representing where the data extents are in the relocation destination chunk.However the relocation path is just preallocate space for each block,
"is just preallocate" is weird, more like "preallocates"
Quoted 10 lines
then dirty them, cluster by cluster.
It's possible to have a single block at the beginning of the block group, and no other block in the same cluster.Then relocation will preallocate a file extent for that block, dirty the first block. Then memory pressure forces the data reloc inode to be written back, before any other blocks being dirtied/allocated.
Finally commit
3eaf5f082c4c("btrfs: extract inlined creation into a dedicated delalloc helper") changed the timing of delalloc, before that commit we
This isn't about changing the timing but rather changing the order of the rule set for inline/compression/nocow.
Saying timing is confusing.
Quoted 8 lines
always try NOCOW first, so that dirtied block will be written back into the preallocated space.
But with that commit, we always try inline first, and since compression is forced, we try compressing the first block, and then inline the compressed data, resulting in the above inlined file extent in data reloc tree.
Quoted 6 lines
Then the check in
get_new_location()will check the file offset, without checking if the file extent is inlined or not, resulting the above failure.[FIX]
Do not allow compression for data reloc inodes in the first place.
I would add an explanation here about why disallowing compression prevents the inline extent, as this is non-obvious and involves a convoluted path.
So adding this new check in btrfs_inode_can_compress() makes run_delalloc_inline() skip the compression if statement. Then, when it calls can_cow_file_range_inline() that returns false because the data size is >= sector size, causing run_delalloc_inline() to return 1, signaling to the caller, btrfs_run_delalloc_range(), it cannot inline and must follow the nocow path.
Thanks.
Quoted 27 lines
Reported-by [email protected]Fixes 3eaf5f082c4c ("btrfs: extract inlined creation into a dedicated delalloc helper")Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/btrfs_inode.h | 2 ++ 1 file changed, 2 insertions(+)fs/btrfs/btrfs_inode.h +2
-1@@ -476,6 +476,8 @@ static inline bool btrfs_inode_can_compress(const struct btrfs_inode *inode)if (inode->flags &BTRFS_INODE_NODATACOW || inode->flags &BTRFS_INODE_NODATASUM )return false; +if (btrfs_root_id (inode->root) ==BTRFS_DATA_RELOC_TREE_OBJECTID ) +return false;return true; } --2.54.0
PATCH v2 2/2 btrfs: reject inline file extent item in get_new_location()
In reply to Qu WenruoCommit a6908f88c9da ("btrfs: validate data reloc tree file extent item members") introduced extra checks on file extent items for data reloc inodes, but it checks file extent offset without checking if the file extent is inlined.
This can lead to either false alerts (as the offset member is inside the inlined data) or even read beyond the item range.
This has already triggered a warning in a syzbot report. Although the root fix is to avoid compression for data reloc inodes, for the sake of consistency, reject inlined file extents first.
--- fs/btrfs/relocation.c | 7 +++++++ 1 file changed, 7 insertions(+)
fs/btrfs/relocation.c +7 -0
@@ -893,6 +893,13 @@ static int get_new_location(struct inode *reloc_inode, u64 *new_bytenr, leaf = path->nodes[0 ]; fi =btrfs_item_ptr (leaf, path->slots[0 ], struct btrfs_file_extent_item); +if (unlikely (btrfs_file_extent_type (leaf, fi) ==BTRFS_FILE_EXTENT_INLINE )) { +btrfs_print_leaf (leaf); +btrfs_err (fs_info, +"unexpected inline file extent item for data reloc inode %llu key offset %llu" , +btrfs_ino (BTRFS_I (reloc_inode)), bytenr); +return -EUCLEAN ; + }/* * The cluster-boundary key searched above is always written by
Signature · 1 line
2.54.0
A message that is not in the archive (sent off-list or not yet received): 20260622225540.9ZqL0Mh2J9h-lFv7N3RWN2wumRXixcik8a0xiNBBO-c@z
Boris Burkov
Reviewed-byRe: [PATCH] btrfs: print-tree: print header owner as signed
Quoted 5 lines · Qu Wenruo
When dumpping a tree block,
btrfs_header::owneris printed as unsigned, which can result in numbers that are hard to read, e.g:BTRFS info (device loop0): leaf 8908800 gen 16 total ptrs 28 free space 1676 owner 18446744073709551607
Quoted 8 lines
For the above output, 18446744073709551607 is (s64)-9, the root id of data reloc tree.
Despite those pre-defined root ids that are already negative, existing subvolume trees will not have any negative values, as subvolume trees can only utilize the lower 48 bits, so there will be no output change for existing subvolumes, thus no extra confusion.
What do you think about a macro or function wrapping a switch statement that also prints the string name of known trees? What about some identifying info like a volume path for subvol trees? That one might be a stretch.
Anyway, this is already a definite improvement, thanks.
Quoted 37 lines
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/print-tree.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/btrfs/print-tree.c +4
-4@@ -449,9 +449,9 @@ void btrfs_print_leaf(const struct extent_buffer *l) nr =btrfs_header_nritems (l);btrfs_info (fs_info, -"leaf %llu gen %llu total ptrs %d free space %d owner %llu" , +"leaf %llu gen %llu total ptrs %d free space %d owner %lld" ,btrfs_header_bytenr (l),btrfs_header_generation (l), nr, -btrfs_leaf_free_space (l),btrfs_header_owner (l)); +btrfs_leaf_free_space (l), (s64 )btrfs_header_owner (l));print_eb_refs_lock (l);for (i =0 ; i < nr ; i++) {char key_buf[KEY_TYPE_BUF_SIZE ];@@ -600,10 +600,10 @@ void btrfs_print_tree(const struct extent_buffer *c, bool follow)return ; }btrfs_info (fs_info, -"node %llu level %d gen %llu total ptrs %d free spc %u owner %llu" , +"node %llu level %d gen %llu total ptrs %d free spc %u owner %lld" ,btrfs_header_bytenr (c), level,btrfs_header_generation (c), nr, (u32 )BTRFS_NODEPTRS_PER_BLOCK (fs_info) - nr, -btrfs_header_owner (c)); + (s64 )btrfs_header_owner (c));print_eb_refs_lock (c);for (i =0 ; i < nr; i++) {btrfs_node_key_to_cpu (c, &key, i);Signature · 1 line
2.54.0
Re: [PATCH] btrfs: print-tree: print header owner as signed
Quoted 9 lines · Boris Burkov
Quoted 8 lines · Qu Wenruo
When dumpping a tree block,
btrfs_header::owneris printed as unsigned, which can result in numbers that are hard to read, e.g:BTRFS info (device loop0): leaf 8908800 gen 16 total ptrs 28 free space 1676 owner 18446744073709551607
For the above output, 18446744073709551607 is (s64)-9, the root id of data reloc tree.
Quoted 8 lines
Quoted 5 lines
Despite those pre-defined root ids that are already negative, existing subvolume trees will not have any negative values, as subvolume trees can only utilize the lower 48 bits, so there will be no output change for existing subvolumes, thus no extra confusion.
What do you think about a macro or function wrapping a switch statement that also prints the string name of known trees?
That is possible, but I'm not sure if we need to go this complex for kernel modules.
A signed s64 output would be enough for most developers/advanced users to debug. For end users, I do not know if things like 2 or EXTENT_TREE make any difference.
Quoted 2 lines
What about some identifying
info like a volume path for subvol trees? That one might be a stretch.
That's is too dangerous, it will need extra tree search and backref lookup.
Meanwhile for a tree dump inside a kernel, it's already an emergency, normally we do not have the extra headroom for such complex workload just to make the subvolume id a little easier to read.
Thanks,
Qu
Quoted 41 lines
Anyway, this is already a definite improvement, thanks.
Reviewed-by Boris Burkov <[email protected]>Quoted 37 lines
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/print-tree.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/btrfs/print-tree.c +4
-4@@ -449,9 +449,9 @@ void btrfs_print_leaf(const struct extent_buffer *l) nr =btrfs_header_nritems (l);btrfs_info (fs_info, -"leaf %llu gen %llu total ptrs %d free space %d owner %llu" , +"leaf %llu gen %llu total ptrs %d free space %d owner %lld" ,btrfs_header_bytenr (l),btrfs_header_generation (l), nr, -btrfs_leaf_free_space (l),btrfs_header_owner (l)); +btrfs_leaf_free_space (l), (s64 )btrfs_header_owner (l));print_eb_refs_lock (l);for (i =0 ; i < nr ; i++) {char key_buf[KEY_TYPE_BUF_SIZE ];@@ -600,10 +600,10 @@ void btrfs_print_tree(const struct extent_buffer *c, bool follow)return ; }btrfs_info (fs_info, -"node %llu level %d gen %llu total ptrs %d free spc %u owner %llu" , +"node %llu level %d gen %llu total ptrs %d free spc %u owner %lld" ,btrfs_header_bytenr (c), level,btrfs_header_generation (c), nr, (u32 )BTRFS_NODEPTRS_PER_BLOCK (fs_info) - nr, -btrfs_header_owner (c)); + (s64 )btrfs_header_owner (c));print_eb_refs_lock (c);for (i =0 ; i < nr; i++) {btrfs_node_key_to_cpu (c, &key, i);Signature · 1 line
2.54.0