Better transaction abort reports

PATCH New David Sterba 1 participant 4 messages Apr 24, 2015 Btrfs file system

lore

Share & tools

lore

David Sterba

author

2015-04-24 17:11 UTC #0 lore raw

cover Better transaction abort reports

Hi,

patch #1 moves the WARN at transaction abort time to the callsite (via macro). That way we get the exact location of the error and not the common location. This is supposed to help debugging and screening report, but the change comes at some cost and increases the resulting asm code.

I vote for better error reports, the .text grows all the time anyway. Patch #2 adds some compiler hints so the error blocks are likely to be put out of the hot paths.

Available in

git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git dev/abort-onsite

David Sterba (3):
btrfs: report exact callsite where transaction abort occurs
btrfs: add 'cold' compiler annotations to all error handling functions
btrfs: fix warnings after changes in btrfs_abort_transaction

 fs/btrfs/ctree.h   | 16 +++++++++++++---
 fs/btrfs/ioctl.c   |  2 +-
 fs/btrfs/super.c   | 11 +++--------
 fs/btrfs/volumes.c |  6 +++---
 4 files changed, 20 insertions(+), 15 deletions(-)

Signature · 1 line

2.1.3

David Sterba

author

2015-04-24 17:11 UTC #1 lore raw

PATCH 1/3 btrfs: report exact callsite where transaction abort occurs

WARN is called from a single location and all bugreports say that's in super.c __btrfs_abort_transaction. This is slightly confusing as we'd rather want to know the exact callsite. Whereas this information is printed in the syslog below the stacktrace, this requires further look and we usually see only the headline from WARNING.

Moving the WARN into the macro has to inline some code and increases code by a few kilobytes:

text data bss dec hex filename

835481   20305   14120  869906   d4612 btrfs.ko.before
842883   20305   14120  877308   d62fc btrfs.ko.after

The delta is +7k (130+ calls), measured on 3.19 x86_64, distro config. The increase is not small and could lead to worse icache use. The code is on error/exit paths that can be recognized by compiler as cold and moved out of the way so the impact is speculated to be low, if measurable at all.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h | 12 +++++++++---
 fs/btrfs/super.c |  8 --------
 2 files changed, 9 insertions(+), 11 deletions(-)

fs/btrfs/ctree.h +9 -3

index f9c89cae39ee..b0f9ba4b47c8 100644
@@ -4072,11 +4072,17 @@ static inline int __btrfs_fs_incompat(struct btrfs_fs_info *fs_info, u64 flag)
  * Call btrfs_abort_transaction as early as possible when an error condition is
  * detected, that way the exact line number is reported.
  */
-
 #define btrfs_abort_transaction(trans, root, errno)		\
 do {								\
-	__btrfs_abort_transaction(trans, root, __func__,	\
-				  __LINE__, errno);		\
+	/* Report first abort since mount */			\
+	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,	\
+			&((root)->fs_info->fs_state))) {	\
+		WARN(1, KERN_DEBUG				\
+		"BTRFS: Transaction aborted (error %d)\n",	\
+		(errno));					\
+	}							\
+	__btrfs_abort_transaction((trans), (root), __func__,	\
+				  __LINE__, (errno));		\
 } while (0)
 
 #define btrfs_std_error(fs_info, errno)				\

fs/btrfs/super.c +0 -8

index 05fef198ff94..3c6033768ad3 100644
@@ -251,14 +251,6 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root, const char *function,
 			       unsigned int line, int errno)
 {
-	/*
-	 * Report first abort since mount
-	 */
-	if (!test_and_set_bit(BTRFS_FS_STATE_TRANS_ABORTED,
-				&root->fs_info->fs_state)) {
-		WARN(1, KERN_DEBUG "BTRFS: Transaction aborted (error %d)\n",
-				errno);
-	}
 	trans->aborted = errno;
 	/* Nothing used. The other threads that have joined this
 	 * transaction may be able to continue. */

Signature · 1 line

2.1.3

David Sterba

author

2015-04-24 17:11 UTC #2 lore raw

PATCH 2/3 btrfs: add 'cold' compiler annotations to all error handling functions

In reply to David Sterba

The annotated functios will be placed into .text.unlikely section. The annotation also hints compiler to move the code out of the hot paths, and may implicitly mark if-statement leading to that block as unlikely.

This is a heuristic, the impact on the generated code is not significant.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ctree.h | 4 ++++
 fs/btrfs/super.c | 3 +++
 2 files changed, 7 insertions(+)

fs/btrfs/ctree.h +4 -0

index b0f9ba4b47c8..3a303451f010 100644
@@ -4011,6 +4011,7 @@ void btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 
 #ifdef CONFIG_BTRFS_ASSERT
 
+__cold
 static inline void assfail(char *expr, char *file, int line)
 {
 	pr_err("BTRFS: assertion failed: %s, file: %s, line: %d",

@@ -4026,10 +4027,12 @@ static inline void assfail(char *expr, char *file, int line)
 
 #define btrfs_assert()
 __printf(5, 6)
+__cold
 void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
 		     unsigned int line, int errno, const char *fmt, ...);
 
 
+__cold
 void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root, const char *function,
 			       unsigned int line, int errno);

@@ -4099,6 +4102,7 @@ do {								\
 } while (0)
 
 __printf(5, 6)
+__cold
 void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
 		   unsigned int line, int errno, const char *fmt, ...);

fs/btrfs/super.c +3 -0

index 3c6033768ad3..51a36bf80b2e 100644
@@ -135,6 +135,7 @@ static void btrfs_handle_error(struct btrfs_fs_info *fs_info)
  * __btrfs_std_error decodes expected errors from the caller and
  * invokes the approciate error response.
  */
+__cold
 void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
 		       unsigned int line, int errno, const char *fmt, ...)
 {

@@ -247,6 +248,7 @@ void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
  * We'll complete the cleanup in btrfs_end_transaction and
  * btrfs_commit_transaction.
  */
+__cold
 void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
 			       struct btrfs_root *root, const char *function,
 			       unsigned int line, int errno)

@@ -273,6 +275,7 @@ void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
  * __btrfs_panic decodes unexpected, fatal errors from the caller,
  * issues an alert, and either panics or BUGs, depending on mount options.
  */
+__cold
 void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
 		   unsigned int line, int errno, const char *fmt, ...)
 {

Signature · 1 line

2.1.3

David Sterba

author

2015-04-24 17:12 UTC #3 lore raw

PATCH 3/3 btrfs: fix warnings after changes in btrfs_abort_transaction

In reply to David Sterba

fs/btrfs/volumes.c: In function ‘btrfs_create_uuid_tree’:
fs/btrfs/volumes.c:3909:3: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘long int’ [-Wformat=]
btrfs_abort_transaction(trans, tree_root,
^
CC [M] fs/btrfs/ioctl.o
fs/btrfs/ioctl.c: In function ‘create_subvol’:
fs/btrfs/ioctl.c:549:3: warning: format ‘%d’ expects argument of type ‘int’, but argument 4 has type ‘long int’ [-Wformat=]
btrfs_abort_transaction(trans, root, PTR_ERR(new_root));

PTR_ERR returns long, but we're really using 'int' for the error codes everywhere so just set and use the local variable.

Signed-off-by David Sterba <[email protected]>

---
 fs/btrfs/ioctl.c   | 2 +-
 fs/btrfs/volumes.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

fs/btrfs/ioctl.c +1 -1

index 74609b931ba5..75bb2f011f1b 100644
@@ -546,8 +546,8 @@ static noinline int create_subvol(struct inode *dir,
 	key.offset = (u64)-1;
 	new_root = btrfs_read_fs_root_no_name(root->fs_info, &key);
 	if (IS_ERR(new_root)) {
-		btrfs_abort_transaction(trans, root, PTR_ERR(new_root));
 		ret = PTR_ERR(new_root);
+		btrfs_abort_transaction(trans, root, ret);
 		goto fail;
 	}

fs/btrfs/volumes.c +3 -3

index 8222f6f74147..029ba2ab5c4b 100644
@@ -3906,9 +3906,9 @@ int btrfs_create_uuid_tree(struct btrfs_fs_info *fs_info)
 	uuid_root = btrfs_create_tree(trans, fs_info,
 				      BTRFS_UUID_TREE_OBJECTID);
 	if (IS_ERR(uuid_root)) {
-		btrfs_abort_transaction(trans, tree_root,
-					PTR_ERR(uuid_root));
-		return PTR_ERR(uuid_root);
+		ret = PTR_ERR(uuid_root);
+		btrfs_abort_transaction(trans, tree_root, ret);
+		return ret;
 	}
 
 	fs_info->uuid_root = uuid_root;

Signature · 1 line

2.1.3