Forgot to mention, although this doesn't cause any data corruption, it
breaks snapper, which has some kind of "space aware cleaner algorithm",
which put all newly created snapshots into 1/0, but not the current root
subvolume.
And since snapper uses snapshot ioctl to assign qgroup relationship
directly, without using qgrou assign ioctl, it has no way to detect such
problem.
Hopes we can get this patch into current release cycle.
Thanks,
Qu
On 2020/4/2 下午2:37, Qu Wenruo wrote:
> [BUG]
> For the following operation, qgroup is guaranteed to be screwed up due
> to snapshot adding to a new qgroup:
>
> # mkfs.btrfs -f $dev
> # mount $dev $mnt
> # btrfs qgroup en $mnt
> # btrfs subv create $mnt/src
> # xfs_io -f -c "pwrite 0 1m" $mnt/src/file
> # sync
> # btrfs qgroup create 1/0 $mnt/src
> # btrfs subv snapshot -i 1/0 $mnt/src $mnt/snapshot
> # btrfs qgroup show -prce $mnt/src
> qgroupid rfer excl max_rfer max_excl parent child
> -------- ---- ---- -------- -------- ------ -----
> 0/5 16.00KiB 16.00KiB none none --- ---
> 0/257 1.02MiB 16.00KiB none none --- ---
> 0/258 1.02MiB 16.00KiB none none 1/0 ---
> 1/0 0.00B 0.00B none none --- 0/258
> ^^^^^^^^^^^^^^^^^^^^
>
> [CAUSE]
> The problem is in btrfs_qgroup_inherit(), we don't have good enough
> check to determine if the new relation ship would break the existing
> accounting.
>
> Unlike btrfs_add_qgroup_relation(), which has proper check to determine
> if we can do quick update without a rescan, in btrfs_qgroup_inherit() we
> can even assign a snapshot to multiple qgroups.
>
> [FIX]
> Fix the problem by manually marking qgroup inconsistent for snapshot
> inheritance.
>
> For subvolume creation, since all its extents are exclusively owned by
> itself, we don't need to rescan.
>
> In theory, we should call relationship check like quick_update_accounting()
> when doing qgroup inheritance and inform user about qgroup inconsistent.
>
> But we don't have good enough mechanism to inform user in the snapshot
> creation context, thus we can only silently mark the qgroup
> inconsistent.
>
> Anyway, user shouldn't use qgroup inheritance during snapshot creation,
> and should add qgroup relationship after snapshot creation by 'btrfs
> qgroup assign', which has a much better UI to inform user about qgroup
> inconsistent and kick in rescan automatically.
>
> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
> ---
> fs/btrfs/qgroup.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index c3888fb367e7..81b2efca48b4 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -2622,6 +2622,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> struct btrfs_root *quota_root;
> struct btrfs_qgroup *srcgroup;
> struct btrfs_qgroup *dstgroup;
> + bool need_rescan = false;
> u32 level_size = 0;
> u64 nums;
>
> @@ -2765,6 +2766,13 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> goto unlock;
> }
> ++i_qgroups;
> +
> + /*
> + * If we're doing a snapshot, and adding the snapshot to a new
> + * qgroup, the numbers are guaranteed to be incorrect.
> + */
> + if (srcid)
> + need_rescan = true;
> }
>
> for (i = 0; i < inherit->num_ref_copies; ++i, i_qgroups += 2) {
> @@ -2784,6 +2792,9 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>
> dst->rfer = src->rfer - level_size;
> dst->rfer_cmpr = src->rfer_cmpr - level_size;
> +
> + /* Manually tweaking numbers? No way to keep qgroup sane */
> + need_rescan = true;
> }
> for (i = 0; i < inherit->num_excl_copies; ++i, i_qgroups += 2) {
> struct btrfs_qgroup *src;
> @@ -2802,6 +2813,7 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
>
> dst->excl = src->excl + level_size;
> dst->excl_cmpr = src->excl_cmpr + level_size;
> + need_rescan = true;
> }
>
> unlock:
> @@ -2809,6 +2821,8 @@ int btrfs_qgroup_inherit(struct btrfs_trans_handle *trans, u64 srcid,
> out:
> if (!committing)
> mutex_unlock(&fs_info->qgroup_ioctl_lock);
> + if (need_rescan)
> + fs_info->qgroup_flags |= BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
> return ret;
> }
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
