linux-kernel - Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAL3q7H4_vNoKokn213rY2Q0MNkDLWSk7XRBqJLxfiw1ikRGM7Q@mail.gmail.com>
Date: Wed, 16 Apr 2025 20:40:54 +0100
From: Filipe Manana <fdmanana@...nel.org>
To: dsterba@...e.cz
Cc: 李扬韬 <frank.li@...o.com>, 
	Sun YangKai <sunk67188@...il.com>, "clm@...com" <clm@...com>, "dsterba@...e.com" <dsterba@...e.com>, 
	"josef@...icpanda.com" <josef@...icpanda.com>, 
	"linux-btrfs@...r.kernel.org" <linux-btrfs@...r.kernel.org>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "neelx@...e.com" <neelx@...e.com>
Subject: Re: [PATCH 1/3] btrfs: get rid of path allocation in btrfs_del_inode_extref()

On Wed, Apr 16, 2025 at 8:11 PM David Sterba <dsterba@...e.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@...o.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.