[PATCH 2/2] Btrfs: simplify error handling of clean_pinned_extents()

[PATCH 2/2] Btrfs: simplify error handling of clean_pinned_extents()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At clean_pinned_extents(), whether we end up returning success or failure,
we pretty much have to do the same things:

1) unlock unused_bg_unpin_mutex
2) decrement reference count on the previous transaction

We also call btrfs_dec_block_group_ro() in case of failure, but that is
better done in its caller, btrfs_delete_unused_bgs(), since its the
caller that calls inc_block_group_ro(), so it should be responsible for
the decrement operation, as it is in case any of the other functions it
calls fail.

So move the call to btrfs_dec_block_group_ro() from clean_pinned_extents()
into  btrfs_delete_unused_bgs() and unify the error and success return
paths for clean_pinned_extents(), reducing duplicated code and making it
simpler.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 93c180ffcb80..a615d8585df4 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1280,25 +1280,17 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
 		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
 					EXTENT_DIRTY);
 		if (ret)
-			goto err;
+			goto out;
 	}
 
 	ret = clear_extent_bits(&trans->transaction->pinned_extents, start, end,
 				EXTENT_DIRTY);
-	if (ret)
-		goto err;
+out:
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 	if (prev_trans)
 		refcount_dec(&prev_trans->use_count);
 
-	return true;
-
-err:
-	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
-	if (prev_trans)
-		refcount_dec(&prev_trans->use_count);
-	btrfs_dec_block_group_ro(bg);
-	return false;
+	return ret == 0;
 }
 
 /*
@@ -1396,8 +1388,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		if (!clean_pinned_extents(trans, block_group))
+		if (!clean_pinned_extents(trans, block_group)) {
+			btrfs_dec_block_group_ro(block_group);
 			goto end_trans;
+		}
 
 		/*
 		 * At this point, the block_group is read only and should fail
-- 
2.11.0

Re: [PATCH 2/2] Btrfs: simplify error handling of clean_pinned_extents()

[David Sterba]

On Fri, Apr 17, 2020 at 03:40:21PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At clean_pinned_extents(), whether we end up returning success or failure,
> we pretty much have to do the same things:
> 
> 1) unlock unused_bg_unpin_mutex
> 2) decrement reference count on the previous transaction
> 
> We also call btrfs_dec_block_group_ro() in case of failure, but that is
> better done in its caller, btrfs_delete_unused_bgs(), since its the
> caller that calls inc_block_group_ro(), so it should be responsible for
> the decrement operation, as it is in case any of the other functions it
> calls fail.
> 
> So move the call to btrfs_dec_block_group_ro() from clean_pinned_extents()
> into  btrfs_delete_unused_bgs() and unify the error and success return
> paths for clean_pinned_extents(), reducing duplicated code and making it
> simpler.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.

Re: [PATCH 2/2] Btrfs: simplify error handling of clean_pinned_extents()

[Nikolay Borisov]

On 17.04.20 г. 17:40 ч., fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At clean_pinned_extents(), whether we end up returning success or failure,
> we pretty much have to do the same things:
> 
> 1) unlock unused_bg_unpin_mutex
> 2) decrement reference count on the previous transaction
> 
> We also call btrfs_dec_block_group_ro() in case of failure, but that is
> better done in its caller, btrfs_delete_unused_bgs(), since its the
> caller that calls inc_block_group_ro(), so it should be responsible for
> the decrement operation, as it is in case any of the other functions it
> calls fail.
> 
> So move the call to btrfs_dec_block_group_ro() from clean_pinned_extents()
> into  btrfs_delete_unused_bgs() and unify the error and success return
> paths for clean_pinned_extents(), reducing duplicated code and making it
> simpler.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

[PATCH v2 2/2] Btrfs: simplify error handling of clean_pinned_extents()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At clean_pinned_extents(), whether we end up returning success or failure,
we pretty much have to do the same things:

1) unlock unused_bg_unpin_mutex
2) decrement reference count on the previous transaction

We also call btrfs_dec_block_group_ro() in case of failure, but that is
better done in its caller, btrfs_delete_unused_bgs(), since its the
caller that calls inc_block_group_ro(), so it should be responsible for
the decrement operation, as it is in case any of the other functions it
calls fail.

So move the call to btrfs_dec_block_group_ro() from clean_pinned_extents()
into  btrfs_delete_unused_bgs() and unify the error and success return
paths for clean_pinned_extents(), reducing duplicated code and making it
simpler.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---

V2: Updated version after patch 1 in the series changed.

 fs/btrfs/block-group.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index af9e9a008724..f96ab9d6f3fe 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1280,25 +1280,17 @@ static bool clean_pinned_extents(struct btrfs_trans_handle *trans,
 		ret = clear_extent_bits(&prev_trans->pinned_extents, start, end,
 					EXTENT_DIRTY);
 		if (ret)
-			goto err;
+			goto out;
 	}
 
 	ret = clear_extent_bits(&trans->transaction->pinned_extents, start, end,
 				EXTENT_DIRTY);
-	if (ret)
-		goto err;
+out:
 	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
 	if (prev_trans)
 		btrfs_put_transaction(prev_trans);
 
-	return true;
-
-err:
-	mutex_unlock(&fs_info->unused_bg_unpin_mutex);
-	if (prev_trans)
-		btrfs_put_transaction(prev_trans);
-	btrfs_dec_block_group_ro(bg);
-	return false;
+	return ret == 0;
 }
 
 /*
@@ -1396,8 +1388,10 @@ void btrfs_delete_unused_bgs(struct btrfs_fs_info *fs_info)
 		 * We could have pending pinned extents for this block group,
 		 * just delete them, we don't care about them anymore.
 		 */
-		if (!clean_pinned_extents(trans, block_group))
+		if (!clean_pinned_extents(trans, block_group)) {
+			btrfs_dec_block_group_ro(block_group);
 			goto end_trans;
+		}
 
 		/*
 		 * At this point, the block_group is read only and should fail
-- 
2.11.0