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

[Nikolay Borisov <nborisov@suse.com>]

On 27.04.22 г. 0:45 ч., Gabriel Niebler wrote:
> … 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(-)
> 

<snip>

> @@ -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)) {
> -		gang[0] = list_entry(fs_info->dead_roots.next,
> -				     struct btrfs_root, root_list);
> -		list_del(&gang[0]->root_list);
> +		root = list_entry(fs_info->dead_roots.next,
> +				  struct btrfs_root, root_list);
> +		list_del(&root->root_list);
>   
> -		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);
>   	}
>   
> -	while (1) {
> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> -					     (void **)gang, 0,
> -					     ARRAY_SIZE(gang));
> -		if (!ret)
> -			break;
> -		for (i = 0; i < ret; i++)
> -			btrfs_drop_and_free_fs_root(fs_info, gang[i]);
> +	while (!xa_empty(&fs_info->fs_roots))  > +		xa_for_each(&fs_info->fs_roots, index, root) {

Why do you need the nested loop structure? xa_for_each should be 
sufficient as btrfs_free_fs_roots happens when the fs is being unmounted 
and we can't get another loop added after having done xa_for_each. And 
even if it could the nested loop structure won't help because we don't 
hold any locks, so hypothetically speaking, even if there was a race 
using nested loops doesn't solve it.

TLDR: Leavint this xa_for_each should be sufficient.

> +			btrfs_drop_and_free_fs_root(fs_info, root);
> +		}
>   	}
>   }
>   

<snip>

> @@ -4491,12 +4480,12 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>   {
>   	bool drop_ref = false;
>   
> -	spin_lock(&fs_info->fs_roots_radix_lock);
> -	radix_tree_delete(&fs_info->fs_roots_radix,
> -			  (unsigned long)root->root_key.objectid);
> +	spin_lock(&fs_info->fs_roots_lock);
> +	xa_erase(&fs_info->fs_roots,
> +		 (unsigned long)root->root_key.objectid);

nit: No need to have the 2nd argument on a new line, as the line is 
short enough.
>   	if (test_and_clear_bit(BTRFS_ROOT_IN_RADIX, &root->state))
>   		drop_ref = true;
> -	spin_unlock(&fs_info->fs_roots_radix_lock);
> +	spin_unlock(&fs_info->fs_roots_lock);
>   
>   	if (BTRFS_FS_ERROR(fs_info)) {
>   		ASSERT(root->log_root == NULL);
> @@ -4512,50 +4501,54 @@ void btrfs_drop_and_free_fs_root(struct btrfs_fs_info *fs_info,
>   
>   int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>   {
> -	u64 root_objectid = 0;
> -	struct btrfs_root *gang[8];
> -	int i = 0;
> +	struct btrfs_root *roots[8];
> +	unsigned long index = 0;
> +	int i;
nit: This can be defined into the 2 loops that use the variable.


>   	int err = 0;
> -	unsigned int ret = 0;
> +	int grabbed;
>   
>   	while (1) {
> -		spin_lock(&fs_info->fs_roots_radix_lock);
> -		ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> -					     (void **)gang, root_objectid,
> -					     ARRAY_SIZE(gang));
> -		if (!ret) {
> -			spin_unlock(&fs_info->fs_roots_radix_lock);
> +		struct btrfs_root *root;
> +
> +		spin_lock(&fs_info->fs_roots_lock);
> +		if (!xa_find(&fs_info->fs_roots, &index,
> +			     ULONG_MAX, XA_PRESENT)) {
> +			spin_unlock(&fs_info->fs_roots_lock);
>   			break;
>   		}
> -		root_objectid = gang[ret - 1]->root_key.objectid + 1;
>   
> -		for (i = 0; i < ret; i++) {
> -			/* Avoid to grab roots in dead_roots */
> -			if (btrfs_root_refs(&gang[i]->root_item) == 0) {
> -				gang[i] = NULL;
> -				continue;
> +		grabbed = 0;
> +		xa_for_each_start(&fs_info->fs_roots, index, root,
> +				  index) {
> +			/* Avoid grabbing roots in dead_roots */
> +			if (btrfs_root_refs(&root->root_item) == 0) {
> +				roots[grabbed] = NULL;
> +			} else {
> +				/* Grab all the search results for later use */
> +				roots[grabbed] = btrfs_grab_root(root);
>   			}
> -			/* grab all the search result for later use */
> -			gang[i] = btrfs_grab_root(gang[i]);
> +			grabbed++;
> +			if (grabbed >= ARRAY_SIZE(roots))
> +				break;
>   		}
> -		spin_unlock(&fs_info->fs_roots_radix_lock);
> +		spin_unlock(&fs_info->fs_roots_lock);
>   
> -		for (i = 0; i < ret; i++) {
> -			if (!gang[i])
> +		for (i = 0; i < grabbed; i++) {
> +			if (!roots[i])
>   				continue;
> -			root_objectid = gang[i]->root_key.objectid;
> -			err = btrfs_orphan_cleanup(gang[i]);
> +			index = roots[i]->root_key.objectid;
> +			err = btrfs_orphan_cleanup(roots[i]);
>   			if (err)
>   				break;
> -			btrfs_put_root(gang[i]);
> +			btrfs_put_root(roots[i]);
>   		}
> -		root_objectid++;
> +		index++;
>   	}
>   
> -	/* release the uncleaned roots due to error */
> -	for (; i < ret; i++) {
> -		if (gang[i])
> -			btrfs_put_root(gang[i]);
> +	/* Release the roots that remain uncleaned due to error */
> +	for (; i < grabbed; i++) {
> +		if (roots[i])
> +			btrfs_put_root(roots[i]);
>   	}
>   	return err;
>   }
> @@ -4872,31 +4865,36 @@ static void btrfs_error_commit_super(struct btrfs_fs_info *fs_info)
>   
>   static void btrfs_drop_all_logs(struct btrfs_fs_info *fs_info)
>   {
> -	struct btrfs_root *gang[8];
> -	u64 root_objectid = 0;
> -	int ret;
> +	unsigned long index = 0;
>   
> -	spin_lock(&fs_info->fs_roots_radix_lock);
> -	while ((ret = radix_tree_gang_lookup(&fs_info->fs_roots_radix,
> -					     (void **)gang, root_objectid,
> -					     ARRAY_SIZE(gang))) != 0) {
> +	spin_lock(&fs_info->fs_roots_lock);
> +	while (xa_find(&fs_info->fs_roots,
> +		       &index, ULONG_MAX, XA_PRESENT)) {

nit: No need to split across two lines as it's short enough.

> +		struct btrfs_root *root;
> +		struct btrfs_root *roots[8];
>   		int i;

nit: This can probably be defined inside the for loop

> +		int grabbed = 0;
>   
> -		for (i = 0; i < ret; i++)
> -			gang[i] = btrfs_grab_root(gang[i]);
> -		spin_unlock(&fs_info->fs_roots_radix_lock);
> +		xa_for_each_start(&fs_info->fs_roots, index, root,
> +				  index) {
> +			roots[grabbed] = btrfs_grab_root(root);
> +			grabbed++;
> +			if (grabbed >= ARRAY_SIZE(roots))
> +				break;
> +		}
> +		spin_unlock(&fs_info->fs_roots_lock);
>   
> -		for (i = 0; i < ret; i++) {
> -			if (!gang[i])
> +		for (i = 0; i < grabbed; i++) {
> +			if (!roots[i])
>   				continue;
> -			root_objectid = gang[i]->root_key.objectid;
> -			btrfs_free_log(NULL, gang[i]);
> -			btrfs_put_root(gang[i]);
> +			index = roots[i]->root_key.objectid;
> +			btrfs_free_log(NULL, roots[i]);
> +			btrfs_put_root(roots[i]);
>   		}
> -		root_objectid++;
> -		spin_lock(&fs_info->fs_roots_radix_lock);
> +		index++;
> +		spin_lock(&fs_info->fs_roots_lock);
>   	}
> -	spin_unlock(&fs_info->fs_roots_radix_lock);
> +	spin_unlock(&fs_info->fs_roots_lock);
>   	btrfs_free_log_root_tree(NULL, fs_info);
>   }
>   
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 5082b9c70f8c..d0ef3a17ce11 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -3494,6 +3494,7 @@ int btrfs_orphan_cleanup(struct btrfs_root *root)
>   	u64 last_objectid = 0;
>   	int ret = 0, nr_unlink = 0;
>   
> +	/* Bail out if the cleanup is already running. */
nit: This seems like an irrelevant change.

>   	if (test_and_set_bit(BTRFS_ROOT_ORPHAN_CLEANUP, &root->state))
>   		return 0;
>   

<snip>

> @@ -1404,9 +1402,8 @@ void btrfs_add_dead_root(struct btrfs_root *root)
>   static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>   {
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
> -	struct btrfs_root *gang[8];
> -	int i;
> -	int ret;
> +	struct btrfs_root *root;
> +	unsigned long index;
>   
>   	/*
>   	 * At this point no one can be using this transaction to modify any tree
> @@ -1414,17 +1411,11 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>   	 */
>   	ASSERT(trans->transaction->state == TRANS_STATE_COMMIT_DOING);
>   
> -	spin_lock(&fs_info->fs_roots_radix_lock);
> -	while (1) {
> -		ret = radix_tree_gang_lookup_tag(&fs_info->fs_roots_radix,
> -						 (void **)gang, 0,
> -						 ARRAY_SIZE(gang),
> -						 BTRFS_ROOT_TRANS_TAG);
> -		if (ret == 0)
> -			break;
> -		for (i = 0; i < ret; i++) {
> -			struct btrfs_root *root = gang[i];
> -			int ret2;
> +	spin_lock(&fs_info->fs_roots_lock);
> +	while (xa_marked(&fs_info->fs_roots, BTRFS_ROOT_TRANS_TAG)) {
> +		xa_for_each_marked(&fs_info->fs_roots, index, root,

While the while/xa_for_each_marked and not straight xa_for_each_marked?

> +				   BTRFS_ROOT_TRANS_TAG) {
> +			int ret;
>   
>   			/*
>   			 * At this point we can neither have tasks logging inodes

<snip>