[PATCH 0/2] btrfs: return early in allocation failures

[PATCH 0/2] btrfs: return early in allocation failures

[Miquel Sabaté Solà]

Minor cleanups I saw when I was working on [1]. Both commits are related to
a pattern where two pointers were being initialized and checked after that.

[1] https://lore.kernel.org/linux-btrfs/20260223234451.277369-1-mssola@mssola.com/

Miquel Sabaté Solà (2):
  btrfs: return early if allocations fail on add_block_entry()
  btrfs: return early if allocations fail on raid56

 fs/btrfs/raid56.c     | 29 ++++++++++++++++++-----------
 fs/btrfs/ref-verify.c | 10 ++++++----
 2 files changed, 24 insertions(+), 15 deletions(-)

--
2.53.0

[PATCH 1/2] btrfs: return early if allocations fail on add_block_entry()

[Miquel Sabaté Solà]

In add_block_entry(), if the allocation of 're' fails, return right away
instead of trying to allocate 'be' next. This also removes a useless
kfree() call.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/ref-verify.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index f78369ff2a66..7b4d52db7897 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -246,14 +246,16 @@ static struct block_entry *add_block_entry(struct btrfs_fs_info *fs_info,
 					   u64 bytenr, u64 len,
 					   u64 root_objectid)
 {
-	struct block_entry *be = NULL, *exist;
-	struct root_entry *re = NULL;
+	struct block_entry *be, *exist;
+	struct root_entry *re;
 
 	re = kzalloc_obj(struct root_entry, GFP_NOFS);
+	if (!re)
+		return ERR_PTR(-ENOMEM);
+
 	be = kzalloc_obj(struct block_entry, GFP_NOFS);
-	if (!be || !re) {
+	if (!be) {
 		kfree(re);
-		kfree(be);
 		return ERR_PTR(-ENOMEM);
 	}
 	be->bytenr = bytenr;
-- 
2.53.0

Re: [PATCH 1/2] btrfs: return early if allocations fail on add_block_entry()

[Qu Wenruo]

在 2026/2/28 01:47, Miquel Sabaté Solà 写道:
> In add_block_entry(), if the allocation of 're' fails, return right away
> instead of trying to allocate 'be' next. This also removes a useless
> kfree() call.
> 
> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>

Please don't.

It's a common pattern for multiple allocations, and it's very expandable.
If you need to allocate new things, just a new allocation and add the 
pointer check into the if () condition.

It's way more expandable than checking each return pointer.

Thanks,
Qu

> ---
>   fs/btrfs/ref-verify.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
> index f78369ff2a66..7b4d52db7897 100644
> --- a/fs/btrfs/ref-verify.c
> +++ b/fs/btrfs/ref-verify.c
> @@ -246,14 +246,16 @@ static struct block_entry *add_block_entry(struct btrfs_fs_info *fs_info,
>   					   u64 bytenr, u64 len,
>   					   u64 root_objectid)
>   {
> -	struct block_entry *be = NULL, *exist;
> -	struct root_entry *re = NULL;
> +	struct block_entry *be, *exist;
> +	struct root_entry *re;
>   
>   	re = kzalloc_obj(struct root_entry, GFP_NOFS);
> +	if (!re)
> +		return ERR_PTR(-ENOMEM);
> +
>   	be = kzalloc_obj(struct block_entry, GFP_NOFS);
> -	if (!be || !re) {
> +	if (!be) {
>   		kfree(re);
> -		kfree(be);
>   		return ERR_PTR(-ENOMEM);
>   	}
>   	be->bytenr = bytenr;

[PATCH 2/2] btrfs: return early if allocations fail on raid56

[Miquel Sabaté Solà]

In both the recover_sectors() and the recover_scrub_rbio() functions we
initialized two pointers by allocating them, and then returned early if
either of them failed. But we can simply allocate the first one and do
the check, and repeat for the second pointer. This way we return earlier
on allocation failures, and we don't perform unneeded kfree() calls.

Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>
---
 fs/btrfs/raid56.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
index e31d57d6ab1e..c8ece97259e3 100644
--- a/fs/btrfs/raid56.c
+++ b/fs/btrfs/raid56.c
@@ -2094,8 +2094,8 @@ static int recover_vertical(struct btrfs_raid_bio *rbio, int sector_nr,
 
 static int recover_sectors(struct btrfs_raid_bio *rbio)
 {
-	void **pointers = NULL;
-	void **unmap_array = NULL;
+	void **pointers;
+	void **unmap_array;
 	int sectornr;
 	int ret = 0;
 
@@ -2105,11 +2105,15 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
 	 * @unmap_array stores copy of pointers that does not get reordered
 	 * during reconstruction so that kunmap_local works.
 	 */
+
 	pointers = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
+	if (!pointers)
+		return -ENOMEM;
+
 	unmap_array = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
-	if (!pointers || !unmap_array) {
-		ret = -ENOMEM;
-		goto out;
+	if (!unmap_array) {
+		kfree(pointers);
+		return -ENOMEM;
 	}
 
 	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
@@ -2126,7 +2130,6 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
 			break;
 	}
 
-out:
 	kfree(pointers);
 	kfree(unmap_array);
 	return ret;
@@ -2828,8 +2831,8 @@ static inline int is_data_stripe(struct btrfs_raid_bio *rbio, int stripe)
 
 static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
 {
-	void **pointers = NULL;
-	void **unmap_array = NULL;
+	void **pointers;
+	void **unmap_array;
 	int sector_nr;
 	int ret = 0;
 
@@ -2839,11 +2842,15 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
 	 * @unmap_array stores copy of pointers that does not get reordered
 	 * during reconstruction so that kunmap_local works.
 	 */
+
 	pointers = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
+	if (!pointers)
+		return -ENOMEM;
+
 	unmap_array = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
-	if (!pointers || !unmap_array) {
-		ret = -ENOMEM;
-		goto out;
+	if (!unmap_array) {
+		kfree(pointers);
+		return -ENOMEM;
 	}
 
 	for (sector_nr = 0; sector_nr < rbio->stripe_nsectors; sector_nr++) {
-- 
2.53.0

Re: [PATCH 2/2] btrfs: return early if allocations fail on raid56

[Qu Wenruo]

在 2026/2/28 01:47, Miquel Sabaté Solà 写道:
> In both the recover_sectors() and the recover_scrub_rbio() functions we
> initialized two pointers by allocating them, and then returned early if
> either of them failed. But we can simply allocate the first one and do
> the check, and repeat for the second pointer. This way we return earlier
> on allocation failures, and we don't perform unneeded kfree() calls.
> 
> Signed-off-by: Miquel Sabaté Solà <mssola@mssola.com>

Again, the old pattern is just fine.

Thanks,
Qu

> ---
>   fs/btrfs/raid56.c | 29 ++++++++++++++++++-----------
>   1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
> index e31d57d6ab1e..c8ece97259e3 100644
> --- a/fs/btrfs/raid56.c
> +++ b/fs/btrfs/raid56.c
> @@ -2094,8 +2094,8 @@ static int recover_vertical(struct btrfs_raid_bio *rbio, int sector_nr,
>   
>   static int recover_sectors(struct btrfs_raid_bio *rbio)
>   {
> -	void **pointers = NULL;
> -	void **unmap_array = NULL;
> +	void **pointers;
> +	void **unmap_array;
>   	int sectornr;
>   	int ret = 0;
>   
> @@ -2105,11 +2105,15 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
>   	 * @unmap_array stores copy of pointers that does not get reordered
>   	 * during reconstruction so that kunmap_local works.
>   	 */
> +
>   	pointers = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
> +	if (!pointers)
> +		return -ENOMEM;
> +
>   	unmap_array = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
> -	if (!pointers || !unmap_array) {
> -		ret = -ENOMEM;
> -		goto out;
> +	if (!unmap_array) {
> +		kfree(pointers);
> +		return -ENOMEM;
>   	}
>   
>   	if (rbio->operation == BTRFS_RBIO_READ_REBUILD) {
> @@ -2126,7 +2130,6 @@ static int recover_sectors(struct btrfs_raid_bio *rbio)
>   			break;
>   	}
>   
> -out:
>   	kfree(pointers);
>   	kfree(unmap_array);
>   	return ret;
> @@ -2828,8 +2831,8 @@ static inline int is_data_stripe(struct btrfs_raid_bio *rbio, int stripe)
>   
>   static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
>   {
> -	void **pointers = NULL;
> -	void **unmap_array = NULL;
> +	void **pointers;
> +	void **unmap_array;
>   	int sector_nr;
>   	int ret = 0;
>   
> @@ -2839,11 +2842,15 @@ static int recover_scrub_rbio(struct btrfs_raid_bio *rbio)
>   	 * @unmap_array stores copy of pointers that does not get reordered
>   	 * during reconstruction so that kunmap_local works.
>   	 */
> +
>   	pointers = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
> +	if (!pointers)
> +		return -ENOMEM;
> +
>   	unmap_array = kzalloc_objs(void *, rbio->real_stripes, GFP_NOFS);
> -	if (!pointers || !unmap_array) {
> -		ret = -ENOMEM;
> -		goto out;
> +	if (!unmap_array) {
> +		kfree(pointers);
> +		return -ENOMEM;
>   	}
>   
>   	for (sector_nr = 0; sector_nr < rbio->stripe_nsectors; sector_nr++) {