[PATCH] btrfs: fix btrfs_read_block_groups return value

[PATCH] btrfs: fix btrfs_read_block_groups return value

[Jeff Mahoney]

 btrfs_read_block_groups returns an ambiguous value. Whether it finds
 a block group or not, it will return -ENOENT. find_first_block_group
 will eventually return -ENOENT when it reaches past the last block
 group, and that is what is returned to the caller.

 Also, if the kzalloc fails, it will return 0.

 None of this matters right now because open_ctree isn't checking
 the return value, but I have a patch to handle that as well.

 This patch returns 0 if find_first_block_group after it has already
 found at least one block group, and -ENOENT if it has found none. It
 also returns -ENOMEM if the kzalloc fails.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6106,7 +6106,7 @@ int btrfs_free_block_groups(struct btrfs
 int btrfs_read_block_groups(struct btrfs_root *root)
 {
 	struct btrfs_path *path;
-	int ret;
+	int ret, found = 0;
 	struct btrfs_block_group_cache *cache;
 	struct btrfs_fs_info *info = root->fs_info;
 	struct btrfs_space_info *space_info;
@@ -6124,10 +6124,11 @@ int btrfs_read_block_groups(struct btrfs
 
 	while (1) {
 		ret = find_first_block_group(root, path, &key);
-		if (ret > 0) {
+		if (ret > 0 || (found && ret == -ENOENT)) {
 			ret = 0;
 			goto error;
 		}
+
 		if (ret != 0)
 			goto error;
 
@@ -6136,7 +6137,7 @@ int btrfs_read_block_groups(struct btrfs
 		cache = kzalloc(sizeof(*cache), GFP_NOFS);
 		if (!cache) {
 			ret = -ENOMEM;
-			break;
+			goto error;
 		}
 
 		atomic_set(&cache->count, 1);
@@ -6168,6 +6169,7 @@ int btrfs_read_block_groups(struct btrfs
 		set_avail_alloc_bits(root->fs_info, cache->flags);
 		if (btrfs_chunk_readonly(root, cache->key.objectid))
 			set_block_group_readonly(cache);
+		found = 1;
 	}
 	ret = 0;
 error:
-- 
Jeff Mahoney
SUSE Labs

[PATCH #2] btrfs: fix btrfs_read_block_groups return value

[Jeff Mahoney]

 btrfs_read_block_groups returns an ambiguous value. Whether it finds
 a block group or not, it will return -ENOENT. find_first_block_group
 will eventually return -ENOENT when it reaches past the last block
 group, and that is what is returned to the caller.

 Also, if the kzalloc fails, it will return 0.

 None of this matters right now because open_ctree() isn't checking
 the return value, but I have a patch to handle that as well.

 This patch returns 0 if find_first_block_group after it has already
 found at least one block group, and -ENOENT if it has found none. It
 also returns -ENOMEM if the kzalloc fails.

 Take 2: After I wrote this analysis, I realized the ret = 0 after the
 loop is never going to be reached and then the error label is redundant.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 fs/btrfs/extent-tree.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6106,7 +6106,7 @@ int btrfs_free_block_groups(struct btrfs
 int btrfs_read_block_groups(struct btrfs_root *root)
 {
 	struct btrfs_path *path;
-	int ret;
+	int ret, found = 0;
 	struct btrfs_block_group_cache *cache;
 	struct btrfs_fs_info *info = root->fs_info;
 	struct btrfs_space_info *space_info;
@@ -6124,12 +6124,13 @@ int btrfs_read_block_groups(struct btrfs
 
 	while (1) {
 		ret = find_first_block_group(root, path, &key);
-		if (ret > 0) {
+		if (ret > 0 || (found && ret == -ENOENT)) {
 			ret = 0;
-			goto error;
+			break;
 		}
+
 		if (ret != 0)
-			goto error;
+			break;
 
 		leaf = path->nodes[0];
 		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
@@ -6168,9 +6169,8 @@ int btrfs_read_block_groups(struct btrfs
 		set_avail_alloc_bits(root->fs_info, cache->flags);
 		if (btrfs_chunk_readonly(root, cache->key.objectid))
 			set_block_group_readonly(cache);
+		found = 1;
 	}
-	ret = 0;
-error:
 	btrfs_free_path(path);
 	return ret;
 }

Re: [PATCH] btrfs: fix btrfs_read_block_groups return value

[Chris Mason]

On Wed, 2009-02-11 at 20:51 -0500, Jeff Mahoney wrote:
> btrfs_read_block_groups returns an ambiguous value. Whether it finds
>  a block group or not, it will return -ENOENT. find_first_block_group
>  will eventually return -ENOENT when it reaches past the last block
>  group, and that is what is returned to the caller.

Jeff, thanks for starting on all of this.  Could I talk you into setting
up a git tree?  I think you're going to have a long stream of patches
and merging them with my trees is going to be much easier over git.

-chris

Re: [PATCH] btrfs: fix btrfs_read_block_groups return value

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Chris Mason wrote:
> On Wed, 2009-02-11 at 20:51 -0500, Jeff Mahoney wrote:
>> btrfs_read_block_groups returns an ambiguous value. Whether it finds
>>  a block group or not, it will return -ENOENT. find_first_block_group
>>  will eventually return -ENOENT when it reaches past the last block
>>  group, and that is what is returned to the caller.
> 
> Jeff, thanks for starting on all of this.  Could I talk you into setting
> up a git tree?  I think you're going to have a long stream of patches
> and merging them with my trees is going to be much easier over git.

Sure. I'm operating on a clone of your tree right now anyway. It
shouldn't be difficult to find a way to publish it.

- -Jeff

- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.9 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iEYEARECAAYFAkmUOCUACgkQLPWxlyuTD7JqjwCfRCF2+Bwv/BUJoFW85qFizxSS
KtAAnRpvLy5i4qpWduGwpv8lBlgF7xKn
=4ZAW
-----END PGP SIGNATURE-----