[10/11] btrfs: merge unlocking to common exit block in btrfs_commit_transaction
diff mbox series

Message ID 133258557ae4387d6a1d01bafa3e5214ca91228d.1582302545.git.dsterba@suse.com
State New
Headers show
Series
  • Minor cleanups
Related show

Commit Message

David Sterba Feb. 21, 2020, 4:31 p.m. UTC
The tree_log_mutex and reloc_mutex locks are properly nested so we can
simplify error handling and add labels for them. This reduces line count
of the function.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/transaction.c | 57 +++++++++++++++---------------------------
 1 file changed, 20 insertions(+), 37 deletions(-)

Comments

Johannes Thumshirn Feb. 22, 2020, 8:50 a.m. UTC | #1
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Anand Jain Feb. 24, 2020, 3:56 a.m. UTC | #2
On 2/22/20 12:31 AM, David Sterba wrote:
> The tree_log_mutex and reloc_mutex locks are properly nested so we can
> simplify error handling and add labels for them. This reduces line count
> of the function.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>   fs/btrfs/transaction.c | 57 +++++++++++++++---------------------------
>   1 file changed, 20 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index fdfdfc426539..3610b6fec627 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * core function of the snapshot creation.
>   	 */
>   	ret = create_pending_snapshots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	/*
>   	 * We insert the dir indexes of the snapshots and update the inode
> @@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * the nodes and leaves.
>   	 */
>   	ret = btrfs_run_delayed_items(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;
>   
>   	/*
>   	 * make sure none of the code above managed to slip in a
> @@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	mutex_lock(&fs_info->tree_log_mutex);
>   
>   	ret = commit_fs_roots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_reloc;

                goto unlock_tree_log;


Otherwise looks good. When fixed this you may add.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks, Anand

>   	/*
>   	 * Since the transaction is done, we can apply the pending changes
> @@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 * new delayed refs. Must handle them or qgroup can be wrong.
>   	 */
>   	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_tree_log;
>   
>   	/*
>   	 * Since fs roots are all committed, we can get a quite accurate
>   	 * new_roots. So let's do quota accounting.
>   	 */
>   	ret = btrfs_qgroup_account_extents(trans);
> -	if (ret < 0) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret < 0)
> +		goto unlock_tree_log;
>   
>   	ret = commit_cowonly_roots(trans);
> -	if (ret) {
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> -	}
> +	if (ret)
> +		goto unlock_tree_log;
>   
>   	/*
>   	 * The tasks which save the space cache and inode cache may also
> @@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	 */
>   	if (TRANS_ABORTED(cur_trans)) {
>   		ret = cur_trans->aborted;
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		mutex_unlock(&fs_info->reloc_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;
>   	}
>   
>   	btrfs_prepare_extent_commit(fs_info);
> @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   	if (ret) {
>   		btrfs_handle_fs_error(fs_info, ret,
>   				      "Error while writing out transaction");
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;
>   	}
>   
>   	ret = write_all_supers(fs_info, 0);
> @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>   
>   	return ret;
>   
> +unlock_tree_log:
> +	mutex_unlock(&fs_info->tree_log_mutex);
> +unlock_reloc:
> +	mutex_unlock(&fs_info->reloc_mutex);
>   scrub_continue:
>   	btrfs_scrub_continue(fs_info);
>   cleanup_transaction:
>
David Sterba Feb. 24, 2020, 3:02 p.m. UTC | #3
On Fri, Feb 21, 2020 at 05:31:22PM +0100, David Sterba wrote:
>  	btrfs_prepare_extent_commit(fs_info);
> @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  	if (ret) {
>  		btrfs_handle_fs_error(fs_info, ret,
>  				      "Error while writing out transaction");
> -		mutex_unlock(&fs_info->tree_log_mutex);
> -		goto scrub_continue;
> +		goto unlock_tree_log;

Hm and this one is also wrong, in other cases that jump to
unlock_tree_log the unlocking order is tree_log_mutex/reloc_mutex, while
a few lines before there is unlock of reloc_mutex (while tree_log_mutex
is still held). This means the unlocking order is reversed compared to
the other cases and we can't jump to the label as this would lead to
double unlock of reloc_mutex.

So the above hunk must stay as is, with a comment.

>  	}
>  
>  	ret = write_all_supers(fs_info, 0);
> @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  
>  	return ret;
>  
> +unlock_tree_log:
> +	mutex_unlock(&fs_info->tree_log_mutex);
> +unlock_reloc:
> +	mutex_unlock(&fs_info->reloc_mutex);
>  scrub_continue:
>  	btrfs_scrub_continue(fs_info);
>  cleanup_transaction:
> -- 
> 2.25.0

Patch
diff mbox series

diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index fdfdfc426539..3610b6fec627 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2194,10 +2194,8 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * core function of the snapshot creation.
 	 */
 	ret = create_pending_snapshots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * We insert the dir indexes of the snapshots and update the inode
@@ -2210,16 +2208,12 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * the nodes and leaves.
 	 */
 	ret = btrfs_run_delayed_items(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * make sure none of the code above managed to slip in a
@@ -2245,11 +2239,8 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	mutex_lock(&fs_info->tree_log_mutex);
 
 	ret = commit_fs_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_reloc;
 
 	/*
 	 * Since the transaction is done, we can apply the pending changes
@@ -2267,29 +2258,20 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 * new delayed refs. Must handle them or qgroup can be wrong.
 	 */
 	ret = btrfs_run_delayed_refs(trans, (unsigned long)-1);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * Since fs roots are all committed, we can get a quite accurate
 	 * new_roots. So let's do quota accounting.
 	 */
 	ret = btrfs_qgroup_account_extents(trans);
-	if (ret < 0) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret < 0)
+		goto unlock_tree_log;
 
 	ret = commit_cowonly_roots(trans);
-	if (ret) {
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
-	}
+	if (ret)
+		goto unlock_tree_log;
 
 	/*
 	 * The tasks which save the space cache and inode cache may also
@@ -2297,9 +2279,7 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	 */
 	if (TRANS_ABORTED(cur_trans)) {
 		ret = cur_trans->aborted;
-		mutex_unlock(&fs_info->tree_log_mutex);
-		mutex_unlock(&fs_info->reloc_mutex);
-		goto scrub_continue;
+		goto unlock_tree_log;
 	}
 
 	btrfs_prepare_extent_commit(fs_info);
@@ -2346,8 +2326,7 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 	if (ret) {
 		btrfs_handle_fs_error(fs_info, ret,
 				      "Error while writing out transaction");
-		mutex_unlock(&fs_info->tree_log_mutex);
-		goto scrub_continue;
+		goto unlock_tree_log;
 	}
 
 	ret = write_all_supers(fs_info, 0);
@@ -2394,6 +2373,10 @@  int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 
 	return ret;
 
+unlock_tree_log:
+	mutex_unlock(&fs_info->tree_log_mutex);
+unlock_reloc:
+	mutex_unlock(&fs_info->reloc_mutex);
 scrub_continue:
 	btrfs_scrub_continue(fs_info);
 cleanup_transaction: