Re: [PATCH 1/2] btrfs-progs: Merge btrfs_alloc_data_chunk into btrfs_alloc_chunk

[Qu Wenruo <wqu@suse.com>]

[-- Attachment #1.1: Type: text/plain, Size: 20461 bytes --]



On 2018年01月05日 17:36, Su Yue wrote:
> 
> 
> On 01/05/2018 04:12 PM, Qu Wenruo wrote:
>> We used to have two chunk allocators, btrfs_alloc_chunk() and
>> btrfs_alloc_data_chunk(), the former is the more generic one, while the
>> later is only used in mkfs and convert, to allocate SINGLE data chunk.
>>
>> Although btrfs_alloc_data_chunk() has some special hacks to cooperate
>> with convert, it's quite simple to integrity it into the generic chunk
>> allocator.
>>
>> So merge them into one btrfs_alloc_chunk(), with extra @convert
>> parameter and necessary comment, to make code less duplicated and less
>> thing to maintain.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   convert/main.c |   6 +-
>>   extent-tree.c  |   2 +-
>>   mkfs/main.c    |  14 ++--
>>   volumes.c      | 220
>> ++++++++++++++++++++++-----------------------------------
>>   volumes.h      |   5 +-
>>   5 files changed, 98 insertions(+), 149 deletions(-)
>>
>> diff --git a/convert/main.c b/convert/main.c
>> index 4a510a786394..8ee858fb2d05 100644
>> --- a/convert/main.c
>> +++ b/convert/main.c
>> @@ -910,9 +910,9 @@ static int make_convert_data_block_groups(struct
>> btrfs_trans_handle *trans,
>>                 len = min(max_chunk_size,
>>                     cache->start + cache->size - cur);
>> -            ret = btrfs_alloc_data_chunk(trans, fs_info,
>> -                    &cur_backup, len,
>> -                    BTRFS_BLOCK_GROUP_DATA, 1);
>> +            ret = btrfs_alloc_chunk(trans, fs_info,
>> +                    &cur_backup, &len,
>> +                    BTRFS_BLOCK_GROUP_DATA, true);
>>               if (ret < 0)
>>                   break;
>>               ret = btrfs_make_block_group(trans, fs_info, 0,
>> diff --git a/extent-tree.c b/extent-tree.c
>> index db24da3a3a8c..4231be11bd53 100644
>> --- a/extent-tree.c
>> +++ b/extent-tree.c
>> @@ -1907,7 +1907,7 @@ static int do_chunk_alloc(struct
>> btrfs_trans_handle *trans,
>>           return 0;
>>         ret = btrfs_alloc_chunk(trans, fs_info, &start, &num_bytes,
>> -                            space_info->flags);
>> +                            space_info->flags, false);
>>       if (ret == -ENOSPC) {
>>           space_info->full = 1;
>>           return 0;
>> diff --git a/mkfs/main.c b/mkfs/main.c
>> index 938025bfd32e..f8e27a7ec8b8 100644
>> --- a/mkfs/main.c
>> +++ b/mkfs/main.c
>> @@ -92,7 +92,7 @@ static int create_metadata_block_groups(struct
>> btrfs_root *root, int mixed,
>>           ret = btrfs_alloc_chunk(trans, fs_info,
>>                       &chunk_start, &chunk_size,
>>                       BTRFS_BLOCK_GROUP_METADATA |
>> -                    BTRFS_BLOCK_GROUP_DATA);
>> +                    BTRFS_BLOCK_GROUP_DATA, false);
>>           if (ret == -ENOSPC) {
>>               error("no space to allocate data/metadata chunk");
>>               goto err;
>> @@ -109,7 +109,7 @@ static int create_metadata_block_groups(struct
>> btrfs_root *root, int mixed,
>>       } else {
>>           ret = btrfs_alloc_chunk(trans, fs_info,
>>                       &chunk_start, &chunk_size,
>> -                    BTRFS_BLOCK_GROUP_METADATA);
>> +                    BTRFS_BLOCK_GROUP_METADATA, false);
>>           if (ret == -ENOSPC) {
>>               error("no space to allocate metadata chunk");
>>               goto err;
>> @@ -143,7 +143,7 @@ static int create_data_block_groups(struct
>> btrfs_trans_handle *trans,
>>       if (!mixed) {
>>           ret = btrfs_alloc_chunk(trans, fs_info,
>>                       &chunk_start, &chunk_size,
>> -                    BTRFS_BLOCK_GROUP_DATA);
>> +                    BTRFS_BLOCK_GROUP_DATA, false);
>>           if (ret == -ENOSPC) {
>>               error("no space to allocate data chunk");
>>               goto err;
>> @@ -251,7 +251,7 @@ static int create_one_raid_group(struct
>> btrfs_trans_handle *trans,
>>       int ret;
>>         ret = btrfs_alloc_chunk(trans, fs_info,
>> -                &chunk_start, &chunk_size, type);
>> +                &chunk_start, &chunk_size, type, false);
>>       if (ret == -ENOSPC) {
>>           error("not enough free space to allocate chunk");
>>           exit(1);
>> @@ -1003,7 +1003,7 @@ static int create_chunks(struct
>> btrfs_trans_handle *trans,
>>         for (i = 0; i < num_of_meta_chunks; i++) {
>>           ret = btrfs_alloc_chunk(trans, fs_info,
>> -                    &chunk_start, &chunk_size, meta_type);
>> +                &chunk_start, &chunk_size, meta_type, false);
>>           if (ret)
>>               return ret;
>>           ret = btrfs_make_block_group(trans, fs_info, 0,
>> @@ -1019,8 +1019,8 @@ static int create_chunks(struct
>> btrfs_trans_handle *trans,
>>       if (size_of_data < minimum_data_chunk_size)
>>           size_of_data = minimum_data_chunk_size;
>>   -    ret = btrfs_alloc_data_chunk(trans, fs_info,
>> -                     &chunk_start, size_of_data, data_type, 0);
>> +    ret = btrfs_alloc_chunk(trans, fs_info,
>> +                &chunk_start, &size_of_data, data_type, false);
>>       if (ret)
>>           return ret;
>>       ret = btrfs_make_block_group(trans, fs_info, 0,
>> diff --git a/volumes.c b/volumes.c
>> index fa3c6de023f9..89c2f952f5b3 100644
>> --- a/volumes.c
>> +++ b/volumes.c
>> @@ -844,9 +844,23 @@ error:
>>                   - 2 * sizeof(struct btrfs_chunk))    \
>>                   / sizeof(struct btrfs_stripe) + 1)
>>   +/*
>> + * Alloc a chunk, will insert dev extents, chunk item.
>> + * NOTE: This function will not insert block group item nor mark newly
>> + * allocated chunk available for later allocation.
>> + * Block group item and free space update is handled by
>> btrfs_make_block_group()
>> + *
>> + * @start:    return value of allocated chunk start bytenr.
>> + * @num_bytes:    return value of allocated chunk size
>> + * @type:    chunk type (including both profile and type)
>> + * @convert:    if the chunk is allocated for convert case.
>> + *         If @convert is true, chunk allocator will skip device extent
>> + *         search, but use *start and *num_bytes as chunk
>> start/num_bytes
>> + *         and devive offset, to build a 1:1 chunk mapping for convert.
>> + */
>>   int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>                 struct btrfs_fs_info *info, u64 *start,
>> -              u64 *num_bytes, u64 type)
>> +              u64 *num_bytes, u64 type, bool convert)
>>   {
>>       u64 dev_offset;
>>       struct btrfs_root *extent_root = info->extent_root;
>> @@ -876,10 +890,39 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle
>> *trans,
>>       struct btrfs_key key;
>>       u64 offset;
>>   -    if (list_empty(dev_list)) {
>> +    INIT_LIST_HEAD(&private_devs);
> 
> private_devs is initiated twice in the function.
> It seems that you forgot to delete latter one.

That's completely fine, since @private_devs is empty anyway.
Reinitiliazing it won't cause any problem.

On the other hand, if you don't init @private_devs, you will easily hit
BUG_ON(!list_empty(private_devs));

But since the unconditionally re-init seems a little annoying, I could
move it do convert branch, or just enhance the later BUG_ON() to skip
the check for convert == true case.

>> +    if (list_empty(dev_list))
>>           return -ENOSPC;
>> -    }
>>   +    if (convert) {
>> +        /* For convert, profile must be BTRFS_BLOCK_GROUP_DATA */
> 
> I wonder why not also check type like:
> "if (type & BTRFS_BLOCK_GROUP_DATA) ..."

Well, wrong comment.
Here I mean SINGLE profile, so the code is wrong but with wrong comment.

> 
>> +        if (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
>> +            error("convert only suports SINGLE profile");
>> +            return -EINVAL;
>> +        }
>> +        if (!IS_ALIGNED(*start, info->sectorsize)) {
>> +            error("chunk start not aligned, start=%llu sectorsize=%u\n",
>> +                *start, info->sectorsize);
> 
> error() breaks line itself. '\n' is unnecessary.

Yep, I forgot that.

Will update them soon.

Thanks,
Qu

>> +            return -EINVAL;
>> +        }
>> +        if (!IS_ALIGNED(*num_bytes, info->sectorsize)) {
>> +            error("chunk size not aligned, size=%llu sectorsize=%u\n",
>> +                *num_bytes, info->sectorsize);
> 
> And line break here.
> 
> Other changes are nice to me.
> 
> Thanks,
> Su
>> +            return -EINVAL;
>> +        }
>> +        calc_size = *num_bytes;
>> +        offset = *start;
>> +        /*
>> +         * For convert, we use 1:1 chunk mapping specified by @start and
>> +         * @num_bytes, so there is no need to go through dev_extent
>> +         * searching.
>> +         */
>> +        goto alloc_chunk;
>> +    }
>> +   
>> +    /*
>> +     * Chunk size calculation part.
>> +     */
>>       if (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
>>           if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
>>               calc_size = SZ_8M;
>> @@ -950,6 +993,9 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle
>> *trans,
>>       percent_max =
>> div_factor(btrfs_super_total_bytes(info->super_copy), 1);
>>       max_chunk_size = min(percent_max, max_chunk_size);
>>   +    /*
>> +     * Reserve space from each device.
>> +     */
>>   again:
>>       if (chunk_bytes_by_type(type, calc_size, num_stripes,
>> sub_stripes) >
>>           max_chunk_size) {
>> @@ -980,7 +1026,8 @@ again:
>>               return ret;
>>           cur = cur->next;
>>           if (avail >= min_free) {
>> -            list_move_tail(&device->dev_list, &private_devs);
>> +            list_move_tail(&device->dev_list,
>> +                       &private_devs);
>>               index++;
>>               if (type & BTRFS_BLOCK_GROUP_DUP)
>>                   index++;
>> @@ -1007,9 +1054,16 @@ again:
>>           }
>>           return -ENOSPC;
>>       }
>> -    ret = find_next_chunk(info, &offset);
>> -    if (ret)
>> -        return ret;
>> +
>> +    /*
>> +     * Fill chunk mapping and chunk stripes
>> +     */
>> +alloc_chunk:
>> +    if (!convert) {
>> +        ret = find_next_chunk(info, &offset);
>> +        if (ret)
>> +            return ret;
>> +    }
>>       key.objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
>>       key.type = BTRFS_CHUNK_ITEM_KEY;
>>       key.offset = offset;
>> @@ -1030,17 +1084,31 @@ again:
>>       index = 0;
>>       while(index < num_stripes) {
>>           struct btrfs_stripe *stripe;
>> -        BUG_ON(list_empty(&private_devs));
>> -        cur = private_devs.next;
>> -        device = list_entry(cur, struct btrfs_device, dev_list);
>>   -        /* loop over this device again if we're doing a dup group */
>> -        if (!(type & BTRFS_BLOCK_GROUP_DUP) ||
>> -            (index == num_stripes - 1))
>> -            list_move_tail(&device->dev_list, dev_list);
>> +        if (!convert) {
>> +            if (list_empty(&private_devs))
>> +                return -ENODEV;
>> +            cur = private_devs.next;
>> +            device = list_entry(cur, struct btrfs_device, dev_list);
>> +            if (!(type & BTRFS_BLOCK_GROUP_DUP) ||
>> +                (index == num_stripes - 1)) {
>> +                /*
>> +                 * loop over this device again if we're doing a
>> +                 * dup group
>> +                 */
>> +                list_move_tail(&device->dev_list, dev_list);
>> +            }
>> +        } else {
>> +            /* Only SINGLE is accepted in convert case */
>> +            BUG_ON(num_stripes > 1);
>> +            device = list_entry(dev_list->next, struct btrfs_device,
>> +                        dev_list);
>> +            key.offset = *start;
>> +            dev_offset = *start;
>> +        }
>>             ret = btrfs_alloc_dev_extent(trans, device, key.offset,
>> -                 calc_size, &dev_offset, 0);
>> +                 calc_size, &dev_offset, convert);
>>           if (ret < 0)
>>               goto out_chunk_map;
>>   @@ -1077,6 +1145,9 @@ again:
>>       map->num_stripes = num_stripes;
>>       map->sub_stripes = sub_stripes;
>>   +    /*
>> +     * Insert chunk item and chunk mapping.
>> +     */
>>       ret = btrfs_insert_item(trans, chunk_root, &key, chunk,
>>                   btrfs_chunk_item_size(num_stripes));
>>       BUG_ON(ret);
>> @@ -1106,125 +1177,6 @@ out_chunk:
>>       return ret;
>>   }
>>   -/*
>> - * Alloc a DATA chunk with SINGLE profile.
>> - *
>> - * If 'convert' is set, it will alloc a chunk with 1:1 mapping
>> - * (btrfs logical bytenr == on-disk bytenr)
>> - * For that case, caller must make sure the chunk and dev_extent are not
>> - * occupied.
>> - */
>> -int btrfs_alloc_data_chunk(struct btrfs_trans_handle *trans,
>> -               struct btrfs_fs_info *info, u64 *start,
>> -               u64 num_bytes, u64 type, int convert)
>> -{
>> -    u64 dev_offset;
>> -    struct btrfs_root *extent_root = info->extent_root;
>> -    struct btrfs_root *chunk_root = info->chunk_root;
>> -    struct btrfs_stripe *stripes;
>> -    struct btrfs_device *device = NULL;
>> -    struct btrfs_chunk *chunk;
>> -    struct list_head *dev_list = &info->fs_devices->devices;
>> -    struct list_head *cur;
>> -    struct map_lookup *map;
>> -    u64 calc_size = SZ_8M;
>> -    int num_stripes = 1;
>> -    int sub_stripes = 0;
>> -    int ret;
>> -    int index;
>> -    int stripe_len = BTRFS_STRIPE_LEN;
>> -    struct btrfs_key key;
>> -
>> -    key.objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
>> -    key.type = BTRFS_CHUNK_ITEM_KEY;
>> -    if (convert) {
>> -        if (*start != round_down(*start, info->sectorsize)) {
>> -            error("DATA chunk start not sectorsize aligned: %llu",
>> -                    (unsigned long long)*start);
>> -            return -EINVAL;
>> -        }
>> -        key.offset = *start;
>> -        dev_offset = *start;
>> -    } else {
>> -        u64 tmp;
>> -
>> -        ret = find_next_chunk(info, &tmp);
>> -        key.offset = tmp;
>> -        if (ret)
>> -            return ret;
>> -    }
>> -
>> -    chunk = kmalloc(btrfs_chunk_item_size(num_stripes), GFP_NOFS);
>> -    if (!chunk)
>> -        return -ENOMEM;
>> -
>> -    map = kmalloc(btrfs_map_lookup_size(num_stripes), GFP_NOFS);
>> -    if (!map) {
>> -        kfree(chunk);
>> -        return -ENOMEM;
>> -    }
>> -
>> -    stripes = &chunk->stripe;
>> -    calc_size = num_bytes;
>> -
>> -    index = 0;
>> -    cur = dev_list->next;
>> -    device = list_entry(cur, struct btrfs_device, dev_list);
>> -
>> -    while (index < num_stripes) {
>> -        struct btrfs_stripe *stripe;
>> -
>> -        ret = btrfs_alloc_dev_extent(trans, device, key.offset,
>> -                 calc_size, &dev_offset, convert);
>> -        BUG_ON(ret);
>> -
>> -        device->bytes_used += calc_size;
>> -        ret = btrfs_update_device(trans, device);
>> -        BUG_ON(ret);
>> -
>> -        map->stripes[index].dev = device;
>> -        map->stripes[index].physical = dev_offset;
>> -        stripe = stripes + index;
>> -        btrfs_set_stack_stripe_devid(stripe, device->devid);
>> -        btrfs_set_stack_stripe_offset(stripe, dev_offset);
>> -        memcpy(stripe->dev_uuid, device->uuid, BTRFS_UUID_SIZE);
>> -        index++;
>> -    }
>> -
>> -    /* key was set above */
>> -    btrfs_set_stack_chunk_length(chunk, num_bytes);
>> -    btrfs_set_stack_chunk_owner(chunk, extent_root->root_key.objectid);
>> -    btrfs_set_stack_chunk_stripe_len(chunk, stripe_len);
>> -    btrfs_set_stack_chunk_type(chunk, type);
>> -    btrfs_set_stack_chunk_num_stripes(chunk, num_stripes);
>> -    btrfs_set_stack_chunk_io_align(chunk, stripe_len);
>> -    btrfs_set_stack_chunk_io_width(chunk, stripe_len);
>> -    btrfs_set_stack_chunk_sector_size(chunk, info->sectorsize);
>> -    btrfs_set_stack_chunk_sub_stripes(chunk, sub_stripes);
>> -    map->sector_size = info->sectorsize;
>> -    map->stripe_len = stripe_len;
>> -    map->io_align = stripe_len;
>> -    map->io_width = stripe_len;
>> -    map->type = type;
>> -    map->num_stripes = num_stripes;
>> -    map->sub_stripes = sub_stripes;
>> -
>> -    ret = btrfs_insert_item(trans, chunk_root, &key, chunk,
>> -                btrfs_chunk_item_size(num_stripes));
>> -    BUG_ON(ret);
>> -    if (!convert)
>> -        *start = key.offset;
>> -
>> -    map->ce.start = key.offset;
>> -    map->ce.size = num_bytes;
>> -
>> -    ret = insert_cache_extent(&info->mapping_tree.cache_tree, &map->ce);
>> -    BUG_ON(ret);
>> -
>> -    kfree(chunk);
>> -    return ret;
>> -}
>> -
>>   int btrfs_num_copies(struct btrfs_fs_info *fs_info, u64 logical, u64
>> len)
>>   {
>>       struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
>> diff --git a/volumes.h b/volumes.h
>> index 11572e78c04f..7bbdf615d31a 100644
>> --- a/volumes.h
>> +++ b/volumes.h
>> @@ -208,10 +208,7 @@ int btrfs_read_sys_array(struct btrfs_fs_info
>> *fs_info);
>>   int btrfs_read_chunk_tree(struct btrfs_fs_info *fs_info);
>>   int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>                 struct btrfs_fs_info *fs_info, u64 *start,
>> -              u64 *num_bytes, u64 type);
>> -int btrfs_alloc_data_chunk(struct btrfs_trans_handle *trans,
>> -               struct btrfs_fs_info *fs_info, u64 *start,
>> -               u64 num_bytes, u64 type, int convert);
>> +              u64 *num_bytes, u64 type, bool convert);
>>   int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
>>                  int flags);
>>   int btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
>>
> 
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 504 bytes --]