From: Zygo Blaxell Date: Sun, 16 Aug 2026 07:26:53 +0000 (-0400) Subject: btrfs: reflink: never block on space reservations while the locked range has delalloc X-Git-Url: http://git.hungrycats.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=01fe0af720e7de9b095ba07ce304a22b3a071f67;p=linux btrfs: reflink: never block on space reservations while the locked range has delalloc The previous commit made clone_copy_inline_extent() take its transaction handle before dirtying a folio in the locked destination range, closing a flushoncommit deadlock for the single-extent inline clone. Two gaps remained, and one of them was captured live on a test box within hours: 1) A source file can have an inline extent at offset 0 followed by more items (e.g. created small, then extended by an append). After the inline extent's data is copied into a folio - leaving delalloc inside the locked destination range - the clone loop continues to the next item and btrfs_replace_file_extents() starts more transactions under the lock. The captured deadlock (drgn against a live hung kernel): dedupe: btrfs_replace_file_extents() at [4096,40959], blocked in __reserve_bytes() on a metadata reservation ticket, while holding the dst range lock with extent_state [0,4095] = EXTENT_LOCKED | EXTENT_DELALLOC (dirtied by the inline copy of the source's first item) reclaim: flush_space() -> btrfs_commit_current_transaction() -> btrfs_start_delalloc_flush() -> try_to_writeback_inodes_sb() flusher: find_lock_delalloc_range() on that inode's [0,4095], blocked on the dedupe's extent lock Everything else on the filesystem then queues behind the starved ticket. Note the blocking point here is a reservation ticket, not TRANS_STATE_COMMIT_START: ticket servicing commits the transaction, so with flushoncommit ANY blocking reservation made while the locked range has delalloc can deadlock. Fix: once the inline copy's inode update is committed, unlock the dirtied block before the clone continues. Every byte still locked is clean, so the commit-time flusher never needs our lock, and i_rwsem (held for the whole remap) keeps writers away from the unlocked block until the clone finishes. 2) Delalloc may already exist inside the range when it is first locked: the pre-lock flush waits for ordered extents, but compressed writeback is asynchronous and may not have created them yet. Check for EXTENT_DELALLOC after locking; if present, unlock, flush and retry, giving up with -EAGAIN after a few attempts. Fixes: 05a5a7621ce6 ("Btrfs: implement full reflink support for inline extents") Assisted-by: Claude:claude-fable-5 --- diff --git a/fs/btrfs/reflink.c b/fs/btrfs/reflink.c index 1c16609f62069..b71eea4d815ce 100644 --- a/fs/btrfs/reflink.c +++ b/fs/btrfs/reflink.c @@ -155,7 +155,8 @@ static int clone_copy_inline_extent(struct btrfs_inode *inode, const u64 size, const u8 comp_type, char *inline_data, - struct btrfs_trans_handle **trans_out) + struct btrfs_trans_handle **trans_out, + bool *copied_to_page) { struct btrfs_root *root = inode->root; struct btrfs_fs_info *fs_info = root->fs_info; @@ -338,13 +339,17 @@ copy_to_page: btrfs_delalloc_release_space(inode, data_reserved, new_key->offset, fs_info->sectorsize, true); - } else if (new_key->offset + datal > i_size_read(&inode->vfs_inode)) { - /* - * Keep i_size ahead of a folio we dirtied beyond EOF, so that - * writeback sends it to disk instead of discarding it through - * folio invalidation (see commit b48c980b6a7e). - */ - i_size_write(&inode->vfs_inode, new_key->offset + datal); + } else { + *copied_to_page = true; + if (new_key->offset + datal > i_size_read(&inode->vfs_inode)) { + /* + * Keep i_size ahead of a folio we dirtied beyond EOF, + * so that writeback sends it to disk instead of + * discarding it through folio invalidation (see commit + * b48c980b6a7e). + */ + i_size_write(&inode->vfs_inode, new_key->offset + datal); + } } btrfs_delalloc_release_extents(inode, fs_info->sectorsize); @@ -406,6 +411,7 @@ static int btrfs_clone(struct inode *src, struct inode *inode, u64 datao = 0, datal = 0; u8 comp; u64 drop_start; + bool copied_to_page = false; /* Note the key will change type as we walk through the tree */ ret = btrfs_search_slot(NULL, BTRFS_I(src)->root, &key, path, @@ -556,7 +562,8 @@ process_slot: ret = clone_copy_inline_extent(BTRFS_I(inode), path, &new_key, drop_start, datal, size, - comp, buf, &trans); + comp, buf, &trans, + &copied_to_page); if (ret) goto out; } @@ -587,6 +594,28 @@ process_slot: destoff, olen, no_time_update); if (ret) goto out; + /* + * Copying an inline extent's data into a folio left delalloc + * inside our caller's locked destination range. The rest of + * the clone starts transactions and reserves space, which must + * not block while the lock covers dirty data: under the + * flushoncommit mount option, a transaction commit (including + * one issued to satisfy a metadata reservation ticket) flushes + * dirty inodes through the generic writeback path, and its + * flusher would block forever on our extent lock while we wait + * for that same commit or ticket. Unlock the dirtied block + * now: every byte still locked is clean, so writeback never + * needs our lock, and the inode's i_rwsem (held for the whole + * remap) keeps writers away from the unlocked block until the + * clone finishes. The inode item for the copied data was + * already updated by clone_finish_inode_update() above. + */ + if (copied_to_page) + btrfs_unlock_extent(&BTRFS_I(inode)->io_tree, + new_key.offset, + ALIGN(new_key.offset + datal, + fs_info->sectorsize) - 1, + NULL); if (new_key.offset + datal >= destoff + len) break; @@ -672,8 +701,32 @@ static int btrfs_extent_same_range(struct btrfs_inode *src, u64 loff, u64 len, * we are safe from concurrency with relocation of source extents * because we have already locked the inode's i_mmap_lock in exclusive * mode. + * + * The locked range must also have no delalloc: the clone below starts + * transactions and reserves space while holding the lock, and under + * the flushoncommit mount option a transaction commit (including one + * issued to satisfy a metadata reservation ticket) flushes dirty + * inodes through the generic writeback path, whose flusher blocks on + * our extent lock while we may be blocked waiting on that very commit + * or ticket - a deadlock. The ranges were flushed before locking, + * but compressed writeback is asynchronous and may leave delalloc + * behind by the time we lock; both inodes' i_rwsem are held, so once + * the range is clean and locked nothing can dirty it again (the only + * exception, copying an inline extent's data into a folio, unlocks + * the dirtied block before the clone continues - see btrfs_clone()). */ - btrfs_lock_extent(&dst->io_tree, dst_loff, end, &cached_state); + for (int i = 0; ; i++) { + btrfs_lock_extent(&dst->io_tree, dst_loff, end, &cached_state); + if (!btrfs_test_range_bit_exists(&dst->io_tree, dst_loff, end, + EXTENT_DELALLOC)) + break; + btrfs_unlock_extent(&dst->io_tree, dst_loff, end, &cached_state); + if (i == 2) + return -EAGAIN; + ret = btrfs_wait_ordered_range(dst, dst_loff, end + 1 - dst_loff); + if (ret) + return ret; + } ret = btrfs_clone(&src->vfs_inode, &dst->vfs_inode, loff, len, ALIGN(len, bs), dst_loff, 1); btrfs_unlock_extent(&dst->io_tree, dst_loff, end, &cached_state); @@ -774,9 +827,26 @@ static noinline int btrfs_clone_files(struct file *file, struct file *file_src, * we are safe from concurrency with relocation of source extents * because we have already locked the inode's i_mmap_lock in exclusive * mode. + * + * The locked range must also have no delalloc before the clone starts + * transactions or reserves space under the lock - see the comment in + * btrfs_extent_same_range() for the flushoncommit deadlock this + * prevents. */ end = destoff + len - 1; - btrfs_lock_extent(&BTRFS_I(inode)->io_tree, destoff, end, &cached_state); + for (int i = 0; ; i++) { + btrfs_lock_extent(&BTRFS_I(inode)->io_tree, destoff, end, &cached_state); + if (!btrfs_test_range_bit_exists(&BTRFS_I(inode)->io_tree, + destoff, end, EXTENT_DELALLOC)) + break; + btrfs_unlock_extent(&BTRFS_I(inode)->io_tree, destoff, end, &cached_state); + if (i == 2) + return -EAGAIN; + ret = btrfs_wait_ordered_range(BTRFS_I(inode), destoff, + end + 1 - destoff); + if (ret) + return ret; + } ret = btrfs_clone(src, inode, off, olen, len, destoff, 0); btrfs_unlock_extent(&BTRFS_I(inode)->io_tree, destoff, end, &cached_state);