[06/35] btrfs: check if free bgs for commit
diff mbox series

Message ID 20180830174225.2200-7-josef@toxicpanda.com
State New
Headers show
Series
  • My current patch queue
Related show

Commit Message

Josef Bacik Aug. 30, 2018, 5:41 p.m. UTC
may_commit_transaction will skip committing the transaction if we don't
have enough pinned space or if we're trying to find space for a SYSTEM
chunk.  However if we have pending free block groups in this transaction
we still want to commit as we may be able to allocate a chunk to make
our reservation.  So instead of just returning ENOSPC, check if we have
free block groups pending, and if so commit the transaction to allow us
to use that free space.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent-tree.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Omar Sandoval Aug. 31, 2018, 11:18 p.m. UTC | #1
On Thu, Aug 30, 2018 at 01:41:56PM -0400, Josef Bacik wrote:
> may_commit_transaction will skip committing the transaction if we don't
> have enough pinned space or if we're trying to find space for a SYSTEM
> chunk.  However if we have pending free block groups in this transaction
> we still want to commit as we may be able to allocate a chunk to make
> our reservation.  So instead of just returning ENOSPC, check if we have
> free block groups pending, and if so commit the transaction to allow us
> to use that free space.

This makes sense.

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 6e7f350754d2..80615a579b18 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4804,6 +4804,7 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  	struct btrfs_trans_handle *trans;
>  	u64 bytes;
>  	u64 reclaim_bytes = 0;
> +	bool do_commit = true;

I find this naming a little mind bending when I read

	do_commit = false;
	goto commit;

Since the end result is that we always join the transaction if we make
it past the (!bytes) check anyways, can we do the pending bgs check
first? I find the following easier to follow, fwiw.

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index de6f75f5547b..dd7aeb5fb6bf 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4779,18 +4779,25 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 	if (!bytes)
 		return 0;
 
-	/* See if there is enough pinned space to make this reservation */
-	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
-				   bytes,
-				   BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0)
-		goto commit;
+	trans = btrfs_join_transaction(fs_info->extent_root);
+	if (IS_ERR(trans))
+		return -ENOSPC;
+
+	/*
+	 * See if we have a pending bg or there is enough pinned space to make
+	 * this reservation.
+	 */
+	if (test_bit(BTRFS_TRANS_HAVE_FREE_BGS, &trans->transaction->flags) ||
+	    __percpu_counter_compare(&space_info->total_bytes_pinned, bytes,
+				     BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0)
+		return btrfs_commit_transaction(trans);
 
 	/*
 	 * See if there is some space in the delayed insertion reservation for
 	 * this reservation.
 	 */
 	if (space_info != delayed_rsv->space_info)
-		return -ENOSPC;
+		goto enospc;
 
 	spin_lock(&delayed_rsv->lock);
 	if (delayed_rsv->size > bytes)
@@ -4801,16 +4808,14 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 
 	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
 				   bytes,
-				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) {
-		return -ENOSPC;
-	}
-
-commit:
-	trans = btrfs_join_transaction(fs_info->extent_root);
-	if (IS_ERR(trans))
-		return -ENOSPC;
+				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
+		goto enospc;
 
 	return btrfs_commit_transaction(trans);
+
+enospc:
+	btrfs_end_transaction(trans);
+	return -ENOSPC;
 }
 
 /*
Nikolay Borisov Sept. 3, 2018, 9:06 a.m. UTC | #2
On 30.08.2018 20:41, Josef Bacik wrote:
> may_commit_transaction will skip committing the transaction if we don't
> have enough pinned space or if we're trying to find space for a SYSTEM
> chunk.  However if we have pending free block groups in this transaction
> we still want to commit as we may be able to allocate a chunk to make
> our reservation.  So instead of just returning ENOSPC, check if we have
> free block groups pending, and if so commit the transaction to allow us
> to use that free space.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 6e7f350754d2..80615a579b18 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -4804,6 +4804,7 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  	struct btrfs_trans_handle *trans;
>  	u64 bytes;
>  	u64 reclaim_bytes = 0;
> +	bool do_commit = true;
>  
>  	trans = (struct btrfs_trans_handle *)current->journal_info;
>  	if (trans)

While you are at it, does this check even make sense, since the
transaction handle is  acquired proper later I think this can be removed?

> @@ -4832,8 +4833,10 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  	 * See if there is some space in the delayed insertion reservation for
>  	 * this reservation.
>  	 */
> -	if (space_info != delayed_rsv->space_info)
> -		return -ENOSPC;
> +	if (space_info != delayed_rsv->space_info) {
> +		do_commit = false;
> +		goto commit;
> +	}
>  
>  	spin_lock(&delayed_rsv->lock);
>  	reclaim_bytes += delayed_rsv->reserved;
> @@ -4848,15 +4851,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>  
>  	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
>  				   bytes,
> -				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) {
> -		return -ENOSPC;
> -	}
> -
> +				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
> +		do_commit = false;
>  commit:
>  	trans = btrfs_join_transaction(fs_info->extent_root);
>  	if (IS_ERR(trans))
>  		return -ENOSPC;
>  
> +	if (!do_commit &&> +	    !test_bit(BTRFS_TRANS_HAVE_FREE_BGS, &trans->transaction->flags)) {
> +		btrfs_end_transaction(trans);
> +		return -ENOSPC;
> +	}
>  	return btrfs_commit_transaction(trans);
>  }
>  
>
Nikolay Borisov Sept. 3, 2018, 1:19 p.m. UTC | #3
On  3.09.2018 12:06, Nikolay Borisov wrote:
> 
> 
> On 30.08.2018 20:41, Josef Bacik wrote:
>> may_commit_transaction will skip committing the transaction if we don't
>> have enough pinned space or if we're trying to find space for a SYSTEM
>> chunk.  However if we have pending free block groups in this transaction
>> we still want to commit as we may be able to allocate a chunk to make
>> our reservation.  So instead of just returning ENOSPC, check if we have
>> free block groups pending, and if so commit the transaction to allow us
>> to use that free space.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>  fs/btrfs/extent-tree.c | 18 ++++++++++++------
>>  1 file changed, 12 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 6e7f350754d2..80615a579b18 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -4804,6 +4804,7 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>>  	struct btrfs_trans_handle *trans;
>>  	u64 bytes;
>>  	u64 reclaim_bytes = 0;
>> +	bool do_commit = true;
>>  
>>  	trans = (struct btrfs_trans_handle *)current->journal_info;
>>  	if (trans)
> 
> While you are at it, does this check even make sense, since the
> transaction handle is  acquired proper later I think this can be removed?
> 
>> @@ -4832,8 +4833,10 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>>  	 * See if there is some space in the delayed insertion reservation for
>>  	 * this reservation.
>>  	 */
>> -	if (space_info != delayed_rsv->space_info)
>> -		return -ENOSPC;
>> +	if (space_info != delayed_rsv->space_info) {
>> +		do_commit = false;
>> +		goto commit;
>> +	}
>>  
>>  	spin_lock(&delayed_rsv->lock);
>>  	reclaim_bytes += delayed_rsv->reserved;
>> @@ -4848,15 +4851,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info,
>>  
>>  	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
>>  				   bytes,
>> -				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) {
>> -		return -ENOSPC;
>> -	}
>> -
>> +				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
>> +		do_commit = false;
>>  commit:
>>  	trans = btrfs_join_transaction(fs_info->extent_root);
>>  	if (IS_ERR(trans))
>>  		return -ENOSPC;
>>  
>> +	if (!do_commit &&> +	    !test_bit(BTRFS_TRANS_HAVE_FREE_BGS, &trans->transaction->flags)) {


offtopic: And yet on a more different note, do we really need the
BTRFS_TRANS_HAVE_FREE_BFS flag when we can just check
fs_info->free_chunk_space ?

>> +		btrfs_end_transaction(trans);
>> +		return -ENOSPC;
>> +	}
>>  	return btrfs_commit_transaction(trans);
>>  }
>>  
>>
>

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 6e7f350754d2..80615a579b18 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -4804,6 +4804,7 @@  static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 	struct btrfs_trans_handle *trans;
 	u64 bytes;
 	u64 reclaim_bytes = 0;
+	bool do_commit = true;
 
 	trans = (struct btrfs_trans_handle *)current->journal_info;
 	if (trans)
@@ -4832,8 +4833,10 @@  static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 	 * See if there is some space in the delayed insertion reservation for
 	 * this reservation.
 	 */
-	if (space_info != delayed_rsv->space_info)
-		return -ENOSPC;
+	if (space_info != delayed_rsv->space_info) {
+		do_commit = false;
+		goto commit;
+	}
 
 	spin_lock(&delayed_rsv->lock);
 	reclaim_bytes += delayed_rsv->reserved;
@@ -4848,15 +4851,18 @@  static int may_commit_transaction(struct btrfs_fs_info *fs_info,
 
 	if (__percpu_counter_compare(&space_info->total_bytes_pinned,
 				   bytes,
-				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) {
-		return -ENOSPC;
-	}
-
+				   BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0)
+		do_commit = false;
 commit:
 	trans = btrfs_join_transaction(fs_info->extent_root);
 	if (IS_ERR(trans))
 		return -ENOSPC;
 
+	if (!do_commit &&
+	    !test_bit(BTRFS_TRANS_HAVE_FREE_BGS, &trans->transaction->flags)) {
+		btrfs_end_transaction(trans);
+		return -ENOSPC;
+	}
 	return btrfs_commit_transaction(trans);
 }