Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1716403 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] BTRFS: Fine-tuning for five function implementations SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 14:40 +0200
    [PATCH 2/5] btrfs: Use common error handling code in  __btrfs_free_extent() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 14:40 +0200
      Re: [PATCH 2/5] btrfs: Use common error handling code in  __btrfs_free_extent() Jeff Mahoney <jeffm@suse.com> - 2017-08-21 15:10 +0200
        Re: [PATCH 2/5] btrfs: Use common error handling code in  __btrfs_free_extent() Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-21 15:20 +0200
    [PATCH 4/5] btrfs: Use common error handling code in  update_ref_path() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 14:50 +0200
      Re: [PATCH 4/5] btrfs: Use common error handling code in  update_ref_path() Jeff Mahoney <jeffm@suse.com> - 2017-08-21 15:10 +0200
        Re: [PATCH 4/5] btrfs: Use common error handling code in  update_ref_path() Dan Carpenter <dan.carpenter@oracle.com> - 2017-08-21 15:50 +0200
          [PATCH v2] btrfs: Use common error handling code in update_ref_path() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 16:00 +0200
    [PATCH 5/5] btrfs: Use common error handling code in  btrfs_mark_extent_written() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 14:50 +0200
      Re: [PATCH 5/5] btrfs: Use common error handling code in  btrfs_mark_extent_written() Jeff Mahoney <jeffm@suse.com> - 2017-08-21 15:10 +0200
    [PATCH 3/5] btrfs: Use common error handling code in  btrfs_update_root() SF Markus Elfring <elfring@users.sourceforge.net> - 2017-08-21 14:50 +0200
      Re: [PATCH 3/5] btrfs: Use common error handling code in  btrfs_update_root() Jeff Mahoney <jeffm@suse.com> - 2017-08-21 15:10 +0200
    Re: [PATCH 0/5] BTRFS: Fine-tuning for five function implementations David Sterba <dsterba@suse.cz> - 2017-08-21 15:30 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1716452

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web