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 1c16609f620690ed19ca2c1de67003242e47db40..b71eea4d815cef9bf3f8d969d6c9a0bfa7f5afc5 100644 (file)
--- 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);