[v2,2/7] btrfs-progs: Refactor btrfs_read_block_groups()
diff mbox series

Message ID 20191008044936.157873-3-wqu@suse.com
State New
Headers show
Series
  • btrfs-progs: Support for BG_TREE feature
Related show

Commit Message

Qu Wenruo Oct. 8, 2019, 4:49 a.m. UTC
This patch does the following refactor:
- Refactor parameter from @root to @fs_info

- Refactor the large loop body into another function
  Now we have a helper function, read_one_block_group(), to handle
  block group cache and space info related routine.

- Refactor the return value
  Even we have the code handling ret > 0 from find_first_block_group(),
  it never works, as when there is no more block group,
  find_first_block_group() just return -ENOENT other than 1.

  This is super confusing, it's almost a mircle it even works.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 ctree.h       |   2 +-
 disk-io.c     |   9 ++-
 extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
 3 files changed, 97 insertions(+), 74 deletions(-)

Comments

Anand Jain Oct. 17, 2019, 3:23 a.m. UTC | #1
On 10/8/19 12:49 PM, Qu Wenruo wrote:
> This patch does the following refactor:
> - Refactor parameter from @root to @fs_info
> 
> - Refactor the large loop body into another function
>    Now we have a helper function, read_one_block_group(), to handle
>    block group cache and space info related routine.
> 
> - Refactor the return value
>    Even we have the code handling ret > 0 from find_first_block_group(),
>    it never works, as when there is no more block group,
>    find_first_block_group() just return -ENOENT other than 1.


  Can it be separated into patches? My concern is as it alters the return
  value of the rescue command. So we shall have clarity of a discrete
  patch to blame. Otherwise I agree its a good change.


>    This is super confusing, it's almost a mircle it even works.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   ctree.h       |   2 +-
>   disk-io.c     |   9 ++-
>   extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
>   3 files changed, 97 insertions(+), 74 deletions(-)
> 
> diff --git a/ctree.h b/ctree.h
> index 8c7b3cb40151..2899de358613 100644
> --- a/ctree.h
> +++ b/ctree.h
> @@ -2550,7 +2550,7 @@ int update_space_info(struct btrfs_fs_info *info, u64 flags,
>   		      u64 total_bytes, u64 bytes_used,
>   		      struct btrfs_space_info **space_info);
>   int btrfs_free_block_groups(struct btrfs_fs_info *info);
> -int btrfs_read_block_groups(struct btrfs_root *root);
> +int btrfs_read_block_groups(struct btrfs_fs_info *info);
>   struct btrfs_block_group_cache *
>   btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used, u64 type,
>   		      u64 chunk_offset, u64 size);
> diff --git a/disk-io.c b/disk-io.c
> index be44eead5cef..8978f0cb60c7 100644
> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -983,14 +983,17 @@ int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
>   	fs_info->last_trans_committed = generation;
>   	if (extent_buffer_uptodate(fs_info->extent_root->node) &&
>   	    !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
> -		ret = btrfs_read_block_groups(fs_info->tree_root);
> +		ret = btrfs_read_block_groups(fs_info);
>   		/*
>   		 * If we don't find any blockgroups (ENOENT) we're either
>   		 * restoring or creating the filesystem, where it's expected,
>   		 * anything else is error
>   		 */
> -		if (ret != -ENOENT)
> -			return -EIO;
> +		if (ret < 0 && ret != -ENOENT) {
> +			errno = -ret;
> +			error("failed to read block groups: %m");
> +			return ret;
> +		}
>   	}


As mentioned this alters the rescue command semantics as show below.
Earlier we had only -EIO as error now its much more and accurate
which is good. fstests is fine but anything else?

cmd_rescue_chunk_recover()
   btrfs_recover_chunk_tree()
     open_ctree_with_broken_chunk()
       btrfs_setup_all_roots()

Thanks, Anand

>   	key.objectid = BTRFS_FS_TREE_OBJECTID;
> diff --git a/extent-tree.c b/extent-tree.c
> index 19d1ea0df570..9713d627764c 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -2607,6 +2607,13 @@ int btrfs_free_block_groups(struct btrfs_fs_info *info)
>   	return 0;
>   }
>   
> +/*
> + * Find a block group which starts >= @key->objectid in extent tree.
> + *
> + * Return 0 for found
> + * Retrun >0 for not found
> + * Return <0 for error
> + */
>   static int find_first_block_group(struct btrfs_root *root,
>   		struct btrfs_path *path, struct btrfs_key *key)
>   {
> @@ -2636,36 +2643,95 @@ static int find_first_block_group(struct btrfs_root *root,
>   			return 0;
>   		path->slots[0]++;
>   	}
> -	ret = -ENOENT;
> +	ret = 1;
>   error:
>   	return ret;
>   }
>   
> -int btrfs_read_block_groups(struct btrfs_root *root)
> +/*
> + * Helper function to read out one BLOCK_GROUP_ITEM and insert it into
> + * block group cache.
> + *
> + * Return 0 if nothing wrong (either insert the bg cache or skip 0 sized bg)
> + * Return <0 for error.
> + */
> +static int read_one_block_group(struct btrfs_fs_info *fs_info,
> +				 struct btrfs_path *path)
>   {
> -	struct btrfs_path *path;
> -	int ret;
> -	int bit;
> -	struct btrfs_block_group_cache *cache;
> -	struct btrfs_fs_info *info = root->fs_info;
> +	struct extent_io_tree *block_group_cache = &fs_info->block_group_cache;
> +	struct extent_buffer *leaf = path->nodes[0];
>   	struct btrfs_space_info *space_info;
> -	struct extent_io_tree *block_group_cache;
> +	struct btrfs_block_group_cache *cache;
>   	struct btrfs_key key;
> -	struct btrfs_key found_key;
> -	struct extent_buffer *leaf;
> +	int slot = path->slots[0];
> +	int bit = 0;
> +	int ret;
>   
> -	block_group_cache = &info->block_group_cache;
> +	btrfs_item_key_to_cpu(leaf, &key, slot);
> +	ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY);
> +
> +	/*
> +	 * Skip 0 sized block group, don't insert them into block
> +	 * group cache tree, as its length is 0, it won't get
> +	 * freed at close_ctree() time.
> +	 */
> +	if (key.offset == 0)
> +		return 0;
> +
> +	cache = kzalloc(sizeof(*cache), GFP_NOFS);
> +	if (!cache)
> +		return -ENOMEM;
> +	read_extent_buffer(leaf, &cache->item,
> +			   btrfs_item_ptr_offset(leaf, slot),
> +			   sizeof(cache->item));
> +	memcpy(&cache->key, &key, sizeof(key));
> +	cache->cached = 0;
> +	cache->pinned = 0;
> +	cache->flags = btrfs_block_group_flags(&cache->item);
> +	if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
> +		bit = BLOCK_GROUP_DATA;
> +	} else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> +		bit = BLOCK_GROUP_SYSTEM;
> +	} else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
> +		bit = BLOCK_GROUP_METADATA;
> +	}
> +	set_avail_alloc_bits(fs_info, cache->flags);
> +	if (btrfs_chunk_readonly(fs_info, cache->key.objectid))
> +		cache->ro = 1;
> +	exclude_super_stripes(fs_info, cache);
> +
> +	ret = update_space_info(fs_info, cache->flags, cache->key.offset,
> +				btrfs_block_group_used(&cache->item),
> +				&space_info);
> +	if (ret < 0) {
> +		free(cache);
> +		return ret;
> +	}
> +	cache->space_info = space_info;
> +
> +	set_extent_bits(block_group_cache, cache->key.objectid,
> +			cache->key.objectid + cache->key.offset - 1,
> +			bit | EXTENT_LOCKED);
> +	set_state_private(block_group_cache, cache->key.objectid,
> +			  (unsigned long)cache);
> +	return 0;
> +}
>   
> -	root = info->extent_root;
> +int btrfs_read_block_groups(struct btrfs_fs_info *fs_info)
> +{
> +	struct btrfs_path path;
> +	struct btrfs_root *root;
> +	int ret;
> +	struct btrfs_key key;
> +
> +	root = fs_info->extent_root;
>   	key.objectid = 0;
>   	key.offset = 0;
>   	key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
> -	path = btrfs_alloc_path();
> -	if (!path)
> -		return -ENOMEM;
> +	btrfs_init_path(&path);
>   
>   	while(1) {
> -		ret = find_first_block_group(root, path, &key);
> +		ret = find_first_block_group(root, &path, &key);
>   		if (ret > 0) {
>   			ret = 0;
>   			goto error;
> @@ -2673,67 +2739,21 @@ int btrfs_read_block_groups(struct btrfs_root *root)
>   		if (ret != 0) {
>   			goto error;
>   		}
> -		leaf = path->nodes[0];
> -		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
> +		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
>   
> -		cache = kzalloc(sizeof(*cache), GFP_NOFS);
> -		if (!cache) {
> -			ret = -ENOMEM;
> +		ret = read_one_block_group(fs_info, &path);
> +		if (ret < 0)
>   			goto error;
> -		}
>   
> -		read_extent_buffer(leaf, &cache->item,
> -				   btrfs_item_ptr_offset(leaf, path->slots[0]),
> -				   sizeof(cache->item));
> -		memcpy(&cache->key, &found_key, sizeof(found_key));
> -		cache->cached = 0;
> -		cache->pinned = 0;
> -		key.objectid = found_key.objectid + found_key.offset;
> -		if (found_key.offset == 0)
> +		if (key.offset == 0)
>   			key.objectid++;
> -		btrfs_release_path(path);
> -
> -		/*
> -		 * Skip 0 sized block group, don't insert them into block
> -		 * group cache tree, as its length is 0, it won't get
> -		 * freed at close_ctree() time.
> -		 */
> -		if (found_key.offset == 0) {
> -			free(cache);
> -			continue;
> -		}
> -
> -		cache->flags = btrfs_block_group_flags(&cache->item);
> -		bit = 0;
> -		if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
> -			bit = BLOCK_GROUP_DATA;
> -		} else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
> -			bit = BLOCK_GROUP_SYSTEM;
> -		} else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
> -			bit = BLOCK_GROUP_METADATA;
> -		}
> -		set_avail_alloc_bits(info, cache->flags);
> -		if (btrfs_chunk_readonly(info, cache->key.objectid))
> -			cache->ro = 1;
> -
> -		exclude_super_stripes(info, cache);
> -
> -		ret = update_space_info(info, cache->flags, found_key.offset,
> -					btrfs_block_group_used(&cache->item),
> -					&space_info);
> -		BUG_ON(ret);
> -		cache->space_info = space_info;
> -
> -		/* use EXTENT_LOCKED to prevent merging */
> -		set_extent_bits(block_group_cache, found_key.objectid,
> -				found_key.objectid + found_key.offset - 1,
> -				bit | EXTENT_LOCKED);
> -		set_state_private(block_group_cache, found_key.objectid,
> -				  (unsigned long)cache);
> +		else
> +			key.objectid = key.objectid + key.offset;
> +		btrfs_release_path(&path);
>   	}
>   	ret = 0;
>   error:
> -	btrfs_free_path(path);
> +	btrfs_release_path(&path);
>   	return ret;
>   }
>   
>
Qu Wenruo Oct. 17, 2019, 4:33 a.m. UTC | #2
On 2019/10/17 上午11:23, Anand Jain wrote:
> On 10/8/19 12:49 PM, Qu Wenruo wrote:
>> This patch does the following refactor:
>> - Refactor parameter from @root to @fs_info
>>
>> - Refactor the large loop body into another function
>>    Now we have a helper function, read_one_block_group(), to handle
>>    block group cache and space info related routine.
>>
>> - Refactor the return value
>>    Even we have the code handling ret > 0 from find_first_block_group(),
>>    it never works, as when there is no more block group,
>>    find_first_block_group() just return -ENOENT other than 1.
> 
> 
>  Can it be separated into patches? My concern is as it alters the return
>  value of the rescue command. So we shall have clarity of a discrete
>  patch to blame. Otherwise I agree its a good change.

No problem.

What about 3 patches split by the mentioned 3 refactors?
> 
> 
>>    This is super confusing, it's almost a mircle it even works.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   ctree.h       |   2 +-
>>   disk-io.c     |   9 ++-
>>   extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
>>   3 files changed, 97 insertions(+), 74 deletions(-)
>>
>> diff --git a/ctree.h b/ctree.h
>> index 8c7b3cb40151..2899de358613 100644
>> --- a/ctree.h
>> +++ b/ctree.h
>> @@ -2550,7 +2550,7 @@ int update_space_info(struct btrfs_fs_info
>> *info, u64 flags,
>>                 u64 total_bytes, u64 bytes_used,
>>                 struct btrfs_space_info **space_info);
>>   int btrfs_free_block_groups(struct btrfs_fs_info *info);
>> -int btrfs_read_block_groups(struct btrfs_root *root);
>> +int btrfs_read_block_groups(struct btrfs_fs_info *info);
>>   struct btrfs_block_group_cache *
>>   btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used,
>> u64 type,
>>                 u64 chunk_offset, u64 size);
>> diff --git a/disk-io.c b/disk-io.c
>> index be44eead5cef..8978f0cb60c7 100644
>> --- a/disk-io.c
>> +++ b/disk-io.c
>> @@ -983,14 +983,17 @@ int btrfs_setup_all_roots(struct btrfs_fs_info
>> *fs_info, u64 root_tree_bytenr,
>>       fs_info->last_trans_committed = generation;
>>       if (extent_buffer_uptodate(fs_info->extent_root->node) &&
>>           !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
>> -        ret = btrfs_read_block_groups(fs_info->tree_root);
>> +        ret = btrfs_read_block_groups(fs_info);
>>           /*
>>            * If we don't find any blockgroups (ENOENT) we're either
>>            * restoring or creating the filesystem, where it's expected,
>>            * anything else is error
>>            */
>> -        if (ret != -ENOENT)
>> -            return -EIO;
>> +        if (ret < 0 && ret != -ENOENT) {
>> +            errno = -ret;
>> +            error("failed to read block groups: %m");
>> +            return ret;
>> +        }
>>       }
> 
> 
> As mentioned this alters the rescue command semantics as show below.
> Earlier we had only -EIO as error now its much more and accurate
> which is good. fstests is fine but anything else?
> 
> cmd_rescue_chunk_recover()
>   btrfs_recover_chunk_tree()
>     open_ctree_with_broken_chunk()
>       btrfs_setup_all_roots()

I'm not sure if I got the point.

Although btrfs_setup_all_roots() get called in above call chain, it
doesn't have any special handling of -EIO or others.

It just reads the extent tree root.

Would you mind to explain a little more?

Thanks,
Qu


> 
> Thanks, Anand
> 
>>       key.objectid = BTRFS_FS_TREE_OBJECTID;
>> diff --git a/extent-tree.c b/extent-tree.c
>> index 19d1ea0df570..9713d627764c 100644
>> --- a/extent-tree.c
>> +++ b/extent-tree.c
>> @@ -2607,6 +2607,13 @@ int btrfs_free_block_groups(struct
>> btrfs_fs_info *info)
>>       return 0;
>>   }
>>   +/*
>> + * Find a block group which starts >= @key->objectid in extent tree.
>> + *
>> + * Return 0 for found
>> + * Retrun >0 for not found
>> + * Return <0 for error
>> + */
>>   static int find_first_block_group(struct btrfs_root *root,
>>           struct btrfs_path *path, struct btrfs_key *key)
>>   {
>> @@ -2636,36 +2643,95 @@ static int find_first_block_group(struct
>> btrfs_root *root,
>>               return 0;
>>           path->slots[0]++;
>>       }
>> -    ret = -ENOENT;
>> +    ret = 1;
>>   error:
>>       return ret;
>>   }
>>   -int btrfs_read_block_groups(struct btrfs_root *root)
>> +/*
>> + * Helper function to read out one BLOCK_GROUP_ITEM and insert it into
>> + * block group cache.
>> + *
>> + * Return 0 if nothing wrong (either insert the bg cache or skip 0
>> sized bg)
>> + * Return <0 for error.
>> + */
>> +static int read_one_block_group(struct btrfs_fs_info *fs_info,
>> +                 struct btrfs_path *path)
>>   {
>> -    struct btrfs_path *path;
>> -    int ret;
>> -    int bit;
>> -    struct btrfs_block_group_cache *cache;
>> -    struct btrfs_fs_info *info = root->fs_info;
>> +    struct extent_io_tree *block_group_cache =
>> &fs_info->block_group_cache;
>> +    struct extent_buffer *leaf = path->nodes[0];
>>       struct btrfs_space_info *space_info;
>> -    struct extent_io_tree *block_group_cache;
>> +    struct btrfs_block_group_cache *cache;
>>       struct btrfs_key key;
>> -    struct btrfs_key found_key;
>> -    struct extent_buffer *leaf;
>> +    int slot = path->slots[0];
>> +    int bit = 0;
>> +    int ret;
>>   -    block_group_cache = &info->block_group_cache;
>> +    btrfs_item_key_to_cpu(leaf, &key, slot);
>> +    ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY);
>> +
>> +    /*
>> +     * Skip 0 sized block group, don't insert them into block
>> +     * group cache tree, as its length is 0, it won't get
>> +     * freed at close_ctree() time.
>> +     */
>> +    if (key.offset == 0)
>> +        return 0;
>> +
>> +    cache = kzalloc(sizeof(*cache), GFP_NOFS);
>> +    if (!cache)
>> +        return -ENOMEM;
>> +    read_extent_buffer(leaf, &cache->item,
>> +               btrfs_item_ptr_offset(leaf, slot),
>> +               sizeof(cache->item));
>> +    memcpy(&cache->key, &key, sizeof(key));
>> +    cache->cached = 0;
>> +    cache->pinned = 0;
>> +    cache->flags = btrfs_block_group_flags(&cache->item);
>> +    if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
>> +        bit = BLOCK_GROUP_DATA;
>> +    } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>> +        bit = BLOCK_GROUP_SYSTEM;
>> +    } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
>> +        bit = BLOCK_GROUP_METADATA;
>> +    }
>> +    set_avail_alloc_bits(fs_info, cache->flags);
>> +    if (btrfs_chunk_readonly(fs_info, cache->key.objectid))
>> +        cache->ro = 1;
>> +    exclude_super_stripes(fs_info, cache);
>> +
>> +    ret = update_space_info(fs_info, cache->flags, cache->key.offset,
>> +                btrfs_block_group_used(&cache->item),
>> +                &space_info);
>> +    if (ret < 0) {
>> +        free(cache);
>> +        return ret;
>> +    }
>> +    cache->space_info = space_info;
>> +
>> +    set_extent_bits(block_group_cache, cache->key.objectid,
>> +            cache->key.objectid + cache->key.offset - 1,
>> +            bit | EXTENT_LOCKED);
>> +    set_state_private(block_group_cache, cache->key.objectid,
>> +              (unsigned long)cache);
>> +    return 0;
>> +}
>>   -    root = info->extent_root;
>> +int btrfs_read_block_groups(struct btrfs_fs_info *fs_info)
>> +{
>> +    struct btrfs_path path;
>> +    struct btrfs_root *root;
>> +    int ret;
>> +    struct btrfs_key key;
>> +
>> +    root = fs_info->extent_root;
>>       key.objectid = 0;
>>       key.offset = 0;
>>       key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
>> -    path = btrfs_alloc_path();
>> -    if (!path)
>> -        return -ENOMEM;
>> +    btrfs_init_path(&path);
>>         while(1) {
>> -        ret = find_first_block_group(root, path, &key);
>> +        ret = find_first_block_group(root, &path, &key);
>>           if (ret > 0) {
>>               ret = 0;
>>               goto error;
>> @@ -2673,67 +2739,21 @@ int btrfs_read_block_groups(struct btrfs_root
>> *root)
>>           if (ret != 0) {
>>               goto error;
>>           }
>> -        leaf = path->nodes[0];
>> -        btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
>> +        btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
>>   -        cache = kzalloc(sizeof(*cache), GFP_NOFS);
>> -        if (!cache) {
>> -            ret = -ENOMEM;
>> +        ret = read_one_block_group(fs_info, &path);
>> +        if (ret < 0)
>>               goto error;
>> -        }
>>   -        read_extent_buffer(leaf, &cache->item,
>> -                   btrfs_item_ptr_offset(leaf, path->slots[0]),
>> -                   sizeof(cache->item));
>> -        memcpy(&cache->key, &found_key, sizeof(found_key));
>> -        cache->cached = 0;
>> -        cache->pinned = 0;
>> -        key.objectid = found_key.objectid + found_key.offset;
>> -        if (found_key.offset == 0)
>> +        if (key.offset == 0)
>>               key.objectid++;
>> -        btrfs_release_path(path);
>> -
>> -        /*
>> -         * Skip 0 sized block group, don't insert them into block
>> -         * group cache tree, as its length is 0, it won't get
>> -         * freed at close_ctree() time.
>> -         */
>> -        if (found_key.offset == 0) {
>> -            free(cache);
>> -            continue;
>> -        }
>> -
>> -        cache->flags = btrfs_block_group_flags(&cache->item);
>> -        bit = 0;
>> -        if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
>> -            bit = BLOCK_GROUP_DATA;
>> -        } else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
>> -            bit = BLOCK_GROUP_SYSTEM;
>> -        } else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
>> -            bit = BLOCK_GROUP_METADATA;
>> -        }
>> -        set_avail_alloc_bits(info, cache->flags);
>> -        if (btrfs_chunk_readonly(info, cache->key.objectid))
>> -            cache->ro = 1;
>> -
>> -        exclude_super_stripes(info, cache);
>> -
>> -        ret = update_space_info(info, cache->flags, found_key.offset,
>> -                    btrfs_block_group_used(&cache->item),
>> -                    &space_info);
>> -        BUG_ON(ret);
>> -        cache->space_info = space_info;
>> -
>> -        /* use EXTENT_LOCKED to prevent merging */
>> -        set_extent_bits(block_group_cache, found_key.objectid,
>> -                found_key.objectid + found_key.offset - 1,
>> -                bit | EXTENT_LOCKED);
>> -        set_state_private(block_group_cache, found_key.objectid,
>> -                  (unsigned long)cache);
>> +        else
>> +            key.objectid = key.objectid + key.offset;
>> +        btrfs_release_path(&path);
>>       }
>>       ret = 0;
>>   error:
>> -    btrfs_free_path(path);
>> +    btrfs_release_path(&path);
>>       return ret;
>>   }
>>  
>
Anand Jain Oct. 17, 2019, 5:08 a.m. UTC | #3
On 10/17/19 12:33 PM, Qu Wenruo wrote:
> 
> 
> On 2019/10/17 上午11:23, Anand Jain wrote:
>> On 10/8/19 12:49 PM, Qu Wenruo wrote:
>>> This patch does the following refactor:
>>> - Refactor parameter from @root to @fs_info
>>>
>>> - Refactor the large loop body into another function
>>>     Now we have a helper function, read_one_block_group(), to handle
>>>     block group cache and space info related routine.
>>>
>>> - Refactor the return value
>>>     Even we have the code handling ret > 0 from find_first_block_group(),
>>>     it never works, as when there is no more block group,
>>>     find_first_block_group() just return -ENOENT other than 1.
>>
>>
>>   Can it be separated into patches? My concern is as it alters the return
>>   value of the rescue command. So we shall have clarity of a discrete
>>   patch to blame. Otherwise I agree its a good change.
> 
> No problem.
> 
> What about 3 patches split by the mentioned 3 refactors?
>>
>>
>>>     This is super confusing, it's almost a mircle it even works.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>    ctree.h       |   2 +-
>>>    disk-io.c     |   9 ++-
>>>    extent-tree.c | 160 ++++++++++++++++++++++++++++----------------------
>>>    3 files changed, 97 insertions(+), 74 deletions(-)
>>>
>>> diff --git a/ctree.h b/ctree.h
>>> index 8c7b3cb40151..2899de358613 100644
>>> --- a/ctree.h
>>> +++ b/ctree.h
>>> @@ -2550,7 +2550,7 @@ int update_space_info(struct btrfs_fs_info
>>> *info, u64 flags,
>>>                  u64 total_bytes, u64 bytes_used,
>>>                  struct btrfs_space_info **space_info);
>>>    int btrfs_free_block_groups(struct btrfs_fs_info *info);
>>> -int btrfs_read_block_groups(struct btrfs_root *root);
>>> +int btrfs_read_block_groups(struct btrfs_fs_info *info);
>>>    struct btrfs_block_group_cache *
>>>    btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used,
>>> u64 type,
>>>                  u64 chunk_offset, u64 size);
>>> diff --git a/disk-io.c b/disk-io.c
>>> index be44eead5cef..8978f0cb60c7 100644
>>> --- a/disk-io.c
>>> +++ b/disk-io.c
>>> @@ -983,14 +983,17 @@ int btrfs_setup_all_roots(struct btrfs_fs_info
>>> *fs_info, u64 root_tree_bytenr,
>>>        fs_info->last_trans_committed = generation;
>>>        if (extent_buffer_uptodate(fs_info->extent_root->node) &&
>>>            !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
>>> -        ret = btrfs_read_block_groups(fs_info->tree_root);
>>> +        ret = btrfs_read_block_groups(fs_info);
>>>            /*
>>>             * If we don't find any blockgroups (ENOENT) we're either
>>>             * restoring or creating the filesystem, where it's expected,
>>>             * anything else is error
>>>             */
>>> -        if (ret != -ENOENT)
>>> -            return -EIO;
>>> +        if (ret < 0 && ret != -ENOENT) {
>>> +            errno = -ret;
>>> +            error("failed to read block groups: %m");
>>> +            return ret;
>>> +        }
>>>        }
>>
>>
>> As mentioned this alters the rescue command semantics as show below.
>> Earlier we had only -EIO as error now its much more and accurate
>> which is good. fstests is fine but anything else?
>>
>> cmd_rescue_chunk_recover()
>>    btrfs_recover_chunk_tree()
>>      open_ctree_with_broken_chunk()
>>        btrfs_setup_all_roots()
> 
> I'm not sure if I got the point.
> 
> Although btrfs_setup_all_roots() get called in above call chain, it
> doesn't have any special handling of -EIO or others.
> 
> It just reads the extent tree root.
> 
> Would you mind to explain a little more?

  sure.
  The above thread is in the call chain of the command
     btrfs rescue chunk-recover [options] <device>"

  And as the its return error code is being changed for the same problem,
  so a separate patch not part of the bg-tree changes would make sense.

Thanks, Anand

Patch
diff mbox series

diff --git a/ctree.h b/ctree.h
index 8c7b3cb40151..2899de358613 100644
--- a/ctree.h
+++ b/ctree.h
@@ -2550,7 +2550,7 @@  int update_space_info(struct btrfs_fs_info *info, u64 flags,
 		      u64 total_bytes, u64 bytes_used,
 		      struct btrfs_space_info **space_info);
 int btrfs_free_block_groups(struct btrfs_fs_info *info);
-int btrfs_read_block_groups(struct btrfs_root *root);
+int btrfs_read_block_groups(struct btrfs_fs_info *info);
 struct btrfs_block_group_cache *
 btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used, u64 type,
 		      u64 chunk_offset, u64 size);
diff --git a/disk-io.c b/disk-io.c
index be44eead5cef..8978f0cb60c7 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -983,14 +983,17 @@  int btrfs_setup_all_roots(struct btrfs_fs_info *fs_info, u64 root_tree_bytenr,
 	fs_info->last_trans_committed = generation;
 	if (extent_buffer_uptodate(fs_info->extent_root->node) &&
 	    !(flags & OPEN_CTREE_NO_BLOCK_GROUPS)) {
-		ret = btrfs_read_block_groups(fs_info->tree_root);
+		ret = btrfs_read_block_groups(fs_info);
 		/*
 		 * If we don't find any blockgroups (ENOENT) we're either
 		 * restoring or creating the filesystem, where it's expected,
 		 * anything else is error
 		 */
-		if (ret != -ENOENT)
-			return -EIO;
+		if (ret < 0 && ret != -ENOENT) {
+			errno = -ret;
+			error("failed to read block groups: %m");
+			return ret;
+		}
 	}
 
 	key.objectid = BTRFS_FS_TREE_OBJECTID;
diff --git a/extent-tree.c b/extent-tree.c
index 19d1ea0df570..9713d627764c 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2607,6 +2607,13 @@  int btrfs_free_block_groups(struct btrfs_fs_info *info)
 	return 0;
 }
 
+/*
+ * Find a block group which starts >= @key->objectid in extent tree.
+ *
+ * Return 0 for found
+ * Retrun >0 for not found
+ * Return <0 for error
+ */
 static int find_first_block_group(struct btrfs_root *root,
 		struct btrfs_path *path, struct btrfs_key *key)
 {
@@ -2636,36 +2643,95 @@  static int find_first_block_group(struct btrfs_root *root,
 			return 0;
 		path->slots[0]++;
 	}
-	ret = -ENOENT;
+	ret = 1;
 error:
 	return ret;
 }
 
-int btrfs_read_block_groups(struct btrfs_root *root)
+/*
+ * Helper function to read out one BLOCK_GROUP_ITEM and insert it into
+ * block group cache.
+ *
+ * Return 0 if nothing wrong (either insert the bg cache or skip 0 sized bg)
+ * Return <0 for error.
+ */
+static int read_one_block_group(struct btrfs_fs_info *fs_info,
+				 struct btrfs_path *path)
 {
-	struct btrfs_path *path;
-	int ret;
-	int bit;
-	struct btrfs_block_group_cache *cache;
-	struct btrfs_fs_info *info = root->fs_info;
+	struct extent_io_tree *block_group_cache = &fs_info->block_group_cache;
+	struct extent_buffer *leaf = path->nodes[0];
 	struct btrfs_space_info *space_info;
-	struct extent_io_tree *block_group_cache;
+	struct btrfs_block_group_cache *cache;
 	struct btrfs_key key;
-	struct btrfs_key found_key;
-	struct extent_buffer *leaf;
+	int slot = path->slots[0];
+	int bit = 0;
+	int ret;
 
-	block_group_cache = &info->block_group_cache;
+	btrfs_item_key_to_cpu(leaf, &key, slot);
+	ASSERT(key.type == BTRFS_BLOCK_GROUP_ITEM_KEY);
+
+	/*
+	 * Skip 0 sized block group, don't insert them into block
+	 * group cache tree, as its length is 0, it won't get
+	 * freed at close_ctree() time.
+	 */
+	if (key.offset == 0)
+		return 0;
+
+	cache = kzalloc(sizeof(*cache), GFP_NOFS);
+	if (!cache)
+		return -ENOMEM;
+	read_extent_buffer(leaf, &cache->item,
+			   btrfs_item_ptr_offset(leaf, slot),
+			   sizeof(cache->item));
+	memcpy(&cache->key, &key, sizeof(key));
+	cache->cached = 0;
+	cache->pinned = 0;
+	cache->flags = btrfs_block_group_flags(&cache->item);
+	if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
+		bit = BLOCK_GROUP_DATA;
+	} else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
+		bit = BLOCK_GROUP_SYSTEM;
+	} else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
+		bit = BLOCK_GROUP_METADATA;
+	}
+	set_avail_alloc_bits(fs_info, cache->flags);
+	if (btrfs_chunk_readonly(fs_info, cache->key.objectid))
+		cache->ro = 1;
+	exclude_super_stripes(fs_info, cache);
+
+	ret = update_space_info(fs_info, cache->flags, cache->key.offset,
+				btrfs_block_group_used(&cache->item),
+				&space_info);
+	if (ret < 0) {
+		free(cache);
+		return ret;
+	}
+	cache->space_info = space_info;
+
+	set_extent_bits(block_group_cache, cache->key.objectid,
+			cache->key.objectid + cache->key.offset - 1,
+			bit | EXTENT_LOCKED);
+	set_state_private(block_group_cache, cache->key.objectid,
+			  (unsigned long)cache);
+	return 0;
+}
 
-	root = info->extent_root;
+int btrfs_read_block_groups(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_path path;
+	struct btrfs_root *root;
+	int ret;
+	struct btrfs_key key;
+
+	root = fs_info->extent_root;
 	key.objectid = 0;
 	key.offset = 0;
 	key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-	path = btrfs_alloc_path();
-	if (!path)
-		return -ENOMEM;
+	btrfs_init_path(&path);
 
 	while(1) {
-		ret = find_first_block_group(root, path, &key);
+		ret = find_first_block_group(root, &path, &key);
 		if (ret > 0) {
 			ret = 0;
 			goto error;
@@ -2673,67 +2739,21 @@  int btrfs_read_block_groups(struct btrfs_root *root)
 		if (ret != 0) {
 			goto error;
 		}
-		leaf = path->nodes[0];
-		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
+		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
 
-		cache = kzalloc(sizeof(*cache), GFP_NOFS);
-		if (!cache) {
-			ret = -ENOMEM;
+		ret = read_one_block_group(fs_info, &path);
+		if (ret < 0)
 			goto error;
-		}
 
-		read_extent_buffer(leaf, &cache->item,
-				   btrfs_item_ptr_offset(leaf, path->slots[0]),
-				   sizeof(cache->item));
-		memcpy(&cache->key, &found_key, sizeof(found_key));
-		cache->cached = 0;
-		cache->pinned = 0;
-		key.objectid = found_key.objectid + found_key.offset;
-		if (found_key.offset == 0)
+		if (key.offset == 0)
 			key.objectid++;
-		btrfs_release_path(path);
-
-		/*
-		 * Skip 0 sized block group, don't insert them into block
-		 * group cache tree, as its length is 0, it won't get
-		 * freed at close_ctree() time.
-		 */
-		if (found_key.offset == 0) {
-			free(cache);
-			continue;
-		}
-
-		cache->flags = btrfs_block_group_flags(&cache->item);
-		bit = 0;
-		if (cache->flags & BTRFS_BLOCK_GROUP_DATA) {
-			bit = BLOCK_GROUP_DATA;
-		} else if (cache->flags & BTRFS_BLOCK_GROUP_SYSTEM) {
-			bit = BLOCK_GROUP_SYSTEM;
-		} else if (cache->flags & BTRFS_BLOCK_GROUP_METADATA) {
-			bit = BLOCK_GROUP_METADATA;
-		}
-		set_avail_alloc_bits(info, cache->flags);
-		if (btrfs_chunk_readonly(info, cache->key.objectid))
-			cache->ro = 1;
-
-		exclude_super_stripes(info, cache);
-
-		ret = update_space_info(info, cache->flags, found_key.offset,
-					btrfs_block_group_used(&cache->item),
-					&space_info);
-		BUG_ON(ret);
-		cache->space_info = space_info;
-
-		/* use EXTENT_LOCKED to prevent merging */
-		set_extent_bits(block_group_cache, found_key.objectid,
-				found_key.objectid + found_key.offset - 1,
-				bit | EXTENT_LOCKED);
-		set_state_private(block_group_cache, found_key.objectid,
-				  (unsigned long)cache);
+		else
+			key.objectid = key.objectid + key.offset;
+		btrfs_release_path(&path);
 	}
 	ret = 0;
 error:
-	btrfs_free_path(path);
+	btrfs_release_path(&path);
 	return ret;
 }