[PATCH 0/3] btrfs: squota inheritance improvements

[PATCH 0/3] btrfs: squota inheritance improvements

[Boris Burkov]

One bug fix for an off-by-one metadata accounting error and two generic
improvements. Details in the patches.

Boris Burkov (3):
  btrfs: fix qgroup_snapshot_quick_inherit() squota bug
  btrfs: check squota parent usage on membership change
  btrfs: relax squota parent qgroup deletion rule

 fs/btrfs/qgroup.c | 93 +++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 15 deletions(-)

-- 
2.52.0

[PATCH 1/3] btrfs: fix qgroup_snapshot_quick_inherit() squota bug

[Boris Burkov]

qgroup_snapshot_quick_inherit() detects conditions where the snapshot
destination would land in the same parent qgroup as the snapshot source
subvolume. In this case we can avoid costly qgroup calculations and just
add the nodesize of the new snapshot to the parent.

However, in the case of squotas this is actually a double count, and
also an undercount for deeper qgroup nestings.

The following annotated script shows the issue:

  btrfs quota enable --simple "$mnt"

  # Create 2-level qgroup hierarchy
  btrfs qgroup create 2/100 "$mnt"  # Q2 (level 2)
  btrfs qgroup create 1/100 "$mnt"  # Q1 (level 1)
  btrfs qgroup assign 1/100 2/100 "$mnt"

  # Create base subvolume
  btrfs subvolume create "$mnt/base" >/dev/null
  base_id=$(btrfs subvolume show "$mnt/base" | grep 'Subvolume ID:' | awk '{print $3}')

  # Create intermediate snapshot and add to Q1
  btrfs subvolume snapshot "$mnt/base" "$mnt/intermediate" >/dev/null
  inter_id=$(btrfs subvolume show "$mnt/intermediate" | grep 'Subvolume ID:' | awk '{print $3}')
  btrfs qgroup assign "0/$inter_id" 1/100 "$mnt"

  # Create working snapshot with --inherit (auto-adds to Q1)
  # src=intermediate (in only Q1)
  # dst=snap (inheriting only into Q1)
  # This double counts the 16k nodesize of the snapshot in Q1, and
  # undercounts it in Q2.
  btrfs subvolume snapshot -i 1/100 "$mnt/intermediate" "$mnt/snap" >/dev/null
  snap_id=$(btrfs subvolume show "$mnt/snap" | grep 'Subvolume ID:' | awk '{print $3}')

  # Fully complete snapshot creation
  sync

  # Delete working snapshot
  # Q1 and Q2 will lose the full snap usage
  btrfs subvolume delete "$mnt/snap" >/dev/null

  # Delete intermediate and remove from Q1
  # Q1 and Q2 will lose the full intermediate usage
  btrfs qgroup remove "0/$inter_id" 1/100 "$mnt"
  btrfs subvolume delete "$mnt/intermediate" >/dev/null

  # Q1 should be at 0, but still has 16k. Q2 is "correct" at 0 (for now...)

  # Trigger cleaner, wait for deletions
  mount -o remount,sync=1 "$mnt"
  btrfs subvolume sync "$mnt" "$snap_id"
  btrfs subvolume sync "$mnt" "$inter_id"

  # Remove Q1 from Q2
  # Frees 16k more from Q2, underflowing it to 16EiB
  btrfs qgroup remove 1/100 2/100 "$mnt"

  # And show the bad state:
  btrfs qgroup show -pc "$mnt"

        Qgroupid    Referenced    Exclusive Parent   Child   Path
        --------    ----------    --------- ------   -----   ----
        0/5           16.00KiB     16.00KiB -        -       <toplevel>
        0/256         16.00KiB     16.00KiB -        -       base
        1/100         16.00KiB     16.00KiB -        -       <0 member qgroups>
        2/100         16.00EiB     16.00EiB -        -       <0 member qgroups>

Fix this by simply not doing this quick inheritance with squotas.

I suspect that it is also wrong in normal qgroups to not recurse up the
qgroup tree in the quick inherit case, though other consistency checks
will likely fix it anyway.

Fixes: b20fe56cd285 ("btrfs: qgroup: allow quick inherit if snapshot is created and added to the same parent")
Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/qgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9e2b53e90dcb..d46f2653510d 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3223,6 +3223,9 @@ static int qgroup_snapshot_quick_inherit(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup_list *list;
 	int nr_parents = 0;
 
+	if (btrfs_qgroup_mode(fs_info) != BTRFS_QGROUP_MODE_FULL)
+		return 0;
+
 	src = find_qgroup_rb(fs_info, srcid);
 	if (!src)
 		return -ENOENT;
-- 
2.52.0

[PATCH 2/3] btrfs: check squota parent usage on membership change

[Boris Burkov]

We could have detected the quick inherit bug more directly if we had
an extra warning about squota hierarchy consistency while modifying the
hierarchy. In squotas, the parent usage always simply adds up to the sum of
its children, so we can just check for that when changing membership and
detect more accounting bugs.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/qgroup.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index d46f2653510d..9e7e0c2e98ac 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -346,6 +346,42 @@ int btrfs_verify_qgroup_counts(const struct btrfs_fs_info *fs_info, u64 qgroupid
 }
 #endif
 
+static bool squota_check_parent_usage(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *parent)
+{
+	u64 excl_sum = 0;
+	u64 rfer_sum = 0;
+	u64 excl_cmpr_sum = 0;
+	u64 rfer_cmpr_sum = 0;
+	struct btrfs_qgroup_list *glist;
+	int nr_members = 0;
+	bool mismatch;
+
+	if (btrfs_qgroup_mode(fs_info) != BTRFS_QGROUP_MODE_SIMPLE)
+		return false;
+	if (btrfs_qgroup_level(parent->qgroupid) == 0)
+		return false;
+
+	/* Eligible parent qgroup. Squota; level > 0; empty members list. */
+	list_for_each_entry(glist, &parent->members, next_member) {
+		excl_sum += glist->member->excl;
+		rfer_sum += glist->member->rfer;
+		excl_cmpr_sum += glist->member->excl_cmpr;
+		rfer_cmpr_sum += glist->member->rfer_cmpr;
+		nr_members++;
+	}
+	mismatch = (parent->excl != excl_sum || parent->rfer != rfer_sum ||
+		    parent->excl_cmpr != excl_cmpr_sum || parent->rfer_cmpr != excl_cmpr_sum);
+
+	WARN(mismatch,
+	     "parent squota qgroup %hu/%llu has mismatched usage from its %d members. "
+	     "%llu %llu %llu %llu vs %llu %llu %llu %llu\n",
+	     btrfs_qgroup_level(parent->qgroupid),
+	     btrfs_qgroup_subvolid(parent->qgroupid), nr_members, parent->excl,
+	     parent->rfer, parent->excl_cmpr, parent->rfer_cmpr, excl_sum,
+	     rfer_sum, excl_cmpr_sum, rfer_cmpr_sum);
+	return mismatch;
+}
+
 __printf(2, 3)
 static void qgroup_mark_inconsistent(struct btrfs_fs_info *fs_info, const char *fmt, ...)
 {
@@ -1569,6 +1605,7 @@ int btrfs_add_qgroup_relation(struct btrfs_trans_handle *trans, u64 src, u64 dst
 		goto out;
 	}
 	ret = quick_update_accounting(fs_info, src, dst, 1);
+	squota_check_parent_usage(fs_info, parent);
 	spin_unlock(&fs_info->qgroup_lock);
 out:
 	kfree(prealloc);
@@ -1625,6 +1662,8 @@ static int __del_qgroup_relation(struct btrfs_trans_handle *trans, u64 src,
 		spin_lock(&fs_info->qgroup_lock);
 		del_relation_rb(fs_info, src, dst);
 		ret = quick_update_accounting(fs_info, src, dst, -1);
+		ASSERT(parent);
+		squota_check_parent_usage(fs_info, parent);
 		spin_unlock(&fs_info->qgroup_lock);
 	}
 out:
-- 
2.52.0

[PATCH 3/3] btrfs: relax squota parent qgroup deletion rule

[Boris Burkov]

Currently, with squotas, we do not allow removing a parent qgroup with
no members if it still has usage accounted to it. This makes it really
difficult to recover from accounting bugs, as we have no good way of
getting back to 0 usage.

Instead, allow deletion (it's safe at 0 members..) while still warning
about the inconsistency by adding a squota parent check.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/qgroup.c | 51 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 9e7e0c2e98ac..731ab71ff8ef 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1458,6 +1458,7 @@ static void qgroup_iterator_clean(struct list_head *head)
 	}
 }
 
+
 /*
  * The easy accounting, we're updating qgroup relationship whose child qgroup
  * only has exclusive extents.
@@ -1730,6 +1731,36 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
 	return ret;
 }
 
+static bool can_delete_parent_qgroup(struct btrfs_qgroup *qgroup)
+
+{
+	ASSERT(btrfs_qgroup_level(qgroup->qgroupid));
+	return list_empty(&qgroup->members);
+}
+
+/*
+ * Return true if we can delete the squota qgroup and false otherwise.
+ *
+ * Rules for whether we can delete:
+ *
+ * A subvolume qgroup can be removed iff the subvolume is fully deleted, which
+ * is iff there is 0 usage in the qgroup.
+ *
+ * A higher level qgroup can be removed iff it has no members.
+ * Note: We audit its usage to warn on inconsitencies without blocking deletion.
+ */
+static bool can_delete_squota_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *qgroup)
+{
+	ASSERT(btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE);
+
+	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
+		squota_check_parent_usage(fs_info, qgroup);
+		return can_delete_parent_qgroup(qgroup);
+	}
+
+	return !(qgroup->rfer || qgroup->excl || qgroup->rfer_cmpr || qgroup->excl_cmpr);
+}
+
 /*
  * Return 0 if we can not delete the qgroup (not empty or has children etc).
  * Return >0 if we can delete the qgroup.
@@ -1740,23 +1771,13 @@ static int can_delete_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup
 	struct btrfs_key key;
 	BTRFS_PATH_AUTO_FREE(path);
 
-	/*
-	 * Squota would never be inconsistent, but there can still be case
-	 * where a dropped subvolume still has qgroup numbers, and squota
-	 * relies on such qgroup for future accounting.
-	 *
-	 * So for squota, do not allow dropping any non-zero qgroup.
-	 */
-	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE &&
-	    (qgroup->rfer || qgroup->excl || qgroup->excl_cmpr || qgroup->rfer_cmpr))
-		return 0;
+	/* Since squotas cannot be inconsistent, they have special rules for deletion. */
+	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
+		return can_delete_squota_qgroup(fs_info, qgroup);
 
 	/* For higher level qgroup, we can only delete it if it has no child. */
-	if (btrfs_qgroup_level(qgroup->qgroupid)) {
-		if (!list_empty(&qgroup->members))
-			return 0;
-		return 1;
-	}
+	if (btrfs_qgroup_level(qgroup->qgroupid))
+		return can_delete_parent_qgroup(qgroup);
 
 	/*
 	 * For level-0 qgroups, we can only delete it if it has no subvolume
-- 
2.52.0

Re: [PATCH 3/3] btrfs: relax squota parent qgroup deletion rule

[Qu Wenruo]

在 2025/12/4 07:41, Boris Burkov 写道:
> Currently, with squotas, we do not allow removing a parent qgroup with
> no members if it still has usage accounted to it. This makes it really
> difficult to recover from accounting bugs, as we have no good way of
> getting back to 0 usage.
> 
> Instead, allow deletion (it's safe at 0 members..) while still warning
> about the inconsistency by adding a squota parent check.
> 
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/qgroup.c | 51 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9e7e0c2e98ac..731ab71ff8ef 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1458,6 +1458,7 @@ static void qgroup_iterator_clean(struct list_head *head)
>   	}
>   }
>   
> +

A stray new line.

Otherwise looks good to me. I'll reply on the cover letter.

Thanks,
Qu

>   /*
>    * The easy accounting, we're updating qgroup relationship whose child qgroup
>    * only has exclusive extents.
> @@ -1730,6 +1731,36 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
>   	return ret;
>   }
>   
> +static bool can_delete_parent_qgroup(struct btrfs_qgroup *qgroup)
> +
> +{
> +	ASSERT(btrfs_qgroup_level(qgroup->qgroupid));
> +	return list_empty(&qgroup->members);
> +}
> +
> +/*
> + * Return true if we can delete the squota qgroup and false otherwise.
> + *
> + * Rules for whether we can delete:
> + *
> + * A subvolume qgroup can be removed iff the subvolume is fully deleted, which
> + * is iff there is 0 usage in the qgroup.
> + *
> + * A higher level qgroup can be removed iff it has no members.
> + * Note: We audit its usage to warn on inconsitencies without blocking deletion.
> + */
> +static bool can_delete_squota_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *qgroup)
> +{
> +	ASSERT(btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE);
> +
> +	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
> +		squota_check_parent_usage(fs_info, qgroup);
> +		return can_delete_parent_qgroup(qgroup);
> +	}
> +
> +	return !(qgroup->rfer || qgroup->excl || qgroup->rfer_cmpr || qgroup->excl_cmpr);
> +}
> +
>   /*
>    * Return 0 if we can not delete the qgroup (not empty or has children etc).
>    * Return >0 if we can delete the qgroup.
> @@ -1740,23 +1771,13 @@ static int can_delete_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup
>   	struct btrfs_key key;
>   	BTRFS_PATH_AUTO_FREE(path);
>   
> -	/*
> -	 * Squota would never be inconsistent, but there can still be case
> -	 * where a dropped subvolume still has qgroup numbers, and squota
> -	 * relies on such qgroup for future accounting.
> -	 *
> -	 * So for squota, do not allow dropping any non-zero qgroup.
> -	 */
> -	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE &&
> -	    (qgroup->rfer || qgroup->excl || qgroup->excl_cmpr || qgroup->rfer_cmpr))
> -		return 0;
> +	/* Since squotas cannot be inconsistent, they have special rules for deletion. */
> +	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
> +		return can_delete_squota_qgroup(fs_info, qgroup);
>   
>   	/* For higher level qgroup, we can only delete it if it has no child. */
> -	if (btrfs_qgroup_level(qgroup->qgroupid)) {
> -		if (!list_empty(&qgroup->members))
> -			return 0;
> -		return 1;
> -	}
> +	if (btrfs_qgroup_level(qgroup->qgroupid))
> +		return can_delete_parent_qgroup(qgroup);
>   
>   	/*
>   	 * For level-0 qgroups, we can only delete it if it has no subvolume

Re: [PATCH 1/3] btrfs: fix qgroup_snapshot_quick_inherit() squota bug

[Qu Wenruo]

在 2025/12/4 07:41, Boris Burkov 写道:
> qgroup_snapshot_quick_inherit() detects conditions where the snapshot
> destination would land in the same parent qgroup as the snapshot source
> subvolume. In this case we can avoid costly qgroup calculations and just
> add the nodesize of the new snapshot to the parent.
> 
> However, in the case of squotas this is actually a double count, and
> also an undercount for deeper qgroup nestings.
> 
> The following annotated script shows the issue:
> 
>    btrfs quota enable --simple "$mnt"
> 
>    # Create 2-level qgroup hierarchy
>    btrfs qgroup create 2/100 "$mnt"  # Q2 (level 2)
>    btrfs qgroup create 1/100 "$mnt"  # Q1 (level 1)
>    btrfs qgroup assign 1/100 2/100 "$mnt"
> 
>    # Create base subvolume
>    btrfs subvolume create "$mnt/base" >/dev/null
>    base_id=$(btrfs subvolume show "$mnt/base" | grep 'Subvolume ID:' | awk '{print $3}')
> 
>    # Create intermediate snapshot and add to Q1
>    btrfs subvolume snapshot "$mnt/base" "$mnt/intermediate" >/dev/null
>    inter_id=$(btrfs subvolume show "$mnt/intermediate" | grep 'Subvolume ID:' | awk '{print $3}')
>    btrfs qgroup assign "0/$inter_id" 1/100 "$mnt"
> 
>    # Create working snapshot with --inherit (auto-adds to Q1)
>    # src=intermediate (in only Q1)
>    # dst=snap (inheriting only into Q1)
>    # This double counts the 16k nodesize of the snapshot in Q1, and
>    # undercounts it in Q2.
>    btrfs subvolume snapshot -i 1/100 "$mnt/intermediate" "$mnt/snap" >/dev/null
>    snap_id=$(btrfs subvolume show "$mnt/snap" | grep 'Subvolume ID:' | awk '{print $3}')
> 
>    # Fully complete snapshot creation
>    sync
> 
>    # Delete working snapshot
>    # Q1 and Q2 will lose the full snap usage
>    btrfs subvolume delete "$mnt/snap" >/dev/null
> 
>    # Delete intermediate and remove from Q1
>    # Q1 and Q2 will lose the full intermediate usage
>    btrfs qgroup remove "0/$inter_id" 1/100 "$mnt"
>    btrfs subvolume delete "$mnt/intermediate" >/dev/null
> 
>    # Q1 should be at 0, but still has 16k. Q2 is "correct" at 0 (for now...)
> 
>    # Trigger cleaner, wait for deletions
>    mount -o remount,sync=1 "$mnt"
>    btrfs subvolume sync "$mnt" "$snap_id"
>    btrfs subvolume sync "$mnt" "$inter_id"
> 
>    # Remove Q1 from Q2
>    # Frees 16k more from Q2, underflowing it to 16EiB
>    btrfs qgroup remove 1/100 2/100 "$mnt"
> 
>    # And show the bad state:
>    btrfs qgroup show -pc "$mnt"
> 
>          Qgroupid    Referenced    Exclusive Parent   Child   Path
>          --------    ----------    --------- ------   -----   ----
>          0/5           16.00KiB     16.00KiB -        -       <toplevel>
>          0/256         16.00KiB     16.00KiB -        -       base
>          1/100         16.00KiB     16.00KiB -        -       <0 member qgroups>
>          2/100         16.00EiB     16.00EiB -        -       <0 member qgroups>
> 
> Fix this by simply not doing this quick inheritance with squotas.
> 
> I suspect that it is also wrong in normal qgroups to not recurse up the
> qgroup tree in the quick inherit case, though other consistency checks
> will likely fix it anyway.

It's indeed wrong for the regular qgroup:

  mkfs.btrfs  -f -O quota $dev
  mount $dev $mnt
  btrfs subv create $mnt/subv1
  btrfs qgroup create 1/100 $mnt
  btrfs qgroup create 2/100 $mnt
  btrfs qgroup assign 1/100 2/100 $mnt
  btrfs qgroup assign 0/256 1/100 $mnt
  btrfs qgroup show -p --sync $mnt
  Qgroupid    Referenced    Exclusive Parent     Path
  --------    ----------    --------- ------     ----
  0/5           16.00KiB     16.00KiB -          <toplevel>
  0/256         16.00KiB     16.00KiB 1/100      subv1
  1/100         16.00KiB     16.00KiB 2/100      2/100<1 member qgroup>
  2/100         16.00KiB     16.00KiB -          <0 member qgroups>
  # So far so good

  btrfs subv snap -i 1/100 $mnt/subv1 $mnt/snap1
  btrfs qgroup show -p --sync $mnt
  Qgroupid    Referenced    Exclusive Parent     Path
  --------    ----------    --------- ------     ----
  0/5           16.00KiB     16.00KiB -          <toplevel>
  0/256         16.00KiB     16.00KiB 1/100      subv1
  0/257         16.00KiB     16.00KiB 1/100      snap1
  1/100         32.00KiB     32.00KiB 2/100      2/100<1 member qgroup>
  2/100         16.00KiB     16.00KiB -          <0 member qgroups>
  # Just as you suspected, higher qgroup didn't get updated.

  btrfs subv snap -i 1/100 /mnt/btrfs/subv1 /mnt/btrfs/subv2
  btrfs qgr show -prce /mnt/btrfs/

I'll fix the regular qgroup bug soon.

Thanks,
Qu

> 
> Fixes: b20fe56cd285 ("btrfs: qgroup: allow quick inherit if snapshot is created and added to the same parent")
> Signed-off-by: Boris Burkov <boris@bur.io>
> ---
>   fs/btrfs/qgroup.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 9e2b53e90dcb..d46f2653510d 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3223,6 +3223,9 @@ static int qgroup_snapshot_quick_inherit(struct btrfs_fs_info *fs_info,
>   	struct btrfs_qgroup_list *list;
>   	int nr_parents = 0;
>   
> +	if (btrfs_qgroup_mode(fs_info) != BTRFS_QGROUP_MODE_FULL)
> +		return 0;
> +
>   	src = find_qgroup_rb(fs_info, srcid);
>   	if (!src)
>   		return -ENOENT;

Re: [PATCH 0/3] btrfs: squota inheritance improvements

[Qu Wenruo]

在 2025/12/4 07:41, Boris Burkov 写道:
> One bug fix for an off-by-one metadata accounting error and two generic
> improvements. Details in the patches.

Reviewed-by: Qu Wenruo <wqu@suse.com>

> 
> Boris Burkov (3):
>    btrfs: fix qgroup_snapshot_quick_inherit() squota bug

Thanks for the bug report of regular qgroup.

>    btrfs: check squota parent usage on membership change
>    btrfs: relax squota parent qgroup deletion rule

And don't forget to remove the stray new line at merge time.

Thanks,
Qu

> 
>   fs/btrfs/qgroup.c | 93 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 78 insertions(+), 15 deletions(-)
> 

Re: [PATCH 3/3] btrfs: relax squota parent qgroup deletion rule

[Boris Burkov]

On Thu, Dec 04, 2025 at 01:49:53PM +1030, Qu Wenruo wrote:
> 
> 
> 在 2025/12/4 07:41, Boris Burkov 写道:
> > Currently, with squotas, we do not allow removing a parent qgroup with
> > no members if it still has usage accounted to it. This makes it really
> > difficult to recover from accounting bugs, as we have no good way of
> > getting back to 0 usage.
> > 
> > Instead, allow deletion (it's safe at 0 members..) while still warning
> > about the inconsistency by adding a squota parent check.
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> > ---
> >   fs/btrfs/qgroup.c | 51 +++++++++++++++++++++++++++++++++--------------
> >   1 file changed, 36 insertions(+), 15 deletions(-)
> > 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index 9e7e0c2e98ac..731ab71ff8ef 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -1458,6 +1458,7 @@ static void qgroup_iterator_clean(struct list_head *head)
> >   	}
> >   }
> > +
> 
> A stray new line.
> 
> Otherwise looks good to me. I'll reply on the cover letter.
> 
> Thanks,
> Qu
> 

I didn't quite get to sending the new fstest before leaving on a
vacation before LPC, but I intend to do it next week. So please don't
feel the burden to also prepare an fstest of the new reproducer.

Thanks for the review,
Boris

> >   /*
> >    * The easy accounting, we're updating qgroup relationship whose child qgroup
> >    * only has exclusive extents.
> > @@ -1730,6 +1731,36 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
> >   	return ret;
> >   }
> > +static bool can_delete_parent_qgroup(struct btrfs_qgroup *qgroup)
> > +
> > +{
> > +	ASSERT(btrfs_qgroup_level(qgroup->qgroupid));
> > +	return list_empty(&qgroup->members);
> > +}
> > +
> > +/*
> > + * Return true if we can delete the squota qgroup and false otherwise.
> > + *
> > + * Rules for whether we can delete:
> > + *
> > + * A subvolume qgroup can be removed iff the subvolume is fully deleted, which
> > + * is iff there is 0 usage in the qgroup.
> > + *
> > + * A higher level qgroup can be removed iff it has no members.
> > + * Note: We audit its usage to warn on inconsitencies without blocking deletion.
> > + */
> > +static bool can_delete_squota_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *qgroup)
> > +{
> > +	ASSERT(btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE);
> > +
> > +	if (btrfs_qgroup_level(qgroup->qgroupid) > 0) {
> > +		squota_check_parent_usage(fs_info, qgroup);
> > +		return can_delete_parent_qgroup(qgroup);
> > +	}
> > +
> > +	return !(qgroup->rfer || qgroup->excl || qgroup->rfer_cmpr || qgroup->excl_cmpr);
> > +}
> > +
> >   /*
> >    * Return 0 if we can not delete the qgroup (not empty or has children etc).
> >    * Return >0 if we can delete the qgroup.
> > @@ -1740,23 +1771,13 @@ static int can_delete_qgroup(struct btrfs_fs_info *fs_info, struct btrfs_qgroup
> >   	struct btrfs_key key;
> >   	BTRFS_PATH_AUTO_FREE(path);
> > -	/*
> > -	 * Squota would never be inconsistent, but there can still be case
> > -	 * where a dropped subvolume still has qgroup numbers, and squota
> > -	 * relies on such qgroup for future accounting.
> > -	 *
> > -	 * So for squota, do not allow dropping any non-zero qgroup.
> > -	 */
> > -	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE &&
> > -	    (qgroup->rfer || qgroup->excl || qgroup->excl_cmpr || qgroup->rfer_cmpr))
> > -		return 0;
> > +	/* Since squotas cannot be inconsistent, they have special rules for deletion. */
> > +	if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_SIMPLE)
> > +		return can_delete_squota_qgroup(fs_info, qgroup);
> >   	/* For higher level qgroup, we can only delete it if it has no child. */
> > -	if (btrfs_qgroup_level(qgroup->qgroupid)) {
> > -		if (!list_empty(&qgroup->members))
> > -			return 0;
> > -		return 1;
> > -	}
> > +	if (btrfs_qgroup_level(qgroup->qgroupid))
> > +		return can_delete_parent_qgroup(qgroup);
> >   	/*
> >   	 * For level-0 qgroups, we can only delete it if it has no subvolume
> 

Re: [PATCH 2/3] btrfs: check squota parent usage on membership change

[Chris Mason]

Boris Burkov <boris@bur.io> wrote:
> We could have detected the quick inherit bug more directly if we had
> an extra warning about squota hierarchy consistency while modifying the
> hierarchy. In squotas, the parent usage always simply adds up to the sum of
> its children, so we can just check for that when changing membership and
> detect more accounting bugs.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit 9c46bcda5f347febdbb4d117fb21a37ffcec5fa4
Author: Boris Burkov <boris@bur.io>

btrfs: check squota parent usage on membership change

This commit adds a diagnostic check to detect squota hierarchy
consistency issues when modifying qgroup membership. The check verifies
that parent usage equals the sum of its children's usage.

> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 206587820fec0..3a74759b59ee4 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -346,6 +346,42 @@ int btrfs_verify_qgroup_counts(const struct btrfs_fs_info *fs_info, u64 qgroupid
>  }
>  #endif
>
> +static bool squota_check_parent_usage(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *parent)
> +{
> +	u64 excl_sum = 0;
> +	u64 rfer_sum = 0;
> +	u64 excl_cmpr_sum = 0;
> +	u64 rfer_cmpr_sum = 0;
> +	struct btrfs_qgroup_list *glist;
> +	int nr_members = 0;
> +	bool mismatch;
> +
> +	if (btrfs_qgroup_mode(fs_info) != BTRFS_QGROUP_MODE_SIMPLE)
> +		return false;
> +	if (btrfs_qgroup_level(parent->qgroupid) == 0)
> +		return false;
> +
> +	/* Eligible parent qgroup. Squota; level > 0; empty members list. */
> +	list_for_each_entry(glist, &parent->members, next_member) {
> +		excl_sum += glist->member->excl;
> +		rfer_sum += glist->member->rfer;
> +		excl_cmpr_sum += glist->member->excl_cmpr;
> +		rfer_cmpr_sum += glist->member->rfer_cmpr;
> +		nr_members++;
> +	}
> +	mismatch = (parent->excl != excl_sum || parent->rfer != rfer_sum ||
> +		    parent->excl_cmpr != excl_cmpr_sum || parent->rfer_cmpr != excl_cmpr_sum);
                                                                            ^^^^^^^^^^^^^^^^

Is parent->rfer_cmpr being compared to the wrong variable? The code
compares parent->rfer_cmpr to excl_cmpr_sum, but rfer_cmpr_sum was
calculated above and never used. Should this be comparing
parent->rfer_cmpr != rfer_cmpr_sum instead?

> +
> +	WARN(mismatch,
> +	     "parent squota qgroup %hu/%llu has mismatched usage from its %d members. "
> +	     "%llu %llu %llu %llu vs %llu %llu %llu %llu\n",
> +	     btrfs_qgroup_level(parent->qgroupid),
> +	     btrfs_qgroup_subvolid(parent->qgroupid), nr_members, parent->excl,
> +	     parent->rfer, parent->excl_cmpr, parent->rfer_cmpr, excl_sum,
> +	     rfer_sum, excl_cmpr_sum, rfer_cmpr_sum);
> +	return mismatch;
> +}

[ ... ]

Re: [PATCH 2/3] btrfs: check squota parent usage on membership change

[Boris Burkov]

On Sun, Feb 08, 2026 at 10:59:01AM -0800, Chris Mason wrote:
> Boris Burkov <boris@bur.io> wrote:
> > We could have detected the quick inherit bug more directly if we had
> > an extra warning about squota hierarchy consistency while modifying the
> > hierarchy. In squotas, the parent usage always simply adds up to the sum of
> > its children, so we can just check for that when changing membership and
> > detect more accounting bugs.
> 
> Hi everyone,
> 
> I'm running the btrfs for-next branch, along with the encryption patch set
> through my AI review prompts:
> 
> https://github.com/masoncl/review-prompts
> 
> I'm working on some btrfs specific instructions, but hopefully these reviews
> are already useful.  I've kept the headers to make it easier to double check
> that I'm actually replying to the right email, but please let me know if any
> parts of this are incorrect:
> 
> commit 9c46bcda5f347febdbb4d117fb21a37ffcec5fa4
> Author: Boris Burkov <boris@bur.io>
> 
> btrfs: check squota parent usage on membership change
> 
> This commit adds a diagnostic check to detect squota hierarchy
> consistency issues when modifying qgroup membership. The check verifies
> that parent usage equals the sum of its children's usage.
> 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index 206587820fec0..3a74759b59ee4 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -346,6 +346,42 @@ int btrfs_verify_qgroup_counts(const struct btrfs_fs_info *fs_info, u64 qgroupid
> >  }
> >  #endif
> >
> > +static bool squota_check_parent_usage(struct btrfs_fs_info *fs_info, struct btrfs_qgroup *parent)
> > +{
> > +	u64 excl_sum = 0;
> > +	u64 rfer_sum = 0;
> > +	u64 excl_cmpr_sum = 0;
> > +	u64 rfer_cmpr_sum = 0;
> > +	struct btrfs_qgroup_list *glist;
> > +	int nr_members = 0;
> > +	bool mismatch;
> > +
> > +	if (btrfs_qgroup_mode(fs_info) != BTRFS_QGROUP_MODE_SIMPLE)
> > +		return false;
> > +	if (btrfs_qgroup_level(parent->qgroupid) == 0)
> > +		return false;
> > +
> > +	/* Eligible parent qgroup. Squota; level > 0; empty members list. */
> > +	list_for_each_entry(glist, &parent->members, next_member) {
> > +		excl_sum += glist->member->excl;
> > +		rfer_sum += glist->member->rfer;
> > +		excl_cmpr_sum += glist->member->excl_cmpr;
> > +		rfer_cmpr_sum += glist->member->rfer_cmpr;
> > +		nr_members++;
> > +	}
> > +	mismatch = (parent->excl != excl_sum || parent->rfer != rfer_sum ||
> > +		    parent->excl_cmpr != excl_cmpr_sum || parent->rfer_cmpr != excl_cmpr_sum);
>                                                                             ^^^^^^^^^^^^^^^^
> 
> Is parent->rfer_cmpr being compared to the wrong variable? The code
> compares parent->rfer_cmpr to excl_cmpr_sum, but rfer_cmpr_sum was
> calculated above and never used. Should this be comparing
> parent->rfer_cmpr != rfer_cmpr_sum instead?
> 

I believe this is correct, but "irrelevant" because rfer == excl in
squota. (and rfer_cmpr == excl_cmpr == 0)

But might as well clean it up, as it is needlessly messy. I'll send the patch.

Thanks,
Boris

> > +
> > +	WARN(mismatch,
> > +	     "parent squota qgroup %hu/%llu has mismatched usage from its %d members. "
> > +	     "%llu %llu %llu %llu vs %llu %llu %llu %llu\n",
> > +	     btrfs_qgroup_level(parent->qgroupid),
> > +	     btrfs_qgroup_subvolid(parent->qgroupid), nr_members, parent->excl,
> > +	     parent->rfer, parent->excl_cmpr, parent->rfer_cmpr, excl_sum,
> > +	     rfer_sum, excl_cmpr_sum, rfer_cmpr_sum);
> > +	return mismatch;
> > +}
> 
> [ ... ]
> 
>