On Wed, Oct 13, 2021 at 03:21:47PM +0100, Filipe Manana wrote:

quoted

quoted

@@ -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.

I tend to agree to keep the fix minimal and do unrelated cleanups if it
happens in the scope of the fix. Backporing such patches is easier but I
understand the comment that sometimes it's worth to do the collateral
cleanups. No hard rules here.

quoted

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.

I'm not sure what's the overhead of lockdep_assert_held but it could be
potentially a perf hit, where we would care even for a debugging build.
If the call chain is not spread over many functions/files I'd say it's
ok to do lockdep_assert only on the entry function and not each in the
call sub tree.