[v2,3/4] btrfs-progs: receive: don't lookup clone root for received subvolume

Commit Message

Omar Sandoval July 22, 2019, 7:15 p.m. UTC

From: Omar Sandoval <[e-mail]>

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 subvolume 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 send.data subvol
mkdir first second
btrfs receive -f send.data first
btrfs receive -f send.data 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.

Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
Signed-off-by: Omar Sandoval <[e-mail]>
---
 cmds/receive.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Filipe Manana July 23, 2019, 11:19 a.m. UTC | #1

On Tue, Jul 23, 2019 at 3:25 AM Omar Sandoval <[e-mail]> wrote:
>
> From: Omar Sandoval <[e-mail]>
>
> 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 subvolume 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 send.data subvol
> mkdir first second
> btrfs receive -f send.data first
> btrfs receive -f send.data 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.
>
> Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> Signed-off-by: Omar Sandoval <[e-mail]>

Reviewed-by: Filipe Manana <[e-mail]>

Thanks!

> ---
>  cmds/receive.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index dba05982..1e6a29dd 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -744,15 +744,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
> @@ -760,7 +759,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
>                         error("clone: did not find source subvol");
>                         goto out;
>                 }
> -       } else {
>                 /* 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
>

Filipe Manana Feb. 24, 2020, 2:53 p.m. UTC | #2

On Tue, Jul 23, 2019 at 12:19 PM Filipe Manana <[e-mail]> wrote:
>
> On Tue, Jul 23, 2019 at 3:25 AM Omar Sandoval <[e-mail]> wrote:
> >
> > From: Omar Sandoval <[e-mail]>
> >
> > 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 subvolume 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 send.data subvol
> > mkdir first second
> > btrfs receive -f send.data first
> > btrfs receive -f send.data 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.
> >
> > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> > Signed-off-by: Omar Sandoval <[e-mail]>
>
> Reviewed-by: Filipe Manana <[e-mail]>

I can't find this patch in btrfs-progs. Any reason why it was never applied?

thanks

>
> Thanks!
>
> > ---
> >  cmds/receive.c | 18 ++++++++----------
> >  1 file changed, 8 insertions(+), 10 deletions(-)
> >
> > diff --git a/cmds/receive.c b/cmds/receive.c
> > index dba05982..1e6a29dd 100644
> > --- a/cmds/receive.c
> > +++ b/cmds/receive.c
> > @@ -744,15 +744,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
> > @@ -760,7 +759,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
> >                         error("clone: did not find source subvol");
> >                         goto out;
> >                 }
> > -       } else {
> >                 /* 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
> >
>
>
> --
> Filipe David Manana,
>
> “Whether you think you can, or you think you can't — you're right.”

Omar Sandoval March 11, 2020, 8:03 a.m. UTC | #3

On Mon, Feb 24, 2020 at 02:53:51PM +0000, Filipe Manana wrote:
> On Tue, Jul 23, 2019 at 12:19 PM Filipe Manana <[e-mail]> wrote:
> >
> > On Tue, Jul 23, 2019 at 3:25 AM Omar Sandoval <[e-mail]> wrote:
> > >
> > > From: Omar Sandoval <[e-mail]>
> > >
> > > 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 subvolume 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 send.data subvol
> > > mkdir first second
> > > btrfs receive -f send.data first
> > > btrfs receive -f send.data 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.
> > >
> > > Fixes: f1c24cd80dfd ("Btrfs-progs: add btrfs send/receive commands")
> > > Signed-off-by: Omar Sandoval <[e-mail]>
> >
> > Reviewed-by: Filipe Manana <[e-mail]>
> 
> I can't find this patch in btrfs-progs. Any reason why it was never applied?
> 
> thanks

It must have gotten lost at some point. I'll resend it.

Patch

diff --git a/cmds/receive.c b/cmds/receive.c
index dba05982..1e6a29dd 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -744,15 +744,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
@@ -760,7 +759,6 @@  static int process_clone(const char *path, u64 offset, u64 len,
 			error("clone: did not find source subvol");
 			goto out;
 		}
-	} else {
 		/* 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);