btrfs_make_block_group() calls btrfs_add_new_free_space() before
assigning cache->space_info. On a zoned filesystem that ends up in
__btrfs_add_free_space_zoned(), which dereferences
block_group->space_info and thus hits a NULL pointer dereference when a
non-initial free space range is added (e.g. during relocation).
Assign cache->space_info before the btrfs_add_new_free_space() call.
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
David Sterba has been doing the Btrfs maintainership work for years, and
my email update to mason@kernel.org seems like a good time to make the
MAINTAINERS file a little more accurate.
Link: https://lore.kernel.org/all/20260827193032.786461-1-clm@meta.com/
Signed-off-by: Chris Mason <clm@meta.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Inside test_find_delalloc() of extent-io-tests.c, if we fail to allocate
a dummy root or the test inode, we go to out label to clean up.
But at that stage, @inode is still NULL and we will call
process_page_range() to access the page cache of the inode, this will
cause NULL pointer dereference.
This is a very minor bug, as it only affects selftests which are not
compiled in by default for most distros, and very hard to trigger.
Fix it by adding a new out_root_info label to handle root and inode
allocation failure.
This is a pre-existing bug reported by Sashiko while reviewing another
patch.
Link: https://sashiko.dev/#/patchset/cover.1786095309.git.wqu%40suse.com
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
A writer can sleep forever in zstd_get_workspace() even though a workspace
is free. When zstd_alloc_workspace() fails, the task is queued on
zwsm->wait and schedules unconditionally, never re-testing the pool.
zstd_put_workspace() publishes the workspace and then calls cond_wake_up(),
which only wakes when a sleeper is already visible, so a workspace returned
between the failed allocation and prepare_to_wait() wakes nobody. The
window is wide: zstd_alloc_workspace() goes through kvmalloc() and may
enter reclaim.
Only a max level workspace triggers the wakeup and one is deliberately kept
allocated as the fallback every waiter waits for, so once its wakeup is
lost the writer stays in TASK_UNINTERRUPTIBLE until some other task happens
to return one. Re-check the pool after prepare_to_wait() has published the
waiter, and use the workspace if one turned up.
Fixes: 3f93aef535 ("btrfs: add zstd compression level support")
Assisted-by: Claude:claude-opus-5
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: FAN YE <fy15309206903@gmail.com>
Signed-off-by: David Sterba <dsterba@suse.com>
[BUG]
When running btrfs/252 with quota enabled through MKFS_OPTIONS="-O quota",
it has a high chance to trigger the following kernel warning and flips
the fs RO:
BTRFS info (device dm-2): relocating block group 30408704 flags metadata|dup
------------[ cut here ]------------
WARNING: fs/btrfs/extent-tree.c:879 at lookup_inline_extent_backref+0x74b/0x960 [btrfs], CPU#4: btrfs/2173
CPU: 4 UID: 0 PID: 2173 Comm: btrfs Not tainted 7.2.0-rc6-custom+ #457 PREEMPT(full) 3adc6528fb66f7a55fe1095385818e742f200aab
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS unknown 02/02/2022
RIP: 0010:lookup_inline_extent_backref+0x74b/0x960 [btrfs]
Call Trace:
<TASK>
insert_inline_extent_backref+0x7c/0x160 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
__btrfs_inc_extent_ref+0xa9/0x270 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
__btrfs_run_delayed_refs+0x4af/0x11c0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_run_delayed_refs+0x9d/0xf0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
create_pending_snapshot+0x39d/0xf00 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
create_pending_snapshots+0x9b/0xc0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_commit_transaction+0x280/0xeb0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
prepare_to_relocate+0x147/0x200 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
relocate_block_group+0x6b/0x5e0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_relocate_block_group+0x92c/0x2380 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_relocate_chunk+0x3f/0x1a0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_balance+0xa2c/0x19c0 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
btrfs_ioctl+0x2839/0x2d30 [btrfs 32f09462c54d9c922fca74a3e4866f4aa7737b72]
__x64_sys_ioctl+0x416/0x9a0
do_syscall_64+0xe1/0x790
entry_SYSCALL_64_after_hwframe+0x4b/0x53
</TASK>
---[ end trace 0000000000000000 ]---
BTRFS info (device dm-2): leaf 4593991680 gen 233 total ptrs 175 free space 5953 owner 2
BTRFS info (device dm-2): refs 3 lock_owner 2173 current 2173
item 0 key (166772736 METADATA_ITEM 1) itemoff 16250 itemsize 33
extent refs 1 gen 222 flags 2
ref#0: tree block backref root 266
[ Skip the tree dump ]
item 174 key (263225344 METADATA_ITEM 0) itemoff 10328 itemsize 33
extent refs 1 gen 162 flags 258
ref#0: tree block backref root 267
BTRFS error (device dm-2): extent item not found for insert, bytenr 179847168 num_bytes 16384 parent 4594335744 root_objectid 273 owner 0 offset 0
BTRFS error (device dm-2): failed to run delayed ref for logical 179847168 num_bytes 16384 type 182 action 1 ref_mod 1: -117
[CAUSE]
The above error is showing that there is a tree reference to a metadata
extent that is no longer there.
With "ref_verify" mount option (requires CONFIG_BTRFS_DEBUG), there is
some extra debug output:
BTRFS error (device dm-2): dumping block entry [180961280 16384], num_refs 0, metadata 1, from disk 0
BTRFS error (device dm-2): root entry 256, num_refs 18446744073709551615
BTRFS error (device dm-2): root entry 273, num_refs 18446744073709551615
BTRFS error (device dm-2): Ref action 3, root 273, ref_root 273, parent 0, owner 0, offset 0, num_refs 1
btrfs_force_cow_block+0x129/0x7d0 [btrfs]
btrfs_cow_block+0x10a/0x250 [btrfs]
btrfs_search_slot+0x5eb/0xf40 [btrfs]
btrfs_insert_empty_items+0x3a/0x70 [btrfs]
insert_with_overflow+0x53/0x130 [btrfs]
btrfs_insert_dir_item+0x125/0x290 [btrfs]
btrfs_add_link+0xaa/0x410 [btrfs]
btrfs_rename+0x5ea/0xcd0 [btrfs]
btrfs_rename2+0x28/0x60 [btrfs]
vfs_rename+0x5b2/0xe10
filename_renameat2+0x244/0x430
__x64_sys_rename+0x48/0x70
do_syscall_64+0xe1/0x790
entry_SYSCALL_64_after_hwframe+0x4b/0x53
BTRFS error (device dm-2): Ref action 2, root 273, ref_root 273, parent 0, owner 0, offset 0, num_refs 18446744073709551615
btrfs_force_cow_block+0x327/0x7d0 [btrfs]
btrfs_cow_block+0x10a/0x250 [btrfs]
btrfs_search_slot+0x5eb/0xf40 [btrfs]
btrfs_lookup_file_extent+0x4d/0x70 [btrfs]
btrfs_drop_extents+0x151/0xf00 [btrfs]
insert_reserved_file_extent+0xfe/0x3e0 [btrfs]
btrfs_finish_one_ordered+0x549/0xc40 [btrfs]
btrfs_work_helper+0xde/0x350 [btrfs]
process_one_work+0x198/0x380
worker_thread+0x1c8/0x330
kthread+0xee/0x120
ret_from_fork+0x28f/0x310
ret_from_fork_asm+0x11/0x20
BTRFS error (device dm-2): Ref action 1, root 273, ref_root 0, parent 4594335744, owner 0, offset 0, num_refs 1
__btrfs_mod_ref+0x1c5/0x2d0 [btrfs]
btrfs_copy_root+0x262/0x390 [btrfs]
create_reloc_root+0xb9/0x370 [btrfs]
btrfs_init_reloc_root+0xb0/0x1b0 [btrfs]
record_root_in_trans+0xa6/0xd0 [btrfs]
create_pending_snapshot+0x383/0xf00 [btrfs]
create_pending_snapshots+0x9b/0xc0 [btrfs]
btrfs_commit_transaction+0x280/0xeb0 [btrfs]
prepare_to_relocate+0x147/0x200 [btrfs]
relocate_block_group+0x6b/0x5e0 [btrfs]
btrfs_relocate_block_group+0x92c/0x2380 [btrfs]
btrfs_relocate_chunk+0x3f/0x1a0 [btrfs]
btrfs_balance+0xa2c/0x19c0 [btrfs]
btrfs_ioctl+0x2839/0x2d30 [btrfs]
__x64_sys_ioctl+0x416/0x9a0
do_syscall_64+0xe1/0x790
The above shows the direct cause, Ref action 3 is the oldest operation,
which shows the tree block is created by COW. Then ref action 2 shows
it's COWed away, by a metadata update, meaning the tree block is already
released, should not be referred any more.
Then the final one, is trying to create a reloc tree for subvolume 273,
and that reloc root creation is referring to the already dropped tree
block.
The root cause is that, during qgroup_account_snapshot(), we are calling
record_root_in_trans() with "force = true".
So if the root has no reloc root, we will create one, but at that
timing it's already too late.
Normally reloc root should be created before the commit and current
roots diverge, to avoid the same problem we are hitting.
But during relocation initialization, we are committing the current
running transaction, with a new reloc_control attached halfway.
And if qgroup is enabled, the record_root_in_trans() with "force = true"
calls will force reloc root creation even if we do not and should not
create reloc root at that timing.
[FIX]
Do not force reloc root creation during record_root_in_trans() with
"force = true" cases, which is only called by qgroup_account_snapshot().
If we're really under relocation, the reloc root should be created way
early, before the commit and current root diverge. If the root has no
reloc tree yet, it means we're still initializing the reloc, and do not
need a reloc root.
So skipping the reloc tree creation in qgroup_account_snapshot() should
be safe.
Link: https://bugzilla.suse.com/show_bug.cgi?id=1275740
Fixes: 4d31778aa2 ("btrfs: qgroup: Fix root item corruption when multiple same source snapshots are created with quota enabled")
Assisted-by: LLM (initial analysis, but incorrect conclusion with too many burnt tokens)
Tested-by: Disha Goel <disgoel@linux.ibm.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
The direct-return refactoring in commit b3047a42f5 ("btrfs: send:
directly return from will_overwrite_ref() and simplify it") changed
will_overwrite_ref() to return directly instead of going through the
common out label.
That resulted in a negative return value from is_inode_existent() to
start being converted to 0, making lookup errors unable to be
distinguished from the inode not existing.
process_recorded_refs() expects negative errors from
will_overwrite_ref() and aborts processing when it receives one.
Return the value from is_inode_existent() to restore the previous error
propagation behavior as it was before the refactor.
Fixes: b3047a42f5 ("btrfs: send: directly return from will_overwrite_ref() and simplify it")
Signed-off-by: Avi Weiss <thnkslprpt@gmail.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
When transaction metadata writeout fails in btrfs_commit_transaction(),
the current code only logs the error, drops tree_log_mutex and then goes
through cleanup_transaction(), which aborts the transaction and records
the fs error.
That is too late for the tree log side. A log sync can already be
waiting on tree_log_mutex, because the committing transaction is moved
to TRANS_STATE_UNBLOCKED while that mutex is held, which lets fsyncs
join the next transaction and queue up in btrfs_sync_log(). Once the
failed commit drops tree_log_mutex, such a log sync acquires it, sees
BTRFS_FS_ERROR() still clear, and writes super_for_commit. That
superblock holds the roots prepared for the transaction that has just
failed to write out its metadata, so it can point at tree blocks that
never reached the disk, and the next mount fails with a parent transid
mismatch.
Commit 165ea85f14 ("btrfs: do not write supers if we have an fs
error") fixed this class of problem by making btrfs_sync_log() check for
an fs error right after taking tree_log_mutex. That check only works if
the commit path publishes the fs error before it releases the same
mutex, and commit 68d4ece9c3 ("btrfs: don't call
btrfs_handle_fs_error() in btrfs_commit_transaction()") removed the only
thing that did so.
Restore the ordering by aborting the transaction while tree_log_mutex is
still held. We have a transaction handle here, so this does not need to
bring back the btrfs_handle_fs_error() call: __btrfs_abort_transaction()
records the fs error itself, which is all btrfs_sync_log() looks at, and
the error message put in its place is kept.
This is what commit 3810ab40af ("btrfs: abort transaction on error in
write_all_supers()") already does for the next call in this function.
This is reproducible on an unmodified kernel by failing the first
couple of bios of a transaction commit with fail_make_request while a
concurrent fsync workload keeps log syncs queued on tree_log_mutex.
Fixes: 68d4ece9c3 ("btrfs: don't call btrfs_handle_fs_error() in btrfs_commit_transaction()")
CC: stable@vger.kernel.org # 7.0+
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: jlayton@meta.com <jlayton@meta.com>
Signed-off-by: Leo Martins <loemra.dev@gmail.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_zone_finish_endio() ignored the return value of do_zone_finish()
and always returned 0, silently dropping a failed zone finish.
Instead propagate any error from do_zone_finish() as the caller
btrfs_finish_ordered_io() already handles it.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
do_zone_finish() clears BLOCK_GROUP_FLAG_ZONE_IS_ACTIVE before finishing
the zones. If call_zone_finish() then fails it returned early, leaving the
now inactive block group on fs_info->zone_active_bgs, leaking its
reference, the BTRFS_FS_NEED_ZONE_FINISH waiters are never woken, and as
its alloc_offset equals the zone capacity btrfs_zone_finish_one_bg() keeps
selecting it, spinning btrfs_zoned_activate_one_bg().
Fall through to the cleanup on failure too and return the error, but keep
the block group read-only as its zones are left inconsistent.
Fixes: d70cbdda75 ("btrfs: zoned: consolidate zone finish functions")
Link: https://sashiko.dev/#/patchset/20260818100037.1366563-1-johannes.thumshirn%40wdc.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>
[BUG]
A corrupted subvolume tree can leave an EXTENT_DATA item attached to an
inode whose mode is not S_IFREG or S_IFLNK. During send, such an item can
be treated as file data and crash through a NULL address_space operation:
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
Call Trace:
<TASK>
read_pages+0x80b/0xb30 mm/readahead.c:173
page_cache_ra_unbounded+0x40d/0x890 mm/readahead.c:302
do_page_cache_ra mm/readahead.c:332 [inline]
page_cache_ra_order+0xa16/0xcd0 mm/readahead.c:535
page_cache_sync_ra+0x5ce/0x9d0 mm/readahead.c:626
page_cache_sync_readahead include/linux/pagemap.h:1379 [inline]
put_file_data fs/btrfs/send.c:5224 [inline]
send_write fs/btrfs/send.c:5291 [inline]
send_extent_data+0x16b2/0x29b0 fs/btrfs/send.c:5715
send_write_or_clone fs/btrfs/send.c:6135 [inline]
process_extent+0x5d4/0x17b0 fs/btrfs/send.c:6504
changed_extent fs/btrfs/send.c:7079 [inline]
changed_cb+0x22f9/0x3cd0 fs/btrfs/send.c:7245
full_send_tree fs/btrfs/send.c:7318 [inline]
send_subvol fs/btrfs/send.c:7910 [inline]
btrfs_ioctl_send+0x46a9/0x57f0 fs/btrfs/send.c:8248
...
[CAUSE]
process_extent() skips extent items for symlinks but assumes every other
inode with an extent item is a regular file. For a corrupted non-regular
inode, btrfs_iget() does not install the regular file address_space
operations. The readahead fallback can then call a NULL read_folio
callback before the existing validation in btrfs_get_extent() can run.
[FIX]
Reject extent items for inode types other than regular files and symlinks
at the common send extent-processing boundary. Symlink handling is left
unchanged because send emits symlink data from read_symlink(). This covers
full, incremental and new-generation sends without adding a check to the
regular I/O path.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: ZhengYuan Huang <gality369@gmail.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
The function btrfs_abort_transaction() only accepts negative error code,
and have the macro VERIFY_NEGATIVE_ERROR() to verify that error code.
But inside update_raid_extent_item(), if there is such key found, we
return 1, breaking the negative error code scheme.
Furthermore if we hit some real error during the tree search, e.g. -EIO,
then the error code is always over-written to -EINVAL.
Fix both problems by following other call sites by overwriting @ret to
-ENOENT if the btrfs_search_slot() failed to locate the key.
This is very unlikely to hit, as we only enter update_raid_extent_item()
if there is a conflicting key already in the raid stripe tree.
This was reported by Sashiko when reviewing another patch.
Link: https://sashiko.dev/#/patchset/20260817021512.3010812-1-shuangpeng.kernel%40gmail.com
Fixes: 8c4cba2adb ("btrfs: update stripe extents for existing logical addresses")
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
There are two possible ways to leak bioc memory on
btrfs_ordered_extent::bioc_list:
- An error occurred for btrfs_insert_one_raid_extent()
Then the function btrfs_insert_raid_extent() immediately return
without freeing any bioc in the bioc_list.
- An ordered extent hit an IO error
In that case the ordered extent will have BTRFS_ORDERED_IOERR set, and
skip the call on btrfs_insert_raid_extent() completely.
Fix the problem by:
- Introduce a new helper, btrfs_cleanup_ordered_bioc_list()
Which will remove all bioc from the bioc_list, and release the bioc.
- Call the above helper for btrfs_insert_raid_extent()
So that the cleanup helper is always called no matter what.
- Call the above helper for btrfs_finish_one_ordered()
This is called just before the final release on the ordered extent.
This was reported by Sashiko when reviewing another patch.
Link: https://sashiko.dev/#/patchset/20260817021512.3010812-1-shuangpeng.kernel%40gmail.com
Fixes: 02c372e1f0 ("btrfs: add support for inserting raid stripe extents")
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
If allocation of a RAID stripe extent fails,
btrfs_insert_one_raid_extent() aborts and ends the transaction before
returning -ENOMEM.
btrfs_finish_one_ordered(), the production caller through
btrfs_insert_raid_extent(), still owns the transaction handle. It handles
the error by aborting the transaction and then reaches the common exit
path, which ends the transaction again.
The premature end can free the handle and drop its transaction reference.
Transaction cleanup can then free the transaction before the caller's
second abort accesses the handle and transaction, resulting in
use-after-free.
Keep the abort at the failure site, but let the caller's common exit path
end the transaction once, after it has finished using both objects.
Fixes: 02c372e1f0 ("btrfs: add support for inserting raid stripe extents")
Assisted-by: Codex:GPT-5
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
Signed-off-by: David Sterba <dsterba@suse.com>
scrub_stripe_report_errors() iterates over the sectors of a stripe, but
every message it emits passes stripe->logical, the address of the first
sector of the 64KiB stripe, rather than the address of the sector being
reported. The physical address is likewise computed once, before the
loop, from stripe->logical.
This matters because scrub_print_common_warning() uses that logical
address for the backref walk which produces the "root %llu inode %llu
offset %llu ... (path: ...)" part of the message. As the address is
always the stripe base, the reported root/inode/offset/path can identify
a different file from the one whose sector actually failed.
A 64KiB stripe routinely spans several extents belonging to unrelated
files. On the machine where this was found, the stripe at logical
0x17D9380000 holds four sectors of /usr/share/plasma/emoji/bg.dict, then
a file inside a docker volume, then sectors referenced only by
snapshots. Every error anywhere in that stripe is attributed to bg.dict.
The effect is visible statistically: across ten months and four kernel
series that machine logged 81 distinct flagged logical addresses, and
every one of them is exactly 64KiB aligned. Since BTRFS_STRIPE_LEN is
64KiB and stripe->logical is stripe aligned by construction, real
failures distributed across sectors could not produce that.
Report the address of the sector actually being examined. Adding the
sector offset to the physical address is valid because BTRFS_STRIPE_LEN
is the unit contiguous on a single device for every profile, so a stripe
never crosses a device boundary.
Fixes: 0096580713 ("btrfs: scrub: introduce error reporting functionality for scrub_stripe")
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: James C. Owens <jamesowens@optonline.net>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Setting the compression property on an inode also sets BTRFS_INODE_COMPRESS
on it, and btrfs_inode_flags_to_fsflags() reports that back as FS_COMPR_FL
to FS_IOC_GETFLAGS. chattr(1), like any other FS_IOC_SETFLAGS caller, reads
the current flags, flips only the bit the user asked for and writes the
whole set back, so a request as unrelated as "chattr +i" reaches
btrfs_fileattr_set() with FS_COMPR_FL set.
btrfs_fileattr_set() takes that as a request to enable compression and
overwrites the compression property with the algorithm from the mount
options, falling back to zlib when the filesystem was not mounted with
-o compress. The algorithm the user selected is silently replaced:
# btrfs property set /mnt/foo compression zstd
# btrfs property get /mnt/foo compression
compression=zstd
# chattr +i /mnt/foo
# btrfs property get /mnt/foo compression
compression=zlib
Every chattr operation triggers this, not just +i, and directories are
affected as well, so files created afterwards inherit the wrong algorithm
too. On a filesystem mounted with -o compress=lzo the property is replaced
with lzo instead. Recovering needs a chattr -i first, because the immutable
flag rejects the setxattr that "btrfs property set" issues.
Prefer the algorithm recorded in the compression property and only fall
back to the mount default when there is no property, so that unrelated
flag changes no longer overwrite the user's choice. Inodes that have the
compress flag set but no property still get the default, so they behave
as before.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Sam Ho <samho@synology.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_init_new_device() switches latest_dev and possibly s_bdev from the
seed device to the new sprout device before creating the first writable
chunks.
If chunk creation or the subsequent sprout setup fails, the error path
releases the new device without switching those pointers back.
btrfs_show_devname() can then dereference the freed latest_dev and crash.
Restore the active device pointers to the latest seed device before
removing and releasing the failed sprout device.
Fixes: b7cb29e666 ("btrfs: update latest_dev when we create a sprout device")
Assisted-by: Codex:gpt-5
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Guanghui Yang <3497809730@qq.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
When creating the first metadata chunk for a sprout filesystem,
create_chunk() adds the new device to the transaction dev_update_list
through device->post_commit_list.
If the subsequent system chunk creation fails, btrfs_init_new_device()
aborts the transaction and releases the device while post_commit_list is
still linked. This triggers a warning in btrfs_free_device() and leaves
the transaction list referencing freed memory.
Detach the device while holding chunk_mutex before releasing it.
Fixes: bbbf7243d6 ("btrfs: combine device update operations during transaction commit")
Assisted-by: Codex:gpt-5
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Guanghui Yang <3497809730@qq.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_dev_replace_start() adds the replacement target to the device list
before marking block groups to copy. If marking fails, returning
directly leaves the target linked and keeps the device accounting
incremented.
Jump to the existing cleanup path so the target device is removed and
released on failure.
The issue was found by a failure-path metadata residual analyzer and
verified with targeted failure injection on v6.14.
Assisted-by: Codex:gpt-5
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Guanghui Yang <3497809730@qq.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
If we the no-holes feature is enabled (a default since btrfs-progs 5.15),
when doing a full fsync we always iterate of all leaves in the subvolume
root that contain file extent items in order to detect holes between them.
This can take a lot of time for files with a large number of extents.
But if we know there are no prealloc extents and the amount of space
(uncompressed space) is greater than or equals to the i_size of the
inode, then we cannot have holes and therefore avoid searching for
them. So skip the search if those conditions are met.
The following test script was used:
$ cat test.sh
#!/bin/bash
MNT=/mnt/nullb0
DEV=/dev/nullb0
umount $MNT &> /dev/null
mkfs.btrfs -f $DEV
mount $DEV $MNT
# 256M gives 64K extents of 4K each.
FILE_SIZE=$((256 * 1024 * 1024))
touch $MNT/foobar
for ((i = 0; i < $FILE_SIZE; i += 8192)); do
xfs_io -c "pwrite -S 0xab $i 4K" $MNT/foobar > /dev/null
done
xfs_io -c "fsync" $MNT/foobar
for ((i = 4096; i < $FILE_SIZE; i += 8192)); do
xfs_io -c "pwrite -S 0xab $i 4K" $MNT/foobar > /dev/null
done
# unmount and mount, clear caches and ensure the next fsync is a
# full sync.
umount $MNT
mount $DEV $MNT
# Do some change to the file in order to fsync.
xfs_io -c "pwrite -S 0xcd 0 4K" $MNT/foobar > /dev/null
T0=$(date +%s%N)
xfs_io -c "fsync" $MNT/foobar
T1=$(date +%s%N)
echo
echo "Took $(( (T1 - T0) / 1000 ))us"
umount $MNT
Before this change:
Took 28721us
After this change:
Took 5453us
That's about 5.3x times faster.
Reviewed-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Inspired by the previous crash exposed by generic/795, we want to make
sure every folio from btrfs page cache is properly aligned to block
size.
This is especially important for bs > ps support, as every btrfs
infrastructure, e.g. extent map and extent state, requires strong block
alignment checks.
Furthermore, also output the minimal folio order from the inode mapping,
which is the determining factor during debugging, helping a lot pinning
down the final cause.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
extent_buffer readahead should not be able to painfully stall a
search_slot and hog tree locks by getting stuck in direct reclaim.
If the allocation fails, that is fine, we simply fail to do the
readahead in that case.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Boris Burkov <boris@bur.io>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Now that we have the btrfs_eb_prealloc struct to carry the allocation
and the "needs prealloc" signal, wire that up between the various
search_slot style callers down into alloc_extent_buffer.
If the prealloc struct indicates that it supports a nowait try, then
alloc_extent_buffer tries to allocate NOWAIT. If that succeeds, great.
Otherwise, we return EAGAIN and signal via the struct that preallocation
is required. The caller then does the allocation and tries again with
the eb, bfs, and folios wired through in the prealloc struct.
If unlock-and-allocate retries are not supported then we just use the
normal gfp flags like before.
Note that there are still two GFP_NOFS allocations, as far as I know,
that happen under the lock and cannot be preallocated:
- the __xa_cmpxchg to insert the eb into the eb xarray
- the xarray allocations for filemap_add_folio to add the folios to
the btree_inode mapping.
The former we could wire up with xa_reserve if we signaled the "prealloc
start" back up to the retry point. However, since there is no concept of
reservation in the filemap xarray, it seemed relatively unhelpful to
bother. These allocations are relatively small cached slab allocations,
so hopefully we can move the needle on reclaim stalls without reserving
them.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Boris Burkov <boris@bur.io>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
In further preparation for supporting NOFAIL allocations with retries
outside the critical section, add a struct to carry the extent_buffer
and btrfs_folio_state we need to allocate.
Refactor the allocation pathways to use the new struct but with no
functional change. Wire empty prealloc structs in from callers.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Boris Burkov <boris@bur.io>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
In preparation for preallocating extent_buffer data, factor eb
initialization away from specifically allocating it. This allows us to
allocate the eb, bfs, folios, etc. together in the main search_slot code
paths, but still share initialization code with the dummy/test/clone
allocation paths.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Boris Burkov <boris@bur.io>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
In that function, we round down the start position and round up the
ending position.
But during the calculation of @len, we use "round_up(start + len,
sectorsize)", which is the rounded up end position, not the rounded up
length.
Which results a much larger length, and later we are still using
"start + len", which is completely incorrect.
Fix it by declaring a local @aligned_start and @aligned_len and use them
instead.
Fixes: bc42bda223 ("btrfs: qgroup: Fix qgroup reserved space underflow by only freeing reserved ranges")
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Extent maps have the extra validation since commit 3f255ece2f ("btrfs:
introduce extra sanity checks for extent maps"), but extent states do
not have a similar check.
Introduce a basic alignment check for the following call sites, so that
we can cover all extent states inserted into the tree:
- insert_state_fast()
- insert_state()
- split_state()
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
In btrfs_extent_same_range() and btrfs_clone_files(), the range passed
into btrfs_lock_extent() is not aligned at its end, because we can
reflink until the EOF, which may not be block aligned.
Although this is not a big deal, for the sake of consistency, and to
prepare for the upcoming stricter alignment check, pass an aligned range
end to btrfs_lock_extent() and btrfs_unlock_extent().
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
The @end parameter for all extent io tree helpers is inclusive, but
the call site in extent_fiemap() is passing an exclusive end into
btrfs_lock_extent(), which will step into the next block unexpectedly.
Pass the inclusive end into btrfs_lock_extent() and
btrfs_unlock_extent().
Fixes: ac3c0d36a2 ("btrfs: make fiemap more efficient and accurate reporting extent sharedness")
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
On a zoned filesystem a freed-but-still-dirty tree block is written out
as zeros (EXTENT_BUFFER_ZONED_ZEROOUT) only to keep the zone write
pointer advancing. btree_csum_one_bio() implemented this by memzeroing
the extent buffer's own folios before submission.
That destroys the in-memory buffer while it may still be referenced. In
particular btrfs_free_tree_block() can run on it afterwards and reads
the header to add a delayed reference; once the header has been zeroed
it frees bytenr 0 and corrupts the extent tree (the
btrfs_header_bytenr(buf) != 0 ASSERT in btrfs_free_tree_block(), or an
"unable to find ref" abort). It is flaky and reproduces under fsstress,
e.g. generic/461 and generic/013.
Write the zeros to disk from the shared zero page instead and leave the
extent buffer content untouched, so any later reference - including the
delayed reference from btrfs_free_tree_block() - still sees a valid
header. end_bbio_meta_write() now clears writeback on the buffer's own
folios, as the bio no longer carries them.
Fixes: aa6313e6ff ("btrfs: zoned: don't clear dirty flag of extent buffer")
Assisted-by: LLM (debugging, commit message)
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
On a zoned filesystem a freed tree block is kept dirty and flagged
EXTENT_BUFFER_ZONED_ZEROOUT so a later writeback zeroes it out and
advances the zone write pointer. Unsynced tree-log updates (e.g. from
rename or link) leave such buffers behind when the log is freed at
commit, and across log generations they can end up ahead of the write
pointer behind a hole, so btree_writepages() can never write them.
During normal operation the space is later reclaimed by a zone reset; at
unmount it is not, and the buffers survive to the final iput() of the
btree inode, which hangs in folio_wait_writeback() once the endio
workqueues are stopped.
They cannot be written back from where they are freed (free_log_tree(),
inside the committing transaction) without deadlocking against that
commit, and they are stale anyway, not referenced by the committed
superblock. Drop their dirty state in close_ctree(), before
btrfs_stop_all_workers().
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
On a zoned filesystem a freed tree block is not cleared but kept dirty
and flagged EXTENT_BUFFER_ZONED_ZEROOUT, so a later writeback zeroes it
out and advances the zone write pointer. A transaction abort turns the
filesystem read-only before that writeback runs, so these buffers stay
dirty and stranded ahead of the write pointer where btree_writepages()
can no longer write them. They survive to the final iput() of the btree
inode at unmount, which submits the write after the endio workqueues are
gone, hanging unmount in folio_wait_writeback().
Clear the dirty state of such buffers when cleaning up the aborted
transaction, where the buffer tree still references all of them.
Assisted-by: LLM (debugging, commit message)
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
btree_writepages() writes the btree inode's dirty metadata in ascending
logical address order. On a zoned filesystem only one metadata and one
system block group is active for writing at a time, and
check_bg_is_active() (via btrfs_check_meta_write_pointer()) pivots the
active block group as writeback moves from one block group to the next.
If the active block group sits at a higher logical address than another
block group that also holds dirty metadata, the ascending walk reaches
the lower one first and, to write it, has to finish the active block
group and activate the lower one. It cannot finish a block group that
still has unsent IO, and during WB_SYNC_ALL && !for_sync (commit)
writeback it deliberately refuses to wait for that IO under
fs_info->zoned_meta_io_lock, as that can deadlock. The pivot thus cannot
issue the submission itself either, so it gives up:
btrfs_check_meta_write_pointer() returns -EAGAIN, which
btrfs_write_and_wait_transaction() treats as fatal and aborts the
transaction, forcing the filesystem read-only. This happens
intermittently under metadata-heavy relocation (e.g. fstests btrfs/187).
Flush the active metadata and system block groups at the start of
btree_writepages(), under the fs_info->zoned_meta_io_lock it already
holds, so they have no unsent IO left and the later pivot can finish
them and make forward progress.
Fixes: 13bb483d32 ("btrfs: zoned: activate metadata block group on write time")
Assisted-by: LLM (debugging, commit message)
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Inside reflink.c we still have a lot of functions passing VFS inode
pointers, then internally convert them into btrfs_inode pointers.
For example, inside btrfs_clone(), we have 12 BTRFS_I() call sites,
while only 3 callsites that really require a VFS inode pointer.
Do the cleanup to convert the following functions to pass a btrfs_inode
pointer instead of a vanilla inode pointer:
- btrfs_clone()
- btrfs_extent_same_range()
- clone_finish_inode_update().
Which covers all ad-hoc BTRFS_I() call sites inside reflink.c.
Reviewed-by: Daniel Vacek <neelx@suse.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
We are using atomic types for the log_commit array of struct btrfs_root
but all we need is simple booleans. The log_commit array elements are
always protected by the root's log_mutex, both for writes and reads, so
we can use a simple boolean. The use of atomics if from the very early
days of the log tree code where the access to the fields was not protected
by any lock.
So switch to simple booleans, which results in cheaper code and slightly
reduces the object size too.
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
We check for the exit condition after we add ourselves to the wait queue
and before we unlock the root's log_mutex, sleep and lock again log_mutex.
This is not incorrect, but it's not optimal since in the first iteration
this is pointless because we already know that root->log_commit[index] is
not zero, so we should check the exit condition only after unlocking
log_mutex, sleeping, waking up and locking again the log_mutex.
So move the check for the exit condition to bottom of the loop, after we
were woken and locked log_mutex again.
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Instead of having every caller check for root->log_commit[] being non-zero
and then call wait_log_commit(), move the check into wait_log_commit() and
have the callers call it unconditionally.
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
We have the log batch counter defined per root which is now useless after
the previous patch (titled: "btrfs: stop sleeping for one jiffy in non-ssd
mounts during log commit"). The counter is incremented early in the fsync
path, before and after flushing dellaloc and waiting for writeback, and
then the counter is read during the log sync path. The goal was to wait
for tasks that are about to join a log transaction, so that we could
reduce the amount of IO and log syncing (flush all log tree extent buffers
and write super blocks), but that mechanism does not work since if there
are currently no log writers, btrfs_sync_log() does not unlock the root's
log_mutex, so no new log writers can join the log transaction. Having
concurrent fsync tasks increasing the log_batch counter only makes us loop
unnecessarily in btrfs_sync_log() - that is always true since the previous
patch mentioned above and was true before that patch only when not using
the "-o ssd" mount option (which is activated by default if the filesystem
does not have rotational devices).
So remove the log batch counter. No performance changes were observed
after removing it.
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Joining/starting a log transaction tracks if we ever had more than one task
concurrently logging by setting the flag BTRFS_ROOT_MULTI_LOG_TASKS in the
respective root. Once set, this flag remains for the rest of the lifetime
of the transaction, only cleared when we don't have a log root and need to
create a new one (transaction commits drop log roots).
During log commit, if we are not on a ssd mount (or use the -o nossd mount
option) and the BTRFS_ROOT_MULTI_LOG_TASKS flag is set, we sleep for one
jiffy with the excuse to allow future log writers to join and log inodes
and then commit a larger log transaction to reduce overall IO. However
this is extremely inefficient because:
1) If at some point we had multiple tasks logging concurrently but now
we have only one task at a time, we force it to wait for 1 jiffy;
2) One jiffy can vary between 1ms to 10ms, depending on the kernel
config option CONFIG_HZ, which by default has a value of 250HZ and
that corresponds to 4ms - that is a lot.
This massively reduces the latency of fsyncs for non-ssd mounts, even
on consumer grade spinning disks.
Remove this mechanism to track if we have (or ever had) multiple tasks
logging and wait for 1 jiffy.
The following fio test was used to benchmark:
$ cat fio-buffered-fsync.sh
DEV=/dev/sdj
MNT=/mnt/sdj
MOUNT_OPTIONS=""
MKFS_OPTIONS=""
if [ $# -ne 6 ]; then
echo "Use $0 NUM_JOBS FILE_SIZE IO_SIZE FSYNC_FREQ BLOCK_SIZE [write|randwrite]"
exit 1
fi
NUM_JOBS=$1
FILE_SIZE=$2
IO_SIZE=$3
FSYNC_FREQ=$4
BLOCK_SIZE=$5
WRITE_MODE=$6
if [ "$WRITE_MODE" != "write" ] && [ "$WRITE_MODE" != "randwrite" ]; then
echo "Invalid WRITE_MODE, must be 'write' or 'randwrite'"
exit 1
fi
cat <<EOF > /tmp/fio-job.ini
[writers]
rw=$WRITE_MODE
fsync=$FSYNC_FREQ
fallocate=none
group_reporting=1
direct=0
bs=$BLOCK_SIZE
ioengine=psync
filesize=$FILE_SIZE
io_size=$IO_SIZE
directory=$MNT
numjobs=$NUM_JOBS
EOF
echo
echo "Using config:"
echo
cat /tmp/fio-job.ini
echo
umount $MNT &> /dev/null
mkfs.btrfs -f $MKFS_OPTIONS $DEV
mount $MOUNT_OPTIONS $DEV $MNT
fio /tmp/fio-job.ini
umount $MNT
Running the script as: ./fio-buffered-fsync.sh 8 64M 64M 1 4K randwrite
Before patch:
WRITE: bw=2647KiB/s (2711kB/s), 2647KiB/s-2647KiB/s (2711kB/s-2711kB/s), io=512MiB (537MB), run=198055-198055msec
After patch:
WRITE: bw=14.9MiB/s (15.6MB/s), 14.9MiB/s-14.9MiB/s (15.6MB/s-15.6MB/s), io=512MiB (537MB), run=34471-34471msec
That's about 5.7 times faster.
Reviewed-by: Boris Burkov <boris@bur.io>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
The correct path of the "read_policy" module parameter should be
/sys/module/btrfs/parameters/read_policy. Fix it.
Acked-by: Randy Dunlap <rdunlap@infradead.org>
Signed-off-by: Zenghui Yu <zenghui.yu@linux.dev>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
btrfs_read_merkle_tree_page() can find a folio in the mapping that is not
uptodate. After taking the folio lock, the current code treats that state
as a read error and returns -EIO.
That can make a previous transient read failure sticky. If the failed read
left a not-uptodate folio in the mapping, later callers find that folio and
fail instead of retrying the read.
Keep the existing page-cache insertion and locking order, but retry the
Merkle item read when a not-uptodate folio is found in the mapping. Also
unlock the folio when read_key_bytes() fails so that a later caller can
lock it and retry the read.
Fixes: 06ed09351b ("btrfs: convert btrfs_read_merkle_tree_page() to use a folio")
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Yichong Chen <chenyichong@uniontech.com>
Signed-off-by: David Sterba <dsterba@suse.com>
During an interrupted mount, I got the following messages:
workqueue: Failed to create a rescuer kthread for wq "btrfs-qgroup-rescan": -EINTR
BTRFS error (device dm-3): open_ctree failed: -12
Workqueue code is outputting a human readable error string, meanwhile
we're still using a numeric error code.
So follow the workqueue code to use "%pe" format, which will
automatically convert an error pointer to the human readable string.
However this is a minor pitfall, if the return value is not an error
code, e.g. a positive number, "%pe" with "ERR_PTR(ret)" will output the
pointer as a hash value, e.g.:
ret=1 %pe out=0000000019414716
ret=-22 %pe out=-EINVAL
So we should not use this "%pe" output for callsites that are known to
return positive values.
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
That macro is only utilized 4 times, all inside file.c, while we have
tons of open-coded usages. And since it's a macro, there is no proper
type checks at all.
There isn't much need for such a rarely utilized macro.
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Commit f9a48549a1 ("btrfs: inhibit extent buffer writeback to prevent
COW amplification") tracks the extent buffers a transaction handle has
inhibited in a per-handle xarray. Keying the tracking to the transaction
handle is correct, but using an xarray for it causes two problems in
production.
First, a write_iops regression. Every COW calls
btrfs_inhibit_eb_writeback() from btrfs_force_cow_block() and
should_cow_block(), which does an xa_store() keyed by eb->start. The
kernel test robot reported a 22.6% fio.write_iops regression on a
single-task 4k randwrite workload (ftruncate ioengine, buffered IO) on
btrfs. The cost is the per-COW xarray store done on every COW'd block.
Replacing it with a non-allocating fixed buffer recovers the lost
throughput, and that buffer does more per-COW bookkeeping yet still
recovers, so the cost is the xarray operation itself rather than the
extra tracking work.
Second, an unbounded cleanup walk. btrfs_uninhibit_all_eb_writeback()
iterates every eb the handle inhibited with xa_for_each(). A single
handle that COWs a very large number of blocks (inode eviction, or
truncate of a file with many extents, where btrfs_truncate_inode_items()
loops over many search_again descents under one handle) makes that walk
arbitrarily long. It runs in __btrfs_end_transaction() before
num_writers is dropped, so it blocks the committing thread; this shows up
as multi-second stalls and RCU stall reports.
Replace the xarray with a fixed inline array on btrfs_trans_handle,
managed with a CLOCK (second-chance) eviction policy. Inhibiting a buffer
becomes an array append with no allocation and no tree walk, and the
end-of-handle cleanup is bounded by the array size.
The set that actually needs protection is the working set the handle
revisits across search_again descents, the search path frontier, which is
on the order of the tree height. It is not every block the handle ever
COWs. should_cow_block() re-inhibiting an already tracked buffer marks it
referenced, so revisited buffers survive eviction while write-once buffers
are reclaimed first. A small fixed buffer is therefore enough where a
non-evicting array would either overflow or have to grow without bound.
BTRFS_INHIBITED_EBS_SLOTS is 8 and the reference bits pack into a u32.
The CLOCK eviction is what justifies the extra complexity over a plain
non-evicting array. The test workload stresses amplification: it removes
16 heavily fragmented 64 MiB files in one transaction while background
writeback keeps writing out in-use metadata. A re-COW event is a buffer
already COWed in the running transaction that was written back and then
COWed again; the figure below is the ratio of re-COW events to first-COW
events summed across the eviction (n=5, lower is better):
tracking re-COW per first-COW
no inhibition 6.1
non-evicting array, 32 slots 3.8
CLOCK array, 8 slots (this patch) 1.6
unbounded xarray (reverted) 1.4
The non-evicting array fills with write-once buffers and stops covering
the buffers the handle keeps revisiting, so even at four times the slots
it leaves most of the amplification. CLOCK evicts the cold buffers and
keeps the revisited ones, recovering almost all of the unbounded benefit.
The eviction policy, not the buffer size, is what closes the gap.
eb->writeback_inhibitors and the WB_SYNC_ALL bypass in
lock_extent_buffer_for_io() are unchanged, so fsync and commit behavior
are unaffected. A reference is taken on each tracked buffer so it cannot
be freed while the array points at it; eviction drops that reference and
the inhibitor count.
There's another testing report, showing 20% latency improvement on
reflink and deduplication synthetic benchmark. Full detailed report at
https://github.com/lcf0399/linux-regression-evidence/tree/main/btrfs-remap-writeback-inhibition-v2 .
Link: https://lore.kernel.org/all/CANGjgd=fQkHht2PdDi-+EAdzWH7UtxxWhhJ7b80Rr17PbpgxOw@mail.gmail.com/
Reported-by: kernel test robot <oliver.sang@intel.com>
Fixes: f9a48549a1 ("btrfs: inhibit extent buffer writeback to prevent COW amplification")
Closes: https://lore.kernel.org/oe-lkp/202603112240.f7605968-lkp@intel.com
Tested-by: Chengfeng Lin <lin2530632123@gmail.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: Sun YangKai <sunk67188@gmail.com>
Signed-off-by: Leo Martins <loemra.dev@gmail.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Avoid allocating the heuristic buckets separately from the workspace,
the lifetime is the same.
The new size of struct heuristic_ws is 2112. SLUB merges same/similar
sized structures for the named caches, so there's a chance such size
already exists on the system, like below:
$ grep 2112 /proc/slabinfo
sighand_cache 593 1335 2112 15 8
Signed-off-by: Rosen Penev <rosenp@gmail.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
For a filesystem which has btrfs read-only property set to true, all
write operations including acl and xattr should be denied. However, acl
can still be set even if btrfs ro property is true.
This happens because no function on the set_acl code path checks the root
is readonly or not. It was checked in btrfs_setxattr_trans() but got
removed in commit 353c2ea735 ("btrfs: remove redundant readonly root
check in btrfs_setxattr_trans")
That commit didn't check if all the callers properly check the root's
read-only flag. A previous fix is commit b51111271b ("btrfs: check if
root is readonly while setting security xattr").
Always check if the root is read-only before performing the set acl
operation.
Fixes: 353c2ea735 ("btrfs: remove redundant readonly root check in btrfs_setxattr_trans")
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Sun YangKai <sunyangkai@fnnas.com>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
During relocation recovery, each fs root gets a reference to its relocation
root. If loading or adding a later root fails, or if the first transaction
commit fails, btrfs_recover_relocation() jumps to out_unset before
merge_reloc_roots() and clean_dirty_subvols().
put_reloc_control() drops the list-owned relocation root references, but it
does not clear fs_root->reloc_root or drop the references owned by those
pointers. Mount cleanup only drops them when BTRFS_FS_ERROR is set, so an
error such as -ENOMEM while processing a later root can leave references
behind.
Keep temporary references to the fs roots associated during recovery. On
failure, clear their reloc_root pointers and drop the corresponding
references. Once the first transaction commit succeeds, drop only the
temporary fs root references and let the normal merge and cleanup paths
handle the relocation roots.
Fault injection on a pending-relocation image confirmed the cleanup gap.
With an injected first-commit failure, 25 fs roots had reloc_root set with
fs_error=0. With this fix, the same failure path drops that count to 0
before mount fails.
Fixes: f44deb7442 ("btrfs: hold a ref on the root->reloc_root")
CC: stable@vger.kernel.org
Signed-off-by: Guanghui Yang <3497809730@qq.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Add sctx NULL check in the for loop condition of the sort_clone_roots
cleanup path for consistency with the else branch.
Reviewed-by: Boris Burkov <boris@bur.io>
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Recently kernel RAID56 lib is trying to remove the unexpected
single-data-RAID56 (2 disks RAID5 or 3 disk RAID5) support, meanwhile
btrfs still supports such setup, which means in the long run btrfs has
to handle such corner case by ourselves.
Thankfully single-data-RAID56 is really RAID1/RAID1C3, since data and
P/Q stripes all match each other, rotation also makes no difference.
This patch will disguise those single-data-RAID56 chunks as
RAID1/RAID1C3 chunks.
This is done at two timings:
- Chunk read
- Chunk allocation
This is done by introducing btrfs_chunk_map::on_disk_type member, which
stores the type read from the on-disk metadata.
Meanwhile btrfs_chunk_map::type is calculated using on_disk_type.
For most profiles @type matches @on_disk_type, but for
single-data-RAID56, the @type will be RAID1/RAID1C3.
This method has a minimal impact on the fs, all other operations like
scrub and read-repair, are all based on the chunk map type, so the
disguise method will require no extra modification to those call sites.
Although there are still some locations that are checking against
block_group->flags, e.g. scrub. Those call sites will still get extra
limits assuming the bg is RAID56. But it should not cause any extra
problem.
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
In the function fill_dummy_bgs(), bg->flags is assigned twice.
Just remove the second assignment.
Signed-off-by: Qu Wenruo <wqu@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
Those two members are read from on-disk metadata, but never utilized.
And for new chunks we always set those members to BTRFS_STRIPE_LEN
anyway.
Thus there is no need to keep them inside btrfs_chunk_map.
Signed-off-by: Qu Wenruo <wqu@suse.com>
Signed-off-by: David Sterba <dsterba@suse.com>
