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(-) -- 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
Jan Schmidt
2012-Nov-01 15:01 UTC
[PATCH 1/2] Btrfs-progs: correcting misnamed parameter options for btrfs send
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
--
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
Jan Schmidt
2012-Nov-01 15:01 UTC
[PATCH 2/2] Btrfs-progs: bugfix for subvolume parent determination in btrfs send
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
--
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
Chris Mason
2012-Nov-01 15:07 UTC
Re: [PATCH 0/2] Btrfs-progs: urgent fixes for btrfs send
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 -- 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
Jan Schmidt
2012-Nov-01 15:48 UTC
Re: [PATCH 0/2] Btrfs-progs: urgent fixes for btrfs send
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 -- 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
Chris Mason
2012-Nov-01 18:46 UTC
Re: [PATCH 0/2] Btrfs-progs: urgent fixes for btrfs send
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 -- 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