[PATCH 0/3] Btrfs-progs: restriper interface

[PATCH 0/3] Btrfs-progs: restriper interface

[Ilya Dryomov]

Hello,

This is the userspace part of restriper, rebased onto the new progs
infrastructure.  Restriper commands are located under 'balance' prefix,
which is now the top level command group.  However to not confuse
existing users 'balance' prefix is also available under 'filesystem':

btrfs [filesystem] balance start
btrfs [filesystem] balance pause
btrfs [filesystem] balance cancel
btrfs [filesystem] balance resume
btrfs [filesystem] balance status

Backwards compatibility is fully preserved thanks to the new command
parser: the old 'btrfs filesystem balance <path>' works as expected.

The patchset is on top of the new parser posted half an hour ago,
available at:

git://github.com/idryomov/btrfs-progs.git restriper

Thanks,

		Ilya


Ilya Dryomov (3):
  Btrfs-progs: add restriper headers
  Btrfs-progs: add 'balance' command group infrastructure
  Btrfs-progs: add restriper commands

 Makefile          |    2 +-
 btrfs.c           |    1 +
 cmds-balance.c    |  684 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 cmds-filesystem.c |   38 +---
 commands.h        |    2 +
 ctree.h           |   23 ++-
 ioctl.h           |   53 ++++
 print-tree.c      |    6 +
 volumes.h         |   31 +++
 9 files changed, 795 insertions(+), 45 deletions(-)
 create mode 100644 cmds-balance.c

-- 
1.7.6.3

[PATCH 1/3] Btrfs-progs: add restriper headers

[Ilya Dryomov]

Add restriper headers and update print-tree.c

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 ctree.h      |   23 ++++++++++++++++-------
 ioctl.h      |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 print-tree.c |    6 ++++++
 volumes.h    |   31 +++++++++++++++++++++++++++++++
 4 files changed, 106 insertions(+), 7 deletions(-)

diff --git a/ctree.h b/ctree.h
index 54748c8..1c3740c 100644
--- a/ctree.h
+++ b/ctree.h
@@ -61,6 +61,9 @@ struct btrfs_trans_handle;
 #define BTRFS_CSUM_TREE_OBJECTID 7ULL
 
 
+/* for storing balance parameters in the root tree */
+#define BTRFS_BALANCE_OBJECTID -4ULL
+
 /* oprhan objectid for tracking unlinked/truncated files */
 #define BTRFS_ORPHAN_OBJECTID -5ULL
 
@@ -697,13 +700,17 @@ struct btrfs_csum_item {
 } __attribute__ ((__packed__));
 
 /* tag for the radix tree of block groups in ram */
-#define BTRFS_BLOCK_GROUP_DATA     (1 << 0)
-#define BTRFS_BLOCK_GROUP_SYSTEM   (1 << 1)
-#define BTRFS_BLOCK_GROUP_METADATA (1 << 2)
-#define BTRFS_BLOCK_GROUP_RAID0    (1 << 3)
-#define BTRFS_BLOCK_GROUP_RAID1    (1 << 4)
-#define BTRFS_BLOCK_GROUP_DUP	   (1 << 5)
-#define BTRFS_BLOCK_GROUP_RAID10   (1 << 6)
+#define BTRFS_BLOCK_GROUP_DATA		(1ULL << 0)
+#define BTRFS_BLOCK_GROUP_SYSTEM	(1ULL << 1)
+#define BTRFS_BLOCK_GROUP_METADATA	(1ULL << 2)
+#define BTRFS_BLOCK_GROUP_RAID0		(1ULL << 3)
+#define BTRFS_BLOCK_GROUP_RAID1		(1ULL << 4)
+#define BTRFS_BLOCK_GROUP_DUP		(1ULL << 5)
+#define BTRFS_BLOCK_GROUP_RAID10	(1ULL << 6)
+#define BTRFS_BLOCK_GROUP_RESERVED	BTRFS_AVAIL_ALLOC_BIT_SINGLE
+
+/* used in struct btrfs_balance_args fields */
+#define BTRFS_AVAIL_ALLOC_BIT_SINGLE	(1ULL << 48)
 
 struct btrfs_block_group_item {
 	__le64 used;
@@ -911,6 +918,8 @@ struct btrfs_root {
 #define BTRFS_DEV_ITEM_KEY	216
 #define BTRFS_CHUNK_ITEM_KEY	228
 
+#define BTRFS_BALANCE_ITEM_KEY	248
+
 /*
  * string items are for debugging.  They just store a short string of
  * data in the FS
diff --git a/ioctl.h b/ioctl.h
index 1ae7537..f2e5d8d 100644
--- a/ioctl.h
+++ b/ioctl.h
@@ -91,6 +91,54 @@ struct btrfs_ioctl_fs_info_args {
 	__u64 reserved[124];			/* pad to 1k */
 };
 
+/* balance control ioctl modes */
+#define BTRFS_BALANCE_CTL_PAUSE		1
+#define BTRFS_BALANCE_CTL_CANCEL	2
+#define BTRFS_BALANCE_CTL_RESUME	3
+
+/*
+ * this is packed, because it should be exactly the same as its disk
+ * byte order counterpart (struct btrfs_disk_balance_args)
+ */
+struct btrfs_balance_args {
+	__u64 profiles;
+	__u64 usage;
+	__u64 devid;
+	__u64 pstart;
+	__u64 pend;
+	__u64 vstart;
+	__u64 vend;
+
+	__u64 target;
+
+	__u64 flags;
+
+	__u64 unused[8];
+} __attribute__ ((__packed__));
+
+struct btrfs_balance_progress {
+	__u64 expected;
+	__u64 considered;
+	__u64 completed;
+};
+
+#define BTRFS_BALANCE_STATE_RUNNING	(1ULL << 0)
+#define BTRFS_BALANCE_STATE_PAUSE_REQ	(1ULL << 1)
+#define BTRFS_BALANCE_STATE_CANCEL_REQ	(1ULL << 2)
+
+struct btrfs_ioctl_balance_args {
+	__u64 flags;				/* in/out */
+	__u64 state;				/* out */
+
+	struct btrfs_balance_args data;		/* in/out */
+	struct btrfs_balance_args meta;		/* in/out */
+	struct btrfs_balance_args sys;		/* in/out */
+
+	struct btrfs_balance_progress stat;	/* out */
+
+	__u64 unused[72];			/* pad to 1k */
+};
+
 struct btrfs_ioctl_search_key {
 	/* which root are we searching.  0 is the tree of tree roots */
 	__u64 tree_id;
@@ -273,6 +321,11 @@ struct btrfs_ioctl_logical_ino_args {
 					struct btrfs_ioctl_dev_info_args)
 #define BTRFS_IOC_FS_INFO _IOR(BTRFS_IOCTL_MAGIC, 31, \
                                  struct btrfs_ioctl_fs_info_args)
+#define BTRFS_IOC_BALANCE_V2 _IOWR(BTRFS_IOCTL_MAGIC, 32, \
+				   struct btrfs_ioctl_balance_args)
+#define BTRFS_IOC_BALANCE_CTL _IOW(BTRFS_IOCTL_MAGIC, 33, int)
+#define BTRFS_IOC_BALANCE_PROGRESS _IOR(BTRFS_IOCTL_MAGIC, 34, \
+					struct btrfs_ioctl_balance_args)
 #define BTRFS_IOC_INO_PATHS _IOWR(BTRFS_IOCTL_MAGIC, 35, \
 					struct btrfs_ioctl_ino_path_args)
 #define BTRFS_IOC_LOGICAL_INO _IOWR(BTRFS_IOCTL_MAGIC, 36, \
diff --git a/print-tree.c b/print-tree.c
index 6039699..494ba8b 100644
--- a/print-tree.c
+++ b/print-tree.c
@@ -351,6 +351,9 @@ static void print_key_type(u8 type)
 	case BTRFS_DEV_EXTENT_KEY:
 		printf("DEV_EXTENT");
 		break;
+	case BTRFS_BALANCE_ITEM_KEY:
+		printf("BALANCE_ITEM");
+		break;
 	case BTRFS_STRING_ITEM_KEY:
 		printf("STRING_ITEM");
 		break;
@@ -391,6 +394,9 @@ static void print_objectid(unsigned long long objectid, u8 type)
 	case BTRFS_CSUM_TREE_OBJECTID:
 		printf("CSUM_TREE");
 		break;
+	case BTRFS_BALANCE_OBJECTID:
+		printf("BALANCE");
+		break;
 	case BTRFS_ORPHAN_OBJECTID:
 		printf("ORPHAN");
 		break;
diff --git a/volumes.h b/volumes.h
index 7104d36..d7fcef6 100644
--- a/volumes.h
+++ b/volumes.h
@@ -91,6 +91,37 @@ struct btrfs_multi_bio {
 #define btrfs_multi_bio_size(n) (sizeof(struct btrfs_multi_bio) + \
 			    (sizeof(struct btrfs_bio_stripe) * (n)))
 
+/*
+ * Restriper's general type filter
+ */
+#define BTRFS_BALANCE_DATA		(1ULL << 0)
+#define BTRFS_BALANCE_SYSTEM		(1ULL << 1)
+#define BTRFS_BALANCE_METADATA		(1ULL << 2)
+
+#define BTRFS_BALANCE_TYPE_MASK		(BTRFS_BALANCE_DATA |	    \
+					 BTRFS_BALANCE_SYSTEM |	    \
+					 BTRFS_BALANCE_METADATA)
+
+#define BTRFS_BALANCE_FORCE		(1ULL << 3)
+#define BTRFS_BALANCE_RESUME		(1ULL << 4)
+
+/*
+ * Balance filters
+ */
+#define BTRFS_BALANCE_ARGS_PROFILES	(1ULL << 0)
+#define BTRFS_BALANCE_ARGS_USAGE	(1ULL << 1)
+#define BTRFS_BALANCE_ARGS_DEVID	(1ULL << 2)
+#define BTRFS_BALANCE_ARGS_DRANGE	(1ULL << 3)
+#define BTRFS_BALANCE_ARGS_VRANGE	(1ULL << 4)
+
+/*
+ * Profile changing flags.  When SOFT is set we won't relocate chunk if
+ * it already has the target profile (even though it may be
+ * half-filled).
+ */
+#define BTRFS_BALANCE_ARGS_CONVERT	(1ULL << 8)
+#define BTRFS_BALANCE_ARGS_SOFT		(1ULL << 9)
+
 int btrfs_alloc_dev_extent(struct btrfs_trans_handle *trans,
 			   struct btrfs_device *device,
 			   u64 chunk_tree, u64 chunk_objectid,
-- 
1.7.6.3

[PATCH 2/3] Btrfs-progs: add 'balance' command group infrastructure

[Ilya Dryomov]

Add balance command group under both 'btrfs' and 'btrfs filesystem'.
Preserve the old 'btrfs filesystem balance <path>' behaviour.

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 Makefile          |    2 +-
 btrfs.c           |    1 +
 cmds-balance.c    |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 cmds-filesystem.c |   38 +--------------------
 commands.h        |    2 +
 5 files changed, 101 insertions(+), 38 deletions(-)
 create mode 100644 cmds-balance.c

diff --git a/Makefile b/Makefile
index deafda6..cd5f7f1 100644
--- a/Makefile
+++ b/Makefile
@@ -6,7 +6,7 @@ objects = ctree.o disk-io.o radix-tree.o extent-tree.o print-tree.o \
 	  inode-map.o crc32c.o rbtree.o extent-cache.o extent_io.o \
 	  volumes.o utils.o btrfs-list.o btrfslabel.o
 cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \
-	       cmds-inspect.o
+	       cmds-inspect.o cmds-balance.o
 
 CHECKFLAGS= -D__linux__ -Dlinux -D__STDC__ -Dunix -D__unix__ -Wbitwise \
 	    -Wuninitialized -Wshadow -Wundef
diff --git a/btrfs.c b/btrfs.c
index 599a3e7..76f725a 100644
--- a/btrfs.c
+++ b/btrfs.c
@@ -240,6 +240,7 @@ const struct cmd_group btrfs_cmd_group = {
 	btrfs_cmd_group_usage, btrfs_cmd_group_info, {
 		{ "subvolume", cmd_subvolume, NULL, &subvolume_cmd_group, 0 },
 		{ "filesystem", cmd_filesystem, NULL, &filesystem_cmd_group, 0 },
+		{ "balance", cmd_balance, NULL, &balance_cmd_group, 0 },
 		{ "device", cmd_device, NULL, &device_cmd_group, 0 },
 		{ "scrub", cmd_scrub, NULL, &scrub_cmd_group, 0 },
 		{ "inspect-internal", cmd_inspect, NULL, &inspect_cmd_group, 0 },
diff --git a/cmds-balance.c b/cmds-balance.c
new file mode 100644
index 0000000..d141b46
--- /dev/null
+++ b/cmds-balance.c
@@ -0,0 +1,96 @@
+/*
+ * 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 <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <errno.h>
+
+#include "kerncompat.h"
+#include "ctree.h"
+#include "ioctl.h"
+#include "volumes.h"
+
+#include "commands.h"
+
+static const char balance_cmd_group_usage[] =
+	"btrfs [filesystem] balance [<command>] [options] <path>";
+
+static const char balance_cmd_group_info[] =
+	"'btrfs filesystem balance' command is deprecated, please use\n"
+	"'btrfs balance start' command instead.";
+
+static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args)
+{
+	int fd;
+	int ret;
+	int e;
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_BALANCE_V2, args);
+	e = errno;
+	close(fd);
+
+	if (ret < 0) {
+		if (e == ECANCELED) {
+			if (args->state & BTRFS_BALANCE_STATE_PAUSE_REQ)
+				fprintf(stderr, "balance paused by user\n");
+			if (args->state & BTRFS_BALANCE_STATE_CANCEL_REQ)
+				fprintf(stderr, "balance canceled by user\n");
+		} else {
+			fprintf(stderr, "ERROR: error during balancing '%s' "
+				"- %s\n", path, strerror(e));
+			if (e != EINPROGRESS)
+				fprintf(stderr, "There may be more info in "
+					"syslog - try dmesg | tail\n");
+			return 19;
+		}
+	} else {
+		printf("Done, had to relocate %llu out of %llu chunks\n",
+		       (unsigned long long)args->stat.completed,
+		       (unsigned long long)args->stat.considered);
+	}
+
+	return 0;
+}
+
+const struct cmd_group balance_cmd_group = {
+	balance_cmd_group_usage, balance_cmd_group_info, {
+		{ 0, 0, 0, 0, 0 }
+	}
+};
+
+int cmd_balance(int argc, char **argv)
+{
+	if (argc == 2) {
+		/* old 'btrfs filesystem balance <path>' syntax */
+		struct btrfs_ioctl_balance_args args;
+
+		memset(&args, 0, sizeof(args));
+		args.flags |= BTRFS_BALANCE_TYPE_MASK;
+
+		return do_balance(argv[1], &args);
+	}
+
+	return handle_command_group(&balance_cmd_group, argc, argv);
+}
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 828ca0c..a2efb93 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -453,42 +453,6 @@ static int cmd_defrag(int argc, char **argv)
 	return errors + 20;
 }
 
-static const char * const cmd_balance_usage[] = {
-	"btrfs filesystem balance <path>",
-	"Balance the chunks across the device",
-	NULL
-};
-
-static int cmd_balance(int argc, char **argv)
-{
-	int	fdmnt, ret=0, e;
-	struct btrfs_ioctl_vol_args args;
-	char	*path;
-
-	if (check_argc_exact(argc, 2))
-		usage(cmd_balance_usage);
-
-	path = argv[1];
-
-	fdmnt = open_file_or_dir(path);
-	if (fdmnt < 0) {
-		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
-		return 12;
-	}
-
-	memset(&args, 0, sizeof(args));
-	ret = ioctl(fdmnt, BTRFS_IOC_BALANCE, &args);
-	e = errno;
-	close(fdmnt);
-	if(ret<0){
-		fprintf(stderr, "ERROR: error during balancing '%s' - %s\n", 
-			path, strerror(e));
-
-		return 19;
-	}
-	return 0;
-}
-
 static const char * const cmd_resize_usage[] = {
 	"btrfs filesystem resize [+/-]<newsize>[gkm]|max <path>",
 	"Resize a filesystem",
@@ -559,7 +523,7 @@ const struct cmd_group filesystem_cmd_group = {
 		{ "show", cmd_show, cmd_show_usage, NULL, 0 },
 		{ "sync", cmd_sync, cmd_sync_usage, NULL, 0 },
 		{ "defragment", cmd_defrag, cmd_defrag_usage, NULL, 0 },
-		{ "balance", cmd_balance, cmd_balance_usage, NULL, 0 },
+		{ "balance", cmd_balance, NULL, &balance_cmd_group, 1 },
 		{ "resize", cmd_resize, cmd_resize_usage, NULL, 0 },
 		{ "label", cmd_label, cmd_label_usage, NULL, 0 },
 		{ 0, 0, 0, 0, 0 },
diff --git a/commands.h b/commands.h
index 5d024c2..de2c75c 100644
--- a/commands.h
+++ b/commands.h
@@ -84,12 +84,14 @@ int open_file_or_dir(const char *fname);
 
 extern const struct cmd_group subvolume_cmd_group;
 extern const struct cmd_group filesystem_cmd_group;
+extern const struct cmd_group balance_cmd_group;
 extern const struct cmd_group device_cmd_group;
 extern const struct cmd_group scrub_cmd_group;
 extern const struct cmd_group inspect_cmd_group;
 
 int cmd_subvolume(int argc, char **argv);
 int cmd_filesystem(int argc, char **argv);
+int cmd_balance(int argc, char **argv);
 int cmd_device(int argc, char **argv);
 int cmd_scrub(int argc, char **argv);
 int cmd_inspect(int argc, char **argv);
-- 
1.7.6.3

[PATCH 3/3] Btrfs-progs: add restriper commands

[Ilya Dryomov]

Signed-off-by: Ilya Dryomov <idryomov@gmail.com>
---
 cmds-balance.c |  588 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 588 insertions(+), 0 deletions(-)

diff --git a/cmds-balance.c b/cmds-balance.c
index d141b46..a6886a0 100644
--- a/cmds-balance.c
+++ b/cmds-balance.c
@@ -18,6 +18,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <getopt.h>
 #include <sys/ioctl.h>
 #include <errno.h>
 
@@ -35,6 +36,240 @@ static const char balance_cmd_group_info[] =
 	"'btrfs filesystem balance' command is deprecated, please use\n"
 	"'btrfs balance start' command instead.";
 
+static int parse_one_profile(const char *profile, u64 *flags)
+{
+	if (!strcmp(profile, "raid0")) {
+		*flags |= BTRFS_BLOCK_GROUP_RAID0;
+	} else if (!strcmp(profile, "raid1")) {
+		*flags |= BTRFS_BLOCK_GROUP_RAID1;
+	} else if (!strcmp(profile, "raid10")) {
+		*flags |= BTRFS_BLOCK_GROUP_RAID10;
+	} else if (!strcmp(profile, "dup")) {
+		*flags |= BTRFS_BLOCK_GROUP_DUP;
+	} else if (!strcmp(profile, "single")) {
+		*flags |= BTRFS_AVAIL_ALLOC_BIT_SINGLE;
+	} else {
+		fprintf(stderr, "Unknown profile '%s'\n", profile);
+		return 1;
+	}
+
+	return 0;
+}
+
+static int parse_profiles(char *profiles, u64 *flags)
+{
+	char *this_char;
+	char *save_ptr;
+
+	for (this_char = strtok_r(profiles, "|", &save_ptr);
+	     this_char != NULL;
+	     this_char = strtok_r(NULL, "|", &save_ptr)) {
+		if (parse_one_profile(this_char, flags))
+			return 1;
+	}
+
+	return 0;
+}
+
+static int parse_u64(const char *str, u64 *result)
+{
+	char *endptr;
+	u64 val;
+
+	val = strtoull(str, &endptr, 10);
+	if (*endptr)
+		return 1;
+
+	*result = val;
+	return 0;
+}
+
+static int parse_range(const char *range, u64 *start, u64 *end)
+{
+	char *dots;
+
+	dots = strstr(range, "..");
+	if (dots) {
+		const char *rest = dots + 2;
+		int skipped = 0;
+
+		*dots = 0;
+
+		if (!*rest) {
+			*end = (u64)-1;
+			skipped++;
+		} else {
+			if (parse_u64(rest, end))
+				return 1;
+		}
+		if (dots == range) {
+			*start = 0;
+			skipped++;
+		} else {
+			if (parse_u64(range, start))
+				return 1;
+		}
+
+		if (*start >= *end) {
+			fprintf(stderr, "Range %llu..%llu doesn't make "
+				"sense\n", (unsigned long long)*start,
+				(unsigned long long)*end);
+			return 1;
+		}
+
+		if (skipped <= 1)
+			return 0;
+	}
+
+	return 1;
+}
+
+static int parse_filters(char *filters, struct btrfs_balance_args *args)
+{
+	char *this_char;
+	char *value;
+	char *save_ptr;
+
+	if (!filters)
+		return 0;
+
+	for (this_char = strtok_r(filters, ",", &save_ptr);
+	     this_char != NULL;
+	     this_char = strtok_r(NULL, ",", &save_ptr)) {
+		if ((value = strchr(this_char, '=')) != NULL)
+			*value++ = 0;
+		if (!strcmp(this_char, "profiles")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the profiles filter requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_profiles(value, &args->profiles)) {
+				fprintf(stderr, "Invalid profiles argument\n");
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_PROFILES;
+		} else if (!strcmp(this_char, "usage")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the usage filter requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_u64(value, &args->usage) ||
+			    args->usage < 1 || args->usage > 100) {
+				fprintf(stderr, "Invalid usage argument: %s\n",
+				       value);
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_USAGE;
+		} else if (!strcmp(this_char, "devid")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the devid filter requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_u64(value, &args->devid) ||
+			    args->devid == 0) {
+				fprintf(stderr, "Invalid devid argument: %s\n",
+				       value);
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_DEVID;
+		} else if (!strcmp(this_char, "drange")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the drange filter requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_range(value, &args->pstart, &args->pend)) {
+				fprintf(stderr, "Invalid drange argument\n");
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_DRANGE;
+		} else if (!strcmp(this_char, "vrange")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the vrange filter requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_range(value, &args->vstart, &args->vend)) {
+				fprintf(stderr, "Invalid vrange argument\n");
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_VRANGE;
+		} else if (!strcmp(this_char, "convert")) {
+			if (!value || !*value) {
+				fprintf(stderr, "the convert option requires "
+				       "an argument\n");
+				return 1;
+			}
+			if (parse_one_profile(value, &args->target)) {
+				fprintf(stderr, "Invalid convert argument\n");
+				return 1;
+			}
+			args->flags |= BTRFS_BALANCE_ARGS_CONVERT;
+		} else if (!strcmp(this_char, "soft")) {
+			args->flags |= BTRFS_BALANCE_ARGS_SOFT;
+		} else {
+			fprintf(stderr, "Unrecognized balance option '%s'\n",
+				this_char);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+static void dump_balance_args(struct btrfs_balance_args *args)
+{
+	if (args->flags & BTRFS_BALANCE_ARGS_CONVERT) {
+		printf("converting, target=%llu, soft is %s",
+		       (unsigned long long)args->target,
+		       (args->flags & BTRFS_BALANCE_ARGS_SOFT) ? "on" : "off");
+	} else {
+		printf("balancing");
+	}
+
+	if (args->flags & BTRFS_BALANCE_ARGS_PROFILES)
+		printf(", profiles=%llu", (unsigned long long)args->profiles);
+	if (args->flags & BTRFS_BALANCE_ARGS_USAGE)
+		printf(", usage=%llu", (unsigned long long)args->usage);
+	if (args->flags & BTRFS_BALANCE_ARGS_DEVID)
+		printf(", devid=%llu", (unsigned long long)args->devid);
+	if (args->flags & BTRFS_BALANCE_ARGS_DRANGE)
+		printf(", drange=%llu..%llu",
+		       (unsigned long long)args->pstart,
+		       (unsigned long long)args->pend);
+	if (args->flags & BTRFS_BALANCE_ARGS_VRANGE)
+		printf(", vrange=%llu..%llu",
+		       (unsigned long long)args->vstart,
+		       (unsigned long long)args->vend);
+
+	printf("\n");
+}
+
+static void dump_ioctl_balance_args(struct btrfs_ioctl_balance_args *args)
+{
+	printf("Dumping filters: flags 0x%llx, state 0x%llx, force is %s\n",
+	       (unsigned long long)args->flags, (unsigned long long)args->state,
+	       (args->flags & BTRFS_BALANCE_FORCE) ? "on" : "off");
+	if (args->flags & BTRFS_BALANCE_DATA) {
+		printf("  DATA (flags 0x%llx): ",
+		       (unsigned long long)args->data.flags);
+		dump_balance_args(&args->data);
+	}
+	if (args->flags & BTRFS_BALANCE_METADATA) {
+		printf("  METADATA (flags 0x%llx): ",
+		       (unsigned long long)args->meta.flags);
+		dump_balance_args(&args->meta);
+	}
+	if (args->flags & BTRFS_BALANCE_SYSTEM) {
+		printf("  SYSTEM (flags 0x%llx): ",
+		       (unsigned long long)args->sys.flags);
+		dump_balance_args(&args->sys);
+	}
+}
+
 static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args)
 {
 	int fd;
@@ -74,8 +309,361 @@ static int do_balance(const char *path, struct btrfs_ioctl_balance_args *args)
 	return 0;
 }
 
+static const char * const cmd_balance_start_usage[] = {
+	"btrfs [filesystem] balance start [options] <path>",
+	"Balance chunks across the devices",
+	"Balance and/or convert (change allocation profile of) chunks that",
+	"passed all filters in a comma-separated list of filters for a",
+	"particular chunk type.  If filter list is not given balance all",
+	"chunks of that type.  In case none of the -d, -m or -s options is",
+	"given balance all chunks in a filesystem.",
+	"",
+	"-d[filters]    act on data chunks",
+	"-m[filters]    act on metadata chunks",
+	"-s[filetrs]    act on system chunks (only under -f)",
+	"-v             be verbose",
+	"-f             force reducing of metadata integrity",
+	NULL
+};
+
+static int cmd_balance_start(int argc, char **argv)
+{
+	struct btrfs_ioctl_balance_args args;
+	struct btrfs_balance_args *ptrs[] = { &args.data, &args.sys,
+						&args.meta, NULL };
+	int force = 0;
+	int verbose = 0;
+	int nofilters = 1;
+	int i;
+
+	memset(&args, 0, sizeof(args));
+
+	optind = 1;
+	while (1) {
+		int longindex;
+		static struct option longopts[] = {
+			{ "data", optional_argument, NULL, 'd'},
+			{ "metadata", optional_argument, NULL, 'm' },
+			{ "system", optional_argument, NULL, 's' },
+			{ "force", no_argument, NULL, 'f' },
+			{ "verbose", no_argument, NULL, 'v' },
+			{ 0, 0, 0, 0 }
+		};
+
+		int opt = getopt_long(argc, argv, "d::s::m::fv", longopts,
+				      &longindex);
+		if (opt < 0)
+			break;
+
+		switch (opt) {
+		case 'd':
+			nofilters = 0;
+			args.flags |= BTRFS_BALANCE_DATA;
+
+			if (parse_filters(optarg, &args.data))
+				return 1;
+			break;
+		case 's':
+			nofilters = 0;
+			args.flags |= BTRFS_BALANCE_SYSTEM;
+
+			if (parse_filters(optarg, &args.sys))
+				return 1;
+			break;
+		case 'm':
+			nofilters = 0;
+			args.flags |= BTRFS_BALANCE_METADATA;
+
+			if (parse_filters(optarg, &args.meta))
+				return 1;
+			break;
+		case 'f':
+			force = 1;
+			break;
+		case 'v':
+			verbose = 1;
+			break;
+		default:
+			usage(cmd_balance_start_usage);
+		}
+	}
+
+	if (check_argc_exact(argc - optind, 1))
+		usage(cmd_balance_start_usage);
+
+	/*
+	 * allow -s only under --force, otherwise do with system chunks
+	 * the same thing we were ordered to do with meta chunks
+	 */
+	if (args.flags & BTRFS_BALANCE_SYSTEM) {
+		if (!force) {
+			fprintf(stderr,
+"Refusing to explicitly operate on system chunks.\n"
+"Pass --force if you really want to do that.\n");
+			return 1;
+		}
+	} else if (args.flags & BTRFS_BALANCE_METADATA) {
+		args.flags |= BTRFS_BALANCE_SYSTEM;
+		memcpy(&args.sys, &args.meta,
+			sizeof(struct btrfs_balance_args));
+	}
+
+	if (nofilters) {
+		/* relocate everything - no filters */
+		args.flags |= BTRFS_BALANCE_TYPE_MASK;
+	}
+
+	/* drange makes sense only when devid is set */
+	for (i = 0; ptrs[i]; i++) {
+		if ((ptrs[i]->flags & BTRFS_BALANCE_ARGS_DRANGE) &&
+		    !(ptrs[i]->flags & BTRFS_BALANCE_ARGS_DEVID)) {
+			fprintf(stderr, "drange filter can be used only if "
+				"devid filter is used\n");
+			return 1;
+		}
+	}
+
+	/* soft makes sense only when convert for corresponding type is set */
+	for (i = 0; ptrs[i]; i++) {
+		if ((ptrs[i]->flags & BTRFS_BALANCE_ARGS_SOFT) &&
+		    !(ptrs[i]->flags & BTRFS_BALANCE_ARGS_CONVERT)) {
+			fprintf(stderr, "'soft' option can be used only if "
+				"changing profiles\n");
+			return 1;
+		}
+	}
+
+	if (force)
+		args.flags |= BTRFS_BALANCE_FORCE;
+	if (verbose)
+		dump_ioctl_balance_args(&args);
+
+	return do_balance(argv[optind], &args);
+}
+
+static const char * const cmd_balance_pause_usage[] = {
+	"btrfs [filesystem] balance pause <path>",
+	"Pause running balance",
+	NULL
+};
+
+static int cmd_balance_pause(int argc, char **argv)
+{
+	const char *path;
+	int fd;
+	int ret;
+	int e;
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_balance_pause_usage);
+
+	path = argv[1];
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_BALANCE_CTL, BTRFS_BALANCE_CTL_PAUSE);
+	e = errno;
+	close(fd);
+
+	if (ret < 0) {
+		fprintf(stderr, "ERROR: balance pause on '%s' failed - %s\n",
+			path, (e == ENOTCONN) ? "Not running" : strerror(e));
+		return 19;
+	}
+
+	return 0;
+}
+
+static const char * const cmd_balance_cancel_usage[] = {
+	"btrfs [filesystem] balance cancel <path>",
+	"Cancel running or paused balance",
+	NULL
+};
+
+static int cmd_balance_cancel(int argc, char **argv)
+{
+	const char *path;
+	int fd;
+	int ret;
+	int e;
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_balance_cancel_usage);
+
+	path = argv[1];
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_BALANCE_CTL, BTRFS_BALANCE_CTL_CANCEL);
+	e = errno;
+	close(fd);
+
+	if (ret < 0) {
+		fprintf(stderr, "ERROR: balance cancel on '%s' failed - %s\n",
+			path, (e == ENOTCONN) ? "Not in progress" : strerror(e));
+		return 19;
+	}
+
+	return 0;
+}
+
+static const char * const cmd_balance_resume_usage[] = {
+	"btrfs [filesystem] balance resume <path>",
+	"Resume interrupted balance",
+	NULL
+};
+
+static int cmd_balance_resume(int argc, char **argv)
+{
+	struct btrfs_ioctl_balance_args args;
+	const char *path;
+	int fd;
+	int ret;
+	int e;
+
+	if (check_argc_exact(argc, 2))
+		usage(cmd_balance_resume_usage);
+
+	path = argv[1];
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	memset(&args, 0, sizeof(args));
+	args.flags |= BTRFS_BALANCE_RESUME;
+
+	ret = ioctl(fd, BTRFS_IOC_BALANCE_V2, &args);
+	e = errno;
+	close(fd);
+
+	if (ret < 0) {
+		if (e == ECANCELED) {
+			if (args.state & BTRFS_BALANCE_STATE_PAUSE_REQ)
+				fprintf(stderr, "balance paused by user\n");
+			if (args.state & BTRFS_BALANCE_STATE_CANCEL_REQ)
+				fprintf(stderr, "balance canceled by user\n");
+		} else if (e == ENOTCONN || e == EINPROGRESS) {
+			fprintf(stderr, "ERROR: balance resume on '%s' "
+				"failed - %s\n", path,
+				(e == ENOTCONN) ? "Not in progress" :
+						  "Already running");
+			return 19;
+		} else {
+			fprintf(stderr,
+"ERROR: error during balancing '%s' - %s\n"
+"There may be more info in syslog - try dmesg | tail\n", path, strerror(e));
+			return 19;
+		}
+	} else {
+		printf("Done, had to relocate %llu out of %llu chunks\n",
+		       (unsigned long long)args.stat.completed,
+		       (unsigned long long)args.stat.considered);
+	}
+
+	return 0;
+}
+
+static const char * const cmd_balance_status_usage[] = {
+	"btrfs [filesystem] balance status [-v] <path>",
+	"Show status of running or paused balance",
+	"",
+	"-v     be verbose",
+	NULL
+};
+
+static int cmd_balance_status(int argc, char **argv)
+{
+	struct btrfs_ioctl_balance_args args;
+	const char *path;
+	int fd;
+	int verbose = 0;
+	int ret;
+	int e;
+
+	optind = 1;
+	while (1) {
+		int longindex;
+		static struct option longopts[] = {
+			{ "verbose", no_argument, NULL, 'v' },
+			{ 0, 0, 0, 0}
+		};
+
+		int opt = getopt_long(argc, argv, "v", longopts, &longindex);
+		if (opt < 0)
+			break;
+
+		switch (opt) {
+		case 'v':
+			verbose = 1;
+			break;
+		default:
+			usage(cmd_balance_status_usage);
+		}
+	}
+
+	if (check_argc_exact(argc - optind, 1))
+		usage(cmd_balance_status_usage);
+
+	path = argv[optind];
+
+	fd = open_file_or_dir(path);
+	if (fd < 0) {
+		fprintf(stderr, "ERROR: can't access to '%s'\n", path);
+		return 12;
+	}
+
+	ret = ioctl(fd, BTRFS_IOC_BALANCE_PROGRESS, &args);
+	e = errno;
+	close(fd);
+
+	if (ret < 0) {
+		fprintf(stderr, "ERROR: balance status on '%s' failed - %s\n",
+			path, (e == ENOTCONN) ? "Not in progress" : strerror(e));
+		return 19;
+	}
+
+	if (args.state & BTRFS_BALANCE_STATE_RUNNING) {
+		printf("Balance on '%s' is running", path);
+		if (args.state & BTRFS_BALANCE_STATE_CANCEL_REQ)
+			printf(", cancel requested\n");
+		else if (args.state & BTRFS_BALANCE_STATE_PAUSE_REQ)
+			printf(", pause requested\n");
+		else
+			printf("\n");
+	} else {
+		printf("Balance on '%s' is paused\n", path);
+	}
+
+	printf("%llu out of about %llu chunks balanced (%llu considered), "
+	       "%3.f%% left\n", (unsigned long long)args.stat.completed,
+	       (unsigned long long)args.stat.expected,
+	       (unsigned long long)args.stat.considered,
+	       100 * (1 - (float)args.stat.completed/args.stat.expected));
+
+	if (verbose)
+		dump_ioctl_balance_args(&args);
+
+	return 0;
+}
+
 const struct cmd_group balance_cmd_group = {
 	balance_cmd_group_usage, balance_cmd_group_info, {
+		{ "start", cmd_balance_start, cmd_balance_start_usage, NULL, 0 },
+		{ "pause", cmd_balance_pause, cmd_balance_pause_usage, NULL, 0 },
+		{ "cancel", cmd_balance_cancel, cmd_balance_cancel_usage, NULL, 0 },
+		{ "resume", cmd_balance_resume, cmd_balance_resume_usage, NULL, 0 },
+		{ "status", cmd_balance_status, cmd_balance_status_usage, NULL, 0 },
 		{ 0, 0, 0, 0, 0 }
 	}
 };
-- 
1.7.6.3

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 657 bytes --]

   Hi, Ilya,

On Fri, Feb 03, 2012 at 11:49:09PM +0200, Ilya Dryomov wrote:
> This is the userspace part of restriper, rebased onto the new progs
> infrastructure.

   I'm definitely very busy for the next three weeks, but if nobody
else has picked these (and the parser patches) up by then, I'll grab
them for my integration branch at that point.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
   --- To an Englishman, 100 miles is a long way;  to an American, ---   
                        100 years is a long time.                        

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Chris Mason]

On Fri, Feb 03, 2012 at 10:13:42PM +0000, Hugo Mills wrote:
>    Hi, Ilya,
> 
> On Fri, Feb 03, 2012 at 11:49:09PM +0200, Ilya Dryomov wrote:
> > This is the userspace part of restriper, rebased onto the new progs
> > infrastructure.
> 
>    I'm definitely very busy for the next three weeks, but if nobody
> else has picked these (and the parser patches) up by then, I'll grab
> them for my integration branch at that point.

Thanks Hugo, I've got them here.

-chris

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Goffredo Baroncelli]

Hi Ilya,

On Friday, 03 February, 2012 22:49:09 Ilya Dryomov wrote:
> Hello,
> 
> This is the userspace part of restriper, rebased onto the new progs
> infrastructure.  Restriper commands are located under 'balance' prefix,
> which is now the top level command group.  However to not confuse
> existing users 'balance' prefix is also available under 'filesystem':
> 
> btrfs [filesystem] balance start
> btrfs [filesystem] balance pause
> btrfs [filesystem] balance cancel
> btrfs [filesystem] balance resume
> btrfs [filesystem] balance status
> 
> Backwards compatibility is fully preserved thanks to the new command
> parser: the old 'btrfs filesystem balance <path>' works as expected.

I prefer to avoid prefix which may confuse the user. If I have a subvolume 
called "s", what means:

	btrfs filesystem balance s

Is it a missing argument (the path required for btrfs filesystem balance) or 
is it a synonymus of

	btrfs filesystem balance start ./s

(or btrfs filesystem balance stop ./s)

?

To me I prefer to add another family of command called

	btrfs balance *

and to leave

	btrfs filesystem balance 

as synonymus of

	btrfs balance start


This was discussed several years ago. Allowing the user to abbreviate the 
commands fights with the ability to have different length of commands , when 
an option could beconfused with a command (s vs start/stop....).
> 
> The patchset is on top of the new parser posted half an hour ago,
> available at:
> 
> git://github.com/idryomov/btrfs-progs.git restriper
> 
> Thanks,
> 
> 		Ilya
> 
> 
> Ilya Dryomov (3):
>   Btrfs-progs: add restriper headers
>   Btrfs-progs: add 'balance' command group infrastructure
>   Btrfs-progs: add restriper commands
> 
>  Makefile          |    2 +-
>  btrfs.c           |    1 +
>  cmds-balance.c    |  684
> +++++++++++++++++++++++++++++++++++++++++++++++++++++ cmds-filesystem.c | 
>  38 +---
>  commands.h        |    2 +
>  ctree.h           |   23 ++-
>  ioctl.h           |   53 ++++
>  print-tree.c      |    6 +
>  volumes.h         |   31 +++
>  9 files changed, 795 insertions(+), 45 deletions(-)
>  create mode 100644 cmds-balance.c

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Ilya Dryomov]

On Sat, Feb 04, 2012 at 02:06:28PM +0100, Goffredo Baroncelli wrote:
> Hi Ilya,
> 
> On Friday, 03 February, 2012 22:49:09 Ilya Dryomov wrote:
> > Hello,
> > 
> > This is the userspace part of restriper, rebased onto the new progs
> > infrastructure.  Restriper commands are located under 'balance' prefix,
> > which is now the top level command group.  However to not confuse
> > existing users 'balance' prefix is also available under 'filesystem':
> > 
> > btrfs [filesystem] balance start
> > btrfs [filesystem] balance pause
> > btrfs [filesystem] balance cancel
> > btrfs [filesystem] balance resume
> > btrfs [filesystem] balance status
> > 
> > Backwards compatibility is fully preserved thanks to the new command
> > parser: the old 'btrfs filesystem balance <path>' works as expected.
> 
> I prefer to avoid prefix which may confuse the user. If I have a subvolume 
> called "s", what means:
> 
> 	btrfs filesystem balance s
> 
> Is it a missing argument (the path required for btrfs filesystem balance) or 
> is it a synonymus of
> 
> 	btrfs filesystem balance start ./s
> 
> (or btrfs filesystem balance stop ./s)
> 
> ?

It's a path.  It's unambiguous because every subcommand requires an argument.
If you omit the path argument you get the following:

btrfs-progs-new$ ./btrfs fi balance
usage: btrfs [filesystem] balance [<command>] [options] <path>

    btrfs [filesystem] balance start [options] <path>
        Balance chunks across the devices
    btrfs [filesystem] balance pause <path>
        Pause running balance
    btrfs [filesystem] balance cancel <path>
        Cancel running or paused balance
    btrfs [filesystem] balance resume <path>
        Resume interrupted balance
    btrfs [filesystem] balance status [-v] <path>
        Show status of running or paused balance

'btrfs filesystem balance' command is deprecated, please use
'btrfs balance start' command instead.

which clearly indicates that <command> is optional.  Finally, if you type

	btrfs fi balance s /mnt

(which is ambiguous because 's' could mean 'start' or 'status') you get
the following:

btrfs-progs-new$ ./btrfs fi balance s /mnt
btrfs filesystem balance: ambiguous token 's'

Did you mean one of these ?
	start
	status

Pretty clear to me.

> 
> To me I prefer to add another family of command called
> 
> 	btrfs balance *
> 
> and to leave
> 
> 	btrfs filesystem balance 
> 
> as synonymus of
> 
> 	btrfs balance start

I discussed this with Chris and we agreed that it will be confusing if
'btrfs filesystem balance' is different from 'btrfs balance'.

> 
> 
> This was discussed several years ago. Allowing the user to abbreviate the 
> commands fights with the ability to have different length of commands , when 
> an option could beconfused with a command (s vs start/stop....).

It generally does, but in this particular case (where every subcommand
in the group requires at least one argument) it does not.

Thanks,

		Ilya

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Goffredo Baroncelli]

On Saturday, 04 February, 2012 15:47:17 Ilya Dryomov wrote:
> On Sat, Feb 04, 2012 at 02:06:28PM +0100, Goffredo Baroncelli wrote:
> > Hi Ilya,
> > 
> > On Friday, 03 February, 2012 22:49:09 Ilya Dryomov wrote:
> > > Hello,
> > > 
> > > This is the userspace part of restriper, rebased onto the new progs
> > > infrastructure.  Restriper commands are located under 'balance' prefix,
> > > which is now the top level command group.  However to not confuse
> > > existing users 'balance' prefix is also available under 'filesystem':
> > > 
> > > btrfs [filesystem] balance start
> > > btrfs [filesystem] balance pause
> > > btrfs [filesystem] balance cancel
> > > btrfs [filesystem] balance resume
> > > btrfs [filesystem] balance status
> > > 
> > > Backwards compatibility is fully preserved thanks to the new command
> > > parser: the old 'btrfs filesystem balance <path>' works as expected.
> > 
> > I prefer to avoid prefix which may confuse the user. If I have a
> > subvolume
> > 
> > called "s", what means:
> > 	btrfs filesystem balance s
> > 
> > Is it a missing argument (the path required for btrfs filesystem balance)
> > or is it a synonymus of
> > 
> > 	btrfs filesystem balance start ./s
> > 
> > (or btrfs filesystem balance stop ./s)
> > 
> > ?
> 
> It's a path.  It's unambiguous because every subcommand requires an
> argument. If you omit the path argument you get the following:

Sorry, I disagree. It is ambiguous because you are not able to  understand if 
there is an error of missing parameters.

If exist a filesystem called 's' 

# btrfs fi bal s

could be:

1) an error (missing parameters for the command "btrfs filesystem balance 
start/status" )
2) the starting of the balance of the filesystem 's'




> 
> btrfs-progs-new$ ./btrfs fi balance
> usage: btrfs [filesystem] balance [<command>] [options] <path>
> 
>     btrfs [filesystem] balance start [options] <path>
>         Balance chunks across the devices
>     btrfs [filesystem] balance pause <path>
>         Pause running balance
>     btrfs [filesystem] balance cancel <path>
>         Cancel running or paused balance
>     btrfs [filesystem] balance resume <path>
>         Resume interrupted balance
>     btrfs [filesystem] balance status [-v] <path>
>         Show status of running or paused balance
> 
> 'btrfs filesystem balance' command is deprecated, please use
> 'btrfs balance start' command instead.
> 
> which clearly indicates that <command> is optional.  Finally, if you type
> 
> 	btrfs fi balance s /mnt
> 
> (which is ambiguous because 's' could mean 'start' or 'status') you get
> the following:
> 
> btrfs-progs-new$ ./btrfs fi balance s /mnt
> btrfs filesystem balance: ambiguous token 's'
> 
> Did you mean one of these ?
> 	start
> 	status
> 
> Pretty clear to me.
> 
> > To me I prefer to add another family of command called
> > 
> > 	btrfs balance *
> > 
> > and to leave
> > 
> > 	btrfs filesystem balance
> > 
> > as synonymus of
> > 
> > 	btrfs balance start
> 
> I discussed this with Chris and we agreed that it will be confusing if
> 'btrfs filesystem balance' is different from 'btrfs balance'.
> 
> > This was discussed several years ago. Allowing the user to abbreviate the
> > commands fights with the ability to have different length of commands ,
> > when an option could beconfused with a command (s vs start/stop....).
> 
> It generally does, but in this particular case (where every subcommand
> in the group requires at least one argument) it does not.
> 
> Thanks,
> 
> 		Ilya
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Ilya Dryomov]

On Mon, Feb 06, 2012 at 11:20:18PM +0100, Goffredo Baroncelli wrote:
> On Saturday, 04 February, 2012 15:47:17 Ilya Dryomov wrote:
> > On Sat, Feb 04, 2012 at 02:06:28PM +0100, Goffredo Baroncelli wrote:
> > > Hi Ilya,
> > > 
> > > On Friday, 03 February, 2012 22:49:09 Ilya Dryomov wrote:
> > > > Hello,
> > > > 
> > > > This is the userspace part of restriper, rebased onto the new progs
> > > > infrastructure.  Restriper commands are located under 'balance' prefix,
> > > > which is now the top level command group.  However to not confuse
> > > > existing users 'balance' prefix is also available under 'filesystem':
> > > > 
> > > > btrfs [filesystem] balance start
> > > > btrfs [filesystem] balance pause
> > > > btrfs [filesystem] balance cancel
> > > > btrfs [filesystem] balance resume
> > > > btrfs [filesystem] balance status
> > > > 
> > > > Backwards compatibility is fully preserved thanks to the new command
> > > > parser: the old 'btrfs filesystem balance <path>' works as expected.
> > > 
> > > I prefer to avoid prefix which may confuse the user. If I have a
> > > subvolume
> > > 
> > > called "s", what means:
> > > 	btrfs filesystem balance s
> > > 
> > > Is it a missing argument (the path required for btrfs filesystem balance)
> > > or is it a synonymus of
> > > 
> > > 	btrfs filesystem balance start ./s
> > > 
> > > (or btrfs filesystem balance stop ./s)
> > > 
> > > ?
> > 
> > It's a path.  It's unambiguous because every subcommand requires an
> > argument. If you omit the path argument you get the following:
> 
> Sorry, I disagree. It is ambiguous because you are not able to  understand if 
> there is an error of missing parameters.
> 
> If exist a filesystem called 's' 
> 
> # btrfs fi bal s
> 
> could be:
> 
> 1) an error (missing parameters for the command "btrfs filesystem balance 
> start/status" )
> 2) the starting of the balance of the filesystem 's'

It's true, but you are supposed to look at the synopsis before using new
commands.  The old user (who doesn't know anything about these new fancy
commands) won't see the difference.  The new users should take a look at
the man page or at least at the help output.

To make things even more clear I'll merge a slight change to allow
multi-line synopsis for command groups.  So the help looks like:

btrfs-progs-new$ ./btrfs fi bal --help
usage: btrfs [filesystem] balance <command> [options] <path>
   or: btrfs [filesystem] balance <path>

    btrfs [filesystem] balance start [options] <path>
        Balance chunks across the devices
    btrfs [filesystem] balance pause <path>
        Pause running balance
    btrfs [filesystem] balance cancel <path>
        Cancel running or paused balance
    btrfs [filesystem] balance resume <path>
        Resume interrupted balance
    btrfs [filesystem] balance status [-v] <path>
        Show status of running or paused balance

'btrfs filesystem balance' command is deprecated, please use
'btrfs balance start' command instead.

It clearly shows that 's' in your example will be interpreted as a path.

Thanks,

		Ilya

Re: [PATCH 0/3] Btrfs-progs: restriper interface

[Hugo Mills]

[-- Attachment #1: Type: text/plain, Size: 1147 bytes --]

On Tue, Feb 07, 2012 at 11:30:37AM +0200, Ilya Dryomov wrote:
> On Mon, Feb 06, 2012 at 11:20:18PM +0100, Goffredo Baroncelli wrote:
> > If exist a filesystem called 's' 
> > 
> > # btrfs fi bal s
> > 
> > could be:
> > 
> > 1) an error (missing parameters for the command "btrfs filesystem balance 
> > start/status" )
> > 2) the starting of the balance of the filesystem 's'
> 
> It's true, but you are supposed to look at the synopsis before using new
> commands.

   Haha. This is naive at best.

   I'm with Goffredo on this one -- I would much prefer to stick to
commands of precisely two words: a group and an action.

   That should mean that we have "filesystem balance" as a synonym for
"balance start", and all the other restriper functions under "balance
XYZ". Then the remaining parameter will always be the filesystem path.

   Hugo.

-- 
=== Hugo Mills: hugo@... carfax.org.uk | darksatanic.net | lug.org.uk ===
  PGP key: 515C238D from wwwkeys.eu.pgp.net or http://www.carfax.org.uk
   --- "How deep will this sub go?" "Oh,  she'll go all the way to ---   
                    the bottom if we don't stop her."                    

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 190 bytes --]