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

[permalink] [raw]

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2017-08-21 12:39:12

[permalink] [raw]

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2017-08-21 12:41:10

[permalink] [raw]

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2017-08-21 12:41:57

[permalink] [raw]

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2017-08-21 12:42:52

[permalink] [raw]

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

From: Markus Elfring <[email protected]>
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 <[email protected]>
---
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

2017-08-21 13:07:14

[permalink] [raw]

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

On 8/21/17 8:42 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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

Attachments:

2017-08-21 13:07:53

[permalink] [raw]

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

On 8/21/17 8:38 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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

Attachments:

2017-08-21 13:08:11

[permalink] [raw]

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

On 8/21/17 8:41 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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

Attachments:

2017-08-21 13:09:19

[permalink] [raw]

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

On 8/21/17 8:40 AM, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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 <[email protected]>
> ---
> 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

Attachments:

2017-08-21 13:14:51

by Dan Carpenter

[permalink] [raw]

Subject: 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 <[email protected]>
> > 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

2017-08-21 13:25:55

by David Sterba

[permalink] [raw]

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

On Mon, Aug 21, 2017 at 02:35:56PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> 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.

2017-08-21 13:42:55

by Dan Carpenter

[permalink] [raw]

Subject: 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 <[email protected]>
> > 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

2017-08-21 13:53:08