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

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

[Jan Schmidt]

Hi everybody,

We made a bad mistake with "btrfs send" command line arguments and we'd
better fix it before it's being widely used (read: *now*).

When using "btrfs send" as in the current master, the "-i" option does
*not* give you an incremental stream as you'd expect. There are two
problems in the back:

	- The "-i" option adds clone sources and btrfs determines itself
	  whether any of these should be used to generate an incremental
	  stream.

	- That determination is broken.

There is however a "-p" option to force generation of an incremental
stream. This is a must change in my opinion. We should be using "send
-i" in the same way "zfs send" is using it. I expect anything else to
cause wide confusion. Therefore, these 2 patches do the following:

	- Turn "-i" into "-r", which stands for "remote" or "receiving
	  side". The option is meant to tell btrfs which subvolumes
	  exist on the receiver.

	- Turn "-p" into "-i". Yes, this is a clash.

	- Fix the parent determination (required in combination with
	  the "-r" option) in a way, that we never overwrite a base for
	  an incremental snapshot given with "-i".

Outcome for people who are already used to the current way btrfs send
works:

	- "btrfs send -p [base] [subvol]"
	  This command now prints a fatal error message to use -i
	  instead.
	  NEW: "btrfs send -i [base] [subvol]"

	- "btrfs send -i [snap] [subvol]"
	  This command now does exactly what one would have expected
	  from the previous documentation: it should have automatically
	  determined [snap] as base for an incremental stream. Now it
	  really selects [snap] as base.

	- "btrfs send -p [base] -i [snap1] -i [snap2] [subvol]"
	  Prints the same error message as the first example.
	  NEW: "btrfs send -i [base] -r [snap1] -r [snap2] [subvol]"

	- "btrfs send -i [snap1] -i [snap2] [subvol]"
	  Previously, determination of the best base was likely to fail,
	  resulting in a full stream. Now you get a fatal error message
	  for specifying -i multiple times.
	  NEW: "btrfs send -r [snap1] -r [snap2] [subvol]"

Although this gives a change in command line options rather late in the
game, I'll emphasis again that I think it's a no-go to leave it as it
currently is. The outcome as outlined should be acceptable for anyone, I
don't see a case where this change does something completely different
after the change. Users will have to adapt to the corrected switches,
though.

For ease of management, you can fetch these patches from my git repo,
based on top of the current cmason/master:

	git://git.jan-o-sch.net/btrfs-progs for-chris

-Jan

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  |   97 +++++++++++++++++++++++++++++++---------------------------
 send-utils.c |    4 +-
 2 files changed, 54 insertions(+), 47 deletions(-)

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

[Jan Schmidt]

Unfortunately, the command line options for btrfs send were misnamed. To
specify a base for an incremental snapshot transfer, the best choice is -i
for "incremental" (was: -p). To optionally add snapshots existing on the
receiver as clone sources, the best choice is -c (was: -i).

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

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

diff --git a/cmds-send.c b/cmds-send.c
index 9b47e70..9db65e9 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 base_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 = base_root_id;
 	ret = ioctl(subvol_fd, BTRFS_IOC_SEND, &io_send);
 	if (ret) {
 		ret = -errno;
@@ -420,19 +420,20 @@ int cmd_send_start(int argc, char **argv)
 	struct btrfs_send send;
 	u32 i;
 	char *mount_root = NULL;
-	char *snapshot_parent = NULL;
+	char *incremental_base = NULL;
 	u64 root_id;
-	u64 parent_root_id = 0;
+	u64 base_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, "vf:i:p:r:")) != -1) {
 		switch (c) {
 		case 'v':
 			g_verbose++;
 			break;
-		case 'i': {
+		case 'r':
 			subvol = realpath(optarg, NULL);
 			if (!subvol) {
 				ret = -errno;
@@ -455,19 +456,26 @@ int cmd_send_start(int argc, char **argv)
 			add_clone_source(&send, root_id);
 			free(subvol);
 			break;
-		}
 		case 'f':
 			outname = optarg;
 			break;
-		case 'p':
-			snapshot_parent = realpath(optarg, NULL);
-			if (!snapshot_parent) {
+		case 'i':
+			if (incremental_base) {
+				fprintf(stderr, "ERROR: you cannot have more than one base for incremental send (-i)\n");
+				return 1;
+			}
+			incremental_base = realpath(optarg, NULL);
+			if (!incremental_base) {
 				ret = -errno;
 				fprintf(stderr, "ERROR: realpath %s failed. "
 						"%s\n", optarg, strerror(-ret));
 				goto out;
 			}
+			full_send = 0;
 			break;
+		case 'p':
+			fprintf(stderr, "ERROR: -p option was removed. use -i instead\n");
+			return 1;
 		case '?':
 		default:
 			fprintf(stderr, "ERROR: send args invalid.\n");
@@ -504,17 +512,17 @@ int cmd_send_start(int argc, char **argv)
 	if (ret < 0)
 		goto out;
 
-	if (snapshot_parent != NULL) {
+	if (incremental_base != NULL) {
 		ret = get_root_id(&send,
-				get_subvol_name(&send, snapshot_parent),
-				&parent_root_id);
+				get_subvol_name(&send, incremental_base),
+				&base_root_id);
 		if (ret < 0) {
 			fprintf(stderr, "ERROR: could not resolve root_id "
-					"for %s\n", snapshot_parent);
+					"for %s\n", incremental_base);
 			goto out;
 		}
 
-		add_clone_source(&send, parent_root_id);
+		add_clone_source(&send, base_root_id);
 	}
 
 	for (i = optind; i < argc; i++) {
@@ -573,10 +581,13 @@ int cmd_send_start(int argc, char **argv)
 			goto out;
 		}
 
-		if (!parent_root_id) {
-			ret = find_good_parent(&send, root_id, &parent_root_id);
-			if (ret < 0)
-				parent_root_id = 0;
+		if (!full_send && !base_root_id) {
+			ret = find_good_parent(&send, root_id, &base_root_id);
+			if (ret < 0) {
+				fprintf(stderr, "ERROR: parent determination failed for %lld\n",
+					root_id);
+				goto out;
+			}
 		}
 
 		ret = is_subvol_ro(&send, subvol);
@@ -589,14 +600,15 @@ int cmd_send_start(int argc, char **argv)
 			goto out;
 		}
 
-		ret = do_send(&send, root_id, parent_root_id);
+		ret = do_send(&send, root_id, base_root_id);
 		if (ret < 0)
 			goto out;
 
 		/* done with this subvol, so add it to the clone sources */
 		add_clone_source(&send, root_id);
 
-		parent_root_id = 0;
+		base_root_id = 0;
+		full_send = 0;
 		free(subvol);
 	}
 
@@ -614,32 +626,27 @@ 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] [-i <base>] [-r <snap>] <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 '-i <incremental-base>'. If you know the snapshots"
+	"available on the receiving side, use '-r <snap>' (multiple times",
+	"where applicable). This allows 'btrfs send' to clone from these",
+	"snapshots. You must not specify these unless you guarantee they are",
+	"exactly in the same state on both sides. It is allowed to omit the",
+	"'-i <base>' option when specifying one or more '-r <snap>' options,",
+	"in which case 'btrfs send' will determine the base itself."
+	"\n",
+	"-v               Enable verbose debug output. Each occurrency of",
+	"                 this option increases the verbose level more.",
+	"-i <base>        Send an incremental stream between <incr-base> and",
+	"                 <subvol>.",
+	"-r <snap>        This snapshot exists on the receiver exactly in the ",
+	"                 same state as on the sender. (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 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

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

[Chris Mason]

On Thu, Nov 01, 2012 at 09:01:24AM -0600, Jan Schmidt wrote:
> Hi everybody,
> 
> We made a bad mistake with "btrfs send" command line arguments and we'd
> better fix it before it's being widely used (read: *now*).

Ok, I do agree that -i was confusing.  I didn't end up using it in my
backup scripts here.

How about:

Make -p and -i mean the same thing.  Add -r for what -i should have
done.  

This has the advantage of not breaking the people that did get working
btrfs send setups ;)

-chris

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

[Jan Schmidt]

On Thu, November 01, 2012 at 16:07 (+0100), Chris Mason wrote:
> On Thu, Nov 01, 2012 at 09:01:24AM -0600, Jan Schmidt wrote:
>> Hi everybody,
>>
>> We made a bad mistake with "btrfs send" command line arguments and we'd
>> better fix it before it's being widely used (read: *now*).
> 
> Ok, I do agree that -i was confusing.  I didn't end up using it in my
> backup scripts here.

Good we agree here :-)

> How about:
> 
> Make -p and -i mean the same thing.  Add -r for what -i should have
> done.  
> 
> This has the advantage of not breaking the people that did get working
> btrfs send setups ;)

I'd carefully argue that we're still in the position to break things, because
the 3.7 kernel isn't released and you cannot use "btrfs send" without it. The
number of users should be really small.

I prefer having a clean and painful cut over suffering from bad decisions
forever. That may not be the most popular opinion in the world. In the end, I
could live with -p and -i doing the same thing.

-Jan

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

[Chris Mason]

On Thu, Nov 01, 2012 at 09:48:25AM -0600, Jan Schmidt wrote:
> On Thu, November 01, 2012 at 16:07 (+0100), Chris Mason wrote:
> > On Thu, Nov 01, 2012 at 09:01:24AM -0600, Jan Schmidt wrote:
> >> Hi everybody,
> >>
> >> We made a bad mistake with "btrfs send" command line arguments and we'd
> >> better fix it before it's being widely used (read: *now*).
> > 
> > Ok, I do agree that -i was confusing.  I didn't end up using it in my
> > backup scripts here.
> 
> Good we agree here :-)
> 
> > How about:
> > 
> > Make -p and -i mean the same thing.  Add -r for what -i should have
> > done.  
> > 
> > This has the advantage of not breaking the people that did get working
> > btrfs send setups ;)
> 
> I'd carefully argue that we're still in the position to break things, because
> the 3.7 kernel isn't released and you cannot use "btrfs send" without it. The
> number of users should be really small.
> 
> I prefer having a clean and painful cut over suffering from bad decisions
> forever. That may not be the most popular opinion in the world. In the end, I
> could live with -p and -i doing the same thing.

But we have a working -p, I'm not sure why we'd rename it to -i?  I'm
even fine with just flat out removing -i.  This is mostly because
--parent makes a lot of sense to me, but I'm more than open to other
ideas.

-chris