Re: [PATCH v5 06/18] btrfs: create qgroup earlier in snapshot creation

David Sterba
2023-09-07T15:36:10Z

On Thu, Jul 27, 2023 at 03:12:53PM -0700, Boris Burkov wrote:
> Pull creating the qgroup earlier in the snapshot. This allows simple
> quotas qgroups to see all the metadata writes related to the snapshot
> being created and to be born with the root node accounted.
>
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
> fs/btrfs/qgroup.c | 3 +++
> fs/btrfs/transaction.c | 6 ++++++
> 2 files changed, 9 insertions(+)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 18f521716e8d..8e3a4ced3077 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1672,6 +1672,9 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
> struct btrfs_qgroup *qgroup;
> int ret = 0;
>
> + if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED)
> + return 0;
> +
> mutex_lock(&fs_info->qgroup_ioctl_lock);
> if (!fs_info->quota_root) {
> ret = -ENOTCONN;
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 89ff15aa085f..25217888e897 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1722,6 +1722,12 @@ static noinline int create_pending_snapshot(struct btrfs_trans_handle *trans,
> }
> btrfs_release_path(path);
>
> + ret = btrfs_create_qgroup(trans, objectid);
> + if (ret) {
> + btrfs_abort_transaction(trans, ret);

This adds and error case to the middle of a transaction commit.
Snapshots are created in two parts, first is the ioctl adding the
structure and then commit actually creates that. So the first phase
preallocates what's needed (the root_item and path) and should do the
same with the qgroups as much as possible.

Also check all the things that btrfs_create_qgroup() does, searches the
qgroup tree, adds the new item, takes the qgroup_ioctl_lock mutex, and
adds the sysfs entry (that does allocations under GFP_KERNEL).
If you really need to create the qgroup like that then it needs much
more care.