[PATCH 0/2] btrfs-progs: revert `btrfs subvolume delete --delete-qgroup` option

[PATCH 0/2] btrfs-progs: revert `btrfs subvolume delete --delete-qgroup` option

[Qu Wenruo]

The introduction of `btrfs subvolume delete --delete-qgroup` would not
work for a lot of real world cases.

This would leads to unnecessary errors, and can be very confusing for
end users.

Furthermore the new options do not take the lifespan of a subvolume into
consideration or the possible conflicts with other qgroup features.

Although it's already too late, we should revert it to prevent further
confusion and damage.

Qu Wenruo (2):
  Revert "btrfs-progs: subvol delete: add options to delete the qgroup"
  btrfs: misc-tests: remove the subvol-delete-qgroup test case

 Documentation/btrfs-subvolume.rst             |  7 ---
 cmds/subvolume.c                              | 26 ----------
 .../061-subvol-delete-qgroup/test.sh          | 47 -------------------
 3 files changed, 80 deletions(-)
 delete mode 100755 tests/misc-tests/061-subvol-delete-qgroup/test.sh

--
2.44.0

[PATCH 1/2] Revert "btrfs-progs: subvol delete: add options to delete the qgroup"

[Qu Wenruo]

This reverts commit 9da773aa46ba33a9c3bdd83b31e15b031b3bfe4d.

There are several problems related to the --delete-qgroup option:

- Currently kernel doesn not allow to delete non-empty qgroups

- A qgroup can only be empty after fully dropped and a transaction is
  committed
  The tool doesn not take either factor into consideration

- Things like drop_subtree_threshold or other operations can mark qgroup
  inconsistent and skip accounting
  This can mean the target qgroup will never be empty until next rescan

On the other hand, even if we do the proper way, it would hugely delay
the command (wait until the subvolume to be dropped).

Furthermore, even all the wait is handled properly,
drop_subtree_threshold can still prevent us deleting the qgroup (qgroup
numbers are inconsistent, and accounting is skipped completely).

So this qgroup cleanup needs kernel work to support them anyway, and it
would be much easier to handle all the operations inside kernel.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-subvolume.rst |  7 -------
 cmds/subvolume.c                  | 26 --------------------------
 2 files changed, 33 deletions(-)

diff --git a/Documentation/btrfs-subvolume.rst b/Documentation/btrfs-subvolume.rst
index d4379a2df83d..d1e89f15e1e2 100644
--- a/Documentation/btrfs-subvolume.rst
+++ b/Documentation/btrfs-subvolume.rst
@@ -112,13 +112,6 @@ delete [options] [<subvolume> [<subvolume>...]], delete -i|--subvolid <subvolid>
         -i|--subvolid <subvolid>
                 subvolume id to be removed instead of the <path> that should point to the
                 filesystem with the subvolume
-
-        --delete-qgroup
-                also delete the qgroup 0/subvolid if it exists
-
-        --no-delete-qgroup
-                do not delete the 0/subvolid qgroup (default)
-
         -v|--verbose
                 (deprecated) alias for global *-v* option
 
diff --git a/cmds/subvolume.c b/cmds/subvolume.c
index 319f595ca495..f77a6e091569 100644
--- a/cmds/subvolume.c
+++ b/cmds/subvolume.c
@@ -348,8 +348,6 @@ static const char * const cmd_subvolume_delete_usage[] = {
 	OPTLINE("-c|--commit-after", "wait for transaction commit at the end of the operation"),
 	OPTLINE("-C|--commit-each", "wait for transaction commit after deleting each subvolume"),
 	OPTLINE("-i|--subvolid", "subvolume id of the to be removed subvolume"),
-	OPTLINE("--delete-qgroup", "also delete the qgroup 0/subvolid if it exists"),
-	OPTLINE("--no-delete-qgroup", "do not delete the qgroup 0/subvolid if it exists (default)"),
 	OPTLINE("-v|--verbose", "deprecated, alias for global -v option"),
 	HELPINFO_INSERT_GLOBALS,
 	HELPINFO_INSERT_VERBOSE,
@@ -378,20 +376,15 @@ static int cmd_subvolume_delete(const struct cmd_struct *cmd, int argc, char **a
 	enum { COMMIT_AFTER = 1, COMMIT_EACH = 2 };
 	enum btrfs_util_error err;
 	uint64_t default_subvol_id, target_subvol_id = 0;
-	bool opt_delete_qgroup = false;
 
 	optind = 0;
 	while (1) {
 		int c;
-		enum { GETOPT_VAL_DELETE_QGROUP = GETOPT_VAL_FIRST,
-		       GETOPT_VAL_NO_DELETE_QGROUP };
 		static const struct option long_options[] = {
 			{"commit-after", no_argument, NULL, 'c'},
 			{"commit-each", no_argument, NULL, 'C'},
 			{"subvolid", required_argument, NULL, 'i'},
 			{"verbose", no_argument, NULL, 'v'},
-			{"delete-qgroup", no_argument, NULL, GETOPT_VAL_DELETE_QGROUP },
-			{"no-delete-qgroup", no_argument, NULL, GETOPT_VAL_NO_DELETE_QGROUP },
 			{NULL, 0, NULL, 0}
 		};
 
@@ -412,12 +405,6 @@ static int cmd_subvolume_delete(const struct cmd_struct *cmd, int argc, char **a
 		case 'v':
 			bconf_be_verbose();
 			break;
-		case GETOPT_VAL_DELETE_QGROUP:
-			opt_delete_qgroup = true;
-			break;
-		case GETOPT_VAL_NO_DELETE_QGROUP:
-			opt_delete_qgroup = false;
-			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -553,19 +540,6 @@ again:
 			warning("deletion failed with EPERM, you don't have permissions or send may be in progress");
 		ret = 1;
 		goto out;
-	} else if (opt_delete_qgroup) {
-		struct btrfs_ioctl_qgroup_create_args args = { .qgroupid = target_subvol_id };
-
-		ret = ioctl(fd, BTRFS_IOC_QGROUP_CREATE, &args);
-		if (ret == 0) {
-			pr_verbose(LOG_DEFAULT, "Delete qgroup 0/%" PRIu64 "\n", target_subvol_id);
-		} else if (ret < 0 && (errno == ENOTCONN || errno == ENOENT)) {
-			/* Quotas not enabled, or there's no qgroup. */
-		} else {
-			warning("unable to delete qgroup 0/%llu: %m", subvolid);
-		}
-		/* Qgroup errors are not fatal. */
-		ret = 0;
 	}
 
 	if (commit_mode == COMMIT_EACH) {
-- 
2.44.0

[PATCH 2/2] btrfs: misc-tests: remove the subvol-delete-qgroup test case

[Qu Wenruo]

The test case relies on `--delete-qgroup` option, but the feature is not
properly designed from the very beginning, and would not work for most
cases.

The test case does not take the complexity of subvolume dropping into
consideration and only tested the simplest cases.

Since the `--delete-qgroup` option patch is reverted, we also need to
revert this one too.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../061-subvol-delete-qgroup/test.sh          | 47 -------------------
 1 file changed, 47 deletions(-)
 delete mode 100755 tests/misc-tests/061-subvol-delete-qgroup/test.sh

diff --git a/tests/misc-tests/061-subvol-delete-qgroup/test.sh b/tests/misc-tests/061-subvol-delete-qgroup/test.sh
deleted file mode 100755
index c2637ac33cdc..000000000000
--- a/tests/misc-tests/061-subvol-delete-qgroup/test.sh
+++ /dev/null
@@ -1,47 +0,0 @@
-#!/bin/bash
-# Create subvolumes with enabled qutoas and check that subvolume deleteion will
-# also delete the 0-level qgruop.
-
-source "$TEST_TOP/common" || exit
-
-setup_root_helper
-prepare_test_dev
-
-run_check_mkfs_test_dev
-run_check_mount_test_dev
-run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT"/file bs=1M count=1
-
-# Without quotas
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --delete-qgroup "$TEST_MNT/subv1"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --no-delete-qgroup "$TEST_MNT/subv2"
-run_check $SUDO_HELPER "$TOP/btrfs" filesystem sync "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume sync "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvol list "$TEST_MNT"
-
-# With quotas enabled
-run_check $SUDO_HELPER "$TOP/btrfs" quota enable "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv1"
-rootid1=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid "$TEST_MNT/subv1")
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv2"
-rootid2=$(run_check_stdout "$TOP/btrfs" inspect-internal rootid "$TEST_MNT/subv2")
-run_check $SUDO_HELPER "$TOP/btrfs" qgroup create 1/1 "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" qgroup assign "0/$rootid1" 1/1 "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" qgroup assign "0/$rootid2" 1/1 "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" quota rescan --wait "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvol list "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" qgroup show "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --delete-qgroup "$TEST_MNT/subv1"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete --no-delete-qgroup "$TEST_MNT/subv2"
-run_check $SUDO_HELPER "$TOP/btrfs" filesystem sync "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvolume sync "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" subvol list "$TEST_MNT"
-run_check $SUDO_HELPER "$TOP/btrfs" qgroup show "$TEST_MNT"
-if run_check_stdout $SUDO_HELPER "$TOP/btrfs" qgroup show "$TEST_MNT" | grep -q "0/$rootid1"; then
-	_fail "qgroup 0/$rootid1 not deleted"
-fi
-if ! run_check_stdout $SUDO_HELPER "$TOP/btrfs" qgroup show "$TEST_MNT" | grep -q "0/$rootid2"; then
-	_fail "qgroup 0/$rootid2 deleted"
-fi
-run_check_umount_test_dev
-- 
2.44.0

Re: [PATCH 0/2] btrfs-progs: revert `btrfs subvolume delete --delete-qgroup` option

[David Sterba]

On Fri, Apr 26, 2024 at 07:35:51AM +0930, Qu Wenruo wrote:
> The introduction of `btrfs subvolume delete --delete-qgroup` would not
> work for a lot of real world cases.
> 
> This would leads to unnecessary errors, and can be very confusing for
> end users.
> 
> Furthermore the new options do not take the lifespan of a subvolume into
> consideration or the possible conflicts with other qgroup features.
> 
> Although it's already too late, we should revert it to prevent further
> confusion and damage.

The options have been there since 6.6.3 so it's about 2 full releases
but it better be removed now.

> Qu Wenruo (2):
>   Revert "btrfs-progs: subvol delete: add options to delete the qgroup"
>   btrfs: misc-tests: remove the subvol-delete-qgroup test case

Added to devel, thanks.

Re: [PATCH 2/2] btrfs: misc-tests: remove the subvol-delete-qgroup test case

[David Sterba]

Subject: "btrfs: misc-tests: remove the subvol-delete-qgroup test case"

Please use the subject format "btrfs-progs: tests: ..."