Re: [PATCH 2/2] btrfs: use xarray for btrfs_root::delayed_nodes_tree instead of radix-tree

[David Sterba <dsterba@suse.cz>]

On Fri, Dec 01, 2023 at 11:03:25AM +0000, Filipe Manana wrote:
> On Thu, Nov 30, 2023 at 10:56 PM David Sterba <dsterba@suse.com> wrote:
> >
> > Port btrfs_root::delayed_nodes_tree to the xarray API. The functionality
> > is equivalent, the flags are still GFP_ATOMIC as the changes are done
> > under a spin lock. Using a sleeping allocation would need changing the
> > lock to mutex.
> >
> > The conversion is almost direct, btrfs_kill_all_delayed_nodes() uses an
> > iterator to collect the items to delete.
> >
> > The patch is almost the same as 253bf57555e451 ("btrfs: turn
> > delayed_nodes_tree into an XArray"), there are renames, comments and
> > change of the GFP flags for xa_insert.
> 
> Can we include in the changelog why we are converting from radix tree to xarray?
> What do we gain with it? Why not the more recent maple tree for example?

I haven't evaluated maple tree for as a replacement. Regarding the
radix-tree, it's actually implemeted by xarray so it's more or less just
an API change. The reason to remove radix-tree is that it's an obsolete
API, https://lwn.net/Articles/745073/

"The current patch set converts a number of radix-tree users to
XArrays, but some still remain. If all goes according to Wilcox's plan,
though, those will be converted in the near future and the radix-tree
API will head toward removal."

> Codewise this is about the same amount of LOCs, and I don't find it
> either particularly simpler or more complex either.
> Performance wise, there's no indication of anything.

Yeah, line count should not matter, performance should be the same
(because it's the same underlying data structure).

> It also removed the preallocation using GFP_NOFS and now depends on
> atomic allocations on insertions.
> This is odd because we've been making efforts over time to eliminate
> atomic allocations in order to reduce the chances of allocation
> failures,
> yet we are now allowing for more atomic allocations without mentioning
> what we gain in return.

Right now it's because of the API conversion.  The preload mechanism is
gone from the API, we might be able to add it using the specialized API
or xa_reserve. The preload disables preemption so we can't use sleeping
allocations like GFP_NOFS in xa_insert(). This would need the spinlock
-> mutex conversion.

I'd like to get rid of the atomic allocations too, but wuold like to do
it in steps. The radix-tree -> xarray is almost direct, switching
preload to atomic allocations is potentially problematic but only in
some cases. We have other atomic allocations and it does not seem to be
causing problems.

The long term plan:

- radix-tree -> xarray
- spinlock -> mutex
- no atomic allocations for xarrays