[v2,19/21] btrfs: factor out chunk_allocation_failed()
diff mbox series

Message ID 20200212072048.629856-20-naohiro.aota@wdc.com
State New
Headers show
Series
  • btrfs: refactor and generalize chunk/dev_extent/extent allocation
Related show

Commit Message

Naohiro Aota Feb. 12, 2020, 7:20 a.m. UTC
Factor out chunk_allocation_failed() from find_free_extent_update_loop().
This function is called when it failed to allocate a chunk. The function
can modify "ffe_ctl->loop" and return 0 to continue with the next stage.
Or, it can return -ENOSPC to give up here.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/extent-tree.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

Comments

Johannes Thumshirn Feb. 12, 2020, 2:56 p.m. UTC | #1
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Josef Bacik Feb. 13, 2020, 7:52 p.m. UTC | #2
On 2/12/20 2:20 AM, Naohiro Aota wrote:
> Factor out chunk_allocation_failed() from find_free_extent_update_loop().
> This function is called when it failed to allocate a chunk. The function
> can modify "ffe_ctl->loop" and return 0 to continue with the next stage.
> Or, it can return -ENOSPC to give up here.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>   fs/btrfs/extent-tree.c | 24 ++++++++++++++++--------
>   1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index efc653e6be29..8f0d489f76fa 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3748,6 +3748,21 @@ static void found_extent(struct find_free_extent_ctl *ffe_ctl,
>   	}
>   }
>   
> +static int chunk_allocation_failed(struct find_free_extent_ctl *ffe_ctl)
> +{
> +	switch (ffe_ctl->policy) {
> +	case BTRFS_EXTENT_ALLOC_CLUSTERED:
> +		/*
> +		 * If we can't allocate a new chunk we've already looped through
> +		 * at least once, move on to the NO_EMPTY_SIZE case.
> +		 */
> +		ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
> +		return 0;
> +	default:
> +		BUG();
> +	}
> +}
> +
>   /*
>    * Return >0 means caller needs to re-search for free extent
>    * Return 0 means we have the needed free extent.
> @@ -3819,19 +3834,12 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>   			ret = btrfs_chunk_alloc(trans, ffe_ctl->flags,
>   						CHUNK_ALLOC_FORCE);
>   
> -			/*
> -			 * If we can't allocate a new chunk we've already looped
> -			 * through at least once, move on to the NO_EMPTY_SIZE
> -			 * case.
> -			 */
>   			if (ret == -ENOSPC)
> -				ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
> +				ret = chunk_allocation_failed(ffe_ctl);
>   
>   			/* Do not bail out on ENOSPC since we can do more. */
>   			if (ret < 0 && ret != -ENOSPC)
>   				btrfs_abort_transaction(trans, ret);
> -			else
> -				ret = 0;

You can't delete this, btrfs_chunk_alloc() will return 1 if it succeeded, and 
we'll leak that out somewhere and bad things will happen.  Thanks,

Josef
Naohiro Aota Feb. 20, 2020, 9:57 a.m. UTC | #3
On Thu, Feb 13, 2020 at 02:52:08PM -0500, Josef Bacik wrote:
>On 2/12/20 2:20 AM, Naohiro Aota wrote:
>>Factor out chunk_allocation_failed() from find_free_extent_update_loop().
>>This function is called when it failed to allocate a chunk. The function
>>can modify "ffe_ctl->loop" and return 0 to continue with the next stage.
>>Or, it can return -ENOSPC to give up here.
>>
>>Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>---
>>  fs/btrfs/extent-tree.c | 24 ++++++++++++++++--------
>>  1 file changed, 16 insertions(+), 8 deletions(-)
>>
>>diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>index efc653e6be29..8f0d489f76fa 100644
>>--- a/fs/btrfs/extent-tree.c
>>+++ b/fs/btrfs/extent-tree.c
>>@@ -3748,6 +3748,21 @@ static void found_extent(struct find_free_extent_ctl *ffe_ctl,
>>  	}
>>  }
>>+static int chunk_allocation_failed(struct find_free_extent_ctl *ffe_ctl)
>>+{
>>+	switch (ffe_ctl->policy) {
>>+	case BTRFS_EXTENT_ALLOC_CLUSTERED:
>>+		/*
>>+		 * If we can't allocate a new chunk we've already looped through
>>+		 * at least once, move on to the NO_EMPTY_SIZE case.
>>+		 */
>>+		ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
>>+		return 0;
>>+	default:
>>+		BUG();
>>+	}
>>+}
>>+
>>  /*
>>   * Return >0 means caller needs to re-search for free extent
>>   * Return 0 means we have the needed free extent.
>>@@ -3819,19 +3834,12 @@ static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
>>  			ret = btrfs_chunk_alloc(trans, ffe_ctl->flags,
>>  						CHUNK_ALLOC_FORCE);
>>-			/*
>>-			 * If we can't allocate a new chunk we've already looped
>>-			 * through at least once, move on to the NO_EMPTY_SIZE
>>-			 * case.
>>-			 */
>>  			if (ret == -ENOSPC)
>>-				ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
>>+				ret = chunk_allocation_failed(ffe_ctl);
>>  			/* Do not bail out on ENOSPC since we can do more. */
>>  			if (ret < 0 && ret != -ENOSPC)
>>  				btrfs_abort_transaction(trans, ret);
>>-			else
>>-				ret = 0;
>
>You can't delete this, btrfs_chunk_alloc() will return 1 if it 
>succeeded, and we'll leak that out somewhere and bad things will 
>happen.  Thanks,
>
>Josef

Ah, I missed that case... I'll fix in the next series.

Thanks,

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index efc653e6be29..8f0d489f76fa 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3748,6 +3748,21 @@  static void found_extent(struct find_free_extent_ctl *ffe_ctl,
 	}
 }
 
+static int chunk_allocation_failed(struct find_free_extent_ctl *ffe_ctl)
+{
+	switch (ffe_ctl->policy) {
+	case BTRFS_EXTENT_ALLOC_CLUSTERED:
+		/*
+		 * If we can't allocate a new chunk we've already looped through
+		 * at least once, move on to the NO_EMPTY_SIZE case.
+		 */
+		ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
+		return 0;
+	default:
+		BUG();
+	}
+}
+
 /*
  * Return >0 means caller needs to re-search for free extent
  * Return 0 means we have the needed free extent.
@@ -3819,19 +3834,12 @@  static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
 			ret = btrfs_chunk_alloc(trans, ffe_ctl->flags,
 						CHUNK_ALLOC_FORCE);
 
-			/*
-			 * If we can't allocate a new chunk we've already looped
-			 * through at least once, move on to the NO_EMPTY_SIZE
-			 * case.
-			 */
 			if (ret == -ENOSPC)
-				ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
+				ret = chunk_allocation_failed(ffe_ctl);
 
 			/* Do not bail out on ENOSPC since we can do more. */
 			if (ret < 0 && ret != -ENOSPC)
 				btrfs_abort_transaction(trans, ret);
-			else
-				ret = 0;
 			if (!exist)
 				btrfs_end_transaction(trans);
 			if (ret)