[PATCH 0/4] btrfs-progs: cmds/qgroup: enhance stale qgroups handling

[PATCH 0/4] btrfs-progs: cmds/qgroup: enhance stale qgroups handling

[Qu Wenruo]

Currently the `<stale>` label is give incorrectly for a lot of qgroups
that can not and should not be deleted.

The patchset would address the problem for `btrfs qgroup show` and
`btrfs qgroup clear-stale` commands, so that:

- `clear-stale` would not try to delete incorrect qgroups
  Thus it should not return false errors.

- `show` would show extra types of qgroups
  Including `<under deletion>` and `<squota space holder>`

Qu Wenruo (4):
  btrfs-progs: cmds/qgroup: sync the fs before doing qgroup search
  btrfs-progs: cmds/qgroup: add qgroup_lookup::flags member
  btrfs-progs: cmds/qgroup: handle stale qgroup deleteion more
    accurately
  btrfs-progs: cmds/qgroup: add more special status for qgroups

 Documentation/btrfs-qgroup.rst |  24 +++++
 cmds/qgroup.c                  | 160 ++++++++++++++++++++++++++++-----
 2 files changed, 162 insertions(+), 22 deletions(-)

--
2.45.0

[PATCH 1/4] btrfs-progs: cmds/qgroup: sync the fs before doing qgroup search

[Qu Wenruo]

Since qgroup numbers are only updated at transaction commit time, it's
better to do the a sync before reading the quota tree, to reduce the
chance of uncommitted qgroup changes.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/qgroup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 6e88db5803a5..09eac0d2b36e 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -2135,6 +2135,7 @@ static const char * const cmd_qgroup_clear_stale_usage[] = {
 
 static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char **argv)
 {
+	enum btrfs_util_error err;
 	int ret = 0;
 	int fd;
 	char *path = NULL;
@@ -2151,6 +2152,11 @@ static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char *
 	if (fd < 0)
 		return 1;
 
+	/* Sync the fs so that the qgroup numbers are uptodate. */
+	err = btrfs_util_sync_fd(fd);
+	if (err)
+		warning("sync ioctl failed on '%s': %m", path);
+
 	ret = qgroups_search_all(fd, &qgroup_lookup);
 	if (ret == -ENOTTY) {
 		error("can't list qgroups: quotas not enabled");
-- 
2.45.0

[PATCH 2/4] btrfs-progs: cmds/qgroup: add qgroup_lookup::flags member

[Qu Wenruo]

This allows the users to identify if the running qgroup mode and whether
the numebrs are already inconsistent.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/qgroup.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 09eac0d2b36e..a023f0948180 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -65,6 +65,8 @@ struct btrfs_qgroup_list {
 };
 
 struct qgroup_lookup {
+	/* This matches btrfs_qgroup_status_item::flags. */
+	u64 flags;
 	struct rb_root root;
 };
 
@@ -1313,6 +1315,7 @@ static int __qgroups_search(int fd, struct btrfs_tree_search_args *args,
 			case BTRFS_QGROUP_STATUS_KEY:
 				si = btrfs_tree_search_data(args, off);
 				flags = btrfs_stack_qgroup_status_flags(si);
+				qgroup_lookup->flags = flags;
 
 				print_status_flag_warning(flags);
 				break;
-- 
2.45.0

[PATCH 3/4] btrfs-progs: cmds/qgroup: handle stale qgroup deleteion more accurately

[Qu Wenruo]

The current stale qgroup delete is a mess, it doesn't handle the
following cases at all:

- It doesn't detect stale qgroups correctly
  The current check is using the root backref, which means unlinked but
  not yet fully dropped subvolumes would mark its corresponding qgroups
  stale.

  This is incorrect. The real stale check should be based on the root
  item, not root backref.

- Squota non-empty but stale qgroups
  Such qgroups can not and should not be deleted, as future accounting
  still require them.

- Full accounting mode, stale qgroups but not empty
  Since qgroup numbers are inconsistent already, it's common to have
  such stale qgroups with non-zero numbers.

  Now it's dependent on the kernel to determine whether such qgroup can
  be deleted.

The patch would address the above problems by:

- Do root_item based detection
  So that btrfs_qgroup::stale would properly indicate if there is a
  subvolume root item for the qgroup.

- Do not attempt to delete squota stale but non-empty qgroups

- Attempt to delete stale but non-empty qgroups for full accounting mode
  And deletion failure would not count as an error.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/qgroup.c | 119 +++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 103 insertions(+), 16 deletions(-)

diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index a023f0948180..70a306117ebd 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -80,9 +80,24 @@ struct btrfs_qgroup {
 	struct rb_node all_parent_node;
 	u64 qgroupid;
 
-	/* NULL for qgroups with level > 0 */
+	/*
+	 * NULL for qgroups with level > 0 or the subvolume is *unlinked*.
+	 *
+	 * Note that, an unlinked subvolume doesn't mean it has been fully
+	 * dropped, so callers should not rely on this to determine if
+	 * a qgroup is stale.
+	 *
+	 * This member is only to help locating the path of the corresponding
+	 * subvolume.
+	 */
 	const char *path;
 
+	/*
+	 * This is only true if the qgroup is level 0 qgroup, and there is
+	 * no subvolume tree for the qgroup at all.
+	 */
+	bool stale;
+
 	/*
 	 * info_item
 	 */
@@ -229,6 +244,12 @@ static struct {
 static btrfs_qgroup_filter_func all_filter_funcs[];
 static btrfs_qgroup_comp_func all_comp_funcs[];
 
+static bool is_qgroup_empty(struct btrfs_qgroup *qg)
+{
+	return !(qg->info.referenced || qg->info.exclusive ||
+		 qg->info.referenced_compressed ||
+		 qg->info.exclusive_compressed);
+}
 static void qgroup_setup_print_column(enum btrfs_qgroup_column_enum column)
 {
 	int i;
@@ -795,7 +816,6 @@ static struct btrfs_qgroup *get_or_add_qgroup(int fd,
 		uret = btrfs_util_subvolume_path_fd(fd, qgroupid, &path);
 		if (uret == BTRFS_UTIL_OK)
 			bq->path = path;
-		/* Ignore stale qgroup items */
 		else if (uret != BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND) {
 			error("%s", btrfs_util_strerror(uret));
 			if (uret == BTRFS_UTIL_ERROR_NO_MEMORY)
@@ -803,6 +823,24 @@ static struct btrfs_qgroup *get_or_add_qgroup(int fd,
 			else
 				return ERR_PTR(-EIO);
 		}
+		/*
+		 * Do a correct stale detection by searching for the ROOT_ITEM of
+		 * the subvolume.
+		 *
+		 * Passing @subvol as NULL will force the search to only search
+		 * for the ROOT_ITEM.
+		 */
+		uret = btrfs_util_subvolume_info_fd(fd, qgroupid, NULL);
+		if (uret == BTRFS_UTIL_OK) {
+			bq->stale = false;
+		} else if (uret == BTRFS_UTIL_ERROR_SUBVOLUME_NOT_FOUND) {
+			bq->stale = true;
+		} else {
+			warning("failed to search root item for qgroup %u/%llu, assuming it not stale",
+				btrfs_qgroup_level(qgroupid),
+				btrfs_qgroup_subvolid(qgroupid));
+			bq->stale = false;
+		}
 	}
 
 	ret = qgroup_tree_insert(qgroup_lookup, bq);
@@ -2136,6 +2174,65 @@ static const char * const cmd_qgroup_clear_stale_usage[] = {
 	NULL
 };
 
+/*
+ * Return >0 if the qgroup should or can not be deleted.
+ * Return 0 if the qgroup is deleted.
+ * Return <0 for critical error.
+ */
+static int delete_one_stale_qgroup(struct qgroup_lookup *lookup,
+				   struct btrfs_qgroup *qg, int fd)
+{
+	u16 level = btrfs_qgroup_level(qg->qgroupid);
+	struct btrfs_ioctl_qgroup_create_args args = { .create = false };
+	const bool inconsistent = lookup->flags & BTRFS_QGROUP_STATUS_FLAG_INCONSISTENT;
+	const bool squota = lookup->flags & BTRFS_QGROUP_STATUS_FLAG_SIMPLE_MODE;
+	const bool empty = is_qgroup_empty(qg);
+	bool attempt = false;
+	int ret;
+
+	if (level || !qg->stale)
+		return 1;
+
+	/*
+	 * By design, squota can have a subvolume fully dropped but its qgroup numbers
+	 * are not zero.
+	 * Such qgroup is still needed for future accounting, thus can not be deleted.
+	 */
+	if (squota && !empty)
+		return 1;
+
+	/*
+	 * We can have inconsistent qgroup numbers, in that case a really stale
+	 * qgroup can exist while its numbers are not zero.
+	 *
+	 * In this case we only attempt to delete the qgroup, depending on the
+	 * kernel implementation, we may or may not be able to delete it.
+	 */
+	if (inconsistent && !empty)
+		attempt = true;
+
+	if (attempt)
+		pr_verbose(LOG_DEFAULT,
+		"Attempt to delete stale but non-empty qgroup %u/%llu\n",
+			   level, btrfs_qgroup_subvolid(qg->qgroupid));
+	else
+		pr_verbose(LOG_DEFAULT, "Delete stale qgroup %u/%llu\n",
+			   level, btrfs_qgroup_subvolid(qg->qgroupid));
+	args.qgroupid = qg->qgroupid;
+	ret = ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args);
+	if (ret < 0) {
+		if (attempt) {
+			warning("not supported to delete non-empty stale qgroup %u/%llu",
+				level, btrfs_qgroup_subvolid(qg->qgroupid));
+			ret = 1;
+		} else {
+			error("failed to delete qgroup %u/%llu",
+				level, btrfs_qgroup_subvolid(qg->qgroupid));
+		}
+	}
+	return ret;
+}
+
 static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	enum btrfs_util_error err;
@@ -2172,22 +2269,12 @@ static int cmd_qgroup_clear_stale(const struct cmd_struct *cmd, int argc, char *
 
 	node = rb_first(&qgroup_lookup.root);
 	while (node) {
-		u64 level;
-		struct btrfs_ioctl_qgroup_create_args args = { .create = false };
+		int delete_ret;
 
 		entry = rb_entry(node, struct btrfs_qgroup, rb_node);
-		level = btrfs_qgroup_level(entry->qgroupid);
-		if (!entry->path && level == 0) {
-			pr_verbose(LOG_DEFAULT, "Delete stale qgroup %llu/%llu\n",
-					level, btrfs_qgroup_subvolid(entry->qgroupid));
-			args.qgroupid = entry->qgroupid;
-			ret = ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args);
-			if (ret < 0) {
-				error("cannot delete qgroup %llu/%llu: %m",
-					level,
-					btrfs_qgroup_subvolid(entry->qgroupid));
-			}
-		}
+		delete_ret = delete_one_stale_qgroup(&qgroup_lookup, entry, fd);
+		if (delete_ret < 0 && !ret)
+			ret = delete_ret;
 		node = rb_next(node);
 	}
 
-- 
2.45.0

[PATCH 4/4] btrfs-progs: cmds/qgroup: add more special status for qgroups

[Qu Wenruo]

Currently `btrfs qgroup show` command shows any 0 level qgroup withou a
root backref as `<stale>`, which is not correct.

There are several more cases:

- Under deletion
  The subvolume is not yet full dropped, but unlinked.
  In that case we would not have a root backref item, but the qgroup is
  not stale.

- Squota space holder
  This is for squota mode, that a fully dropped subvolume still have
  extents accounting on the already-gone subvolume.
  In this case it's not stale either, and future accounting relies on
  it.

This patch would add above special cases, and add an extra `SPECIAL
PATHS` section to explain all the cases, including `<stale>`.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-qgroup.rst | 24 ++++++++++++++++++++++++
 cmds/qgroup.c                  | 32 ++++++++++++++++++++++++++------
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/Documentation/btrfs-qgroup.rst b/Documentation/btrfs-qgroup.rst
index 343626767eb5..0e47c336619f 100644
--- a/Documentation/btrfs-qgroup.rst
+++ b/Documentation/btrfs-qgroup.rst
@@ -153,6 +153,30 @@ show [options] <path>
                 To retrieve information after updating the state of qgroups,
                 force sync of the filesystem identified by *path* before getting information.
 
+SPECIAL PATHS
+-------------
+For `btrfs qgroup show` subcommand, the ``path`` column may has some special
+strings:
+
+`<toplevel>`
+	The toplevel subvolume
+
+`<under deletion>`
+	The subvolume is unlinked, but not yet fully deleted.
+
+`<squota space holder>`
+	For simple quota mode only.
+	By its design, a fully deleted subvolume may still have accounting on
+	it, so even the subvolume is gone, the numbers are still here for future
+	accounting.
+
+`<stale>`
+	The qgroup has no corresponding subvolume anymore, and the qgroup
+	can be cleaned up under most cases.
+	The only exception is that, if the qgroup numbers are inconsistent and
+	the qgroup numbers are not all zeros, some older kernels may refuse to
+	delete such qgroups until a full rescan.
+
 QUOTA RESCAN
 ------------
 
diff --git a/cmds/qgroup.c b/cmds/qgroup.c
index 70a306117ebd..928c432ca432 100644
--- a/cmds/qgroup.c
+++ b/cmds/qgroup.c
@@ -71,6 +71,8 @@ struct qgroup_lookup {
 };
 
 struct btrfs_qgroup {
+	struct qgroup_lookup *lookup;
+
 	struct rb_node rb_node;
 	struct rb_node sort_node;
 	/*
@@ -321,6 +323,26 @@ static void print_qgroup_column_add_blank(enum btrfs_qgroup_column_enum column,
 		printf(" ");
 }
 
+static const char *get_qgroup_path(struct btrfs_qgroup *qgroup)
+{
+	if (qgroup->path)
+		return qgroup->path;
+
+	/* No path but not stale either, the qgroup is being deleted. */
+	if (!qgroup->stale)
+		return "<under deletion>";
+	/*
+	 * Squota mode stale qgroup, but not empty.
+	 * This is fully deleted but still necessary.
+	 */
+	if (qgroup->stale &&
+	    (qgroup->lookup->flags & BTRFS_QGROUP_STATUS_FLAG_SIMPLE_MODE) &&
+	    !is_qgroup_empty(qgroup))
+		return "<squota space holder>";
+
+	return "<stale>";
+}
+
 static void print_path_column(struct btrfs_qgroup *qgroup)
 {
 	struct btrfs_qgroup_list *list = NULL;
@@ -338,11 +360,8 @@ static void print_path_column(struct btrfs_qgroup *qgroup)
 			if (count)
 				pr_verbose(LOG_DEFAULT, " ");
 			if (level == 0) {
-				const char *path = member->path;
-
-				if (!path)
-					path = "<stale>";
-				pr_verbose(LOG_DEFAULT, "%s", path);
+				pr_verbose(LOG_DEFAULT, "%s",
+					   get_qgroup_path(qgroup));
 			} else {
 				pr_verbose(LOG_DEFAULT, "%llu/%llu", level, sid);
 			}
@@ -353,7 +372,7 @@ static void print_path_column(struct btrfs_qgroup *qgroup)
 	} else if (qgroup->path) {
 		pr_verbose(LOG_DEFAULT, "%s%s", (*qgroup->path ? "" : "<toplevel>"), qgroup->path);
 	} else {
-		pr_verbose(LOG_DEFAULT, "<stale>");
+		pr_verbose(LOG_DEFAULT, "%s", get_qgroup_path(qgroup));
 	}
 }
 
@@ -805,6 +824,7 @@ static struct btrfs_qgroup *get_or_add_qgroup(int fd,
 		return ERR_PTR(-ENOMEM);
 	}
 
+	bq->lookup = qgroup_lookup;
 	bq->qgroupid = qgroupid;
 	INIT_LIST_HEAD(&bq->qgroups);
 	INIT_LIST_HEAD(&bq->members);
-- 
2.45.0

Re: [PATCH 0/4] btrfs-progs: cmds/qgroup: enhance stale qgroups handling

[David Sterba]

On Thu, May 09, 2024 at 06:42:29PM +0930, Qu Wenruo wrote:
> Currently the `<stale>` label is give incorrectly for a lot of qgroups
> that can not and should not be deleted.
> 
> The patchset would address the problem for `btrfs qgroup show` and
> `btrfs qgroup clear-stale` commands, so that:
> 
> - `clear-stale` would not try to delete incorrect qgroups
>   Thus it should not return false errors.
> 
> - `show` would show extra types of qgroups
>   Including `<under deletion>` and `<squota space holder>`
> 
> Qu Wenruo (4):
>   btrfs-progs: cmds/qgroup: sync the fs before doing qgroup search
>   btrfs-progs: cmds/qgroup: add qgroup_lookup::flags member
>   btrfs-progs: cmds/qgroup: handle stale qgroup deleteion more
>     accurately
>   btrfs-progs: cmds/qgroup: add more special status for qgroups

Added to devel, thanks. Please use the subjects that match the style
used in progs, ie. no 'cmds/' prefix.