Re: [PATCH 4/5] btrfs-progs: image: Rebuild dev extents using chunk tree

[Nikolay Borisov <nborisov@suse.com>]

On 27.11.18 г. 9:30 ч., Qu Wenruo wrote:
> 
> 
> On 2018/11/27 下午3:28, Nikolay Borisov wrote:
>>
>>
>> On 27.11.18 г. 4:33 ч., Qu Wenruo wrote:
>>> With existing dev extents cleaned up, now we can rebuild dev extents
>>> using the correct chunk tree.
>>>
>>> Since new dev extents are all rebuild from scratch, even we're restoring
>>> image from multi-device fs to single disk, we won't have any problem
>>> reported by btrfs check.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  image/main.c | 34 ++++++++++++++++++++++++++++++++++
>>>  volumes.c    | 10 +++++-----
>>>  volumes.h    |  4 ++++
>>>  3 files changed, 43 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/image/main.c b/image/main.c
>>> index 707568f22e01..626eb933d5cc 100644
>>> --- a/image/main.c
>>> +++ b/image/main.c
>>> @@ -2265,12 +2265,46 @@ out:
>>>  static int fixup_dev_extents(struct btrfs_trans_handle *trans,
>>>  			     struct btrfs_fs_info *fs_info)
>>>  {
>>> +	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
>>> +	struct btrfs_device *dev;
>>> +	struct cache_extent *ce;
>>> +	struct map_lookup *map;
>>> +	u64 devid = btrfs_stack_device_id(&fs_info->super_copy->dev_item);
>>> +	int i;
>>>  	int ret;
>>>  
>>>  	ret = remove_all_dev_extents(trans, fs_info);
>>>  	if (ret < 0)
>>>  		error("failed to remove all existing dev extents: %s",
>>>  			strerror(-ret));
>>> +
>>> +	dev = btrfs_find_device(fs_info, devid, NULL, NULL);
>>> +	if (!dev) {
>>> +		error("faild to find devid %llu", devid);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	/* Rebuild all dev extents using chunk maps */
>>> +	for (ce = search_cache_extent(&map_tree->cache_tree, 0); ce;
>>> +	     ce = next_cache_extent(ce)) {
>>> +		u64 stripe_len;
>>> +
>>> +		map = container_of(ce, struct map_lookup, ce);
>>> +		stripe_len = calc_stripe_length(map->type, ce->size,
>>> +						map->num_stripes);
>>> +		for (i = 0; i < map->num_stripes; i++) {
>>> +			ret = btrfs_alloc_dev_extent(trans, dev, ce->start,
>>> +				stripe_len, &map->stripes[i].physical, 1);
>>> +			if (ret < 0) {
>>> +				error(
>>> +				"failed to insert dev extent %llu %llu: %s",
>>> +					devid, map->stripes[i].physical,
>>> +					strerror(-ret));
>>> +				goto out;
>>> +			}
>>> +		}
>>> +	}
>>> +out:
>>>  	return ret;
>>>  }
>>>  
>>> diff --git a/volumes.c b/volumes.c
>>> index 30090ce5f8e8..73c9204fa7d1 100644
>>> --- a/volumes.c
>>> +++ b/volumes.c
>>> @@ -530,10 +530,10 @@ static int find_free_dev_extent(struct btrfs_device *device, u64 num_bytes,
>>>  	return find_free_dev_extent_start(device, num_bytes, 0, start, len);
>>>  }
>>>  
>>> -static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>> -				  struct btrfs_device *device,
>>> -				  u64 chunk_offset, u64 num_bytes, u64 *start,
>>> -				  int convert)
>>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>> +			   struct btrfs_device *device,
>>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>>> +			   int insert_asis)
>>
>> Make that parameter a bool. Also why do you rename it ?
> 
> Since it's no longer only used by convert.
> 
> The best naming may be two function, one called
> btrfs_insert_device_extent(), and then btrfs_alloc_device_extent().
> 
> As for convert and this use case, we are not allocating, but just
> inserting one.
> 
> What about above naming change?

Indeed  two functions seem better, btrfs_alloc_device_extent will be a
wrapper of btrfs_insert_device_extent + the added find_free_dev_extent,
whereas btrfs_insert_device_extent will be the function doing the actual
insert.

> 
> Thanks,
> Qu
> 
>>
>>>  {
>>>  	int ret;
>>>  	struct btrfs_path *path;
>>> @@ -550,7 +550,7 @@ static int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>>  	 * For convert case, just skip search free dev_extent, as caller
>>>  	 * is responsible to make sure it's free.
>>>  	 */
>>> -	if (!convert) {
>>> +	if (!insert_asis) {
>>>  		ret = find_free_dev_extent(device, num_bytes, start, NULL);
>>>  		if (ret)
>>>  			goto err;
>>> diff --git a/volumes.h b/volumes.h
>>> index b4ea93f0bec3..5ca2779ebd45 100644
>>> --- a/volumes.h
>>> +++ b/volumes.h
>>> @@ -271,6 +271,10 @@ void btrfs_close_all_devices(void);
>>>  int btrfs_add_device(struct btrfs_trans_handle *trans,
>>>  		     struct btrfs_fs_info *fs_info,
>>>  		     struct btrfs_device *device);
>>> +int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
>>> +			   struct btrfs_device *device,
>>> +			   u64 chunk_offset, u64 num_bytes, u64 *start,
>>> +			   int insert_asis);
>>>  int btrfs_update_device(struct btrfs_trans_handle *trans,
>>>  			struct btrfs_device *device);
>>>  int btrfs_scan_one_device(int fd, const char *path,
>>>
>