Message ID | 20200212072048.629856-10-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 create_chunk() from __btrfs_alloc_chunk(). This function finally > creates a chunk. There is no functional changes. > > Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> > --- > fs/btrfs/volumes.c | 130 ++++++++++++++++++++++++--------------------- > 1 file changed, 70 insertions(+), 60 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 00085943e4dd..3e2e3896d72a 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -5052,90 +5052,53 @@ static int decide_stripe_size(struct btrfs_fs_devices *fs_devices, > } > } > > -static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > - u64 start, u64 type) > +static int create_chunk(struct btrfs_trans_handle *trans, > + struct alloc_chunk_ctl *ctl, > + struct btrfs_device_info *devices_info) > { > struct btrfs_fs_info *info = trans->fs_info; > - struct btrfs_fs_devices *fs_devices = info->fs_devices; > struct map_lookup *map = NULL; > struct extent_map_tree *em_tree; > struct extent_map *em; > - struct btrfs_device_info *devices_info = NULL; > - struct alloc_chunk_ctl ctl; > + u64 start = ctl->start; > + u64 type = ctl->type; > int ret; > int i; > int j; > > - if (!alloc_profile_is_valid(type, 0)) { > - ASSERT(0); > - return -EINVAL; > - } > - > - if (list_empty(&fs_devices->alloc_list)) { > - if (btrfs_test_opt(info, ENOSPC_DEBUG)) > - btrfs_debug(info, "%s: no writable device", __func__); > - return -ENOSPC; > - } > - > - if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { > - btrfs_err(info, "invalid chunk type 0x%llx requested", type); > - BUG(); > - } > - > - ctl.start = start; > - ctl.type = type; > - init_alloc_chunk_ctl(fs_devices, &ctl); > - > - devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info), > - GFP_NOFS); > - if (!devices_info) > + map = kmalloc(map_lookup_size(ctl->num_stripes), GFP_NOFS); > + if (!map) > return -ENOMEM; > + map->num_stripes = ctl->num_stripes; > > - ret = gather_device_info(fs_devices, &ctl, devices_info); > - if (ret < 0) > - goto error; > - > - ret = decide_stripe_size(fs_devices, &ctl, devices_info); > - if (ret < 0) > - goto error; > - > - map = kmalloc(map_lookup_size(ctl.num_stripes), GFP_NOFS); > - if (!map) { > - ret = -ENOMEM; > - goto error; > - } > - > - map->num_stripes = ctl.num_stripes; > - > - for (i = 0; i < ctl.ndevs; ++i) { > - for (j = 0; j < ctl.dev_stripes; ++j) { > - int s = i * ctl.dev_stripes + j; > + for (i = 0; i < ctl->ndevs; ++i) { > + for (j = 0; j < ctl->dev_stripes; ++j) { > + int s = i * ctl->dev_stripes + j; > map->stripes[s].dev = devices_info[i].dev; > map->stripes[s].physical = devices_info[i].dev_offset + > - j * ctl.stripe_size; > + j * ctl->stripe_size; > } > } > map->stripe_len = BTRFS_STRIPE_LEN; > map->io_align = BTRFS_STRIPE_LEN; > map->io_width = BTRFS_STRIPE_LEN; > map->type = type; > - map->sub_stripes = ctl.sub_stripes; > + map->sub_stripes = ctl->sub_stripes; > > - trace_btrfs_chunk_alloc(info, map, start, ctl.chunk_size); > + trace_btrfs_chunk_alloc(info, map, start, ctl->chunk_size); > > em = alloc_extent_map(); > if (!em) { > kfree(map); > - ret = -ENOMEM; > - goto error; > + return -ENOMEM; > } > set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags); > em->map_lookup = map; > em->start = start; > - em->len = ctl.chunk_size; > + em->len = ctl->chunk_size; > em->block_start = 0; > em->block_len = em->len; > - em->orig_block_len = ctl.stripe_size; > + em->orig_block_len = ctl->stripe_size; > > em_tree = &info->mapping_tree; > write_lock(&em_tree->lock); > @@ -5143,11 +5106,11 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > if (ret) { > write_unlock(&em_tree->lock); > free_extent_map(em); > - goto error; > + return ret; > } > write_unlock(&em_tree->lock); > > - ret = btrfs_make_block_group(trans, 0, type, start, ctl.chunk_size); > + ret = btrfs_make_block_group(trans, 0, type, start, ctl->chunk_size); > if (ret) > goto error_del_extent; > > @@ -5155,20 +5118,19 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > struct btrfs_device *dev = map->stripes[i].dev; > > btrfs_device_set_bytes_used(dev, > - dev->bytes_used + ctl.stripe_size); > + dev->bytes_used + ctl->stripe_size); > if (list_empty(&dev->post_commit_list)) > list_add_tail(&dev->post_commit_list, > &trans->transaction->dev_update_list); > } > > - atomic64_sub(ctl.stripe_size * map->num_stripes, > + atomic64_sub(ctl->stripe_size * map->num_stripes, > &info->free_chunk_space); > > free_extent_map(em); > check_raid56_incompat_flag(info, type); > check_raid1c34_incompat_flag(info, type); > > - kfree(devices_info); > return 0; > > error_del_extent: > @@ -5180,7 +5142,55 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > free_extent_map(em); > /* One for the tree reference */ > free_extent_map(em); > -error: > + > + return ret; > +} > + > +static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, > + u64 start, u64 type) > +{ > + struct btrfs_fs_info *info = trans->fs_info; > + struct btrfs_fs_devices *fs_devices = info->fs_devices; > + struct btrfs_device_info *devices_info = NULL; > + struct alloc_chunk_ctl ctl; > + int ret; > + > + if (!alloc_profile_is_valid(type, 0)) { > + ASSERT(0); > + return -EINVAL; > + } > + > + if (list_empty(&fs_devices->alloc_list)) { > + if (btrfs_test_opt(info, ENOSPC_DEBUG)) > + btrfs_debug(info, "%s: no writable device", __func__); > + return -ENOSPC; > + } > + > + if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { > + btrfs_err(info, "invalid chunk type 0x%llx requested", type); > + BUG(); > + } This is superfluous, alloc_profile_is_valid() handles this check. Thanks, Josef
On Thu, Feb 13, 2020 at 11:24:57AM -0500, Josef Bacik wrote: >On 2/12/20 2:20 AM, Naohiro Aota wrote: >>Factor out create_chunk() from __btrfs_alloc_chunk(). This function finally >>creates a chunk. There is no functional changes. >> >>Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> >>--- >> fs/btrfs/volumes.c | 130 ++++++++++++++++++++++++--------------------- >> 1 file changed, 70 insertions(+), 60 deletions(-) >> >>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c >>index 00085943e4dd..3e2e3896d72a 100644 >>--- a/fs/btrfs/volumes.c >>+++ b/fs/btrfs/volumes.c >>@@ -5052,90 +5052,53 @@ static int decide_stripe_size(struct btrfs_fs_devices *fs_devices, >> } >> } >>-static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >>- u64 start, u64 type) >>+static int create_chunk(struct btrfs_trans_handle *trans, >>+ struct alloc_chunk_ctl *ctl, >>+ struct btrfs_device_info *devices_info) >> { >> struct btrfs_fs_info *info = trans->fs_info; >>- struct btrfs_fs_devices *fs_devices = info->fs_devices; >> struct map_lookup *map = NULL; >> struct extent_map_tree *em_tree; >> struct extent_map *em; >>- struct btrfs_device_info *devices_info = NULL; >>- struct alloc_chunk_ctl ctl; >>+ u64 start = ctl->start; >>+ u64 type = ctl->type; >> int ret; >> int i; >> int j; >>- if (!alloc_profile_is_valid(type, 0)) { >>- ASSERT(0); >>- return -EINVAL; >>- } >>- >>- if (list_empty(&fs_devices->alloc_list)) { >>- if (btrfs_test_opt(info, ENOSPC_DEBUG)) >>- btrfs_debug(info, "%s: no writable device", __func__); >>- return -ENOSPC; >>- } >>- >>- if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { >>- btrfs_err(info, "invalid chunk type 0x%llx requested", type); >>- BUG(); >>- } >>- >>- ctl.start = start; >>- ctl.type = type; >>- init_alloc_chunk_ctl(fs_devices, &ctl); >>- >>- devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info), >>- GFP_NOFS); >>- if (!devices_info) >>+ map = kmalloc(map_lookup_size(ctl->num_stripes), GFP_NOFS); >>+ if (!map) >> return -ENOMEM; >>+ map->num_stripes = ctl->num_stripes; >>- ret = gather_device_info(fs_devices, &ctl, devices_info); >>- if (ret < 0) >>- goto error; >>- >>- ret = decide_stripe_size(fs_devices, &ctl, devices_info); >>- if (ret < 0) >>- goto error; >>- >>- map = kmalloc(map_lookup_size(ctl.num_stripes), GFP_NOFS); >>- if (!map) { >>- ret = -ENOMEM; >>- goto error; >>- } >>- >>- map->num_stripes = ctl.num_stripes; >>- >>- for (i = 0; i < ctl.ndevs; ++i) { >>- for (j = 0; j < ctl.dev_stripes; ++j) { >>- int s = i * ctl.dev_stripes + j; >>+ for (i = 0; i < ctl->ndevs; ++i) { >>+ for (j = 0; j < ctl->dev_stripes; ++j) { >>+ int s = i * ctl->dev_stripes + j; >> map->stripes[s].dev = devices_info[i].dev; >> map->stripes[s].physical = devices_info[i].dev_offset + >>- j * ctl.stripe_size; >>+ j * ctl->stripe_size; >> } >> } >> map->stripe_len = BTRFS_STRIPE_LEN; >> map->io_align = BTRFS_STRIPE_LEN; >> map->io_width = BTRFS_STRIPE_LEN; >> map->type = type; >>- map->sub_stripes = ctl.sub_stripes; >>+ map->sub_stripes = ctl->sub_stripes; >>- trace_btrfs_chunk_alloc(info, map, start, ctl.chunk_size); >>+ trace_btrfs_chunk_alloc(info, map, start, ctl->chunk_size); >> em = alloc_extent_map(); >> if (!em) { >> kfree(map); >>- ret = -ENOMEM; >>- goto error; >>+ return -ENOMEM; >> } >> set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags); >> em->map_lookup = map; >> em->start = start; >>- em->len = ctl.chunk_size; >>+ em->len = ctl->chunk_size; >> em->block_start = 0; >> em->block_len = em->len; >>- em->orig_block_len = ctl.stripe_size; >>+ em->orig_block_len = ctl->stripe_size; >> em_tree = &info->mapping_tree; >> write_lock(&em_tree->lock); >>@@ -5143,11 +5106,11 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >> if (ret) { >> write_unlock(&em_tree->lock); >> free_extent_map(em); >>- goto error; >>+ return ret; >> } >> write_unlock(&em_tree->lock); >>- ret = btrfs_make_block_group(trans, 0, type, start, ctl.chunk_size); >>+ ret = btrfs_make_block_group(trans, 0, type, start, ctl->chunk_size); >> if (ret) >> goto error_del_extent; >>@@ -5155,20 +5118,19 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >> struct btrfs_device *dev = map->stripes[i].dev; >> btrfs_device_set_bytes_used(dev, >>- dev->bytes_used + ctl.stripe_size); >>+ dev->bytes_used + ctl->stripe_size); >> if (list_empty(&dev->post_commit_list)) >> list_add_tail(&dev->post_commit_list, >> &trans->transaction->dev_update_list); >> } >>- atomic64_sub(ctl.stripe_size * map->num_stripes, >>+ atomic64_sub(ctl->stripe_size * map->num_stripes, >> &info->free_chunk_space); >> free_extent_map(em); >> check_raid56_incompat_flag(info, type); >> check_raid1c34_incompat_flag(info, type); >>- kfree(devices_info); >> return 0; >> error_del_extent: >>@@ -5180,7 +5142,55 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >> free_extent_map(em); >> /* One for the tree reference */ >> free_extent_map(em); >>-error: >>+ >>+ return ret; >>+} >>+ >>+static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, >>+ u64 start, u64 type) >>+{ >>+ struct btrfs_fs_info *info = trans->fs_info; >>+ struct btrfs_fs_devices *fs_devices = info->fs_devices; >>+ struct btrfs_device_info *devices_info = NULL; >>+ struct alloc_chunk_ctl ctl; >>+ int ret; >>+ >>+ if (!alloc_profile_is_valid(type, 0)) { >>+ ASSERT(0); >>+ return -EINVAL; >>+ } >>+ >>+ if (list_empty(&fs_devices->alloc_list)) { >>+ if (btrfs_test_opt(info, ENOSPC_DEBUG)) >>+ btrfs_debug(info, "%s: no writable device", __func__); >>+ return -ENOSPC; >>+ } >>+ >>+ if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { >>+ btrfs_err(info, "invalid chunk type 0x%llx requested", type); >>+ BUG(); >>+ } > >This is superfluous, alloc_profile_is_valid() handles this check. Thanks, > >Josef This checks if at least one block group type (data, metadata or system) flag is set. OTOH, alloc_profile_is_valid() checks if profile bits are valid. Maybe, we can move this check into alloc_profile_is_valid()? Thanks,
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 00085943e4dd..3e2e3896d72a 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -5052,90 +5052,53 @@ static int decide_stripe_size(struct btrfs_fs_devices *fs_devices, } } -static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, - u64 start, u64 type) +static int create_chunk(struct btrfs_trans_handle *trans, + struct alloc_chunk_ctl *ctl, + struct btrfs_device_info *devices_info) { struct btrfs_fs_info *info = trans->fs_info; - struct btrfs_fs_devices *fs_devices = info->fs_devices; struct map_lookup *map = NULL; struct extent_map_tree *em_tree; struct extent_map *em; - struct btrfs_device_info *devices_info = NULL; - struct alloc_chunk_ctl ctl; + u64 start = ctl->start; + u64 type = ctl->type; int ret; int i; int j; - if (!alloc_profile_is_valid(type, 0)) { - ASSERT(0); - return -EINVAL; - } - - if (list_empty(&fs_devices->alloc_list)) { - if (btrfs_test_opt(info, ENOSPC_DEBUG)) - btrfs_debug(info, "%s: no writable device", __func__); - return -ENOSPC; - } - - if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { - btrfs_err(info, "invalid chunk type 0x%llx requested", type); - BUG(); - } - - ctl.start = start; - ctl.type = type; - init_alloc_chunk_ctl(fs_devices, &ctl); - - devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info), - GFP_NOFS); - if (!devices_info) + map = kmalloc(map_lookup_size(ctl->num_stripes), GFP_NOFS); + if (!map) return -ENOMEM; + map->num_stripes = ctl->num_stripes; - ret = gather_device_info(fs_devices, &ctl, devices_info); - if (ret < 0) - goto error; - - ret = decide_stripe_size(fs_devices, &ctl, devices_info); - if (ret < 0) - goto error; - - map = kmalloc(map_lookup_size(ctl.num_stripes), GFP_NOFS); - if (!map) { - ret = -ENOMEM; - goto error; - } - - map->num_stripes = ctl.num_stripes; - - for (i = 0; i < ctl.ndevs; ++i) { - for (j = 0; j < ctl.dev_stripes; ++j) { - int s = i * ctl.dev_stripes + j; + for (i = 0; i < ctl->ndevs; ++i) { + for (j = 0; j < ctl->dev_stripes; ++j) { + int s = i * ctl->dev_stripes + j; map->stripes[s].dev = devices_info[i].dev; map->stripes[s].physical = devices_info[i].dev_offset + - j * ctl.stripe_size; + j * ctl->stripe_size; } } map->stripe_len = BTRFS_STRIPE_LEN; map->io_align = BTRFS_STRIPE_LEN; map->io_width = BTRFS_STRIPE_LEN; map->type = type; - map->sub_stripes = ctl.sub_stripes; + map->sub_stripes = ctl->sub_stripes; - trace_btrfs_chunk_alloc(info, map, start, ctl.chunk_size); + trace_btrfs_chunk_alloc(info, map, start, ctl->chunk_size); em = alloc_extent_map(); if (!em) { kfree(map); - ret = -ENOMEM; - goto error; + return -ENOMEM; } set_bit(EXTENT_FLAG_FS_MAPPING, &em->flags); em->map_lookup = map; em->start = start; - em->len = ctl.chunk_size; + em->len = ctl->chunk_size; em->block_start = 0; em->block_len = em->len; - em->orig_block_len = ctl.stripe_size; + em->orig_block_len = ctl->stripe_size; em_tree = &info->mapping_tree; write_lock(&em_tree->lock); @@ -5143,11 +5106,11 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, if (ret) { write_unlock(&em_tree->lock); free_extent_map(em); - goto error; + return ret; } write_unlock(&em_tree->lock); - ret = btrfs_make_block_group(trans, 0, type, start, ctl.chunk_size); + ret = btrfs_make_block_group(trans, 0, type, start, ctl->chunk_size); if (ret) goto error_del_extent; @@ -5155,20 +5118,19 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, struct btrfs_device *dev = map->stripes[i].dev; btrfs_device_set_bytes_used(dev, - dev->bytes_used + ctl.stripe_size); + dev->bytes_used + ctl->stripe_size); if (list_empty(&dev->post_commit_list)) list_add_tail(&dev->post_commit_list, &trans->transaction->dev_update_list); } - atomic64_sub(ctl.stripe_size * map->num_stripes, + atomic64_sub(ctl->stripe_size * map->num_stripes, &info->free_chunk_space); free_extent_map(em); check_raid56_incompat_flag(info, type); check_raid1c34_incompat_flag(info, type); - kfree(devices_info); return 0; error_del_extent: @@ -5180,7 +5142,55 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, free_extent_map(em); /* One for the tree reference */ free_extent_map(em); -error: + + return ret; +} + +static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, + u64 start, u64 type) +{ + struct btrfs_fs_info *info = trans->fs_info; + struct btrfs_fs_devices *fs_devices = info->fs_devices; + struct btrfs_device_info *devices_info = NULL; + struct alloc_chunk_ctl ctl; + int ret; + + if (!alloc_profile_is_valid(type, 0)) { + ASSERT(0); + return -EINVAL; + } + + if (list_empty(&fs_devices->alloc_list)) { + if (btrfs_test_opt(info, ENOSPC_DEBUG)) + btrfs_debug(info, "%s: no writable device", __func__); + return -ENOSPC; + } + + if (!(type & BTRFS_BLOCK_GROUP_TYPE_MASK)) { + btrfs_err(info, "invalid chunk type 0x%llx requested", type); + BUG(); + } + + ctl.start = start; + ctl.type = type; + init_alloc_chunk_ctl(fs_devices, &ctl); + + devices_info = kcalloc(fs_devices->rw_devices, sizeof(*devices_info), + GFP_NOFS); + if (!devices_info) + return -ENOMEM; + + ret = gather_device_info(fs_devices, &ctl, devices_info); + if (ret < 0) + goto out; + + ret = decide_stripe_size(fs_devices, &ctl, devices_info); + if (ret < 0) + goto out; + + ret = create_chunk(trans, &ctl, devices_info); + +out: kfree(devices_info); return ret; }
Factor out create_chunk() from __btrfs_alloc_chunk(). This function finally creates a chunk. There is no functional changes. Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com> --- fs/btrfs/volumes.c | 130 ++++++++++++++++++++++++--------------------- 1 file changed, 70 insertions(+), 60 deletions(-)