- 11 Jul, 2024 40 commits
-
-
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:
Josef Bacik <josef@toxicpanda.com> Signed-off-by:
Boris Burkov <boris@bur.io> Signed-off-by:
David Sterba <dsterba@suse.com>
-
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:
Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-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:
Qu Wenruo <wqu@suse.com> Signed-off-by:
-
David Sterba authored
There's a transaction joined in the qgroup relation add/remove ioctl and any error will lead to abort/error. We could lift the allocation from btrfs_add_qgroup_relation() and move it outside of the transaction context. The relation deletion does not need that. The ownership of the structure is moved to the add relation handler. Reviewed-by:
-
David Sterba authored
The errors during removing a chunk item are fatal, we expect to have a matching item in the chunk map from which the chunk_offset is taken. Handle that by transaction abort. Reviewed-by:
-
David Sterba authored
The extent item used to have a v0 that was removed in 6.6. There's a check for minimum expected size that could lead to btrfs_handle_fs_error() that would make the filesystem read-only. As we don't have v0 anymore (and haven't seen any reports in the deprecation period), handle this in a less intrusive way. Reviewed-by:
-
David Sterba authored
When an extended ref is deleted we do a sanity check right before removing the item, if we can't find it then handle the error. This is done by btrfs_handle_fs_error() but this is from the time before we had the transaction abort infrastructure, so switch to that. The end result is the same, the error is reported and switched to read-only. We newly return the -ENOENT error code as this better represents what happened. Reviewed-by:
-
Filipe Manana authored
We always allocate a delayed extent op structure when allocating a tree block (except for log trees), but most of the time we don't need it as we only need to set the BTRFS_BLOCK_FLAG_FULL_BACKREF if we're dealing with a relocation tree and we only need to set the key of a tree block in a btrfs_tree_block_info structure if we are not using skinny metadata (feature enabled by default since btrfs-progs 3.18 and available as of kernel 3.10). In these cases, where we don't need neither to update flags nor to set the key, we only use the delayed extent op structure to set the tree block's level. This is a waste of memory and besides that, the memory allocation can fail and can add additional latency. Instead of using a delayed extent op structure to store the level of the tree block, use the delayed ref head to store it. This doesn't change the size of neither structure and helps us avoid allocating delayed extent ops structures when using the skinny metadata feature and there's no relocation going on. This also gets rid of a BUG_ON(). For example, for a fs_mark run, with 5 iterations, 8 threads and 100K files per iteration, before this patch there were 118109 allocations of delayed extent op structures and after it there were none. Reviewed-by:
Filipe Manana <fdmanana@suse.com> Signed-off-by:
-
Boris Burkov authored
When qgroups are enabled, during data reservation, we allocate the ulist_nodes that track the exact reserved extents with GFP_ATOMIC unconditionally. This is unnecessary, and we can follow the model already employed by the struct extent_state we preallocate in the non qgroups case, which should reduce the risk of allocation failures with GFP_ATOMIC. Add a prealloc node to struct ulist which ulist_add will grab when it is present, and try to allocate it before taking the tree lock while we can still take advantage of a less strict gfp mask. The lifetime of that node belongs to the new prealloc field, until it is used, at which point it belongs to the ulist linked list. Reviewed-by:
-
Filipe Manana authored
Instead of doing a BUG_ON() handle the error by returning -EUCLEAN, aborting the transaction and logging an error message. Reviewed-by:
-
Filipe Manana authored
Instead of using an if-else statement when processing the extent item at btrfs_lookup_extent_info(), use a single if statement for the error case since it does a goto at the end and leave the success (expected) case following the if statement, reducing indentation and making the logic a bit easier to follow. Also make the if statement's condition as unlikely since it's not expected to ever happen, as it signals some corruption, making it clear and hint the compiler to generate more efficient code. Reviewed-by:
-
Filipe Manana authored
If we didn't found an extent item with the initial btrfs_search_slot() call, it's pointless to test if the "metadata" variable is "true", because right after we check if the key type is BTRFS_METADATA_ITEM_KEY and that is the case only when "metadata" is set to "true". So remove the redundant check. Reviewed-by:
-
Filipe Manana authored
Instead of a BUG_ON() just return an error, log an error message and abort the transaction in case we find an extent buffer belonging to the relocation tree that doesn't have the full backref flag set. This is unexpected and should never happen (save for bugs or a potential bad memory). Reviewed-by:
-
Filipe Manana authored
We keep a "new_flags" variable only to keep track if we need to update the metadata extent's flags, and when we set BTRFS_BLOCK_FLAG_FULL_BACKREF in the variable, we do it in an inner scope. Then check in an outer scope if the variable is not 0 and if so call btrfs_set_disk_extent_flags(). The variable isn't used for anything else. This is somewhat confusing, so to make it more straightforward update the extent's flags where we are currently updating "new_flags" and remove the variable. Reviewed-by:
-
Filipe Manana authored
There are no callers of btrfs_lookup_extent_info() that pass a NULL value for the transaction handle argument, so there's no point in having special logic to deal with the NULL. The last caller that passed a NULL value was removed in commit 19b546d7 ("btrfs: relocation: Use btrfs_find_all_leafs to locate data extent parent tree leaves"). So remove the NULL handling from btrfs_lookup_extent_info(). Reviewed-by:
-
Filipe Manana authored
At btrfs_force_cow_block() we have several error paths that need to unlock the "cow" extent buffer, drop the reference on it and then return an error. This is a bit verbose so add a label where we perform these tasks and make the error paths jump to that label. Signed-off-by:
-
Filipe Manana authored
When freeing a tree block, at btrfs_free_tree_block(), if we fail to create a delayed reference we don't deal with the error and just do a BUG_ON(). The error most likely to happen is -ENOMEM, and we have a comment mentioning that only -ENOMEM can happen, but that is not true, because in case qgroups are enabled any error returned from btrfs_qgroup_trace_extent_post() (can be -EUCLEAN or anything returned from btrfs_search_slot() for example) can be propagated back to btrfs_free_tree_block(). So stop doing a BUG_ON() and return the error to the callers and make them abort the transaction to prevent leaking space. Syzbot was triggering this, likely due to memory allocation failure injection. Reported-by: syzbot+a306f914b4d01b3958fe@syzkaller.appspotmail.com Link: https://lore.kernel.org/linux-btrfs/000000000000fcba1e05e998263c@google.com/Reviewed-by:
-
Filipe Manana authored
It's pointless to pass a super block argument to btrfs_iget_locked() because we always pass a root and from it we can get the super block through: root->fs_info->sb So remove the super block argument. Reviewed-by:
-
Filipe Manana authored
It's pointless to pass a super block argument to btrfs_iget_path() because we always pass a root and from it we can get the super block through: root->fs_info->sb So remove the super block argument. Reviewed-by:
-
Filipe Manana authored
It's pointless to pass a super block argument to btrfs_iget() because we always pass a root and from it we can get the super block through: root->fs_info->sb So remove the super block argument. Reviewed-by:
-
Qu Wenruo authored
Since commit 2b2553f1 ("btrfs: stop setting PageError in the data I/O path") btrfs no longer utilizes subpage error bitmaps anymore, but the commit forgot to remove the error bitmap in btrfs_subpage_dump_bitmap(), resulting in possible meaningless result for the error bitmap. Fix it by just removing the error bitmap dumping. Signed-off-by:
-
Qu Wenruo authored
[BUG] There is a bug report that a canceled checksum conversion (still experimental feature) results in unexpected super block flags: csum_type 0 (crc32c) csum_size 4 csum 0x14973811 [match] bytenr 65536 flags 0x1000000001 ( WRITTEN | CHANGING_FSID_V2 ) magic _BHRfS_M [match] While for a filesystem with ongoing checksum conversion it should have either CHANGING_DATA_CSUM or CHANGING_META_CSUM. [CAUSE] It turns out that, due to btrfs-progs keeps its own extra flags inside its own ctree.h headers, not the shared uapi headers, we have conflicting super flags: kernel-shared/uapi/btrfs_tree.h:#define BTRFS_SUPER_FLAG_METADUMP_V2 (1ULL << 34) kernel-shared/uapi/btrfs_tree.h:#define BTRFS_SUPER_FLAG_CHANGING_FSID (1ULL << 35) kernel-shared/uapi/btrfs_tree.h:#define BTRFS_SUPER_FLAG_CHANGING_FSID_V2 (1ULL << 36) kernel-shared/ctree.h:#define BTRFS_SUPER_FLAG_CHANGING_DATA_CSUM (1ULL << 36) kernel-shared/ctree.h:#define BTRFS_SUPER_FLAG_CHANGING_META_CSUM (1ULL << 37) Note that CHANGING_FSID_V2 is conflicting with CHANGING_DATA_CSUM. [FIX] The proper fix would be done inside btrfs-progs, but to keep everything properly recorded, we should have everything inside the same uapi header. Copy all the new flags into uapi header, and change the value for CHANGING_DATA_CSUM and CHANGING_META_CSUM, while keep the value of CHANGING_BG_TREE untouched. Thankfully checksum change is still only experimental and all those CHANGING_* flags are transient (only for btrfs-progs to resume the conversion, and kernel will reject them all), the damage is still minor. Signed-off-by:
-
Josef Bacik authored
Snapshot delete has some complicated looking code that is weirdly subtle at times. I've cleaned it up the best I can without re-writing it, but there are still a lot of details that are non-obvious. Add a bunch of comments to the main parts of the code to help future developers better understand the mechanics of snapshot deletion. Signed-off-by:
-
Josef Bacik authored
In walk_up_proc() we BUG_ON(ret) from btrfs_dec_ref(). This is incorrect, we have proper error handling here, return the error. Signed-off-by:
-
Josef Bacik authored
In walk_up_proc() we have several sanity checks that should only trip if the programmer made a mistake. Convert these to ASSERT()'s instead of BUG_ON()'s. Signed-off-by:
-
Josef Bacik authored
In reada we BUG_ON(refs == 0), which could be unkind since we aren't holding a lock on the extent leaf and thus could get a transient incorrect answer. In walk_down_proc we also BUG_ON(refs == 0), which could happen if we have extent tree corruption. Change that to return -EUCLEAN. In do_walk_down() we catch this case and handle it correctly, however we return -EIO, which -EUCLEAN is a more appropriate error code. Finally in walk_up_proc we have the same BUG_ON(refs == 0), so convert that to proper error handling. Also adjust the error message so we can actually do something with the information. Signed-off-by:
-
Josef Bacik authored
We have a couple of areas where we check to make sure the tree block is locked before looking up or messing with references. This is old code so it has this as BUG_ON(). Convert this to ASSERT() for developers. Signed-off-by:
-
Josef Bacik authored
We have blanket BUG_ON(ret) after every one of these reference mod attempts, which is just incorrect. If we encounter any errors during walk_down_tree() we will abort, so abort on any one of these failures. Signed-off-by:
-
Josef Bacik authored
We handle errors here properly, ENOMEM isn't fatal, return the error. Signed-off-by:
-
Josef Bacik authored
This is a big chunk of code in do_walk_down() that will conditionally remove the reference for the child block we're currently evaluating. Extract it out into it's own helper and call that from do_walk_down() instead. Signed-off-by:
-
Josef Bacik authored
We currently duplicate the logic for walking into a node during snapshot delete. In one case it is during the actual delete, and in the other we use it for deciding if we should reada the block or not. Factor this code into it's own helper and comment fully what we're doing, and then update the two users to use the new helper. Signed-off-by:
-
Josef Bacik authored
We only set this if wc->refs[level - 1] > 1, and we check this way up above where we need it because the first thing we do before dropping our refs is reset wc->refs[level - 1] to 0. Reorder resetting of wc->refs to after our drop logic, and then remove the need_account variable and simply check wc->refs[level - 1] directly instead of using a variable. Signed-off-by:
-
Josef Bacik authored
do_walk_down() already has a bunch of things going on, and there's a bit of code related to reading in the next eb if we decide we need it. Move this code off into it's own helper to clean up do_walk_down() a little bit. Signed-off-by:
-
Josef Bacik authored
Instead of using a flag we're passing around everywhere, add a field to walk_control that we're already passing around everywhere and use that instead. Signed-off-by:
-
Josef Bacik authored
Currently if our extent buffer isn't uptodate we will drop the lock, free it, and then call read_tree_block() for the bytenr. This is inefficient, we already have the extent buffer, we can simply call btrfs_read_extent_buffer(). Merge these two cases down into one if statement, if we are not uptodate we can drop the lock, trigger readahead, and do the read using btrfs_read_extent_buffer(), and carry on. Signed-off-by:
-
Josef Bacik authored
Currently we have a handful of btrfs_check_eb_owner() calls in various places and helpers that read extent buffers. However we call this in the endio handler for every metadata block, so these extra checks are unnecessary, simply remove them from everywhere except the endio handler. Reviewed-by:
-
Josef Bacik authored
We do find_extent_buffer(), and then if we don't find the eb in cache we call btrfs_find_create_tree_block(), which calls find_extent_buffer() first and then allocates the extent buffer. The reason we're doing this is because if we don't find the extent buffer in cache we set reada = 1. However this doesn't matter, because lower down we only trigger reada if !btrfs_buffer_uptodate(eb), which is what the case would be if we didn't find the extent buffer in cache and had to allocate it. Clean this up to simply call btrfs_find_create_tree_block(), and then use the fact that we're having to read the extent buffer off of disk to go ahead and kick off readahead. Reviewed-by:
-
Filipe Manana authored
As of commit 1b53e51a ("btrfs: don't commit transaction for every subvol create") we started to make any fsync after creating a subvolume to fallback to a transaction commit if the fsync is performed in the same transaction that was used to create the subvolume. This happens with the following at ioctl.c:create_subvol(): $ cat fs/btrfs/ioctl.c (...) /* Tree log can't currently deal with an inode which is a new root. */ btrfs_set_log_full_commit(trans); (...) Note that the comment is misleading as the problem is not that fsync can not deal with the root inode of a new root, but that we can not log any inode that belongs to a root that was not yet persisted because that would make log replay fail since the root doesn't exist at log replay time. The above simply makes any fsync fallback to a full transaction commit if it happens in the same transaction used to create the subvolume - even if it's an inode that belongs to any other subvolume. This is a brute force solution and it doesn't necessarily improve performance for every workload out there - it just moves a full transaction commit from one place, the subvolume creation, to another - an fsync for any inode. Just improve on this by making the fallback to a transaction commit only for an fsync against an inode of the new subvolume, or for the directory that contains the dentry that points to the new subvolume (in case anyone attempts to fsync the directory in the same transaction). Reviewed-by:
-
Filipe Manana authored
When creating and deleting a subvolume, after starting a transaction we are explicitly calling btrfs_record_root_in_trans() for the root which we passed to btrfs_start_transaction(). This is pointless because at transaction.c:start_transaction() we end up doing that call, regardless of whether we actually start a new transaction or join an existing one, and if we were not it would mean the root item of that root would not be updated in the root tree when committing the transaction, leading to problems easy to spot with fstests for example. Remove these redundant calls. They were introduced with commit 74e97958 ("btrfs: qgroup: fix qgroup prealloc rsv leak in subvolume operations"). Reviewed-by:
-
