Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

Filipe Manana
2025-04-16T19:41:33Z

On Wed, Apr 16, 2025 at 8:11 PM David Sterba <dsterba@suse.cz> wrote:
>
> On Wed, Apr 16, 2025 at 02:37:33PM +0100, Filipe Manana wrote:
> > On Wed, Apr 16, 2025 at 2:24 PM 李扬韬 <frank.li@vivo.com> wrote:
> > >
> > >
> > >
> > > > Also a good point, the path should be in a pristine state, as if it were just allocated. Releasing paths in other functions may want to keep the bits but in this case we're crossing a function boundary and the same assumptions may not be the same.
> > >
> > > > Release resets the ->nodes, so what's left is from ->slots until the the end of the structure. And a helper for that would be desirable rather than opencoding that.
> > >
> > > IIUC, use btrfs_reset_path instead of btrfs_release_path?
> > >
> > > noinline void btrfs_reset_path(struct btrfs_path *p)
> > > {
> > > int i;
> > >
> > > for (i = 0; i < BTRFS_MAX_LEVEL; i++) {
> > > if (!p->nodes[i])
> > > continue;
> > > if (p->locks[i])
> > > btrfs_tree_unlock_rw(p->nodes[i], p->locks[i]);
> > > free_extent_buffer(p->nodes[i]);
> > > }
> > > memset(p, 0, sizeof(struct btrfs_path));
> > > }
> > >
> > > BTW, I have seen released paths being passed across functions in some other paths.
> > >
> > > Should these also be changed to reset paths, or should these flags be cleared in the release path?
> >
> > Please don't complicate things unnecessarily.
> > The patch is fine, all that needs to be done is to call
> > btrfs_release_path() before passing the path to
> > btrfs_del_inode_extref(), which resets nodes, slots and locks.
>
> But this leaves the bits set, btrfs_insert_inode_ref() sets
> path->skip_release_on_error, this should be reset. In this case it may
> not be significant but I'd rather make the path reusing pattern correct
> from the beginning.
>
> My idea was to add only
>
> btrfs_reset_path() {
> memset(p, 0, sizeof(struct btrfs_path));
> }
>
> and use it in conection with btrfs_release_path() only in case it's
> optimizing the allocation.

Honestly I don't like adding yet another function to do such "reset" thing.

Leaving path->skip_release_on_error is perfectly fine in this scenario.
If that bothers anyone so much, just set path->skip_release_on_error
to 0 after calling btrfs_release_path() and before passing the path to
btrfs_insert_inode_extref().

This is the sort of optimization that is not worth spending this much
time and adding new APIs - freeing and allocating a path shortly after
is almost always fast as we're using a slab, plus this is a rarely hit
use case - having to use extrefs, meaning we have a very large number
of inode refs.