| Message ID | 20200212072048.629856-20-naohiro.aota@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: refactor and generalize chunk/dev_extent/extent allocation | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
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
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,
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)
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(-)