btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance()
Subsystems (3)
PATCH 1/1 btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance()
From: Markus Elfring <[email protected]>
Date: Sun, 5 Nov 2017 22:03:22 +0100
* Adjust jump targets so that a call of the function "mutex_unlock" can be better reused for error cases at the end of this function.
* Replace three calls by goto statements.
This issue was detected by using the Coccinelle software.
--- fs/btrfs/volumes.c | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-)
fs/btrfs/volumes.c +17 -17
@@ -3497,7 +3497,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)if (ret) {/* btrfs_shrink_device never returns ret > 0 */ WARN_ON (ret >0 ); - goto error; + goto free_path; } trans =btrfs_start_transaction (dev_root,0 );
@@ -3507,7 +3507,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)"resize: unable to start transaction after shrinking device %s (error %d), old size %llu, new size %llu" ,rcu_str_deref (device->name), ret, old_size, old_size - size_to_free); - goto error; + goto free_path; } ret =btrfs_grow_device (trans, device, old_size);
@@ -3519,7 +3519,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)"resize: unable to grow device after shrinking device %s (error %d), old size %llu, new size %llu" ,rcu_str_deref (device->name), ret, old_size, old_size - size_to_free); - goto error; + goto free_path; }btrfs_end_transaction (trans);
@@ -3529,7 +3529,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) path =btrfs_alloc_path ();if (!path) { ret = -ENOMEM ; - goto error; + goto free_path; }/* zero out stat counters */
@@ -3554,15 +3554,13 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)if ((!counting &&atomic_read (&fs_info->balance_pause_req)) ||atomic_read (&fs_info->balance_cancel_req)) { ret = -ECANCELED ; - goto error; + goto free_path; }mutex_lock (&fs_info->delete_unused_bgs_mutex); ret =btrfs_search_slot (NULL , chunk_root, &key, path,0 ,0 ); -if (ret <0 ) { -mutex_unlock (&fs_info->delete_unused_bgs_mutex); - goto error; - } +if (ret <0 ) + goto unlock;/* * this shouldn't happen, it means the last relocate
@@ -3645,25 +3643,23 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) !chunk_reserved && !bytes_used) { trans =btrfs_start_transaction (chunk_root,0 );if (IS_ERR (trans)) { -mutex_unlock (&fs_info->delete_unused_bgs_mutex); ret =PTR_ERR (trans); - goto error; + goto unlock; } ret =btrfs_force_chunk_alloc (trans, fs_info,BTRFS_BLOCK_GROUP_DATA );btrfs_end_transaction (trans); -if (ret <0 ) { -mutex_unlock (&fs_info->delete_unused_bgs_mutex); - goto error; - } +if (ret <0 ) + goto unlock; + chunk_reserved =1 ; } ret =btrfs_relocate_chunk (fs_info, found_key.offset);mutex_unlock (&fs_info->delete_unused_bgs_mutex);if (ret && ret != -ENOSPC ) - goto error; + goto free_path;if (ret = -ENOSPC ) { enospc_errors++; }else {
@@ -3682,7 +3678,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) counting = false; goto again; } -error: +free_path:btrfs_free_path (path);if (enospc_errors) { btrfs_info(fs_info, "%d enospc errors during balance",
@@ -3692,6 +3688,10 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) }return ret; + +unlock: +mutex_unlock (&fs_info->delete_unused_bgs_mutex); + goto free_path; }/**
Signature · 1 line
2.15.0
Quoted 20 lines · SF Markus Elfring
* Adjust jump targets so that a call of the function "
mutex_unlock" can be better reused for error cases at the end of this function.* Replace three calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by Markus Elfring <[email protected]>---
fs/btrfs/volumes.c +1
-1@@ -3682,7 +3678,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) counting = false; goto again; } -error: +free_path:btrfs_free_path (path);if (enospc_errors) { btrfs_info(fs_info, "%d enospc errors during balance",
fs/btrfs/volumes.c static int __btrfs_balance(struct btrfs_fs_info *fs_info) · line 3695
Quoted 8 lines
fs/btrfs/volumes.c +4
-0@@ -3692,6 +3688,10 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) }return ret; + +unlock: +mutex_unlock (&fs_info->delete_unused_bgs_mutex); + goto free_path; }
This is also an anti-pattern, the label followed by a goto jumping back to the exit/cleanup block, right at the end of a function. I've sent some patches in the past to clean that up and don't want to reintroduce it.
Quoted 12 lines
Quoted 12 lines
fs/btrfs/volumes.c +1
-1@@ -3682,7 +3678,7 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) counting = false; goto again; } -error: +free_path:btrfs_free_path (path);if (enospc_errors) { btrfs_info(fs_info, "%d enospc errors during balance",@@ -3692,6 +3688,10 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info) }
Quoted 8 lines
fs/btrfs/volumes.c static int __btrfs_balance(struct btrfs_fs_info *fs_info) · line 3695
Quoted 6 lines
fs/btrfs/volumes.c +4
-0return ret; + +unlock: +mutex_unlock (&fs_info->delete_unused_bgs_mutex); + goto free_path; }This is also an anti-pattern,
I got an other software development opinion for this use case.
Quoted 2 lines
the label followed by a goto jumping back to the exit/cleanup block, right at the end of a function.
I find that this way can be useful for efficient exception handling.
Quoted 1 line
I've sent some patches in the past to clean that up
Interesting …
Quoted 1 line
and don't want to reintroduce it.
Would you like to reconsider this view if the object code size could be reduced a bit for the affected function implementation?
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html