Re: [PATCH 2/2] btrfs: more trivial BTRFS_PATH_AUTO_FREE conversions

David Sterba
2025-08-29T00:02:46Z

On Wed, Aug 27, 2025 at 08:24:33PM +0800, Sun YangKai wrote:
> > On Fri, Aug 22, 2025 at 11:51:18PM +0800, Sun YangKai wrote:
> > > > Please split the patch to parts that have the described trivial changes,
> > > > and then one patch per function in case it's not trivial and needs some
> > > > adjustments.
> > >
> > > After learning more about the auto-free/cleanup mechanism, I realized that
> > > its only advantage is to eliminate the need for the goto out; pattern.
> > > Therefore, it seems unnecessary to apply this conversion in non-trivial
> > > cases.
> > I wouldn't say it's the only advantage, the code readability is also
> > improved. The path is an auxiliary object and if the freeing is handled
> > automatically then it reduces the cognitive load and the error cleanup
> > paths.
> >
> > > Moreover, if the cleanup code contains other logic, it might be better to
> > > leave it unchanged even in trivial cases.
> >
> > Depends on what we want. So far we've started with the path auto
> > cleaning but there are more possibilities like using the raw __free
> > cleanup with kfree. If this is combined and leads to simpler exit and
> > cleanup blocks I think it's worth. In the trivial cases it's clear it
> > does not interfere with the rest of the code and does not complicate any
> > logic there.
> >
> > > > The freeing followed by other code can be still converted to auto
> > > > cleaning but there must be an explicit path = NULL after the free.
> > >
> > > I'm sorry, I didn't understand. If the freeing is followed by other code,
> > > maybe we could just leave them untouched?
> >
> > Maybe yes, this is up to consideration on a per site basis, I've seen
> > examples where conversion to auto path cleaning would not hurt.
> >
> > Let me know if you want to continue with this because I think you don't
> > seem to see the value in the conversions (which is fine of course).
>
> I apologize if my previous comments came across as unclear — I certainly see
> the value in the conversions. My intention was actually to discuss in which
> specific scenarios these conversions would be most proper, so that we can align
> on how to apply them.
>
> Based on your suggestions, I'll restrict the conversions to the following
> scenarios:
>
> 1. Cases where there are no operations between btrfs_free_path and the
> function return.

OK

> 2. Cases where only simple cleanup operations (such as kfree, kvfree,
> clear_bit, and fs_path_free) are present between btrfs_free_path and the
> function return.

Also OK

> Please let me know if these criteria align with what you had in mind.

Yes please do the trivial ones described above, we'll see what's left
after that.