Re: [PATCH] btrfs: Turn fs_roots_radix in btrfs_fs_info into an XArray

Gabriel Niebler
2022-04-27T14:51:13Z

Am 26.04.22 um 23:45 schrieb Gabriel Niebler:
> … rename it to simply fs_roots and adjust all usages of this object to use
> the XArray API, because it is notionally easier to use and unserstand, as
> it provides array semantics, and also takes care of locking for us,
> further simplifying the code.
>
> Also do some refactoring, esp. where the API change requires largely
> rewriting some functions, anyway.
>
> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
> ---
> fs/btrfs/ctree.h | 5 +-
> fs/btrfs/disk-io.c | 176 ++++++++++++++++++++---------------------
> fs/btrfs/inode.c | 13 +--
> fs/btrfs/transaction.c | 67 +++++++---------
> 4 files changed, 126 insertions(+), 135 deletions(-)
>
> [...]
> @@ -2346,28 +2340,23 @@ void btrfs_put_root(struct btrfs_root *root)
>
> void btrfs_free_fs_roots(struct btrfs_fs_info *fs_info)
> {
> - int ret;
> - struct btrfs_root *gang[8];
> - int i;
> + struct btrfs_root *root;
> + unsigned long index = 0;
>
> while (!list_empty(&fs_info->dead_roots)) {
> [...]
> - if (test_bit(BTRFS_ROOT_IN_RADIX, &gang[0]->state))
> - btrfs_drop_and_free_fs_root(fs_info, gang[0]);
> - btrfs_put_root(gang[0]);
> + if (test_bit(BTRFS_ROOT_IN_RADIX, &root->state))
> + btrfs_drop_and_free_fs_root(fs_info, root);
> + btrfs_put_root(root);

It occurs to me that BTRFS_ROOT_IN_RADIX should probably be renamed to
something else within or following this patch...

... but what to rename it /to/?

Naming things is hard. Here are some ideas I've had:

BTRFS_ROOT_IN_XARRAY is obvious, but it also includes kind of a needless
implementation detail.

BTRFS_ROOT_IN_(FS_)ROOTS would be technically accurate, but might be
confusing for the reader (e.g. if they don't know about
btrfs_fs_indo->fs_roots of the top off their head).

BTRFS_ROOT_IN_FS_INFO_ROOTS makes that a bit clearer (someone might
think it refers to fbtrfs_fs_info->roots, but when they find that
doesn't exist they'd quickly catch on, I think), but a bit lengthy.

BTRFS_ROOT_IN_FS_INFO_FS_ROOTS is probably the most accurate, but really
quite long...

Does anyone else have any ideas?