Message ID | 6af59460e12a8120bf643a923f5fa6bd3b135b20.1563600688.git.osandov@fb.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | btrfs-progs: fix clone from wrong subvolume | expand |
On Sat, Jul 20, 2019 at 3:34 PM Omar Sandoval <osandov@osandov.com> wrote: > > From: Omar Sandoval <osandov@fb.com> > > When we process a clone request, we look up the source subvolume by > UUID, even if the source is the subvolume that we're currently > receiving. Usually, this is fine. However, if for some reason we > previously received the same subvolume, then this will use paths > relative to the previously received subvolume instead of the current > one. This is incorrect, since the send stream may use temporary names > for the clone source. This can be reproduced as follows: > > btrfs subvol create subvol > dd if=/dev/urandom of=subvol/foo bs=1M count=1 > cp --reflink subvol/foo subvol/bar > mkdir subvol/dir > mv subvol/foo subvol/dir/ > btrfs property set subvol ro true > btrfs send -f stream subvol > mkdir first second > btrfs receive -f stream first > btrfs receive -f stream second > > The second receive results in this error: > > ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory > > Fix it by always cloning from the current subvolume if its UUID matches. > This has the nice side effect of avoiding unnecessary UUID tree lookups > in that case. Also, while we're here, get rid of some code that has been > commented out since it was added. > > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands") > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > cmds/receive.c | 34 ++++++++-------------------------- > 1 file changed, 8 insertions(+), 26 deletions(-) > > diff --git a/cmds/receive.c b/cmds/receive.c > index a3e62985..ed089af2 100644 > --- a/cmds/receive.c > +++ b/cmds/receive.c > @@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len, > if (ret < 0) > goto out; > > - si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, > - NULL, > - subvol_search_by_received_uuid); > - if (IS_ERR_OR_NULL(si)) { > - if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, > - BTRFS_UUID_SIZE) == 0) { > - /* TODO check generation of extent */ > - subvol_path = rctx->cur_subvol_path; > - } else { > + if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, > + BTRFS_UUID_SIZE) == 0) { > + subvol_path = rctx->cur_subvol_path; > + } else { > + si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, > + NULL, > + subvol_search_by_received_uuid); > + if (IS_ERR_OR_NULL(si)) { Hi Omar, This, and the change log, look good. > if (!si) > ret = -ENOENT; > else > @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len, > error("clone: did not find source subvol"); > goto out; > } > - } else { > - /*if (rs_args.ctransid > rs_args.rtransid) { > - if (!r->force) { > - ret = -EINVAL; > - fprintf(stderr, "ERROR: subvolume %s was " > - "modified after it was " > - "received.\n", > - r->subvol_parent_name); > - goto out; > - } else { > - fprintf(stderr, "WARNING: subvolume %s was " > - "modified after it was " > - "received.\n", > - r->subvol_parent_name); > - } > - }*/ > - Isn't this unrelated? Shouldn't go to a separate patch? Also, would you please create a test case as well? Thanks. > /* strip the subvolume that we are receiving to from the start of subvol_path */ > if (rctx->full_root_path) { > size_t root_len = strlen(rctx->full_root_path); > -- > 2.22.0 >
On Mon, Jul 22, 2019 at 01:16:35PM +0100, Filipe Manana wrote: > On Sat, Jul 20, 2019 at 3:34 PM Omar Sandoval <osandov@osandov.com> wrote: > > > > From: Omar Sandoval <osandov@fb.com> > > > > When we process a clone request, we look up the source subvolume by > > UUID, even if the source is the subvolume that we're currently > > receiving. Usually, this is fine. However, if for some reason we > > previously received the same subvolume, then this will use paths > > relative to the previously received subvolume instead of the current > > one. This is incorrect, since the send stream may use temporary names > > for the clone source. This can be reproduced as follows: > > > > btrfs subvol create subvol > > dd if=/dev/urandom of=subvol/foo bs=1M count=1 > > cp --reflink subvol/foo subvol/bar > > mkdir subvol/dir > > mv subvol/foo subvol/dir/ > > btrfs property set subvol ro true > > btrfs send -f stream subvol > > mkdir first second > > btrfs receive -f stream first > > btrfs receive -f stream second > > > > The second receive results in this error: > > > > ERROR: cannot open first/subvol/o259-7-0/foo: No such file or directory > > > > Fix it by always cloning from the current subvolume if its UUID matches. > > This has the nice side effect of avoiding unnecessary UUID tree lookups > > in that case. Also, while we're here, get rid of some code that has been > > commented out since it was added. > > > > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands") > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > --- > > cmds/receive.c | 34 ++++++++-------------------------- > > 1 file changed, 8 insertions(+), 26 deletions(-) > > > > diff --git a/cmds/receive.c b/cmds/receive.c > > index a3e62985..ed089af2 100644 > > --- a/cmds/receive.c > > +++ b/cmds/receive.c > > @@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len, > > if (ret < 0) > > goto out; > > > > - si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, > > - NULL, > > - subvol_search_by_received_uuid); > > - if (IS_ERR_OR_NULL(si)) { > > - if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, > > - BTRFS_UUID_SIZE) == 0) { > > - /* TODO check generation of extent */ > > - subvol_path = rctx->cur_subvol_path; > > - } else { > > + if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, > > + BTRFS_UUID_SIZE) == 0) { > > + subvol_path = rctx->cur_subvol_path; > > + } else { > > + si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, > > + NULL, > > + subvol_search_by_received_uuid); > > + if (IS_ERR_OR_NULL(si)) { > > Hi Omar, > > This, and the change log, look good. > > > if (!si) > > ret = -ENOENT; > > else > > @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len, > > error("clone: did not find source subvol"); > > goto out; > > } > > - } else { > > - /*if (rs_args.ctransid > rs_args.rtransid) { > > - if (!r->force) { > > - ret = -EINVAL; > > - fprintf(stderr, "ERROR: subvolume %s was " > > - "modified after it was " > > - "received.\n", > > - r->subvol_parent_name); > > - goto out; > > - } else { > > - fprintf(stderr, "WARNING: subvolume %s was " > > - "modified after it was " > > - "received.\n", > > - r->subvol_parent_name); > > - } > > - }*/ > > - > > Isn't this unrelated? Shouldn't go to a separate patch? It didn't seem worth it to make a separate patch when I'm moving this code around anyways, but I noticed that there's a similar check in another location, so I'll just make it a separate patch. > Also, would you please create a test case as well? Will do. Thanks!
diff --git a/cmds/receive.c b/cmds/receive.c index a3e62985..ed089af2 100644 --- a/cmds/receive.c +++ b/cmds/receive.c @@ -753,15 +753,14 @@ static int process_clone(const char *path, u64 offset, u64 len, if (ret < 0) goto out; - si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, - NULL, - subvol_search_by_received_uuid); - if (IS_ERR_OR_NULL(si)) { - if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, - BTRFS_UUID_SIZE) == 0) { - /* TODO check generation of extent */ - subvol_path = rctx->cur_subvol_path; - } else { + if (memcmp(clone_uuid, rctx->cur_subvol.received_uuid, + BTRFS_UUID_SIZE) == 0) { + subvol_path = rctx->cur_subvol_path; + } else { + si = subvol_uuid_search(&rctx->sus, 0, clone_uuid, clone_ctransid, + NULL, + subvol_search_by_received_uuid); + if (IS_ERR_OR_NULL(si)) { if (!si) ret = -ENOENT; else @@ -769,23 +768,6 @@ static int process_clone(const char *path, u64 offset, u64 len, error("clone: did not find source subvol"); goto out; } - } else { - /*if (rs_args.ctransid > rs_args.rtransid) { - if (!r->force) { - ret = -EINVAL; - fprintf(stderr, "ERROR: subvolume %s was " - "modified after it was " - "received.\n", - r->subvol_parent_name); - goto out; - } else { - fprintf(stderr, "WARNING: subvolume %s was " - "modified after it was " - "received.\n", - r->subvol_parent_name); - } - }*/ - /* strip the subvolume that we are receiving to from the start of subvol_path */ if (rctx->full_root_path) { size_t root_len = strlen(rctx->full_root_path);