Repository navigation
Conversation
commented
Mar 5, 2026
Member
Author
|
Re #1623 . |
kdave
force-pushed
the
ci-kvm
branch
2 times, most recently
from
March 5, 2026 23:30
69fc6c9 to
98bf7e7
Compare
kdave
force-pushed
the
for-next
branch
3 times, most recently
from
March 13, 2026 11:46
934d926 to
0cb5a8a
Compare
kdave
force-pushed
the
ci-kvm
branch
3 times, most recently
from
March 13, 2026 19:11
2cd3911 to
c09d7cb
Compare
kdave
force-pushed
the
for-next
branch
8 times, most recently
from
March 17, 2026 16:00
c61e262 to
daed989
Compare
kdave
force-pushed
the
for-next
branch
4 times, most recently
from
March 19, 2026 13:21
9dc51d6 to
d76ed94
Compare
wait_eb_writebacks() iterates the fs_info->buffer_tree xarray and waits for the writeback of these extent-buffers. Waiting for writeback needs to be done without the rcu_read_lock() held (because it can sleep) and thus the loop drops the rcu_read_lock() before calling into wait_on_extent_buffer_writeback(). But extent_buffers are freed through RCU, so if btrfs_release_extent_buffer_rcu() is scheduled while we're still waiting on the writeback, the extent_buffer will be freed causing a use-after-free. Similar to what is done in find_extent_buffer_nolock(), get a reference to the extent_buffer before calling into wait_on_extent_buffer_writeback() so a successful writeback does not free the extent_buffer while we still have a reference to it. Fixes: 2dd7e7b ("btrfs: zoned: wait for extent buffer IOs before finishing a zone") Reviewed-by: Daniel Vacek <neelx@suse.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_reset_unused_block_groups() drops the block-group from fs_info->unused_bgs before performing the work that can actually fail, the actual submission of a REQ_OP_ZONE_RESET. Every failed operation after this leaves the block-group dangling, physically reset, but the in-memory structures aren't and it is not requeued to be reset on fs_info->unused_bgs. Instead of bailing out, re-add this block-group to fs_info->unused_bgs list. Fixes: 453a73c ("btrfs: zoned: reclaim unused zone by zone resetting") Reviewed-by: Daniel Vacek <neelx@suse.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: David Sterba <dsterba@suse.com>
The two functions btrfs_free_reserved_bytes() and btrfs_reset_unused_block_groups() both manipulate space_info->bytes_zone_unusable without using the accessor function btrfs_space_info_update_bytes_zone_unusable() potentially risking an underflow (in case of btrfs_reset_unused_block_groups()). Use btrfs_space_info_update_bytes_zone_unusable() in both function, in the case of btrfs_free_reserved_bytes() it is mostly for documenting purposes. Reviewed-by: Daniel Vacek <neelx@suse.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: David Sterba <dsterba@suse.com>
The on-disk btrfs_chunk structure uses u16 for both num_stripes and sub_stripes, so there is no need to use "int" to represent them in btrfs_chunk_map. Follow the on-disk format to use u16 for those members, including verified_stripes too. And since verified_stripes is u16, it no longer completely fill the hole between @refs and @start. Instead use @num_stripes and @sub_stripes to fill the hole, and leave a 6 bytes hole between @verified_stripes and @stripes. This doesn't save any space for btrfs_chunk_map, but leaves enough space for future expansion. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
Currently btrfs doesn't load btrfs_chunk::stripe_len into btrfs_chunk_map, as tree-checker will reject any chunk whose stripe_len is not 64K. But for the future we will want stripe length other than 64K. As a preparation for the variable stripe length in the future, load btrfs_chunk::stripe_len into btrfs_chunk_map::stripe_len and stripe_shift. For now stripe_len is still 64K, no matter if it's loaded from the on-disk metadata or newly allocated chunk. As tree-checker still rejects any chunk whose stripe_len is not 64K. The two new members fit into the existing hole of btrfs_chunk_map, so the size of btrfs_chunk_map stay the same. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
The new member will be utilized by various path that no direct btrfs_chunk_map is available, but a stripe length is still required. The new member is located inside a hole between @refs and @fs_info, so no size increase for btrfs_io_context. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
Several points to note: - scrub_ctx::stripes[] stripe_len updates We always pre-initialize scrub_ctx::stripes[] at the very beginning of scrub, without holding any btrfs_chunk_map. So init_scrub_stripe() still uses BTRFS_STRIPE_LEN, and all allocated memory are based on that stripe length. But before scrubbing each chunk, update_scrub_stripe_len() will be called to update the scrub_stripe::stripe_len. - Stripe_len should not exceed BTRFS_STRIPE_LEN Since scrub_stripe's heap memory are all based on BTRFS_STRIPE_LEN, any stripe_len larger than BTRFS_STRIPE_LEN will cause access-beyond-boundary. Thankfully there is no plan to support such stripe_len > BTRFS_STRIPE_LEN any time soon. - alloc_scrub_bbio() changes Previously we ensure BTRFS_STRIPE_LEN >= PAGE_SIZE, so BTRFS_STRIPE_LEN >> PAGE_SHIFT can never be zero. But now stripe_len can be smaller than PAGE_SIZE. E.g. stripe_len 4K, page size 64K. So we have to use "round_up(stripe_len, PAGE_SIZE) >> PAGE_SHIFT" instead. Finally alloc_scrub_bbio() is always called with a scrub_stripe pointer as the @Private parameter, change the type from (void *) to (struct scrub_stripe *), so that we can grab the stripe_len without a new parameter. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
That helper is currently using a fixed BTRFS_STRIPE_LEN, change it to accept a 'u32 stripe_len'. Also add an ASSERT() to make sure the @stripe_len passed in is power of 2, and no larger than BTRFS_STRIPE_LEN. There is a special case where we still pass BTRFS_STRIPE_LEN into btrfs_stripe_nr_to_offset(), that's for chunk allocation to determine the minimal device extent size. In that case, since our future planned map->stripe_len is always power of 2 and no larger than BTRFS_STRIPE_LEN, we are totally fine to use the old BTRFS_STRIPE_LEN. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
There are still some BTRFS_STRIPE_LEN usage remaining, but they are all for chunk allocation. There is one more refactor for is_same_chunk_map(), as there are more members to compare, the parameter list can go longer and longer. Instead of passing a new @stripe_len member, pass extent_buffer and btrfs_chunk pointers, and grab the needed member inside that function. And doing "if (unlikely()) return false" for each member check. Although this make is_same_chunk_map() more complex, considering it's only called for system chunks, it should not bring too much overhead. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
So that raid56 can support stripe length other than BTRFS_STRIPE_LEN. There are several extra things to note: - btrfs_raid_bio::stripe_npages is removed With stripe_len < BTRFS_STRIPE_LEN, there is no guarantee that a page won't cross multiple stripes. E.g. with 4K stripe_len and sectorsize, 2 data stripes, with 64K page size, one page will cover the full stripe. So stripe_npages no longer makes sense, just remove it. - alloc_rbio_data_pages()/alloc_rbio_parity_pages() fallback For the same above reason, a page can contain both data and parity blocks. In that case we can no longer only allocate pages for data/parity. Add a fallback that if stripe_len is smaller than page size, always allocate all the pages. This is not the best solution, but should be good enough for now until the variable stripe_len is properly tested. - steal_rbio() fallback If stripe_len is smaller than PAGE_SIZE, it's hard to only steal the data pages. Again skip the stealing if stripe_len is smaller than PAGE_SIZE. Reviewed-by: Johannes Thusmhirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
There are still several call sites using BTRFS_STRIPE_LEN, but that's when doing the basic zoned checks. For other call sites with a btrfs_chunk_map, use map->stripe_len instead of the hardcoded BTRFS_STRIPE_LEN. And since we're here, replace the following pattern if (val & BTRFS_STRIPE_LEN_MASK) with if (!IS_ALIGNED(val, stripe_len)) Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
The new limits are: - Stripe_len must be no smaller than sectorsize As sectorsize is still the minimal unit. - Stripe_len must be no larger than BTRFS_STRIPE_LEN We still have a lot of things requiring on BTRFS_STRIPE_LEN alignment, e.g. dev extent and chunk allocation. A stripe_len larger than BTRFS_STRIPE_LEN will cause problems for the existing chunk allocator. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
There are several BUG()/BUG_ON() inside raid56.c, which cause checkpatch warning if code is touching them. This is especially annoying if using b4, with btrfs-workflow git hooks applied, which makes b4 to wait for user input but without any output. Address them by removing one very impossible BUG(), and using ASSERT() for the remaining ones: - __remove_rbio_from_cache() - rbio_is_full() - unlock_stripe() Replace the BUG_ON() with ASSERT(). - finish_parity_scrub() Remove the BUG(), as alloc_rbio() has already properly calculated real_stripes and nr_data using the number of parity. Furthermore, initialize @has_qstripe at the beginning of the function using the bioc->map_type directly. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
An extent in a block-group that uses the RAID stripe-tree has its logical to physical mapping stored in that tree, which only becomes persistent when the stripe-tree is written out at transaction commit. But the RAID stripe-tree is not logged in the log tree. A fsync therefore logs a file-extent item whose logical range cannot be resolved after a crash. Replay restores the file-extent item and the file has the correct size, but reading it fails with EIO and nothing is reported. Force a full commit when logging such an extent. Extents reach the log both from log_one_extent() and from copy_items(), so check in both. copy_items() skips extents from a past transaction, their mapping is already committed, the same reason their checksums are not logged. Fixes: 02c372e ("btrfs: add support for inserting raid stripe extents") Reviewed-by: Filipe Manana <fdmanana@suse.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: David Sterba <dsterba@suse.com>
When using the no-holes feature (enabled by default since btrfs-progs v5.15) and doing an incremental send, we can miss issuing a hole punching command when the following conditions happen: 1) A hole is punched in a tail section of a file extent item that happens to be the last item in a leaf (for example the extent covers the file range [1M, 3M) and a hole is punched in the range [2M, 3M)). Between the end of this file extent item's range and the i_size, there are other file extent items; 2) The next leaf was not modified in the send snapshot, remaining shared with the parent snapshot. In this scenario we are unable to detect a hole was punched and never issue a hole punch command because with no-holes there we don't have an explicit file extent item to mark a hole. Fix this by teaching maybe_send_hole() to detect such case. A test case for fstests will follow soon. Reported-by: Tristan Elvers <tristanelvers@gmail.com> Link: https://lore.kernel.org/linux-btrfs/CAFoLKK+aErV4p6ZJPnVeH5SHQ5++0f=8ZgxPWe-E-CkeKBs8PA@mail.gmail.com/ Fixes: 16e7549 ("Btrfs: incompatible format change to remove hole extents") Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
…e_info() There's no point in setting the block group's space_info member as the space_info was extracted from the block group early when the variable was declared and did not change. Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
It's not used, so remove it. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
The pattern "rbio->nr_data * rbio->stripe_nsectors" is utilized more than 5 times inside raid56.c, it's worthy to extract it into a new helper. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
The function steal_rbio() is utilized to use the pages that belongs to uptodate data blocks, without doing a page copy. That is done by just updating the dest->stripe_pages[] pointer to use the point inside @src directly. That function previously will reject stripe_len < PAGE_SIZE cases, as a page can contain both data and P/Q blocks. But that shouldn't be a big deal, as long as the corresponding P/Q blocks are not marked uptodate, we can still steal the page that also contains P/Q blocks. So rework steal_rbio() to support stripe_len < PAGE_SIZE by: - Calculate the last page index correctly Instead of iterating all pages but skip P/Q pages, calculate the last page index properly. The last page may contain both data and P/Q blocks, but we can still steal the page. - Update the bitmap in one go Instead of updating the stripe_uptodate_bitmap for every page, we can do the update in just one go, out of the main loop. And that update should not touch the P/Q blocks. - Remove unused helpers * page_nr_to_sector_nr() * page_nr_to_num_sectors() * steal_rbio_page() * is_data_stripe_page() * full_page_sectors_uptodate() This one is very annoying, because of its __maybe_unused attribution. They are based on the old BTRFS_STRIPE_LEN >= PAGE_SIZE assumption, and no longer utilized. - Make index_stripe_sectors() update the stripe_paddrs[] without a page This doesn't cause any functional change for now, as there is no one accessing the source stripe_paddrs[] after a steal. But this should still make the function a little more robust. Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
The functions alloc_rbio_parity_pages() and alloc_rbio_data_pages() didn't support stripe_len < PAGE_SIZE cases, and fallback to alloc_rbio_pages() instead. That's mostly to avoid the case where both data and P/Q blocks can exist in the same page. However even in that case, it's not a big deal, the existing infrastructures like index_stripe_sectors() can handle it correctly. Update those functions to support stripe_len < PAGE_SIZE cases, with two points to note: - Extra attention needed for the round direction For alloc_rbio_data_pages() we need to round up, so the last data page gets allocated. For alloc_rbio_parity_pages() we need to round down, so the first P/Q page get allocated. - Mixed page allocation We can call alloc_rbio_parity_pages() first, then alloc_rbio_data_pages(). In that case, it's alloc_pages_bulk() saving us, which always skips allocated slot. Thus we're still safe. Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
We are manually mapping the folio, copying from it with memcpy() and then unmapping, but this can all simply be done with memcpy_from_folio() that is provided by the MM layer. So use memcpy_from_folio() instead. Reviewed-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Johannes Thumshirn <johannes.thumshrin@wdc.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
We are manually mapping the folio, copying to it with memcpy() and then unmapping, but this can all simply be done with memcpy_to_folio() that is provided by the MM layer. So use memcpy_to_folio() instead. Reviewed-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Johannes Thumshirn <johannes.thumshrin@wdc.com> Signed-off-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
do_zone_finish() calls btrfs_wait_ordered_roots(), which holds
fs_info->ordered_operations_mutex while waiting for writeback. It can be
reached from writeback itself:
cow_file_range()
`-> btrfs_reserve_extent()
`-> find_free_extent()
`-> btrfs_zone_finish_one_bg()
`-> do_zone_finish()
If another task holds the mutex and waits for that writeback, this
deadlocks.
In general, we want to ensure all writes in the block group to
complete before finishing the zone, as relocation does. However, the
current callers do not need the wait (or cannot wait) in do_zone_finish():
- Relocation already waits for reservations and ordered extents itself
before calling btrfs_zone_finish().
- btrfs_delete_unused_bgs() checks that ->reserved is zero and sets the
block group read-only while holding groups_sem, so there can be no
in-flight data write.
- btrfs_zone_finish_one_bg() is called from the allocation path and
cannot wait.
Dropping the wait is still safe. Once the block group is read-only,
->reserved can only drop, and for data it only drops after the ordered
extent's I/O has completed. So if ->reserved is zero, no data write is
left in flight. If it is non-zero, do_zone_finish() returns -EAGAIN,
which all callers handle.
This makes btrfs_zone_finish_one_bg() fail with -EAGAIN when it races
with an allocation in the selected block group.
Fixes: 393f646 ("btrfs: zoned: finish least available block group on data bg allocation")
Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
When mounting a filesystem on a zoned drive passed into a VM via a virtio-blk disk lockdep sporadically complains about a possible deadlock between BTRFS' dev_replace::rwsem and virtio-blk's vdev_mutex: ====================================================== WARNING: possible circular locking dependency detected 7.3.0-rc6+ #317 Not tainted ------------------------------------------------------ mount/7418 is trying to acquire lock: ffff888102c07860 (&vblk->vdev_mutex){+.+.}-{4:4}, at: virtblk_report_zones+0xda/0x440 but task is already holding lock: ffff8881fd725ea8 (&fs_info->dev_replace.rwsem){++++}-{4:4}, at: btrfs_load_block_group_zone_info+0x186/0xe50 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #4 (&fs_info->dev_replace.rwsem){++++}-{4:4}: down_read+0x42/0x170 btrfs_map_block+0x1a6/0xe70 btrfs_submit_bbio+0x1a3/0x910 submit_one_bio+0x93/0xd0 btrfs_readahead+0x1c4/0x200 read_pages+0x56/0x200 page_cache_ra_order+0x27c/0x410 filemap_get_pages+0x12f/0x4a0 filemap_read+0xed/0x400 vfs_read+0x240/0x340 ksys_read+0x61/0xd0 do_syscall_64+0xa3/0x4f0 entry_SYSCALL_64_after_hwframe+0x76/0x7e -> #3 (mapping.invalidate_lock#3){++++}-{4:4}: down_read+0x42/0x170 filemap_fault+0xb0/0x1a40 __do_fault+0x34/0xc0 __handle_mm_fault+0x919/0x1db0 handle_mm_fault+0x97/0x240 do_user_addr_fault+0x21d/0x690 exc_page_fault+0x8e/0x230 asm_exc_page_fault+0x26/0x30 -> #2 (vm_lock){++++}-{0:0}: __vma_start_exclude_readers+0x91/0x1b0 __vma_start_write+0x47/0xd0 vma_expand+0x4c/0x2a0 relocate_vma_down+0x126/0x200 setup_arg_pages+0x240/0x3e0 load_elf_binary+0x1be/0x1860 bprm_execve+0x2d2/0x7b0 kernel_execve+0x107/0x130 kernel_init+0xc2/0x130 ret_from_fork+0x1ff/0x2f0 ret_from_fork_asm+0x1a/0x30 -> #1 (&mm->mmap_lock){++++}-{4:4}: __might_fault+0x68/0xa0 _copy_to_user+0x22/0x70 blkdev_copy_zone_to_user+0x22/0x40 virtblk_report_zones+0x297/0x440 blkdev_report_zones_ioctl+0xfd/0x130 blkdev_ioctl+0x20f/0x2c0 __x64_sys_ioctl+0x86/0xd0 do_syscall_64+0xa3/0x4f0 entry_SYSCALL_64_after_hwframe+0x76/0x7e -> #0 (&vblk->vdev_mutex){+.+.}-{4:4}: __lock_acquire+0x15a8/0x2850 lock_acquire+0xea/0x2f0 __mutex_lock+0xa7/0x10d0 virtblk_report_zones+0xda/0x440 blkdev_report_zones_cached+0x162/0x190 btrfs_get_dev_zones+0xdc/0x2c0 btrfs_load_block_group_zone_info+0x2b6/0xe50 btrfs_read_block_groups+0x338/0xc80 open_ctree+0x1aca/0x24b3 btrfs_get_tree.cold+0x105/0x26c vfs_get_tree+0x28/0xb0 __do_sys_fsconfig+0x324/0x690 do_syscall_64+0xa3/0x4f0 entry_SYSCALL_64_after_hwframe+0x76/0x7e other info that might help us debug this: Chain exists of: &vblk->vdev_mutex --> mapping.invalidate_lock#3 --> &fs_info->dev_replace.rwsem Possible unsafe locking scenario: CPU0 CPU1 ---- ---- rlock(&fs_info->dev_replace.rwsem); lock(mapping.invalidate_lock#3); lock(&fs_info->dev_replace.rwsem); lock(&vblk->vdev_mutex); *** DEADLOCK *** locks held by mount/7418: 3, last CPU#0: #0: ffff888110aa6868 (&fc->uapi_mutex){+.+.}-{4:4}, at: __do_sys_fsconfig+0x2ae/0x690 #1: ffff888112ba20e8 (&type->s_umount_key#31/1){+.+.}-{4:4}, at: sget_fc+0x148/0x8d0 #2: ffff8881fd725ea8 (&fs_info->dev_replace.rwsem){++++}-{4:4}, at: btrfs_load_block_group_zone_info+0x186/0xe50 Holding the rwsem was widened in 0090d6e ("btrfs: zoned: fix use-after-free due to race with dev replace"), but it now also covers calling into blkdev_report_zones_cached(). The device replace ioctl() only works on a mounted filesystem and the only caller with setting 'new' to false is read_one_block_group() which is only called during mount, so it is safe to drop the rwsem before calling into blkdev_report_zones_cached(). Fixes: 0090d6e ("btrfs: zoned: fix use-after-free due to race with dev replace") Reviewed-by: Naohiro Aota <naohiro.aota@wdc.com> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_clone_dev_zone_info() duplicates the original orig_dev->zone_info with kmemdup(), which also copies the seq_zones, empty_zones and active_zones pointers. The clone then replaces them one by one with fresh bitmaps. If one of the allocations fails, the error path frees all three pointers, including those not yet replaced, which still point to the bitmaps owned by the original device. Clear the three pointers right after kmemdup() so the error path only frees what this function allocated. Fixes: 21e61ec ("btrfs: zoned: clone zoned device info when cloning a device") Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com> Signed-off-by: David Sterba <dsterba@suse.com>
If an RMW work failed early, e.g. alloc_rbio_parity_pages() failed, it won't get RBIO_RMW_LOCKED_BIT set, and get rbio_orig_end_io() called. In that case, there can be a race between bio_list_get() on the rbio, and new incoming bio merging into the rbio: A is the rbio that rbio_orig_end_io() is running on. a is the higher layer bio that generated the rbio A. B is the new rbio that will be merged into A. b is the higher layer bio that generated the rbio B. rbio_orig_end_io(A) worker | lock_stripe_add(B) --------------------------------------+--------------------------------- rbio_orig_end_io(A) | lock_stripe_add(B) |- cur = bio_list_get(A) | |- rbio_can_merge(A, B) | |- bio = A->bio_list.head; | | which returned true | | which bio a is returned. | | | | | | | |- A->bio_list.tail = NULL; | | | | | | | | | |- merge_rbio(A, B) | | | | |- bio_list_merge_init() | | | | |- if (A->bio_list.tail) | | | | | A->bio_list.tail is | | | | | still NULL | | | | | | | | | |- A->bio_list.head = b | | | | |- A->bio_list.tail = b | |- A->bio_list.head = NULL; | |- return 1; | |- return bio; | Caller expect rbio A to | which is bio a. | complete bio b. | |- extra = bio_list_get() | Which is NULL | |- rbio_endio_bio_list(cur); Which only ends bio a. In above case, the bio_list race can get bio b lost, and never ended. Thankfully this race window is very tiny, so not practical in the real world. Fix it by setting RBIO_RMW_LOCKED_BIT for error path before calling rbio_orig_end_io(). And since we're ensuring all RMW rbios have RBIO_RMW_LOCKED_BIT bit set before calling rbio_orig_end_io(), add two ASSERT()s for that and remove the old comment. Fixes: 5eb30ee ("btrfs: raid56: introduce the main entrance for RMW path") Assisted-by: LLM (find and analyze the bug) Reviewed-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
If rmw_rbio() hit an error after cache_rbio_pages() call, the rbio still have RBIO_CACHE_READY_BIT set, thus can be utilized for future page stealing. But that rbio didn't really reach the disk, thus using the pages for future steal can cause data inconsistency between in-memory cache and on-disk data. Fix the problem by clearing the RBIO_CACHE_READY_BIT on the error path of rmw_rbio(). Fixes: 5eb30ee ("btrfs: raid56: introduce the main entrance for RMW path") Assisted-by: LLM (find the bug) Reviewed-by: Filipe Manana <fdmanana@suse.com> Signed-off-by: Qu Wenruo <wqu@suse.com> Signed-off-by: David Sterba <dsterba@suse.com>
When putting a workspace we always protect the workspace for the maximum level from reclaim, since it can be used not just for that level but as well for any level below it. However the logic is flawed because if we do the put for a workspace at level 10 for example (maximum is level 15) and there is currently no idle workspace for the maximum level because all are currently being used, we don't add the workspace to the lru list and that can result in leaking it if we don't get more compression or decompression requests for level 10 or any level below 10. Fix this by skipping the addition to the lru list only if the level of the workspace matches the maximum level too (besides the list of idle workspaces for the maximum level being empty). Fixes: 3f93aef ("btrfs: add zstd compression level support") Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com>
…space manager The workspace manager is per filesystem and we have access to the fs_info structure of the filesystem in zstd_alloc_workspace_manager(), so pass it to the btrfs_warn() call so that we get a better log message (including the device, state, etc). Reviewed-by: Qu Wenruo <wqu@suse.com> Signed-off-by: Filipe Manana <fdmanana@suse.com>
[BACKGROUND] In the very early days, commit 325cd4b ("Btrfs: properly set blocksize when adding new device.") set the bdev blocksize as at that time btrfs is using __bread() for super block IO, which went through buffer heads thus requires the strict block size limit. Then btrfs super block IO moved away from buffered heads, by commit 8f32380 ("btrfs: use the page cache for super block reading") and commit 314b6dd ("btrfs: use bios instead of buffer_heads from super block writeout"). So after that there is already no strict blocksize requirement for btrfs, except the bdev's block size should not exceed 4K, which is also the super block size of btrfs. Btrfs data read/writes all follow sectorsize, which can be larger than 4K. Btrfs metadata read/writes all follow nodesize, which is already 16K by default. Furthermore, there is already a bug report that set_blocksize() can trigger a lockdep warning from btrfs, as set_blocksize() will acquire inode lock, and under certain races can lead to lockdep warnings. [ENHANCEMENT] Remove the set_blocksize() calls, instead call bdev_validate_blocksize() inside btrfs_open_device_deny_freeze() and btrfs_get_bdev_and_sb(). Btrfs doesn't use buffer heads, thus no strict set_blocksize() requirement. But we still want to call bdev_validate_blocksize(), just in case where a future block device with larger blocksize (e.g. 16K) shows up. For such devices btrfs super block bios will be rejected, so we want to call bdev_validate_blocksize() early to reject such devices. Furthermore, the bdev_validate_blocksize() itself is lockless, thus it should resolve the lockdep report from syzbot. Reported-by: syzbot+5c01398781fb51df783a@syzkaller.appspotmail.com Link: https://lore.kernel.org/linux-btrfs/6adcb3be.4ce6c07b.2dfc0c.0005.GAE@google.com/ Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Signed-off-by: Qu Wenruo <wqu@suse.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Keep this open, the build tests are on self-hosted workers.