Re: [PATCH v6 10/16] btrfs: handle setting up relocation of block group with remap-tree

[Sun Yangkai <sunk67188@gmail.com>]

While reading the thread, I noticed the logic that builds the identity_remap
entries was a bit hard to follow.
I took the liberty of rewriting the function so that the two high-level cases
are immediately visible inside a single if/else. The result has no behavioral
change, and (at least to me) makes it obvious where the head/tail gaps are handled.
The modified code is shown below; feel free to pick it up if you find it useful.
Please let me know if I missed anything.

> 
> +static int create_remap_tree_entries(struct btrfs_trans_handle *trans,
> +				     struct btrfs_path *path,
> +				     struct btrfs_block_group *bg)
> +{
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
> +	struct btrfs_free_space_info *fsi;
> +	struct btrfs_key key, found_key;
> +	struct extent_buffer *leaf;
> +	struct btrfs_root *space_root;
> +	u32 extent_count;
> +	struct space_run *space_runs = NULL;
> +	unsigned int num_space_runs = 0;
> +	struct btrfs_key *entries = NULL;
> +	unsigned int max_entries, num_entries;
> +	int ret;
> +
> +	mutex_lock(&bg->free_space_lock);
> +
> +	if (test_bit(BLOCK_GROUP_FLAG_NEEDS_FREE_SPACE, &bg->runtime_flags)) {
> +		mutex_unlock(&bg->free_space_lock);
> +
> +		ret = btrfs_add_block_group_free_space(trans, bg);
> +		if (ret)
> +			return ret;
> +
> +		mutex_lock(&bg->free_space_lock);
> +	}
> +
> +	fsi = btrfs_search_free_space_info(trans, bg, path, 0);
> +	if (IS_ERR(fsi)) {
> +		mutex_unlock(&bg->free_space_lock);
> +		return PTR_ERR(fsi);
> +	}
> +
> +	extent_count = btrfs_free_space_extent_count(path->nodes[0], fsi);
> +
> +	btrfs_release_path(path);
> +
> +	space_runs = kmalloc(sizeof(*space_runs) * extent_count, GFP_NOFS);
> +	if (!space_runs) {
> +		mutex_unlock(&bg->free_space_lock);
> +		return -ENOMEM;
> +	}
> +
> +	key.objectid = bg->start;
> +	key.type = 0;
> +	key.offset = 0;
> +
> +	space_root = btrfs_free_space_root(bg);
> +
> +	ret = btrfs_search_slot(trans, space_root, &key, path, 0, 0);
> +	if (ret < 0) {
> +		mutex_unlock(&bg->free_space_lock);
> +		goto out;
> +	}
> +
> +	ret = 0;
> +
> +	while (true) {
> +		leaf = path->nodes[0];
> +
> +		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> +
> +		if (found_key.objectid >= bg->start + bg->length)
> +			break;
> +
> +		if (found_key.type == BTRFS_FREE_SPACE_EXTENT_KEY) {
> +			if (num_space_runs != 0 &&
> +			    space_runs[num_space_runs - 1].end == found_key.objectid) {
> +				space_runs[num_space_runs - 1].end =
> +					found_key.objectid + found_key.offset;
> +			} else {
> +				BUG_ON(num_space_runs >= extent_count);
> +
> +				space_runs[num_space_runs].start = found_key.objectid;
> +				space_runs[num_space_runs].end =
> +					found_key.objectid + found_key.offset;
> +
> +				num_space_runs++;
> +			}
> +		} else if (found_key.type == BTRFS_FREE_SPACE_BITMAP_KEY) {
> +			void *bitmap;
> +			unsigned long offset;
> +			u32 data_size;
> +
> +			offset = btrfs_item_ptr_offset(leaf, path->slots[0]);
> +			data_size = btrfs_item_size(leaf, path->slots[0]);
> +
> +			if (data_size != 0) {
> +				bitmap = kmalloc(data_size, GFP_NOFS);
> +				if (!bitmap) {
> +					mutex_unlock(&bg->free_space_lock);
> +					ret = -ENOMEM;
> +					goto out;
> +				}
> +
> +				read_extent_buffer(leaf, bitmap, offset,
> +						   data_size);
> +
> +				parse_bitmap(fs_info->sectorsize, bitmap,
> +					     data_size * BITS_PER_BYTE,
> +					     found_key.objectid, space_runs,
> +					     &num_space_runs);
> +
> +				BUG_ON(num_space_runs > extent_count);
> +
> +				kfree(bitmap);
> +			}
> +		}
> +
> +		path->slots[0]++;
> +
> +		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> +			ret = btrfs_next_leaf(space_root, path);
> +			if (ret != 0) {
> +				if (ret == 1)
> +					ret = 0;
> +				break;
> +			}
> +			leaf = path->nodes[0];
> +		}
> +	}
> +
> +	btrfs_release_path(path);
> +
> +	mutex_unlock(&bg->free_space_lock);
> +
> +	max_entries = extent_count + 2;
> +	entries = kmalloc(sizeof(*entries) * max_entries, GFP_NOFS);
> +	if (!entries) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	num_entries = 0;
> +
> +	if (num_space_runs > 0 && space_runs[0].start > bg->start) {
> +		entries[num_entries].objectid = bg->start;
> +		entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
> +		entries[num_entries].offset = space_runs[0].start - bg->start;
> +		num_entries++;
> +	}
> +
> +	for (unsigned int i = 1; i < num_space_runs; i++) {
> +		entries[num_entries].objectid = space_runs[i - 1].end;
> +		entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
> +		entries[num_entries].offset =
> +			space_runs[i].start - space_runs[i - 1].end;
> +		num_entries++;
> +	}
> +
> +	if (num_space_runs == 0) {
> +		entries[num_entries].objectid = bg->start;
> +		entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
> +		entries[num_entries].offset = bg->length;
> +		num_entries++;
> +	} else if (space_runs[num_space_runs - 1].end < bg->start + bg->length) {
> +		entries[num_entries].objectid = space_runs[num_space_runs - 1].end;
> +		entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
> +		entries[num_entries].offset =
> +			bg->start + bg->length - space_runs[num_space_runs - 1].end;
> +		num_entries++;
> +	}
> +
> +	if (num_entries == 0)
> +		goto out;
> +
> +	bg->identity_remap_count = num_entries;
> +
> +	ret = add_remap_tree_entries(trans, path, entries, num_entries);

We can group the empty and non-empty space_runs cases into an if/else to make
the two main flows obvious and reduce scattered conditions:

 	num_entries = 0;

 	if (num_space_runs == 0) {
 		entries[num_entries].objectid = bg->start;
 		entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
 		entries[num_entries].offset = bg->length;
 		num_entries++;
 	} else {
 		if (space_runs[0].start > bg->start) {
 			entries[num_entries].objectid = bg->start;
 			entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
 			entries[num_entries].offset = space_runs[0].start - bg->start;
 			num_entries++;
 		}
 		for (unsigned int i = 1; i < num_space_runs; i++) {
 			entries[num_entries].objectid = space_runs[i - 1].end;
 			entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
 			entries[num_entries].offset =
 				space_runs[i].start - space_runs[i - 1].end;
 			num_entries++;
 		}
 		if (space_runs[num_space_runs - 1].end < bg->start + bg->length) {
 			entries[num_entries].objectid = space_runs[num_space_runs - 1].end;
 			entries[num_entries].type = BTRFS_IDENTITY_REMAP_KEY;
 			entries[num_entries].offset =
 				bg->start + bg->length - space_runs[num_space_runs - 1].end;
 			num_entries++;
 		}
 		if (num_entries == 0)
 			goto out;
 	}

	// I'm not sure if it's necessary but we can free space_runs earlier
	// since we're also doing allocation in add_remap_tree_entries().
	// kfree(space_runs);
	// space_runs = NULL;

 	bg->identity_remap_count = num_entries;

 	ret = add_remap_tree_entries(trans, path, entries, num_entries);


> +
> +out:
> +	kfree(entries);
> +	kfree(space_runs);
> +
> +	return ret;
> +}

Regards,
Sun Yangkai