- 17 Apr, 2023 40 commits
-
-
Qu Wenruo authored
The new helper, scrub_verify_stripe(), shares the same main workflow of the old scrub code. The major differences are: - How pages/page_offset is grabbed Everything can be grabbed from scrub_stripe easily. - When error report happens Currently the helper only verifies the sectors, not really doing any error reporting. The error reporting would be done after we have done the repair. Signed-off-by:
Qu Wenruo <wqu@suse.com> Reviewed-by:
David Sterba <dsterba@suse.com> Signed-off-by:
-
Qu Wenruo authored
The new helper, scrub_verify_one_metadata(), is almost the same as scrub_checksum_tree_block(). The difference is in how we grab the pages from other structures. Signed-off-by:
-
Qu Wenruo authored
The new helper will search the extent tree to find the first extent of a logical range, then fill the sectors array by two loops: - Loop 1 to fill common bits and metadata generation - Loop 2 to fill csum data (only for data bgs) This loop will use the new btrfs_lookup_csums_bitmap() to fill the full csum buffer, and set scrub_sector_verification::csum. With all the needed info filled by this function, later we only need to submit and verify the stripe. Here we temporarily export the helper to avoid warning on unused static function. Signed-off-by:
-
Qu Wenruo authored
This patch introduces the following structures: - scrub_sector_verification Contains all the needed info to verify one sector (data or metadata). - scrub_stripe Contains all needed members (mostly bitmap based) to scrub one stripe (with a length of BTRFS_STRIPE_LEN). The basic idea is, we keep the existing per-device scrub behavior, but merge all the scrub_bio/scrub_bio into one generic structure, and read the full BTRFS_STRIPE_LEN stripe on the first try. This means we will read some sectors which are not scrub target, but that's fine. At dev-replace time we only writeback the utilized and good sectors, and for read-repair we only writeback the repaired sectors. With every read submitted in BTRFS_STRIPE_LEN, the need for complex bio form shaping would be gone. Although to get the same performance of the old scrub behavior, we would need to submit the initial read for two stripes at once. Signed-off-by:
-
Qu Wenruo authored
Both scrub and read-repair are utilizing a special repair writes that: - Only writes back to a single device Even for read-repair on RAID56, we only update the corrupted data stripe itself, not triggering the full RMW path. - Requires a valid @mirror_num For RAID56 case, only @mirror_num == 1 is valid. For non-RAID56 cases, we need @mirror_num to locate our stripe. - No data csum generation needed These two call sites still have some differences though: - Read-repair goes plain bio It doesn't need a full btrfs_bio, and goes submit_bio_wait(). - New scrub repair would go btrfs_bio To simplify both read and write path. So here this patch would: - Introduce a common helper, btrfs_map_repair_block() Due to the single device nature, we can use an on-stack btrfs_io_stripe to pass device and its physical bytenr. - Introduce a new interface, btrfs_submit_repair_bio(), for later scrub code This is for the incoming scrub code. Signed-off-by:
-
Qu Wenruo authored
Currently we're doing a lot of work for btrfs_bio: - Checksum verification for data read bios - Bio splits if it crosses stripe boundary - Read repair for data read bios However for the incoming scrub patches, we don't want this extra functionality at all, just plain logical + mirror -> physical mapping ability. Thus here we do the following changes: - Introduce btrfs_bio::fs_info This is for the new scrub specific btrfs_bio, which would not populate btrfs_bio::inode. Thus we need such new member to grab a fs_info This new member will always be populated. - Replace @inode argument with @fs_info for btrfs_bio_init() and its caller Since @inode is no longer a mandatory member, replace it with @fs_info, and let involved users populate @inode. - Skip checksum verification and generation if @bbio->inode is NULL - Add extra ASSERT()s To make sure: * bbio->inode is properly set for involved read repair path * if @file_offset is set, bbio->inode is also populated - Grab @fs_info from @bbio directly We can no longer go @bbio->inode->root->fs_info, as bbio->inode can be NULL. This involves: * btrfs_simple_end_io() * should_async_write() * btrfs_wq_submit_bio() * btrfs_use_zone_append() Signed-off-by:
-
Qu Wenruo authored
There is really no need to go through the super complex scrub_sectors() to just handle super blocks. Introduce a dedicated function to handle super block scrubbing. This new function will introduce a behavior change, instead of using the complex but concurrent scrub_bio system, here we just go submit-and-wait. There is really not much sense to care the performance of super block scrubbing. It only has 3 super blocks at most, and they are all scattered around the devices already. Reviewed-by:
Christoph Hellwig <hch@lst.de> Reviewed-by:
Anand Jain <anand.jain@oracle.com> Signed-off-by:
-
Anand Jain authored
Commit 321f69f8 ("btrfs: reset device back to allocation state when removing") included adding extent_io_tree_release(&device->alloc_state) to btrfs_close_one_device(), which had already been called in btrfs_free_device(). The alloc_state tree (IO_TREE_DEVICE_ALLOC_STATE), is created in btrfs_alloc_device() and released in btrfs_close_one_device(). Therefore, the additional call to extent_io_tree_release(&device->alloc_state) in btrfs_free_device() is unnecessary and can be removed. Signed-off-by:
-
Anand Jain authored
During my recent search for the root cause of a reported bug, I realized that it's a good idea to issue a warning for missed cleanup instead of using debug-only assertions. Since most installations run with debug off, missed cleanups and premature calls to close could go unnoticed. However, these issues are serious enough to warrant reporting and fixing. Signed-off-by:
-
Christoph Hellwig authored
This was only ever used by btrfs, and the usage just went away. This effectively reverts df91f56a ("libcrc32c: Add crc32c_impl function"). Acked-by:
Herbert Xu <herbert@gondor.apana.org.au> Signed-off-by:
-
Christoph Hellwig authored
Btrfs can use various different checksumming algorithms, and prints the one used for a given file system at mount time. Don't bother printing the crc32c implementation at module load time, the information is available in /sys/fs/btrfs/FSID/checksum. Signed-off-by:
-
Christoph Hellwig authored
The tree-log code has three almost identical copies for the accounting on an extent_buffer that doesn't need to be written any more. The only difference is that walk_down_log_tree passed the bytenr used to find the buffer instead of extent_buffer.start and calculates the length using the nodesize, while the other two callers look at the extent_buffer.len field that must always be equivalent to the nodesize. Factor the code into a common helper. Signed-off-by:
-
Christoph Hellwig authored
Guard all the code to punt bios to a per-cgroup submission helper by a new CONFIG_BLK_CGROUP_PUNT_BIO symbol that is selected by btrfs. This way non-btrfs kernel builds don't need to have this code. Reviewed-by:
Jens Axboe <axboe@kernel.dk> Signed-off-by:
-
Christoph Hellwig authored
async_bio_lock is only taken from bio submission and workqueue context, both are never in bottom halves. Reviewed-by:
-
Christoph Hellwig authored
REQ_CGROUP_PUNT is a bit annoying as it is hard to follow and adds a branch to the bio submission hot path. To fix this, export blkcg_punt_bio_submit and let btrfs call it directly. Add a new REQ_FS_PRIVATE flag for btrfs to indicate to it's own low-level bio submission code that a punt to the cgroup submission helper is required. Reviewed-by:
-
Christoph Hellwig authored
punt_to_cgroup is only used by extent_write_locked_range, but that function also directly controls the bio flags for the actual submission. Remove th punt_to_cgroup field, and just set REQ_CGROUP_PUNT directly in extent_write_locked_range. Signed-off-by:
-
Christoph Hellwig authored
submit_one_async_extent needs to use submit_one_async_extent no matter if the range it handles ends up beeing compressed or not as the deadlock risk due to cgroup thottling is the same. Call kthread_associate_blkcg earlier to cover submit_uncompressed_range case as well. Signed-off-by:
-
Christoph Hellwig authored
Let submit_one_async_extent, which is the only caller of submit_uncompressed_range handle freeing of the async_extent in one central place. Signed-off-by:
-
Christoph Hellwig authored
btrfs_submit_compressed_write should not have to care if it is called from a helper thread or not. Move the kthread_associate_blkcg handling into submit_one_async_extent, as that is the one caller that needs it. Also move the assignment of REQ_CGROUP_PUNT into cow_file_range_async, as that is the routine that sets up the helper thread offload. Signed-off-by:
-
Filipe Manana authored
When starting a transaction, we are assuming the number of bytes used for each delayed ref update matches the number of bytes used for each item update, that is the return value of: btrfs_calc_insert_metadata_size(fs_info, num_items) However that is not correct when we are using the free space tree, as we need to multiply that value by 2, since delayed ref updates need to modify the free space tree besides the extent tree. So fix this by using btrfs_calc_delayed_ref_bytes() to get the correct number of bytes used for delayed ref updates. Signed-off-by:
Filipe Manana <fdmanana@suse.com> Signed-off-by:
-
Filipe Manana authored
When starting a transaction we are comparing the result of a call to btrfs_block_rsv_full() with 0, but the function returns a boolean. While in practice it is not incorrect, as 0 is equivalent to false, it makes it a bit odd and less readable. So update the check to not compare against 0 and instead use the logical not (!) operator. Signed-off-by:
-
Boris Burkov authored
If an application is doing direct io to a btrfs file and experiences a page fault reading from the write buffer, iomap will issue a partial bio, and allow the fs to keep going. However, there was a subtle bug in this code path in the btrfs dio iomap implementation that led to the partial write ending up as a gap in the file's extents and to be read back as zeros. The sequence of events in a partial write, lightly summarized and trimmed down for brevity is as follows: ==== WRITING TASK ==== btrfs_direct_write __iomap_dio_write iomap_iter btrfs_dio_iomap_begin # create full ordered extent iomap_dio_bio_iter bio_iov_iter_get_pages # page fault; partial read submit_bio # partial bio iomap_iter btrfs_dio_iomap_end btrfs_mark_ordered_io_finished # sets BTRFS_ORDERED_IOERR; # submit to finish_ordered_fn wq fault_in_iov_iter_readable # btrfs_direct_write detects partial write __iomap_dio_write iomap_iter btrfs_dio_iomap_begin # create second partial ordered extent iomap_dio_bio_iter bio_iov_iter_get_pages # read all of remainder submit_bio # partial bio with all of remainder iomap_iter btrfs_dio_iomap_end # nothing exciting to do with ordered io ==== DIO ENDIO ==== == FIRST PARTIAL BIO == btrfs_dio_end_io btrfs_mark_ordered_io_finished # bytes_left > 0 # don't submit to finish_ordered_fn wq == SECOND PARTIAL BIO == btrfs_dio_end_io btrfs_mark_ordered_io_finished # bytes_left == 0 # submit to finish_ordered_fn wq ==== BTRFS FINISH ORDERED WQ ==== == FIRST PARTIAL BIO == btrfs_finish_ordered_io # called by dio_iomap_end_io, sees # BTRFS_ORDERED_IOERR, just drops the # ordered_extent ==SECOND PARTIAL BIO== btrfs_finish_ordered_io # called by btrfs_dio_end_io, writes out file # extents, csums, etc... The essence of the problem is that while btrfs_direct_write and iomap properly interact to submit all the correct bios, there is insufficient logic in the btrfs dio functions (btrfs_dio_iomap_begin, btrfs_dio_submit_io, btrfs_dio_end_io, and btrfs_dio_iomap_end) to ensure that every bio is at least a part of a completed ordered_extent. And it is completing an ordered_extent that results in crucial functionality like writing out a file extent for the range. More specifically, btrfs_dio_end_io treats the ordered extent as unfinished but btrfs_dio_iomap_end sets BTRFS_ORDERED_IOERR on it. Thus, the finish io work doesn't result in file extents, csums, etc. In the aftermath, such a file behaves as though it has a hole in it, instead of the purportedly written data. We considered a few options for fixing the bug: 1. treat the partial bio as if we had truncated the file, which would result in properly finishing it. 2. split the ordered extent when submitting a partial bio. 3. cache the ordered extent across calls to __iomap_dio_rw in iter->private, so that we could reuse it and correctly apply several bios to it. I had trouble with 1, and it felt the most like a hack, so I tried 2 and 3. Since 3 has the benefit of also not creating an extra file extent, and avoids an ordered extent lookup during bio submission, it felt like the best option. However, that turned out to re-introduce a deadlock which this code discarding the ordered_extent between faults was meant to fix in the first place. (Link to an explanation of the deadlock below.) Therefore, go with fix 2, which requires a bit more setup work but fixes the corruption without introducing the deadlock, which is fundamentally caused by the ordered extent existing when we attempt to fault in a range that overlaps with it. Put succinctly, what this patch does is: when we submit a dio bio, check if it is partial against the ordered extent stored in dio_data, and if it is, extract the ordered_extent that matches the bio exactly out of the larger ordered_extent. Keep the remaining ordered_extent around in dio_data for cancellation in iomap_end. Thanks to Josef, Christoph, and Filipe with their help figuring out the bug and the fix. Fixes: 51bd9563 ("btrfs: fix deadlock due to page faults during direct IO reads and writes") Link: https://bugzilla.redhat.com/show_bug.cgi?id=2169947 Link: https://lore.kernel.org/linux-btrfs/aa1fb69e-b613-47aa-a99e-a0a2c9ed273f@app.fastmail.com/ Link: https://pastebin.com/3SDaH8C6 Link: https://lore.kernel.org/linux-btrfs/20230315195231.GW10580@twin.jikos.cz/T/#tReviewed-by:Josef Bacik <josef@toxicpanda.com> Tested-by:
Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by:
Boris Burkov <boris@bur.io> [ hch: refactored the ordered_extent extraction ] Signed-off-by:
-
Boris Burkov authored
NOCOW writes just overwrite an existing extent map, which thus should not be split in btrfs_extract_ordered_extent. The NOCOW case can't currently happen as btrfs_extract_ordered_extent is only used on zoned devices that do not support NOCOW writes, but this will change soon. Reviewed-by:
-
Christoph Hellwig authored
To prepare for a new caller that already has the ordered_extent available, change btrfs_extract_ordered_extent to take an argument for it. Add a wrapper for the bio case that still has to do the lookup (for now). Reviewed-by:
-
Christoph Hellwig authored
split_zoned_em is only ever asked to split out the beginning of an extent map. Change it to only take a len to split out instead of a pre and post region. Also rename the function to split_extent_map as there is nothing zoned device specific about it. Reviewed-by:
-
Christoph Hellwig authored
The function btrfs_clone_ordered_extent is very specific to the usage in btrfs_split_ordered_extent. Now that only a single call to btrfs_clone_ordered_extent is left, just fold it into btrfs_split_ordered_extent to make the operation more clear. Reviewed-by:
-
Christoph Hellwig authored
btrfs_split_ordered_extent is only ever asked to split out the beginning of an ordered_extent (i.e. post == 0). Change it to only take a len to split out, and switch it to allocate the new extent for the beginning, as that helps with callers that want to keep a pointer to the ordered_extent that it is stealing from. Reviewed-by:
Naohiro Aota <naohiro.aota@wdc.com> Reviewed-by:
-
Christoph Hellwig authored
btrfs_extract_ordered_extent is always used to split an ordered_extent and extent_map into two parts, so it doesn't need to deal with a three way split. Simplify it by only allowing for a single split point, and always split out the beginning of the extent, as that is what we'll later need to be able to hold on to a reference to the original ordered_extent that the first part is split off for submission. Reviewed-by:
-
Christoph Hellwig authored
Move the three checks that are about ordered extent internal sanity checking into btrfs_split_ordered_extent instead of doing them in the higher level btrfs_extract_ordered_extent routine. Reviewed-by:
-
Boris Burkov authored
While it is not feasible for an ordered extent to survive across the calls btrfs_direct_write makes into __iomap_dio_rw, it is still helpful to stash it on the dio_data in between creating it in iomap_begin and finishing it in either end_io or iomap_end. The specific use I have in mind is that we can check if a particular bio is partial in submit_io without unconditionally looking up the ordered extent. This is a preparatory patch for a later patch which does just that. Reviewed-by:
-
Boris Burkov authored
The ordered_extent flags are declared as unsigned long, so pass them as such to btrfs_add_ordered_extent. Reviewed-by:
-
Boris Burkov authored
Currently, btrfs_add_ordered_extent allocates a new ordered extent, adds it to the rb_tree, but doesn't return a referenced pointer to the caller. There are cases where it is useful for the creator of a new ordered_extent to hang on to such a pointer, so add a new function btrfs_alloc_ordered_extent which is the same as btrfs_add_ordered_extent, except it takes an additional reference count and returns a pointer to the ordered_extent. Implement btrfs_add_ordered_extent as btrfs_alloc_ordered_extent followed by dropping the new reference and handling the IS_ERR case. The type of flags in btrfs_alloc_ordered_extent and btrfs_add_ordered_extent is changed from unsigned int to unsigned long so it's unified with the other ordered extent functions. Reviewed-by:
-
Johannes Thumshirn authored
The btrfs raid56 sector submission code uses bio_add_page() to add a page to a newly created bio. bio_add_page() can fail, but the return value is never checked. Use __bio_add_page() as adding a single page to a newly created bio is guaranteed to succeed. This brings us a step closer to marking bio_add_page() as __must_check. Reviewed-by:
Damien Le Moal <damien.lemoal@opensource.wdc.com> Signed-off-by:
-
Johannes Thumshirn authored
The btrfs repair bio submission code uses bio_add_page() to add a page to a newly created bio. bio_add_page() can fail, but the return value is never checked. Use __bio_add_page() as adding a single page to a newly created bio is guaranteed to succeed. This brings us a step closer to marking bio_add_page() as __must_check. Reviewed-by:
-
Anand Jain authored
The function wait_dev_flush() tests for the BTRFS_DEV_STATE_FLUSH_SENT bit and then clears it separately. Instead, use test_and_clear_bit(). Though we don't need to do the atomic test and clear, it's following a common pattern. Signed-off-by:
-
Anand Jain authored
The flush error code is maintained in btrfs_device::last_flush_error, so there is no point in returning it in wait_dev_flush() when it is not being used. Instead, we can return a boolean value. Note that even though btrfs_device::last_flush_error may not be used, we will keep it for now. Signed-off-by:
-
Anand Jain authored
check_barrier_error() is almost a single line function, and just calls btrfs_check_rw_degradable(). Instead, open code it. Signed-off-by:
-
Anand Jain authored
We parallelize the flush command across devices using our own code, write_dev_flush() sends the flush command to each device and wait_dev_flush() waits for the flush to complete on all devices. Errors from each device are recorded at device->last_flush_error and reset to BLK_STS_OK in write_dev_flush() and to the error, if any, in wait_dev_flush(). These functions are called from barrier_all_devices(). This patch consolidates the use of device->last_flush_error in write_dev_flush() and wait_dev_flush() to remove it from barrier_all_devices(). Signed-off-by:
-
Filipe Manana authored
Instead of using two labels at btrfs_evict_inode() for exiting depending on whether we need to delete the inode items and orphan or some error happened, we can use a single exit label if we initialize the block reserve to NULL, since btrfs_free_block_rsv() ignores a NULL block reserve pointer. So just do that. It will also make an upcoming change simpler by avoiding one extra error label. Reviewed-by:
-
Filipe Manana authored
When updating the global block reserve, we account for the 6 items needed by an unlink operation and the 6 delayed references for each one of those items. However the calculation for the delayed references is not correct in case we have the free space tree enabled, as in that case we need to touch the free space tree as well and therefore need twice the number of bytes. So use the btrfs_calc_delayed_ref_bytes() helper to calculate the number of bytes need for the delayed references at btrfs_update_global_block_rsv(). Reviewed-by:
-
