- 19 Jun, 2023 40 commits
-
-
Christoph Hellwig authored
verify_parent_transid is only called by btrfs_buffer_uptodate, which confusingly inverts the return value. Merge the two functions and reflow the parent_transid so that error handling is in a branch. Reviewed-by:
Johannes Thumshirn <johannes.thumshirn@wdc.com> Reviewed-by:
Qu Wenruo <wqu@suse.com> Reviewed-by:
Josef Bacik <josef@toxicpanda.com> Signed-off-by:
Christoph Hellwig <hch@lst.de> Reviewed-by:
David Sterba <dsterba@suse.com> Signed-off-by:
-
Christoph Hellwig authored
Setting the buffer uptodate in a function that is named as a validation helper is a it confusing. Move the call from validate_extent_buffer to the one of its two callers that didn't already have a duplicate call to set_extent_buffer_uptodate. Reviewed-by:
-
Christoph Hellwig authored
Call btrfs_page_clear_uptodate instead of ClearPageUptodate to properly manage the uptodate bit for the subpage case. Reported-by:
-
Christoph Hellwig authored
extent_buffer_under_io is only used in extent_io.c, so mark it static. Reviewed-by:
-
Qu Wenruo authored
There is an internal error report that scrub found an error in an orphan inode's data. However there are very limited ways to cleanup such orphan inodes: - btrfs_start_pre_rw_mount() This happens at either mount, or RO->RW switch. This is not a viable solution for root fs which may not be unmounted or RO mounted. Furthermore this doesn't cover every subvolume, it only covers the currently cached subvolumes. - btrfs_lookup_dentry() This happens when we first lookup the subvolume dentry. But dentry can be cached thus it's not ensured to be triggered every time. - create_snapshot() This only happens for the created snapshot, not the source one. This means if we didn't trigger orphan items cleanup, there is really no other way to manually trigger it. Add this step to the START_SYNC ioctl. This is a slight change in the semantics of the ioctl but as sync can be potentially slow and is usually paired with WAIT_SYNC ioctl. The errors are not handled because the main point of the ioctl is the async commit, orphan cleanup is a side effect. Signed-off-by:
-
Filipe Manana authored
There's a comment at btrfs_init_new_buffer() that refers to a function named btrfs_clean_tree_block(), however the function was renamed to btrfs_clear_buffer_dirty() in commit 190a8339 ("btrfs: rename btrfs_clean_tree_block to btrfs_clear_buffer_dirty"). So update the comment to refer to the current name. Reviewed-by:
Anand Jain <anand.jain@oracle.com> Signed-off-by:
Filipe Manana <fdmanana@suse.com> Reviewed-by:
-
Filipe Manana authored
The for_rename argument of btrfs_record_unlink_dir() is defined as an integer, but the argument is in fact used as a boolean. So change it to a boolean to make its use more clear. Reviewed-by:
-
Filipe Manana authored
There's no point of having a label and goto at btrfs_record_unlink_dir() because the function is trivial and can just return early if we are not in a rename context. So remove the label and goto and instead return early if we are not in a rename. Signed-off-by:
-
Filipe Manana authored
Update the comments at btrfs_record_unlink_dir() so that they mention where new names are logged and where old names are removed. Also, while at it make the width of the comments closer to 80 columns and capitalize the sentences and finish them with punctuation. Signed-off-by:
-
Filipe Manana authored
At btrfs_record_unlink_dir() we directly check the logged_trans field of the given inodes to check if they were previously logged in the current transaction, and if any of them were, then we can avoid setting the field last_unlink_trans of the directory to the id of the current transaction if we are in a rename path. Avoiding that can later prevent falling back to a transaction commit if anyone attempts to log the directory. However the logged_trans field, store in struct btrfs_inode, is transient, not persisted in the inode item on its subvolume b+tree, so that means that if an inode is evicted and then loaded again, its original value is lost and it's reset to 0. So directly checking the logged_trans field can lead to some false negative, and that only results in a performance impact as mentioned before. Instead of directly checking the logged_trans field of the inodes, use the inode_logged() helper, which will check in the log tree if an inode was logged before in case its logged_trans field has a value of 0. This way we can avoid setting the directory inode's last_unlink_trans and cause future logging attempts of it to fallback to transaction commits. The following test script shows one example where this happens without this patch: $ cat test.sh #!/bin/bash DEV=/dev/nullb0 MNT=/mnt/nullb0 num_init_files=10000 num_new_files=10000 mkfs.btrfs -f $DEV mount -o ssd $DEV $MNT mkdir $MNT/testdir for ((i = 1; i <= $num_init_files; i++)); do echo -n > $MNT/testdir/file_$i done echo -n > $MNT/testdir/foo sync # Add some files so that there's more work in the transaction other # than just renaming file foo. for ((i = 1; i <= $num_new_files; i++)); do echo -n > $MNT/testdir/new_file_$i done # Change the file, fsync it. setfattr -n user.x1 -v 123 $MNT/testdir/foo xfs_io -c "fsync" $MNT/testdir/foo # Now triggger eviction of file foo but no eviction for our test # directory, since it is being used by the process below. This will # set logged_trans of the file's inode to 0 once it is loaded again. ( cd $MNT/testdir while true; do : done ) & pid=$! echo 2 > /proc/sys/vm/drop_caches kill $pid wait $pid # Move foo out of our testdir. This will set last_unlink_trans # of the directory inode to the current transaction, because # logged_trans of both the directory and the file are set to 0. mv $MNT/testdir/foo $MNT/foo # Change file foo again and fsync it. # This fsync will result in a transaction commit because the rename # above has set last_unlink_trans of the parent directory to the id # of the current transaction and because our inode for file foo has # last_unlink_trans set to the current transaction, since it was # evicted and reloaded and it was previously modified in the current # transaction (the xattr addition). xfs_io -c "pwrite 0 64K" $MNT/foo start=$(date +%s%N) xfs_io -c "fsync" $MNT/foo end=$(date +%s%N) dur=$(( (end - start) / 1000000 )) echo "file fsync took: $dur milliseconds" umount $MNT Before this patch: fsync took 19 milliseconds After this patch: fsync took 5 milliseconds Signed-off-by: -
Filipe Manana authored
At need_log_inode() we directly check the ->logged_trans field of the given inode to check if it was previously logged in the transaction, with the goal of skipping logging the inode again when it's not necessary. The ->logged_trans field in not persisted in the inode item or elsewhere, it's only stored in memory (struct btrfs_inode), so it's transient and lost once the inode is evicted and then loaded again. Once an inode is loaded, we are conservative and set ->logged_trans to 0, which may mean that either the inode was never logged in the current transaction or it was logged but evicted before being loaded again. Instead of checking the inode's ->logged_trans field directly, we can use instead the helper inode_logged(), which will really check if the inode was logged before in the current transaction in case we have a ->logged_trans field with a value of 0. This will prevent unnecessarily logging an inode when it's not needed, and in some cases preventing a transaction commit, in case the logging requires a fallback to a transaction commit. The following test script shows a scenario where due to eviction we fallback a transaction commit when trying to fsync a file that was renamed: $ cat test.sh #!/bin/bash DEV=/dev/nullb0 MNT=/mnt/nullb0 num_init_files=10000 num_new_files=10000 mkfs.btrfs -f $DEV mount -o ssd $DEV $MNT mkdir $MNT/testdir for ((i = 1; i <= $num_init_files; i++)); do echo -n > $MNT/testdir/file_$i done echo -n > $MNT/testdir/foo sync # Add some files so that there's more work in the transaction other # than just renaming file foo. for ((i = 1; i <= $num_new_files; i++)); do echo -n > $MNT/testdir/new_file_$i done # Fsync the directory first. xfs_io -c "fsync" $MNT/testdir # Rename file foo. mv $MNT/testdir/foo $MNT/testdir/bar # Now trigger eviction of the test directory's inode. # Once loaded again, it will have logged_trans set to 0 and # last_unlink_trans set to the current transaction. echo 2 > /proc/sys/vm/drop_caches # Fsync file bar (ex-foo). # Before the patch the fsync would result in a transaction commit # because the inode for file bar has last_unlink_trans set to the # current transaction, so it will attempt to log the parent directory # as well, which will fallback to a full transaction commit because # it also has its last_unlink_trans set to the current transaction, # due to the inode eviction. start=$(date +%s%N) xfs_io -c "fsync" $MNT/testdir/bar end=$(date +%s%N) dur=$(( (end - start) / 1000000 )) echo "file fsync took: $dur milliseconds" umount $MNT Before this patch: fsync took 22 milliseconds After this patch: fsync took 8 milliseconds Signed-off-by: -
Jiapeng Chong authored
These functions are defined in the scrub.c file, but last callers were removed in e9255d6c ("btrfs: scrub: remove the old scrub recheck code"). fs/btrfs/scrub.c:553:20: warning: unused function 'scrub_stripe_index_and_offset'. fs/btrfs/scrub.c:543:19: warning: unused function 'scrub_nr_raid_mirrors'. Reported-by:
Abaci Robot <abaci@linux.alibaba.com> Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4937Signed-off-by:
Jiapeng Chong <jiapeng.chong@linux.alibaba.com> Reviewed-by:
-
Qu Wenruo authored
[BUG] Smatch reports the following errors related to commit ("btrfs: output affected files when relocation fails"): fs/btrfs/inode.c:283 print_data_reloc_error() error: uninitialized symbol 'ref_level'. [CAUSE] That part of code is mostly copied from scrub, but unfortunately scrub code from the beginning is not doing the error handling properly. The offending code looks like this: do { ret = tree_backref_for_extent(); btrfs_warn_rl(); } while (ret != 1); There are several problems involved: - No error handling If that tree_backref_for_extent() failed, we would output the same error again and again, never really exit as it requires ret == 1 to exit. - Always do one extra output As tree_backref_for_extent() only return > 0 if there is no more backref item. This means after the last item we hit, we would output an invalid error message for ret > 0 case. [FIX] Fix the old code by: - Move @ref_root and @ref_level into the if branch And do not initialize them, so we can catch such uninitialized values just like what we do in the inode.c - Explicitly check the return value of tree_backref_for_extent() And handle ret < 0 and ret > 0 cases properly. - No more do {} while () loop Instead go while (true) {} loop since we will handle @ret manually. Reported-by:Dan Carpenter <dan.carpenter@linaro.org> Signed-off-by:
-
Christoph Hellwig authored
When btrfs_redirty_list_add redirties a buffer, it also acquires an extra reference that is released on transaction commit. But this is not required as buffers that are dirty or under writeback are never freed (look for calls to extent_buffer_under_io())). Remove the extra reference and the infrastructure used to drop it again. History behind redirty logic: In the first place, it used releasing_list to hold all the to-be-released extent buffers, and decided which buffers to re-dirty at the commit time. Then, in a later version, the behaviour got changed to re-dirty a necessary buffer and add re-dirtied one to the list in btrfs_free_tree_block(). In short, the list was there mostly for the patch series' historical reason. Reviewed-by:
Naohiro Aota <naohiro.aota@wdc.com> Signed-off-by:
-
Christoph Hellwig authored
dirty_metadata_bytes is decremented in both places that clear the dirty bit in a buffer, but only incremented in btrfs_mark_buffer_dirty, which means that a buffer that is redirtied using btrfs_redirty_list_add won't be added to dirty_metadata_bytes, but it will be subtracted when written out, leading an inconsistency in the counter. Move the dirty_metadata_bytes from btrfs_mark_buffer_dirty into set_extent_buffer_dirty to also account for the redirty case, and remove the now unused set_extent_buffer_dirty return value. Fixes: d3575156 ("btrfs: zoned: redirty released extent buffers") CC: stable@vger.kernel.org # 5.15+ Reviewed-by:
-
Johannes Thumshirn authored
Mark btrfs_run_discard_work static and move it above its callers. Signed-off-by:
-
Josef Bacik authored
This exists internal to ctree.c, however btrfs check needs to use it for some of its operations. I'd rather not duplicate that code inside of btrfs check as this is low level and I want to keep this code in one place, so rename the function to btrfs_del_ptr and export it so that it can be used inside of btrfs-progs safely. Add a comment to make sure this doesn't get removed by a future cleanup. Signed-off-by:
-
Josef Bacik authored
This is needed in btrfs-progs for the tools that convert the checksum types for file systems and a few other things. We don't have it in the kernel as we just want to get the size for the super blocks type. However I don't want to have to manually add this every time we sync ctree.c into btrfs-progs, so add the helper in the kernel with a note so it doesn't get removed by a later cleanup. Signed-off-by:
-
Josef Bacik authored
We want to override this in btrfs-progs, so wrap this in the __KERNEL__ check so we can easily sync this to btrfs-progs and have our local version of btrfs_no_printk do the work. Reviewed-by:
-
Josef Bacik authored
These are more related to the inode item flags on disk than the in-memory btrfs_inode, move the helpers to inode-item.h. Reviewed-by:
-
Josef Bacik authored
This is more a buffer validation helper, move it into the tree-checker files where it makes more sense. Reviewed-by:
-
Josef Bacik authored
This helper returns a btrfs_tree_block_status for the various errors, and then btrfs_check_node() will return -EUCLEAN if it gets anything other than BTRFS_TREE_BLOCK_CLEAN which will be used by the kernel. In the future btrfs-progs will use this helper instead. Signed-off-by:
-
Josef Bacik authored
Instead of blanket returning -EUCLEAN for all the failures in btrfs_check_leaf, use btrfs_tree_block_status and return the appropriate status for each failure. Rename the helper to __btrfs_check_leaf and then make a wrapper of btrfs_check_leaf that will return -EUCLEAN to non-clean error codes. This will allow us to have the __btrfs_check_leaf variant in btrfs-progs while keeping the behavior in the kernel consistent. Signed-off-by:
-
Josef Bacik authored
We have a variety of item specific errors that can occur. For now simply put these under the umbrella of BTRFS_TREE_BLOCK_INVALID_ITEM, this can be fleshed out as we need in the future. Reviewed-by:
-
Josef Bacik authored
We use this in btrfs-progs to determine if we can fix different types of corruptions. We don't care about this in the kernel, however it would be good to share this code between the kernel and btrfs-progs, so add the status definitions so we can start converting the tree-checker code over to using these status flags instead of blanket returning -EUCLEAN. Reviewed-by:
-
Josef Bacik authored
We have two helpers for checking leaves, because we have an extra check for debugging in btrfs_mark_buffer_dirty(), and at that stage we may have item data that isn't consistent yet. However we can handle this case internally in the helper, if BTRFS_HEADER_FLAG_WRITTEN is set we know the buffer should be internally consistent, otherwise we need to skip checking the item data. Simplify this helper down a single helper and handle the item data checking logic internally to the helper. Reviewed-by:
-
Josef Bacik authored
We just pass in btrfs_header_level(eb) for the level, and we're passing in the eb already, so simply get the level from the eb inside of btrfs_set_block_flags. Reviewed-by:
-
Josef Bacik authored
This is completely related to block rsv's, move it out of the free space cache code and into block-rsv.c. Reviewed-by:
-
Qu Wenruo authored
For P/Q stripe scrub, we have quite some duplicated read IO: - Data stripes read for verification This is triggered by the scrub_submit_initial_read() inside scrub_raid56_parity_stripe(). - Data stripes read (again) for P/Q stripe verification This is triggered by scrub_assemble_read_bios() from scrub_rbio(). Although we can have hit rbio cache and avoid unnecessary read, the chance is very low, as scrub would easily flush the whole rbio cache. This means, even we're just scrubbing a single P/Q stripe, we would read the data stripes twice for the best case scenario. If we need to recover some data stripes, it would cause more reads on the same data stripes, again and again. However before we call raid56_parity_submit_scrub_rbio() we already have all data stripes repaired and their contents ready to use. But RAID56 cache is unaware about the scrub cache, thus RAID56 layer itself still needs to re-read the data stripes. To avoid such cache miss, this patch would: - Introduce a new helper, raid56_parity_cache_data_pages() This function would grab the pages from an array, and copy the content to the rbio, marking all the involved sectors uptodate. The page copy is unavoidable because of the cache pages of rbio are all self managed, thus can not utilize outside pages without screwing up the lifespan. - Use the repaired data stripes as cache inside scrub_raid56_parity_stripe() By this, we ensure all the data sectors of the scrub rbio are already uptodate, and no need to read them again from disk. Signed-off-by:
-
Filipe Manana authored
Removing a free space entry from an in memory space cache requires having the corresponding btrfs_free_space_ctl's 'tree_lock' held. We have several code paths that remove an entry, so add assertions where appropriate to verify we are holding the lock, as the lock is acquired by some other function up in the call chain, which makes it easy to miss in the future. Note: for this to work we need to lock the local btrfs_free_space_ctl at load_free_space_cache(), which was not being done because it's local, declared on the stack, so no other task has access to it. Signed-off-by:
-
Filipe Manana authored
When linking a free space entry, at link_free_space(), the caller should be holding the spinlock 'tree_lock' of the given btrfs_free_space_ctl argument, which is necessary for manipulating the red black tree of free space entries (done by tree_insert_offset(), which already asserts the lock is held) and for manipulating the 'free_space', 'free_extents', 'discardable_extents' and 'discardable_bytes' counters of the given struct btrfs_free_space_ctl. So assert that the spinlock 'tree_lock' of the given btrfs_free_space_ctl is held by the current task. We have multiple code paths that end up calling link_free_space(), and all currently take the lock before calling it. Reviewed-by:
-
Filipe Manana authored
When searching for a free space entry by offset, at tree_search_offset(), we are supposed to have the btrfs_free_space_ctl's 'tree_lock' held, so assert that. We have multiple callers of tree_search_offset(), and all currently hold the necessary lock before calling it. Reviewed-by:
-
Filipe Manana authored
There are multiple code paths leading to tree_insert_offset(), and each path takes the necessary locks before tree_insert_offset() is called, since they do other things that require those locks to be held. This makes it easy to miss the locking somewhere, so make tree_insert_offset() assert that the required locks are being held by the calling task. Signed-off-by:
-
Filipe Manana authored
For the in-memory component of space caching (free space cache and free space tree), three of the arguments passed to tree_insert_offset() can always be taken from the new free space entry that we are about to add. So simplify tree_insert_offset() to take the new entry instead of the 'offset', 'node' and 'bitmap' arguments. This will also allow to make further changes simpler. Signed-off-by:
-
Filipe Manana authored
The are two computations of end offsets at do_trimming() that are not necessary, as they were previously computed and stored in local const variables. So just use the variables instead, to make the source code shorter and easier to read. Reviewed-by:
-
Filipe Manana authored
At try_merge_free_space(), avoid calling twice rb_prev() to find the previous node, as that requires looping through the red black tree, so store the result of the rb_prev() call and then use it. Signed-off-by:
-
Filipe Manana authored
At copy_free_space_cache(), we add a new entry to the block group's ctl before we free the entry from the temporary ctl. Adding a new entry requires the allocation of a new struct btrfs_free_space, so we can avoid a temporary extra allocation by freeing the entry from the temporary ctl before we add a new entry to the main ctl, which possibly also reduces the chances for a memory allocation failure in case of very high memory pressure. So just do that. Reviewed-by:
-
Tom Rix authored
A small code simplification, move the default value of transid to its initialization and remove the else-statement. Signed-off-by:
Tom Rix <trix@redhat.com> Reviewed-by:
-
Qu Wenruo authored
[PROBLEM] When relocation fails (mostly due to checksum mismatch), we only got very cryptic error messages like: BTRFS info (device dm-4): relocating block group 13631488 flags data BTRFS warning (device dm-4): csum failed root -9 ino 257 off 0 csum 0x373e1ae3 expected csum 0x98757625 mirror 1 BTRFS error (device dm-4): bdev /dev/mapper/test-scratch1 errs: wr 0, rd 0, flush 0, corrupt 1, gen 0 BTRFS info (device dm-4): balance: ended with status: -5 The end user has to decipher the above messages and use various tools to locate the affected files and find a way to fix the problem (mostly deleting the file). This is not an easy work even for experienced developer, not to mention the end users. [SCRUB IS DOING BETTER] By contrast, scrub is providing much better error messages: BTRFS error (device dm-4): unable to fixup (regular) error at logical 13631488 on dev /dev/mapper/test-scratch1 physical 13631488 BTRFS warning (device dm-4): checksum error at logical 13631488 on dev /dev/mapper/test-scratch1, physical 13631488, root 5, inode 257, offset 0, length 4096, links 1 (path: file) BTRFS info (device dm-4): scrub: finished on devid 1 with status: 0 Which provides the affected files directly to the end user. [IMPROVEMENT] Instead of the generic data checksum error messages, which is not doing a good job for data reloc inodes, this patch introduce a scrub like backref walking based solution. When a sector fails its checksum for data reloc inode, we go the following workflow: - Get the real logical bytenr For data reloc inode, the file offset is the offset inside the block group. Thus the real logical bytenr is @file_off + @block_group->start. - Do an extent type check If it's tree blocks it's much easier to handle, just go through all the tree block backref. - Do a backref walk and inode path resolution for data extents This is mostly the same as scrub. But unfortunately we can not reuse the same function as the output format is different. Now the new output would be more user friendly: BTRFS info (device dm-4): relocating block group 13631488 flags data BTRFS warning (device dm-4): csum failed root -9 ino 257 off 0 logical 13631488 csum 0x373e1ae3 expected csum 0x98757625 mirror 1 BTRFS warning (device dm-4): checksum error at logical 13631488 mirror 1 root 5 inode 257 offset 0 length 4096 links 1 (path: file) BTRFS error (device dm-4): bdev /dev/mapper/test-scratch1 errs: wr 0, rd 0, flush 0, corrupt 2, gen 0 BTRFS info (device dm-4): balance: ended with status: -5 Reviewed-by:
-
Christoph Hellwig authored
Now that btrfs_wq_submit_bio is never called for synchronous I/O, the hipri_workers workqueue is not used anymore and can be removed. Reviewed-by:
Chris Mason <clm@fb.com> Reviewed-by:
-
