* [PATCH] btrfs: Fix btrfs_find_create_tree_block() testing
@ 2019-12-03 9:33 Dan Carpenter
2019-12-03 11:04 ` Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-12-03 9:33 UTC (permalink / raw)
To: Chris Mason, Josef Bacik
Cc: David Sterba, Qu Wenruo, Sakari Ailus, Mike Rapoport,
Bjorn Helgaas, linux-btrfs, kernel-janitors
The btrfs_find_create_tree_block() uses alloc_dummy_extent_buffer() for
testing and alloc_extent_buffer() for production. The problem is that
the test code returns NULL and the production code returns error
pointers. The callers only check for error pointers.
I have changed alloc_dummy_extent_buffer() to return error pointers and
updated the two callers which use it directly.
Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
fs/btrfs/extent_io.c | 6 ++++--
fs/btrfs/tests/free-space-tree-tests.c | 4 ++--
fs/btrfs/tests/qgroup-tests.c | 4 ++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index eb8bd0258360..2f4802f405a2 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5074,12 +5074,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 (!eb)
- return NULL;
+ return ERR_PTR(-ENOMEM);
eb->fs_info = fs_info;
again:
ret = radix_tree_preload(GFP_NOFS);
- if (ret)
+ if (ret) {
+ exists = ERR_PTR(ret);
goto free_eb;
+ }
spin_lock(&fs_info->buffer_lock);
ret = radix_tree_insert(&fs_info->buffer_radix,
start >> PAGE_SHIFT, eb);
diff --git a/fs/btrfs/tests/free-space-tree-tests.c b/fs/btrfs/tests/free-space-tree-tests.c
index 1a846bf6e197..914eea5ba6a7 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -452,9 +452,9 @@ 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)) {
test_std_err(TEST_ALLOC_EXTENT_BUFFER);
- ret = -ENOMEM;
+ ret = PTR_ERR(root->node);
goto out;
}
btrfs_set_header_level(root->node, 0);
diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
index 09aaca1efd62..ac035a6fa003 100644
--- a/fs/btrfs/tests/qgroup-tests.c
+++ b/fs/btrfs/tests/qgroup-tests.c
@@ -484,9 +484,9 @@ 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)) {
test_err("couldn't allocate dummy buffer");
- ret = -ENOMEM;
+ ret = PTR_ERR(root->node);
goto out;
}
btrfs_set_header_level(root->node, 0);
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] btrfs: Fix btrfs_find_create_tree_block() testing
2019-12-03 9:33 [PATCH] btrfs: Fix btrfs_find_create_tree_block() testing Dan Carpenter
@ 2019-12-03 11:04 ` Mike Rapoport
2019-12-03 11:24 ` [PATCH v2] " Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2019-12-03 11:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: Chris Mason, Josef Bacik, David Sterba, Qu Wenruo, Sakari Ailus,
Bjorn Helgaas, linux-btrfs, kernel-janitors
Hi,
On Tue, Dec 03, 2019 at 12:33:04PM +0300, Dan Carpenter wrote:
> The btrfs_find_create_tree_block() uses alloc_dummy_extent_buffer() for
> testing and alloc_extent_buffer() for production. The problem is that
> the test code returns NULL and the production code returns error
> pointers. The callers only check for error pointers.
>
> I have changed alloc_dummy_extent_buffer() to return error pointers and
nit: alloc_test_extent_buffer()
> updated the two callers which use it directly.
>
> Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> fs/btrfs/extent_io.c | 6 ++++--
> fs/btrfs/tests/free-space-tree-tests.c | 4 ++--
> fs/btrfs/tests/qgroup-tests.c | 4 ++--
> 3 files changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index eb8bd0258360..2f4802f405a2 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5074,12 +5074,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 (!eb)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
> eb->fs_info = fs_info;
> again:
> ret = radix_tree_preload(GFP_NOFS);
> - if (ret)
> + if (ret) {
> + exists = ERR_PTR(ret);
> goto free_eb;
> + }
> spin_lock(&fs_info->buffer_lock);
> ret = radix_tree_insert(&fs_info->buffer_radix,
> start >> PAGE_SHIFT, eb);
> diff --git a/fs/btrfs/tests/free-space-tree-tests.c b/fs/btrfs/tests/free-space-tree-tests.c
> index 1a846bf6e197..914eea5ba6a7 100644
> --- a/fs/btrfs/tests/free-space-tree-tests.c
> +++ b/fs/btrfs/tests/free-space-tree-tests.c
> @@ -452,9 +452,9 @@ 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)) {
> test_std_err(TEST_ALLOC_EXTENT_BUFFER);
> - ret = -ENOMEM;
> + ret = PTR_ERR(root->node);
> goto out;
> }
> btrfs_set_header_level(root->node, 0);
> diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
> index 09aaca1efd62..ac035a6fa003 100644
> --- a/fs/btrfs/tests/qgroup-tests.c
> +++ b/fs/btrfs/tests/qgroup-tests.c
> @@ -484,9 +484,9 @@ 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)) {
> test_err("couldn't allocate dummy buffer");
> - ret = -ENOMEM;
> + ret = PTR_ERR(root->node);
> goto out;
> }
> btrfs_set_header_level(root->node, 0);
> --
> 2.11.0
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v2] btrfs: Fix btrfs_find_create_tree_block() testing
2019-12-03 11:04 ` Mike Rapoport
@ 2019-12-03 11:24 ` Dan Carpenter
2019-12-03 18:40 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-12-03 11:24 UTC (permalink / raw)
To: Chris Mason, Josef Bacik
Cc: David Sterba, Qu Wenruo, Sakari Ailus, Mike Rapoport,
Bjorn Helgaas, linux-btrfs, kernel-janitors
The btrfs_find_create_tree_block() uses alloc_test_extent_buffer() for
testing and alloc_extent_buffer() for production. The problem is that
the test code returns NULL and the production code returns error
pointers. The callers only check for error pointers.
I have changed alloc_test_extent_buffer() to return error pointers and
updated the two callers which use it directly.
Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: Correct the commit message. alloc_test_extent_buffer() instead of
alloc_dummy_extent_buffer().
fs/btrfs/extent_io.c | 6 ++++--
fs/btrfs/tests/free-space-tree-tests.c | 4 ++--
fs/btrfs/tests/qgroup-tests.c | 4 ++--
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index eb8bd0258360..2f4802f405a2 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5074,12 +5074,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 (!eb)
- return NULL;
+ return ERR_PTR(-ENOMEM);
eb->fs_info = fs_info;
again:
ret = radix_tree_preload(GFP_NOFS);
- if (ret)
+ if (ret) {
+ exists = ERR_PTR(ret);
goto free_eb;
+ }
spin_lock(&fs_info->buffer_lock);
ret = radix_tree_insert(&fs_info->buffer_radix,
start >> PAGE_SHIFT, eb);
diff --git a/fs/btrfs/tests/free-space-tree-tests.c b/fs/btrfs/tests/free-space-tree-tests.c
index 1a846bf6e197..914eea5ba6a7 100644
--- a/fs/btrfs/tests/free-space-tree-tests.c
+++ b/fs/btrfs/tests/free-space-tree-tests.c
@@ -452,9 +452,9 @@ 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)) {
test_std_err(TEST_ALLOC_EXTENT_BUFFER);
- ret = -ENOMEM;
+ ret = PTR_ERR(root->node);
goto out;
}
btrfs_set_header_level(root->node, 0);
diff --git a/fs/btrfs/tests/qgroup-tests.c b/fs/btrfs/tests/qgroup-tests.c
index 09aaca1efd62..ac035a6fa003 100644
--- a/fs/btrfs/tests/qgroup-tests.c
+++ b/fs/btrfs/tests/qgroup-tests.c
@@ -484,9 +484,9 @@ 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)) {
test_err("couldn't allocate dummy buffer");
- ret = -ENOMEM;
+ ret = PTR_ERR(root->node);
goto out;
}
btrfs_set_header_level(root->node, 0);
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] btrfs: Fix btrfs_find_create_tree_block() testing
2019-12-03 11:24 ` [PATCH v2] " Dan Carpenter
@ 2019-12-03 18:40 ` David Sterba
2019-12-03 18:46 ` Dan Carpenter
0 siblings, 1 reply; 6+ messages in thread
From: David Sterba @ 2019-12-03 18:40 UTC (permalink / raw)
To: Dan Carpenter
Cc: Chris Mason, Josef Bacik, David Sterba, Qu Wenruo, Sakari Ailus,
Mike Rapoport, Bjorn Helgaas, linux-btrfs, kernel-janitors
On Tue, Dec 03, 2019 at 02:24:58PM +0300, Dan Carpenter wrote:
> The btrfs_find_create_tree_block() uses alloc_test_extent_buffer() for
> testing and alloc_extent_buffer() for production. The problem is that
> the test code returns NULL and the production code returns error
> pointers. The callers only check for error pointers.
>
> I have changed alloc_test_extent_buffer() to return error pointers and
> updated the two callers which use it directly.
>
> Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
I edited the changelog because btrfs_find_create_tree_block is
misleading and seems to be unrelated to the actual fix that's just for
alloc_test_extent_buffer. Patch added to misc-next, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] btrfs: Fix btrfs_find_create_tree_block() testing
2019-12-03 18:40 ` David Sterba
@ 2019-12-03 18:46 ` Dan Carpenter
2019-12-03 19:06 ` David Sterba
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2019-12-03 18:46 UTC (permalink / raw)
To: dsterba, Chris Mason, Josef Bacik, David Sterba, Qu Wenruo,
Sakari Ailus, Mike Rapoport, Bjorn Helgaas, linux-btrfs,
kernel-janitors
On Tue, Dec 03, 2019 at 07:40:39PM +0100, David Sterba wrote:
> On Tue, Dec 03, 2019 at 02:24:58PM +0300, Dan Carpenter wrote:
> > The btrfs_find_create_tree_block() uses alloc_test_extent_buffer() for
> > testing and alloc_extent_buffer() for production. The problem is that
> > the test code returns NULL and the production code returns error
> > pointers. The callers only check for error pointers.
> >
> > I have changed alloc_test_extent_buffer() to return error pointers and
> > updated the two callers which use it directly.
> >
> > Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> I edited the changelog because btrfs_find_create_tree_block is
> misleading and seems to be unrelated to the actual fix that's just for
> alloc_test_extent_buffer. Patch added to misc-next, thanks.
The bug is in btrfs_find_create_tree_block()
fs/btrfs/disk-io.c
1046 struct extent_buffer *btrfs_find_create_tree_block(
1047 struct btrfs_fs_info *fs_info,
1048 u64 bytenr)
1049 {
1050 if (btrfs_is_testing(fs_info))
1051 return alloc_test_extent_buffer(fs_info, bytenr);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
NULL
1052 return alloc_extent_buffer(fs_info, bytenr);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error pointers.
1053 }
None of the callers of btrfs_find_create_tree_block() check for NULL.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] btrfs: Fix btrfs_find_create_tree_block() testing
2019-12-03 18:46 ` Dan Carpenter
@ 2019-12-03 19:06 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-12-03 19:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: dsterba, Chris Mason, Josef Bacik, David Sterba, Qu Wenruo,
Sakari Ailus, Mike Rapoport, Bjorn Helgaas, linux-btrfs,
kernel-janitors
On Tue, Dec 03, 2019 at 09:46:01PM +0300, Dan Carpenter wrote:
> On Tue, Dec 03, 2019 at 07:40:39PM +0100, David Sterba wrote:
> > On Tue, Dec 03, 2019 at 02:24:58PM +0300, Dan Carpenter wrote:
> > > The btrfs_find_create_tree_block() uses alloc_test_extent_buffer() for
> > > testing and alloc_extent_buffer() for production. The problem is that
> > > the test code returns NULL and the production code returns error
> > > pointers. The callers only check for error pointers.
> > >
> > > I have changed alloc_test_extent_buffer() to return error pointers and
> > > updated the two callers which use it directly.
> > >
> > > Fixes: faa2dbf004e8 ("Btrfs: add sanity tests for new qgroup accounting code")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > I edited the changelog because btrfs_find_create_tree_block is
> > misleading and seems to be unrelated to the actual fix that's just for
> > alloc_test_extent_buffer. Patch added to misc-next, thanks.
>
> The bug is in btrfs_find_create_tree_block()
>
> fs/btrfs/disk-io.c
> 1046 struct extent_buffer *btrfs_find_create_tree_block(
> 1047 struct btrfs_fs_info *fs_info,
> 1048 u64 bytenr)
> 1049 {
> 1050 if (btrfs_is_testing(fs_info))
> 1051 return alloc_test_extent_buffer(fs_info, bytenr);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> NULL
>
> 1052 return alloc_extent_buffer(fs_info, bytenr);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Error pointers.
>
> 1053 }
>
> None of the callers of btrfs_find_create_tree_block() check for NULL.
I see, though my view is a bug in alloc_test_extent_buffer that should
follow alloc_extent_buffer semantics, and this gets fixed. I'll update
the changelog again, makes sense to mention that it namely affects
btrfs_find_create_tree_block.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-03 19:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 9:33 [PATCH] btrfs: Fix btrfs_find_create_tree_block() testing Dan Carpenter
2019-12-03 11:04 ` Mike Rapoport
2019-12-03 11:24 ` [PATCH v2] " Dan Carpenter
2019-12-03 18:40 ` David Sterba
2019-12-03 18:46 ` Dan Carpenter
2019-12-03 19:06 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).