From: Zygo Blaxell Date: Sat, 18 Jul 2026 03:13:26 +0000 (-0400) Subject: btrfs: backref: detect tree mod log rewind cycles in add_all_parents() X-Git-Url: http://git.hungrycats.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fheads%2Fmisc-next%2Ftopics%2Fbackref-hang;p=linux btrfs: backref: detect tree mod log rewind cycles in add_all_parents() The tree mod log records internal node operations (pointer add/remove/ move and boundary key replaces) but never leaf item changes. A leaf that was COWed in the current transaction before a tree mod log user took its sequence number is modified in place afterwards, with no record kept of its old contents. As a result, the "old" view of a subvolume tree presented by btrfs_search_old_slot() and btrfs_next_old_leaf() is only structurally consistent: rewound internal nodes carry boundary keys as of time_seq, while the leaves they point at may have newer contents. When items are redistributed between sibling leaves (push_leaf_left()/push_leaf_right()), only the resulting boundary key change in the parent is logged (via fixup_low_keys()) and therefore rewound - the item moves themselves are invisible to the log. When the rewound boundary keys disagree with the actual leaf contents, btrfs_next_old_leaf() - which advances by re-searching the last key of the current leaf from the top of the tree - can be routed backwards to a leaf it already visited. Two leaves in that state form a closed cycle: walking off the end of leaf L lands on leaf M, and searching for M's last (too-small) key routes back through L to M again. If this happens while the walked range contains no items matching the backref being resolved, the item count never reaches ref->count and the loop in add_all_parents() spins forever. This is easily reproduced by running LOGICAL_INO_V2 concurrently with heavy clone activity on the same files: the ioctl's attached transaction handle then blocks the transaction commit, which drains all writers, freezing the tree in the inconsistent state and turning the transient loop into a permanent hang with the fs wedged behind the blocked commit. If a matching item does fall inside the cycled range instead, the loop exits after counting it multiple times, silently returning duplicated parents. In a consistent view of the tree, item keys must strictly increase as the walk advances. Remember the previously visited key and bail out with -EAGAIN if the walk ever observes a key that did not increase, turning both the hang and the silent duplication into an error. No attempt is made to retry inside the kernel: a retry could only reuse the same tree mod log sequence number, and when the trees are no longer being modified (in particular when a transaction commit is draining writers and waiting for the walker to release its own transaction handle - exactly the scenario that makes the cycle permanent) an identical re-walk fails the same way every time. Returning the error instead releases the caller's transaction handle, which unblocks the pending commit, and a retry of the ioctl from user space then attaches a fresh tree mod log sequence number with a consistent baseline. EAGAIN tells user space precisely that: try again, nothing is corrupted. The check cannot fire on commit root walks (commit roots are immutable for the duration of the walk) or on BTRFS_SEQ_LAST walks (run from the transaction commit critical section, after all other writers have drained and with delayed refs fully run, so the trees are quiescent): a consistent tree view can never present non-increasing item keys, regardless of the size of the walk. So the qgroup accounting done at transaction commit can never see this error and abort the transaction. Only walks with a real time_seq can reach it: the LOGICAL_INO ioctls, which return it to user space, and the sharedness checks done for fiemap and swap file activation. If the check ever did fire on an immutable or quiescent tree, it would mean the tree really has out of order keys, in which case an error is preferable to the alternative of btrfs_next_leaf() cycling on the damaged keys forever. btrfs_is_data_extent_shared() cannot retry with a fresh tree mod log sequence number from that deep in the call chain, and its callers' interfaces do not expect transient errors, so the error is handled at the call sites. Nothing is stored in the sharedness caches for a failed check. fiemap conservatively reports the extent as shared: the SHARED flag is best effort by design, and while the extent's trees are being concurrently modified, any sharedness answer may be stale by the time user space sees it anyway. Swap file activation also fails, with the error converted to the same -EINVAL that a really shared extent produces: swap_activate is a cross-filesystem interface and is not expected to start returning transient error codes, so the EAGAIN is not passed through. A swap file should be fully written and committed with stable extent sharing before swapon() is called, so a transient failure here means the file's extent sharing was still being changed in the same transaction, which is reason enough to reject it. A distinct warning message replaces "swapfile must not be copy-on-write" - which would assert a property of the file that the walk never established - and tells the admin to just retry swapon(), which attaches a fresh tree mod log sequence number with a consistent baseline. Link: https://lore.kernel.org/linux-btrfs/Y28XvZmK0bAS4Ht/@hungrycats.org/ Assisted-by: Claude:claude-fable-5 Signed-off-by: Zygo Blaxell --- diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c index 1be632c742bdd..793b44c6513ae 100644 --- a/fs/btrfs/backref.c +++ b/fs/btrfs/backref.c @@ -477,6 +477,8 @@ static int add_all_parents(struct btrfs_backref_walk_ctx *ctx, struct btrfs_key *key_for_search = &ref->key_for_search; struct btrfs_file_extent_item *fi; struct extent_inode_elem *eie = NULL, *old = NULL; + struct btrfs_key prev_key = { 0 }; + bool have_prev_key = false; u64 disk_byte; u64 wanted_disk_byte = ref->wanted_disk_byte; u64 count = 0; @@ -521,6 +523,36 @@ static int add_all_parents(struct btrfs_backref_walk_ctx *ctx, key.type != BTRFS_EXTENT_DATA_KEY) break; + /* + * The tree mod log does not record leaf changes, and a leaf + * COWed in the current transaction before our time_seq was + * taken is modified in place after it. So the rewound view of + * the tree can have node boundary keys that are inconsistent + * with the current contents of the leaves they point at (for + * example when items were moved between sibling leaves, as + * only the resulting boundary key change in their parent is + * logged and therefore rewound). When they disagree, + * btrfs_next_old_leaf() can navigate backwards, and two + * leaves in that state can form a cycle which makes this + * loop revisit the same items forever. Keys must strictly + * increase as the walk advances, so if they ever do not, + * we hit such a cycle and our view of the tree is not + * consistent: bail out with EAGAIN instead of looping or + * returning duplicated parents. A retry of the ioctl from + * user space attaches a fresh tree mod log sequence number, + * giving the new walk a consistent baseline. + */ + if (have_prev_key && btrfs_comp_cpu_keys(&prev_key, &key) >= 0) { + btrfs_debug(ctx->fs_info, +"backref walk in root %llu went backwards from key " BTRFS_KEY_FMT " to key " BTRFS_KEY_FMT ", tree mod log rewind is inconsistent", + ref->root_id, BTRFS_KEY_FMT_VALUE(&prev_key), + BTRFS_KEY_FMT_VALUE(&key)); + ret = -EAGAIN; + break; + } + prev_key = key; + have_prev_key = true; + /* * We are searching for normal backref but bytenr of this leaf * matches shared data backref, OR diff --git a/fs/btrfs/fiemap.c b/fs/btrfs/fiemap.c index 7a2a971800993..31f3d56631101 100644 --- a/fs/btrfs/fiemap.c +++ b/fs/btrfs/fiemap.c @@ -428,6 +428,31 @@ static int fiemap_search_slot(struct btrfs_inode *inode, struct btrfs_path *path return 0; } +/* + * Check if a data extent is shared, for the purpose of setting the + * FIEMAP_EXTENT_SHARED flag. + * + * A backref walk can transiently fail with -EAGAIN when the tree mod log + * rewound view of a subvolume tree becomes inconsistent under concurrent + * modifications (see add_all_parents()). We cannot retry with a fresh tree + * mod log baseline from here, and the SHARED flag is best effort by design, + * so conservatively report the extent as shared: while the extent's trees + * are being concurrently modified, any answer may be stale by the time user + * space sees it anyway. + */ +static int fiemap_is_data_extent_shared(struct btrfs_inode *inode, u64 bytenr, + u64 extent_gen, + struct btrfs_backref_share_check_ctx *ctx) +{ + int ret; + + ret = btrfs_is_data_extent_shared(inode, bytenr, extent_gen, ctx); + if (ret == -EAGAIN) + ret = 1; + + return ret; +} + /* * Process a range which is a hole or a prealloc extent in the inode's subvolume * btree. If @disk_bytenr is 0, we are dealing with a hole, otherwise a prealloc @@ -483,10 +508,10 @@ static int fiemap_process_hole(struct btrfs_inode *inode, if (prealloc_len > 0) { if (!checked_extent_shared && fieinfo->fi_extents_max) { - ret = btrfs_is_data_extent_shared(inode, - disk_bytenr, - extent_gen, - backref_ctx); + ret = fiemap_is_data_extent_shared(inode, + disk_bytenr, + extent_gen, + backref_ctx); if (ret < 0) return ret; else if (ret > 0) @@ -532,10 +557,10 @@ static int fiemap_process_hole(struct btrfs_inode *inode, } if (!checked_extent_shared && fieinfo->fi_extents_max) { - ret = btrfs_is_data_extent_shared(inode, - disk_bytenr, - extent_gen, - backref_ctx); + ret = fiemap_is_data_extent_shared(inode, + disk_bytenr, + extent_gen, + backref_ctx); if (ret < 0) return ret; else if (ret > 0) @@ -768,10 +793,10 @@ restart: } else { /* We have a regular extent. */ if (fieinfo->fi_extents_max) { - ret = btrfs_is_data_extent_shared(inode, - disk_bytenr, - extent_gen, - backref_ctx); + ret = fiemap_is_data_extent_shared(inode, + disk_bytenr, + extent_gen, + backref_ctx); if (ret < 0) goto out_unlock; else if (ret > 0) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index f4b68205f621e..4fe2f5cc33ad0 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -10436,7 +10436,20 @@ static int btrfs_swap_activate(struct swap_info_struct *sis, struct file *file, ret = btrfs_is_data_extent_shared(BTRFS_I(inode), disk_bytenr, extent_gen, backref_ctx); - if (ret < 0) { + if (ret == -EAGAIN) { + /* + * A transient backref walk failure (see + * add_all_parents()) means the swap file's extent + * sharing was being changed in the same transaction as + * this swapon(), so reject it. The error is changed to + * EINVAL to avoid introducing a new error code to the + * cross-filesystem swap_activate interface. + */ + btrfs_warn(fs_info, +"cannot activate swapfile while its extent sharing is being modified, try again"); + ret = -EINVAL; + goto out; + } else if (ret < 0) { goto out; } else if (ret > 0) { btrfs_warn(fs_info,