Re: [PATCH 2/2] btrfs-progs: receive: don't lookup clone root for received subvolume

[Omar Sandoval <osandov@osandov.com>]

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!