[PATCH v2 0/2] Btrfs-progs: urgent fixes for btrfs send

[PATCH v2 0/2] Btrfs-progs: urgent fixes for btrfs send

[Jan Schmidt]

After sending v1 some days ago, some onlist and offlist discussion has
brought us to the following conclusions:

	- we should fix it as soon as possible
	- we should stick to the -p option (parent)
	- we should rename the -i option (because it actually means
	  "clone source")

This is what v2 of this patch set is doing, besides fixing one and a
half bugs along the way: We turn -i into -c, emitting a fatal warning
should anybody use -i. As argued in the v1 cover letter, we should
really do this change now in order to avoid a lot of confused users that
"btrfs send -i" is doing things differently in a subtle way from what
"zfs send -i" is doing. We're better off coming up with our own
terminology, thus offering -p and -c options (both can be used to ask
for an incremental send stream, see the usage changes in patch 1),
without providing a "send -i" option at all.

Jan Schmidt (2):
  Btrfs-progs: correcting misnamed parameter options for btrfs send
  Btrfs-progs: bugfix for subvolume parent determination in btrfs send

 cmds-send.c  |   74 +++++++++++++++++++++++++++++++++-------------------------
 send-utils.c |    4 +-
 2 files changed, 44 insertions(+), 34 deletions(-)

[PATCH v2 1/2] Btrfs-progs: correcting misnamed parameter options for btrfs send

[Jan Schmidt]

Unfortunately, the command line options for btrfs send were misnamed. The
-i option should not be used to give "clone sources", we'll be using -c
instead.

Compatibily note: -i option was broken anyway, which makes it less critical
renaming it. For potential users of the old option style, we emit a fatal
warning if the -i option is used.

Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
---
 cmds-send.c |   74 +++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 42 insertions(+), 32 deletions(-)

diff --git a/cmds-send.c b/cmds-send.c
index 9b47e70..3e2fcbe 100644
--- a/cmds-send.c
+++ b/cmds-send.c
@@ -234,7 +234,7 @@ out:
 	return ERR_PTR(ret);
 }
 
-static int do_send(struct btrfs_send *send, u64 root_id, u64 parent_root)
+static int do_send(struct btrfs_send *send, u64 root_id, u64 parent_root_id)
 {
 	int ret;
 	pthread_t t_read;
@@ -286,7 +286,7 @@ static int do_send(struct btrfs_send *send, u64 root_id, u64 parent_root)
 
 	io_send.clone_sources = (__u64*)send->clone_sources;
 	io_send.clone_sources_count = send->clone_sources_count;
-	io_send.parent_root = parent_root;
+	io_send.parent_root = parent_root_id;
 	ret = ioctl(subvol_fd, BTRFS_IOC_SEND, &io_send);
 	if (ret) {
 		ret = -errno;
@@ -423,16 +423,17 @@ int cmd_send_start(int argc, char **argv)
 	char *snapshot_parent = NULL;
 	u64 root_id;
 	u64 parent_root_id = 0;
+	int full_send = 1;
 
 	memset(&send, 0, sizeof(send));
 	send.dump_fd = fileno(stdout);
 
-	while ((c = getopt(argc, argv, "vf:i:p:")) != -1) {
+	while ((c = getopt(argc, argv, "vc:f:i:p:")) != -1) {
 		switch (c) {
 		case 'v':
 			g_verbose++;
 			break;
-		case 'i': {
+		case 'c':
 			subvol = realpath(optarg, NULL);
 			if (!subvol) {
 				ret = -errno;
@@ -454,12 +455,16 @@ int cmd_send_start(int argc, char **argv)
 			}
 			add_clone_source(&send, root_id);
 			free(subvol);
+			full_send = 0;
 			break;
-		}
 		case 'f':
 			outname = optarg;
 			break;
 		case 'p':
+			if (snapshot_parent) {
+				fprintf(stderr, "ERROR: you cannot have more than one parent (-p)\n");
+				return 1;
+			}
 			snapshot_parent = realpath(optarg, NULL);
 			if (!snapshot_parent) {
 				ret = -errno;
@@ -467,7 +472,12 @@ int cmd_send_start(int argc, char **argv)
 						"%s\n", optarg, strerror(-ret));
 				goto out;
 			}
+			full_send = 0;
 			break;
+		case 'i':
+			fprintf(stderr,
+				"ERROR: -i was removed, use -c instead\n");
+			return 1;
 		case '?':
 		default:
 			fprintf(stderr, "ERROR: send args invalid.\n");
@@ -573,10 +583,13 @@ int cmd_send_start(int argc, char **argv)
 			goto out;
 		}
 
-		if (!parent_root_id) {
+		if (!full_send && !parent_root_id) {
 			ret = find_good_parent(&send, root_id, &parent_root_id);
-			if (ret < 0)
-				parent_root_id = 0;
+			if (ret < 0) {
+				fprintf(stderr, "ERROR: parent determination failed for %lld\n",
+					root_id);
+				goto out;
+			}
 		}
 
 		ret = is_subvol_ro(&send, subvol);
@@ -597,6 +610,7 @@ int cmd_send_start(int argc, char **argv)
 		add_clone_source(&send, root_id);
 
 		parent_root_id = 0;
+		full_send = 0;
 		free(subvol);
 	}
 
@@ -614,32 +628,28 @@ static const char * const send_cmd_group_usage[] = {
 };
 
 static const char * const cmd_send_usage[] = {
-	"btrfs send [-v] [-i <subvol>] [-p <parent>] <subvol>",
+	"btrfs send [-v] [-p <parent>] [-c <clone-src>] <subvol>",
 	"Send the subvolume to stdout.",
 	"Sends the subvolume specified by <subvol> to stdout.",
-	"By default, this will send the whole subvolume. To do",
-	"an incremental send, one or multiple '-i <clone_source>'",
-	"arguments have to be specified. A 'clone source' is",
-	"a subvolume that is known to exist on the receiving",
-	"side in exactly the same state as on the sending side.\n",
-	"Normally, a good snapshot parent is searched automatically",
-	"in the list of 'clone sources'. To override this, use",
-	"'-p <parent>' to manually specify a snapshot parent.",
-	"A manually specified snapshot parent is also regarded",
-	"as 'clone source'.\n",
-	"-v               Enable verbose debug output. Each",
-	"                 occurrency of this option increases the",
-	"                 verbose level more.",
-	"-i <subvol>      Informs btrfs send that this subvolume,",
-	"                 can be taken as 'clone source'. This can",
-	"                 be used for incremental sends.",
-	"-p <subvol>      Disable automatic snaphot parent",
-	"                 determination and use <subvol> as parent.",
-	"                 This subvolume is also added to the list",
-	"                 of 'clone sources' (see -i).",
-	"-f <outfile>     Output is normally written to stdout.",
-	"                 To write to a file, use this option.",
-	"                 An alternative would be to use pipes.",
+	"By default, this will send the whole subvolume. To do an incremental",
+	"send, use '-p <parent>'. If you want to allow btrfs to clone from",
+	"any additional local snapshots, use -c <clone-src> (multiple times",
+	"where applicable). You must not specify clone sources unless you",
+	"guarantee that these snapshots are exactly in the same state on both",
+	"sides, the sender and the receiver. It is allowed to omit the",
+	"'-p <parent>' option when '-c <clone-src>' options are given, in",
+	"which case 'btrfs send' will determine a suitable parent among the",
+	"clone sources itself.",
+	"\n",
+	"-v               Enable verbose debug output. Each occurrency of",
+	"                 this option increases the verbose level more.",
+	"-p <parent>      Send an incremental stream from <parent> to",
+	"                 <subvol>.",
+	"-c <clone-src>   Use this snapshot as a clone source for an ",
+	"                 incremental send (multiple allowed)",
+	"-f <outfile>     Output is normally written to stdout. To write to",
+	"                 a file, use this option. An alternative would be to",
+	"                 use pipes.",
 	NULL
 };
 
-- 
1.7.1

[PATCH v2 2/2] Btrfs-progs: bugfix for subvolume parent determination in btrfs send

[Jan Schmidt]

We missed to add the default subvolume, because it has no ROOT_BACKREF_ITEM.
This made get_parent always fail for direct decendants of the default
subvolume, resulting in lots of full streams where incremental streams were
requested.

Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
Reviewed-by: Alexander Block <ablock84@googlemail.com>
---
 send-utils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/send-utils.c b/send-utils.c
index fcde5c2..d8d3972 100644
--- a/send-utils.c
+++ b/send-utils.c
@@ -240,7 +240,8 @@ int subvol_uuid_search_init(int mnt_fd, struct subvol_uuid_search *s)
 				memcpy(&root_item, root_item_ptr,
 						sizeof(root_item));
 				root_item_valid = 1;
-			} else if (sh->type == BTRFS_ROOT_BACKREF_KEY) {
+			} else if (sh->type == BTRFS_ROOT_BACKREF_KEY ||
+				   root_item_valid) {
 				if (!root_item_valid)
 					goto skip;
 
@@ -274,7 +275,6 @@ int subvol_uuid_search_init(int mnt_fd, struct subvol_uuid_search *s)
 				subvol_uuid_search_add(s, si);
 				root_item_valid = 0;
 			} else {
-				root_item_valid = 0;
 				goto skip;
 			}
 
-- 
1.7.1