[PATCH 0/4] btrfs: output more info for -ENOSPC caused transaction abort and other enhancement

[PATCH 0/4] btrfs: output more info for -ENOSPC caused transaction abort and other enhancement

[Qu Wenruo]

We have some internal reports of transaction abort with -ENOSPC.

Unfortunately it's really hard to debug, since we really got nothing
other than the error message for most cases.

Also we have helpers like __btrfs_dump_space_info(), it needs enospc_debug
mount option which only makes sense if the user can reproduce the bug
and retry with that mount option.

Considering ENOSPC should not happen for critical paths which failure
means transaction abort, we should dump all space info for debug purpose
if the transaction arbot is caused by -ENOSPC.

And since we're here, also enhance the output of
__btrfs_dump_space_info().

Now it should be more human readable, the exmaple would look like this:

 BTRFS info (device dm-1: state A): dumpping space info:
 BTRFS info (device dm-1: state A): space_info DATA has 1175552 free, is not full
 BTRFS info (device dm-1: state A):   total:         8388608
 BTRFS info (device dm-1: state A):   used:          7163904
 BTRFS info (device dm-1: state A):   pinned:        49152
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       0
 BTRFS info (device dm-1: state A):   read_only:     0
 BTRFS info (device dm-1: state A): space_info META has 263798784 free, is not full
 BTRFS info (device dm-1: state A):   total:         268435456
 BTRFS info (device dm-1: state A):   used:          180224
 BTRFS info (device dm-1: state A):   pinned:        196608
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       4194304
 BTRFS info (device dm-1: state A):   read_only:     65536
 BTRFS info (device dm-1: state A): space_info SYS has 8372224 free, is not full
 BTRFS info (device dm-1: state A):   total:         8388608
 BTRFS info (device dm-1: state A):   used:          16384
 BTRFS info (device dm-1: state A):   pinned:        0
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       0
 BTRFS info (device dm-1: state A):   read_only:     0
 BTRFS info (device dm-1: state A): dumpping metadata reservation:
 BTRFS info (device dm-1: state A):   global:          (3670016/3670016)
 BTRFS info (device dm-1: state A):   trans:           (0/0)
 BTRFS info (device dm-1: state A):   chuunk:          (0/0)
 BTRFS info (device dm-1: state A):   delayed_inode:   (0/0)
 BTRFS info (device dm-1: state A):   delayed_refs:    (524288/524288)
 BTRFS: error (device dm-1: state A) in cleanup_transaction:1971: errno=-28 No space left
 BTRFS info (device dm-1: state EA): forced readonly

---

Would this be a candidate for stable kernels?
I believe there will be a lot of conflicts, but I think the extra debug
info would be worthy the effort.

Furthermore, any recommendation on extra info needed would be
appreciated, as I'm not 100% sure if the space info dumps would be
enough.
E.g. for metadata over-commit cases, the META space info would not make
much sense.

Qu Wenruo (4):
  btrfs: output human readable space info flag
  btrfs: make __btrfs_dump_space_info() output better formatted
  btrfs: make DUMP_BLOCK_RSV() to have better output
  btrfs: dump all space infos if we abort transaction due to ENOSPC

 fs/btrfs/ctree.h      |  6 ++--
 fs/btrfs/space-info.c | 82 ++++++++++++++++++++++++++++++++-----------
 fs/btrfs/space-info.h |  2 ++
 fs/btrfs/super.c      |  4 ++-
 4 files changed, 71 insertions(+), 23 deletions(-)

-- 
2.37.0

[PATCH 1/4] btrfs: output human readable space info flag

[Qu Wenruo]

For btrfs_space_info, its flags has only 4 possible values:
- BTRFS_BLOCK_GROUP_SYSTEM
- BTRFS_BLOCK_GROUP_METADATA | BTRFS_BLOCK_GROUP_DATA
- BTRFS_BLOCK_GROUP_METADATA
- BTRFS_BLOCK_GROUP_DATA

Thus do debuggers a favor by output a human readable flags in
__btrfs_dump_space_info().

Now the summary line of __btrfs_dump_space_info() looks like:

 BTRFS info (device dm-1: state A): space_info META has 251494400 free, is not full

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/space-info.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 62d25112310d..36b466525318 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -450,14 +450,29 @@ do {									\
 	spin_unlock(&__rsv->lock);					\
 } while (0)
 
+static const char *space_info_flag_to_str(struct btrfs_space_info *space_info)
+{
+	if (space_info->flags == BTRFS_BLOCK_GROUP_SYSTEM)
+		return "SYS";
+
+	/* Handle mixed data+metadata first. */
+	if (space_info->flags == (BTRFS_BLOCK_GROUP_METADATA |
+				  BTRFS_BLOCK_GROUP_DATA))
+		return "DATA+META";
+	if (space_info->flags == BTRFS_BLOCK_GROUP_DATA)
+		return "DATA";
+	return "META";
+}
+
 static void __btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 				    struct btrfs_space_info *info)
 {
+	const char *flag_str = space_info_flag_to_str(info);
 	lockdep_assert_held(&info->lock);
 
 	/* The free space could be negative in case of overcommit */
-	btrfs_info(fs_info, "space_info %llu has %lld free, is %sfull",
-		   info->flags,
+	btrfs_info(fs_info, "space_info %s has %lld free, is %sfull",
+		   flag_str,
 		   (s64)(info->total_bytes - btrfs_space_info_used(info, true)),
 		   info->full ? "" : "not ");
 	btrfs_info(fs_info,
-- 
2.37.0

[PATCH 2/4] btrfs: make __btrfs_dump_space_info() output better formatted

[Qu Wenruo]

The format change includes:

- Output each bytes_* in a separate line

- All bytes_* output starts at the same vertical position
  Do human a favor reading the numbers

- Skip zone specific numbers if zone is not enabled

Now one example of __btrfs_dump_space_info() looks like this for its
bytes_* members.

 BTRFS info (device dm-1: state A): space_info META has 251494400 free, is not full
 BTRFS info (device dm-1: state A):   total:         268435456
 BTRFS info (device dm-1: state A):   used:          376832
 BTRFS info (device dm-1: state A):   pinned:        229376
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       16269312
 BTRFS info (device dm-1: state A):   read_only:     65536

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/space-info.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 36b466525318..623fa0488545 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -475,11 +475,15 @@ static void __btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 		   flag_str,
 		   (s64)(info->total_bytes - btrfs_space_info_used(info, true)),
 		   info->full ? "" : "not ");
-	btrfs_info(fs_info,
-		"space_info total=%llu, used=%llu, pinned=%llu, reserved=%llu, may_use=%llu, readonly=%llu zone_unusable=%llu",
-		info->total_bytes, info->bytes_used, info->bytes_pinned,
-		info->bytes_reserved, info->bytes_may_use,
-		info->bytes_readonly, info->bytes_zone_unusable);
+	btrfs_info(fs_info, "  total:         %llu", info->total_bytes);
+	btrfs_info(fs_info, "  used:          %llu", info->bytes_used);
+	btrfs_info(fs_info, "  pinned:        %llu", info->bytes_pinned);
+	btrfs_info(fs_info, "  reserved:      %llu", info->bytes_reserved);
+	btrfs_info(fs_info, "  may_use:       %llu", info->bytes_may_use);
+	btrfs_info(fs_info, "  read_only:     %llu", info->bytes_readonly);
+	if (btrfs_is_zoned(fs_info))
+		btrfs_info(fs_info,
+			    "  zone_unusable: %llu", info->bytes_zone_unusable);
 
 	DUMP_BLOCK_RSV(fs_info, global_block_rsv);
 	DUMP_BLOCK_RSV(fs_info, trans_block_rsv);
-- 
2.37.0

[PATCH 3/4] btrfs: make DUMP_BLOCK_RSV() to have better output

[Qu Wenruo]

This involves the following change:

- Remove "_block_rsv" suffix

- Better alignment for the numbers

- Better naming distinguish for delayed refs and delayed inode

- Skip "size" and "reserved" output
  Just output the numbers directly

Before:

  BTRFS info (device dm-1: state A): global_block_rsv: size 3670016 reserved 3653632
  BTRFS info (device dm-1: state A): trans_block_rsv: size 0 reserved 0
  BTRFS info (device dm-1: state A): chunk_block_rsv: size 0 reserved 0
  BTRFS info (device dm-1: state A): delayed_block_rsv: size 0 reserved 0
  BTRFS info (device dm-1: state A): delayed_refs_rsv: size 524288 reserved 360448

After:

 BTRFS info (device dm-1: state A): global:          (3670016/3670016)
 BTRFS info (device dm-1: state A): trans:           (0/0)
 BTRFS info (device dm-1: state A): chunk:           (0/0)
 BTRFS info (device dm-1: state A): delayed_inode:   (0/0)
 BTRFS info (device dm-1: state A): delayed_refs:    (524288/524288)

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/space-info.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 623fa0488545..6bbf95e8e4f7 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -441,12 +441,12 @@ void btrfs_try_granting_tickets(struct btrfs_fs_info *fs_info,
 	}
 }
 
-#define DUMP_BLOCK_RSV(fs_info, rsv_name)				\
+#define DUMP_BLOCK_RSV(fs_info, member, name)				\
 do {									\
-	struct btrfs_block_rsv *__rsv = &(fs_info)->rsv_name;		\
+	struct btrfs_block_rsv *__rsv = &(fs_info)->member;		\
 	spin_lock(&__rsv->lock);					\
-	btrfs_info(fs_info, #rsv_name ": size %llu reserved %llu",	\
-		   __rsv->size, __rsv->reserved);			\
+	btrfs_info(fs_info, "%-16s (%llu/%llu)",			\
+		   name, __rsv->reserved, __rsv->size);			\
 	spin_unlock(&__rsv->lock);					\
 } while (0)
 
@@ -485,12 +485,11 @@ static void __btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 		btrfs_info(fs_info,
 			    "  zone_unusable: %llu", info->bytes_zone_unusable);
 
-	DUMP_BLOCK_RSV(fs_info, global_block_rsv);
-	DUMP_BLOCK_RSV(fs_info, trans_block_rsv);
-	DUMP_BLOCK_RSV(fs_info, chunk_block_rsv);
-	DUMP_BLOCK_RSV(fs_info, delayed_block_rsv);
-	DUMP_BLOCK_RSV(fs_info, delayed_refs_rsv);
-
+	DUMP_BLOCK_RSV(fs_info, global_block_rsv, "global:");
+	DUMP_BLOCK_RSV(fs_info, trans_block_rsv, "trans:");
+	DUMP_BLOCK_RSV(fs_info, chunk_block_rsv, "chunk:");
+	DUMP_BLOCK_RSV(fs_info, delayed_block_rsv, "delayed_inode:");
+	DUMP_BLOCK_RSV(fs_info, delayed_refs_rsv, "delayed_refs:");
 }
 
 void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
-- 
2.37.0

[PATCH 4/4] btrfs: dump all space infos if we abort transaction due to ENOSPC

[Qu Wenruo]

We have hit some transaction abort due to -ENOSPC internally.

Normally we should always reserve enough space for metadata for every
transaction, thus hitting -ENOSPC should really indicate some cases we
didn't expect.

But unfrotunately current error reporting will only give a kernel
wanring and backtrace, not really helpful to debug what's causing the
problem.

And debug_enospc can only help when user can reproduce the problem, but
under most cases, such transaction abort by -ENOSPC is really hard to
reproduce.

So this patch will dump all space infos (data, metadata, system) when we
abort the first transaction with -ENOSPC.

This should at least provide some clue to us.

The example of a dump would look like this:

 ------------[ cut here ]------------
 <skip stack dump>
 ---[ end trace 0000000000000000 ]---
 BTRFS info (device dm-1: state A): dumpping space info:
 BTRFS info (device dm-1: state A): space_info DATA has 1175552 free, is not full
 BTRFS info (device dm-1: state A):   total:         8388608
 BTRFS info (device dm-1: state A):   used:          7163904
 BTRFS info (device dm-1: state A):   pinned:        49152
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       0
 BTRFS info (device dm-1: state A):   read_only:     0
 BTRFS info (device dm-1: state A): space_info META has 263798784 free, is not full
 BTRFS info (device dm-1: state A):   total:         268435456
 BTRFS info (device dm-1: state A):   used:          180224
 BTRFS info (device dm-1: state A):   pinned:        196608
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       4194304
 BTRFS info (device dm-1: state A):   read_only:     65536
 BTRFS info (device dm-1: state A): space_info SYS has 8372224 free, is not full
 BTRFS info (device dm-1: state A):   total:         8388608
 BTRFS info (device dm-1: state A):   used:          16384
 BTRFS info (device dm-1: state A):   pinned:        0
 BTRFS info (device dm-1: state A):   reserved:      0
 BTRFS info (device dm-1: state A):   may_use:       0
 BTRFS info (device dm-1: state A):   read_only:     0
 BTRFS info (device dm-1: state A): dumpping metadata reservation:
 BTRFS info (device dm-1: state A):   global:          (3670016/3670016)
 BTRFS info (device dm-1: state A):   trans:           (0/0)
 BTRFS info (device dm-1: state A):   chuuk:           (0/0)
 BTRFS info (device dm-1: state A):   delayed_inode:   (0/0)
 BTRFS info (device dm-1: state A):   delayed_refs:    (524288/524288)
 BTRFS: error (device dm-1: state A) in cleanup_transaction:1971: errno=-28 No space left
 BTRFS info (device dm-1: state EA): forced readonly

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.h      |  6 ++++--
 fs/btrfs/space-info.c | 46 ++++++++++++++++++++++++++++++++-----------
 fs/btrfs/space-info.h |  2 ++
 fs/btrfs/super.c      |  4 +++-
 4 files changed, 44 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 4e2569f84aab..3d6fd7f6b339 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3739,7 +3739,7 @@ const char * __attribute_const__ btrfs_decode_error(int errno);
 __cold
 void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			       const char *function,
-			       unsigned int line, int errno);
+			       unsigned int line, int errno, bool first_hit);
 
 /*
  * Call btrfs_abort_transaction as early as possible when an error condition is
@@ -3747,9 +3747,11 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
  */
 #define btrfs_abort_transaction(trans, errno)		\
 do {								\
+	bool first = false;					\
 	/* Report first abort since mount */			\
 	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
 			&((trans)->fs_info->fs_state))) {	\
+		first = true;					\
 		if ((errno) != -EIO && (errno) != -EROFS) {		\
 			WARN(1, KERN_DEBUG				\
 			"BTRFS: Transaction aborted (error %d)\n",	\
@@ -3761,7 +3763,7 @@ do {								\
 		}						\
 	}							\
 	__btrfs_abort_transaction((trans), __func__,		\
-				  __LINE__, (errno));		\
+				  __LINE__, (errno), first);	\
 } while (0)
 
 #ifdef CONFIG_PRINTK_INDEX
diff --git a/fs/btrfs/space-info.c b/fs/btrfs/space-info.c
index 6bbf95e8e4f7..6a62cc8dc31a 100644
--- a/fs/btrfs/space-info.c
+++ b/fs/btrfs/space-info.c
@@ -441,12 +441,12 @@ void btrfs_try_granting_tickets(struct btrfs_fs_info *fs_info,
 	}
 }
 
-#define DUMP_BLOCK_RSV(fs_info, member, name)				\
+#define DUMP_BLOCK_RSV(fs_info, member, name, prefix)			\
 do {									\
 	struct btrfs_block_rsv *__rsv = &(fs_info)->member;		\
 	spin_lock(&__rsv->lock);					\
-	btrfs_info(fs_info, "%-16s (%llu/%llu)",			\
-		   name, __rsv->reserved, __rsv->size);			\
+	btrfs_info(fs_info, "%s%-16s (%llu/%llu)",			\
+		   prefix, name, __rsv->reserved, __rsv->size);		\
 	spin_unlock(&__rsv->lock);					\
 } while (0)
 
@@ -465,7 +465,8 @@ static const char *space_info_flag_to_str(struct btrfs_space_info *space_info)
 }
 
 static void __btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
-				    struct btrfs_space_info *info)
+				    struct btrfs_space_info *info,
+				    bool dump_block_rsvs)
 {
 	const char *flag_str = space_info_flag_to_str(info);
 	lockdep_assert_held(&info->lock);
@@ -485,11 +486,14 @@ static void __btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 		btrfs_info(fs_info,
 			    "  zone_unusable: %llu", info->bytes_zone_unusable);
 
-	DUMP_BLOCK_RSV(fs_info, global_block_rsv, "global:");
-	DUMP_BLOCK_RSV(fs_info, trans_block_rsv, "trans:");
-	DUMP_BLOCK_RSV(fs_info, chunk_block_rsv, "chunk:");
-	DUMP_BLOCK_RSV(fs_info, delayed_block_rsv, "delayed_inode:");
-	DUMP_BLOCK_RSV(fs_info, delayed_refs_rsv, "delayed_refs:");
+	if (!dump_block_rsvs)
+		return;
+
+	DUMP_BLOCK_RSV(fs_info, global_block_rsv, "global:", "");
+	DUMP_BLOCK_RSV(fs_info, trans_block_rsv, "trans:", "");
+	DUMP_BLOCK_RSV(fs_info, chunk_block_rsv, "chunk:", "");
+	DUMP_BLOCK_RSV(fs_info, delayed_block_rsv, "delayed_inode:", "");
+	DUMP_BLOCK_RSV(fs_info, delayed_refs_rsv, "delayed_refs:", "");
 }
 
 void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
@@ -500,7 +504,7 @@ void btrfs_dump_space_info(struct btrfs_fs_info *fs_info,
 	int index = 0;
 
 	spin_lock(&info->lock);
-	__btrfs_dump_space_info(fs_info, info);
+	__btrfs_dump_space_info(fs_info, info, true);
 	spin_unlock(&info->lock);
 
 	if (!dump_block_groups)
@@ -955,7 +959,7 @@ static bool maybe_fail_all_tickets(struct btrfs_fs_info *fs_info,
 
 	if (btrfs_test_opt(fs_info, ENOSPC_DEBUG)) {
 		btrfs_info(fs_info, "cannot satisfy tickets, dumping space info");
-		__btrfs_dump_space_info(fs_info, space_info);
+		__btrfs_dump_space_info(fs_info, space_info, true);
 	}
 
 	while (!list_empty(&space_info->tickets) &&
@@ -1711,3 +1715,23 @@ int btrfs_reserve_data_bytes(struct btrfs_fs_info *fs_info, u64 bytes,
 	}
 	return ret;
 }
+
+/* Dump all the space infos when we abort a transaction due to ENOSPC. */
+__cold void btrfs_dump_fs_space_info(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_space_info *space_info;
+
+	btrfs_info(fs_info, "dumpping space info:");
+	list_for_each_entry(space_info, &fs_info->space_info, list) {
+		spin_lock(&space_info->lock);
+		__btrfs_dump_space_info(fs_info, space_info, false);
+		spin_unlock(&space_info->lock);
+	}
+	btrfs_info(fs_info, "dumpping metadata reservation:");
+
+	DUMP_BLOCK_RSV(fs_info, global_block_rsv, "global:", "  ");
+	DUMP_BLOCK_RSV(fs_info, trans_block_rsv, "trans:", "  ");
+	DUMP_BLOCK_RSV(fs_info, chunk_block_rsv, "chunk:", "  ");
+	DUMP_BLOCK_RSV(fs_info, delayed_block_rsv, "delayed_inode:", "  ");
+	DUMP_BLOCK_RSV(fs_info, delayed_refs_rsv, "delayed_refs:", "  ");
+}
diff --git a/fs/btrfs/space-info.h b/fs/btrfs/space-info.h
index e7de24a529cf..01287a7a22a4 100644
--- a/fs/btrfs/space-info.h
+++ b/fs/btrfs/space-info.h
@@ -157,4 +157,6 @@ static inline void btrfs_space_info_free_bytes_may_use(
 }
 int btrfs_reserve_data_bytes(struct btrfs_fs_info *fs_info, u64 bytes,
 			     enum btrfs_reserve_flush_enum flush);
+void btrfs_dump_fs_space_info(struct btrfs_fs_info *fs_info);
+
 #endif /* BTRFS_SPACE_INFO_H */
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4c7089b1681b..f6bc8aa00f44 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -346,12 +346,14 @@ void __cold btrfs_err_32bit_limit(struct btrfs_fs_info *fs_info)
 __cold
 void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			       const char *function,
-			       unsigned int line, int errno)
+			       unsigned int line, int errno, bool first_hit)
 {
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 
 	WRITE_ONCE(trans->aborted, errno);
 	WRITE_ONCE(trans->transaction->aborted, errno);
+	if (first_hit && errno == -ENOSPC)
+		btrfs_dump_fs_space_info(fs_info);
 	/* Wake up anybody who may be waiting on this transaction */
 	wake_up(&fs_info->transaction_wait);
 	wake_up(&fs_info->transaction_blocked_wait);
-- 
2.37.0

Re: [PATCH 3/4] btrfs: make DUMP_BLOCK_RSV() to have better output

[Johannes Thumshirn]

On 15.07.22 08:58, Qu Wenruo wrote:
> - Skip "size" and "reserved" output
>   Just output the numbers directly
> 
> Before:
> 
>   BTRFS info (device dm-1: state A): global_block_rsv: size 3670016 reserved 3653632
>   BTRFS info (device dm-1: state A): trans_block_rsv: size 0 reserved 0
>   BTRFS info (device dm-1: state A): chunk_block_rsv: size 0 reserved 0
>   BTRFS info (device dm-1: state A): delayed_block_rsv: size 0 reserved 0
>   BTRFS info (device dm-1: state A): delayed_refs_rsv: size 524288 reserved 360448
> 
> After:
> 
>  BTRFS info (device dm-1: state A): global:          (3670016/3670016)
>  BTRFS info (device dm-1: state A): trans:           (0/0)
>  BTRFS info (device dm-1: state A): chunk:           (0/0)
>  BTRFS info (device dm-1: state A): delayed_inode:   (0/0)
>  BTRFS info (device dm-1: state A): delayed_refs:    (524288/524288)

Pure personal preference, but I find it a tad bit easier to have size and 
reserved printed. So you don't have to look up the code when you encounter
the error.

Maybe:
BTRFS info (device dm-1: state A): global:          size:3670016,res:3670016

But in the end of the day it doesn't matter I guess.

Re: [PATCH 0/4] btrfs: output more info for -ENOSPC caused transaction abort and other enhancement

[Johannes Thumshirn]

Apart from the small nit in 3/4

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Generally I think this is a huge improvement for debugging

Re: [PATCH 3/4] btrfs: make DUMP_BLOCK_RSV() to have better output

[Qu Wenruo]

On 2022/7/15 16:14, Johannes Thumshirn wrote:
> On 15.07.22 08:58, Qu Wenruo wrote:
>> - Skip "size" and "reserved" output
>>    Just output the numbers directly
>>
>> Before:
>>
>>    BTRFS info (device dm-1: state A): global_block_rsv: size 3670016 reserved 3653632
>>    BTRFS info (device dm-1: state A): trans_block_rsv: size 0 reserved 0
>>    BTRFS info (device dm-1: state A): chunk_block_rsv: size 0 reserved 0
>>    BTRFS info (device dm-1: state A): delayed_block_rsv: size 0 reserved 0
>>    BTRFS info (device dm-1: state A): delayed_refs_rsv: size 524288 reserved 360448
>>
>> After:
>>
>>   BTRFS info (device dm-1: state A): global:          (3670016/3670016)
>>   BTRFS info (device dm-1: state A): trans:           (0/0)
>>   BTRFS info (device dm-1: state A): chunk:           (0/0)
>>   BTRFS info (device dm-1: state A): delayed_inode:   (0/0)
>>   BTRFS info (device dm-1: state A): delayed_refs:    (524288/524288)
>
> Pure personal preference, but I find it a tad bit easier to have size and
> reserved printed. So you don't have to look up the code when you encounter
> the error.
>
> Maybe:
> BTRFS info (device dm-1: state A): global:          size:3670016,res:3670016
>
> But in the end of the day it doesn't matter I guess.

The reason here is I want to avoid size/res affecting the number output.

But your concern is also valid.

What about a new line before everything, showing something like:
Dumping global metadata reservations (reserved/size) :

So that with that line, we know what is reserved and what is size?

Thanks,
Qu

Re: [PATCH 3/4] btrfs: make DUMP_BLOCK_RSV() to have better output

[Johannes Thumshirn]

On 15.07.22 10:17, Qu Wenruo wrote:
> 
> 
> On 2022/7/15 16:14, Johannes Thumshirn wrote:
>> On 15.07.22 08:58, Qu Wenruo wrote:
>>> - Skip "size" and "reserved" output
>>>    Just output the numbers directly
>>>
>>> Before:
>>>
>>>    BTRFS info (device dm-1: state A): global_block_rsv: size 3670016 reserved 3653632
>>>    BTRFS info (device dm-1: state A): trans_block_rsv: size 0 reserved 0
>>>    BTRFS info (device dm-1: state A): chunk_block_rsv: size 0 reserved 0
>>>    BTRFS info (device dm-1: state A): delayed_block_rsv: size 0 reserved 0
>>>    BTRFS info (device dm-1: state A): delayed_refs_rsv: size 524288 reserved 360448
>>>
>>> After:
>>>
>>>   BTRFS info (device dm-1: state A): global:          (3670016/3670016)
>>>   BTRFS info (device dm-1: state A): trans:           (0/0)
>>>   BTRFS info (device dm-1: state A): chunk:           (0/0)
>>>   BTRFS info (device dm-1: state A): delayed_inode:   (0/0)
>>>   BTRFS info (device dm-1: state A): delayed_refs:    (524288/524288)
>>
>> Pure personal preference, but I find it a tad bit easier to have size and
>> reserved printed. So you don't have to look up the code when you encounter
>> the error.
>>
>> Maybe:
>> BTRFS info (device dm-1: state A): global:          size:3670016,res:3670016
>>
>> But in the end of the day it doesn't matter I guess.
> 
> The reason here is I want to avoid size/res affecting the number output.
> 
> But your concern is also valid.
> 
> What about a new line before everything, showing something like:
> Dumping global metadata reservations (reserved/size) :
> 
> So that with that line, we know what is reserved and what is size?

Yeah that sounds good to me