Linux-BTRFS Archive on lore.kernel.org
 help / color / Atom feed
From: Anand Jain <anand.jain@oracle.com>
To: Qu Wenruo <wqu@suse.com>, linux-btrfs@vger.kernel.org
Subject: Re: [PATCH v2 2/7] btrfs-progs: Refactor btrfs_read_block_groups()
Date: Thu, 17 Oct 2019 11:23:48 +0800
Message-ID: <fd4f3e1c-fd98-6550-6284-f1456e0332ba@oracle.com> (raw)
In-Reply-To: <20191008044936.157873-3-wqu@suse.com>

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;
>   }
>   
> 


  reply index

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-08  4:49 [PATCH v2 0/7] btrfs-progs: Support for BG_TREE feature Qu Wenruo
2019-10-08  4:49 ` [PATCH v2 1/7] btrfs-progs: Refactor excluded extent functions to use fs_info Qu Wenruo
2019-10-08  9:22   ` Johannes Thumshirn
2019-10-17  2:16   ` Anand Jain
2019-10-08  4:49 ` [PATCH v2 2/7] btrfs-progs: Refactor btrfs_read_block_groups() Qu Wenruo
2019-10-17  3:23   ` Anand Jain [this message]
2019-10-17  4:33     ` Qu Wenruo
2019-10-17  5:08       ` Anand Jain
2019-10-08  4:49 ` [PATCH v2 3/7] btrfs-progs: Enable read-write ability for 'bg_tree' feature Qu Wenruo
2019-10-17  4:56   ` Anand Jain
2019-10-08  4:49 ` [PATCH v2 4/7] btrfs-progs: mkfs: Introduce -O bg-tree Qu Wenruo
2019-10-08  8:16   ` [PATCH v2.1 " Qu Wenruo
2019-10-08  4:49 ` [PATCH v2 5/7] btrfs-progs: dump-tree/dump-super: Introduce support for bg tree Qu Wenruo
2019-10-08  4:49 ` [PATCH v2 6/7] btrfs-progs: check: Introduce support for bg-tree feature Qu Wenruo
2019-10-08  4:49 ` [PATCH v2 7/7] btrfs-progs: btrfstune: Allow to enable bg-tree feature offline Qu Wenruo
2019-10-17  4:17   ` Anand Jain
2019-10-17  4:28     ` Qu Wenruo
2019-10-14 15:17 ` [PATCH v2 0/7] btrfs-progs: Support for BG_TREE feature David Sterba
2019-10-15  0:32   ` Qu Wenruo
2019-10-16 11:16     ` David Sterba
2019-10-16 11:19       ` Qu WenRuo
2019-10-18 17:27         ` David Sterba
2019-10-19  0:04           ` Qu Wenruo
2019-10-21 15:44             ` David Sterba
2019-10-22  0:49               ` Qu Wenruo
2019-10-22  6:30                 ` Qu Wenruo
2019-10-22 12:23                   ` David Sterba
2019-10-22 12:27                     ` Qu Wenruo

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fd4f3e1c-fd98-6550-6284-f1456e0332ba@oracle.com \
    --to=anand.jain@oracle.com \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=wqu@suse.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-BTRFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-btrfs/0 linux-btrfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-btrfs linux-btrfs/ https://lore.kernel.org/linux-btrfs \
		linux-btrfs@vger.kernel.org
	public-inbox-index linux-btrfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-btrfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git