[v2,20/21] btrfs: skip LOOP_NO_EMPTY_SIZE if not clustered allocation
diff mbox series

Message ID 20200212072048.629856-21-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
LOOP_NO_EMPTY_SIZE is solely dedicated for clustered allocation. So,
we can skip this stage and go to LOOP_GIVEUP stage to indicate we gave
up the allocation. This commit also moves the scope of the "clustered"
variable.

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

Comments

Josef Bacik Feb. 13, 2020, 7:55 p.m. UTC | #1
On 2/12/20 2:20 AM, Naohiro Aota wrote:
> LOOP_NO_EMPTY_SIZE is solely dedicated for clustered allocation. So,
> we can skip this stage and go to LOOP_GIVEUP stage to indicate we gave
> up the allocation. This commit also moves the scope of the "clustered"
> variable.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> ---
>   fs/btrfs/extent-tree.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 8f0d489f76fa..3ab0d2f5d718 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3373,6 +3373,7 @@ enum btrfs_loop_type {
>   	LOOP_CACHING_WAIT,
>   	LOOP_ALLOC_CHUNK,
>   	LOOP_NO_EMPTY_SIZE,
> +	LOOP_GIVEUP,

Why do we need a new loop definition here?  Can we just return ENOSPC and be 
done?  You don't appear to use it anywhere, so it doesn't seem like it's needed. 
  Thanks,

Josef
Naohiro Aota Feb. 20, 2020, 9:56 a.m. UTC | #2
On Thu, Feb 13, 2020 at 02:55:30PM -0500, Josef Bacik wrote:
>On 2/12/20 2:20 AM, Naohiro Aota wrote:
>>LOOP_NO_EMPTY_SIZE is solely dedicated for clustered allocation. So,
>>we can skip this stage and go to LOOP_GIVEUP stage to indicate we gave
>>up the allocation. This commit also moves the scope of the "clustered"
>>variable.
>>
>>Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>---
>>  fs/btrfs/extent-tree.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>>diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>index 8f0d489f76fa..3ab0d2f5d718 100644
>>--- a/fs/btrfs/extent-tree.c
>>+++ b/fs/btrfs/extent-tree.c
>>@@ -3373,6 +3373,7 @@ enum btrfs_loop_type {
>>  	LOOP_CACHING_WAIT,
>>  	LOOP_ALLOC_CHUNK,
>>  	LOOP_NO_EMPTY_SIZE,
>>+	LOOP_GIVEUP,
>
>Why do we need a new loop definition here?  Can we just return ENOSPC 
>and be done?  You don't appear to use it anywhere, so it doesn't seem 
>like it's needed.  Thanks,
>
>Josef

This is for other allocation policy to skip unnecessary loop stages
(e.g. LOOP_NO_EMPTY_SIZE) from an earlier stage. For example, zoned
allocation policy can implement the code below in
chunk_allocation_failed() to skip the following stages.

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 4badfae0c932..0a18c09b078b 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3775,6 +3854,10 @@ static int chunk_allocation_failed(struct find_free_extent_ctl *ffe_ctl)
                  */
                 ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
                 return 0;
+       case BTRFS_EXTENT_ALLOC_ZONED:
+               /* give up here */
+               ffe_ctl->loop = LOOP_GIVEUP;
+               return -ENOSPC;
         default:
                 BUG();
         }

But, I can keep this LOOP_GIVEUP introduction patch later with this
zoned allocator ones.

Thanks,
Josef Bacik Feb. 20, 2020, 3:40 p.m. UTC | #3
On 2/20/20 4:56 AM, Naohiro Aota wrote:
> On Thu, Feb 13, 2020 at 02:55:30PM -0500, Josef Bacik wrote:
>> On 2/12/20 2:20 AM, Naohiro Aota wrote:
>>> LOOP_NO_EMPTY_SIZE is solely dedicated for clustered allocation. So,
>>> we can skip this stage and go to LOOP_GIVEUP stage to indicate we gave
>>> up the allocation. This commit also moves the scope of the "clustered"
>>> variable.
>>>
>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>> ---
>>>  fs/btrfs/extent-tree.c | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index 8f0d489f76fa..3ab0d2f5d718 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -3373,6 +3373,7 @@ enum btrfs_loop_type {
>>>      LOOP_CACHING_WAIT,
>>>      LOOP_ALLOC_CHUNK,
>>>      LOOP_NO_EMPTY_SIZE,
>>> +    LOOP_GIVEUP,
>>
>> Why do we need a new loop definition here?  Can we just return ENOSPC and be 
>> done?  You don't appear to use it anywhere, so it doesn't seem like it's 
>> needed.  Thanks,
>>
>> Josef
> 
> This is for other allocation policy to skip unnecessary loop stages
> (e.g. LOOP_NO_EMPTY_SIZE) from an earlier stage. For example, zoned
> allocation policy can implement the code below in
> chunk_allocation_failed() to skip the following stages.
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 4badfae0c932..0a18c09b078b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -3775,6 +3854,10 @@ static int chunk_allocation_failed(struct 
> find_free_extent_ctl *ffe_ctl)
>                   */
>                  ffe_ctl->loop = LOOP_NO_EMPTY_SIZE;
>                  return 0;
> +       case BTRFS_EXTENT_ALLOC_ZONED:
> +               /* give up here */
> +               ffe_ctl->loop = LOOP_GIVEUP;
> +               return -ENOSPC;
>          default:
>                  BUG();
>          }
> 
> But, I can keep this LOOP_GIVEUP introduction patch later with this
> zoned allocator ones.
> 

Yes I'd rather they be with the real user, otherwise it's just confusing.  Thanks,

Josef

Patch
diff mbox series

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 8f0d489f76fa..3ab0d2f5d718 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -3373,6 +3373,7 @@  enum btrfs_loop_type {
 	LOOP_CACHING_WAIT,
 	LOOP_ALLOC_CHUNK,
 	LOOP_NO_EMPTY_SIZE,
+	LOOP_GIVEUP,
 };
 
 static inline void
@@ -3847,6 +3848,11 @@  static int find_free_extent_update_loop(struct btrfs_fs_info *fs_info,
 		}
 
 		if (ffe_ctl->loop == LOOP_NO_EMPTY_SIZE) {
+			if (ffe_ctl->policy != BTRFS_EXTENT_ALLOC_CLUSTERED) {
+				ffe_ctl->loop = LOOP_GIVEUP;
+				return -ENOSPC;
+			}
+
 			/*
 			 * Don't loop again if we already have no empty_size and
 			 * no empty_cluster.