* [PATCH] btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance()
@ 2017-11-06 8:04 SF Markus Elfring
2017-11-06 14:24 ` David Sterba
0 siblings, 1 reply; 3+ messages in thread
From: SF Markus Elfring @ 2017-11-06 8:04 UTC (permalink / raw)
To: linux-btrfs, Chris Mason, David Sterba, Josef Bacik
Cc: linux-fsdevel, kernel-janitors, LKML
From: Markus Elfring <elfring@users.sourceforge.net>
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.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
fs/btrfs/volumes.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ea8b20839ac0..3bc430623849 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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;
}
/**
--
2.15.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance()
2017-11-06 8:04 [PATCH] btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance() SF Markus Elfring
@ 2017-11-06 14:24 ` David Sterba
2017-11-06 15:08 ` SF Markus Elfring
0 siblings, 1 reply; 3+ messages in thread
From: David Sterba @ 2017-11-06 14:24 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-btrfs, Chris Mason, David Sterba, Josef Bacik,
linux-fsdevel, kernel-janitors, LKML
On Mon, Nov 06, 2017 at 09:04:37AM +0100, SF Markus Elfring wrote:
>
> * 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 <elfring@users.sourceforge.net>
> ---
> @@ -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;
> }
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance()
2017-11-06 14:24 ` David Sterba
@ 2017-11-06 15:08 ` SF Markus Elfring
0 siblings, 0 replies; 3+ messages in thread
From: SF Markus Elfring @ 2017-11-06 15:08 UTC (permalink / raw)
To: linux-btrfs, David Sterba
Cc: dsterba, Chris Mason, Josef Bacik, linux-fsdevel, kernel-janitors, LKML
>> @@ -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;
>> }
>
> This is also an anti-pattern,
I got an other software development opinion for this use case.
> 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.
> I've sent some patches in the past to clean that up
Interesting …
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-11-06 15:08 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-06 8:04 [PATCH] btrfs/volumes: Improve unlocking of a mutex in __btrfs_balance() SF Markus Elfring
2017-11-06 14:24 ` David Sterba
2017-11-06 15:08 ` SF Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).