[PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Qu Wenruo]

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
Which is based on v5.0-rc7

There are 5 extent buffer alloc functions in btrfs:
__alloc_extent_buffer();
alloc_extent_buffer();
__alloc_dummy_extent_buffer();
alloc_dummy_extent_buffer();
alloc_test_extent_buffer();

However their return value is not unified for failure mode:
__alloc_extent_buffer()		Never fail
alloc_extent_buffer()		PTR_ERR()
__alloc_dummy_extent_buffer()	NULL
alloc_dummy_extent_buffer()	NULL
alloc_test_extent_buffer()	NULL

This causes some wrapper function to have 2 failure modes, like
btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
its failure.

This inconsistent behavior is making static checker and reader crazy.

This patchset will unify the failure more of above 5 functions to
PTR_ERR().

Qu Wenruo (5):
  btrfs: extent_io: Add comment about the return value of
    alloc_extent_buffer()
  btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
    with alloc_extent_buffer()
  btrfs: extent_io: Unify the return value of
    alloc_dummy_extent_buffer() with alloc_extent_buffer()
  btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
    with alloc_extent_buffer()
  btrfs: extent_io: Unify the return value of
    btrfs_clone_extent_buffer() with alloc_extent_buffer()

 fs/btrfs/backref.c                     |  8 ++--
 fs/btrfs/ctree.c                       | 16 ++++----
 fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
 fs/btrfs/qgroup.c                      |  5 ++-
 fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
 fs/btrfs/tests/extent-io-tests.c       |  4 +-
 fs/btrfs/tests/free-space-tree-tests.c |  3 +-
 fs/btrfs/tests/inode-tests.c           |  6 ++-
 fs/btrfs/tests/qgroup-tests.c          |  3 +-
 9 files changed, 68 insertions(+), 39 deletions(-)

-- 
2.20.1

[PATCH 1/5] btrfs: extent_io: Add comment about the return value of alloc_extent_buffer()

[Qu Wenruo]

To inform later developers how to check the return value of it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 52abe4082680..b28a75546700 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4890,6 +4890,13 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 }
 #endif
 
+/*
+ * Allocate an extent buffer structure, with all its pages attached and locked.
+ *
+ * Return valid pointer if nothing goes wrong.
+ * Return PTR_ERR() if failed.
+ * Will never return NULL.
+ */
 struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 					  u64 start)
 {
-- 
2.20.1

[PATCH 2/5] btrfs: extent_io: Unify the return value of __alloc_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

Although __alloc_extent_buffer() shouldn't fail due to its __GFP_NOFAIL
flag, to unify the return value type, let's pretend it will return
PTR_ERR() and never return NULL.

The direct callers are:
1) alloc_extent_buffer()
   It's the stub of the call chain, as the only caller is
   btrfs_find_create_tree_block(), and all its callers have already
   checked the return value correctly.

2) __alloc_dummy_extent_buffer()/alloc_dummy_extent_buffer
3) btrfs_clone_extent_buffer()

For call sites 2) and 3), this patch only checks the return value from
__alloc_extent_buffer() but still allow its caller to return NULL.
The NULL return value will be addressed in later commits.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index b28a75546700..c73da1752041 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4661,6 +4661,11 @@ static inline void btrfs_release_extent_buffer(struct extent_buffer *eb)
 	__free_extent_buffer(eb);
 }
 
+/*
+ * This function should not fail due to its __GFP_NOFAIL flag.
+ *
+ * But caller should check for PTR_ERR() to be future-proof.
+ */
 static struct extent_buffer *
 __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 		      unsigned long len)
@@ -4707,7 +4712,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
 	int num_pages = num_extent_pages(src);
 
 	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
-	if (new == NULL)
+	if (IS_ERR(new))
 		return NULL;
 
 	for (i = 0; i < num_pages; i++) {
@@ -4737,7 +4742,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 	int i;
 
 	eb = __alloc_extent_buffer(fs_info, start, len);
-	if (!eb)
+	if (IS_ERR(eb))
 		return NULL;
 
 	num_pages = num_extent_pages(eb);
@@ -4921,7 +4926,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		return eb;
 
 	eb = __alloc_extent_buffer(fs_info, start, len);
-	if (!eb)
+	if (IS_ERR(eb))
 		return ERR_PTR(-ENOMEM);
 
 	num_pages = num_extent_pages(eb);
-- 
2.20.1

[PATCH 3/5] btrfs: extent_io: Unify the return value of alloc_dummy_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

All direct callers are:
1) tests/inode-tests.c
2) tests/extent-buffer-tests.c
3) tests/extent-io-tests.c
   These call sites are all stubs.

4) tree_mod_log_rewind()
5) get_old_root()
   These two call sites no longer follows the function name pattern
   *_extent_buffer(). So they are also considered as the boundary.

This patch should cover all the call sites of
alloc_dummy_extent_buffer().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/ctree.c                     |  4 ++--
 fs/btrfs/extent_io.c                 | 13 ++++++++++---
 fs/btrfs/tests/extent-buffer-tests.c |  6 ++++--
 fs/btrfs/tests/extent-io-tests.c     |  4 ++--
 fs/btrfs/tests/inode-tests.c         |  6 ++++--
 5 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5a6c39b44c84..3276b743f40a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1293,7 +1293,7 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 	if (tm->op == MOD_LOG_KEY_REMOVE_WHILE_FREEING) {
 		BUG_ON(tm->slot != 0);
 		eb_rewin = alloc_dummy_extent_buffer(fs_info, eb->start);
-		if (!eb_rewin) {
+		if (IS_ERR(eb_rewin)) {
 			btrfs_tree_read_unlock_blocking(eb);
 			free_extent_buffer(eb);
 			return NULL;
@@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
 		free_extent_buffer(eb_root);
 	}
 
-	if (!eb)
+	if (IS_ERR_OR_NULL(eb))
 		return NULL;
 	btrfs_tree_read_lock(eb);
 	if (old_root) {
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c73da1752041..f9c9da9b84a3 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4734,6 +4734,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
 	return new;
 }
 
+/*
+ * Allocate an extent buffer for selftest.
+ *
+ * Return valid pointer if everything goes well.
+ * Return PTR_ERR() for error.
+ * Will NEVER return NULL.
+ */
 struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 						  u64 start, unsigned long len)
 {
@@ -4743,7 +4750,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 
 	eb = __alloc_extent_buffer(fs_info, start, len);
 	if (IS_ERR(eb))
-		return NULL;
+		return eb;
 
 	num_pages = num_extent_pages(eb);
 	for (i = 0; i < num_pages; i++) {
@@ -4760,7 +4767,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
 	for (; i > 0; i--)
 		__free_page(eb->pages[i - 1]);
 	__free_extent_buffer(eb);
-	return NULL;
+	return ERR_PTR(-ENOMEM);
 }
 
 struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
@@ -4866,7 +4873,7 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 	if (eb)
 		return eb;
 	eb = alloc_dummy_extent_buffer(fs_info, start);
-	if (!eb)
+	if (IS_ERR(eb))
 		return NULL;
 	eb->fs_info = fs_info;
 again:
diff --git a/fs/btrfs/tests/extent-buffer-tests.c b/fs/btrfs/tests/extent-buffer-tests.c
index 7d72eab6d32c..edf2ab6a7d74 100644
--- a/fs/btrfs/tests/extent-buffer-tests.c
+++ b/fs/btrfs/tests/extent-buffer-tests.c
@@ -48,12 +48,14 @@ static int test_btrfs_split_item(u32 sectorsize, u32 nodesize)
 		goto out;
 	}
 
-	path->nodes[0] = eb = alloc_dummy_extent_buffer(fs_info, nodesize);
-	if (!eb) {
+	eb = alloc_dummy_extent_buffer(fs_info, nodesize);
+	if (IS_ERR(eb)) {
+		eb = NULL;
 		test_err("could not allocate dummy buffer");
 		ret = -ENOMEM;
 		goto out;
 	}
+	path->nodes[0] = eb;
 	path->slots[0] = 0;
 
 	key.objectid = 0;
diff --git a/fs/btrfs/tests/extent-io-tests.c b/fs/btrfs/tests/extent-io-tests.c
index 3c46d7f23456..d1f3b727fbf2 100644
--- a/fs/btrfs/tests/extent-io-tests.c
+++ b/fs/btrfs/tests/extent-io-tests.c
@@ -396,7 +396,7 @@ static int test_eb_bitmaps(u32 sectorsize, u32 nodesize)
 	}
 
 	eb = __alloc_dummy_extent_buffer(fs_info, 0, len);
-	if (!eb) {
+	if (IS_ERR(eb)) {
 		test_err("couldn't allocate test extent buffer");
 		kfree(bitmap);
 		return -ENOMEM;
@@ -409,7 +409,7 @@ static int test_eb_bitmaps(u32 sectorsize, u32 nodesize)
 	/* Do it over again with an extent buffer which isn't page-aligned. */
 	free_extent_buffer(eb);
 	eb = __alloc_dummy_extent_buffer(NULL, nodesize / 2, len);
-	if (!eb) {
+	if (IS_ERR(eb)) {
 		test_err("couldn't allocate test extent buffer");
 		kfree(bitmap);
 		return -ENOMEM;
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index af0c8e30d9e2..56a112a39211 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -249,7 +249,8 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 	}
 
 	root->node = alloc_dummy_extent_buffer(fs_info, nodesize);
-	if (!root->node) {
+	if (IS_ERR(root->node)) {
+		root->node = NULL;
 		test_err("couldn't allocate dummy buffer");
 		goto out;
 	}
@@ -850,7 +851,8 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
 	}
 
 	root->node = alloc_dummy_extent_buffer(fs_info, nodesize);
-	if (!root->node) {
+	if (IS_ERR(root->node)) {
+		root->node = NULL;
 		test_err("couldn't allocate dummy buffer");
 		goto out;
 	}
-- 
2.20.1

[PATCH 4/5] btrfs: extent_io: Unify the return value of alloc_test_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

The function only get called in two locations:
1) free-space-tree-tests.c
2) qgroup-tests.c
   These call sites are stubs.

3) btrfs_find_create_tree_block()
   Since all existing btrfs_find_create_tree_block() callers handles
   the return value correctly, it's also a stub.

So all alloc_test_extent_buffer() callers should be covered by this
patch.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c                   | 21 +++++++++++----------
 fs/btrfs/tests/free-space-tree-tests.c |  3 ++-
 fs/btrfs/tests/qgroup-tests.c          |  3 ++-
 3 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index f9c9da9b84a3..a19bd8bc3b32 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4863,6 +4863,7 @@ struct extent_buffer *find_extent_buffer(struct btrfs_fs_info *fs_info,
 }
 
 #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
+/* Will either return valid pointer or PTR_ERR(), NEVER NULL pointer. */
 struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 					u64 start)
 {
@@ -4874,12 +4875,14 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 		return eb;
 	eb = alloc_dummy_extent_buffer(fs_info, start);
 	if (IS_ERR(eb))
-		return NULL;
+		return eb;
 	eb->fs_info = fs_info;
 again:
 	ret = radix_tree_preload(GFP_NOFS);
-	if (ret)
-		goto free_eb;
+	if (ret) {
+		btrfs_release_extent_buffer(eb);
+		return ERR_PTR(ret);
+	}
 	spin_lock(&fs_info->buffer_lock);
 	ret = radix_tree_insert(&fs_info->buffer_radix,
 				start >> PAGE_SHIFT, eb);
@@ -4887,18 +4890,16 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 	radix_tree_preload_end();
 	if (ret == -EEXIST) {
 		exists = find_extent_buffer(fs_info, start);
-		if (exists)
-			goto free_eb;
-		else
-			goto again;
+		if (exists) {
+			btrfs_release_extent_buffer(eb);
+			return exists;
+		}
+		goto again;
 	}
 	check_buffer_tree_ref(eb);
 	set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags);
 
 	return eb;
-free_eb:
-	btrfs_release_extent_buffer(eb);
-	return exists;
 }
 #endif
 
diff --git a/fs/btrfs/tests/free-space-tree-tests.c b/fs/btrfs/tests/free-space-tree-tests.c
index 89346da890cf..025fce63959a 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -462,7 +462,8 @@ static int run_test(test_func_t test_func, int bitmaps, u32 sectorsize,
 	root->fs_info->tree_root = root;
 
 	root->node = alloc_test_extent_buffer(root->fs_info, nodesize);
-	if (!root->node) {
+	if (IS_ERR(root->node)) {
+		root->node = NULL;
 		test_err("couldn't allocate dummy buffer");
 		ret = -ENOMEM;
 		goto out;
diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
index 412b910b04cc..536600eb4d9d 100644
--- a/fs/btrfs/tests/qgroup-tests.c
+++ b/fs/btrfs/tests/qgroup-tests.c
@@ -484,7 +484,8 @@ int btrfs_test_qgroups(u32 sectorsize, u32 nodesize)
 	 * *cough*backref walking code*cough*
 	 */
 	root->node = alloc_test_extent_buffer(root->fs_info, nodesize);
-	if (!root->node) {
+	if (IS_ERR(root->node)) {
+		root->node = NULL;
 		test_err("couldn't allocate dummy buffer");
 		ret = -ENOMEM;
 		goto out;
-- 
2.20.1

[PATCH 5/5] btrfs: extent_io: Unify the return value of btrfs_clone_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

The direct callers are:
1) qgroup_rescan_leaf()
2) btrfs_compare_trees()
   These two call sites are stubs.

3) tree_mod_log_rewind()
4) get_old_root()
   These two doesn't follow the name pattern *_extent_buffer(), thus we
   don't need to unify their return value.
   However for get_old_root(), all its eb assignment can no longer be
   NULL, change its IS_ERR_OR_NULL() to IS_ERR().

5) btrfs_search_slot_get_root()
   This is a stub. The only caller has already handled the return value.

This patch should cover all the call sites of
btrfs_clone_extent_buffer().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/backref.c   |  8 ++++----
 fs/btrfs/ctree.c     | 14 ++++++++------
 fs/btrfs/extent_io.c |  4 ++--
 fs/btrfs/qgroup.c    |  5 +++--
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 78556447e1d5..b6e469a12011 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2016,8 +2016,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 		parent = found_key.offset;
 		slot = path->slots[0];
 		eb = btrfs_clone_extent_buffer(path->nodes[0]);
-		if (!eb) {
-			ret = -ENOMEM;
+		if (IS_ERR(eb)) {
+			ret = PTR_ERR(eb);
 			break;
 		}
 		btrfs_release_path(path);
@@ -2075,8 +2075,8 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
 
 		slot = path->slots[0];
 		eb = btrfs_clone_extent_buffer(path->nodes[0]);
-		if (!eb) {
-			ret = -ENOMEM;
+		if (IS_ERR(eb)) {
+			ret = PTR_ERR(eb);
 			break;
 		}
 		btrfs_release_path(path);
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3276b743f40a..ea7ff0012007 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
 		free_extent_buffer(eb_root);
 	}
 
-	if (IS_ERR_OR_NULL(eb))
+	if (IS_ERR(eb))
 		return NULL;
 	btrfs_tree_read_lock(eb);
 	if (old_root) {
@@ -2624,8 +2624,8 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 			down_read(&fs_info->commit_root_sem);
 			b = btrfs_clone_extent_buffer(root->commit_root);
 			up_read(&fs_info->commit_root_sem);
-			if (!b)
-				return ERR_PTR(-ENOMEM);
+			if (IS_ERR(b))
+				return b;
 
 		} else {
 			b = root->commit_root;
@@ -5431,7 +5431,8 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 			btrfs_clone_extent_buffer(left_root->commit_root);
 	if (!left_path->nodes[left_level]) {
 		up_read(&fs_info->commit_root_sem);
-		ret = -ENOMEM;
+		ret = PTR_ERR(left_path->nodes[left_level]);
+		left_path->nodes[left_level] = NULL;
 		goto out;
 	}
 
@@ -5439,9 +5440,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 	right_root_level = right_level;
 	right_path->nodes[right_level] =
 			btrfs_clone_extent_buffer(right_root->commit_root);
-	if (!right_path->nodes[right_level]) {
+	if (IS_ERR(right_path->nodes[right_level])) {
 		up_read(&fs_info->commit_root_sem);
-		ret = -ENOMEM;
+		ret = PTR_ERR(right_path->nodes[right_level]);
+		right_path->nodes[right_level] = NULL;
 		goto out;
 	}
 	up_read(&fs_info->commit_root_sem);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a19bd8bc3b32..dbdb649f1957 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4713,13 +4713,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
 
 	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
 	if (IS_ERR(new))
-		return NULL;
+		return new;
 
 	for (i = 0; i < num_pages; i++) {
 		p = alloc_page(GFP_NOFS);
 		if (!p) {
 			btrfs_release_extent_buffer(new);
-			return NULL;
+			return ERR_PTR(-ENOMEM);
 		}
 		attach_extent_buffer_page(new, p);
 		WARN_ON(PageDirty(p));
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4e473a998219..ccfa992a10bf 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3105,8 +3105,9 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
 	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
 
 	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
-	if (!scratch_leaf) {
-		ret = -ENOMEM;
+	if (IS_ERR(scratch_leaf)) {
+		ret = PTR_ERR(scratch_leaf);
+		scratch_leaf = NULL;
 		mutex_unlock(&fs_info->qgroup_rescan_lock);
 		goto out;
 	}
-- 
2.20.1

Re: [PATCH 5/5] btrfs: extent_io: Unify the return value of btrfs_clone_extent_buffer() with alloc_extent_buffer()

[Nikolay Borisov]

On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
> The direct callers are:
> 1) qgroup_rescan_leaf()
> 2) btrfs_compare_trees()
>    These two call sites are stubs.
> 
> 3) tree_mod_log_rewind()
> 4) get_old_root()
>    These two doesn't follow the name pattern *_extent_buffer(), thus we
>    don't need to unify their return value.
>    However for get_old_root(), all its eb assignment can no longer be
>    NULL, change its IS_ERR_OR_NULL() to IS_ERR().
> 
> 5) btrfs_search_slot_get_root()
>    This is a stub. The only caller has already handled the return value.
> 
> This patch should cover all the call sites of
> btrfs_clone_extent_buffer().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/backref.c   |  8 ++++----
>  fs/btrfs/ctree.c     | 14 ++++++++------
>  fs/btrfs/extent_io.c |  4 ++--
>  fs/btrfs/qgroup.c    |  5 +++--
>  4 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 78556447e1d5..b6e469a12011 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -2016,8 +2016,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
>  		parent = found_key.offset;
>  		slot = path->slots[0];
>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
> -		if (!eb) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(eb)) {
> +			ret = PTR_ERR(eb);
>  			break;
>  		}
>  		btrfs_release_path(path);
> @@ -2075,8 +2075,8 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
>  
>  		slot = path->slots[0];
>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
> -		if (!eb) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(eb)) {
> +			ret = PTR_ERR(eb);
>  			break;
>  		}
>  		btrfs_release_path(path);
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3276b743f40a..ea7ff0012007 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
>  		free_extent_buffer(eb_root);
>  	}
>  
> -	if (IS_ERR_OR_NULL(eb))
> +	if (IS_ERR(eb))
>  		return NULL;
>  	btrfs_tree_read_lock(eb);
>  	if (old_root) {
> @@ -2624,8 +2624,8 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>  			down_read(&fs_info->commit_root_sem);
>  			b = btrfs_clone_extent_buffer(root->commit_root);
>  			up_read(&fs_info->commit_root_sem);
> -			if (!b)
> -				return ERR_PTR(-ENOMEM);
> +			if (IS_ERR(b))
> +				return b;
>  
>  		} else {
>  			b = root->commit_root;
> @@ -5431,7 +5431,8 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>  			btrfs_clone_extent_buffer(left_root->commit_root);
>  	if (!left_path->nodes[left_level]) {
>  		up_read(&fs_info->commit_root_sem);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(left_path->nodes[left_level]);
> +		left_path->nodes[left_level] = NULL;

If we go into this branch this means left_path->nodes[left_level] was
NULL. PTR_ERR of NULL is, well, NULL and then you also set the
left_level to NULL. This is pointless...

>  		goto out;
>  	}
>  
> @@ -5439,9 +5440,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>  	right_root_level = right_level;
>  	right_path->nodes[right_level] =
>  			btrfs_clone_extent_buffer(right_root->commit_root);
> -	if (!right_path->nodes[right_level]) {
> +	if (IS_ERR(right_path->nodes[right_level])) {

And here you do change the check to IS_ERR but only for the right_Path
and not left path this introduces asymmetry which makes code harder to
comprehend.

>  		up_read(&fs_info->commit_root_sem);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(right_path->nodes[right_level]);
> +		right_path->nodes[right_level] = NULL;
>  		goto out;
>  	}
>  	up_read(&fs_info->commit_root_sem);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index a19bd8bc3b32..dbdb649f1957 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4713,13 +4713,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>  
>  	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
>  	if (IS_ERR(new))
> -		return NULL;
> +		return new;
>  
>  	for (i = 0; i < num_pages; i++) {
>  		p = alloc_page(GFP_NOFS);
>  		if (!p) {
>  			btrfs_release_extent_buffer(new);
> -			return NULL;
> +			return ERR_PTR(-ENOMEM);
>  		}
>  		attach_extent_buffer_page(new, p);
>  		WARN_ON(PageDirty(p));
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 4e473a998219..ccfa992a10bf 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3105,8 +3105,9 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
>  	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
>  
>  	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
> -	if (!scratch_leaf) {
> -		ret = -ENOMEM;
> +	if (IS_ERR(scratch_leaf)) {
> +		ret = PTR_ERR(scratch_leaf);
> +		scratch_leaf = NULL;
>  		mutex_unlock(&fs_info->qgroup_rescan_lock);
>  		goto out;
>  	}
> 

Re: [PATCH 5/5] btrfs: extent_io: Unify the return value of btrfs_clone_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 5415 bytes --]



On 2019/2/22 下午8:47, Nikolay Borisov wrote:
> 
> 
> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>> The direct callers are:
>> 1) qgroup_rescan_leaf()
>> 2) btrfs_compare_trees()
>>    These two call sites are stubs.
>>
>> 3) tree_mod_log_rewind()
>> 4) get_old_root()
>>    These two doesn't follow the name pattern *_extent_buffer(), thus we
>>    don't need to unify their return value.
>>    However for get_old_root(), all its eb assignment can no longer be
>>    NULL, change its IS_ERR_OR_NULL() to IS_ERR().
>>
>> 5) btrfs_search_slot_get_root()
>>    This is a stub. The only caller has already handled the return value.
>>
>> This patch should cover all the call sites of
>> btrfs_clone_extent_buffer().
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/backref.c   |  8 ++++----
>>  fs/btrfs/ctree.c     | 14 ++++++++------
>>  fs/btrfs/extent_io.c |  4 ++--
>>  fs/btrfs/qgroup.c    |  5 +++--
>>  4 files changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index 78556447e1d5..b6e469a12011 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -2016,8 +2016,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
>>  		parent = found_key.offset;
>>  		slot = path->slots[0];
>>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
>> -		if (!eb) {
>> -			ret = -ENOMEM;
>> +		if (IS_ERR(eb)) {
>> +			ret = PTR_ERR(eb);
>>  			break;
>>  		}
>>  		btrfs_release_path(path);
>> @@ -2075,8 +2075,8 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
>>  
>>  		slot = path->slots[0];
>>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
>> -		if (!eb) {
>> -			ret = -ENOMEM;
>> +		if (IS_ERR(eb)) {
>> +			ret = PTR_ERR(eb);
>>  			break;
>>  		}
>>  		btrfs_release_path(path);
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 3276b743f40a..ea7ff0012007 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
>>  		free_extent_buffer(eb_root);
>>  	}
>>  
>> -	if (IS_ERR_OR_NULL(eb))
>> +	if (IS_ERR(eb))
>>  		return NULL;
>>  	btrfs_tree_read_lock(eb);
>>  	if (old_root) {
>> @@ -2624,8 +2624,8 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>>  			down_read(&fs_info->commit_root_sem);
>>  			b = btrfs_clone_extent_buffer(root->commit_root);
>>  			up_read(&fs_info->commit_root_sem);
>> -			if (!b)
>> -				return ERR_PTR(-ENOMEM);
>> +			if (IS_ERR(b))
>> +				return b;
>>  
>>  		} else {
>>  			b = root->commit_root;
>> @@ -5431,7 +5431,8 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>>  			btrfs_clone_extent_buffer(left_root->commit_root);
>>  	if (!left_path->nodes[left_level]) {
>>  		up_read(&fs_info->commit_root_sem);
>> -		ret = -ENOMEM;
>> +		ret = PTR_ERR(left_path->nodes[left_level]);
>> +		left_path->nodes[left_level] = NULL;
> 
> If we go into this branch this means left_path->nodes[left_level] was
> NULL. PTR_ERR of NULL is, well, NULL and then you also set the
> left_level to NULL. This is pointless...

Oh, previous "!left_path->nodes[left_level]" should be
"IS_ERR(left_path->nodes[left_level]".

That should explain all the problem.

Thanks,
Qu

> 
>>  		goto out;
>>  	}
>>  
>> @@ -5439,9 +5440,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>>  	right_root_level = right_level;
>>  	right_path->nodes[right_level] =
>>  			btrfs_clone_extent_buffer(right_root->commit_root);
>> -	if (!right_path->nodes[right_level]) {
>> +	if (IS_ERR(right_path->nodes[right_level])) {
> 
> And here you do change the check to IS_ERR but only for the right_Path
> and not left path this introduces asymmetry which makes code harder to
> comprehend.
> 
>>  		up_read(&fs_info->commit_root_sem);
>> -		ret = -ENOMEM;
>> +		ret = PTR_ERR(right_path->nodes[right_level]);
>> +		right_path->nodes[right_level] = NULL;
>>  		goto out;
>>  	}
>>  	up_read(&fs_info->commit_root_sem);
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index a19bd8bc3b32..dbdb649f1957 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -4713,13 +4713,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>>  
>>  	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
>>  	if (IS_ERR(new))
>> -		return NULL;
>> +		return new;
>>  
>>  	for (i = 0; i < num_pages; i++) {
>>  		p = alloc_page(GFP_NOFS);
>>  		if (!p) {
>>  			btrfs_release_extent_buffer(new);
>> -			return NULL;
>> +			return ERR_PTR(-ENOMEM);
>>  		}
>>  		attach_extent_buffer_page(new, p);
>>  		WARN_ON(PageDirty(p));
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 4e473a998219..ccfa992a10bf 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -3105,8 +3105,9 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
>>  	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
>>  
>>  	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
>> -	if (!scratch_leaf) {
>> -		ret = -ENOMEM;
>> +	if (IS_ERR(scratch_leaf)) {
>> +		ret = PTR_ERR(scratch_leaf);
>> +		scratch_leaf = NULL;
>>  		mutex_unlock(&fs_info->qgroup_rescan_lock);
>>  		goto out;
>>  	}
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Nikolay Borisov]

On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
> Which is based on v5.0-rc7
> 
> There are 5 extent buffer alloc functions in btrfs:
> __alloc_extent_buffer();
> alloc_extent_buffer();
> __alloc_dummy_extent_buffer();
> alloc_dummy_extent_buffer();
> alloc_test_extent_buffer();
> 
> However their return value is not unified for failure mode:
> __alloc_extent_buffer()		Never fail
> alloc_extent_buffer()		PTR_ERR()
> __alloc_dummy_extent_buffer()	NULL

This function can never return NULL, if __alloc_extent_buffer cannot
fail then the only error this function returns is ERR_PTR(ENOMEM);

> alloc_dummy_extent_buffer()	NULL
Same thing applies to this function

> alloc_test_extent_buffer()	NULL

Same thing for this function, if we return exists then we must have
found it by find_extent_buffer hence it cannot be null. Otherwise we
return eb as allocated from alloc_dummy_extent_buffer. So how can null
be returned?

To me it really seems none of the function could return a NULL value, no?

> 
> This causes some wrapper function to have 2 failure modes, like
> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
> its failure.
> 
> This inconsistent behavior is making static checker and reader crazy.
> 
> This patchset will unify the failure more of above 5 functions to
> PTR_ERR().
> 
> Qu Wenruo (5):
>   btrfs: extent_io: Add comment about the return value of
>     alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>     with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of
>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>     with alloc_extent_buffer()
>   btrfs: extent_io: Unify the return value of
>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
> 
>  fs/btrfs/backref.c                     |  8 ++--
>  fs/btrfs/ctree.c                       | 16 ++++----
>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>  fs/btrfs/qgroup.c                      |  5 ++-
>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>  9 files changed, 68 insertions(+), 39 deletions(-)
> 

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3350 bytes --]



On 2019/2/22 下午8:54, Nikolay Borisov wrote:
> 
> 
> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>> This patchset can be fetched from github:
>> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
>> Which is based on v5.0-rc7
>>
>> There are 5 extent buffer alloc functions in btrfs:
>> __alloc_extent_buffer();
>> alloc_extent_buffer();
>> __alloc_dummy_extent_buffer();
>> alloc_dummy_extent_buffer();
>> alloc_test_extent_buffer();
>>
>> However their return value is not unified for failure mode:
>> __alloc_extent_buffer()		Never fail
>> alloc_extent_buffer()		PTR_ERR()
>> __alloc_dummy_extent_buffer()	NULL
> 
> This function can never return NULL, if __alloc_extent_buffer cannot
> fail then the only error this function returns is ERR_PTR(ENOMEM);

Nope.

        for (i = 0; i < num_pages; i++) {
                eb->pages[i] = alloc_page(GFP_NOFS);
                if (!eb->pages[i])
                        goto err; <<< Page alloc failure here
        }
...
err:
        for (; i > 0; i--)
                __free_page(eb->pages[i - 1]);
        __free_extent_buffer(eb);
        return NULL; << We got NULL.
}

For __alloc_dummy_extent_buffer, that's the only failure case.

And I'm interested how did you get the PTR_ERR() case?

> 
>> alloc_dummy_extent_buffer()	NULL
> Same thing applies to this function

Nope.

> 
>> alloc_test_extent_buffer()	NULL
> 
> Same thing for this function, if we return exists then we must have
> found it by find_extent_buffer hence it cannot be null. Otherwise we
> return eb as allocated from alloc_dummy_extent_buffer. So how can null
> be returned?

And nope.

> 
> To me it really seems none of the function could return a NULL value, no?

Your misunderstand of __alloc_dummy_extent_buffer() makes the call chain
all wrong.

Thanks,
Qu

> 
>>
>> This causes some wrapper function to have 2 failure modes, like
>> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
>> its failure.
>>
>> This inconsistent behavior is making static checker and reader crazy.
>>
>> This patchset will unify the failure more of above 5 functions to
>> PTR_ERR().
>>
>> Qu Wenruo (5):
>>   btrfs: extent_io: Add comment about the return value of
>>     alloc_extent_buffer()
>>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>>     with alloc_extent_buffer()
>>   btrfs: extent_io: Unify the return value of
>>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>>     with alloc_extent_buffer()
>>   btrfs: extent_io: Unify the return value of
>>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
>>
>>  fs/btrfs/backref.c                     |  8 ++--
>>  fs/btrfs/ctree.c                       | 16 ++++----
>>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>>  fs/btrfs/qgroup.c                      |  5 ++-
>>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>>  9 files changed, 68 insertions(+), 39 deletions(-)
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v1.1 5/5] btrfs: extent_io: Unify the return value of btrfs_clone_extent_buffer() with alloc_extent_buffer()

[Qu Wenruo]

The direct callers are:
1) qgroup_rescan_leaf()
2) btrfs_compare_trees()
   These two call sites are stubs.

3) tree_mod_log_rewind()
4) get_old_root()
   These two doesn't follow the name pattern *_extent_buffer(), thus we
   don't need to unify their return value.
   However for get_old_root(), all its eb assignment can no longer be
   NULL, change its IS_ERR_OR_NULL() to IS_ERR().

5) btrfs_search_slot_get_root()
   This is a stub. The only caller has already handled the return value.

This patch should cover all the call sites of
btrfs_clone_extent_buffer().

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
changelog
v1.1:
- Add a missing IS_ERR() check for left_path.
---
 fs/btrfs/backref.c   |  8 ++++----
 fs/btrfs/ctree.c     | 16 +++++++++-------
 fs/btrfs/extent_io.c |  4 ++--
 fs/btrfs/qgroup.c    |  5 +++--
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 78556447e1d5..b6e469a12011 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2016,8 +2016,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 		parent = found_key.offset;
 		slot = path->slots[0];
 		eb = btrfs_clone_extent_buffer(path->nodes[0]);
-		if (!eb) {
-			ret = -ENOMEM;
+		if (IS_ERR(eb)) {
+			ret = PTR_ERR(eb);
 			break;
 		}
 		btrfs_release_path(path);
@@ -2075,8 +2075,8 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
 
 		slot = path->slots[0];
 		eb = btrfs_clone_extent_buffer(path->nodes[0]);
-		if (!eb) {
-			ret = -ENOMEM;
+		if (IS_ERR(eb)) {
+			ret = PTR_ERR(eb);
 			break;
 		}
 		btrfs_release_path(path);
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3276b743f40a..ebd640f2ad86 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
 		free_extent_buffer(eb_root);
 	}
 
-	if (IS_ERR_OR_NULL(eb))
+	if (IS_ERR(eb))
 		return NULL;
 	btrfs_tree_read_lock(eb);
 	if (old_root) {
@@ -2624,8 +2624,8 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 			down_read(&fs_info->commit_root_sem);
 			b = btrfs_clone_extent_buffer(root->commit_root);
 			up_read(&fs_info->commit_root_sem);
-			if (!b)
-				return ERR_PTR(-ENOMEM);
+			if (IS_ERR(b))
+				return b;
 
 		} else {
 			b = root->commit_root;
@@ -5429,9 +5429,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 	left_root_level = left_level;
 	left_path->nodes[left_level] =
 			btrfs_clone_extent_buffer(left_root->commit_root);
-	if (!left_path->nodes[left_level]) {
+	if (IS_ERR(left_path->nodes[left_level])) {
 		up_read(&fs_info->commit_root_sem);
-		ret = -ENOMEM;
+		ret = PTR_ERR(left_path->nodes[left_level]);
+		left_path->nodes[left_level] = NULL;
 		goto out;
 	}
 
@@ -5439,9 +5440,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 	right_root_level = right_level;
 	right_path->nodes[right_level] =
 			btrfs_clone_extent_buffer(right_root->commit_root);
-	if (!right_path->nodes[right_level]) {
+	if (IS_ERR(right_path->nodes[right_level])) {
 		up_read(&fs_info->commit_root_sem);
-		ret = -ENOMEM;
+		ret = PTR_ERR(right_path->nodes[right_level]);
+		right_path->nodes[right_level] = NULL;
 		goto out;
 	}
 	up_read(&fs_info->commit_root_sem);
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a19bd8bc3b32..dbdb649f1957 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4713,13 +4713,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
 
 	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
 	if (IS_ERR(new))
-		return NULL;
+		return new;
 
 	for (i = 0; i < num_pages; i++) {
 		p = alloc_page(GFP_NOFS);
 		if (!p) {
 			btrfs_release_extent_buffer(new);
-			return NULL;
+			return ERR_PTR(-ENOMEM);
 		}
 		attach_extent_buffer_page(new, p);
 		WARN_ON(PageDirty(p));
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4e473a998219..ccfa992a10bf 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3105,8 +3105,9 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
 	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
 
 	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
-	if (!scratch_leaf) {
-		ret = -ENOMEM;
+	if (IS_ERR(scratch_leaf)) {
+		ret = PTR_ERR(scratch_leaf);
+		scratch_leaf = NULL;
 		mutex_unlock(&fs_info->qgroup_rescan_lock);
 		goto out;
 	}
-- 
2.20.1

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Nikolay Borisov]

On 22.02.19 г. 15:02 ч., Qu Wenruo wrote:
> 
> 
> On 2019/2/22 下午8:54, Nikolay Borisov wrote:
>>
>>
>> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>>> This patchset can be fetched from github:
>>> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
>>> Which is based on v5.0-rc7
>>>
>>> There are 5 extent buffer alloc functions in btrfs:
>>> __alloc_extent_buffer();
>>> alloc_extent_buffer();
>>> __alloc_dummy_extent_buffer();
>>> alloc_dummy_extent_buffer();
>>> alloc_test_extent_buffer();
>>>
>>> However their return value is not unified for failure mode:
>>> __alloc_extent_buffer()		Never fail
>>> alloc_extent_buffer()		PTR_ERR()
>>> __alloc_dummy_extent_buffer()	NULL
>>
>> This function can never return NULL, if __alloc_extent_buffer cannot
>> fail then the only error this function returns is ERR_PTR(ENOMEM);
> 
> Nope.
> 
>         for (i = 0; i < num_pages; i++) {
>                 eb->pages[i] = alloc_page(GFP_NOFS);
>                 if (!eb->pages[i])
>                         goto err; <<< Page alloc failure here
>         }
> ...
> err:
>         for (; i > 0; i--)
>                 __free_page(eb->pages[i - 1]);
>         __free_extent_buffer(eb);
>         return NULL; << We got NULL.

Right, I was looking at the code AFTER having applied your patches. So I
agree with yout. However, the ordering of your patches and the
changelogs make it rather hard to understand. What I'd suggest regarding
the changelogs is - forget about unification, just say what you are
doing, which is always ensuring that an error is returned from
__alloc_extent_buffer in one patch - this should involve both changes to
__alloc_extent_buffer as well as it's (in)direct callers. Then you do
the same for other function. Otherwise review is somewhat hindered.

> }
> 
> For __alloc_dummy_extent_buffer, that's the only failure case.
> 
> And I'm interested how did you get the PTR_ERR() case?
> 
>>
>>> alloc_dummy_extent_buffer()	NULL
>> Same thing applies to this function
> 
> Nope.
> 
>>
>>> alloc_test_extent_buffer()	NULL
>>
>> Same thing for this function, if we return exists then we must have
>> found it by find_extent_buffer hence it cannot be null. Otherwise we
>> return eb as allocated from alloc_dummy_extent_buffer. So how can null
>> be returned?
> 
> And nope.
> 
>>
>> To me it really seems none of the function could return a NULL value, no?
> 
> Your misunderstand of __alloc_dummy_extent_buffer() makes the call chain
> all wrong.
> 
> Thanks,
> Qu
> 
>>
>>>
>>> This causes some wrapper function to have 2 failure modes, like
>>> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
>>> its failure.
>>>
>>> This inconsistent behavior is making static checker and reader crazy.
>>>
>>> This patchset will unify the failure more of above 5 functions to
>>> PTR_ERR().
>>>
>>> Qu Wenruo (5):
>>>   btrfs: extent_io: Add comment about the return value of
>>>     alloc_extent_buffer()
>>>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>>>     with alloc_extent_buffer()
>>>   btrfs: extent_io: Unify the return value of
>>>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>>>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>>>     with alloc_extent_buffer()
>>>   btrfs: extent_io: Unify the return value of
>>>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
>>>
>>>  fs/btrfs/backref.c                     |  8 ++--
>>>  fs/btrfs/ctree.c                       | 16 ++++----
>>>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>>>  fs/btrfs/qgroup.c                      |  5 ++-
>>>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>>>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>>>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>>>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>>>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>>>  9 files changed, 68 insertions(+), 39 deletions(-)
>>>
>>
> 

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4339 bytes --]



On 2019/2/22 下午9:29, Nikolay Borisov wrote:
> 
> 
> On 22.02.19 г. 15:02 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/2/22 下午8:54, Nikolay Borisov wrote:
>>>
>>>
>>> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>>>> This patchset can be fetched from github:
>>>> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
>>>> Which is based on v5.0-rc7
>>>>
>>>> There are 5 extent buffer alloc functions in btrfs:
>>>> __alloc_extent_buffer();
>>>> alloc_extent_buffer();
>>>> __alloc_dummy_extent_buffer();
>>>> alloc_dummy_extent_buffer();
>>>> alloc_test_extent_buffer();
>>>>
>>>> However their return value is not unified for failure mode:
>>>> __alloc_extent_buffer()		Never fail
>>>> alloc_extent_buffer()		PTR_ERR()
>>>> __alloc_dummy_extent_buffer()	NULL
>>>
>>> This function can never return NULL, if __alloc_extent_buffer cannot
>>> fail then the only error this function returns is ERR_PTR(ENOMEM);
>>
>> Nope.
>>
>>         for (i = 0; i < num_pages; i++) {
>>                 eb->pages[i] = alloc_page(GFP_NOFS);
>>                 if (!eb->pages[i])
>>                         goto err; <<< Page alloc failure here
>>         }
>> ...
>> err:
>>         for (; i > 0; i--)
>>                 __free_page(eb->pages[i - 1]);
>>         __free_extent_buffer(eb);
>>         return NULL; << We got NULL.
> 
> Right, I was looking at the code AFTER having applied your patches. So I
> agree with yout. However, the ordering of your patches and the
> changelogs make it rather hard to understand. What I'd suggest regarding
> the changelogs is - forget about unification, just say what you are
> doing, which is always ensuring that an error is returned from
> __alloc_extent_buffer in one patch - this should involve both changes to
> __alloc_extent_buffer as well as it's (in)direct callers. Then you do
> the same for other function. Otherwise review is somewhat hindered.

Makes sense.

I'll reword and reorder the patches.

Thanks,
Qu

> 
>> }
>>
>> For __alloc_dummy_extent_buffer, that's the only failure case.
>>
>> And I'm interested how did you get the PTR_ERR() case?
>>
>>>
>>>> alloc_dummy_extent_buffer()	NULL
>>> Same thing applies to this function
>>
>> Nope.
>>
>>>
>>>> alloc_test_extent_buffer()	NULL
>>>
>>> Same thing for this function, if we return exists then we must have
>>> found it by find_extent_buffer hence it cannot be null. Otherwise we
>>> return eb as allocated from alloc_dummy_extent_buffer. So how can null
>>> be returned?
>>
>> And nope.
>>
>>>
>>> To me it really seems none of the function could return a NULL value, no?
>>
>> Your misunderstand of __alloc_dummy_extent_buffer() makes the call chain
>> all wrong.
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> This causes some wrapper function to have 2 failure modes, like
>>>> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
>>>> its failure.
>>>>
>>>> This inconsistent behavior is making static checker and reader crazy.
>>>>
>>>> This patchset will unify the failure more of above 5 functions to
>>>> PTR_ERR().
>>>>
>>>> Qu Wenruo (5):
>>>>   btrfs: extent_io: Add comment about the return value of
>>>>     alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
>>>>
>>>>  fs/btrfs/backref.c                     |  8 ++--
>>>>  fs/btrfs/ctree.c                       | 16 ++++----
>>>>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>>>>  fs/btrfs/qgroup.c                      |  5 ++-
>>>>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>>>>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>>>>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>>>>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>>>>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>>>>  9 files changed, 68 insertions(+), 39 deletions(-)
>>>>
>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4339 bytes --]



On 2019/2/22 下午9:29, Nikolay Borisov wrote:
> 
> 
> On 22.02.19 г. 15:02 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/2/22 下午8:54, Nikolay Borisov wrote:
>>>
>>>
>>> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>>>> This patchset can be fetched from github:
>>>> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
>>>> Which is based on v5.0-rc7
>>>>
>>>> There are 5 extent buffer alloc functions in btrfs:
>>>> __alloc_extent_buffer();
>>>> alloc_extent_buffer();
>>>> __alloc_dummy_extent_buffer();
>>>> alloc_dummy_extent_buffer();
>>>> alloc_test_extent_buffer();
>>>>
>>>> However their return value is not unified for failure mode:
>>>> __alloc_extent_buffer()		Never fail
>>>> alloc_extent_buffer()		PTR_ERR()
>>>> __alloc_dummy_extent_buffer()	NULL
>>>
>>> This function can never return NULL, if __alloc_extent_buffer cannot
>>> fail then the only error this function returns is ERR_PTR(ENOMEM);
>>
>> Nope.
>>
>>         for (i = 0; i < num_pages; i++) {
>>                 eb->pages[i] = alloc_page(GFP_NOFS);
>>                 if (!eb->pages[i])
>>                         goto err; <<< Page alloc failure here
>>         }
>> ...
>> err:
>>         for (; i > 0; i--)
>>                 __free_page(eb->pages[i - 1]);
>>         __free_extent_buffer(eb);
>>         return NULL; << We got NULL.
> 
> Right, I was looking at the code AFTER having applied your patches. So I
> agree with yout. However, the ordering of your patches and the
> changelogs make it rather hard to understand. What I'd suggest regarding
> the changelogs is - forget about unification, just say what you are
> doing, which is always ensuring that an error is returned from
> __alloc_extent_buffer in one patch - this should involve both changes to
> __alloc_extent_buffer as well as it's (in)direct callers. Then you do
> the same for other function. Otherwise review is somewhat hindered.

Makes sense.

I'll reword and reorder the patches.

Thanks,
Qu

> 
>> }
>>
>> For __alloc_dummy_extent_buffer, that's the only failure case.
>>
>> And I'm interested how did you get the PTR_ERR() case?
>>
>>>
>>>> alloc_dummy_extent_buffer()	NULL
>>> Same thing applies to this function
>>
>> Nope.
>>
>>>
>>>> alloc_test_extent_buffer()	NULL
>>>
>>> Same thing for this function, if we return exists then we must have
>>> found it by find_extent_buffer hence it cannot be null. Otherwise we
>>> return eb as allocated from alloc_dummy_extent_buffer. So how can null
>>> be returned?
>>
>> And nope.
>>
>>>
>>> To me it really seems none of the function could return a NULL value, no?
>>
>> Your misunderstand of __alloc_dummy_extent_buffer() makes the call chain
>> all wrong.
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> This causes some wrapper function to have 2 failure modes, like
>>>> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
>>>> its failure.
>>>>
>>>> This inconsistent behavior is making static checker and reader crazy.
>>>>
>>>> This patchset will unify the failure more of above 5 functions to
>>>> PTR_ERR().
>>>>
>>>> Qu Wenruo (5):
>>>>   btrfs: extent_io: Add comment about the return value of
>>>>     alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
>>>>
>>>>  fs/btrfs/backref.c                     |  8 ++--
>>>>  fs/btrfs/ctree.c                       | 16 ++++----
>>>>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>>>>  fs/btrfs/qgroup.c                      |  5 ++-
>>>>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>>>>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>>>>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>>>>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>>>>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>>>>  9 files changed, 68 insertions(+), 39 deletions(-)
>>>>
>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] btrfs: extent_io: Add comment about the return value of alloc_extent_buffer()

[David Sterba]

On Fri, Feb 22, 2019 at 06:16:40PM +0800, Qu Wenruo wrote:
> To inform later developers how to check the return value of it.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 52abe4082680..b28a75546700 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4890,6 +4890,13 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>  }
>  #endif
>  
> +/*
> + * Allocate an extent buffer structure, with all its pages attached and locked.
> + *
> + * Return valid pointer if nothing goes wrong.
> + * Return PTR_ERR() if failed.
> + * Will never return NULL.

I'd rather drop this as it depends on another function and the
GFP_NOFAIL flag that will be removed eventually. The PTR_ERR covers
ENOMEM and allocation failures.

Re: [PATCH 1/5] btrfs: extent_io: Add comment about the return value of alloc_extent_buffer()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1102 bytes --]



On 2019/2/27 下午9:36, David Sterba wrote:
> On Fri, Feb 22, 2019 at 06:16:40PM +0800, Qu Wenruo wrote:
>> To inform later developers how to check the return value of it.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/extent_io.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 52abe4082680..b28a75546700 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -4890,6 +4890,13 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
>>  }
>>  #endif
>>  
>> +/*
>> + * Allocate an extent buffer structure, with all its pages attached and locked.
>> + *
>> + * Return valid pointer if nothing goes wrong.
>> + * Return PTR_ERR() if failed.
>> + * Will never return NULL.
> 
> I'd rather drop this as it depends on another function and the
> GFP_NOFAIL flag that will be removed eventually. The PTR_ERR covers
> ENOMEM and allocation failures.
> 

I'm completely OK with the removal.

Do need to resend this patch?

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/5] btrfs: extent_io: Add comment about the return value of alloc_extent_buffer()

[David Sterba]

On Wed, Feb 27, 2019 at 09:41:17PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/2/27 下午9:36, David Sterba wrote:
> > On Fri, Feb 22, 2019 at 06:16:40PM +0800, Qu Wenruo wrote:
> >> To inform later developers how to check the return value of it.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  fs/btrfs/extent_io.c | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> >> index 52abe4082680..b28a75546700 100644
> >> --- a/fs/btrfs/extent_io.c
> >> +++ b/fs/btrfs/extent_io.c
> >> @@ -4890,6 +4890,13 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
> >>  }
> >>  #endif
> >>  
> >> +/*
> >> + * Allocate an extent buffer structure, with all its pages attached and locked.
> >> + *
> >> + * Return valid pointer if nothing goes wrong.
> >> + * Return PTR_ERR() if failed.
> >> + * Will never return NULL.
> > 
> > I'd rather drop this as it depends on another function and the
> > GFP_NOFAIL flag that will be removed eventually. The PTR_ERR covers
> > ENOMEM and allocation failures.
> > 
> 
> I'm completely OK with the removal.
> 
> Do need to resend this patch?

Wait, I'm going through the rest.

Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[David Sterba]

On Fri, Feb 22, 2019 at 09:31:29PM +0800, Qu Wenruo wrote:
> >> Nope.
> >>
> >>         for (i = 0; i < num_pages; i++) {
> >>                 eb->pages[i] = alloc_page(GFP_NOFS);
> >>                 if (!eb->pages[i])
> >>                         goto err; <<< Page alloc failure here
> >>         }
> >> ...
> >> err:
> >>         for (; i > 0; i--)
> >>                 __free_page(eb->pages[i - 1]);
> >>         __free_extent_buffer(eb);
> >>         return NULL; << We got NULL.
> > 
> > Right, I was looking at the code AFTER having applied your patches. So I
> > agree with yout. However, the ordering of your patches and the
> > changelogs make it rather hard to understand. What I'd suggest regarding
> > the changelogs is - forget about unification, just say what you are
> > doing, which is always ensuring that an error is returned from
> > __alloc_extent_buffer in one patch - this should involve both changes to
> > __alloc_extent_buffer as well as it's (in)direct callers. Then you do
> > the same for other function. Otherwise review is somewhat hindered.
> 
> Makes sense.
> 
> I'll reword and reorder the patches.

Yes please, I'll have another look at the updated series. So far I agree
that the unification to IS_ERR/PTR_ERR is desired. For the plain
allocator functions that are only kmalloc+initialization it's generally
ok to return NULL and let the callers do the conversion to PTR_ERR.

In case of the eb functions, there are wrappers like
alloc_dummy_extent_buffer that only forward the return value so this
would be inconsistent and needs one to remember which functions return
what. For this reason I think it's ok to convert all.

Another question is how to do it, ideally as separate patches but the
callchains are deep and half conversion is not what we want. I'll need
to see new patches first.

Also please drop all mentions of the 'never returns NULL' in the
comments, except the function that does the GFP_NOFAIL.