[PATCH 0/5] BTRFS: Fine-tuning for five function implementations

[PATCH 0/5] BTRFS: Fine-tuning for five function implementations

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 14:30:12 +0200

A few update suggestions were taken into account
from static source code analysis.

Markus Elfring (5):
  Use common error handling code in tree_mod_log_eb_copy()
  Use common error handling code in __btrfs_free_extent()
  Use common error handling code in btrfs_update_root()
  Use common error handling code in update_ref_path()
  Use common error handling code in btrfs_mark_extent_written()

 fs/btrfs/ctree.c       | 14 +++++-----
 fs/btrfs/extent-tree.c | 69 ++++++++++++++++++++------------------------------
 fs/btrfs/file.c        | 62 ++++++++++++++++++---------------------------
 fs/btrfs/root-tree.c   | 27 ++++++++------------
 fs/btrfs/send.c        |  8 +++---
 5 files changed, 72 insertions(+), 108 deletions(-)

-- 
2.14.0

[PATCH 1/5] btrfs: Use common error handling code in tree_mod_log_eb_copy()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 09:36:48 +0200

Add a jump target so that a bit of exception handling can be better reused
in this function.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/btrfs/ctree.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 6d49db7d86be..d29cf946ebf9 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -823,17 +823,13 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 	for (i = 0; i < nr_items; i++) {
 		tm_list_rem[i] = alloc_tree_mod_elem(src, i + src_offset,
 		    MOD_LOG_KEY_REMOVE, GFP_NOFS);
-		if (!tm_list_rem[i]) {
-			ret = -ENOMEM;
-			goto free_tms;
-		}
+		if (!tm_list_rem[i])
+			goto e_nomem;
 
 		tm_list_add[i] = alloc_tree_mod_elem(dst, i + dst_offset,
 		    MOD_LOG_KEY_ADD, GFP_NOFS);
-		if (!tm_list_add[i]) {
-			ret = -ENOMEM;
-			goto free_tms;
-		}
+		if (!tm_list_add[i])
+			goto e_nomem;
 	}
 
 	if (tree_mod_dont_log(fs_info, NULL))
@@ -854,6 +850,8 @@ tree_mod_log_eb_copy(struct btrfs_fs_info *fs_info, struct extent_buffer *dst,
 
 	return 0;
 
+e_nomem:
+	ret = -ENOMEM;
 free_tms:
 	for (i = 0; i < nr_items * 2; i++) {
 		if (tm_list[i] && !RB_EMPTY_NODE(&tm_list[i]->node))
-- 
2.14.0

[PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 10:03:00 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/btrfs/extent-tree.c | 69 ++++++++++++++++++++------------------------------
 1 file changed, 27 insertions(+), 42 deletions(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 116c5615d6c2..c6b7aca88491 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6913,10 +6913,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			ret = remove_extent_backref(trans, info, path, NULL,
 						    refs_to_drop,
 						    is_data, &last_ref);
-			if (ret) {
-				btrfs_abort_transaction(trans, ret);
-				goto out;
-			}
+			if (ret)
+				goto abort_transaction;
+
 			btrfs_release_path(path);
 			path->leave_spinning = 1;
 
@@ -6962,10 +6961,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 				if (ret > 0)
 					btrfs_print_leaf(path->nodes[0]);
 			}
-			if (ret < 0) {
-				btrfs_abort_transaction(trans, ret);
-				goto out;
-			}
+			if (ret < 0)
+				goto abort_transaction;
+
 			extent_slot = path->slots[0];
 		}
 	} else if (WARN_ON(ret == -ENOENT)) {
@@ -6974,11 +6972,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			"unable to find ref byte nr %llu parent %llu root %llu  owner %llu offset %llu",
 			bytenr, parent, root_objectid, owner_objectid,
 			owner_offset);
-		btrfs_abort_transaction(trans, ret);
-		goto out;
+		goto abort_transaction;
 	} else {
-		btrfs_abort_transaction(trans, ret);
-		goto out;
+		goto abort_transaction;
 	}
 
 	leaf = path->nodes[0];
@@ -6988,10 +6984,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 		BUG_ON(found_extent || extent_slot != path->slots[0]);
 		ret = convert_extent_item_v0(trans, info, path, owner_objectid,
 					     0);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 
 		btrfs_release_path(path);
 		path->leave_spinning = 1;
@@ -7008,10 +7002,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 				ret, bytenr);
 			btrfs_print_leaf(path->nodes[0]);
 		}
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 
 		extent_slot = path->slots[0];
 		leaf = path->nodes[0];
@@ -7035,8 +7027,7 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			  "trying to drop %d refs but we only have %Lu for bytenr %Lu",
 			  refs_to_drop, refs, bytenr);
 		ret = -EINVAL;
-		btrfs_abort_transaction(trans, ret);
-		goto out;
+		goto abort_transaction;
 	}
 	refs -= refs_to_drop;
 
@@ -7057,10 +7048,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 			ret = remove_extent_backref(trans, info, path,
 						    iref, refs_to_drop,
 						    is_data, &last_ref);
-			if (ret) {
-				btrfs_abort_transaction(trans, ret);
-				goto out;
-			}
+			if (ret)
+				goto abort_transaction;
 		}
 	} else {
 		if (found_extent) {
@@ -7078,37 +7067,33 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
 		last_ref = 1;
 		ret = btrfs_del_items(trans, extent_root, path, path->slots[0],
 				      num_to_del);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
+
 		btrfs_release_path(path);
 
 		if (is_data) {
 			ret = btrfs_del_csums(trans, info, bytenr, num_bytes);
-			if (ret) {
-				btrfs_abort_transaction(trans, ret);
-				goto out;
-			}
+			if (ret)
+				goto abort_transaction;
 		}
 
 		ret = add_to_free_space_tree(trans, info, bytenr, num_bytes);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
 
 		ret = update_block_group(trans, info, bytenr, num_bytes, 0);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
 	}
 	btrfs_release_path(path);
 
 out:
 	btrfs_free_path(path);
 	return ret;
+abort_transaction:
+	btrfs_abort_transaction(trans, ret);
+	goto out;
 }
 
 /*
-- 
2.14.0

[PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 13:10:15 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/btrfs/root-tree.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
index 5b488af6f25e..bc497ba9d9d1 100644
--- a/fs/btrfs/root-tree.c
+++ b/fs/btrfs/root-tree.c
@@ -145,10 +145,8 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 		return -ENOMEM;
 
 	ret = btrfs_search_slot(trans, root, key, path, 0, 1);
-	if (ret < 0) {
-		btrfs_abort_transaction(trans, ret);
-		goto out;
-	}
+	if (ret < 0)
+		goto abort_transaction;
 
 	if (ret != 0) {
 		btrfs_print_leaf(path->nodes[0]);
@@ -171,23 +169,17 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 		btrfs_release_path(path);
 		ret = btrfs_search_slot(trans, root, key, path,
 				-1, 1);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 
 		ret = btrfs_del_item(trans, root, path);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 		btrfs_release_path(path);
 		ret = btrfs_insert_empty_item(trans, root, path,
 				key, sizeof(*item));
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 		l = path->nodes[0];
 		slot = path->slots[0];
 		ptr = btrfs_item_ptr_offset(l, slot);
@@ -204,6 +196,9 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
 out:
 	btrfs_free_path(path);
 	return ret;
+abort_transaction:
+	btrfs_abort_transaction(trans, ret);
+	goto out;
 }
 
 int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
-- 
2.14.0

[PATCH 4/5] btrfs: Use common error handling code in update_ref_path()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 13:34:29 +0200

Add a jump target so that a bit of exception handling can be better reused
in this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/btrfs/send.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 59fb1ed6ca20..a96edc91a101 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -3697,12 +3697,12 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
 		return -ENOMEM;
 
 	ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
-	if (ret < 0) {
-		fs_path_free(new_path);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_path;
+
 	ret = fs_path_add(new_path, ref->name, ref->name_len);
 	if (ret < 0) {
+free_path:
 		fs_path_free(new_path);
 		return ret;
 	}
-- 
2.14.0

[PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 14:15:23 +0200

Add jump targets so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 fs/btrfs/file.c | 62 ++++++++++++++++++++++-----------------------------------
 1 file changed, 24 insertions(+), 38 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 74fd7756cff3..675683051cbc 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1122,25 +1122,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 
 	leaf = path->nodes[0];
 	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
-	if (key.objectid != ino ||
-	    key.type != BTRFS_EXTENT_DATA_KEY) {
-		ret = -EINVAL;
-		btrfs_abort_transaction(trans, ret);
-		goto out;
-	}
+	if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
+		goto e_inval;
+
 	fi = btrfs_item_ptr(leaf, path->slots[0],
 			    struct btrfs_file_extent_item);
-	if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC) {
-		ret = -EINVAL;
-		btrfs_abort_transaction(trans, ret);
-		goto out;
-	}
+	if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC)
+		goto e_inval;
+
 	extent_end = key.offset + btrfs_file_extent_num_bytes(leaf, fi);
-	if (key.offset > start || extent_end < end) {
-		ret = -EINVAL;
-		btrfs_abort_transaction(trans, ret);
-		goto out;
-	}
+	if (key.offset > start || extent_end < end)
+		goto e_inval;
 
 	bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
 	num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
@@ -1213,10 +1205,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 			btrfs_release_path(path);
 			goto again;
 		}
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 
 		leaf = path->nodes[0];
 		fi = btrfs_item_ptr(leaf, path->slots[0] - 1,
@@ -1237,18 +1227,15 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		ret = btrfs_inc_extent_ref(trans, fs_info, bytenr, num_bytes,
 					   0, root->root_key.objectid,
 					   ino, orig_offset);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
 
 		if (split == start) {
 			key.offset = start;
 		} else {
 			if (start != key.offset) {
 				ret = -EINVAL;
-				btrfs_abort_transaction(trans, ret);
-				goto out;
+				goto abort_transaction;
 			}
 			path->slots[0]--;
 			extent_end = end;
@@ -1271,10 +1258,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
 					0, root->root_key.objectid,
 					ino, orig_offset);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
 	}
 	other_start = 0;
 	other_end = start;
@@ -1291,10 +1276,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
 					0, root->root_key.objectid,
 					ino, orig_offset);
-		if (ret) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret)
+			goto abort_transaction;
 	}
 	if (del_nr == 0) {
 		fi = btrfs_item_ptr(leaf, path->slots[0],
@@ -1314,14 +1297,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
 		btrfs_mark_buffer_dirty(leaf);
 
 		ret = btrfs_del_items(trans, root, path, del_slot, del_nr);
-		if (ret < 0) {
-			btrfs_abort_transaction(trans, ret);
-			goto out;
-		}
+		if (ret < 0)
+			goto abort_transaction;
 	}
 out:
 	btrfs_free_path(path);
 	return 0;
+e_inval:
+	ret = -EINVAL;
+abort_transaction:
+	btrfs_abort_transaction(trans, ret);
+	goto out;
 }
 
 /*
-- 
2.14.0

Re: [PATCH 5/5] btrfs: Use common error handling code in btrfs_mark_extent_written()

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 4419 bytes --]

On 8/21/17 8:42 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 14:15:23 +0200
> 
> Add jump targets so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.

btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.

-Jeff

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/btrfs/file.c | 62 ++++++++++++++++++++++-----------------------------------
>  1 file changed, 24 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 74fd7756cff3..675683051cbc 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1122,25 +1122,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  
>  	leaf = path->nodes[0];
>  	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
> -	if (key.objectid != ino ||
> -	    key.type != BTRFS_EXTENT_DATA_KEY) {
> -		ret = -EINVAL;
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> -	}
> +	if (key.objectid != ino || key.type != BTRFS_EXTENT_DATA_KEY)
> +		goto e_inval;
> +
>  	fi = btrfs_item_ptr(leaf, path->slots[0],
>  			    struct btrfs_file_extent_item);
> -	if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC) {
> -		ret = -EINVAL;
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> -	}
> +	if (btrfs_file_extent_type(leaf, fi) != BTRFS_FILE_EXTENT_PREALLOC)
> +		goto e_inval;
> +
>  	extent_end = key.offset + btrfs_file_extent_num_bytes(leaf, fi);
> -	if (key.offset > start || extent_end < end) {
> -		ret = -EINVAL;
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> -	}
> +	if (key.offset > start || extent_end < end)
> +		goto e_inval;
>  
>  	bytenr = btrfs_file_extent_disk_bytenr(leaf, fi);
>  	num_bytes = btrfs_file_extent_disk_num_bytes(leaf, fi);
> @@ -1213,10 +1205,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  			btrfs_release_path(path);
>  			goto again;
>  		}
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  
>  		leaf = path->nodes[0];
>  		fi = btrfs_item_ptr(leaf, path->slots[0] - 1,
> @@ -1237,18 +1227,15 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  		ret = btrfs_inc_extent_ref(trans, fs_info, bytenr, num_bytes,
>  					   0, root->root_key.objectid,
>  					   ino, orig_offset);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
>  
>  		if (split == start) {
>  			key.offset = start;
>  		} else {
>  			if (start != key.offset) {
>  				ret = -EINVAL;
> -				btrfs_abort_transaction(trans, ret);
> -				goto out;
> +				goto abort_transaction;
>  			}
>  			path->slots[0]--;
>  			extent_end = end;
> @@ -1271,10 +1258,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  		ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
>  					0, root->root_key.objectid,
>  					ino, orig_offset);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
>  	}
>  	other_start = 0;
>  	other_end = start;
> @@ -1291,10 +1276,8 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  		ret = btrfs_free_extent(trans, fs_info, bytenr, num_bytes,
>  					0, root->root_key.objectid,
>  					ino, orig_offset);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
>  	}
>  	if (del_nr == 0) {
>  		fi = btrfs_item_ptr(leaf, path->slots[0],
> @@ -1314,14 +1297,17 @@ int btrfs_mark_extent_written(struct btrfs_trans_handle *trans,
>  		btrfs_mark_buffer_dirty(leaf);
>  
>  		ret = btrfs_del_items(trans, root, path, del_slot, del_nr);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  	}
>  out:
>  	btrfs_free_path(path);
>  	return 0;
> +e_inval:
> +	ret = -EINVAL;
> +abort_transaction:
> +	btrfs_abort_transaction(trans, ret);
> +	goto out;
>  }
>  
>  /*
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 4979 bytes --]

On 8/21/17 8:38 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 10:03:00 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.

btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.

-Jeff


> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/btrfs/extent-tree.c | 69 ++++++++++++++++++++------------------------------
>  1 file changed, 27 insertions(+), 42 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 116c5615d6c2..c6b7aca88491 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6913,10 +6913,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  			ret = remove_extent_backref(trans, info, path, NULL,
>  						    refs_to_drop,
>  						    is_data, &last_ref);
> -			if (ret) {
> -				btrfs_abort_transaction(trans, ret);
> -				goto out;
> -			}
> +			if (ret)
> +				goto abort_transaction;
> +
>  			btrfs_release_path(path);
>  			path->leave_spinning = 1;
>  
> @@ -6962,10 +6961,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  				if (ret > 0)
>  					btrfs_print_leaf(path->nodes[0]);
>  			}
> -			if (ret < 0) {
> -				btrfs_abort_transaction(trans, ret);
> -				goto out;
> -			}
> +			if (ret < 0)
> +				goto abort_transaction;
> +
>  			extent_slot = path->slots[0];
>  		}
>  	} else if (WARN_ON(ret == -ENOENT)) {
> @@ -6974,11 +6972,9 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  			"unable to find ref byte nr %llu parent %llu root %llu  owner %llu offset %llu",
>  			bytenr, parent, root_objectid, owner_objectid,
>  			owner_offset);
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> +		goto abort_transaction;
>  	} else {
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> +		goto abort_transaction;
>  	}
>  
>  	leaf = path->nodes[0];
> @@ -6988,10 +6984,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  		BUG_ON(found_extent || extent_slot != path->slots[0]);
>  		ret = convert_extent_item_v0(trans, info, path, owner_objectid,
>  					     0);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  
>  		btrfs_release_path(path);
>  		path->leave_spinning = 1;
> @@ -7008,10 +7002,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  				ret, bytenr);
>  			btrfs_print_leaf(path->nodes[0]);
>  		}
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  
>  		extent_slot = path->slots[0];
>  		leaf = path->nodes[0];
> @@ -7035,8 +7027,7 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  			  "trying to drop %d refs but we only have %Lu for bytenr %Lu",
>  			  refs_to_drop, refs, bytenr);
>  		ret = -EINVAL;
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> +		goto abort_transaction;
>  	}
>  	refs -= refs_to_drop;
>  
> @@ -7057,10 +7048,8 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  			ret = remove_extent_backref(trans, info, path,
>  						    iref, refs_to_drop,
>  						    is_data, &last_ref);
> -			if (ret) {
> -				btrfs_abort_transaction(trans, ret);
> -				goto out;
> -			}
> +			if (ret)
> +				goto abort_transaction;
>  		}
>  	} else {
>  		if (found_extent) {
> @@ -7078,37 +7067,33 @@ static int __btrfs_free_extent(struct btrfs_trans_handle *trans,
>  		last_ref = 1;
>  		ret = btrfs_del_items(trans, extent_root, path, path->slots[0],
>  				      num_to_del);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
> +
>  		btrfs_release_path(path);
>  
>  		if (is_data) {
>  			ret = btrfs_del_csums(trans, info, bytenr, num_bytes);
> -			if (ret) {
> -				btrfs_abort_transaction(trans, ret);
> -				goto out;
> -			}
> +			if (ret)
> +				goto abort_transaction;
>  		}
>  
>  		ret = add_to_free_space_tree(trans, info, bytenr, num_bytes);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
>  
>  		ret = update_block_group(trans, info, bytenr, num_bytes, 0);
> -		if (ret) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret)
> +			goto abort_transaction;
>  	}
>  	btrfs_release_path(path);
>  
>  out:
>  	btrfs_free_path(path);
>  	return ret;
> +abort_transaction:
> +	btrfs_abort_transaction(trans, ret);
> +	goto out;
>  }
>  
>  /*
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] btrfs: Use common error handling code in update_ref_path()

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 1259 bytes --]

On 8/21/17 8:41 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 13:34:29 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> in this function.
> 
> This issue was detected by using the Coccinelle software.

Adding a jump label in the middle of a conditional for "common" error
handling makes the code more difficult to understand.

-Jeff

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/btrfs/send.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 59fb1ed6ca20..a96edc91a101 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -3697,12 +3697,12 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
>  		return -ENOMEM;
>  
>  	ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
> -	if (ret < 0) {
> -		fs_path_free(new_path);
> -		return ret;
> -	}
> +	if (ret < 0)
> +		goto free_path;
> +
>  	ret = fs_path_add(new_path, ref->name, ref->name_len);
>  	if (ret < 0) {
> +free_path:
>  		fs_path_free(new_path);
>  		return ret;
>  	}
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/5] btrfs: Use common error handling code in btrfs_update_root()

[Jeff Mahoney]

[-- Attachment #1.1: Type: text/plain, Size: 2441 bytes --]

On 8/21/17 8:40 AM, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 13:10:15 +0200
> 
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
> 
> This issue was detected by using the Coccinelle software.

btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
makes the code more difficult to debug.

-Jeff

> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  fs/btrfs/root-tree.c | 27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> index 5b488af6f25e..bc497ba9d9d1 100644
> --- a/fs/btrfs/root-tree.c
> +++ b/fs/btrfs/root-tree.c
> @@ -145,10 +145,8 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
>  		return -ENOMEM;
>  
>  	ret = btrfs_search_slot(trans, root, key, path, 0, 1);
> -	if (ret < 0) {
> -		btrfs_abort_transaction(trans, ret);
> -		goto out;
> -	}
> +	if (ret < 0)
> +		goto abort_transaction;
>  
>  	if (ret != 0) {
>  		btrfs_print_leaf(path->nodes[0]);
> @@ -171,23 +169,17 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
>  		btrfs_release_path(path);
>  		ret = btrfs_search_slot(trans, root, key, path,
>  				-1, 1);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  
>  		ret = btrfs_del_item(trans, root, path);
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  		btrfs_release_path(path);
>  		ret = btrfs_insert_empty_item(trans, root, path,
>  				key, sizeof(*item));
> -		if (ret < 0) {
> -			btrfs_abort_transaction(trans, ret);
> -			goto out;
> -		}
> +		if (ret < 0)
> +			goto abort_transaction;
>  		l = path->nodes[0];
>  		slot = path->slots[0];
>  		ptr = btrfs_item_ptr_offset(l, slot);
> @@ -204,6 +196,9 @@ int btrfs_update_root(struct btrfs_trans_handle *trans, struct btrfs_root
>  out:
>  	btrfs_free_path(path);
>  	return ret;
> +abort_transaction:
> +	btrfs_abort_transaction(trans, ret);
> +	goto out;
>  }
>  
>  int btrfs_insert_root(struct btrfs_trans_handle *trans, struct btrfs_root *root,
> 


-- 
Jeff Mahoney
SUSE Labs


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/5] btrfs: Use common error handling code in __btrfs_free_extent()

[Dan Carpenter]

On Mon, Aug 21, 2017 at 09:07:47AM -0400, Jeff Mahoney wrote:
> On 8/21/17 8:38 AM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 21 Aug 2017 10:03:00 +0200
> > 
> > Add a jump target so that a bit of exception handling can be better reused
> > at the end of this function.
> > 
> > This issue was detected by using the Coccinelle software.
> 
> btrfs_abort_transaction dumps __FILE__:__LINE__ in the log so this patch
> makes the code more difficult to debug.
> 

I was just reviewing this and I missed that issue.  These patches are
just exhausting...

regards,
dan carpenter

Re: [PATCH 0/5] BTRFS: Fine-tuning for five function implementations

[David Sterba]

On Mon, Aug 21, 2017 at 02:35:56PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 21 Aug 2017 14:30:12 +0200
> 
> A few update suggestions were taken into account
> from static source code analysis.

All patches make a nice gallery of anti-patterns.  None of them will get
applied.

Re: [PATCH 4/5] btrfs: Use common error handling code in update_ref_path()

[Dan Carpenter]

On Mon, Aug 21, 2017 at 09:08:04AM -0400, Jeff Mahoney wrote:
> On 8/21/17 8:41 AM, SF Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Mon, 21 Aug 2017 13:34:29 +0200
> > 
> > Add a jump target so that a bit of exception handling can be better reused
> > in this function.
> > 
> > This issue was detected by using the Coccinelle software.
> 
> Adding a jump label in the middle of a conditional for "common" error
> handling makes the code more difficult to understand.
> 

I have said that a bunch of times.  It's like bashing my face into the
keyboard for all the good it does.  On the other hand, some people
accept these oddly placed labels...  No one else writes code like this
so far as I know.

regards,
dan carpenter

[PATCH v2] btrfs: Use common error handling code in update_ref_path()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 21 Aug 2017 15:45:23 +0200

Add a jump target so that a bit of exception handling can be better reused
at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---

v2:
Do you find this refactoring acceptable instead?

 fs/btrfs/send.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 59fb1ed6ca20..527a9a735664 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -3697,20 +3697,20 @@ static int update_ref_path(struct send_ctx *sctx, struct recorded_ref *ref)
 		return -ENOMEM;
 
 	ret = get_cur_path(sctx, ref->dir, ref->dir_gen, new_path);
-	if (ret < 0) {
-		fs_path_free(new_path);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_path;
+
 	ret = fs_path_add(new_path, ref->name, ref->name_len);
-	if (ret < 0) {
-		fs_path_free(new_path);
-		return ret;
-	}
+	if (ret < 0)
+		goto free_path;
 
 	fs_path_free(ref->full_path);
 	set_ref_path(ref, new_path);
 
 	return 0;
+free_path:
+	fs_path_free(new_path);
+	return ret;
 }
 
 /*
-- 
2.14.0