| Message ID | 20200212072048.629856-21-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: refactor and generalize chunk/dev_extent/extent allocation | expand |
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
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,
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
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.
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(+)