* [PATCH RESEND v2 0/3] btrfs-progs: fix clone from wrong subvolume
@ 2020-03-11 8:17 Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks Omar Sandoval
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Omar Sandoval @ 2020-03-11 8:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, Filipe Manana
From: Omar Sandoval <osandov@fb.com>
Hi,
This is a resend of [1] from last July fixing a bug when the same
subvolume is received multiple times. It appears that the series got
lost at some point.
The only changes from v2 are adding Filipe's reviewed-bys and removing
the strdup cleanup patch that was already merged. Based on the devel
branch.
Thanks!
1: https://lore.kernel.org/linux-btrfs/cover.1563822638.git.osandov@fb.com/
Omar Sandoval (3):
btrfs-progs: receive: remove commented out transid checks
btrfs-progs: receive: don't lookup clone root for received subvolume
btrfs-progs: tests: add test for receiving clone from duplicate
subvolume
cmds/receive.c | 43 ++++---------------
.../test.sh | 34 +++++++++++++++
2 files changed, 42 insertions(+), 35 deletions(-)
create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
--
2.25.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks
2020-03-11 8:17 [PATCH RESEND v2 0/3] btrfs-progs: fix clone from wrong subvolume Omar Sandoval
@ 2020-03-11 8:17 ` Omar Sandoval
2020-03-11 15:50 ` Filipe Manana
2020-03-11 8:17 ` [PATCH RESEND v2 2/3] btrfs-progs: receive: don't lookup clone root for received subvolume Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume Omar Sandoval
2 siblings, 1 reply; 6+ messages in thread
From: Omar Sandoval @ 2020-03-11 8:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, Filipe Manana
From: Omar Sandoval <osandov@fb.com>
The checks for a subvolume being modified after it was received have
been commented out since they were added back in commit f1c24cd80dfd
("Btrfs-progs: add btrfs send/receive commands"). Let's just get rid of
the noise.
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
cmds/receive.c | 25 -------------------------
1 file changed, 25 deletions(-)
diff --git a/cmds/receive.c b/cmds/receive.c
index c4827c1d..a4bf8787 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -345,15 +345,6 @@ static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid,
parent_subvol->path[sub_len - root_len - 1] = '\0';
}
}
- /*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);
- }
- }*/
if (*parent_subvol->path == 0)
args_v2.fd = dup(rctx->mnt_fd);
@@ -771,22 +762,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
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);
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RESEND v2 2/3] btrfs-progs: receive: don't lookup clone root for received subvolume
2020-03-11 8:17 [PATCH RESEND v2 0/3] btrfs-progs: fix clone from wrong subvolume Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks Omar Sandoval
@ 2020-03-11 8:17 ` Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume Omar Sandoval
2 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2020-03-11 8:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, Filipe Manana, Filipe Manana
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 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")
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
cmds/receive.c | 18 ++++++++----------
1 file changed, 8 insertions(+), 10 deletions(-)
diff --git a/cmds/receive.c b/cmds/receive.c
index a4bf8787..74b73f48 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -745,15 +745,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
@@ -761,7 +760,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.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume
2020-03-11 8:17 [PATCH RESEND v2 0/3] btrfs-progs: fix clone from wrong subvolume Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 2/3] btrfs-progs: receive: don't lookup clone root for received subvolume Omar Sandoval
@ 2020-03-11 8:17 ` Omar Sandoval
2020-03-23 20:01 ` David Sterba
2 siblings, 1 reply; 6+ messages in thread
From: Omar Sandoval @ 2020-03-11 8:17 UTC (permalink / raw)
To: linux-btrfs; +Cc: kernel-team, Filipe Manana, Filipe Manana
From: Omar Sandoval <osandov@fb.com>
This test case is the reproducer for the previous fix.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Signed-off-by: Omar Sandoval <osandov@fb.com>
---
.../test.sh | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
diff --git a/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh b/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
new file mode 100755
index 00000000..be648605
--- /dev/null
+++ b/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
@@ -0,0 +1,34 @@
+#!/bin/bash
+# Test that when receiving a subvolume whose received UUID already exists in
+# the filesystem, we clone from the correct source (the subvolume that we are
+# receiving, not the existing subvolume). This is a regression test for
+# "btrfs-progs: receive: don't lookup clone root for received subvolume".
+
+source "$TEST_TOP/common"
+
+check_prereq btrfs
+check_prereq mkfs.btrfs
+
+setup_root_helper
+
+rm -f disk
+run_check truncate -s 1G disk
+chmod a+w disk
+run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f disk
+run_check $SUDO_HELPER mount -o loop disk "$TEST_MNT"
+
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subvol"
+run_check $SUDO_HELPER dd if=/dev/urandom of="$TEST_MNT/subvol/foo" \
+ bs=1M count=1 status=none
+run_check $SUDO_HELPER cp --reflink "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/bar"
+run_check $SUDO_HELPER mkdir "$TEST_MNT/subvol/dir"
+run_check $SUDO_HELPER mv "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/dir"
+run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/subvol" ro true
+run_check $SUDO_HELPER "$TOP/btrfs" send -f send.data "$TEST_MNT/subvol"
+
+run_check $SUDO_HELPER mkdir "$TEST_MNT/first" "$TEST_MNT/second"
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/first"
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/second"
+
+run_check $SUDO_HELPER umount "$TEST_MNT"
+rm -f disk send.data
--
2.25.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks
2020-03-11 8:17 ` [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks Omar Sandoval
@ 2020-03-11 15:50 ` Filipe Manana
0 siblings, 0 replies; 6+ messages in thread
From: Filipe Manana @ 2020-03-11 15:50 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-btrfs, kernel-team
On Wed, Mar 11, 2020 at 8:17 AM Omar Sandoval <osandov@osandov.com> wrote:
>
> From: Omar Sandoval <osandov@fb.com>
>
> The checks for a subvolume being modified after it was received have
> been commented out since they were added back in commit f1c24cd80dfd
> ("Btrfs-progs: add btrfs send/receive commands"). Let's just get rid of
> the noise.
Indeed. If they were ever in place, it would have never been possible
to do an incremental send and running dedupe against the parent
snapshot.
That particular use case used to cause send, the kernel side, to fail
(initially with a BUG_ON() and later with -EIO returned to user
space):
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=b4f9a1a87a48c255bb90d8a6c3d555a1abb88130
If this code in btrfs-progs was not commented, it would have been
easier to find that problem.
Reviewed-by: Filipe Manana <fdmanana@suse.com>
Thanks.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> cmds/receive.c | 25 -------------------------
> 1 file changed, 25 deletions(-)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index c4827c1d..a4bf8787 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -345,15 +345,6 @@ static int process_snapshot(const char *path, const u8 *uuid, u64 ctransid,
> parent_subvol->path[sub_len - root_len - 1] = '\0';
> }
> }
> - /*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);
> - }
> - }*/
>
> if (*parent_subvol->path == 0)
> args_v2.fd = dup(rctx->mnt_fd);
> @@ -771,22 +762,6 @@ static int process_clone(const char *path, u64 offset, u64 len,
> 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);
> --
> 2.25.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume
2020-03-11 8:17 ` [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume Omar Sandoval
@ 2020-03-23 20:01 ` David Sterba
0 siblings, 0 replies; 6+ messages in thread
From: David Sterba @ 2020-03-23 20:01 UTC (permalink / raw)
To: Omar Sandoval; +Cc: linux-btrfs, kernel-team, Filipe Manana, Filipe Manana
On Wed, Mar 11, 2020 at 01:17:11AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> This test case is the reproducer for the previous fix.
>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Omar Sandoval <osandov@fb.com>
> ---
> .../test.sh | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100755 tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
>
> diff --git a/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh b/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
> new file mode 100755
> index 00000000..be648605
> --- /dev/null
> +++ b/tests/misc-tests/038-receive-clone-from-current-subvolume/test.sh
> @@ -0,0 +1,34 @@
> +#!/bin/bash
> +# Test that when receiving a subvolume whose received UUID already exists in
> +# the filesystem, we clone from the correct source (the subvolume that we are
> +# receiving, not the existing subvolume). This is a regression test for
> +# "btrfs-progs: receive: don't lookup clone root for received subvolume".
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq btrfs
> +check_prereq mkfs.btrfs
> +
> +setup_root_helper
> +
> +rm -f disk
> +run_check truncate -s 1G disk
> +chmod a+w disk
> +run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f disk
> +run_check $SUDO_HELPER mount -o loop disk "$TEST_MNT"
I don't see any special reason to use the loop device (like an
additional one to the default), so I think this should be fine using the
default image and the common helpers.
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subvol"
> +run_check $SUDO_HELPER dd if=/dev/urandom of="$TEST_MNT/subvol/foo" \
> + bs=1M count=1 status=none
> +run_check $SUDO_HELPER cp --reflink "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/bar"
> +run_check $SUDO_HELPER mkdir "$TEST_MNT/subvol/dir"
> +run_check $SUDO_HELPER mv "$TEST_MNT/subvol/foo" "$TEST_MNT/subvol/dir"
> +run_check $SUDO_HELPER "$TOP/btrfs" property set "$TEST_MNT/subvol" ro true
> +run_check $SUDO_HELPER "$TOP/btrfs" send -f send.data "$TEST_MNT/subvol"
> +
> +run_check $SUDO_HELPER mkdir "$TEST_MNT/first" "$TEST_MNT/second"
> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/first"
> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f send.data "$TEST_MNT/second"
All paths are inside $TEST_MNT, so a 'cd' into the directory would save
some typing. I'll fix it.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-23 20:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 8:17 [PATCH RESEND v2 0/3] btrfs-progs: fix clone from wrong subvolume Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 1/3] btrfs-progs: receive: remove commented out transid checks Omar Sandoval
2020-03-11 15:50 ` Filipe Manana
2020-03-11 8:17 ` [PATCH RESEND v2 2/3] btrfs-progs: receive: don't lookup clone root for received subvolume Omar Sandoval
2020-03-11 8:17 ` [PATCH RESEND v2 3/3] btrfs-progs: tests: add test for receiving clone from duplicate subvolume Omar Sandoval
2020-03-23 20:01 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).