- 29 Apr, 2019 40 commits
-
-
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 <wqu@suse.com> Reviewed-by:
David Sterba <dsterba@suse.com> 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:
Johannes Thumshirn <jthumshirn@suse.de> Signed-off-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:
Nikolay Borisov <nborisov@suse.com> 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:
Filipe Manana <fdmanana@suse.com> 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:
-
David Sterba authored
The error code used here is wrong as it's not invalid to try to start scrub when umount has begun. Returning EAGAIN is more user friendly as it's recoverable. Signed-off-by: -
Goldwyn Rodrigues authored
inode->i_op is initialized multiple times. Perform it once. This was left by 4779cc04 ("Btrfs: get rid of btrfs_symlink_aops"). Signed-off-by:
Goldwyn Rodrigues <rgoldwyn@suse.com> Reviewed-by:
-
Qu Wenruo authored
When we failed to find a root key in btrfs_update_root(), we just panic. That's definitely not cool, fix it by outputting an unique error message, aborting current transaction and return -EUCLEAN. This should not normally happen as the root has been used by the callers in some way. Reviewed-by:
-
