On Mon, Jul 22, 2019 at 01:16:35PM +0100, Filipe Manana wrote:
> On Sat, Jul 20, 2019 at 3:34 PM Omar Sandoval <osan...@osandov.com> wrote:
> >
> > From: Omar Sandoval <osan...@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 <osan...@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!
