[PATCH 0/2] Preparation for later btrfs_alloc_chunk() rework, Part 2

[PATCH 0/2] Preparation for later btrfs_alloc_chunk() rework, Part 2

[Qu Wenruo]

This patchset, along with its prerequisite (patchset named:
"[PATCH 0/5] Cleanups for later btrfs_alloc_chunk() rework") can be
fetched from github:
https://github.com/adam900710/btrfs-progs/tree/chunk_alloc_enospc

This patchset can still be treated as cleanup, but brings a much larger
structure modification of btrfs_alloc_chunk().

The first patch will merge the original btrfs_alloc_data_chunk() with
more generic btrfs_alloc_chunk().

And 2nd patch integrate btrfs_make_block_group() into
btrfs_alloc_chunk(), provides the critical basis for later rework.
(Later rework needs to update space info before any tree modification,
so btrfs_make_block_group must be integrated)

Considering the importance of btrfs_alloc_chunk() in btrfs-progs, these
2 patches are separated out for a longer review window before larger
rework.

Qu Wenruo (2):
  btrfs-progs: Merge btrfs_alloc_data_chunk into btrfs_alloc_chunk
  btrfs-progs: Make btrfs_alloc_chunk to handle block group creation

 convert/main.c |  10 +--
 extent-tree.c  |  12 +---
 mkfs/main.c    |  42 ++---------
 volumes.c      | 222 +++++++++++++++++++++++----------------------------------
 volumes.h      |   5 +-
 5 files changed, 102 insertions(+), 189 deletions(-)

-- 
2.15.1

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

[Qu Wenruo]

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);
+	if (list_empty(dev_list))
 		return -ENOSPC;
-	}
 
+	if (convert) {
+		/* For convert, profile must be BTRFS_BLOCK_GROUP_DATA */
+		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);
+			return -EINVAL;
+		}
+		if (!IS_ALIGNED(*num_bytes, info->sectorsize)) {
+			error("chunk size not aligned, size=%llu sectorsize=%u\n",
+				*num_bytes, info->sectorsize);
+			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);
-- 
2.15.1

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

[Su Yue]

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.
> +	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) ..."

> +		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.
> +			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);
> 

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

[Su Yue]

On 01/05/2018 05:36 PM, 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.
>> +    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) ..."
Sorry, Should be "if (type & BTRFS_BLOCK_GROUP_DATA == 0) ..."

Thanks,
Su
> 
>> +        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.
>> +            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);
>>

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

[Qu Wenruo]

[-- 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 --]

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

[Qu Wenruo]

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>
---
changelog:
v2:
  Instead of re-init @private_devs list, change the later BUG_ON() for
  convert case.
  Update wrong comment to reflect the code.
  Remove unnecessary new-line in error().
---
 convert/main.c |   6 +-
 extent-tree.c  |   2 +-
 mkfs/main.c    |  14 ++--
 volumes.c      | 221 ++++++++++++++++++++++-----------------------------------
 volumes.h      |   5 +-
 5 files changed, 98 insertions(+), 150 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..a6522b3c72f8 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,38 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	struct btrfs_key key;
 	u64 offset;
 
-	if (list_empty(dev_list)) {
+	if (list_empty(dev_list))
 		return -ENOSPC;
-	}
 
+	if (convert) {
+		/* For convert, profile must be SINGLE */
+		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",
+				*start, info->sectorsize);
+			return -EINVAL;
+		}
+		if (!IS_ALIGNED(*num_bytes, info->sectorsize)) {
+			error("chunk size not aligned, size=%llu sectorsize=%u",
+				*num_bytes, info->sectorsize);
+			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 +992,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 +1025,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 +1053,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 +1083,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;
 
@@ -1057,7 +1124,7 @@ again:
 		memcpy(stripe->dev_uuid, device->uuid, BTRFS_UUID_SIZE);
 		index++;
 	}
-	BUG_ON(!list_empty(&private_devs));
+	BUG_ON(!convert && !list_empty(&private_devs));
 
 	/* key was set above */
 	btrfs_set_stack_chunk_length(chunk, *num_bytes);
@@ -1077,6 +1144,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 +1176,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);
-- 
2.15.1

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

[Su Yue]

On 01/08/2018 04:25 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>

Looks good to me.
Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
> changelog:
> v2:
>    Instead of re-init @private_devs list, change the later BUG_ON() for
>    convert case.
>    Update wrong comment to reflect the code.
>    Remove unnecessary new-line in error().
> ---
>   convert/main.c |   6 +-
>   extent-tree.c  |   2 +-
>   mkfs/main.c    |  14 ++--
>   volumes.c      | 221 ++++++++++++++++++++++-----------------------------------
>   volumes.h      |   5 +-
>   5 files changed, 98 insertions(+), 150 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..a6522b3c72f8 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,38 @@ int btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>   	struct btrfs_key key;
>   	u64 offset;
>   
> -	if (list_empty(dev_list)) {
> +	if (list_empty(dev_list))
>   		return -ENOSPC;
> -	}
>   
> +	if (convert) {
> +		/* For convert, profile must be SINGLE */
> +		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",
> +				*start, info->sectorsize);
> +			return -EINVAL;
> +		}
> +		if (!IS_ALIGNED(*num_bytes, info->sectorsize)) {
> +			error("chunk size not aligned, size=%llu sectorsize=%u",
> +				*num_bytes, info->sectorsize);
> +			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 +992,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 +1025,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 +1053,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 +1083,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;
>   
> @@ -1057,7 +1124,7 @@ again:
>   		memcpy(stripe->dev_uuid, device->uuid, BTRFS_UUID_SIZE);
>   		index++;
>   	}
> -	BUG_ON(!list_empty(&private_devs));
> +	BUG_ON(!convert && !list_empty(&private_devs));
>   
>   	/* key was set above */
>   	btrfs_set_stack_chunk_length(chunk, *num_bytes);
> @@ -1077,6 +1144,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 +1176,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);
> 

[PATCH 2/2] btrfs-progs: Make btrfs_alloc_chunk to handle block group creation

[Qu Wenruo]

Before this patch, chunk allocation is split into 2 parts:

1) Chunk allocation
   Handled by btrfs_alloc_chunk(), which will insert chunk and device
   extent items.

2) Block group allocation
   Handled by btrfs_make_block_group(), which will insert block group
   item and update space info.

However for chunk allocation, we don't really need to split these
operations as all btrfs_alloc_chunk() has btrfs_make_block_group()
followed.

So it's reasonable to merge btrfs_make_block_group() call into
btrfs_alloc_chunk() to save several lines, and provides the basis for
later btrfs_alloc_chunk() rework.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 convert/main.c |  4 ----
 extent-tree.c  | 10 ++--------
 mkfs/main.c    | 28 ----------------------------
 volumes.c      | 10 ++++++----
 4 files changed, 8 insertions(+), 44 deletions(-)

diff --git a/convert/main.c b/convert/main.c
index 8ee858fb2d05..96a04eda5b18 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -915,10 +915,6 @@ static int make_convert_data_block_groups(struct btrfs_trans_handle *trans,
 					BTRFS_BLOCK_GROUP_DATA, true);
 			if (ret < 0)
 				break;
-			ret = btrfs_make_block_group(trans, fs_info, 0,
-					BTRFS_BLOCK_GROUP_DATA, cur, len);
-			if (ret < 0)
-				break;
 			cur += len;
 		}
 	}
diff --git a/extent-tree.c b/extent-tree.c
index 4231be11bd53..90e792a3fe62 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -1910,15 +1910,9 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
 	                        space_info->flags, false);
 	if (ret == -ENOSPC) {
 		space_info->full = 1;
-		return 0;
+		return ret;
 	}
-
-	BUG_ON(ret);
-
-	ret = btrfs_make_block_group(trans, fs_info, 0, space_info->flags,
-				     start, num_bytes);
-	BUG_ON(ret);
-	return 0;
+	return ret;
 }
 
 static int update_block_group(struct btrfs_trans_handle *trans,
diff --git a/mkfs/main.c b/mkfs/main.c
index f8e27a7ec8b8..9377aa30f39d 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -97,12 +97,6 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 			error("no space to allocate data/metadata chunk");
 			goto err;
 		}
-		if (ret)
-			return ret;
-		ret = btrfs_make_block_group(trans, fs_info, 0,
-					     BTRFS_BLOCK_GROUP_METADATA |
-					     BTRFS_BLOCK_GROUP_DATA,
-					     chunk_start, chunk_size);
 		if (ret)
 			return ret;
 		allocation->mixed += chunk_size;
@@ -116,12 +110,7 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
 		}
 		if (ret)
 			return ret;
-		ret = btrfs_make_block_group(trans, fs_info, 0,
-					     BTRFS_BLOCK_GROUP_METADATA,
-					     chunk_start, chunk_size);
 		allocation->metadata += chunk_size;
-		if (ret)
-			return ret;
 	}
 
 	root->fs_info->system_allocs = 0;
@@ -150,12 +139,7 @@ static int create_data_block_groups(struct btrfs_trans_handle *trans,
 		}
 		if (ret)
 			return ret;
-		ret = btrfs_make_block_group(trans, fs_info, 0,
-					     BTRFS_BLOCK_GROUP_DATA,
-					     chunk_start, chunk_size);
 		allocation->data += chunk_size;
-		if (ret)
-			return ret;
 	}
 
 err:
@@ -259,9 +243,6 @@ static int create_one_raid_group(struct btrfs_trans_handle *trans,
 	if (ret)
 		return ret;
 
-	ret = btrfs_make_block_group(trans, fs_info, 0,
-				     type, chunk_start, chunk_size);
-
 	type &= BTRFS_BLOCK_GROUP_TYPE_MASK;
 	if (type == BTRFS_BLOCK_GROUP_DATA) {
 		allocation->data += chunk_size;
@@ -1006,12 +987,7 @@ static int create_chunks(struct btrfs_trans_handle *trans,
 				&chunk_start, &chunk_size, meta_type, false);
 		if (ret)
 			return ret;
-		ret = btrfs_make_block_group(trans, fs_info, 0,
-					     meta_type, chunk_start,
-					     chunk_size);
 		allocation->metadata += chunk_size;
-		if (ret)
-			return ret;
 		set_extent_dirty(&root->fs_info->free_space_cache,
 				 chunk_start, chunk_start + chunk_size - 1);
 	}
@@ -1023,11 +999,7 @@ static int create_chunks(struct btrfs_trans_handle *trans,
 				&chunk_start, &size_of_data, data_type, false);
 	if (ret)
 		return ret;
-	ret = btrfs_make_block_group(trans, fs_info, 0,
-				     data_type, chunk_start, size_of_data);
 	allocation->data += size_of_data;
-	if (ret)
-		return ret;
 	set_extent_dirty(&root->fs_info->free_space_cache,
 			 chunk_start, chunk_start + size_of_data - 1);
 	return ret;
diff --git a/volumes.c b/volumes.c
index 89c2f952f5b3..7bcebc708190 100644
--- a/volumes.c
+++ b/volumes.c
@@ -845,10 +845,9 @@ error:
 				/ 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()
+ * Alloc a chunk, will insert dev extents, chunk item, and insert new
+ * block group and update space info (so that extent allocator can use
+ * newly allocated chunk).
  *
  * @start:	return value of allocated chunk start bytenr.
  * @num_bytes:	return value of allocated chunk size
@@ -1168,6 +1167,9 @@ alloc_chunk:
 	}
 
 	kfree(chunk);
+
+	ret = btrfs_make_block_group(trans, info, 0, type, map->ce.start,
+				     map->ce.size);
 	return ret;
 
 out_chunk_map:
-- 
2.15.1

Re: [PATCH 2/2] btrfs-progs: Make btrfs_alloc_chunk to handle block group creation

[Su Yue]

On 01/05/2018 04:12 PM, Qu Wenruo wrote:
> Before this patch, chunk allocation is split into 2 parts:
> 
> 1) Chunk allocation
>     Handled by btrfs_alloc_chunk(), which will insert chunk and device
>     extent items.
> 
> 2) Block group allocation
>     Handled by btrfs_make_block_group(), which will insert block group
>     item and update space info.
> 
> However for chunk allocation, we don't really need to split these
> operations as all btrfs_alloc_chunk() has btrfs_make_block_group()
> followed.
> 
> So it's reasonable to merge btrfs_make_block_group() call into
> btrfs_alloc_chunk() to save several lines, and provides the basis for
> later btrfs_alloc_chunk() rework.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Looks good to me.
Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   convert/main.c |  4 ----
>   extent-tree.c  | 10 ++--------
>   mkfs/main.c    | 28 ----------------------------
>   volumes.c      | 10 ++++++----
>   4 files changed, 8 insertions(+), 44 deletions(-)
> 
> diff --git a/convert/main.c b/convert/main.c
> index 8ee858fb2d05..96a04eda5b18 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -915,10 +915,6 @@ static int make_convert_data_block_groups(struct btrfs_trans_handle *trans,
>   					BTRFS_BLOCK_GROUP_DATA, true);
>   			if (ret < 0)
>   				break;
> -			ret = btrfs_make_block_group(trans, fs_info, 0,
> -					BTRFS_BLOCK_GROUP_DATA, cur, len);
> -			if (ret < 0)
> -				break;
>   			cur += len;
>   		}
>   	}
> diff --git a/extent-tree.c b/extent-tree.c
> index 4231be11bd53..90e792a3fe62 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -1910,15 +1910,9 @@ static int do_chunk_alloc(struct btrfs_trans_handle *trans,
>   	                        space_info->flags, false);
>   	if (ret == -ENOSPC) {
>   		space_info->full = 1;
> -		return 0;
> +		return ret;
>   	}
> -
> -	BUG_ON(ret);
> -
> -	ret = btrfs_make_block_group(trans, fs_info, 0, space_info->flags,
> -				     start, num_bytes);
> -	BUG_ON(ret);
> -	return 0;
> +	return ret;
>   }
>   
>   static int update_block_group(struct btrfs_trans_handle *trans,
> diff --git a/mkfs/main.c b/mkfs/main.c
> index f8e27a7ec8b8..9377aa30f39d 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -97,12 +97,6 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
>   			error("no space to allocate data/metadata chunk");
>   			goto err;
>   		}
> -		if (ret)
> -			return ret;
> -		ret = btrfs_make_block_group(trans, fs_info, 0,
> -					     BTRFS_BLOCK_GROUP_METADATA |
> -					     BTRFS_BLOCK_GROUP_DATA,
> -					     chunk_start, chunk_size);
>   		if (ret)
>   			return ret;
>   		allocation->mixed += chunk_size;
> @@ -116,12 +110,7 @@ static int create_metadata_block_groups(struct btrfs_root *root, int mixed,
>   		}
>   		if (ret)
>   			return ret;
> -		ret = btrfs_make_block_group(trans, fs_info, 0,
> -					     BTRFS_BLOCK_GROUP_METADATA,
> -					     chunk_start, chunk_size);
>   		allocation->metadata += chunk_size;
> -		if (ret)
> -			return ret;
>   	}
>   
>   	root->fs_info->system_allocs = 0;
> @@ -150,12 +139,7 @@ static int create_data_block_groups(struct btrfs_trans_handle *trans,
>   		}
>   		if (ret)
>   			return ret;
> -		ret = btrfs_make_block_group(trans, fs_info, 0,
> -					     BTRFS_BLOCK_GROUP_DATA,
> -					     chunk_start, chunk_size);
>   		allocation->data += chunk_size;
> -		if (ret)
> -			return ret;
>   	}
>   
>   err:
> @@ -259,9 +243,6 @@ static int create_one_raid_group(struct btrfs_trans_handle *trans,
>   	if (ret)
>   		return ret;
>   
> -	ret = btrfs_make_block_group(trans, fs_info, 0,
> -				     type, chunk_start, chunk_size);
> -
>   	type &= BTRFS_BLOCK_GROUP_TYPE_MASK;
>   	if (type == BTRFS_BLOCK_GROUP_DATA) {
>   		allocation->data += chunk_size;
> @@ -1006,12 +987,7 @@ static int create_chunks(struct btrfs_trans_handle *trans,
>   				&chunk_start, &chunk_size, meta_type, false);
>   		if (ret)
>   			return ret;
> -		ret = btrfs_make_block_group(trans, fs_info, 0,
> -					     meta_type, chunk_start,
> -					     chunk_size);
>   		allocation->metadata += chunk_size;
> -		if (ret)
> -			return ret;
>   		set_extent_dirty(&root->fs_info->free_space_cache,
>   				 chunk_start, chunk_start + chunk_size - 1);
>   	}
> @@ -1023,11 +999,7 @@ static int create_chunks(struct btrfs_trans_handle *trans,
>   				&chunk_start, &size_of_data, data_type, false);
>   	if (ret)
>   		return ret;
> -	ret = btrfs_make_block_group(trans, fs_info, 0,
> -				     data_type, chunk_start, size_of_data);
>   	allocation->data += size_of_data;
> -	if (ret)
> -		return ret;
>   	set_extent_dirty(&root->fs_info->free_space_cache,
>   			 chunk_start, chunk_start + size_of_data - 1);
>   	return ret;
> diff --git a/volumes.c b/volumes.c
> index 89c2f952f5b3..7bcebc708190 100644
> --- a/volumes.c
> +++ b/volumes.c
> @@ -845,10 +845,9 @@ error:
>   				/ 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()
> + * Alloc a chunk, will insert dev extents, chunk item, and insert new
> + * block group and update space info (so that extent allocator can use
> + * newly allocated chunk).
>    *
>    * @start:	return value of allocated chunk start bytenr.
>    * @num_bytes:	return value of allocated chunk size
> @@ -1168,6 +1167,9 @@ alloc_chunk:
>   	}
>   
>   	kfree(chunk);
> +
> +	ret = btrfs_make_block_group(trans, info, 0, type, map->ce.start,
> +				     map->ce.size);
>   	return ret;
>   
>   out_chunk_map:
>