[PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Sun YangKai]

Restructure the error handling flow in btrfs_tree_mod_log_insert_key
to address memory allocation failures more cleanly.

No functional changes are made - this is purely a code readability
improvement.

Signed-off-by: Sun YangKai <sunk67188@gmail.com>
---
 fs/btrfs/tree-mod-log.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/tree-mod-log.c b/fs/btrfs/tree-mod-log.c
index 9e8cb3b7c064..4fd7859ad7dc 100644
--- a/fs/btrfs/tree-mod-log.c
+++ b/fs/btrfs/tree-mod-log.c
@@ -267,9 +267,8 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
 	if (!tree_mod_need_log(eb->fs_info, eb))
 		return 0;
 
+	/* Allocation error is handled later. */
 	tm = alloc_tree_mod_elem(eb, slot, op);
-	if (!tm)
-		ret = -ENOMEM;
 
 	if (tree_mod_dont_log(eb->fs_info, eb)) {
 		kfree(tm);
@@ -278,16 +277,14 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
 		 * need to log.
 		 */
 		return 0;
-	} else if (ret != 0) {
-		/*
-		 * We previously failed to allocate memory and we need to log,
-		 * so we have to fail.
-		 */
-		goto out_unlock;
 	}
 
-	ret = tree_mod_log_insert(eb->fs_info, tm);
-out_unlock:
+	/* Deal with allocation error. */
+	if (tm)
+		ret = tree_mod_log_insert(eb->fs_info, tm);
+	else
+		ret = -ENOMEM;
+
 	write_unlock(&eb->fs_info->tree_mod_log_lock);
 	if (ret)
 		kfree(tm);
-- 
2.51.0

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Qu Wenruo]

在 2025/9/29 16:26, Sun YangKai 写道:
> Restructure the error handling flow in btrfs_tree_mod_log_insert_key
> to address memory allocation failures more cleanly.
> 
> No functional changes are made - this is purely a code readability
> improvement.
> 
> Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> ---
>   fs/btrfs/tree-mod-log.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/tree-mod-log.c b/fs/btrfs/tree-mod-log.c
> index 9e8cb3b7c064..4fd7859ad7dc 100644
> --- a/fs/btrfs/tree-mod-log.c
> +++ b/fs/btrfs/tree-mod-log.c
> @@ -267,9 +267,8 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
>   	if (!tree_mod_need_log(eb->fs_info, eb))
>   		return 0;
>   
> +	/* Allocation error is handled later. */
>   	tm = alloc_tree_mod_elem(eb, slot, op);
> -	if (!tm)
> -		ret = -ENOMEM;
>   
>   	if (tree_mod_dont_log(eb->fs_info, eb)) {
>   		kfree(tm);
> @@ -278,16 +277,14 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
>   		 * need to log.
>   		 */
>   		return 0;
> -	} else if (ret != 0) {
> -		/*
> -		 * We previously failed to allocate memory and we need to log,
> -		 * so we have to fail.
> -		 */
> -		goto out_unlock;
>   	}
>   
> -	ret = tree_mod_log_insert(eb->fs_info, tm);
> -out_unlock:
> +	/* Deal with allocation error. */
> +	if (tm)
> +		ret = tree_mod_log_insert(eb->fs_info, tm);

Sorry, I don't think this is really that better.

In fact I'm wondering why we don't delay the allocation after 
tree_mod_dont_log()?

That would be way more straightforward.

Thanks,
Qu
> +	else
> +		ret = -ENOMEM;
> +
>   	write_unlock(&eb->fs_info->tree_mod_log_lock);
>   	if (ret)
>   		kfree(tm);

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Sun YangKai]

> > -	ret = tree_mod_log_insert(eb->fs_info, tm);
> > -out_unlock:
> > +	/* Deal with allocation error. */
> > +	if (tm)
> > +		ret = tree_mod_log_insert(eb->fs_info, tm);
> 
> Sorry, I don't think this is really that better.
> 
> In fact I'm wondering why we don't delay the allocation after
> tree_mod_dont_log()?

That's what I want to do at first. However, after looking further into 
tree_mod_dont_log(), I found that it trys to get the tree_mod_log_lock. I'm 
not sure if it's better or worse to do the allocation things after getting the 
lock.

> 
> That would be way more straightforward.
> 
> Thanks,
> Qu
> 
> > +	else
> > +		ret = -ENOMEM;
> > +
> > 

Thanks,
Sun YangKai

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Filipe Manana]

On Mon, Sep 29, 2025 at 9:29 AM Sun YangKai <sunk67188@gmail.com> wrote:
>
> > > -   ret = tree_mod_log_insert(eb->fs_info, tm);
> > > -out_unlock:
> > > +   /* Deal with allocation error. */
> > > +   if (tm)
> > > +           ret = tree_mod_log_insert(eb->fs_info, tm);
> >
> > Sorry, I don't think this is really that better.
> >
> > In fact I'm wondering why we don't delay the allocation after
> > tree_mod_dont_log()?
>
> That's what I want to do at first. However, after looking further into
> tree_mod_dont_log(), I found that it trys to get the tree_mod_log_lock. I'm
> not sure if it's better or worse to do the allocation things after getting the
> lock.

You can't do a GFP_NOFS allocation after acquiring a rw lock... The
only allocation that can be done is a GFP_ATOMIC in that context, and
we want to avoid them.

>
> >
> > That would be way more straightforward.
> >
> > Thanks,
> > Qu
> >
> > > +   else
> > > +           ret = -ENOMEM;
> > > +
> > >
>
> Thanks,
> Sun YangKai
>
>
>
>
>

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Qu Wenruo]

在 2025/9/29 18:02, Filipe Manana 写道:
> On Mon, Sep 29, 2025 at 9:29 AM Sun YangKai <sunk67188@gmail.com> wrote:
>>
>>>> -   ret = tree_mod_log_insert(eb->fs_info, tm);
>>>> -out_unlock:
>>>> +   /* Deal with allocation error. */
>>>> +   if (tm)
>>>> +           ret = tree_mod_log_insert(eb->fs_info, tm);
>>>
>>> Sorry, I don't think this is really that better.
>>>
>>> In fact I'm wondering why we don't delay the allocation after
>>> tree_mod_dont_log()?
>>
>> That's what I want to do at first. However, after looking further into
>> tree_mod_dont_log(), I found that it trys to get the tree_mod_log_lock. I'm
>> not sure if it's better or worse to do the allocation things after getting the
>> lock.
> 
> You can't do a GFP_NOFS allocation after acquiring a rw lock... The
> only allocation that can be done is a GFP_ATOMIC in that context, and
> we want to avoid them.

You're both right. Forgot it's a rwlock.

There is no better solution, I guess it's the best solution for now.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> 
>>
>>>
>>> That would be way more straightforward.
>>>
>>> Thanks,
>>> Qu
>>>
>>>> +   else
>>>> +           ret = -ENOMEM;
>>>> +
>>>>
>>
>> Thanks,
>> Sun YangKai
>>
>>
>>
>>
>>

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Filipe Manana]

On Mon, Sep 29, 2025 at 8:31 AM Sun YangKai <sunk67188@gmail.com> wrote:
>
> Restructure the error handling flow in btrfs_tree_mod_log_insert_key
> to address memory allocation failures more cleanly.
>
> No functional changes are made - this is purely a code readability
> improvement.
>
> Signed-off-by: Sun YangKai <sunk67188@gmail.com>
> ---
>  fs/btrfs/tree-mod-log.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/tree-mod-log.c b/fs/btrfs/tree-mod-log.c
> index 9e8cb3b7c064..4fd7859ad7dc 100644
> --- a/fs/btrfs/tree-mod-log.c
> +++ b/fs/btrfs/tree-mod-log.c
> @@ -267,9 +267,8 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
>         if (!tree_mod_need_log(eb->fs_info, eb))
>                 return 0;
>
> +       /* Allocation error is handled later. */
>         tm = alloc_tree_mod_elem(eb, slot, op);
> -       if (!tm)
> -               ret = -ENOMEM;

I prefer the current way we do things.
First it's less confusing to see some action right after the memory
allocation, rather than much later like this patch proposes.

>
>         if (tree_mod_dont_log(eb->fs_info, eb)) {
>                 kfree(tm);
> @@ -278,16 +277,14 @@ int btrfs_tree_mod_log_insert_key(const struct extent_buffer *eb, int slot,
>                  * need to log.
>                  */
>                 return 0;
> -       } else if (ret != 0) {
> -               /*
> -                * We previously failed to allocate memory and we need to log,
> -                * so we have to fail.
> -                */
> -               goto out_unlock;

Second, having this check and comment here makes it very explicit when
the memory allocation is a problem.

>         }
>
> -       ret = tree_mod_log_insert(eb->fs_info, tm);
> -out_unlock:
> +       /* Deal with allocation error. */

This is a useless comment... The existing comment is much more helpful
and the fact that's in the else statement above, makes it easier to
grok.

I also wonder why you picked only this function, since this pattern is
followed in several other functions...

Not a fan of the proposed change.

> +       if (tm)
> +               ret = tree_mod_log_insert(eb->fs_info, tm);
> +       else
> +               ret = -ENOMEM;
> +
>         write_unlock(&eb->fs_info->tree_mod_log_lock);
>         if (ret)
>                 kfree(tm);
> --
> 2.51.0
>
>

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Sun YangKai]

> > -       ret = tree_mod_log_insert(eb->fs_info, tm);
> > -out_unlock:
> > +       /* Deal with allocation error. */
> 
> This is a useless comment... The existing comment is much more helpful
> and the fact that's in the else statement above, makes it easier to
> grok.

I agree that the existing comment is much more helpful, while I personally 
don't like the else branch appears after a return/goto/continue/break 
statement. So I think it's just about personal code style.

> I also wonder why you picked only this function, since this pattern is
> followed in several other functions...

Because I happened to read this, and it takes me minutes to realise what is 
happening and why it was written like this...
And I'm not sure how to make it better. Since this is the most simple one with 
this pattern, I just have a try to make it more clear IMO.

> Not a fan of the proposed change.
> 
> > +       if (tm)
> > +               ret = tree_mod_log_insert(eb->fs_info, tm);
> > +       else
> > +               ret = -ENOMEM;
> > +
> > 
> >         write_unlock(&eb->fs_info->tree_mod_log_lock);
> >         if (ret)
> >         
> >                 kfree(tm);
> > 
> > --
> > 2.51.0

Re: [PATCH] btrfs: reorganize error handling in btrfs_tree_mod_log_insert_key

[Filipe Manana]

On Mon, Sep 29, 2025 at 11:24 AM Sun YangKai <sunk67188@gmail.com> wrote:
>
> > > -       ret = tree_mod_log_insert(eb->fs_info, tm);
> > > -out_unlock:
> > > +       /* Deal with allocation error. */
> >
> > This is a useless comment... The existing comment is much more helpful
> > and the fact that's in the else statement above, makes it easier to
> > grok.
>
> I agree that the existing comment is much more helpful, while I personally
> don't like the else branch appears after a return/goto/continue/break
> statement. So I think it's just about personal code style.
>
> > I also wonder why you picked only this function, since this pattern is
> > followed in several other functions...
>
> Because I happened to read this, and it takes me minutes to realise what is
> happening and why it was written like this...

Well, having to spend minutes to understand something isn't unusual,
even for more experienced people.
You can't expect to understand all the details of the tree mod log in
just a few minutes, that's unreasonable.

> And I'm not sure how to make it better. Since this is the most simple one with
> this pattern, I just have a try to make it more clear IMO.

This is a short function and I find the existing flow and comments a
lot better to understand (well, I wrote them actually).

>
> > Not a fan of the proposed change.
> >
> > > +       if (tm)
> > > +               ret = tree_mod_log_insert(eb->fs_info, tm);
> > > +       else
> > > +               ret = -ENOMEM;
> > > +
> > >
> > >         write_unlock(&eb->fs_info->tree_mod_log_lock);
> > >         if (ret)
> > >
> > >                 kfree(tm);
> > >
> > > --
> > > 2.51.0
>
>
>
>