Re: [PATCH 08/18] btrfs: function for recording simple quota deltas

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Jul 05, 2023 at 04:20:45PM -0700, Boris Burkov wrote:
> Rather than re-computing shared/exclusive ownership based on backrefs
> and walking roots for implicit backrefs, simple quotas does an increment
> when creating an extent and a decrement when deleting it. Add the API
> for the extent item code to use to track those events.
> 
> Also add a helper function to make collecting parent qgroups in a ulist
> easier for functions like this.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/qgroup.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++
>  fs/btrfs/qgroup.h | 11 ++++++-
>  2 files changed, 92 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8419270f7417..97c00697b475 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -333,6 +333,44 @@ static int del_relation_rb(struct btrfs_fs_info *fs_info,
>  	return -ENOENT;
>  }
>  
> +static int qgroup_collect_parents(struct btrfs_qgroup *qgroup,
> +				  struct ulist *ul)
> +{
> +	struct ulist_iterator uiter;
> +	struct ulist_node *unode;
> +	struct btrfs_qgroup_list *glist;
> +	struct btrfs_qgroup *qg;
> +	bool err_free = false;
> +	int ret = 0;
> +
> +	if (!ul) {
> +		ul = ulist_alloc(GFP_KERNEL);
> +		err_free = true;
> +	} else {
> +		ulist_reinit(ul);
> +	}

You're calling this under a spin_lock, so GFP_KERNEL isn't allowed here.
Additionally it doesn't seem like you ever call it with a NULL ulist, so
probably just get rid of the above.

> +
> +	ret = ulist_add(ul, qgroup->qgroupid,
> +			qgroup_to_aux(qgroup), GFP_ATOMIC);

We're going to hit ENOMEM in production here, we used to hit it all the time
with the extent state stuff.  How will this affect us?  Will it screw up
accounting?  If so we need to figure out a way to back up out of the lock and
pre-allocate a node to use in this case.

> +	if (ret < 0)
> +		goto out;
> +	ULIST_ITER_INIT(&uiter);
> +	while ((unode = ulist_next(ul, &uiter))) {
> +		qg = unode_aux_to_qgroup(unode);
> +		list_for_each_entry(glist, &qg->groups, next_group) {
> +			ret = ulist_add(ul, glist->group->qgroupid,
> +					qgroup_to_aux(glist->group), GFP_ATOMIC);
> +			if (ret < 0)
> +				goto out;
> +		}
> +	}
> +	ret = 0;
> +out:
> +	if (ret && err_free)
> +		ulist_free(ul);
> +	return ret;
> +}
> +
>  #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>  int btrfs_verify_qgroup_counts(struct btrfs_fs_info *fs_info, u64 qgroupid,
>  			       u64 rfer, u64 excl)
> @@ -4531,3 +4569,47 @@ void btrfs_qgroup_destroy_extent_records(struct btrfs_transaction *trans)
>  		kfree(entry);
>  	}
>  }
> +
> +int btrfs_record_simple_quota_delta(struct btrfs_fs_info *fs_info,
> +				    struct btrfs_simple_quota_delta *delta)
> +{
> +	int ret;
> +	struct ulist *ul = fs_info->qgroup_ulist;

This isn't NULL if we're enabled right?  Thanks,

Josef