- 29 Apr, 2019 40 commits
-
-
Qu Wenruo authored
Just add one extra line to show when the corruption is detected. Currently only read time detection is possible. The planned distinguish line would be: read time: <detailed report> block=XXXXX read time tree block corruption detected write time: <detailed report> block=XXXXX write time tree block corruption detected Reviewed-by:Nikolay Borisov <nborisov@suse.com> Reviewed-by:
Johannes Thumshirn <jthumshirn@suse.de> Signed-off-by:
Qu Wenruo <wqu@suse.com> Reviewed-by:
David Sterba <dsterba@suse.com> Signed-off-by:
-
Josef Bacik authored
We've been seeing the following sporadically throughout our fleet panic: kernel BUG at fs/btrfs/relocation.c:4584! netversion: 5.0-0 Backtrace: #0 [ffffc90003adb880] machine_kexec at ffffffff81041da8 #1 [ffffc90003adb8c8] __crash_kexec at ffffffff8110396c #2 [ffffc90003adb988] crash_kexec at ffffffff811048ad #3 [ffffc90003adb9a0] oops_end at ffffffff8101c19a #4 [ffffc90003adb9c0] do_trap at ffffffff81019114 #5 [ffffc90003adba00] do_error_trap at ffffffff810195d0 #6 [ffffc90003adbab0] invalid_op at ffffffff81a00a9b [exception RIP: btrfs_reloc_cow_block+692] RIP: ffffffff8143b614 RSP: ffffc90003adbb68 RFLAGS: 00010246 RAX: fffffffffffffff7 RBX: ffff8806b9c32000 RCX: ffff8806aad00690 RDX: ffff880850b295e0 RSI: ffff8806b9c32000 RDI: ffff88084f205bd0 RBP: ffff880849415000 R8: ffffc90003adbbe0 R9: ffff88085ac90000 R10: ffff8805f7369140 R11: 0000000000000000 R12: ffff880850b295e0 R13: ffff88084f205bd0 R14: 0000000000000000 R15: 0000000000000000 ORIG_RAX: ffffffffffffffff CS: 0010 SS: 0018 #7 [ffffc90003adbbb0] __btrfs_cow_block at ffffffff813bf1cd #8 [ffffc90003adbc28] btrfs_cow_block at ffffffff813bf4b3 #9 [ffffc90003adbc78] btrfs_search_slot at ffffffff813c2e6c The way relocation moves data extents is by creating a reloc inode and preallocating extents in this inode and then copying the data into these preallocated extents. Once we've done this for all of our extents, we'll write out these dirty pages, which marks the extent written, and goes into btrfs_reloc_cow_block(). From here we get our current reloc_control, which _should_ match the reloc_control for the current block group we're relocating. However if we get an ENOSPC in this path at some point we'll bail out, never initiating writeback on this inode. Not a huge deal, unless we happen to be doing relocation on a different block group, and this block group is now rc->stage == UPDATE_DATA_PTRS. This trips the BUG_ON() in btrfs_reloc_cow_block(), because we expect to be done modifying the data inode. We are in fact done modifying the metadata for the data inode we're currently using, but not the one from the failed block group, and thus we BUG_ON(). (This happens when writeback finishes for extents from the previous group, when we are at btrfs_finish_ordered_io() which updates the data reloc tree (inode item, drops/adds extent items, etc).) Fix this by writing out the reloc data inode always, and then breaking out of the loop after that point to keep from tripping this BUG_ON() later. Signed-off-by:Josef Bacik <josef@toxicpanda.com> Reviewed-by:
Filipe Manana <fdmanana@suse.com> [ add note from Filipe ] Signed-off-by:
-
Nikolay Borisov authored
The uptodate parameter of btrfs_writepage_endio_finish_ordered is used to signal whether an error has occured while writing the given page. 0 signals an error, which is propagated to callees and 1 signifies success. In end_compressed_bio_write the ->bi_status is checked and based on it either BLK_STS_OK (0) or BLK_STS_NOTSUPP (1) are used. While from functional point of view this is ok it's a for the poor reader of the code, since the block layer values are conflated with the semantics of the parameter. Just use plain 0 or 1. No functional changes. Signed-off-by:
-
Qu Wenruo authored
We can only get <=0 from extent_write_cache_pages, add an ASSERT() for it just in case. Then instead of submitting the write bio even if we got some error, check the return value first. If we have already hit some error, just clean up the corrupted or half-baked bio, and return error. If there is no error so far, then call flush_write_bio() and return the result. Signed-off-by:
-
Qu Wenruo authored
This function needs some extra checks on locked pages and eb. For error handling we need to unlock locked pages and the eb. There is a rare >0 return value branch, where all pages get locked while write bio is not flushed. Thankfully it's handled by the only caller, btree_write_cache_pages(), as later write_one_eb() call will trigger submit_one_bio(). So there shouldn't be any problem. Signed-off-by:
-
Qu Wenruo authored
We can only get @ret <= 0. Add an ASSERT() for it just in case. Then, instead of submitting the write bio even we got some error, check the return value first. If we have already hit some error, just clean up the corrupted or half-baked bio, and return error. If there is no error so far, then call flush_write_bio() and return the result. Signed-off-by:
-
Qu Wenruo authored
Since __extent_writepage() will no longer return >0 value, (ret == AOP_WRITEPAGE_ACTIVATE) will never be true. Kill that dead branch. Reviewed-by:
-
Qu Wenruo authored
In btree_write_cache_pages(), we can only get @ret <= 0. Add an ASSERT() for it just in case. Then instead of submitting the write bio even we got some error, check the return value first. If we have already hit some error, just clean up the corrupted or half-baked bio, and return error. If there is no error so far, then call flush_write_bio() and return the result. Signed-off-by:
-
Qu Wenruo authored
Since now flush_write_bio() could return error, kill the BUG_ON() first. Then don't call flush_write_bio() unconditionally, instead we check the return value from __extent_writepage() first. If __extent_writepage() fails, we do cleanup, and return error without submitting the possible corrupted or half-baked bio. If __extent_writepage() successes, then we call flush_write_bio() and return the result. Signed-off-by:
-
Qu Wenruo authored
We have a BUG_ON() in flush_write_bio() to handle the return value of submit_one_bio(). Move the BUG_ON() one level up to all its callers. This patch will introduce temporary variable, @flush_ret to keep code change minimal in this patch. That variable will be cleaned up when enhancing the error handling later. Reviewed-by:
-
Qu Wenruo authored
We have internal report of strange transaction abort due to EUCLEAN without any error message. Since error message inside verify_level_key() is only enabled for CONFIG_BTRFS_DEBUG, the error message won't be printed on most builds. This patch will make the error message mandatory, so when problem happens we know what's causing the problem. Reviewed-by:
-
Qu Wenruo authored
[BUG] When reading a file from a fuzzed image, kernel can panic like: BTRFS warning (device loop0): csum failed root 5 ino 270 off 0 csum 0x98f94189 expected csum 0x00000000 mirror 1 assertion failed: !memcmp_extent_buffer(b, &disk_key, offsetof(struct btrfs_leaf, items[0].key), sizeof(disk_key)), file: fs/btrfs/ctree.c, line: 2544 ------------[ cut here ]------------ kernel BUG at fs/btrfs/ctree.h:3500! invalid opcode: 0000 [#1] PREEMPT SMP NOPTI RIP: 0010:btrfs_search_slot.cold.24+0x61/0x63 [btrfs] Call Trace: btrfs_lookup_csum+0x52/0x150 [btrfs] __btrfs_lookup_bio_sums+0x209/0x640 [btrfs] btrfs_submit_bio_hook+0x103/0x170 [btrfs] submit_one_bio+0x59/0x80 [btrfs] extent_read_full_page+0x58/0x80 [btrfs] generic_file_read_iter+0x2f6/0x9d0 __vfs_read+0x14d/0x1a0 vfs_read+0x8d/0x140 ksys_read+0x52/0xc0 do_syscall_64+0x60/0x210 entry_SYSCALL_64_after_hwframe+0x49/0xbe [CAUSE] The fuzzed image has a corrupted leaf whose first key doesn't match its parent: checksum tree key (CSUM_TREE ROOT_ITEM 0) node 29741056 level 1 items 14 free 107 generation 19 owner CSUM_TREE fs uuid 3381d111-94a3-4ac7-8f39-611bbbdab7e6 chunk uuid 9af1c3c7-2af5-488b-8553-530bd515f14c ... key (EXTENT_CSUM EXTENT_CSUM 79691776) block 29761536 gen 19 leaf 29761536 items 1 free space 1726 generation 19 owner CSUM_TREE leaf 29761536 flags 0x1(WRITTEN) backref revision 1 fs uuid 3381d111-94a3-4ac7-8f39-611bbbdab7e6 chunk uuid 9af1c3c7-2af5-488b-8553-530bd515f14c item 0 key (EXTENT_CSUM EXTENT_CSUM 8798638964736) itemoff 1751 itemsize 2244 range start 8798638964736 end 8798641262592 length 2297856 When reading the above tree block, we have extent_buffer->refs = 2 in the context: - initial one from __alloc_extent_buffer() alloc_extent_buffer() |- __alloc_extent_buffer() |- atomic_set(&eb->refs, 1) - one being added to fs_info->buffer_radix alloc_extent_buffer() |- check_buffer_tree_ref() |- atomic_inc(&eb->refs) So if even we call free_extent_buffer() in read_tree_block or other similar situation, we only decrease the refs by 1, it doesn't reach 0 and won't be freed right now. The staled eb and its corrupted content will still be kept cached. Furthermore, we have several extra cases where we either don't do first key check or the check is not proper for all callers: - scrub We just don't have first key in this context. - shared tree block One tree block can be shared by several snapshot/subvolume trees. In that case, the first key check for one subvolume doesn't apply to another. So for the above reasons, a corrupted extent buffer can sneak into the buffer cache. [FIX] Call verify_level_key in read_block_for_search to do another verification. For that purpose the function is exported. Due to above reasons, although we can free corrupted extent buffer from cache, we still need the check in read_block_for_search(), for scrub and shared tree blocks. Link: https://bugzilla.kernel.org/show_bug.cgi?id=202755 Link: https://bugzilla.kernel.org/show_bug.cgi?id=202757 Link: https://bugzilla.kernel.org/show_bug.cgi?id=202759 Link: https://bugzilla.kernel.org/show_bug.cgi?id=202761 Link: https://bugzilla.kernel.org/show_bug.cgi?id=202767 Link: https://bugzilla.kernel.org/show_bug.cgi?id=202769Reported-by:Yoon Jungyeon <jungyeon@gatech.edu> CC: stable@vger.kernel.org # 4.19+ Signed-off-by:
-
Nikolay Borisov authored
If a an eb fails to be read for whatever reason - it's corrupted on disk and parent transid/key validations fail or IO for eb pages fail then this buffer must be removed from the buffer cache. Currently the code calls free_extent_buffer if an error occurs. Unfortunately this doesn't achieve the desired behavior since btrfs_find_create_tree_block returns with eb->refs == 2. On the other hand free_extent_buffer will only decrement the refs once leaving it added to the buffer cache radix tree. This enables later code to look up the buffer from the cache and utilize it potentially leading to a crash. The correct way to free the buffer is call free_extent_buffer_stale. This function will correctly call atomic_dec explicitly for the buffer and subsequently call release_extent_buffer which will decrement the final reference thus correctly remove the invalid buffer from buffer cache. This change affects only newly allocated buffers since they have eb->refs == 2. Link: https://bugzilla.kernel.org/show_bug.cgi?id=202755Reported-by:
-
Qu Wenruo authored
From the introduction of btrfs_(set|clear)_header_flag, there is no usage of its return value. So just make it return void. Reviewed-by:
-
Qu Wenruo authored
Commit d2311e69 ("btrfs: relocation: Delay reloc tree deletion after merge_reloc_roots()") expands the life span of root->reloc_root. This breaks certain checs of fs_info->reloc_ctl. Before that commit, if we have a root with valid reloc_root, then it's ensured to have fs_info->reloc_ctl. But now since reloc_root doesn't always mean a valid fs_info->reloc_ctl, such check is unreliable and can cause the following NULL pointer dereference: BUG: unable to handle kernel NULL pointer dereference at 00000000000005c1 IP: btrfs_reloc_pre_snapshot+0x20/0x50 [btrfs] PGD 0 P4D 0 Oops: 0000 [#1] SMP PTI CPU: 0 PID: 10379 Comm: snapperd Not tainted Call Trace: create_pending_snapshot+0xd7/0xfc0 [btrfs] create_pending_snapshots+0x8e/0xb0 [btrfs] btrfs_commit_transaction+0x2ac/0x8f0 [btrfs] btrfs_mksubvol+0x561/0x570 [btrfs] btrfs_ioctl_snap_create_transid+0x189/0x190 [btrfs] btrfs_ioctl_snap_create_v2+0x102/0x150 [btrfs] btrfs_ioctl+0x5c9/0x1e60 [btrfs] do_vfs_ioctl+0x90/0x5f0 SyS_ioctl+0x74/0x80 do_syscall_64+0x7b/0x150 entry_SYSCALL_64_after_hwframe+0x3d/0xa2 RIP: 0033:0x7fd7cdab8467 Fix it by explicitly checking fs_info->reloc_ctl other than using the implied root->reloc_root. Fixes: d2311e69 ("btrfs: relocation: Delay reloc tree deletion after merge_reloc_roots") Signed-off-by:
-
Nikolay Borisov authored
In case we hit the error case for a metadata buffer in end_bio_extent_readpage then 'ret' won't really be checked before it's written again to. This means the -EIO in this case will never be checked, just remove it. Fixes-coverity-id: 1442513 Signed-off-by:
-
Nikolay Borisov authored
Currently extent_readpages (called from btrfs_readpages) will always call __extent_readpages which tries to create contiguous range of pages and call __do_contiguous_readpages when such contiguous range is created. It turns out this is unnecessary due to the fact that generic MM code always calls filesystem's ->readpages callback (btrfs_readpages in this case) with already contiguous pages. Armed with this knowledge it's possible to simplify extent_readpages by eliminating the call to __extent_readpages and directly calling contiguous_readpages. The only edge case that needs to be handled is when add_to_page_cache_lru fails. This is easy as all that is needed is to submit whatever is the number of pages successfully added to the lru. This can happen when the page is already in the range, so it does not need to be read again, and we can't do anything else in case of other errors. Signed-off-by:
-
David Sterba authored
The member is tracking simple status of the lock, we can use bool for that and make some room for further space reduction in the structure. Reviewed-by:
-
David Sterba authored
Use the helpers where open coded. On non-debug builds, the warnings will not trigger and extent_buffer::write_locks become unused and can be moved to the appropriate section, saving a few bytes. Reviewed-by:
-
David Sterba authored
The write_locks are a simple counter to track locking balance and used to assert tree locks. Add helpers to make it conditionally work only in DEBUG builds. Will be used in followup patches. Reviewed-by:
-
David Sterba authored
Use the helpers where open coded. On non-debug builds, the warnings will not trigger and extent_buffer::read_locks become unused and can be moved to the appropriate section, saving a few bytes. Reviewed-by:
-
David Sterba authored
The read_locks are a simple counter to track locking balance and used to assert tree locks. Add helpers to make it conditionally work only in DEBUG builds. Will be used in followup patches. Reviewed-by:
-
David Sterba authored
Use the helpers where open coded. On non-debug builds, the warnings will not trigger and extent_buffer::spining_readers become unused and can be moved to the appropriate section, saving a few bytes. Reviewed-by:
-
David Sterba authored
Add helpers for conditional DEBUG build to assert that the extent buffer spinning_readers constraints are met. Will be used in followup patches. Reviewed-by:
-
David Sterba authored
Use the helpers where open coded. On non-debug builds, the warnings will not trigger and extent_buffer::spining_writers become unused and can be moved to the appropriate section, saving a few bytes. Reviewed-by:
-
David Sterba authored
Add helpers for conditional DEBUG build to assert that the extent buffer spinning_writers constraints are met. Will be used in followup patches. Reviewed-by:
-
Nikolay Borisov authored
This flag just became synonymous to EXTENT_LOCKED, so just remove it and used EXTENT_LOCKED directly. No functional changes. Signed-off-by:
-
Nikolay Borisov authored
This flag was introduced in a52d9a80 ("Btrfs: Extent based page cache code.") and subsequently it's usage effectively was removed by 1edbb734 ("Btrfs: reduce CPU usage in the extent_state tree") and f2a97a9d ("btrfs: remove all unused functions"). Just remove it, no functional changes. Signed-off-by:
-
Nathan Chancellor authored
When building with -Wsometimes-uninitialized, Clang warns: fs/btrfs/uuid-tree.c:129:13: warning: variable 'eb' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] fs/btrfs/uuid-tree.c:129:13: warning: variable 'offset' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] Clang can't tell that all cases are covered with this final else if. Just turn it into an else so that it is clear. Link: https://github.com/ClangBuiltLinux/linux/issues/385Suggested-by:
Nick Desaulniers <ndesaulniers@google.com> Signed-off-by:
Nathan Chancellor <natechancellor@gmail.com> Reviewed-by:
-
Anand Jain authored
btrfs_set_prop() takes transaction pointer as the first argument, however in ioctl.c for the purpose of setting the compression property, we call btrfs_set_prop() with NULL transaction pointer. Down in the call chain btrfs_setxattr() starts transaction to update the attribute and also to update the inode. So for clarity, create btrfs_set_prop_trans() with no transaction pointer as argument, in preparation to start transaction here instead of doing it down the call chain at btrfs_setxattr(). Also now the btrfs_set_prop() is a static function. Signed-off-by:
Anand Jain <anand.jain@oracle.com> Reviewed-by:
-
Anand Jain authored
fs_info is commonly used to represent struct fs_info *, rename to fs_private to avoid confusion. Signed-off-by:
-
Anand Jain authored
Drop forward declaration of the functions: - prop_compression_validate - prop_compression_apply - prop_compression_extract No functional changes. Reviewed-by:
-
Anand Jain authored
btrfs_set_prop() is a redirect to __btrfs_set_prop() with the transaction handle equal to NULL. __btrfs_set_prop() in turn passes this to do_setxattr() which then transaction is actually created. Instead merge __btrfs_set_prop() to btrfs_set_prop(), and update the caller with NULL argument. Signed-off-by:
-
Johannes Thumshirn authored
Since commit c40a3d38 ("Btrfs: Compute and look up csums based on sectorsized blocks") we do a kmap_atomic() on the contents of a bvec. The code before c40a3d38 had the kmap region just around the checksumming too. kmap_atomic() in turn does a preempt_disable() and pagefault_disable(), so we shouldn't map the data for too long. Reduce the time the bvec's page is mapped to when we actually need it. Performance wise it doesn't seem to make a huge difference with a 2 vcpu VM on a /dev/zram device: vanilla patched delta write 17.4MiB/s 17.8MiB/s +0.4MiB/s (+2%) read 40.6MiB/s 41.5MiB/s +0.9MiB/s (+2%) The following fio job profile was used in the comparision: [global] ioengine=libaio direct=1 sync=1 norandommap time_based runtime=10m size=100m group_reporting numjobs=2 [test] filename=/mnt/test/fio rw=randrw rwmixread=70 Signed-off-by:
-
Qu Wenruo authored
Although btrfs heavily relies on extent_io_tree, we don't really have any good trace events for them. This patch will add the folowing trace events: - trace_btrfs_set_extent_bit() - trace_btrfs_clear_extent_bit() - trace_btrfs_convert_extent_bit() Since selftests could create temporary extent_io_tree without fs_info, modify TP_fast_assign_fsid() to accept NULL as fs_info. NULL fs_info will lead to all zero fsid. The output would be: btrfs_set_extent_bit: <FDID>: io_tree=INODE_IO ino=1 root=1 start=22036480 len=4096 set_bits=LOCKED btrfs_set_extent_bit: <FSID>: io_tree=INODE_IO ino=1 root=1 start=22040576 len=4096 set_bits=LOCKED btrfs_set_extent_bit: <FSID>: io_tree=INODE_IO ino=1 root=1 start=22044672 len=4096 set_bits=LOCKED btrfs_set_extent_bit: <FSID>: io_tree=INODE_IO ino=1 root=1 start=22048768 len=4096 set_bits=LOCKED btrfs_clear_extent_bit: <FSID>: io_tree=INODE_IO ino=1 root=1 start=22036480 len=16384 clear_bits=LOCKED ^^^ Extent buffer 22036480 read from disk, the locking progress btrfs_set_extent_bit: <FSID>: io_tree=TRANS_DIRTY_PAGES ino=1 root=1 start=30425088 len=16384 set_bits=DIRTY btrfs_set_extent_bit: <FSID>: io_tree=TRANS_DIRTY_PAGES ino=1 root=1 start=30441472 len=16384 set_bits=DIRTY ^^^ 2 new tree blocks allocated in one transaction btrfs_set_extent_bit: <FSID>: io_tree=FREED_EXTENTS0 ino=0 root=0 start=30523392 len=16384 set_bits=DIRTY btrfs_set_extent_bit: <FSID>: io_tree=FREED_EXTENTS0 ino=0 root=0 start=30556160 len=16384 set_bits=DIRTY ^^^ 2 old tree blocks get pinned down There is one point which need attention: 1) Those trace events can be pretty heavy: The following workload would generate over 400 trace events. mkfs.btrfs -f $dev start_trace mount $dev $mnt -o enospc_debug sync touch $mnt/file1 touch $mnt/file2 touch $mnt/file3 xfs_io -f -c "pwrite 0 16k" $mnt/file4 umount $mnt end_trace It's not recommended to use them in real world environment. Signed-off-by:
-
Qu Wenruo authored
Btrfs has the following different extent_io_trees used: - fs_info::free_extents[2] - btrfs_inode::io_tree - for both normal inodes and the btree inode - btrfs_inode::io_failure_tree - btrfs_transaction::dirty_pages - btrfs_root::dirty_log_pages If we want to trace changes in those trees, it will be pretty hard to distinguish them. Instead of using hard-to-read pointer address, this patch will introduce a new member extent_io_tree::owner to track the owner. This modification needs all the callers of extent_io_tree_init() to accept a new parameter @owner. This patch provides the basis for later trace events. Signed-off-by:
-
David Sterba authored
This patch is split from the following one "btrfs: Introduce extent_io_tree::owner to distinguish different io_trees" from Qu, so the different changes are not mixed together. Signed-off-by: -
Qu Wenruo authored
This patch will add a new member fs_info to extent_io_tree. This provides the basis for later trace events to distinguish the output between different btrfs filesystems. While this increases the size of the structure, we want to know the source of the trace events and passing the fs_info as an argument to all contexts is not possible. The selftests are now allowed to set it to NULL as they don't use the tracepoints. Signed-off-by:
-
Filipe Manana authored
Commit db2462a6 ("btrfs: don't run delayed refs in the end transaction logic") removed its last use, so now it does absolutely nothing, therefore remove it. Reviewed-by:
-
Dennis Zhou authored
While calling functions inside zstd, we don't need to use the indirection provided by the workspace_manager. Forward declarations are added to maintain the function order of btrfs_compress_op. Signed-off-by:
Dennis Zhou <dennis@kernel.org> Reviewed-by:
-
