[PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[Qu Wenruo]

This is the first step to convert btrfstune functionality to "btrfs
tune" subcommand group.

For now only binary features, aka set and clear, is supported,
thus uuid and csum change is not yet implemented.
(Both need their own subcommand groups other than set/clear groups)

And even for set/clear, there is some changes to btrfstune:

- Merge seed feature into set/clear
  To enable seeding, just go "btrfs tune set seed <device>".

- All supported features can be checked by "list-all" feature
  Please note that, "btrfs tune set list-all" and
  "btrfs tune clear list-all" will have different output.

  The reason is some fundamental features like no-holes can not be
  disabled.


Qu Wenruo (7):
  btrfs-progs: export btrfs_feature structure
  btrfs-progs: cmds: add "btrfs tune set" subcommand group
  btrfs-progs: cmds/tune: add set support for free-space-tree feature
  btrfs-progs: cmds/tune: add set support for block-group-tree feature
  btrfs-progs: cmds/tune: add set support for seeding device
  btrfs-progs: cmds/tune: add "btrfs tune clear" subcommand
  btrfs-progs: tests/cli: add a test case for "btrfs tune" subcommand

 Documentation/btrfs-tune.rst           |  47 +++
 Documentation/btrfs.rst                |   5 +
 Documentation/conf.py                  |   1 +
 Documentation/man-index.rst            |   1 +
 Makefile                               |   4 +-
 btrfs.c                                |   1 +
 cmds/commands.h                        |   1 +
 cmds/tune.c                            | 448 +++++++++++++++++++++++++
 common/fsfeatures.c                    |  53 ---
 common/fsfeatures.h                    |  50 +++
 tests/cli-tests/018-btrfs-tune/test.sh |  40 +++
 11 files changed, 596 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/btrfs-tune.rst
 create mode 100644 cmds/tune.c
 create mode 100755 tests/cli-tests/018-btrfs-tune/test.sh

--
2.42.0

[PATCH 1/7] btrfs-progs: export btrfs_feature structure

[Qu Wenruo]

For the incoming "btrfs tune" subcommand, we will have different
features supported by that subcommand.

Instead of bloating the runtime and mkfs features, here we just export
btrfs_feature, so each subcommand can have their own definition of
supported features.

And since we're here, also add needed headers for future users of
"fsfeatures.h".

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/fsfeatures.c | 53 ---------------------------------------------
 common/fsfeatures.h | 50 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 53 deletions(-)

diff --git a/common/fsfeatures.c b/common/fsfeatures.c
index 9ee392d3a8a6..f8eeea7695c1 100644
--- a/common/fsfeatures.c
+++ b/common/fsfeatures.c
@@ -32,64 +32,11 @@
 #include "common/sysfs-utils.h"
 #include "common/messages.h"
 
-/*
- * Insert a root item for temporary tree root
- *
- * Only used in make_btrfs_v2().
- */
-#define VERSION_TO_STRING3(name, a,b,c)				\
-	.name ## _str = #a "." #b "." #c,			\
-	.name ## _ver = KERNEL_VERSION(a,b,c)
-#define VERSION_TO_STRING2(name, a,b)				\
-	.name ## _str = #a "." #b,				\
-	.name ## _ver = KERNEL_VERSION(a,b,0)
-#define VERSION_NULL(name)					\
-	.name ## _str = NULL,					\
-	.name ## _ver = 0
-
 enum feature_source {
 	FS_FEATURES,
 	RUNTIME_FEATURES,
 };
 
-/*
- * Feature stability status and versions: compat <= safe <= default
- */
-struct btrfs_feature {
-	const char *name;
-
-	/*
-	 * At least one of the bit must be set in the following *_flag member.
-	 *
-	 * For features like list-all and quota which don't have any
-	 * incompat/compat_ro bit set, it go to runtime_flag.
-	 */
-	u64 incompat_flag;
-	u64 compat_ro_flag;
-	u64 runtime_flag;
-
-	const char *sysfs_name;
-	/*
-	 * Compatibility with kernel of given version. Filesystem can be
-	 * mounted.
-	 */
-	const char *compat_str;
-	u32 compat_ver;
-	/*
-	 * Considered safe for use, but is not on by default, even if the
-	 * kernel supports the feature.
-	 */
-	const char *safe_str;
-	u32 safe_ver;
-	/*
-	 * Considered safe for use and will be turned on by default if
-	 * supported by the running kernel.
-	 */
-	const char *default_str;
-	u32 default_ver;
-	const char *desc;
-};
-
 static const struct btrfs_feature mkfs_features[] = {
 	{
 		.name		= "mixed-bg",
diff --git a/common/fsfeatures.h b/common/fsfeatures.h
index c4ab704862cd..c9fb489d2d79 100644
--- a/common/fsfeatures.h
+++ b/common/fsfeatures.h
@@ -19,7 +19,9 @@
 
 #include "kerncompat.h"
 #include <stdio.h>
+#include <linux/version.h>
 #include "kernel-lib/sizes.h"
+#include "kernel-shared/uapi/btrfs.h"
 
 #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K
 
@@ -43,6 +45,54 @@ struct btrfs_mkfs_features {
  */
 #define BTRFS_FEATURE_STRING_BUF_SIZE		(160)
 
+#define VERSION_TO_STRING3(name, a,b,c)				\
+	.name ## _str = #a "." #b "." #c,			\
+	.name ## _ver = KERNEL_VERSION(a,b,c)
+#define VERSION_TO_STRING2(name, a,b)				\
+	.name ## _str = #a "." #b,				\
+	.name ## _ver = KERNEL_VERSION(a,b,0)
+#define VERSION_NULL(name)					\
+	.name ## _str = NULL,					\
+	.name ## _ver = 0
+
+/*
+ * Feature stability status and versions: compat <= safe <= default
+ */
+struct btrfs_feature {
+	const char *name;
+
+	/*
+	 * At least one of the bit must be set in the following *_flag member.
+	 *
+	 * For features like list-all and quota which don't have any
+	 * incompat/compat_ro bit set, it go to runtime_flag.
+	 */
+	u64 incompat_flag;
+	u64 compat_ro_flag;
+	u64 runtime_flag;
+
+	const char *sysfs_name;
+	/*
+	 * Compatibility with kernel of given version. Filesystem can be
+	 * mounted.
+	 */
+	const char *compat_str;
+	u32 compat_ver;
+	/*
+	 * Considered safe for use, but is not on by default, even if the
+	 * kernel supports the feature.
+	 */
+	const char *safe_str;
+	u32 safe_ver;
+	/*
+	 * Considered safe for use and will be turned on by default if
+	 * supported by the running kernel.
+	 */
+	const char *default_str;
+	u32 default_ver;
+	const char *desc;
+};
+
 static const struct btrfs_mkfs_features btrfs_mkfs_default_features = {
 	.compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |
 			   BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID,
-- 
2.42.0

[PATCH 2/7] btrfs-progs: cmds: add "btrfs tune set" subcommand group

[Qu Wenruo]

As the first step to convert btrfstune into "btrfs tune" subcommand
group, this patch would add the following subcommand group:

 btrfs tune set <feature> [<device>]

For now the following features are supported:

- extref
- skinny-metadata
- no-holes
  All those are simple super block flags toggle.

- list-all
  This acts the same way as "mkfs.btrfs -O list-all", the difference is
  it would only list the supported features.
  (In the future, there will be "btrfs tune clear" subcommand, which
   would support a different set of features).

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-tune.rst |  40 ++++++
 Documentation/btrfs.rst      |   5 +
 Documentation/conf.py        |   1 +
 Documentation/man-index.rst  |   1 +
 Makefile                     |   2 +-
 btrfs.c                      |   1 +
 cmds/commands.h              |   1 +
 cmds/tune.c                  | 227 +++++++++++++++++++++++++++++++++++
 8 files changed, 277 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/btrfs-tune.rst
 create mode 100644 cmds/tune.c

diff --git a/Documentation/btrfs-tune.rst b/Documentation/btrfs-tune.rst
new file mode 100644
index 000000000000..827c92eadb72
--- /dev/null
+++ b/Documentation/btrfs-tune.rst
@@ -0,0 +1,40 @@
+btrfs-tune(8)
+==================
+
+SYNOPSIS
+--------
+
+**btrfs tune** <subcommand> [<args>]
+
+DESCRIPTION
+-----------
+
+:command:`btrfs tune` is used to tweak various btrfs features on a
+unmounted filesystem.
+
+SUBCOMMAND
+-----------
+
+set <feature> [<device>]
+        Set/enable a feature.
+
+        If *feature* is `list-all`, all supported features would be listed, and
+	no *device* parameter is needed.
+
+EXIT STATUS
+-----------
+
+**btrfs tune** returns a zero exit status if it succeeds. A non-zero value is
+returned in case of failure.
+
+AVAILABILITY
+------------
+
+**btrfs** is part of btrfs-progs.  Please refer to the documentation at
+`https://btrfs.readthedocs.io <https://btrfs.readthedocs.io>`_.
+
+SEE ALSO
+--------
+
+:doc:`mkfs.btrfs`,
+``mount(8)``
diff --git a/Documentation/btrfs.rst b/Documentation/btrfs.rst
index e878f158aaa1..5aea0d1a208c 100644
--- a/Documentation/btrfs.rst
+++ b/Documentation/btrfs.rst
@@ -134,6 +134,10 @@ subvolume
 	Create/delete/list/manage btrfs subvolume.
 	See :doc:`btrfs-subvolume` for details.
 
+tune
+	Change various btrfs features.
+	See :doc:`btrfs-tune` for details.
+
 .. _man-btrfs8-standalone-tools:
 
 STANDALONE TOOLS
@@ -150,6 +154,7 @@ btrfs-convert
         in-place conversion from ext2/3/4 filesystems to btrfs
 btrfstune
         tweak some filesystem properties on a unmounted filesystem
+	(will be replaced by `btrfs-tune`)
 btrfs-select-super
         rescue tool to overwrite primary superblock from a spare copy
 btrfs-find-root
diff --git a/Documentation/conf.py b/Documentation/conf.py
index 1025e10d7206..e0801bca4686 100644
--- a/Documentation/conf.py
+++ b/Documentation/conf.py
@@ -66,6 +66,7 @@ man_pages = [
     ('btrfs-check', 'btrfs-check', 'check or repair a btrfs filesystem', '', 8),
     ('btrfs-balance', 'btrfs-balance', 'balance block groups on a btrfs filesystem', '', 8),
     ('btrfs-subvolume', 'btrfs-subvolume', 'manage btrfs subvolumes', '', 8),
+    ('btrfs-tune', 'btrfs-tune', 'tweak btrfs features', '', 8),
     ('btrfs-map-logical', 'btrfs-map-logical', 'map btrfs logical extent to physical extent', '', 8),
     ('btrfs', 'btrfs', 'a toolbox to manage btrfs filesystems', '', 8),
     ('mkfs.btrfs', 'mkfs.btrfs', 'create a btrfs filesystem', '', 8),
diff --git a/Documentation/man-index.rst b/Documentation/man-index.rst
index 36d45d2903ea..5fcd4cbc4bee 100644
--- a/Documentation/man-index.rst
+++ b/Documentation/man-index.rst
@@ -28,6 +28,7 @@ Manual pages
    btrfs-select-super
    btrfs-send
    btrfs-subvolume
+   btrfs-tune
    btrfstune
    fsck.btrfs
    mkfs.btrfs
diff --git a/Makefile b/Makefile
index f4feb1fff8e1..9857daaa42ac 100644
--- a/Makefile
+++ b/Makefile
@@ -239,7 +239,7 @@ cmds_objects = cmds/subvolume.o cmds/subvolume-list.o \
 	       cmds/rescue-super-recover.o \
 	       cmds/property.o cmds/filesystem-usage.o cmds/inspect-dump-tree.o \
 	       cmds/inspect-dump-super.o cmds/inspect-tree-stats.o cmds/filesystem-du.o \
-	       cmds/reflink.o \
+	       cmds/reflink.o cmds/tune.o \
 	       mkfs/common.o check/mode-common.o check/mode-lowmem.o \
 	       check/clear-cache.o
 
diff --git a/btrfs.c b/btrfs.c
index 751f193ee2e0..c2dae0303ffe 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -389,6 +389,7 @@ static const struct cmd_group btrfs_cmd_group = {
 		&cmd_struct_scrub,
 		&cmd_struct_send,
 		&cmd_struct_subvolume,
+		&cmd_struct_tune,
 
 		/* Help and version stay last */
 		&cmd_struct_help,
diff --git a/cmds/commands.h b/cmds/commands.h
index 5ab7c881f634..aebacd718a7b 100644
--- a/cmds/commands.h
+++ b/cmds/commands.h
@@ -151,5 +151,6 @@ DECLARE_COMMAND(qgroup);
 DECLARE_COMMAND(replace);
 DECLARE_COMMAND(restore);
 DECLARE_COMMAND(rescue);
+DECLARE_COMMAND(tune);
 
 #endif
diff --git a/cmds/tune.c b/cmds/tune.c
new file mode 100644
index 000000000000..92c7b9f1502c
--- /dev/null
+++ b/cmds/tune.c
@@ -0,0 +1,227 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program; if not, write to the
+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
+ * Boston, MA 021110-1307, USA.
+ */
+
+#include <unistd.h>
+#include "kerncompat.h"
+#include "cmds/commands.h"
+#include "common/help.h"
+#include "common/fsfeatures.h"
+#include "kernel-shared/messages.h"
+#include "kernel-shared/disk-io.h"
+#include "kernel-shared/transaction.h"
+
+static const char * const cmd_tune_set_usage[] = {
+	"btrfs tune set <feature> [<device>]",
+	"Set/enable specified feature for the unmounted filesystem",
+	"",
+	HELPINFO_INSERT_GLOBALS,
+	HELPINFO_INSERT_VERBOSE,
+	NULL,
+};
+
+static const struct btrfs_feature set_features[] = {
+	{
+		.name		= "extref",
+		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF,
+		.sysfs_name	= "extended_iref",
+		VERSION_TO_STRING2(compat, 3,7),
+		VERSION_TO_STRING2(safe, 3,12),
+		VERSION_TO_STRING2(default, 3,12),
+		.desc		= "increased hardlink limit per file to 65536"
+	}, {
+		.name		= "skinny-metadata",
+		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA,
+		.sysfs_name	= "skinny_metadata",
+		VERSION_TO_STRING2(compat, 3,10),
+		VERSION_TO_STRING2(safe, 3,18),
+		VERSION_TO_STRING2(default, 3,18),
+		.desc		= "reduced-size metadata extent refs"
+	}, {
+		.name		= "no-holes",
+		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_NO_HOLES,
+		.sysfs_name	= "no_holes",
+		VERSION_TO_STRING2(compat, 3,14),
+		VERSION_TO_STRING2(safe, 4,0),
+		VERSION_TO_STRING2(default, 5,15),
+		.desc		= "no explicit hole extents for files"
+	},
+	/* Keep this one last */
+	{
+		.name		= "list-all",
+		.runtime_flag	= BTRFS_FEATURE_RUNTIME_LIST_ALL,
+		.sysfs_name	= NULL,
+		VERSION_NULL(compat),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= NULL
+	}
+};
+
+static void list_all_features(const char *prefix,
+			      const struct btrfs_feature *features,
+			      int nr_features)
+{
+	/* We should have at least one empty feature. */
+	ASSERT(nr_features > 1);
+
+	printf("features available to %s:\n", prefix);
+	for (int i = 0; i < nr_features - 1; i++) {
+		const struct btrfs_feature *feat = features + i;
+		const char *sep = "";
+
+		fprintf(stderr, "%-20s- %s (", feat->name, feat->desc);
+		if (feat->compat_ver) {
+			fprintf(stderr, "compat=%s", feat->compat_str);
+			sep = ", ";
+		}
+		if (feat->safe_ver) {
+			fprintf(stderr, "%ssafe=%s", sep, feat->safe_str);
+			sep = ", ";
+		}
+		if (feat->default_ver)
+			fprintf(stderr, "%sdefault=%s", sep, feat->default_str);
+		fprintf(stderr, ")\n");
+	}
+}
+
+static int check_features(const char *name, const struct btrfs_feature *features,
+			  int nr_features)
+{
+	bool found = false;
+
+	for (int i = 0; i < nr_features; i++) {
+		const struct btrfs_feature *feat = features + i;
+
+		if (!strcmp(feat->name, name)) {
+			found = true;
+			break;
+		}
+	}
+	if (found)
+		return 0;
+	return -EINVAL;
+}
+
+static int set_super_incompat_flags(struct btrfs_fs_info *fs_info, u64 flags)
+{
+	struct btrfs_root *root = fs_info->tree_root;
+	struct btrfs_trans_handle *trans;
+	struct btrfs_super_block *disk_super;
+	u64 super_flags;
+	int ret;
+
+	disk_super = fs_info->super_copy;
+	super_flags = btrfs_super_incompat_flags(disk_super);
+	super_flags |= flags;
+	trans = btrfs_start_transaction(root, 1);
+	BUG_ON(IS_ERR(trans));
+	btrfs_set_super_incompat_flags(disk_super, super_flags);
+	ret = btrfs_commit_transaction(trans, root);
+
+	return ret;
+}
+
+static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
+{
+	struct btrfs_fs_info *fs_info;
+	struct open_ctree_args oca = { 0 };
+	char *feature;
+	char *path;
+	int ret = 0;
+
+	optind = 0;
+	while (1) {
+		int c = getopt(argc, argv, "");
+		if (c < 0)
+			break;
+
+		switch (c) {
+		default:
+			usage_unknown_option(cmd, argv);
+		}
+	}
+
+	if (check_argc_min(argc - optind, 1))
+		return 1;
+
+	feature = argv[optind];
+
+	if (check_features(feature, set_features, ARRAY_SIZE(set_features))) {
+		error("Unknown feature to set: %s", feature);
+		return 1;
+	}
+	if (!strcmp(feature, "list-all")) {
+		list_all_features("set", set_features, ARRAY_SIZE(set_features));
+		return 0;
+	}
+
+	if (check_argc_exact(argc - optind, 2))
+		return 1;
+
+	path = argv[optind + 1];
+	oca.flags = OPEN_CTREE_WRITES;
+	oca.filename = path;
+	fs_info = open_ctree_fs_info(&oca);
+	if (!fs_info) {
+		error("failed to open btrfs");
+		ret = -EIO;
+		goto out;
+	}
+	/*
+	 * For those 3 features, we only need to update the superblock to add
+	 * the new feature flags.
+	 */
+	if (!strcmp(feature, "extref") ||
+	    !strcmp(feature, "skinny-metadata") ||
+	    !strcmp(feature, "no-holes")) {
+		u64 incompat_flags = 0;
+
+		if (!strcmp(feature, "extref"))
+			incompat_flags |= BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF;
+		if (!strcmp(feature, "skinny-metadata"))
+			incompat_flags |= BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA;
+		if (!strcmp(feature, "no-holes"))
+			incompat_flags |= BTRFS_FEATURE_INCOMPAT_NO_HOLES;
+		ret = set_super_incompat_flags(fs_info, incompat_flags);
+		if (ret < 0) {
+			errno = -ret;
+			error("failed to set feature '%s': %m", feature);
+		}
+		goto out;
+	}
+
+out:
+	if (fs_info)
+		close_ctree_fs_info(fs_info);
+	return !!ret;
+}
+
+static DEFINE_SIMPLE_COMMAND(tune_set, "set");
+
+static const char * const tune_cmd_group_usage[] = {
+	"btrfs tune <command> <args>",
+	NULL,
+};
+
+static const char tune_cmd_group_info[] = "change various btrfs features";
+
+static const struct cmd_group tune_cmd_group = {
+	tune_cmd_group_usage, tune_cmd_group_info, {
+		&cmd_struct_tune_set,
+		NULL
+	}
+};
+DEFINE_GROUP_COMMAND_TOKEN(tune);
-- 
2.42.0

[PATCH 3/7] btrfs-progs: cmds/tune: add set support for free-space-tree feature

[Qu Wenruo]

This patch allows "btrfs tune set" to enable free-space-tree feature,
using pretty much the same code from btrfstune.

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

diff --git a/cmds/tune.c b/cmds/tune.c
index 92c7b9f1502c..9906cde393f4 100644
--- a/cmds/tune.c
+++ b/cmds/tune.c
@@ -17,11 +17,13 @@
 #include <unistd.h>
 #include "kerncompat.h"
 #include "cmds/commands.h"
+#include "check/clear-cache.h"
 #include "common/help.h"
 #include "common/fsfeatures.h"
 #include "kernel-shared/messages.h"
 #include "kernel-shared/disk-io.h"
 #include "kernel-shared/transaction.h"
+#include "kernel-shared/free-space-tree.h"
 
 static const char * const cmd_tune_set_usage[] = {
 	"btrfs tune set <feature> [<device>]",
@@ -57,6 +59,15 @@ static const struct btrfs_feature set_features[] = {
 		VERSION_TO_STRING2(safe, 4,0),
 		VERSION_TO_STRING2(default, 5,15),
 		.desc		= "no explicit hole extents for files"
+	}, {
+		.name		= "free-space-tree",
+		.compat_ro_flag	= BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |
+				  BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID,
+		.sysfs_name = "free_space_tree",
+		VERSION_TO_STRING2(compat, 4,5),
+		VERSION_TO_STRING2(safe, 4,9),
+		VERSION_TO_STRING2(default, 5,15),
+		.desc		= "free space tree (space_cache=v2)"
 	},
 	/* Keep this one last */
 	{
@@ -134,6 +145,36 @@ static int set_super_incompat_flags(struct btrfs_fs_info *fs_info, u64 flags)
 	return ret;
 }
 
+static int convert_to_fst(struct btrfs_fs_info *fs_info)
+{
+	int ret;
+
+	/* We may have invalid old v2 cache, clear them first. */
+	if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) {
+		ret = btrfs_clear_free_space_tree(fs_info);
+		if (ret < 0) {
+			errno = -ret;
+			error("failed to clear stale v2 free space cache: %m");
+			return ret;
+		}
+	}
+	ret = btrfs_clear_v1_cache(fs_info);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to clear v1 free space cache: %m");
+		return ret;
+	}
+
+	ret = btrfs_create_free_space_tree(fs_info);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to create free space tree: %m");
+		return ret;
+	}
+	pr_verbose(LOG_DEFAULT, "Converted to free space tree feature\n");
+	return ret;
+}
+
 static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
 {
 	struct btrfs_fs_info *fs_info;
@@ -202,6 +243,17 @@ static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
 		}
 		goto out;
 	}
+	if (!strcmp(feature, "free-space-tree")) {
+		if (btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE_VALID)) {
+			error("filesystem already has free-space-tree feature");
+			ret = -EINVAL;
+			goto out;
+		}
+		ret = convert_to_fst(fs_info);
+		if (ret < 0)
+			error("failed to convert the filesystem to free-space-tree feature");
+		goto out;
+	}
 
 out:
 	if (fs_info)
-- 
2.42.0

[PATCH 4/7] btrfs-progs: cmds/tune: add set support for block-group-tree feature

[Qu Wenruo]

Most of the code is reusing the existing code from btrfs-tune, only
needs to add extra linkage dependency on tune/convert-bgt.o.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Makefile    |  2 +-
 cmds/tune.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 9857daaa42ac..45d64129352c 100644
--- a/Makefile
+++ b/Makefile
@@ -241,7 +241,7 @@ cmds_objects = cmds/subvolume.o cmds/subvolume-list.o \
 	       cmds/inspect-dump-super.o cmds/inspect-tree-stats.o cmds/filesystem-du.o \
 	       cmds/reflink.o cmds/tune.o \
 	       mkfs/common.o check/mode-common.o check/mode-lowmem.o \
-	       check/clear-cache.o
+	       check/clear-cache.o tune/convert-bgt.o
 
 libbtrfs_objects = \
 		kernel-lib/rbtree.o	\
diff --git a/cmds/tune.c b/cmds/tune.c
index 9906cde393f4..76ad9ebfc39e 100644
--- a/cmds/tune.c
+++ b/cmds/tune.c
@@ -24,6 +24,7 @@
 #include "kernel-shared/disk-io.h"
 #include "kernel-shared/transaction.h"
 #include "kernel-shared/free-space-tree.h"
+#include "tune/tune.h"
 
 static const char * const cmd_tune_set_usage[] = {
 	"btrfs tune set <feature> [<device>]",
@@ -68,6 +69,14 @@ static const struct btrfs_feature set_features[] = {
 		VERSION_TO_STRING2(safe, 4,9),
 		VERSION_TO_STRING2(default, 5,15),
 		.desc		= "free space tree (space_cache=v2)"
+	}, {
+		.name		= "block-group-tree",
+		.compat_ro_flag	= BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE,
+		.sysfs_name	= "block_group_tree",
+		VERSION_TO_STRING2(compat, 6,1),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= "block group tree to reduce mount time"
 	},
 	/* Keep this one last */
 	{
@@ -254,6 +263,22 @@ static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
 			error("failed to convert the filesystem to free-space-tree feature");
 		goto out;
 	}
+	if (!strcmp(feature, "block-group-tree")) {
+		if (btrfs_fs_compat_ro(fs_info, BLOCK_GROUP_TREE)) {
+			error("filesystem already has block-group-tree feature");
+			ret = -EINVAL;
+			goto out;
+		}
+		if (!btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE_VALID)) {
+			error("the filesystem doesn't have free-space-tree feature, please enable it first");
+			ret = -EINVAL;
+			goto out;
+		}
+		ret = convert_to_bg_tree(fs_info);
+		if (ret < 0)
+			error("failed to convert the filesystem to free-space-tree feature");
+		goto out;
+	}
 
 out:
 	if (fs_info)
-- 
2.42.0

[PATCH 5/7] btrfs-progs: cmds/tune: add set support for seeding device

[Qu Wenruo]

With the new "btrfs tune set" command, seeding device support can be
added just like all other features.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Makefile    |  2 +-
 cmds/tune.c | 35 ++++++++++++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 45d64129352c..b502336d7441 100644
--- a/Makefile
+++ b/Makefile
@@ -241,7 +241,7 @@ cmds_objects = cmds/subvolume.o cmds/subvolume-list.o \
 	       cmds/inspect-dump-super.o cmds/inspect-tree-stats.o cmds/filesystem-du.o \
 	       cmds/reflink.o cmds/tune.o \
 	       mkfs/common.o check/mode-common.o check/mode-lowmem.o \
-	       check/clear-cache.o tune/convert-bgt.o
+	       check/clear-cache.o tune/convert-bgt.o tune/seeding.o
 
 libbtrfs_objects = \
 		kernel-lib/rbtree.o	\
diff --git a/cmds/tune.c b/cmds/tune.c
index 76ad9ebfc39e..195d0a235fc8 100644
--- a/cmds/tune.c
+++ b/cmds/tune.c
@@ -19,6 +19,7 @@
 #include "cmds/commands.h"
 #include "check/clear-cache.h"
 #include "common/help.h"
+#include "common/utils.h"
 #include "common/fsfeatures.h"
 #include "kernel-shared/messages.h"
 #include "kernel-shared/disk-io.h"
@@ -30,6 +31,7 @@ static const char * const cmd_tune_set_usage[] = {
 	"btrfs tune set <feature> [<device>]",
 	"Set/enable specified feature for the unmounted filesystem",
 	"",
+	OPTLINE("-f", "force dangerous operations."),
 	HELPINFO_INSERT_GLOBALS,
 	HELPINFO_INSERT_VERBOSE,
 	NULL,
@@ -77,6 +79,13 @@ static const struct btrfs_feature set_features[] = {
 		VERSION_NULL(safe),
 		VERSION_NULL(default),
 		.desc		= "block group tree to reduce mount time"
+	}, {
+		.name		= "seed",
+		.sysfs_name	= NULL,
+		VERSION_TO_STRING3(compat, 2,6,29),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= "seeding device support"
 	},
 	/* Keep this one last */
 	{
@@ -190,15 +199,19 @@ static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
 	struct open_ctree_args oca = { 0 };
 	char *feature;
 	char *path;
+	bool force = false;
 	int ret = 0;
 
 	optind = 0;
 	while (1) {
-		int c = getopt(argc, argv, "");
+		int c = getopt(argc, argv, "f");
 		if (c < 0)
 			break;
 
 		switch (c) {
+		case 'f':
+			force = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
@@ -279,6 +292,26 @@ static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
 			error("failed to convert the filesystem to free-space-tree feature");
 		goto out;
 	}
+	if (!strcmp(feature, "seed")) {
+		if (btrfs_fs_incompat(fs_info, METADATA_UUID)) {
+			error("SEED flag cannot be changed on a metadata-uuid changed fs");
+			ret = -EINVAL;
+			goto out;
+		}
+		if (!force) {
+			warning(
+"this is dangerous, clearing the seeding flag may cause the derived device not to be mountable!");
+			ret = ask_user("We are going to clear the seeding flag, are you sure?");
+			if (!ret) {
+				error("clear seeding flag canceled");
+				ret = -EINVAL;
+				goto out;
+			}
+		}
+
+		ret = update_seeding_flag(fs_info->tree_root, path, 1, force);
+		goto out;
+	}
 
 out:
 	if (fs_info)
-- 
2.42.0

[PATCH 6/7] btrfs-progs: cmds/tune: add "btrfs tune clear" subcommand

[Qu Wenruo]

This new "btrfs tune clear" subcommand is mostly the same as "btrfs tune
set", but with much less features supported:

- seed
- block-group-tree

For the features in "btrfs tune set" but not in "btrfs tune clear", they
are mostly default features now, and we won't support disabling those
features anymore.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-tune.rst |   7 +++
 cmds/tune.c                  | 111 +++++++++++++++++++++++++++++++++++
 2 files changed, 118 insertions(+)

diff --git a/Documentation/btrfs-tune.rst b/Documentation/btrfs-tune.rst
index 827c92eadb72..4a556a197540 100644
--- a/Documentation/btrfs-tune.rst
+++ b/Documentation/btrfs-tune.rst
@@ -21,6 +21,13 @@ set <feature> [<device>]
         If *feature* is `list-all`, all supported features would be listed, and
 	no *device* parameter is needed.
 
+clear <feature> [<device>]
+        Set/enable a feature.
+
+        If *feature* is `list-all`, all supported features would be listed, and
+	no *device* parameter is needed. And the listed features may be different
+	compared to the ones from `btrfs tune set list-all`.
+
 EXIT STATUS
 -----------
 
diff --git a/cmds/tune.c b/cmds/tune.c
index 195d0a235fc8..a85d017ba2e1 100644
--- a/cmds/tune.c
+++ b/cmds/tune.c
@@ -321,6 +321,116 @@ out:
 
 static DEFINE_SIMPLE_COMMAND(tune_set, "set");
 
+static const struct btrfs_feature clear_features[] = {
+	{
+		.name		= "block-group-tree",
+		.compat_ro_flag	= BTRFS_FEATURE_COMPAT_RO_BLOCK_GROUP_TREE,
+		.sysfs_name	= "block_group_tree",
+		VERSION_TO_STRING2(compat, 6,1),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= "block group tree to reduce mount time"
+	}, {
+		.name		= "seed",
+		.sysfs_name	= NULL,
+		VERSION_TO_STRING3(compat, 2,6,29),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= "seeding device support"
+	},
+	/* Keep this one last */
+	{
+		.name		= "list-all",
+		.runtime_flag	= BTRFS_FEATURE_RUNTIME_LIST_ALL,
+		.sysfs_name	= NULL,
+		VERSION_NULL(compat),
+		VERSION_NULL(safe),
+		VERSION_NULL(default),
+		.desc		= NULL
+	}
+};
+
+static int cmd_tune_clear(const struct cmd_struct *cmd, int argc, char **argv)
+{
+	struct btrfs_fs_info *fs_info;
+	struct open_ctree_args oca = { 0 };
+	char *feature;
+	char *path;
+	int ret = 0;
+
+	optind = 0;
+	while (1) {
+		int c = getopt(argc, argv, "f");
+		if (c < 0)
+			break;
+
+		switch (c) {
+		default:
+			usage_unknown_option(cmd, argv);
+		}
+	}
+
+	if (check_argc_min(argc - optind, 1))
+		return 1;
+
+	feature = argv[optind];
+
+	if (check_features(feature, clear_features, ARRAY_SIZE(clear_features))) {
+		error("Unknown feature to clear: %s", feature);
+		return 1;
+	}
+
+	if (!strcmp(feature, "list-all")) {
+		list_all_features("set", set_features, ARRAY_SIZE(set_features));
+		return 0;
+	}
+
+	if (check_argc_exact(argc - optind, 2))
+		return 1;
+
+	path = argv[optind + 1];
+	oca.flags = OPEN_CTREE_WRITES;
+	oca.filename = path;
+	fs_info = open_ctree_fs_info(&oca);
+	if (!fs_info) {
+		error("failed to open btrfs");
+		ret = -EIO;
+		goto out;
+	}
+	if (!strcmp(feature, "seed")) {
+		ret = update_seeding_flag(fs_info->tree_root, path, 0, 0);
+		goto out;
+	}
+	if (!strcmp(feature, "block-group-tree")) {
+		if (!btrfs_fs_compat_ro(fs_info, BLOCK_GROUP_TREE)) {
+			error("filesystem doesn't have block-group-tree feature");
+			ret = -EINVAL;
+			goto out;
+		}
+		ret = convert_to_extent_tree(fs_info);
+		if (ret < 0) {
+			error("failed to convert the filesystem from block group tree feature");
+			goto out;
+		}
+		goto out;
+	}
+out:
+	if (fs_info)
+		close_ctree_fs_info(fs_info);
+	return !!ret;
+}
+
+static const char * const cmd_tune_clear_usage[] = {
+	"btrfs tune clear <feature> [<device>]",
+	"Clear/disable specified feature for the unmounted filesystem",
+	"",
+	HELPINFO_INSERT_GLOBALS,
+	HELPINFO_INSERT_VERBOSE,
+	NULL,
+};
+
+static DEFINE_SIMPLE_COMMAND(tune_clear, "clear");
+
 static const char * const tune_cmd_group_usage[] = {
 	"btrfs tune <command> <args>",
 	NULL,
@@ -331,6 +441,7 @@ static const char tune_cmd_group_info[] = "change various btrfs features";
 static const struct cmd_group tune_cmd_group = {
 	tune_cmd_group_usage, tune_cmd_group_info, {
 		&cmd_struct_tune_set,
+		&cmd_struct_tune_clear,
 		NULL
 	}
 };
-- 
2.42.0

[PATCH 7/7] btrfs-progs: tests/cli: add a test case for "btrfs tune" subcommand

[Qu Wenruo]

The new test case would test all supported features of both set and
clear subcommands.

Also test the error handling of unknown features.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/cli-tests/018-btrfs-tune/test.sh | 40 ++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 tests/cli-tests/018-btrfs-tune/test.sh

diff --git a/tests/cli-tests/018-btrfs-tune/test.sh b/tests/cli-tests/018-btrfs-tune/test.sh
new file mode 100755
index 000000000000..3b2d1ebb3e59
--- /dev/null
+++ b/tests/cli-tests/018-btrfs-tune/test.sh
@@ -0,0 +1,40 @@
+#!/bin/bash
+# test all command line options of btrfstune
+
+source "$TEST_TOP/common" || exit
+
+check_prereq btrfstune
+
+setup_root_helper
+prepare_test_dev
+
+run_mayfail "$TOP/btrfstune" || true
+run_check "$TOP/btrfstune" --help
+
+run_mustfail "must not work on non-existent device" \
+	"$TOP/btrfstune" -r file-does-not-exist
+
+run_check_mkfs_test_dev -O ^extref
+run_check "$TOP/btrfs" tune set extref "$TEST_DEV"
+run_check "$TOP/btrfs" check "$TEST_DEV"
+
+run_check_mkfs_test_dev -O ^skinny-metadata
+run_check "$TOP/btrfs" tune set skinny-metadata "$TEST_DEV"
+run_check "$TOP/btrfs" check "$TEST_DEV"
+
+run_check_mkfs_test_dev -O ^no-holes
+run_check "$TOP/btrfs" tune set no-holes "$TEST_DEV"
+run_check "$TOP/btrfs" check "$TEST_DEV"
+
+run_check_mkfs_test_dev -O ^block-group-tree
+run_check "$TOP/btrfs" tune set block-group-tree "$TEST_DEV"
+run_check "$TOP/btrfs" check "$TEST_DEV"
+run_check "$TOP/btrfs" tune clear block-group-tree "$TEST_DEV"
+run_check "$TOP/btrfs" check "$TEST_DEV"
+
+run_check_mkfs_test_dev
+run_check "$TOP/btrfs" tune set -f seed "$TEST_DEV"
+run_check "$TOP/btrfs" tune clear seed "$TEST_DEV"
+
+run_mustfail "Unknown features" "$TOP/btrfs" tune set unknown-feature "$TEST_DEV"
+run_mustfail "Unknown features" "$TOP/btrfs" tune clear unknown-feature "$TEST_DEV"
-- 
2.42.0

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[Qu Wenruo]

Hi David,

Mind to give some feedback on this?
As this is the basis for all later "btrfs tune" subcommand.

Thanks,
Qu

On 2023/9/5 17:21, Qu Wenruo wrote:
> This is the first step to convert btrfstune functionality to "btrfs
> tune" subcommand group.
> 
> For now only binary features, aka set and clear, is supported,
> thus uuid and csum change is not yet implemented.
> (Both need their own subcommand groups other than set/clear groups)
> 
> And even for set/clear, there is some changes to btrfstune:
> 
> - Merge seed feature into set/clear
>    To enable seeding, just go "btrfs tune set seed <device>".
> 
> - All supported features can be checked by "list-all" feature
>    Please note that, "btrfs tune set list-all" and
>    "btrfs tune clear list-all" will have different output.
> 
>    The reason is some fundamental features like no-holes can not be
>    disabled.
> 
> 
> Qu Wenruo (7):
>    btrfs-progs: export btrfs_feature structure
>    btrfs-progs: cmds: add "btrfs tune set" subcommand group
>    btrfs-progs: cmds/tune: add set support for free-space-tree feature
>    btrfs-progs: cmds/tune: add set support for block-group-tree feature
>    btrfs-progs: cmds/tune: add set support for seeding device
>    btrfs-progs: cmds/tune: add "btrfs tune clear" subcommand
>    btrfs-progs: tests/cli: add a test case for "btrfs tune" subcommand
> 
>   Documentation/btrfs-tune.rst           |  47 +++
>   Documentation/btrfs.rst                |   5 +
>   Documentation/conf.py                  |   1 +
>   Documentation/man-index.rst            |   1 +
>   Makefile                               |   4 +-
>   btrfs.c                                |   1 +
>   cmds/commands.h                        |   1 +
>   cmds/tune.c                            | 448 +++++++++++++++++++++++++
>   common/fsfeatures.c                    |  53 ---
>   common/fsfeatures.h                    |  50 +++
>   tests/cli-tests/018-btrfs-tune/test.sh |  40 +++
>   11 files changed, 596 insertions(+), 55 deletions(-)
>   create mode 100644 Documentation/btrfs-tune.rst
>   create mode 100644 cmds/tune.c
>   create mode 100755 tests/cli-tests/018-btrfs-tune/test.sh
> 
> --
> 2.42.0
> 

Re: [PATCH 1/7] btrfs-progs: export btrfs_feature structure

[Anand Jain]

On 05/09/2023 15:51, Qu Wenruo wrote:
> For the incoming "btrfs tune" subcommand, we will have different
> features supported by that subcommand.
> 
> Instead of bloating the runtime and mkfs features, here we just export
> btrfs_feature, so each subcommand can have their own definition of
> supported features.
> 

Looks good.

> And since we're here, also add needed headers for future users of
> "fsfeatures.h".
> 
I don't see anything added, missed?

Thanks, Anand

> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   common/fsfeatures.c | 53 ---------------------------------------------
>   common/fsfeatures.h | 50 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 50 insertions(+), 53 deletions(-)
> 
> diff --git a/common/fsfeatures.c b/common/fsfeatures.c
> index 9ee392d3a8a6..f8eeea7695c1 100644
> --- a/common/fsfeatures.c
> +++ b/common/fsfeatures.c
> @@ -32,64 +32,11 @@
>   #include "common/sysfs-utils.h"
>   #include "common/messages.h"
>   
> -/*
> - * Insert a root item for temporary tree root
> - *
> - * Only used in make_btrfs_v2().
> - */
> -#define VERSION_TO_STRING3(name, a,b,c)				\
> -	.name ## _str = #a "." #b "." #c,			\
> -	.name ## _ver = KERNEL_VERSION(a,b,c)
> -#define VERSION_TO_STRING2(name, a,b)				\
> -	.name ## _str = #a "." #b,				\
> -	.name ## _ver = KERNEL_VERSION(a,b,0)
> -#define VERSION_NULL(name)					\
> -	.name ## _str = NULL,					\
> -	.name ## _ver = 0
> -
>   enum feature_source {
>   	FS_FEATURES,
>   	RUNTIME_FEATURES,
>   };
>   
> -/*
> - * Feature stability status and versions: compat <= safe <= default
> - */
> -struct btrfs_feature {
> -	const char *name;
> -
> -	/*
> -	 * At least one of the bit must be set in the following *_flag member.
> -	 *
> -	 * For features like list-all and quota which don't have any
> -	 * incompat/compat_ro bit set, it go to runtime_flag.
> -	 */
> -	u64 incompat_flag;
> -	u64 compat_ro_flag;
> -	u64 runtime_flag;
> -
> -	const char *sysfs_name;
> -	/*
> -	 * Compatibility with kernel of given version. Filesystem can be
> -	 * mounted.
> -	 */
> -	const char *compat_str;
> -	u32 compat_ver;
> -	/*
> -	 * Considered safe for use, but is not on by default, even if the
> -	 * kernel supports the feature.
> -	 */
> -	const char *safe_str;
> -	u32 safe_ver;
> -	/*
> -	 * Considered safe for use and will be turned on by default if
> -	 * supported by the running kernel.
> -	 */
> -	const char *default_str;
> -	u32 default_ver;
> -	const char *desc;
> -};
> -
>   static const struct btrfs_feature mkfs_features[] = {
>   	{
>   		.name		= "mixed-bg",
> diff --git a/common/fsfeatures.h b/common/fsfeatures.h
> index c4ab704862cd..c9fb489d2d79 100644
> --- a/common/fsfeatures.h
> +++ b/common/fsfeatures.h
> @@ -19,7 +19,9 @@
>   
>   #include "kerncompat.h"
>   #include <stdio.h>
> +#include <linux/version.h>
>   #include "kernel-lib/sizes.h"
> +#include "kernel-shared/uapi/btrfs.h"
>   
>   #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K
>   
> @@ -43,6 +45,54 @@ struct btrfs_mkfs_features {
>    */
>   #define BTRFS_FEATURE_STRING_BUF_SIZE		(160)
>   
> +#define VERSION_TO_STRING3(name, a,b,c)				\
> +	.name ## _str = #a "." #b "." #c,			\
> +	.name ## _ver = KERNEL_VERSION(a,b,c)
> +#define VERSION_TO_STRING2(name, a,b)				\
> +	.name ## _str = #a "." #b,				\
> +	.name ## _ver = KERNEL_VERSION(a,b,0)
> +#define VERSION_NULL(name)					\
> +	.name ## _str = NULL,					\
> +	.name ## _ver = 0
> +
> +/*
> + * Feature stability status and versions: compat <= safe <= default
> + */
> +struct btrfs_feature {
> +	const char *name;
> +
> +	/*
> +	 * At least one of the bit must be set in the following *_flag member.
> +	 *
> +	 * For features like list-all and quota which don't have any
> +	 * incompat/compat_ro bit set, it go to runtime_flag.
> +	 */
> +	u64 incompat_flag;
> +	u64 compat_ro_flag;
> +	u64 runtime_flag;
> +
> +	const char *sysfs_name;
> +	/*
> +	 * Compatibility with kernel of given version. Filesystem can be
> +	 * mounted.
> +	 */
> +	const char *compat_str;
> +	u32 compat_ver;
> +	/*
> +	 * Considered safe for use, but is not on by default, even if the
> +	 * kernel supports the feature.
> +	 */
> +	const char *safe_str;
> +	u32 safe_ver;
> +	/*
> +	 * Considered safe for use and will be turned on by default if
> +	 * supported by the running kernel.
> +	 */
> +	const char *default_str;
> +	u32 default_ver;
> +	const char *desc;
> +};
> +
>   static const struct btrfs_mkfs_features btrfs_mkfs_default_features = {
>   	.compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |
>   			   BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID,

Re: [PATCH 2/7] btrfs-progs: cmds: add "btrfs tune set" subcommand group

[Anand Jain]

On 05/09/2023 15:51, Qu Wenruo wrote:
> As the first step to convert btrfstune into "btrfs tune" subcommand
> group, this patch would add the following subcommand group:
> 
>   btrfs tune set <feature> [<device>]
> 
> For now the following features are supported:
> 
> - extref
> - skinny-metadata
> - no-holes
>    All those are simple super block flags toggle.
> 
> - list-all
>    This acts the same way as "mkfs.btrfs -O list-all", the difference is
>    it would only list the supported features.
>    (In the future, there will be "btrfs tune clear" subcommand, which
>     would support a different set of features).
> 

With this patchset, the syntax is structured as follows:


    $ btrfs tune --help
    usage: btrfs tune <command> <args>

       btrfs tune set <feature> [<device>]
           Set/enable specified feature for the unmounted filesystem
       btrfs tune clear <feature> [<device>]
           Clear/disable specified feature for the unmounted filesystem

    change various btrfs features



However, for consistency, I suggest the following syntax:

   set:
    $ btrfs tune <feature> /dev/sda

   clear:
    $ btrfs tune <feature> --clear /dev/sda

   list:
    $ btrfs tune --list-all

This syntax aligns with the:

    $ btrfs device scan --forget


Thanks, Anand


> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   Documentation/btrfs-tune.rst |  40 ++++++
>   Documentation/btrfs.rst      |   5 +
>   Documentation/conf.py        |   1 +
>   Documentation/man-index.rst  |   1 +
>   Makefile                     |   2 +-
>   btrfs.c                      |   1 +
>   cmds/commands.h              |   1 +
>   cmds/tune.c                  | 227 +++++++++++++++++++++++++++++++++++
>   8 files changed, 277 insertions(+), 1 deletion(-)
>   create mode 100644 Documentation/btrfs-tune.rst
>   create mode 100644 cmds/tune.c
> 
> diff --git a/Documentation/btrfs-tune.rst b/Documentation/btrfs-tune.rst
> new file mode 100644
> index 000000000000..827c92eadb72
> --- /dev/null
> +++ b/Documentation/btrfs-tune.rst
> @@ -0,0 +1,40 @@
> +btrfs-tune(8)
> +==================
> +
> +SYNOPSIS
> +--------
> +
> +**btrfs tune** <subcommand> [<args>]
> +
> +DESCRIPTION
> +-----------
> +
> +:command:`btrfs tune` is used to tweak various btrfs features on a
> +unmounted filesystem.
> +
> +SUBCOMMAND
> +-----------
> +
> +set <feature> [<device>]
> +        Set/enable a feature.
> +
> +        If *feature* is `list-all`, all supported features would be listed, and
> +	no *device* parameter is needed.
> +
> +EXIT STATUS
> +-----------
> +
> +**btrfs tune** returns a zero exit status if it succeeds. A non-zero value is
> +returned in case of failure.
> +
> +AVAILABILITY
> +------------
> +
> +**btrfs** is part of btrfs-progs.  Please refer to the documentation at
> +`https://btrfs.readthedocs.io <https://btrfs.readthedocs.io>`_.
> +
> +SEE ALSO
> +--------
> +
> +:doc:`mkfs.btrfs`,
> +``mount(8)``
> diff --git a/Documentation/btrfs.rst b/Documentation/btrfs.rst
> index e878f158aaa1..5aea0d1a208c 100644
> --- a/Documentation/btrfs.rst
> +++ b/Documentation/btrfs.rst
> @@ -134,6 +134,10 @@ subvolume
>   	Create/delete/list/manage btrfs subvolume.
>   	See :doc:`btrfs-subvolume` for details.
>   
> +tune
> +	Change various btrfs features.
> +	See :doc:`btrfs-tune` for details.
> +
>   .. _man-btrfs8-standalone-tools:
>   
>   STANDALONE TOOLS
> @@ -150,6 +154,7 @@ btrfs-convert
>           in-place conversion from ext2/3/4 filesystems to btrfs
>   btrfstune
>           tweak some filesystem properties on a unmounted filesystem
> +	(will be replaced by `btrfs-tune`)
>   btrfs-select-super
>           rescue tool to overwrite primary superblock from a spare copy
>   btrfs-find-root
> diff --git a/Documentation/conf.py b/Documentation/conf.py
> index 1025e10d7206..e0801bca4686 100644
> --- a/Documentation/conf.py
> +++ b/Documentation/conf.py
> @@ -66,6 +66,7 @@ man_pages = [
>       ('btrfs-check', 'btrfs-check', 'check or repair a btrfs filesystem', '', 8),
>       ('btrfs-balance', 'btrfs-balance', 'balance block groups on a btrfs filesystem', '', 8),
>       ('btrfs-subvolume', 'btrfs-subvolume', 'manage btrfs subvolumes', '', 8),
> +    ('btrfs-tune', 'btrfs-tune', 'tweak btrfs features', '', 8),
>       ('btrfs-map-logical', 'btrfs-map-logical', 'map btrfs logical extent to physical extent', '', 8),
>       ('btrfs', 'btrfs', 'a toolbox to manage btrfs filesystems', '', 8),
>       ('mkfs.btrfs', 'mkfs.btrfs', 'create a btrfs filesystem', '', 8),
> diff --git a/Documentation/man-index.rst b/Documentation/man-index.rst
> index 36d45d2903ea..5fcd4cbc4bee 100644
> --- a/Documentation/man-index.rst
> +++ b/Documentation/man-index.rst
> @@ -28,6 +28,7 @@ Manual pages
>      btrfs-select-super
>      btrfs-send
>      btrfs-subvolume
> +   btrfs-tune
>      btrfstune
>      fsck.btrfs
>      mkfs.btrfs
> diff --git a/Makefile b/Makefile
> index f4feb1fff8e1..9857daaa42ac 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -239,7 +239,7 @@ cmds_objects = cmds/subvolume.o cmds/subvolume-list.o \
>   	       cmds/rescue-super-recover.o \
>   	       cmds/property.o cmds/filesystem-usage.o cmds/inspect-dump-tree.o \
>   	       cmds/inspect-dump-super.o cmds/inspect-tree-stats.o cmds/filesystem-du.o \
> -	       cmds/reflink.o \
> +	       cmds/reflink.o cmds/tune.o \
>   	       mkfs/common.o check/mode-common.o check/mode-lowmem.o \
>   	       check/clear-cache.o
>   
> diff --git a/btrfs.c b/btrfs.c
> index 751f193ee2e0..c2dae0303ffe 100644
> --- a/btrfs.c
> +++ b/btrfs.c
> @@ -389,6 +389,7 @@ static const struct cmd_group btrfs_cmd_group = {
>   		&cmd_struct_scrub,
>   		&cmd_struct_send,
>   		&cmd_struct_subvolume,
> +		&cmd_struct_tune,
>   
>   		/* Help and version stay last */
>   		&cmd_struct_help,
> diff --git a/cmds/commands.h b/cmds/commands.h
> index 5ab7c881f634..aebacd718a7b 100644
> --- a/cmds/commands.h
> +++ b/cmds/commands.h
> @@ -151,5 +151,6 @@ DECLARE_COMMAND(qgroup);
>   DECLARE_COMMAND(replace);
>   DECLARE_COMMAND(restore);
>   DECLARE_COMMAND(rescue);
> +DECLARE_COMMAND(tune);
>   
>   #endif
> diff --git a/cmds/tune.c b/cmds/tune.c
> new file mode 100644
> index 000000000000..92c7b9f1502c
> --- /dev/null
> +++ b/cmds/tune.c
> @@ -0,0 +1,227 @@
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public
> + * License v2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public
> + * License along with this program; if not, write to the
> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
> + * Boston, MA 021110-1307, USA.
> + */
> +
> +#include <unistd.h>
> +#include "kerncompat.h"
> +#include "cmds/commands.h"
> +#include "common/help.h"
> +#include "common/fsfeatures.h"
> +#include "kernel-shared/messages.h"
> +#include "kernel-shared/disk-io.h"
> +#include "kernel-shared/transaction.h"
> +
> +static const char * const cmd_tune_set_usage[] = {
> +	"btrfs tune set <feature> [<device>]",
> +	"Set/enable specified feature for the unmounted filesystem",
> +	"",
> +	HELPINFO_INSERT_GLOBALS,
> +	HELPINFO_INSERT_VERBOSE,
> +	NULL,
> +};
> +
> +static const struct btrfs_feature set_features[] = {
> +	{
> +		.name		= "extref",
> +		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF,
> +		.sysfs_name	= "extended_iref",
> +		VERSION_TO_STRING2(compat, 3,7),
> +		VERSION_TO_STRING2(safe, 3,12),
> +		VERSION_TO_STRING2(default, 3,12),
> +		.desc		= "increased hardlink limit per file to 65536"
> +	}, {
> +		.name		= "skinny-metadata",
> +		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA,
> +		.sysfs_name	= "skinny_metadata",
> +		VERSION_TO_STRING2(compat, 3,10),
> +		VERSION_TO_STRING2(safe, 3,18),
> +		VERSION_TO_STRING2(default, 3,18),
> +		.desc		= "reduced-size metadata extent refs"
> +	}, {
> +		.name		= "no-holes",
> +		.incompat_flag	= BTRFS_FEATURE_INCOMPAT_NO_HOLES,
> +		.sysfs_name	= "no_holes",
> +		VERSION_TO_STRING2(compat, 3,14),
> +		VERSION_TO_STRING2(safe, 4,0),
> +		VERSION_TO_STRING2(default, 5,15),
> +		.desc		= "no explicit hole extents for files"
> +	},
> +	/* Keep this one last */
> +	{
> +		.name		= "list-all",
> +		.runtime_flag	= BTRFS_FEATURE_RUNTIME_LIST_ALL,
> +		.sysfs_name	= NULL,
> +		VERSION_NULL(compat),
> +		VERSION_NULL(safe),
> +		VERSION_NULL(default),
> +		.desc		= NULL
> +	}
> +};
> +
> +static void list_all_features(const char *prefix,
> +			      const struct btrfs_feature *features,
> +			      int nr_features)
> +{
> +	/* We should have at least one empty feature. */
> +	ASSERT(nr_features > 1);
> +
> +	printf("features available to %s:\n", prefix);
> +	for (int i = 0; i < nr_features - 1; i++) {
> +		const struct btrfs_feature *feat = features + i;
> +		const char *sep = "";
> +
> +		fprintf(stderr, "%-20s- %s (", feat->name, feat->desc);
> +		if (feat->compat_ver) {
> +			fprintf(stderr, "compat=%s", feat->compat_str);
> +			sep = ", ";
> +		}
> +		if (feat->safe_ver) {
> +			fprintf(stderr, "%ssafe=%s", sep, feat->safe_str);
> +			sep = ", ";
> +		}
> +		if (feat->default_ver)
> +			fprintf(stderr, "%sdefault=%s", sep, feat->default_str);
> +		fprintf(stderr, ")\n");
> +	}
> +}
> +
> +static int check_features(const char *name, const struct btrfs_feature *features,
> +			  int nr_features)
> +{
> +	bool found = false;
> +
> +	for (int i = 0; i < nr_features; i++) {
> +		const struct btrfs_feature *feat = features + i;
> +
> +		if (!strcmp(feat->name, name)) {
> +			found = true;
> +			break;
> +		}
> +	}
> +	if (found)
> +		return 0;
> +	return -EINVAL;
> +}
> +
> +static int set_super_incompat_flags(struct btrfs_fs_info *fs_info, u64 flags)
> +{
> +	struct btrfs_root *root = fs_info->tree_root;
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_super_block *disk_super;
> +	u64 super_flags;
> +	int ret;
> +
> +	disk_super = fs_info->super_copy;
> +	super_flags = btrfs_super_incompat_flags(disk_super);
> +	super_flags |= flags;
> +	trans = btrfs_start_transaction(root, 1);
> +	BUG_ON(IS_ERR(trans));
> +	btrfs_set_super_incompat_flags(disk_super, super_flags);
> +	ret = btrfs_commit_transaction(trans, root);
> +
> +	return ret;
> +}
> +
> +static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char **argv)
> +{
> +	struct btrfs_fs_info *fs_info;
> +	struct open_ctree_args oca = { 0 };
> +	char *feature;
> +	char *path;
> +	int ret = 0;
> +
> +	optind = 0;
> +	while (1) {
> +		int c = getopt(argc, argv, "");
> +		if (c < 0)
> +			break;
> +
> +		switch (c) {
> +		default:
> +			usage_unknown_option(cmd, argv);
> +		}
> +	}
> +
> +	if (check_argc_min(argc - optind, 1))
> +		return 1;
> +
> +	feature = argv[optind];
> +
> +	if (check_features(feature, set_features, ARRAY_SIZE(set_features))) {
> +		error("Unknown feature to set: %s", feature);
> +		return 1;
> +	}
> +	if (!strcmp(feature, "list-all")) {
> +		list_all_features("set", set_features, ARRAY_SIZE(set_features));
> +		return 0;
> +	}
> +
> +	if (check_argc_exact(argc - optind, 2))
> +		return 1;
> +
> +	path = argv[optind + 1];
> +	oca.flags = OPEN_CTREE_WRITES;
> +	oca.filename = path;
> +	fs_info = open_ctree_fs_info(&oca);
> +	if (!fs_info) {
> +		error("failed to open btrfs");
> +		ret = -EIO;
> +		goto out;
> +	}
> +	/*
> +	 * For those 3 features, we only need to update the superblock to add
> +	 * the new feature flags.
> +	 */
> +	if (!strcmp(feature, "extref") ||
> +	    !strcmp(feature, "skinny-metadata") ||
> +	    !strcmp(feature, "no-holes")) {
> +		u64 incompat_flags = 0;
> +
> +		if (!strcmp(feature, "extref"))
> +			incompat_flags |= BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF;
> +		if (!strcmp(feature, "skinny-metadata"))
> +			incompat_flags |= BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA;
> +		if (!strcmp(feature, "no-holes"))
> +			incompat_flags |= BTRFS_FEATURE_INCOMPAT_NO_HOLES;
> +		ret = set_super_incompat_flags(fs_info, incompat_flags);
> +		if (ret < 0) {
> +			errno = -ret;
> +			error("failed to set feature '%s': %m", feature);
> +		}
> +		goto out;
> +	}
> +
> +out:
> +	if (fs_info)
> +		close_ctree_fs_info(fs_info);
> +	return !!ret;
> +}
> +
> +static DEFINE_SIMPLE_COMMAND(tune_set, "set");
> +
> +static const char * const tune_cmd_group_usage[] = {
> +	"btrfs tune <command> <args>",
> +	NULL,
> +};
> +
> +static const char tune_cmd_group_info[] = "change various btrfs features";
> +
> +static const struct cmd_group tune_cmd_group = {
> +	tune_cmd_group_usage, tune_cmd_group_info, {
> +		&cmd_struct_tune_set,
> +		NULL
> +	}
> +};
> +DEFINE_GROUP_COMMAND_TOKEN(tune);

Re: [PATCH 2/7] btrfs-progs: cmds: add "btrfs tune set" subcommand group

[Qu Wenruo]

On 2023/9/21 10:23, Anand Jain wrote:
> On 05/09/2023 15:51, Qu Wenruo wrote:
>> As the first step to convert btrfstune into "btrfs tune" subcommand
>> group, this patch would add the following subcommand group:
>>
>>   btrfs tune set <feature> [<device>]
>>
>> For now the following features are supported:
>>
>> - extref
>> - skinny-metadata
>> - no-holes
>>    All those are simple super block flags toggle.
>>
>> - list-all
>>    This acts the same way as "mkfs.btrfs -O list-all", the difference is
>>    it would only list the supported features.
>>    (In the future, there will be "btrfs tune clear" subcommand, which
>>     would support a different set of features).
>>
> 
> With this patchset, the syntax is structured as follows:
> 
> 
>     $ btrfs tune --help
>     usage: btrfs tune <command> <args>
> 
>        btrfs tune set <feature> [<device>]
>            Set/enable specified feature for the unmounted filesystem
>        btrfs tune clear <feature> [<device>]
>            Clear/disable specified feature for the unmounted filesystem
> 
>     change various btrfs features
> 
> 
> 
> However, for consistency, I suggest the following syntax:
> 
>    set:
>     $ btrfs tune <feature> /dev/sda
> 
>    clear:
>     $ btrfs tune <feature> --clear /dev/sda
> 
>    list:
>     $ btrfs tune --list-all

This can be confusing instead.

Remember set and clear have different supported feature set.
E.g. we can not disable no-holes/extref features.

Thus here we want set/clear as subcommands, not the features name itself.

Furthermore, using <feature> as the subcommand wil later conflicts with 
other subcommand like "change-csum" and "change-fsid".

> 
> This syntax aligns with the:
> 
>     $ btrfs device scan --forget

In fact, the current "set/clear" follows the existing behavior, it's 
more clear if you scan a single device:

   btrfs device scan <device_name>

As for btrfs tune it goes:

   btrfs tune set <feature_name> <mnt>

We should put the variable parameter last, not between some fixed 
subcommand.

Thanks,
Qu

> 
> 
> Thanks, Anand
> 
> 
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   Documentation/btrfs-tune.rst |  40 ++++++
>>   Documentation/btrfs.rst      |   5 +
>>   Documentation/conf.py        |   1 +
>>   Documentation/man-index.rst  |   1 +
>>   Makefile                     |   2 +-
>>   btrfs.c                      |   1 +
>>   cmds/commands.h              |   1 +
>>   cmds/tune.c                  | 227 +++++++++++++++++++++++++++++++++++
>>   8 files changed, 277 insertions(+), 1 deletion(-)
>>   create mode 100644 Documentation/btrfs-tune.rst
>>   create mode 100644 cmds/tune.c
>>
>> diff --git a/Documentation/btrfs-tune.rst b/Documentation/btrfs-tune.rst
>> new file mode 100644
>> index 000000000000..827c92eadb72
>> --- /dev/null
>> +++ b/Documentation/btrfs-tune.rst
>> @@ -0,0 +1,40 @@
>> +btrfs-tune(8)
>> +==================
>> +
>> +SYNOPSIS
>> +--------
>> +
>> +**btrfs tune** <subcommand> [<args>]
>> +
>> +DESCRIPTION
>> +-----------
>> +
>> +:command:`btrfs tune` is used to tweak various btrfs features on a
>> +unmounted filesystem.
>> +
>> +SUBCOMMAND
>> +-----------
>> +
>> +set <feature> [<device>]
>> +        Set/enable a feature.
>> +
>> +        If *feature* is `list-all`, all supported features would be 
>> listed, and
>> +    no *device* parameter is needed.
>> +
>> +EXIT STATUS
>> +-----------
>> +
>> +**btrfs tune** returns a zero exit status if it succeeds. A non-zero 
>> value is
>> +returned in case of failure.
>> +
>> +AVAILABILITY
>> +------------
>> +
>> +**btrfs** is part of btrfs-progs.  Please refer to the documentation at
>> +`https://btrfs.readthedocs.io <https://btrfs.readthedocs.io>`_.
>> +
>> +SEE ALSO
>> +--------
>> +
>> +:doc:`mkfs.btrfs`,
>> +``mount(8)``
>> diff --git a/Documentation/btrfs.rst b/Documentation/btrfs.rst
>> index e878f158aaa1..5aea0d1a208c 100644
>> --- a/Documentation/btrfs.rst
>> +++ b/Documentation/btrfs.rst
>> @@ -134,6 +134,10 @@ subvolume
>>       Create/delete/list/manage btrfs subvolume.
>>       See :doc:`btrfs-subvolume` for details.
>> +tune
>> +    Change various btrfs features.
>> +    See :doc:`btrfs-tune` for details.
>> +
>>   .. _man-btrfs8-standalone-tools:
>>   STANDALONE TOOLS
>> @@ -150,6 +154,7 @@ btrfs-convert
>>           in-place conversion from ext2/3/4 filesystems to btrfs
>>   btrfstune
>>           tweak some filesystem properties on a unmounted filesystem
>> +    (will be replaced by `btrfs-tune`)
>>   btrfs-select-super
>>           rescue tool to overwrite primary superblock from a spare copy
>>   btrfs-find-root
>> diff --git a/Documentation/conf.py b/Documentation/conf.py
>> index 1025e10d7206..e0801bca4686 100644
>> --- a/Documentation/conf.py
>> +++ b/Documentation/conf.py
>> @@ -66,6 +66,7 @@ man_pages = [
>>       ('btrfs-check', 'btrfs-check', 'check or repair a btrfs 
>> filesystem', '', 8),
>>       ('btrfs-balance', 'btrfs-balance', 'balance block groups on a 
>> btrfs filesystem', '', 8),
>>       ('btrfs-subvolume', 'btrfs-subvolume', 'manage btrfs 
>> subvolumes', '', 8),
>> +    ('btrfs-tune', 'btrfs-tune', 'tweak btrfs features', '', 8),
>>       ('btrfs-map-logical', 'btrfs-map-logical', 'map btrfs logical 
>> extent to physical extent', '', 8),
>>       ('btrfs', 'btrfs', 'a toolbox to manage btrfs filesystems', '', 8),
>>       ('mkfs.btrfs', 'mkfs.btrfs', 'create a btrfs filesystem', '', 8),
>> diff --git a/Documentation/man-index.rst b/Documentation/man-index.rst
>> index 36d45d2903ea..5fcd4cbc4bee 100644
>> --- a/Documentation/man-index.rst
>> +++ b/Documentation/man-index.rst
>> @@ -28,6 +28,7 @@ Manual pages
>>      btrfs-select-super
>>      btrfs-send
>>      btrfs-subvolume
>> +   btrfs-tune
>>      btrfstune
>>      fsck.btrfs
>>      mkfs.btrfs
>> diff --git a/Makefile b/Makefile
>> index f4feb1fff8e1..9857daaa42ac 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -239,7 +239,7 @@ cmds_objects = cmds/subvolume.o 
>> cmds/subvolume-list.o \
>>              cmds/rescue-super-recover.o \
>>              cmds/property.o cmds/filesystem-usage.o 
>> cmds/inspect-dump-tree.o \
>>              cmds/inspect-dump-super.o cmds/inspect-tree-stats.o 
>> cmds/filesystem-du.o \
>> -           cmds/reflink.o \
>> +           cmds/reflink.o cmds/tune.o \
>>              mkfs/common.o check/mode-common.o check/mode-lowmem.o \
>>              check/clear-cache.o
>> diff --git a/btrfs.c b/btrfs.c
>> index 751f193ee2e0..c2dae0303ffe 100644
>> --- a/btrfs.c
>> +++ b/btrfs.c
>> @@ -389,6 +389,7 @@ static const struct cmd_group btrfs_cmd_group = {
>>           &cmd_struct_scrub,
>>           &cmd_struct_send,
>>           &cmd_struct_subvolume,
>> +        &cmd_struct_tune,
>>           /* Help and version stay last */
>>           &cmd_struct_help,
>> diff --git a/cmds/commands.h b/cmds/commands.h
>> index 5ab7c881f634..aebacd718a7b 100644
>> --- a/cmds/commands.h
>> +++ b/cmds/commands.h
>> @@ -151,5 +151,6 @@ DECLARE_COMMAND(qgroup);
>>   DECLARE_COMMAND(replace);
>>   DECLARE_COMMAND(restore);
>>   DECLARE_COMMAND(rescue);
>> +DECLARE_COMMAND(tune);
>>   #endif
>> diff --git a/cmds/tune.c b/cmds/tune.c
>> new file mode 100644
>> index 000000000000..92c7b9f1502c
>> --- /dev/null
>> +++ b/cmds/tune.c
>> @@ -0,0 +1,227 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public
>> + * License v2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public
>> + * License along with this program; if not, write to the
>> + * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>> + * Boston, MA 021110-1307, USA.
>> + */
>> +
>> +#include <unistd.h>
>> +#include "kerncompat.h"
>> +#include "cmds/commands.h"
>> +#include "common/help.h"
>> +#include "common/fsfeatures.h"
>> +#include "kernel-shared/messages.h"
>> +#include "kernel-shared/disk-io.h"
>> +#include "kernel-shared/transaction.h"
>> +
>> +static const char * const cmd_tune_set_usage[] = {
>> +    "btrfs tune set <feature> [<device>]",
>> +    "Set/enable specified feature for the unmounted filesystem",
>> +    "",
>> +    HELPINFO_INSERT_GLOBALS,
>> +    HELPINFO_INSERT_VERBOSE,
>> +    NULL,
>> +};
>> +
>> +static const struct btrfs_feature set_features[] = {
>> +    {
>> +        .name        = "extref",
>> +        .incompat_flag    = BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF,
>> +        .sysfs_name    = "extended_iref",
>> +        VERSION_TO_STRING2(compat, 3,7),
>> +        VERSION_TO_STRING2(safe, 3,12),
>> +        VERSION_TO_STRING2(default, 3,12),
>> +        .desc        = "increased hardlink limit per file to 65536"
>> +    }, {
>> +        .name        = "skinny-metadata",
>> +        .incompat_flag    = BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA,
>> +        .sysfs_name    = "skinny_metadata",
>> +        VERSION_TO_STRING2(compat, 3,10),
>> +        VERSION_TO_STRING2(safe, 3,18),
>> +        VERSION_TO_STRING2(default, 3,18),
>> +        .desc        = "reduced-size metadata extent refs"
>> +    }, {
>> +        .name        = "no-holes",
>> +        .incompat_flag    = BTRFS_FEATURE_INCOMPAT_NO_HOLES,
>> +        .sysfs_name    = "no_holes",
>> +        VERSION_TO_STRING2(compat, 3,14),
>> +        VERSION_TO_STRING2(safe, 4,0),
>> +        VERSION_TO_STRING2(default, 5,15),
>> +        .desc        = "no explicit hole extents for files"
>> +    },
>> +    /* Keep this one last */
>> +    {
>> +        .name        = "list-all",
>> +        .runtime_flag    = BTRFS_FEATURE_RUNTIME_LIST_ALL,
>> +        .sysfs_name    = NULL,
>> +        VERSION_NULL(compat),
>> +        VERSION_NULL(safe),
>> +        VERSION_NULL(default),
>> +        .desc        = NULL
>> +    }
>> +};
>> +
>> +static void list_all_features(const char *prefix,
>> +                  const struct btrfs_feature *features,
>> +                  int nr_features)
>> +{
>> +    /* We should have at least one empty feature. */
>> +    ASSERT(nr_features > 1);
>> +
>> +    printf("features available to %s:\n", prefix);
>> +    for (int i = 0; i < nr_features - 1; i++) {
>> +        const struct btrfs_feature *feat = features + i;
>> +        const char *sep = "";
>> +
>> +        fprintf(stderr, "%-20s- %s (", feat->name, feat->desc);
>> +        if (feat->compat_ver) {
>> +            fprintf(stderr, "compat=%s", feat->compat_str);
>> +            sep = ", ";
>> +        }
>> +        if (feat->safe_ver) {
>> +            fprintf(stderr, "%ssafe=%s", sep, feat->safe_str);
>> +            sep = ", ";
>> +        }
>> +        if (feat->default_ver)
>> +            fprintf(stderr, "%sdefault=%s", sep, feat->default_str);
>> +        fprintf(stderr, ")\n");
>> +    }
>> +}
>> +
>> +static int check_features(const char *name, const struct 
>> btrfs_feature *features,
>> +              int nr_features)
>> +{
>> +    bool found = false;
>> +
>> +    for (int i = 0; i < nr_features; i++) {
>> +        const struct btrfs_feature *feat = features + i;
>> +
>> +        if (!strcmp(feat->name, name)) {
>> +            found = true;
>> +            break;
>> +        }
>> +    }
>> +    if (found)
>> +        return 0;
>> +    return -EINVAL;
>> +}
>> +
>> +static int set_super_incompat_flags(struct btrfs_fs_info *fs_info, 
>> u64 flags)
>> +{
>> +    struct btrfs_root *root = fs_info->tree_root;
>> +    struct btrfs_trans_handle *trans;
>> +    struct btrfs_super_block *disk_super;
>> +    u64 super_flags;
>> +    int ret;
>> +
>> +    disk_super = fs_info->super_copy;
>> +    super_flags = btrfs_super_incompat_flags(disk_super);
>> +    super_flags |= flags;
>> +    trans = btrfs_start_transaction(root, 1);
>> +    BUG_ON(IS_ERR(trans));
>> +    btrfs_set_super_incompat_flags(disk_super, super_flags);
>> +    ret = btrfs_commit_transaction(trans, root);
>> +
>> +    return ret;
>> +}
>> +
>> +static int cmd_tune_set(const struct cmd_struct *cmd, int argc, char 
>> **argv)
>> +{
>> +    struct btrfs_fs_info *fs_info;
>> +    struct open_ctree_args oca = { 0 };
>> +    char *feature;
>> +    char *path;
>> +    int ret = 0;
>> +
>> +    optind = 0;
>> +    while (1) {
>> +        int c = getopt(argc, argv, "");
>> +        if (c < 0)
>> +            break;
>> +
>> +        switch (c) {
>> +        default:
>> +            usage_unknown_option(cmd, argv);
>> +        }
>> +    }
>> +
>> +    if (check_argc_min(argc - optind, 1))
>> +        return 1;
>> +
>> +    feature = argv[optind];
>> +
>> +    if (check_features(feature, set_features, 
>> ARRAY_SIZE(set_features))) {
>> +        error("Unknown feature to set: %s", feature);
>> +        return 1;
>> +    }
>> +    if (!strcmp(feature, "list-all")) {
>> +        list_all_features("set", set_features, 
>> ARRAY_SIZE(set_features));
>> +        return 0;
>> +    }
>> +
>> +    if (check_argc_exact(argc - optind, 2))
>> +        return 1;
>> +
>> +    path = argv[optind + 1];
>> +    oca.flags = OPEN_CTREE_WRITES;
>> +    oca.filename = path;
>> +    fs_info = open_ctree_fs_info(&oca);
>> +    if (!fs_info) {
>> +        error("failed to open btrfs");
>> +        ret = -EIO;
>> +        goto out;
>> +    }
>> +    /*
>> +     * For those 3 features, we only need to update the superblock to 
>> add
>> +     * the new feature flags.
>> +     */
>> +    if (!strcmp(feature, "extref") ||
>> +        !strcmp(feature, "skinny-metadata") ||
>> +        !strcmp(feature, "no-holes")) {
>> +        u64 incompat_flags = 0;
>> +
>> +        if (!strcmp(feature, "extref"))
>> +            incompat_flags |= BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF;
>> +        if (!strcmp(feature, "skinny-metadata"))
>> +            incompat_flags |= BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA;
>> +        if (!strcmp(feature, "no-holes"))
>> +            incompat_flags |= BTRFS_FEATURE_INCOMPAT_NO_HOLES;
>> +        ret = set_super_incompat_flags(fs_info, incompat_flags);
>> +        if (ret < 0) {
>> +            errno = -ret;
>> +            error("failed to set feature '%s': %m", feature);
>> +        }
>> +        goto out;
>> +    }
>> +
>> +out:
>> +    if (fs_info)
>> +        close_ctree_fs_info(fs_info);
>> +    return !!ret;
>> +}
>> +
>> +static DEFINE_SIMPLE_COMMAND(tune_set, "set");
>> +
>> +static const char * const tune_cmd_group_usage[] = {
>> +    "btrfs tune <command> <args>",
>> +    NULL,
>> +};
>> +
>> +static const char tune_cmd_group_info[] = "change various btrfs 
>> features";
>> +
>> +static const struct cmd_group tune_cmd_group = {
>> +    tune_cmd_group_usage, tune_cmd_group_info, {
>> +        &cmd_struct_tune_set,
>> +        NULL
>> +    }
>> +};
>> +DEFINE_GROUP_COMMAND_TOKEN(tune);
> 

Re: [PATCH 1/7] btrfs-progs: export btrfs_feature structure

[Qu Wenruo]

On 2023/9/21 10:02, Anand Jain wrote:
> On 05/09/2023 15:51, Qu Wenruo wrote:
>> For the incoming "btrfs tune" subcommand, we will have different
>> features supported by that subcommand.
>>
>> Instead of bloating the runtime and mkfs features, here we just export
>> btrfs_feature, so each subcommand can have their own definition of
>> supported features.
>>
>
> Looks good.
>
>> And since we're here, also add needed headers for future users of
>> "fsfeatures.h".
>>
> I don't see anything added, missed?

My bad, the line is not clear enough.

There are two added #include lines inside "fsfeatures.h", as during
development I found if we just include "fsfeatures.h" by itself, there
would be some missing definitions.

Thus we have the following lines:

--- a/common/fsfeatures.h
+++ b/common/fsfeatures.h
@@ -19,7 +19,9 @@

  #include "kerncompat.h"
  #include <stdio.h>
+#include <linux/version.h>
  #include "kernel-lib/sizes.h"
+#include "kernel-shared/uapi/btrfs.h"

Thanks,
Qu
>
> Thanks, Anand
>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   common/fsfeatures.c | 53 ---------------------------------------------
>>   common/fsfeatures.h | 50 ++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 50 insertions(+), 53 deletions(-)
>>
>> diff --git a/common/fsfeatures.c b/common/fsfeatures.c
>> index 9ee392d3a8a6..f8eeea7695c1 100644
>> --- a/common/fsfeatures.c
>> +++ b/common/fsfeatures.c
>> @@ -32,64 +32,11 @@
>>   #include "common/sysfs-utils.h"
>>   #include "common/messages.h"
>> -/*
>> - * Insert a root item for temporary tree root
>> - *
>> - * Only used in make_btrfs_v2().
>> - */
>> -#define VERSION_TO_STRING3(name, a,b,c)                \
>> -    .name ## _str = #a "." #b "." #c,            \
>> -    .name ## _ver = KERNEL_VERSION(a,b,c)
>> -#define VERSION_TO_STRING2(name, a,b)                \
>> -    .name ## _str = #a "." #b,                \
>> -    .name ## _ver = KERNEL_VERSION(a,b,0)
>> -#define VERSION_NULL(name)                    \
>> -    .name ## _str = NULL,                    \
>> -    .name ## _ver = 0
>> -
>>   enum feature_source {
>>       FS_FEATURES,
>>       RUNTIME_FEATURES,
>>   };
>> -/*
>> - * Feature stability status and versions: compat <= safe <= default
>> - */
>> -struct btrfs_feature {
>> -    const char *name;
>> -
>> -    /*
>> -     * At least one of the bit must be set in the following *_flag
>> member.
>> -     *
>> -     * For features like list-all and quota which don't have any
>> -     * incompat/compat_ro bit set, it go to runtime_flag.
>> -     */
>> -    u64 incompat_flag;
>> -    u64 compat_ro_flag;
>> -    u64 runtime_flag;
>> -
>> -    const char *sysfs_name;
>> -    /*
>> -     * Compatibility with kernel of given version. Filesystem can be
>> -     * mounted.
>> -     */
>> -    const char *compat_str;
>> -    u32 compat_ver;
>> -    /*
>> -     * Considered safe for use, but is not on by default, even if the
>> -     * kernel supports the feature.
>> -     */
>> -    const char *safe_str;
>> -    u32 safe_ver;
>> -    /*
>> -     * Considered safe for use and will be turned on by default if
>> -     * supported by the running kernel.
>> -     */
>> -    const char *default_str;
>> -    u32 default_ver;
>> -    const char *desc;
>> -};
>> -
>>   static const struct btrfs_feature mkfs_features[] = {
>>       {
>>           .name        = "mixed-bg",
>> diff --git a/common/fsfeatures.h b/common/fsfeatures.h
>> index c4ab704862cd..c9fb489d2d79 100644
>> --- a/common/fsfeatures.h
>> +++ b/common/fsfeatures.h
>> @@ -19,7 +19,9 @@
>>   #include "kerncompat.h"
>>   #include <stdio.h>
>> +#include <linux/version.h>
>>   #include "kernel-lib/sizes.h"
>> +#include "kernel-shared/uapi/btrfs.h"
>>   #define BTRFS_MKFS_DEFAULT_NODE_SIZE SZ_16K
>> @@ -43,6 +45,54 @@ struct btrfs_mkfs_features {
>>    */
>>   #define BTRFS_FEATURE_STRING_BUF_SIZE        (160)
>> +#define VERSION_TO_STRING3(name, a,b,c)                \
>> +    .name ## _str = #a "." #b "." #c,            \
>> +    .name ## _ver = KERNEL_VERSION(a,b,c)
>> +#define VERSION_TO_STRING2(name, a,b)                \
>> +    .name ## _str = #a "." #b,                \
>> +    .name ## _ver = KERNEL_VERSION(a,b,0)
>> +#define VERSION_NULL(name)                    \
>> +    .name ## _str = NULL,                    \
>> +    .name ## _ver = 0
>> +
>> +/*
>> + * Feature stability status and versions: compat <= safe <= default
>> + */
>> +struct btrfs_feature {
>> +    const char *name;
>> +
>> +    /*
>> +     * At least one of the bit must be set in the following *_flag
>> member.
>> +     *
>> +     * For features like list-all and quota which don't have any
>> +     * incompat/compat_ro bit set, it go to runtime_flag.
>> +     */
>> +    u64 incompat_flag;
>> +    u64 compat_ro_flag;
>> +    u64 runtime_flag;
>> +
>> +    const char *sysfs_name;
>> +    /*
>> +     * Compatibility with kernel of given version. Filesystem can be
>> +     * mounted.
>> +     */
>> +    const char *compat_str;
>> +    u32 compat_ver;
>> +    /*
>> +     * Considered safe for use, but is not on by default, even if the
>> +     * kernel supports the feature.
>> +     */
>> +    const char *safe_str;
>> +    u32 safe_ver;
>> +    /*
>> +     * Considered safe for use and will be turned on by default if
>> +     * supported by the running kernel.
>> +     */
>> +    const char *default_str;
>> +    u32 default_ver;
>> +    const char *desc;
>> +};
>> +
>>   static const struct btrfs_mkfs_features btrfs_mkfs_default_features = {
>>       .compat_ro_flags = BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE |
>>                  BTRFS_FEATURE_COMPAT_RO_FREE_SPACE_TREE_VALID,
>

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[waxhead]

Qu Wenruo wrote:
> This is the first step to convert btrfstune functionality to "btrfs
> tune" subcommand group.
> 
> For now only binary features, aka set and clear, is supported,
> thus uuid and csum change is not yet implemented.
> (Both need their own subcommand groups other than set/clear groups)
> 
> And even for set/clear, there is some changes to btrfstune:
> 
> - Merge seed feature into set/clear
>    To enable seeding, just go "btrfs tune set seed <device>".
> 
> - All supported features can be checked by "list-all" feature
>    Please note that, "btrfs tune set list-all" and
>    "btrfs tune clear list-all" will have different output.
> 
As just a regular BTRFS user I would like to please ask you to consider 
having functions that may have consequences e.g. "dangerous" functions 
disabled until enabled.

In other words much like you need to arm a missile or flip a safety 
cover on a toggle switch

So if btrfs on every mount set a bit that disabled access to dangerous 
functions (such as the tune functionality you would have to explicitly 
enable it to be able to do scary stuff).

btrfs tune enable
btrfs tune whatever_needs_tuning=123
btrfs tune disable (manually disable scary functions)

And yes, I am aware that root users have powers to do scary stuff, but 
sometimes even root users need to be protected against free will.

Please consider something like the above...

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[Qu Wenruo]

On 2023/9/22 08:03, waxhead wrote:
> Qu Wenruo wrote:
>> This is the first step to convert btrfstune functionality to "btrfs
>> tune" subcommand group.
>>
>> For now only binary features, aka set and clear, is supported,
>> thus uuid and csum change is not yet implemented.
>> (Both need their own subcommand groups other than set/clear groups)
>>
>> And even for set/clear, there is some changes to btrfstune:
>>
>> - Merge seed feature into set/clear
>>    To enable seeding, just go "btrfs tune set seed <device>".
>>
>> - All supported features can be checked by "list-all" feature
>>    Please note that, "btrfs tune set list-all" and
>>    "btrfs tune clear list-all" will have different output.
>>
> As just a regular BTRFS user I would like to please ask you to consider 
> having functions that may have consequences e.g. "dangerous" functions 
> disabled until enabled.
> 
> In other words much like you need to arm a missile or flip a safety 
> cover on a toggle switch
> 
> So if btrfs on every mount set a bit that disabled access to dangerous 
> functions (such as the tune functionality you would have to explicitly 
> enable it to be able to do scary stuff).
> 
> btrfs tune enable
> btrfs tune whatever_needs_tuning=123
> btrfs tune disable (manually disable scary functions)

We hide unsafe features behind the experimental flag of btrfs-progs, 
which is determined at compiling time.

Furthermore, all those features can be set/clear in "btrfs tune" must be 
done offline (unmounted).

Thus I think there is not much I can do to further improve the situation.

Thanks,
Qu

> 
> And yes, I am aware that root users have powers to do scary stuff, but 
> sometimes even root users need to be protected against free will.
> 
> Please consider something like the above...

Re: [PATCH 2/7] btrfs-progs: cmds: add "btrfs tune set" subcommand group

[David Sterba]

On Thu, Sep 21, 2023 at 08:53:11AM +0800, Anand Jain wrote:
> On 05/09/2023 15:51, Qu Wenruo wrote:
> > As the first step to convert btrfstune into "btrfs tune" subcommand
> > group, this patch would add the following subcommand group:
> > 
> >   btrfs tune set <feature> [<device>]
> > 
> > For now the following features are supported:
> > 
> > - extref
> > - skinny-metadata
> > - no-holes
> >    All those are simple super block flags toggle.
> > 
> > - list-all
> >    This acts the same way as "mkfs.btrfs -O list-all", the difference is
> >    it would only list the supported features.
> >    (In the future, there will be "btrfs tune clear" subcommand, which
> >     would support a different set of features).
> > 
> 
> With this patchset, the syntax is structured as follows:
> 
> 
>     $ btrfs tune --help
>     usage: btrfs tune <command> <args>
> 
>        btrfs tune set <feature> [<device>]
>            Set/enable specified feature for the unmounted filesystem
>        btrfs tune clear <feature> [<device>]
>            Clear/disable specified feature for the unmounted filesystem
> 
>     change various btrfs features
> 
> 
> 
> However, for consistency, I suggest the following syntax:

Consistency with what? If it's with btrfstune then no, the reason why
the command is split into subcommands is because the command line
parameters became unwieldy and what you suggest below copies that.

> 
>    set:
>     $ btrfs tune <feature> /dev/sda
> 
>    clear:
>     $ btrfs tune <feature> --clear /dev/sda
> 
>    list:
>     $ btrfs tune --list-all
> 
> This syntax aligns with the:
> 
>     $ btrfs device scan --forget

The difference is that scan won't be extended much if ever. The 'tune'
interface grows over time so we need properly structure that so we don't
end in command line option mess.

As an anti-pattern I can mention the 'mdadm' utility, that comes from
times when the commands were not without the initial "--" like we know
from git etc, so the main commands like assemble are specified by -A or
--assemble. What I find confusing is that it's just another option but
means a main action. The command groups logically and visually
encapsulate one action and can be possibly fine tuned by parameters
without affecting other commands.

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[David Sterba]

On Fri, Sep 22, 2023 at 12:33:04AM +0200, waxhead wrote:
> Qu Wenruo wrote:
> > This is the first step to convert btrfstune functionality to "btrfs
> > tune" subcommand group.
> > 
> > For now only binary features, aka set and clear, is supported,
> > thus uuid and csum change is not yet implemented.
> > (Both need their own subcommand groups other than set/clear groups)
> > 
> > And even for set/clear, there is some changes to btrfstune:
> > 
> > - Merge seed feature into set/clear
> >    To enable seeding, just go "btrfs tune set seed <device>".
> > 
> > - All supported features can be checked by "list-all" feature
> >    Please note that, "btrfs tune set list-all" and
> >    "btrfs tune clear list-all" will have different output.
> > 
> As just a regular BTRFS user I would like to please ask you to consider 
> having functions that may have consequences e.g. "dangerous" functions 
> disabled until enabled.
> 
> In other words much like you need to arm a missile or flip a safety 
> cover on a toggle switch
> 
> So if btrfs on every mount set a bit that disabled access to dangerous 
> functions (such as the tune functionality you would have to explicitly 
> enable it to be able to do scary stuff).
> 
> btrfs tune enable
> btrfs tune whatever_needs_tuning=123
> btrfs tune disable (manually disable scary functions)
> 
> And yes, I am aware that root users have powers to do scary stuff, but 
> sometimes even root users need to be protected against free will.
> 
> Please consider something like the above...

We can print a warning, add a delay, require a magic parameter. What you
suggest above with the enable/disable commands would require to store
the state somewhere so the next call 'tune whatever_needs_tuning' would
work (but not another such command run from another terminal).

Not all tune actions are dangerous or irreversible but I agree that
there's a lot of potential for damage in case someobody does not read
the documentation or understand the consequences. The actions are meant
to be one-time and with long term effects, like converting to new
structures. Comparable to mkfs, there are checks against accidental
overwrite of an existing filesystem, so at minimum we might require
--force for each command.

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[David Sterba]

On Tue, Sep 05, 2023 at 03:51:42PM +0800, Qu Wenruo wrote:
> This is the first step to convert btrfstune functionality to "btrfs
> tune" subcommand group.
> 
> For now only binary features, aka set and clear, is supported,
> thus uuid and csum change is not yet implemented.
> (Both need their own subcommand groups other than set/clear groups)

There are the easy on/off features that can be added first but you also
added the block-group-tree _conversion_. That's a different category and
IMHO requires a separate command so we can add options to check or reset
the unfinished state. And we also have the the conversion 'to' and
'from'.

My current list of 1st level command groups is:

- enable - for the simple on/off features (on)
- disable - dtto, (off)
- convert-to - for the BGT, checksum
- convert-from - subset where the back conversion like BGT may make sense
- analyze - go through the structures and identify features (all, subset)

We can add aliases like set for enable and clear for disable, the two
conversion commands may not be necessary or even confusing when it's not
clear which direction is the right one.

Actions:

- extref (on/off)
- skinny-metadata (on/off)
- no-holes (on)
- seeding (on/off)
- uuid rewrite (one time)
- metadata_uuid (set value)
- change checksum (convert-to)
- currently there's the simple quota, this may change

This should give examples of all categories of actions we might have in
the future.

> And even for set/clear, there is some changes to btrfstune:
> 
> - Merge seed feature into set/clear
>   To enable seeding, just go "btrfs tune set seed <device>".
> 
> - All supported features can be checked by "list-all" feature
>   Please note that, "btrfs tune set list-all" and
>   "btrfs tune clear list-all" will have different output.

As the help test works, we would get that for free if all the
implemented actions are also command names, i.e. 'btrfs tune convert-to'
would list 'block-group-tree'.

Re: [PATCH 0/7] btrfs-progs: cmds/tune: add set/clear features

[Qu Wenruo]

On 2023/10/14 05:20, David Sterba wrote:
> On Tue, Sep 05, 2023 at 03:51:42PM +0800, Qu Wenruo wrote:
>> This is the first step to convert btrfstune functionality to "btrfs
>> tune" subcommand group.
>>
>> For now only binary features, aka set and clear, is supported,
>> thus uuid and csum change is not yet implemented.
>> (Both need their own subcommand groups other than set/clear groups)
> 
> There are the easy on/off features that can be added first but you also
> added the block-group-tree _conversion_. That's a different category and
> IMHO requires a separate command so we can add options to check or reset
> the unfinished state. And we also have the the conversion 'to' and
> 'from'.
> 
> My current list of 1st level command groups is:
> 
> - enable - for the simple on/off features (on)
> - disable - dtto, (off)
> - convert-to - for the BGT, checksum
> - convert-from - subset where the back conversion like BGT may make sense

So your idea is, if an conversion is a long running one, and can be 
interrupted, then we need to go convert instead of simple on/off?

That makes sense, although my question is, does that makes any difference?

If we have an interrupted conversion, the only valid operation we can do 
is to finish the conversion. No rollback for most conversions.

Thus in the end, it's sill on/off, just with an extra interrupted 
status, but that status can only lead to the "on" state, thus it makes 
no difference to have an dedicated state for it.

Thanks,
Qu

> - analyze - go through the structures and identify features (all, subset)
> 
> We can add aliases like set for enable and clear for disable, the two
> conversion commands may not be necessary or even confusing when it's not
> clear which direction is the right one.
> 
> Actions:
> 
> - extref (on/off)
> - skinny-metadata (on/off)
> - no-holes (on)
> - seeding (on/off)
> - uuid rewrite (one time)
> - metadata_uuid (set value)
> - change checksum (convert-to)
> - currently there's the simple quota, this may change
> 
> This should give examples of all categories of actions we might have in
> the future.
> 
>> And even for set/clear, there is some changes to btrfstune:
>>
>> - Merge seed feature into set/clear
>>    To enable seeding, just go "btrfs tune set seed <device>".
>>
>> - All supported features can be checked by "list-all" feature
>>    Please note that, "btrfs tune set list-all" and
>>    "btrfs tune clear list-all" will have different output.
> 
> As the help test works, we would get that for free if all the
> implemented actions are also command names, i.e. 'btrfs tune convert-to'
> would list 'block-group-tree'.

Re: [PATCH 2/7] btrfs-progs: cmds: add "btrfs tune set" subcommand group

[Anand Jain]

On 10/14/23 01:47, David Sterba wrote:
> On Thu, Sep 21, 2023 at 08:53:11AM +0800, Anand Jain wrote:
>> On 05/09/2023 15:51, Qu Wenruo wrote:
>>> As the first step to convert btrfstune into "btrfs tune" subcommand
>>> group, this patch would add the following subcommand group:
>>>
>>>    btrfs tune set <feature> [<device>]
>>>
>>> For now the following features are supported:
>>>
>>> - extref
>>> - skinny-metadata
>>> - no-holes
>>>     All those are simple super block flags toggle.
>>>
>>> - list-all
>>>     This acts the same way as "mkfs.btrfs -O list-all", the difference is
>>>     it would only list the supported features.
>>>     (In the future, there will be "btrfs tune clear" subcommand, which
>>>      would support a different set of features).
>>>
>>
>> With this patchset, the syntax is structured as follows:
>>
>>
>>      $ btrfs tune --help
>>      usage: btrfs tune <command> <args>
>>
>>         btrfs tune set <feature> [<device>]
>>             Set/enable specified feature for the unmounted filesystem
>>         btrfs tune clear <feature> [<device>]
>>             Clear/disable specified feature for the unmounted filesystem
>>
>>      change various btrfs features
>>
>>
>>
>> However, for consistency, I suggest the following syntax:
> 
> Consistency with what? If it's with btrfstune then no, the reason why
> the command is split into subcommands is because the command line
> parameters became unwieldy and what you suggest below copies that.
> 
>>
>>     set:
>>      $ btrfs tune <feature> /dev/sda
>>
>>     clear:
>>      $ btrfs tune <feature> --clear /dev/sda
>>
>>     list:
>>      $ btrfs tune --list-all
>>
>> This syntax aligns with the:
>>
>>      $ btrfs device scan --forget
> 
> The difference is that scan won't be extended much if ever. The 'tune'
> interface grows over time so we need properly structure that so we don't
> end in command line option mess.


My apologies for not responding earlier. When designing the 'btrfs tune' 
command, pls consider we may need '--noscan' and '--device' options at 
some point. For example:

$ btrfs tune metadata --device=/dev/sda --noscan /dev/sdb

This idea is inspired by the following command:

$ btrfs inspect-internal dump-tree --noscan /dev/sda1

OR

Since some of the commands perform a user-space system-wide scan for
btrfs-block-devices, so global options like is also not a bad idea.

    '--devices' (specify btrfs image files and continue scanning for 
block devices across the system)

and

    '--noscan' (to avoid a system-wide block device scan when needed).


Thx, Anand


> 
> As an anti-pattern I can mention the 'mdadm' utility, that comes from
> times when the commands were not without the initial "--" like we know
> from git etc, so the main commands like assemble are specified by -A or
> --assemble. What I find confusing is that it's just another option but
> means a main action. The command groups logically and visually
> encapsulate one action and can be possibly fine tuned by parameters
> without affecting other commands.