On Wed, Oct 13, 2021 at 3:10 PM Nikolay Borisov [off-list ref] wrote:

On 13.10.21 г. 12:12, fdmanana@kernel.org wrote:

quoted

From: Filipe Manana <redacted>

<snip>

quoted

Reported-by: Hao Sun <redacted>
Link: https://lore.kernel.org/linux-btrfs/CACkBjsax51i4mu6C0C3vJqQN3NR_iVuucoeG3U1HXjrgzn5FFQ@mail.gmail.com/ (local)
Fixes: 79bd37120b1495 ("btrfs: rework chunk allocation to avoid exhaustion of the system chunk array")
Signed-off-by: Filipe Manana <redacted>
---
 fs/btrfs/block-group.c | 145 +++++++++++++++++++++++++----------------
 fs/btrfs/block-group.h |   2 +
 fs/btrfs/relocation.c  |   4 ++
 fs/btrfs/volumes.c     |  15 ++++-
 4 files changed, 110 insertions(+), 56 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 46fdef7bbe20..e790ea0798c7 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c

@@ -3403,25 +3403,6 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
              goto out;
      }

-     /*
-      * If this is a system chunk allocation then stop right here and do not
-      * add the chunk item to the chunk btree. This is to prevent a deadlock
-      * because this system chunk allocation can be triggered while COWing
-      * some extent buffer of the chunk btree and while holding a lock on a
-      * parent extent buffer, in which case attempting to insert the chunk
-      * item (or update the device item) would result in a deadlock on that
-      * parent extent buffer. In this case defer the chunk btree updates to
-      * the second phase of chunk allocation and keep our reservation until
-      * the second phase completes.
-      *
-      * This is a rare case and can only be triggered by the very few cases
-      * we have where we need to touch the chunk btree outside chunk allocation
-      * and chunk removal. These cases are basically adding a device, removing
-      * a device or resizing a device.
-      */
-     if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
-             return 0;
-
      ret = btrfs_chunk_alloc_add_chunk_item(trans, bg);
      /*
       * Normally we are not expected to fail with -ENOSPC here, since we have

@@ -3554,14 +3535,14 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags)
  * This has happened before and commit eafa4fd0ad0607 ("btrfs: fix exhaustion of
  * the system chunk array due to concurrent allocations") provides more details.
  *
- * For allocation of system chunks, we defer the updates and insertions into the
- * chunk btree to phase 2. This is to prevent deadlocks on extent buffers because
- * if the chunk allocation is triggered while COWing an extent buffer of the
- * chunk btree, we are holding a lock on the parent of that extent buffer and
- * doing the chunk btree updates and insertions can require locking that parent.
- * This is for the very few and rare cases where we update the chunk btree that
- * are not chunk allocation or chunk removal: adding a device, removing a device
- * or resizing a device.
+ * Allocation of system chunks does not happen through this function. A task that
+ * needs to update the chunk btree (the only btree that uses system chunks), must
+ * preallocate chunk space by calling either check_system_chunk() or
+ * btrfs_reserve_chunk_metadata() - the former is used when allocating a data or
+ * metadata chunk or when removing a chunk, while the later is used before doing
+ * a modification to the chunk btree - use cases for the later are adding,
+ * removing and resizing a device as well as relocation of a system chunk.
+ * See the comment below for more details.
  *
  * The reservation of system space, done through check_system_chunk(), as well
  * as all the updates and insertions into the chunk btree must be done while

@@ -3598,11 +3579,27 @@ int btrfs_chunk_alloc(struct btrfs_trans_handle *trans, u64 flags,
      if (trans->allocating_chunk)
              return -ENOSPC;
      /*
-      * If we are removing a chunk, don't re-enter or we would deadlock.
-      * System space reservation and system chunk allocation is done by the
-      * chunk remove operation (btrfs_remove_chunk()).
+      * Allocation of system chunks can not happen through this path, as we
+      * could end up in a deadlock if we are allocating a data or metadata
+      * chunk and there is another task modifying the chunk btree.
+      *
+      * This is because while we are holding the chunk mutex, we will attempt
+      * to add the new chunk item to the chunk btree or update an existing
+      * device item in the chunk btree, while the other task that is modifying
+      * the chunk btree is attempting to COW an extent buffer while holding a
+      * lock on it and on its parent - if the COW operation triggers a system
+      * chunk allocation, then we can deadlock because we are holding the
+      * chunk mutex and we may need to access that extent buffer or its parent
+      * in order to add the chunk item or update a device item.
+      *
+      * Tasks that want to modify the chunk tree should reserve system space
+      * before updating the chunk btree, by calling either
+      * btrfs_reserve_chunk_metadata() or check_system_chunk().
+      * It's possible that after a task reserves the space, it still ends up
+      * here - this happens in the cases described above at do_chunk_alloc().
+      * The task will have to either retry or fail.
       */
-     if (trans->removing_chunk)
+     if (flags & BTRFS_BLOCK_GROUP_SYSTEM)
              return -ENOSPC;

      space_info = btrfs_find_space_info(fs_info, flags);

@@ -3701,17 +3698,14 @@ static u64 get_profile_num_devs(struct btrfs_fs_info *fs_info, u64 type)
      return num_dev;
 }

-/*
- * Reserve space in the system space for allocating or removing a chunk
- */
-void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
+static void reserve_chunk_space(struct btrfs_trans_handle *trans,
+                             u64 bytes,
+                             u64 type)
 {
      struct btrfs_fs_info *fs_info = trans->fs_info;
      struct btrfs_space_info *info;
      u64 left;
-     u64 thresh;
      int ret = 0;
-     u64 num_devs;

      /*
       * Needed because we can end up allocating a system chunk and for an

@@ -3724,19 +3718,13 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
      left = info->total_bytes - btrfs_space_info_used(info, true);
      spin_unlock(&info->lock);

-     num_devs = get_profile_num_devs(fs_info, type);
-
-     /* num_devs device items to update and 1 chunk item to add or remove */
-     thresh = btrfs_calc_metadata_size(fs_info, num_devs) +
-             btrfs_calc_insert_metadata_size(fs_info, 1);
-
-     if (left < thresh && btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
+     if (left < bytes && btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
              btrfs_info(fs_info, "left=%llu, need=%llu, flags=%llu",
-                        left, thresh, type);
+                        left, bytes, type);
              btrfs_dump_space_info(fs_info, info, 0, 0);
      }

This can be simplified to if (btrfs_test_opt(fs_info, ENOSPC_DEBUG))
and nested inside the next if (left < bytes). I checked
and even with the extra nesting the code doesn't break the 76 char limit.

This is a bug fix only, I'm not reformatting code blocks I'm not
really changing.

quoted

-     if (left < thresh) {
+     if (left < bytes) {
              u64 flags = btrfs_system_alloc_profile(fs_info);
              struct btrfs_block_group *bg;

@@ -3745,21 +3733,20 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
               * needing it, as we might not need to COW all nodes/leafs from
               * the paths we visit in the chunk tree (they were already COWed
               * or created in the current transaction for example).
-              *
-              * Also, if our caller is allocating a system chunk, do not
-              * attempt to insert the chunk item in the chunk btree, as we
-              * could deadlock on an extent buffer since our caller may be
-              * COWing an extent buffer from the chunk btree.
               */
              bg = btrfs_create_chunk(trans, flags);
              if (IS_ERR(bg)) {
                      ret = PTR_ERR(bg);
-             } else if (!(type & BTRFS_BLOCK_GROUP_SYSTEM)) {
+             } else {

This can be turned into a simple if (!IS_ERR(bg)) {}

Same type of comment as before.

quoted

                      /*
                       * If we fail to add the chunk item here, we end up
                       * trying again at phase 2 of chunk allocation, at
                       * btrfs_create_pending_block_groups(). So ignore
-                      * any error here.
+                      * any error here. An ENOSPC here could happen, due to
+                      * the cases described at do_chunk_alloc() - the system
+                      * block group we just created was just turned into RO
+                      * mode by a scrub for example, or a running discard
+                      * temporarily removed its free space entries, etc.
                       */
                      btrfs_chunk_alloc_add_chunk_item(trans, bg);
              }

@@ -3768,12 +3755,60 @@ void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
      if (!ret) {
              ret = btrfs_block_rsv_add(fs_info->chunk_root,
                                        &fs_info->chunk_block_rsv,
-                                       thresh, BTRFS_RESERVE_NO_FLUSH);
+                                       bytes, BTRFS_RESERVE_NO_FLUSH);
              if (!ret)
-                     trans->chunk_bytes_reserved += thresh;
+                     trans->chunk_bytes_reserved += bytes;
      }

The single btrfs_block_rsv_add call and the addition of bytes to chunk_bytes_reserved
can be collapsed into the above branch. The end result looks like: https://pastebin.com/F09TjVWp

This is results in slightly shorter and more linear code => easy to read.

Same as before, I'm not reformatting or changing the style of the code
that is not being changed here for fixing a bug.

Plus it's highly subjective if that is more readable - I don't like it
more because it adds 1 more indentation level.

quoted

 }

+/*
+ * Reserve space in the system space for allocating or removing a chunk.
+ * The caller must be holding fs_info->chunk_mutex.

Better to use lockdep_assert_held.

reserve_chunk_space() does that, that's why I didn't add it here again.

Thanks.

quoted

+ */
+void check_system_chunk(struct btrfs_trans_handle *trans, u64 type)
+{
+     struct btrfs_fs_info *fs_info = trans->fs_info;
+     const u64 num_devs = get_profile_num_devs(fs_info, type);
+     u64 bytes;
+
+     /* num_devs device items to update and 1 chunk item to add or remove. */
+     bytes = btrfs_calc_metadata_size(fs_info, num_devs) +
+             btrfs_calc_insert_metadata_size(fs_info, 1);
+
+     reserve_chunk_space(trans, bytes, type);
+}
+

<snip>