- 25 Jul, 2024 2 commits
-
-
Filipe Manana authored
If we attempt to insert a compressed extent map that has a range that overlaps another extent map we have in the inode's extent map tree, we can end up with an incorrect offset after adjusting the new extent map at merge_extent_mapping() because we don't update the extent map's offset. For example consider the following scenario: 1) We have a file extent item for a compressed extent covering the file range [108K, 144K) and currently there's no corresponding extent map in the inode's extent map tree; 2) The inode's size is 141K; 3) We have an encoded write (compressed) into the file range [120K, 128K), which overlaps the existing file extent item. The encoded write creates a matching extent map, adds it to the inode's extent map tree and creates an ordered extent for it. Note that the corresponding file extent item is added to the subvolume tree only when the ordered extent completes (when executing btrfs_finish_one_ordered()); 4) We have a write into the file range [160K, 164K). This writes increases the i_size of the file, and there's a hole between the current i_size (141K) and the start offset of this write, and since the old i_size is in the middle of the block [140K, 144K), we have to write zeroes to the range [141K, 144K) (3072 bytes) and therefore dirty that page. We then call btrfs_set_extent_delalloc() with a start offset of 140K. We then end up at btrfs_find_new_delalloc_bytes() which will call btrfs_get_extent() for the range [140K, 144K); 5) The btrfs_get_extent() doesn't find any extent map in the inode's extent map tree covering the range [140K, 144K), so it searches the subvolume tree for any file extent items covering that range. There it finds the file extent item for the range [108K, 144K), creates a compressed extent map for that range and then calls btrfs_add_extent_mapping() with that extent map and passes the range [140K, 144K) via the "start" and "len" parameters; 6) The call to add_extent_mapping() done by btrfs_add_extent_mapping() fails with -EEXIST because there's an extent map, created at step 2 for the [120K, 128K) range, that covers that overlaps with the range of the given extent map ([108K, 144K)). Then it does a lookup for extent map from step 2 add calls merge_extent_mapping() to adjust the input extent map ([108K, 144K)). That adjust the extent map to a start offset of 128K and a length of 16K (starting just after the extent map from step 2), but it does not update the offset field of the extent map, leaving it with a value of zero instead of updating to a value of 20K (128K - 108K = 20K). As a result any read for the range [128K, 144K) can return incorrect data since we read from a wrong section of the extent (unless both the correct and incorrect ranges happen to have the same data). So fix this by changing merge_extent_mapping() to update the extent map's offset even if it's compressed. Also add a test case to the self tests. This didn't happen before the patchset that does big changes in the extent map structure (which includes the commit in the Fixes tag below) because we kept track of the original start offset in the extent map (member "orig_start") so we could always calculate the correct offset by subtracting that offset from the start offset. A test case for fstests that triggered this problem using send/receive with compressed writes will be added soon. Fixes: 3d2ac992 ("btrfs: introduce new members for extent_map") Reviewed-by:
Qu Wenruo <wqu@suse.com> Signed-off-by:
Filipe Manana <fdmanana@suse.com> Signed-off-by:
David Sterba <dsterba@suse.com>
-
Qu Wenruo authored
[CORRUPTION] There is a bug report that btrfs flips RO due to a corruption in the extent tree, the involved dumps looks like this: item 188 key (402811572224 168 4096) itemoff 14598 itemsize 79 extent refs 3 gen 3678544 flags 1 ref#0: extent data backref root 13835058055282163977 objectid 281473384125923 offset 81432576 count 1 ref#1: shared data backref parent 1947073626112 count 1 ref#2: shared data backref parent 1156030103552 count 1 BTRFS critical (device vdc1: state EA): unable to find ref byte nr 402811572224 parent 0 root 265 owner 28703026 offset 81432576 slot 189 BTRFS error (device vdc1: state EA): failed to run delayed ref for logical 402811572224 num_bytes 4096 type 178 action 2 ref_mod 1: -2 [CAUSE] The corrupted entry is ref#0 of item 188. The root number 13835058055282163977 is beyond the upper limit for root items (the current limit is 1 << 48), and the objectid also looks suspicious. Only the offset and count is correct. [ENHANCEMENT] Although it's still unknown why we have such many bytes corrupted randomly, we can still enhance the tree-checker for data backrefs by: - Validate the root value For now there should only be 3 types of roots can have data backref: * subvolume trees * data reloc trees * root tree Only for v1 space cache - validate the objectid value The objectid should be a valid inode number. Hopefully we can catch such problem in the future with the new checkers. Reported-by:Kai Krakow <hurikhan77@gmail.com> Link: https://lore.kernel.org/linux-btrfs/CAMthOuPjg5RDT-G_LXeBBUUtzt3cq=JywF+D1_h+JYxe=WKp-Q@mail.gmail.com/#tReviewed-by:
-
- 19 Jul, 2024 1 commit
-
-
Qu Wenruo authored
Currently the BTRFS_MOUNT_* flags are already beyond 32 bits, this is going to cause compilation errors for some 32 bit systems, as their unsigned long is only 32 bits long, thus flag BTRFS_MOUNT_IGNORESUPERFLAGS overflows and can lead to errors. Fix the problem by: - Migrate all existing BTRFS_MOUNT_* flags to unsigned long long - Migrate all mount option related variables to unsigned long long * btrfs_fs_info::mount_opt * btrfs_fs_context::mount_opt * mount_opt parameter of btrfs_check_options() * old_opts parameter of btrfs_remount_begin() * old_opts parameter of btrfs_remount_cleanup() * mount_opt parameter of btrfs_check_mountopts_zoned() * mount_opt and opt parameters of check_ro_option() Fixes: 32e62165 ("btrfs: introduce new "rescue=ignoresuperflags" mount option") Signed-off-by:
-
- 11 Jul, 2024 37 commits
-
-
Filipe Manana authored
At add_ra_bio_pages() we are accessing the extent map to calculate 'add_size' after we dropped our reference on the extent map, resulting in a use-after-free. Fix this by computing 'add_size' before dropping our extent map reference. Reported-by: syzbot+853d80cba98ce1157ae6@syzkaller.appspotmail.com Link: https://lore.kernel.org/linux-btrfs/000000000000038144061c6d18f2@google.com/ Fixes: 6a404910 ("btrfs: subpage: make add_ra_bio_pages() compatible") CC: stable@vger.kernel.org # 6.1+ Signed-off-by:
-
Filipe Manana authored
If we failed to link a free space entry because there's already a conflicting entry for the same offset, we free the free space entry but we don't free the associated bitmap that we had just allocated before. Fix that by freeing the bitmap before freeing the entry. Reviewed-by:
Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by:
-
Qu Wenruo authored
Previously we had a BUG_ON() inside extent_range_clear_dirty_for_io(), as we expected all involved folios to be still locked, thus no folio should be missing. However for extent_range_clear_dirty_for_io() itself, we can skip the missing folio and handle the remaining ones, and return an error if there is anything wrong. Remove the BUG_ON() and let the caller to handle the error. In the caller we do not have a quick way to cleanup the error, but all the compression routines would handle the missing folio as an error and properly error out, so we only need to do an ASSERT() for developers, while for non-debug build the compression routine would handle the error correctly. Signed-off-by:
-
Qu Wenruo authored
The function is only used inside inode.c by compress_file_range(), so move it to inode.c and unexport it. Signed-off-by:
-
David Sterba authored
Add more verbose and specific messages to all main error points in compression code for all algorithms. Currently there's no way to know which inode is affected or where in the data errors happened. The messages follow a common format: - what happened - error code if relevant - root and inode - additional data like offsets or lengths There's no helper for the messages as they differ in some details and that would be cumbersome to generalize to a single function. As all the errors are "almost never happens" there are the unlikely annotations done as compression is hot path. Signed-off-by: -
Filipe Manana authored
KCSAN complains about a data race when accessing the last_trans field of a root: [ 199.553628] BUG: KCSAN: data-race in btrfs_record_root_in_trans [btrfs] / record_root_in_trans [btrfs] [ 199.555186] read to 0x000000008801e308 of 8 bytes by task 2812 on cpu 1: [ 199.555210] btrfs_record_root_in_trans+0x9a/0x128 [btrfs] [ 199.555999] start_transaction+0x154/0xcd8 [btrfs] [ 199.556780] btrfs_join_transaction+0x44/0x60 [btrfs] [ 199.557559] btrfs_dirty_inode+0x9c/0x140 [btrfs] [ 199.558339] btrfs_update_time+0x8c/0xb0 [btrfs] [ 199.559123] touch_atime+0x16c/0x1e0 [ 199.559151] pipe_read+0x6a8/0x7d0 [ 199.559179] vfs_read+0x466/0x498 [ 199.559204] ksys_read+0x108/0x150 [ 199.559230] __s390x_sys_read+0x68/0x88 [ 199.559257] do_syscall+0x1c6/0x210 [ 199.559286] __do_syscall+0xc8/0xf0 [ 199.559318] system_call+0x70/0x98 [ 199.559431] write to 0x000000008801e308 of 8 bytes by task 2808 on cpu 0: [ 199.559464] record_root_in_trans+0x196/0x228 [btrfs] [ 199.560236] btrfs_record_root_in_trans+0xfe/0x128 [btrfs] [ 199.561097] start_transaction+0x154/0xcd8 [btrfs] [ 199.561927] btrfs_join_transaction+0x44/0x60 [btrfs] [ 199.562700] btrfs_dirty_inode+0x9c/0x140 [btrfs] [ 199.563493] btrfs_update_time+0x8c/0xb0 [btrfs] [ 199.564277] file_update_time+0xb8/0xf0 [ 199.564301] pipe_write+0x8ac/0xab8 [ 199.564326] vfs_write+0x33c/0x588 [ 199.564349] ksys_write+0x108/0x150 [ 199.564372] __s390x_sys_write+0x68/0x88 [ 199.564397] do_syscall+0x1c6/0x210 [ 199.564424] __do_syscall+0xc8/0xf0 [ 199.564452] system_call+0x70/0x98 This is because we update and read last_trans concurrently without any type of synchronization. This should be generally harmless and in the worst case it can make us do extra locking (btrfs_record_root_in_trans()) trigger some warnings at ctree.c or do extra work during relocation - this would probably only happen in case of load or store tearing. So fix this by always reading and updating the field using READ_ONCE() and WRITE_ONCE(), this silences KCSAN and prevents load and store tearing. Reviewed-by:
Josef Bacik <josef@toxicpanda.com> Signed-off-by:
-
Qu Wenruo authored
There is only one caller utilizing the @extra_gfp parameter, alloc_eb_folio_array(). And in that case the extra_gfp is only assigned to __GFP_NOFAIL. Rename the @extra_gfp parameter to @nofail to indicate that. Reviewed-by:
-
Qu Wenruo authored
The function btrfs_alloc_folio_array() is only utilized in btrfs_submit_compressed_read() and no other location, and the only caller is not utilizing the @extra_gfp parameter. Reviewed-by:
-
Qu Wenruo authored
This new mount option allows the kernel to skip the super flags check, it's mostly to allow the kernel to do a rescue mount of an interrupted checksum conversion. Reviewed-by:
-
Qu Wenruo authored
Introduce "rescue=ignoremetacsums" to ignore metadata csums, all the other metadata sanity checks are still kept as is. This new mount option is mostly to allow the kernel to mount an interrupted checksum conversion (at the metadata csum overwrite stage). And since the main part of metadata sanity checks is inside tree-checker, we shouldn't lose much safety, and the new mount option is rescue mount option it requires full read-only mount. Reviewed-by:
-
Qu Wenruo authored
Most of the extra super block flags are beyond 32bits (from CHANGING_FSID_V2 to CHANGING_*_CSUMS), thus using %llu is not only too long and pretty hard to read. Reviewed-by:
-
Qu Wenruo authored
The following three Opt_* enums haven't been utilized since the port to new mount API: - Opt_ignorebadroots - Opt_ignoredatacsums - Opt_rescue_all All those enums are from the old day where we have dedicated mount options, nowadays they have been moved to "rescue=" mount option groups, and no more global tokens for them. So we can safely remove them now. Reviewed-by:
-
Qu Wenruo authored
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. Reviewed-by:
-
Qu Wenruo authored
[HICCUP] After adding extra checks on btrfs_file_extent_item::ram_bytes to tree-checker, running fsstress leads to tree-checker warning at write time, as we created file extent items with an invalid 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 catches such mismatches 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. Reviewed-by:
-
Qu Wenruo authored
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 adds 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 Previous patch has already fixed the ram_bytes for affected file extents. So this enhanced sanity check should not affect end users. Reviewed-by:
-
Qu Wenruo authored
[HICCUP] Kernels can create file extent items with incorrect ram_bytes like this: item 6 key (257 EXTENT_DATA 0) itemoff 15816 itemsize 53 generation 7 type 1 (regular) extent data disk byte 13631488 nr 32768 extent data offset 0 nr 4096 ram 4096 extent compression 0 (none) Thankfully kernel can handle them properly, as in that case ram_bytes is not utilized at all. [ENHANCEMENT] Since the hiccup is not going to cause any data-loss and is only a minor violation of on-disk format, here we only need to ignore the incorrect ram_bytes value, and use the correct one from btrfs_file_extent_item::disk_num_bytes. Reviewed-by:
-
Qu Wenruo authored
[HICCUP] 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. [ENHANCEMENT] - Rename @bytenr as @disk_bytenr - Only declare @disk_bytenr inside the if branch - Make @disk_bytenr const and remove the modification on it Reviewed-by: -
Mark Harmstone authored
There's a typo in an error message when checking the block group tree feature, it mentions fres-space-tree instead of free-space-tree. Fix that. Signed-off-by:
Mark Harmstone <maharmstone@fb.com> Reviewed-by:
-
Filipe Manana authored
The direct IO code is over a thousand lines and it's currently spread between file.c and inode.c, which makes it not easy to locate some parts of it sometimes. Also inode.c is about 11 thousand lines and file.c about 4 thousand lines, both too big. So move all the direct IO code into a dedicated file, so that it's easy to locate all its code and reduce the sizes of inode.c and file.c. This is a pure move of code without any other changes except export a a couple functions from inode.c (get_extent_allocation_hint() and create_io_em()) because they are used in inode.c and the new direct-io.c file, and a couple functions from file.c (btrfs_buffered_write() and btrfs_write_check()) because they are used both in file.c and in the new direct-io.c file. Reviewed-by:
Boris Burkov <boris@bur.io> Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to btrfs_set_prop() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to btrfs_compress_heuristic() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
The structure is internal so we should use struct btrfs_inode for that, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
The structure is internal so we should use struct btrfs_inode for that. Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to btrfs_ioctl_send() and _btrfs_ioctl_send() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
The structure is internal so we should use struct btrfs_inode for that. Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to is_data_inode() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to btrfs_readdir_get_delayed_items() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
David Sterba authored
Pass a struct btrfs_inode to btrfs_readdir_put_delayed_items() as it's an internal interface, allowing to remove some use of BTRFS_I. Reviewed-by:
-
Johannes Thumshirn authored
Remove the encoding field from 'struct btrfs_stripe_extent'. It was originally intended to encode the RAID type as well as if we're a data or a parity stripe. But the RAID type can be inferred form the block-group and the data vs. parity differentiation can be done easier with adding a new key type for parity stripes in the RAID stripe tree. Signed-off-by:
-
Qu Wenruo authored
When debugging the recent ram_bytes mismatch bug, I can hit it with enhanced tree-checker for file extent items at write time. But the bug is not that easy to trigger (mostly triggered with btrfs/06*, which uses 20 threads fsstress), and when I hit it, the only info is the kernel leaf dump, but it doesn't include things like the file extent type (REGULAR or PREALLOC). Add the dump for generation and type (although only numeric output) to make debugging a little easier. Signed-off-by:
-
Boris Burkov authored
Periodic reclaim attempts to avoid block_groups seeing active use with a sweep mark that gets cleared on allocation and set on a sweep. In urgent conditions where we have very little unallocated space (less than one chunk used by the threshold calculation for the unallocated target), we want to be able to override this mechanism. Introduce a second pass that only happens if we fail to find a reclaim candidate and reclaim is urgent. In that case, do a second pass where all block groups are eligible. Reviewed-by:
-
Boris Burkov authored
Periodic reclaim runs the risk of getting stuck in a state where it keeps reclaiming the same block group over and over. This can happen if 1. reclaiming that block_group fails 2. reclaiming that block_group fails to move any extents into existing block_groups and just allocates a fresh chunk and moves everything. Currently, 1. is a very tight loop inside the reclaim worker. That is critical for edge triggered reclaim or else we risk forgetting about a reclaimable group. On the other hand, with level triggered reclaim we can break out of that loop and get it later. With that fixed, 2. applies to both failures and "successes" with no progress. If we have done a periodic reclaim on a space_info and nothing has changed in that space_info, there is not much point to trying again, so don't, until enough space gets free, which we capture with a heuristic of needing to net free 1 chunk. Reviewed-by:
-
Boris Burkov authored
We currently employ a edge-triggered block group reclaim strategy which marks block groups for reclaim as they free down past a threshold. With a dynamic threshold, this is worse than doing it in a level-triggered fashion periodically. That is because the reclaim itself happens periodically, so the threshold at that point in time is what really matters, not the threshold at freeing time. If we mark the reclaim in a big pass, then sort by usage and do reclaim, we also benefit from a negative feedback loop preventing unnecessary reclaims as we crunch through the "best" candidates. Since this is quite a different model, it requires some additional support. The edge triggered reclaim has a good heuristic for not reclaiming fresh block groups, so we need to replace that with a typical GC sweep mark which skips block groups that have seen an allocation since the last sweep. Reviewed-by:
-
Boris Burkov authored
We can currently recover allocated block_groups by: - explicitly starting balance operations - "auto reclaim" via bg_reclaim_threshold The latter works by checking against a fixed threshold on frees. If we pass from above the threshold to below, relocation triggers and the block group will get reclaimed by the cleaner thread (assuming it is still eligible) Picking a threshold is challenging. Too high, and you end up trying to reclaim very full block_groups which is quite costly, and you don't do reclaim on block_groups that don't get quite THAT full, but could still be quite fragmented and stranding a lot of space. Too low, and you similarly miss out on reclaim even if you badly need it to avoid running out of unallocated space, if you have heavily fragmented block groups living above the threshold. No matter the threshold, it suffers from a workload that happens to bounce around that threshold, which can introduce arbitrary amounts of reclaim waste. To improve this situation, introduce a dynamic threshold. The basic idea behind this threshold is that it should be very lax when there is plenty of unallocated space, and increasingly aggressive as we approach zero unallocated space. To that end, it sets a target for unallocated space (10 chunks) and then linearly increases the threshold as the amount of space short of the target we are increases. The formula is: (target - unalloc) / target I tested this by running it on three interesting workloads: 1. bounce allocations around X% full. 2. fill up all the way and introduce full fragmentation. 3. write in a fragmented way until the filesystem is just about full. 1. and 2. attack the weaknesses of a fixed threshold; fixed either works perfectly or fully falls apart, depending on the threshold. Dynamic always handles these cases well. 3. attacks dynamic by checking whether it is too zealous to reclaim in conditions with low unallocated and low unused. It tends to claw back 1GiB of unallocated fairly aggressively, but not much more. Early versions of dynamic threshold struggled on this test. Additional work could be done to intelligently ratchet up the urgency of reclaim in very low unallocated conditions. Existing mechanisms are already useless in that case anyway. Reviewed-by:
-
Boris Burkov authored
This is handy when computing space_info dynamic reclaim thresholds where we do not have access to a block group. We could add it to the various functions as a parameter, but it seems reasonable for space_info to have an fs_info pointer. Reviewed-by:
-
Boris Burkov authored
When evaluating various reclaim strategies/thresholds against each other, it is useful to collect data about the amount of reclaim happening. Expose a count, error count, and byte count via sysfs per space_info. Note that this is only for automatic reclaim, not manually invoked balances or other codepaths that use "relocate_block_group" Reviewed-by:
-
David Sterba authored
Calling btrfs_handle_fs_error() after btrfs_run_qgroups() fails to update the qgroup status is probably not necessary, this would turn the filesystem to read-only. For the same reason aborting the transaction is also not a good option. The state is left inconsistent and can be fixed by rescan, printing a warning should be sufficient. Return code reflects the status of adding/deleting the relation and if the transaction was ended properly. Reviewed-by:
-
