- 05 Mar, 2024 6 commits
-
-
Qu Wenruo authored
[BUG] There is a bug report about very suspicious tree-checker got triggered: BTRFS critical (device dm-0): corrupted node, root=256 block=8550954455682405139 owner mismatch, have 11858205567642294356 expect [256, 18446744073709551360] BTRFS critical (device dm-0): corrupted node, root=256 block=8550954455682405139 owner mismatch, have 11858205567642294356 expect [256, 18446744073709551360] BTRFS critical (device dm-0): corrupted node, root=256 block=8550954455682405139 owner mismatch, have 11858205567642294356 expect [256, 18446744073709551360] SELinux: inode_doinit_use_xattr: getxattr returned 117 for dev=dm-0 ino=5737268 [ANALYZE] The root cause is still unclear, but there are some clues already: - Unaligned eb bytenr The block bytenr is 8550954455682405139, which is not even aligned to 2. This bytenr is fetched from extent buffer header, not from eb->start. This means, at the initial time of read, eb header bytenr is still correct (the very basis check to continue read), but later something wrong happened, got at least the first page corrupted. Thus we got such obviously incorrect value. - Invalid extent buffer header owner The read itself is triggered for subvolume 256, but the eb header owner is 11858205567642294356, which is not really possible. The problem here is, subvolume id is limited to (1 << 48 - 1), and this one definitely goes beyond that limit. So this value is another garbage. We already got two garbage from an extent buffer, which passed the initial bytenr and csum checks, but later the contents become garbage at some point. This looks like a page lifespan problem (e.g. we didn't properly hold the page). [ENHANCEMENT] The current tree-checker only outputs things from the extent buffer, nothing with the page status. So this patch would enhance the tree-checker output by also dumping the first page, which would look like this: page:00000000aa9f3ce8 refcount:4 mapcount:0 mapping:00000000169aa6b6 index:0x1d0c pfn:0x1022e5 memcg:ffff888103456000 aops:btree_aops [btrfs] ino:1 flags: 0x2ffff0000008000(private|node=0|zone=2|lastcpupid=0xffff) page_type: 0xffffffff() raw: 02ffff0000008000 0000000000000000 dead000000000122 ffff88811e06e220 raw: 0000000000001d0c ffff888102fdb1d8 00000004ffffffff ffff888103456000 page dumped because: eb page dump BTRFS critical (device dm-3): corrupt leaf: root=5 block=30457856 slot=6 ino=257 file_offset=0, invalid disk_bytenr for file extent, have 10617606235235216665, should be aligned to 4096 BTRFS error (device dm-3): read time tree block corruption detected on logical 30457856 mirror 1 From the dump we can see some extra info, something can help us to do extra cross-checks: - Page refcount if it's too low, it definitely means something bad. - Page aops Any mapped eb page should have btree_aops with inode number 1. - Page index Since a mapped eb page should has its bytenr matching the page position, (index << PAGE_SHIFT) should match the bytenr of the bytenr from the critical line. - Page Private flags A mapped eb page should have Private flag set to indicate it's managed by btrfs. Link: https://lore.kernel.org/linux-btrfs/CAHk-=whNdMaN9ntZ47XRKP6DBes2E5w7fi-0U3H2+PS18p+Pzw@mail.gmail.com/Signed-off-by:
Qu Wenruo <wqu@suse.com> Reviewed-by:
David Sterba <dsterba@suse.com> Signed-off-by:
-
Qu Wenruo authored
Since commit a440d48c ("Btrfs: heuristic: implement sampling logic"), btrfs_compress_heuristic() is no longer a simple "return true", but more complex to determine if we should compress. Thus the comment is dead and can be confusing, just remove it. Signed-off-by:
-
Qu Wenruo authored
For the writer counter, it's pretty much the same as the reader counter, and they are exclusive. So move them to the new locked bitmap. Signed-off-by:
-
Qu Wenruo authored
Currently btrfs_subpage utilizes its atomic member @reader to manage the reader counter. However it is only utilized to prevent the page to be released/unlocked when we still have reads underway. In that use case, we don't really allow multiple readers on the same subpage sector. So here we can introduce a new locked bitmap to represent exactly which subpage range is locked for read. In theory we can remove btrfs_subpage::reader as it's just the set bits of the new locked bitmap. But unfortunately bitmap doesn't provide such handy API yet, so we still keep the reader counter. Signed-off-by:
-
Qu Wenruo authored
Both functions were introduced in commit 1e1de387 ("btrfs: make process_one_page() to handle subpage locking"), but they have never been utilized out of subpage code. So just unexport them. Signed-off-by:
-
David Sterba authored
We can pass a valid em cache pointer down to __get_extent_map() and drop the validity check. This avoids the special case, the call stacks are simple: btrfs_read_folio btrfs_do_readpage __get_extent_map extent_readahead contiguous_readpages btrfs_do_readpage __get_extent_map Reviewed-by:Josef Bacik <josef@toxicpanda.com> Reviewed-by:
-
- 04 Mar, 2024 34 commits
-
-
David Sterba authored
The helpers btrfs_del_delalloc_inode() and __btrfs_del_delalloc_inode() don't follow the pattern when the "__" helper does a special case and are in fact reversed regarding the naming. We can merge them into one as there's only one place that needs to be open coded. Reviewed-by:
-
David Sterba authored
Replace the two parameters bdev and name by one that can be used to get them both. Reviewed-by:
-
David Sterba authored
Other errors in flush_reservations() are handled and also in the caller. Ignoring commit might make some sense as it's called right after join so it's to poke the whole commit machinery to free space. However for consistency return the error. The caller btrfs_quota_disable() would try to start the transaction which would in turn fail too so there's no effective change. Reviewed-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches when the default values are used. Signed-off-by:
Kunwu Chan <chentao@kylinos.cn> Reviewed-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches related to delayed refs when the default values are used. Signed-off-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches when the default values are used. Signed-off-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches when the default values are used. Signed-off-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches when the default values are used. Signed-off-by:
-
Kunwu Chan authored
Use the KMEM_CACHE() macro instead of kmem_cache_create() to simplify the creation of SLAB caches when the default values are used. Signed-off-by:
-
David Sterba authored
The helpers are doing an initialization or release work, none of which is performance critical that it would require a static inline, so move them to the .c file. Signed-off-by: -
David Sterba authored
The helper is really trivial, reading a cache size can be done directly. Signed-off-by: -
David Sterba authored
The helpers are doing an initialization or release work, none of which is performance critical that it would require a static inline, so move them to the .c file. Signed-off-by: -
David Sterba authored
Using static inline in a .c file should be justified, e.g. when functions are on a hot path but none of the affected functions seem to be. As it's all in one compilation unit let the compiler decide. Signed-off-by: -
David Sterba authored
This is a simple initializer and not on any hot path, it does not need to be static inline. Signed-off-by: -
David Sterba authored
There are many helpers doing simple things but not simple enough to justify the static inline. None of them seems to be on a hot path so move them to .c. Signed-off-by: -
David Sterba authored
The helper is trivial, we can inline it. It's safe to remove the 'if' as the iterator is always valid when used, the potential NULL was never checked anyway. Signed-off-by: -
David Sterba authored
The helper is trivial and used only once, open code it. It's safe to remove the 'if', the pointer is validated in build_backref_tree(). Signed-off-by: -
David Sterba authored
The from/to CPU/disk helpers for balance args are used only in volumes, no need to define them in accessors.h. Signed-off-by: -
Naohiro Aota authored
We disable offloading checksum to workqueues and do it synchronously when the checksum algorithm is fast. However, as reported in the link below, RAID0 with multiple devices may suffer from the sync checksum, because "fast checksum" is still not fast enough to catch up with RAID0 writing. We don't have an effective way to determine whether to offload or not, for now add a sysfs knob so this can be debugged. This is intentionally under CONFIG_BTRFS_DEBUG so ti's not exposed to users as it may be removed in the future agin. Introduce fs_devices->offload_csum_mode, so that a btrfs developer can change the behavior by writing to /sys/fs/btrfs/<uuid>/offload_csum. The default is "auto" which is the same as the previous behavior. Or, you can set "on" or "off" (or "y" or "n" whatever kstrtobool() accepts) to always/never offload checksum. More benchmark need to be collected with this knob to implement a proper criteria to enable/disable checksum offloading. Link: https://lore.kernel.org/linux-btrfs/20230731152223.4EFB.409509F4@e16-tech.com/ Link: https://lore.kernel.org/linux-btrfs/p3vo3g7pqn664mhmdhlotu5dzcna6vjtcoc2hb2lsgo2fwct7k@xzaxclba5tae/Reviewed-by:
Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by:
Naohiro Aota <naohiro.aota@wdc.com> Reviewed-by:
-
Qu Wenruo authored
[BUG] I have got at least two crash report for RAID6 syndrome generation, no matter if it's AVX2 or SSE2, they all seems to have a similar calltrace with corrupted RAX: BUG: kernel NULL pointer dereference, address: 0000000000000000 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 0 P4D 0 Oops: 0000 [#1] PREEMPT SMP PTI Workqueue: btrfs-rmw rmw_rbio_work [btrfs] RIP: 0010:raid6_sse21_gen_syndrome+0x9e/0x130 [raid6_pq] RAX: 0000000000000000 RBX: 0000000000001000 RCX: ffffa0ff4cfa3248 RDX: 0000000000000000 RSI: ffffa0f74cfa3238 RDI: 0000000000000000 Call Trace: <TASK> rmw_rbio+0x5c8/0xa80 [btrfs] process_one_work+0x1c7/0x3d0 worker_thread+0x4d/0x380 kthread+0xf3/0x120 ret_from_fork+0x2c/0x50 </TASK> [CAUSE] The cause is not known. Recently I also hit this in AVX512 path, and that's even in v5.15 backport, which doesn't have any of my RAID56 rework. Furthermore according to the registers: RAX: 0000000000000000 RBX: 0000000000001000 RCX: ffffa0ff4cfa3248 The RAX register is showing the number of stripes (including PQ), which is not correct (0). But the remaining two registers are all sane. - RBX is the sectorsize For x86_64 it should always be 4K and matches the output. - RCX is the pointers array Which is from rbio->finish_pointers, and it looks like a sane kernel address. [WORKAROUND] For now, I can only add extra debug ASSERT()s before we call raid6 gen_syndrome() helper and hopes to catch the problem. The debug requires both CONFIG_BTRFS_DEBUG and CONFIG_BTRFS_ASSERT enabled. My current guess is some use-after-free, but every report is only having corrupted RAX but seemingly valid pointers doesn't make much sense. Signed-off-by:
-
Filipe Manana authored
At btrfs_free_tree_block(), we are always initializing a delayed reference to drop the given extent buffer but we only use if it does not belong to a log root tree. So we are doing unnecessary work here and increasing the duration of a critical section as this is normally called while holding a lock on the parent tree block (if any) and while holding a log transaction open. So initialize the delayed reference only if the extent buffer is not from a log tree, avoiding unnecessary work and making the code also a bit easier to follow. Reviewed-by:
Filipe Manana <fdmanana@suse.com> Reviewed-by:
-
Filipe Manana authored
During an incremental send, before determining if we need to send a hole (write operations full of zeroes) we will search for the last extent's end offset if we are at the first slot of a leaf and the last processed extent's end offset is smaller then the current extent's start offset. However we are repeating this search in case we had the last extent's end offset undefined (set to the (u64)-1 value) when we entered maybe_send_hole(), wasting time. So avoid this duplicated search by combining the two conditions that trigger a search for the last extent's end offset into a single if statement. Reviewed-by:
Sweet Tea Dorminy <sweettea-kernel@dorminy.me> Reviewed-by:
-
David Sterba authored
The validation of vol args v2 name in snapshot and device remove ioctls is not done properly. A terminating NUL is written to the end of the buffer unconditionally, assuming that this would be the last place in case the buffer is used completely. This does not communicate back the actual error (either an invalid or too long path). Factor out all such cases and use a helper to do the verification, simply look for NUL in the buffer. There's no expected practical change, the size of buffer is 4088, this is enough for most paths or names. Reviewed-by:
Boris Burkov <boris@bur.io> Signed-off-by:
-
David Sterba authored
The validation of vol args name in several ioctls is not done properly. a terminating NUL is written to the end of the buffer unconditionally, assuming that this would be the last place in case the buffer is used completely. This does not communicate back the actual error (either an invalid or too long path). Factor out all such cases and use a helper to do the verification, simply look for NUL in the buffer. There's no expected practical change, the size of buffer is 4088, this is enough for most paths or names. Reviewed-by:
-
Filipe Manana authored
The function btrfs_transaction_in_commit() is no longer used, its last use was removed in commit 11aeb97b ("btrfs: don't arbitrarily slow down delalloc if we're committing"), so just remove it. Reviewed-by:
Anand Jain <anand.jain@oracle.com> Signed-off-by:
-
Neal Gompa authored
The IS_ENABLED() macro already guarantees the result will be a suitable boolean return value ("1" for enabled, and "0" for disabled). Thus, it seems that the "!!" used right before is unnecessary to force the 0/1 values. Reviewed-by:Neal Gompa <neal@gompa.dev> Reviewed-by:
-
David Sterba authored
The purpose of the BUG_ON is not clear. The helper btrfs_grab_root() could return a NULL in case args->root would be a NULL or if there are zero references. Then we check if the root pointer stored in the inode still exists. The whole call chain is for iget: btrfs_iget btrfs_iget_path btrfs_iget_locked iget5_locked btrfs_init_locked_inode which is called from many contexts where we the root pointer is used and we can safely assume has enough references. Signed-off-by: -
David Sterba authored
Checking extent item size in add_inline_refs() is redundant, we do that already in tree-checker after reading the extent buffer and it won't change under normal circumstances. It was added long ago in 8da6d581 ("Btrfs: added btrfs_find_all_roots()") and does not seem to have a clear purpose. Similar case in extent_from_logical(), added in a542ad1b ("btrfs: added helper functions to iterate backrefs"). Signed-off-by:
-
David Sterba authored
The BUG_ON is deep in the qgroup code where we can expect that it exists. A NULL pointer would cause a crash. It was added long ago in 550d7a2e ("btrfs: qgroup: Add new qgroup calculation function btrfs_qgroup_account_extents()."). It maybe made sense back then as the quota enable/disable state machine was not that robust as it is nowadays, so we can just delete it. Signed-off-by:
-
David Sterba authored
The only caller do_walk_down() of btrfs_qgroup_trace_subtree() validates the value of level and uses it several times before it's passed as an argument. Same for root_eb that's called 'next' in the caller. Change both BUG_ONs to assertions as this is to assure proper interface use rather than real errors. Signed-off-by: -
David Sterba authored
There's only one caller of tree_move_down() that does not pass level 0 so the assertion is better suited here. Signed-off-by: -
David Sterba authored
Change BUG_ON to proper error handling if building the path buffer fails. The pointers are not printed so we don't accidentally leak kernel addresses. Signed-off-by: -
David Sterba authored
Change BUG_ON to proper error handling when an unexpected inode number is encountered. As the comment says this should never happen. Signed-off-by: -
David Sterba authored
Change BUG_ON to a proper error handling in the unlikely case of seeing data when the command is started. This is supposed to be reset when the command is finished (send_cmd, send_encoded_extent). Signed-off-by:
-
