[PATCH 0/2] btrfs-progs: receive: introduce new --clone-fallback option

[PATCH 0/2] btrfs-progs: receive: introduce new --clone-fallback option

[Qu Wenruo]

When parent stream and incremental stream are received with different
nodatasum mount options, any clone opeartion in the incremental stream
will be rejected by kernel.

There are more situations to cause clone failure, like receiving a stream
on a fs with different sectorsize.

Thus this patchset will introduce a new option, --clone-fallback, for
btrfs receive to fall back to buffered write when clone failed.

This fall back behavior will only happen if the new option is explicitly
specified, as such behavior can hide some send bugs, and under most sane
cases users don't need such option.

Also add a test case for the new option.

Changelog:
RFC->v1:
- Introduce a new option for the fallback behavior
  To avoid hide send bugs.

- Hide the warning message behind -v option
  Since we have a special option for it thus users are aware of what
  they are doing, there is no need to output such warning by default.

- Add a new test case for it

Qu Wenruo (2):
  btrfs-progs: receive: fallback to buffered copy if clone failed
  btrfs-progs: misc-tests: add test case for receive --clone-fallback

 Documentation/btrfs-receive.asciidoc          | 13 ++++
 cmds/receive.c                                | 60 ++++++++++++++++++-
 .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
 3 files changed, 130 insertions(+), 3 deletions(-)
 create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh

-- 
2.33.0

[PATCH 1/2] btrfs-progs: receive: fallback to buffered copy if clone failed

[Qu Wenruo]

[BUG]
There are two every basic send streams:
(a/m/ctime and uuid omitted)

  Stream 1: (Parent subvolume)
  subvol   ./parent_subv           transid=8
  chown    ./parent_subv/          gid=0 uid=0
  chmod    ./parent_subv/          mode=755
  utimes   ./parent_subv/
  mkfile   ./parent_subv/o257-7-0
  rename   ./parent_subv/o257-7-0  dest=./parent_subv/source
  utimes   ./parent_subv/
  write    ./parent_subv/source    offset=0 len=16384
  chown    ./parent_subv/source    gid=0 uid=0
  chmod    ./parent_subv/source    mode=600
  utimes   ./parent_subv/source

  Stream 2: (snapshot and clone)
  snapshot ./dest_subv             transid=14 parent_transid=10
  utimes   ./dest_subv/
  mkfile   ./dest_subv/o258-14-0
  rename   ./dest_subv/o258-14-0   dest=./dest_subv/reflink
  utimes   ./dest_subv/
  clone    ./dest_subv/reflink     offset=0 len=16384 from=./dest_subv/source clone_offset=0
  chown    ./dest_subv/reflink     gid=0 uid=0
  chmod    ./dest_subv/reflink     mode=600
  utimes   ./dest_subv/reflink

But if we receive the first stream with default mount, then remount to
nodatasum, and try to receive the second stream, it will fail:

 # mount /mnt/btrfs
 # btrfs receive -f ~/parent_stream /mnt/btrfs/
 At subvol parent_subv
 # mount -o remount,nodatasum /mnt/btrfs
 # btrfs receive -f ~/clone_stream /mnt/btrfs/
 At snapshot dest_subv
 ERROR: failed to clone extents to reflink: Invalid argument
 # echo $?
 1

[CAUSE]
Btrfs doesn't allow clone source and destination has different NODATASUM
flags.
This is to prevent a data extent to be owned by both NODATASUM inode and
regular DATASUM inode.

For above receive operations, the clone destination is inheriting the
NODATASUM flag from mount option, while the clone source has no
NODATASUM flag, thus preventing us from doing the clone.

[FIX]
Btrfs send/receive doesn't require the underlying inode has the same
flags (thus we can send from compressed extent and receive on a
non-compressed filesystem).

So here we add a new command line option, '--clone-fallback', to allow
btrfs-receive to fall back to buffered write to copy data from the
source file.

Since such behavior can result much less clone operations, which may not
be what the end users really want, and can hide bugs in send stream.
Thus this behavior must be explicitly specified by the new option.

And we will output a warning message each time such fallback is
triggered if the user wants extra debug output.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Documentation/btrfs-receive.asciidoc | 13 ++++++
 cmds/receive.c                       | 60 ++++++++++++++++++++++++++--
 2 files changed, 70 insertions(+), 3 deletions(-)

diff --git a/Documentation/btrfs-receive.asciidoc b/Documentation/btrfs-receive.asciidoc
index e4c4d2c0bf3d..3b88643abe5f 100644
--- a/Documentation/btrfs-receive.asciidoc
+++ b/Documentation/btrfs-receive.asciidoc
@@ -65,6 +65,19 @@ dump the stream metadata, one line per operation
 +
 Does not require the 'path' parameter. The filesystem remains unchanged.
 
+--clone-fallback::
+allow failed clone operation to fallback to buffer copy from source file.
++
+Clone operations have various requirement which can be affected by things like
+mount options (source file has no NODATASUM flag while current fs is mounted
+with NODATASUM), sectorsizes (the stream is from 4K sectorsize fs while the fs
+is in 64K page size).
++
+This option allows users to let receive to handle such failed clone with
+buffered copy from the source, at the cost of less clone operations and even
+some unexposed send bugs. Thus this behavior must be explicitly specified by
+the user.
+
 -q|--quiet::
 (deprecated) alias for global '-q' option
 
diff --git a/cmds/receive.c b/cmds/receive.c
index 48c774cea587..60691b9b61ae 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -76,6 +76,8 @@ struct btrfs_receive
 	struct subvol_uuid_search sus;
 
 	int honor_end_cmd;
+
+	bool clone_fallback;
 };
 
 static int finish_subvol(struct btrfs_receive *rctx)
@@ -705,6 +707,44 @@ out:
 	return ret;
 }
 
+static int buffered_copy(int src_fd, int dst_fd, u64 src_offset, u64 len,
+			 u64 dest_offset)
+{
+	unsigned char buf[SZ_32K];
+	u64 copied = 0;
+	int ret = 0;
+
+	while (copied < len) {
+		u32 copy_len = min_t(u32, ARRAY_SIZE(buf), len - copied);
+		u32 written = 0;
+		ssize_t read_size;
+
+		read_size = pread(src_fd, buf, copy_len, src_offset + copied);
+		if (read < 0) {
+			ret = -errno;
+			error("failed to read source file: %m");
+			return ret;
+		}
+
+		/* Write the buffer to dest file */
+		while (written < read_size) {
+			ssize_t write_size;
+
+			write_size = pwrite(dst_fd, buf + written,
+					read_size - written,
+					dest_offset + copied + written);
+			if (write_size < 0) {
+				ret = -errno;
+				error("failed to write source file: %m");
+				return ret;
+			}
+			written += write_size;
+		}
+		copied += read_size;
+	}
+	return ret;
+}
+
 static int process_clone(const char *path, u64 offset, u64 len,
 			 const u8 *clone_uuid, u64 clone_ctransid,
 			 const char *clone_path, u64 clone_offset,
@@ -788,8 +828,17 @@ static int process_clone(const char *path, u64 offset, u64 len,
 	ret = ioctl(rctx->write_fd, BTRFS_IOC_CLONE_RANGE, &clone_args);
 	if (ret < 0) {
 		ret = -errno;
-		error("failed to clone extents to %s: %m", path);
-		goto out;
+		if (ret != -EINVAL || !rctx->clone_fallback) {
+			error("failed to clone extents to %s: %m", path);
+			goto out;
+		}
+
+		if (bconf.verbose >= 2)
+			warning(
+		"failed to clone extents to %s, fallback to buffered write",
+				path);
+		ret = buffered_copy(clone_fd, rctx->write_fd, clone_offset,
+				    len, offset);
 	}
 
 out:
@@ -1238,11 +1287,13 @@ static int cmd_receive(const struct cmd_struct *cmd, int argc, char **argv)
 	optind = 0;
 	while (1) {
 		int c;
-		enum { GETOPT_VAL_DUMP = 257 };
+		enum { GETOPT_VAL_DUMP = 257, GETOPT_VAL_CLONE_FALLBACK };
 		static const struct option long_opts[] = {
 			{ "max-errors", required_argument, NULL, 'E' },
 			{ "chroot", no_argument, NULL, 'C' },
 			{ "dump", no_argument, NULL, GETOPT_VAL_DUMP },
+			{ "clone-fallback", no_argument, NULL,
+				GETOPT_VAL_CLONE_FALLBACK},
 			{ "quiet", no_argument, NULL, 'q' },
 			{ NULL, 0, NULL, 0 }
 		};
@@ -1286,6 +1337,9 @@ static int cmd_receive(const struct cmd_struct *cmd, int argc, char **argv)
 		case GETOPT_VAL_DUMP:
 			dump = 1;
 			break;
+		case GETOPT_VAL_CLONE_FALLBACK:
+			rctx.clone_fallback = true;
+			break;
 		default:
 			usage_unknown_option(cmd, argv);
 		}
-- 
2.33.0

[PATCH 2/2] btrfs-progs: misc-tests: add test case for receive --clone-fallback

[Qu Wenruo]

The new test case will create two send streams:

- parent_stream
  A full send stream.
  Contains one file, as clone source.

- clone_stream
  An incremental send stream.
  Contains one clone operation.

Then we will receive the parent_stream with default mount options, while
try to receive the clone_stream with nodatasum mount option.

This should result clone failure due to nodatasum flag mismatch.

Then check if the receive can continue with --clone-fallback option.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh

diff --git a/tests/misc-tests/049-receive-clone-fallback/test.sh b/tests/misc-tests/049-receive-clone-fallback/test.sh
new file mode 100755
index 000000000000..d383c0e08a68
--- /dev/null
+++ b/tests/misc-tests/049-receive-clone-fallback/test.sh
@@ -0,0 +1,60 @@
+#!/bin/bash
+# Verify that btrfs receive can fallback to buffered copy when clone
+# failed
+
+source "$TEST_TOP/common"
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+setup_root_helper
+prepare_test_dev
+
+tmp=$(mktemp -d --tmpdir btrfs-progs-send-stream.XXXXXX)
+
+# Create two send stream, one as the parent and the other as an incremental
+# stream with one clone operation.
+run_check_mkfs_test_dev
+run_check_mount_test_dev -o datacow,datasum
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv"
+run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=1 of="$TEST_MNT/subv/file1" 
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
+	"$TEST_MNT/snap1"
+run_check $SUDO_HELPER cp --reflink=always "$TEST_MNT/subv/file1" \
+	"$TEST_MNT/subv/file2"
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
+	"$TEST_MNT/snap2"
+
+run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/parent_stream" \
+	"$TEST_MNT/snap1"
+run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/clone_stream" \
+	-p "$TEST_MNT/snap1" "$TEST_MNT/snap2"
+
+run_check_umount_test_dev
+run_check_mkfs_test_dev
+
+# Now we have the needed stream, try to receive them with different mount
+# options
+run_check_mount_test_dev -o datacow -o datasum
+
+# Receiving the full stream should not fail
+run_check $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/parent_stream" "$TEST_MNT"
+
+# No remount helper, so here we manually unmoutn and re-mount with different
+# nodatasum option
+run_check_umount_test_dev
+run_check_mount_test_dev -o datacow,nodatasum
+
+# Receiving incremental send stream without --clone-fallback should fail.
+# As the clone source and destination have different NODATASUM flags
+run_mustfail "receiving clone with different NODATASUM should fail" \
+	$SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/clone_stream" "$TEST_MNT"
+
+# Firstly we need to cleanup the partially received subvolume
+run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete "$TEST_MNT/snap2"
+
+# With --clone-fallback specified, the receive should finish without problem
+run_check $SUDO_HELPER "$TOP/btrfs" receive --clone-fallback \
+	-f "$tmp/clone_stream" "$TEST_MNT"
+run_check_umount_test_dev
+
+rm -rf -- "$tmp"
-- 
2.33.0

Re: [PATCH 1/2] btrfs-progs: receive: fallback to buffered copy if clone failed

[Filipe Manana]

On Thu, Sep 30, 2021 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> There are two every basic send streams:

every -> very

> (a/m/ctime and uuid omitted)
>
>   Stream 1: (Parent subvolume)
>   subvol   ./parent_subv           transid=8
>   chown    ./parent_subv/          gid=0 uid=0
>   chmod    ./parent_subv/          mode=755
>   utimes   ./parent_subv/
>   mkfile   ./parent_subv/o257-7-0
>   rename   ./parent_subv/o257-7-0  dest=./parent_subv/source
>   utimes   ./parent_subv/
>   write    ./parent_subv/source    offset=0 len=16384
>   chown    ./parent_subv/source    gid=0 uid=0
>   chmod    ./parent_subv/source    mode=600
>   utimes   ./parent_subv/source
>
>   Stream 2: (snapshot and clone)
>   snapshot ./dest_subv             transid=14 parent_transid=10
>   utimes   ./dest_subv/
>   mkfile   ./dest_subv/o258-14-0
>   rename   ./dest_subv/o258-14-0   dest=./dest_subv/reflink
>   utimes   ./dest_subv/
>   clone    ./dest_subv/reflink     offset=0 len=16384 from=./dest_subv/source clone_offset=0
>   chown    ./dest_subv/reflink     gid=0 uid=0
>   chmod    ./dest_subv/reflink     mode=600
>   utimes   ./dest_subv/reflink
>
> But if we receive the first stream with default mount, then remount to
> nodatasum, and try to receive the second stream, it will fail:
>
>  # mount /mnt/btrfs
>  # btrfs receive -f ~/parent_stream /mnt/btrfs/
>  At subvol parent_subv
>  # mount -o remount,nodatasum /mnt/btrfs
>  # btrfs receive -f ~/clone_stream /mnt/btrfs/
>  At snapshot dest_subv
>  ERROR: failed to clone extents to reflink: Invalid argument
>  # echo $?
>  1
>
> [CAUSE]
> Btrfs doesn't allow clone source and destination has different NODATASUM

... and destination files have different ...

> flags.
> This is to prevent a data extent to be owned by both NODATASUM inode and
> regular DATASUM inode.
>
> For above receive operations, the clone destination is inheriting the
> NODATASUM flag from mount option, while the clone source has no
> NODATASUM flag, thus preventing us from doing the clone.
>
> [FIX]
> Btrfs send/receive doesn't require the underlying inode has the same
> flags (thus we can send from compressed extent and receive on a
> non-compressed filesystem).
>
> So here we add a new command line option, '--clone-fallback', to allow
> btrfs-receive to fall back to buffered write to copy data from the
> source file.
>
> Since such behavior can result much less clone operations, which may not
> be what the end users really want, and can hide bugs in send stream.
> Thus this behavior must be explicitly specified by the new option.
>
> And we will output a warning message each time such fallback is
> triggered if the user wants extra debug output.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  Documentation/btrfs-receive.asciidoc | 13 ++++++
>  cmds/receive.c                       | 60 ++++++++++++++++++++++++++--
>  2 files changed, 70 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/btrfs-receive.asciidoc b/Documentation/btrfs-receive.asciidoc
> index e4c4d2c0bf3d..3b88643abe5f 100644
> --- a/Documentation/btrfs-receive.asciidoc
> +++ b/Documentation/btrfs-receive.asciidoc
> @@ -65,6 +65,19 @@ dump the stream metadata, one line per operation
>  +
>  Does not require the 'path' parameter. The filesystem remains unchanged.
>
> +--clone-fallback::
> +allow failed clone operation to fallback to buffer copy from source file.

operation -> operations

buffer -> buffered

For this type o documentation, I don't think it's relevant to mention
it's doing buffered IO.

Something like:

when clone operations fail, attempt to directly copy the data instead

> ++
> +Clone operations have various requirement which can be affected by things like
> +mount options (source file has no NODATASUM flag while current fs is mounted
> +with NODATASUM), sectorsizes (the stream is from 4K sectorsize fs while the fs
> +is in 64K page size).

As this is documentation and not code, it should be "sector size", not
sectorsize.
It should also use "filesystem" instead of "fs", to be more clear for
users and consistent with the rest of the man page.

This also misses mentioning the nodatacow case.

Maybe something like:

"When the source and destination filesystems have a different sector
size or when only one of them
was mounted with either the 'nodatacow' or the 'nodatasum' mount
option, clone operations can fail."

It's not complete either, as nodatacow (and therefore nodatasum) might
have been set through chattr and not through mount options, but I
think many users will be able to infer that due to the previous
mention of 'nodatacow'.

> ++
> +This option allows users to let receive to handle such failed clone with
> +buffered copy from the source, at the cost of less clone operations and even
> +some unexposed send bugs. Thus this behavior must be explicitly specified by
> +the user.

"and even some unexposed send bugs" - This is confusing and scary, if
I were a user, most likely I would not use a tool mentioning something
like that in its documentation, except for testing.

We don't know if we currently have any bugs that result in the kernel
issuing invalid clone operations, so that should not be mentioned as
it makes no sense.


Maybe something like:

"This option makes the receive process attempt to manually copy data
from the source to the destination file when a
clone operation fails due to the cases mentioned before. When this
happens, extents will end up not being shared between the files."

> +
>  -q|--quiet::
>  (deprecated) alias for global '-q' option
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index 48c774cea587..60691b9b61ae 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -76,6 +76,8 @@ struct btrfs_receive
>         struct subvol_uuid_search sus;
>
>         int honor_end_cmd;
> +
> +       bool clone_fallback;
>  };
>
>  static int finish_subvol(struct btrfs_receive *rctx)
> @@ -705,6 +707,44 @@ out:
>         return ret;
>  }
>
> +static int buffered_copy(int src_fd, int dst_fd, u64 src_offset, u64 len,
> +                        u64 dest_offset)
> +{
> +       unsigned char buf[SZ_32K];
> +       u64 copied = 0;
> +       int ret = 0;
> +
> +       while (copied < len) {
> +               u32 copy_len = min_t(u32, ARRAY_SIZE(buf), len - copied);
> +               u32 written = 0;
> +               ssize_t read_size;
> +
> +               read_size = pread(src_fd, buf, copy_len, src_offset + copied);
> +               if (read < 0) {
> +                       ret = -errno;
> +                       error("failed to read source file: %m");
> +                       return ret;
> +               }
> +
> +               /* Write the buffer to dest file */
> +               while (written < read_size) {
> +                       ssize_t write_size;
> +
> +                       write_size = pwrite(dst_fd, buf + written,
> +                                       read_size - written,
> +                                       dest_offset + copied + written);
> +                       if (write_size < 0) {
> +                               ret = -errno;
> +                               error("failed to write source file: %m");
> +                               return ret;
> +                       }
> +                       written += write_size;
> +               }
> +               copied += read_size;
> +       }
> +       return ret;
> +}
> +
>  static int process_clone(const char *path, u64 offset, u64 len,
>                          const u8 *clone_uuid, u64 clone_ctransid,
>                          const char *clone_path, u64 clone_offset,
> @@ -788,8 +828,17 @@ static int process_clone(const char *path, u64 offset, u64 len,
>         ret = ioctl(rctx->write_fd, BTRFS_IOC_CLONE_RANGE, &clone_args);
>         if (ret < 0) {
>                 ret = -errno;
> -               error("failed to clone extents to %s: %m", path);
> -               goto out;
> +               if (ret != -EINVAL || !rctx->clone_fallback) {
> +                       error("failed to clone extents to %s: %m", path);
> +                       goto out;
> +               }
> +
> +               if (bconf.verbose >= 2)
> +                       warning(
> +               "failed to clone extents to %s, fallback to buffered write",
> +                               path);
> +               ret = buffered_copy(clone_fd, rctx->write_fd, clone_offset,
> +                                   len, offset);
>         }
>
>  out:
> @@ -1238,11 +1287,13 @@ static int cmd_receive(const struct cmd_struct *cmd, int argc, char **argv)
>         optind = 0;
>         while (1) {
>                 int c;
> -               enum { GETOPT_VAL_DUMP = 257 };
> +               enum { GETOPT_VAL_DUMP = 257, GETOPT_VAL_CLONE_FALLBACK };
>                 static const struct option long_opts[] = {
>                         { "max-errors", required_argument, NULL, 'E' },
>                         { "chroot", no_argument, NULL, 'C' },
>                         { "dump", no_argument, NULL, GETOPT_VAL_DUMP },
> +                       { "clone-fallback", no_argument, NULL,
> +                               GETOPT_VAL_CLONE_FALLBACK},

The option is not listed and summarized at "cmd_receive_usage" (--help output).
It should be.

Code wise, it looks good, thanks.

>                         { "quiet", no_argument, NULL, 'q' },
>                         { NULL, 0, NULL, 0 }
>                 };
> @@ -1286,6 +1337,9 @@ static int cmd_receive(const struct cmd_struct *cmd, int argc, char **argv)
>                 case GETOPT_VAL_DUMP:
>                         dump = 1;
>                         break;
> +               case GETOPT_VAL_CLONE_FALLBACK:
> +                       rctx.clone_fallback = true;
> +                       break;
>                 default:
>                         usage_unknown_option(cmd, argv);
>                 }
> --
> 2.33.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/2] btrfs-progs: misc-tests: add test case for receive --clone-fallback

[Filipe Manana]

On Thu, Sep 30, 2021 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
>
> The new test case will create two send streams:
>
> - parent_stream
>   A full send stream.
>   Contains one file, as clone source.
>
> - clone_stream
>   An incremental send stream.
>   Contains one clone operation.
>
> Then we will receive the parent_stream with default mount options, while
> try to receive the clone_stream with nodatasum mount option.
>
> This should result clone failure due to nodatasum flag mismatch.
>
> Then check if the receive can continue with --clone-fallback option.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
>  1 file changed, 60 insertions(+)
>  create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh
>
> diff --git a/tests/misc-tests/049-receive-clone-fallback/test.sh b/tests/misc-tests/049-receive-clone-fallback/test.sh
> new file mode 100755
> index 000000000000..d383c0e08a68
> --- /dev/null
> +++ b/tests/misc-tests/049-receive-clone-fallback/test.sh
> @@ -0,0 +1,60 @@
> +#!/bin/bash
> +# Verify that btrfs receive can fallback to buffered copy when clone
> +# failed
> +
> +source "$TEST_TOP/common"
> +
> +check_prereq mkfs.btrfs
> +check_prereq btrfs
> +setup_root_helper
> +prepare_test_dev
> +
> +tmp=$(mktemp -d --tmpdir btrfs-progs-send-stream.XXXXXX)
> +
> +# Create two send stream, one as the parent and the other as an incremental

stream -> streams

> +# stream with one clone operation.
> +run_check_mkfs_test_dev
> +run_check_mount_test_dev -o datacow,datasum
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv"

You can use the default subvolume and therefore avoid creating a
subvolume and making the test longer than needed.
Your call.

> +run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=1 of="$TEST_MNT/subv/file1"
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
> +       "$TEST_MNT/snap1"
> +run_check $SUDO_HELPER cp --reflink=always "$TEST_MNT/subv/file1" \
> +       "$TEST_MNT/subv/file2"
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
> +       "$TEST_MNT/snap2"
> +
> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/parent_stream" \
> +       "$TEST_MNT/snap1"
> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/clone_stream" \
> +       -p "$TEST_MNT/snap1" "$TEST_MNT/snap2"
> +
> +run_check_umount_test_dev
> +run_check_mkfs_test_dev
> +
> +# Now we have the needed stream, try to receive them with different mount

Reading this is confusing, it mentions receiving them with different
mount options, but they are the same for the first receive.

> +# options
> +run_check_mount_test_dev -o datacow -o datasum
> +
> +# Receiving the full stream should not fail
> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/parent_stream" "$TEST_MNT"
> +
> +# No remount helper, so here we manually unmoutn and re-mount with different
> +# nodatasum option

Seems pointless to mention there's a lack of a remount helper in the
test framework.
Just say that now we mount the filesystem with nodatasum so that the
new file received through the incremental stream ends up with the
nodatasum flag set.

Thanks.

> +run_check_umount_test_dev
> +run_check_mount_test_dev -o datacow,nodatasum
> +
> +# Receiving incremental send stream without --clone-fallback should fail.
> +# As the clone source and destination have different NODATASUM flags
> +run_mustfail "receiving clone with different NODATASUM should fail" \
> +       $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/clone_stream" "$TEST_MNT"
> +
> +# Firstly we need to cleanup the partially received subvolume
> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete "$TEST_MNT/snap2"
> +
> +# With --clone-fallback specified, the receive should finish without problem
> +run_check $SUDO_HELPER "$TOP/btrfs" receive --clone-fallback \
> +       -f "$tmp/clone_stream" "$TEST_MNT"
> +run_check_umount_test_dev
> +
> +rm -rf -- "$tmp"
> --
> 2.33.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/2] btrfs-progs: misc-tests: add test case for receive --clone-fallback

[Qu Wenruo]

On 2021/9/30 18:03, Filipe Manana wrote:
> On Thu, Sep 30, 2021 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
>>
>> The new test case will create two send streams:
>>
>> - parent_stream
>>    A full send stream.
>>    Contains one file, as clone source.
>>
>> - clone_stream
>>    An incremental send stream.
>>    Contains one clone operation.
>>
>> Then we will receive the parent_stream with default mount options, while
>> try to receive the clone_stream with nodatasum mount option.
>>
>> This should result clone failure due to nodatasum flag mismatch.
>>
>> Then check if the receive can continue with --clone-fallback option.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
>>   1 file changed, 60 insertions(+)
>>   create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh
>>
>> diff --git a/tests/misc-tests/049-receive-clone-fallback/test.sh b/tests/misc-tests/049-receive-clone-fallback/test.sh
>> new file mode 100755
>> index 000000000000..d383c0e08a68
>> --- /dev/null
>> +++ b/tests/misc-tests/049-receive-clone-fallback/test.sh
>> @@ -0,0 +1,60 @@
>> +#!/bin/bash
>> +# Verify that btrfs receive can fallback to buffered copy when clone
>> +# failed
>> +
>> +source "$TEST_TOP/common"
>> +
>> +check_prereq mkfs.btrfs
>> +check_prereq btrfs
>> +setup_root_helper
>> +prepare_test_dev
>> +
>> +tmp=$(mktemp -d --tmpdir btrfs-progs-send-stream.XXXXXX)
>> +
>> +# Create two send stream, one as the parent and the other as an incremental
>
> stream -> streams
>
>> +# stream with one clone operation.
>> +run_check_mkfs_test_dev
>> +run_check_mount_test_dev -o datacow,datasum
>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv"
>
> You can use the default subvolume and therefore avoid creating a
> subvolume and making the test longer than needed.
> Your call.

I understand we can use the default fs tree, but I just can't find my
mind at peace when doing snapshoting of fs tree.

It always remind me of the bad memories using hacky way to solve the
qgroup problem for such snapshot.

Thus I always try to avoid snapshotting fs tree.

>
>> +run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=1 of="$TEST_MNT/subv/file1"
>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
>> +       "$TEST_MNT/snap1"
>> +run_check $SUDO_HELPER cp --reflink=always "$TEST_MNT/subv/file1" \
>> +       "$TEST_MNT/subv/file2"
>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
>> +       "$TEST_MNT/snap2"
>> +
>> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/parent_stream" \
>> +       "$TEST_MNT/snap1"
>> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/clone_stream" \
>> +       -p "$TEST_MNT/snap1" "$TEST_MNT/snap2"
>> +
>> +run_check_umount_test_dev
>> +run_check_mkfs_test_dev
>> +
>> +# Now we have the needed stream, try to receive them with different mount
>
> Reading this is confusing, it mentions receiving them with different
> mount options, but they are the same for the first receive.
>
>> +# options
>> +run_check_mount_test_dev -o datacow -o datasum
>> +
>> +# Receiving the full stream should not fail
>> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/parent_stream" "$TEST_MNT"
>> +
>> +# No remount helper, so here we manually unmoutn and re-mount with different
>> +# nodatasum option
>
> Seems pointless to mention there's a lack of a remount helper in the
> test framework.
> Just say that now we mount the filesystem with nodatasum so that the
> new file received through the incremental stream ends up with the
> nodatasum flag set.

Or maybe I should just add run_check_remount_test().

Thanks for the review,
Qu

>
> Thanks.
>
>> +run_check_umount_test_dev
>> +run_check_mount_test_dev -o datacow,nodatasum
>> +
>> +# Receiving incremental send stream without --clone-fallback should fail.
>> +# As the clone source and destination have different NODATASUM flags
>> +run_mustfail "receiving clone with different NODATASUM should fail" \
>> +       $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/clone_stream" "$TEST_MNT"
>> +
>> +# Firstly we need to cleanup the partially received subvolume
>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete "$TEST_MNT/snap2"
>> +
>> +# With --clone-fallback specified, the receive should finish without problem
>> +run_check $SUDO_HELPER "$TOP/btrfs" receive --clone-fallback \
>> +       -f "$tmp/clone_stream" "$TEST_MNT"
>> +run_check_umount_test_dev
>> +
>> +rm -rf -- "$tmp"
>> --
>> 2.33.0
>>
>
>

Re: [PATCH 2/2] btrfs-progs: misc-tests: add test case for receive --clone-fallback

[Filipe Manana]

On Thu, Sep 30, 2021 at 11:18 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> On 2021/9/30 18:03, Filipe Manana wrote:
> > On Thu, Sep 30, 2021 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
> >>
> >> The new test case will create two send streams:
> >>
> >> - parent_stream
> >>    A full send stream.
> >>    Contains one file, as clone source.
> >>
> >> - clone_stream
> >>    An incremental send stream.
> >>    Contains one clone operation.
> >>
> >> Then we will receive the parent_stream with default mount options, while
> >> try to receive the clone_stream with nodatasum mount option.
> >>
> >> This should result clone failure due to nodatasum flag mismatch.
> >>
> >> Then check if the receive can continue with --clone-fallback option.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>   .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
> >>   1 file changed, 60 insertions(+)
> >>   create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh
> >>
> >> diff --git a/tests/misc-tests/049-receive-clone-fallback/test.sh b/tests/misc-tests/049-receive-clone-fallback/test.sh
> >> new file mode 100755
> >> index 000000000000..d383c0e08a68
> >> --- /dev/null
> >> +++ b/tests/misc-tests/049-receive-clone-fallback/test.sh
> >> @@ -0,0 +1,60 @@
> >> +#!/bin/bash
> >> +# Verify that btrfs receive can fallback to buffered copy when clone
> >> +# failed
> >> +
> >> +source "$TEST_TOP/common"
> >> +
> >> +check_prereq mkfs.btrfs
> >> +check_prereq btrfs
> >> +setup_root_helper
> >> +prepare_test_dev
> >> +
> >> +tmp=$(mktemp -d --tmpdir btrfs-progs-send-stream.XXXXXX)
> >> +
> >> +# Create two send stream, one as the parent and the other as an incremental
> >
> > stream -> streams
> >
> >> +# stream with one clone operation.
> >> +run_check_mkfs_test_dev
> >> +run_check_mount_test_dev -o datacow,datasum
> >> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv"
> >
> > You can use the default subvolume and therefore avoid creating a
> > subvolume and making the test longer than needed.
> > Your call.
>
> I understand we can use the default fs tree, but I just can't find my
> mind at peace when doing snapshoting of fs tree.
>
> It always remind me of the bad memories using hacky way to solve the
> qgroup problem for such snapshot.

I don't get it.
The fs tree is a subvolume like any other, it was always possible to
create snapshots of it, and snapshotting it is done the same way as
for any other subvolume (both in terms of api and at the
implementation level).
So I don't see anything "hacky" about it, and it feels very natural and common.

So I don't understand the relation with solving some qgroup related
problems in a "hacky" way.

You can leave it though.

>
> Thus I always try to avoid snapshotting fs tree.
>
> >
> >> +run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=1 of="$TEST_MNT/subv/file1"
> >> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
> >> +       "$TEST_MNT/snap1"
> >> +run_check $SUDO_HELPER cp --reflink=always "$TEST_MNT/subv/file1" \
> >> +       "$TEST_MNT/subv/file2"
> >> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
> >> +       "$TEST_MNT/snap2"
> >> +
> >> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/parent_stream" \
> >> +       "$TEST_MNT/snap1"
> >> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/clone_stream" \
> >> +       -p "$TEST_MNT/snap1" "$TEST_MNT/snap2"
> >> +
> >> +run_check_umount_test_dev
> >> +run_check_mkfs_test_dev
> >> +
> >> +# Now we have the needed stream, try to receive them with different mount
> >
> > Reading this is confusing, it mentions receiving them with different
> > mount options, but they are the same for the first receive.
> >
> >> +# options
> >> +run_check_mount_test_dev -o datacow -o datasum
> >> +
> >> +# Receiving the full stream should not fail
> >> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/parent_stream" "$TEST_MNT"
> >> +
> >> +# No remount helper, so here we manually unmoutn and re-mount with different
> >> +# nodatasum option
> >
> > Seems pointless to mention there's a lack of a remount helper in the
> > test framework.
> > Just say that now we mount the filesystem with nodatasum so that the
> > new file received through the incremental stream ends up with the
> > nodatasum flag set.
>
> Or maybe I should just add run_check_remount_test().
>
> Thanks for the review,
> Qu
>
> >
> > Thanks.
> >
> >> +run_check_umount_test_dev
> >> +run_check_mount_test_dev -o datacow,nodatasum
> >> +
> >> +# Receiving incremental send stream without --clone-fallback should fail.
> >> +# As the clone source and destination have different NODATASUM flags
> >> +run_mustfail "receiving clone with different NODATASUM should fail" \
> >> +       $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/clone_stream" "$TEST_MNT"
> >> +
> >> +# Firstly we need to cleanup the partially received subvolume
> >> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete "$TEST_MNT/snap2"
> >> +
> >> +# With --clone-fallback specified, the receive should finish without problem
> >> +run_check $SUDO_HELPER "$TOP/btrfs" receive --clone-fallback \
> >> +       -f "$tmp/clone_stream" "$TEST_MNT"
> >> +run_check_umount_test_dev
> >> +
> >> +rm -rf -- "$tmp"
> >> --
> >> 2.33.0
> >>
> >
> >



-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 2/2] btrfs-progs: misc-tests: add test case for receive --clone-fallback

[Qu Wenruo]

On 2021/9/30 18:30, Filipe Manana wrote:
> On Thu, Sep 30, 2021 at 11:18 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> On 2021/9/30 18:03, Filipe Manana wrote:
>>> On Thu, Sep 30, 2021 at 1:06 AM Qu Wenruo <wqu@suse.com> wrote:
>>>>
>>>> The new test case will create two send streams:
>>>>
>>>> - parent_stream
>>>>     A full send stream.
>>>>     Contains one file, as clone source.
>>>>
>>>> - clone_stream
>>>>     An incremental send stream.
>>>>     Contains one clone operation.
>>>>
>>>> Then we will receive the parent_stream with default mount options, while
>>>> try to receive the clone_stream with nodatasum mount option.
>>>>
>>>> This should result clone failure due to nodatasum flag mismatch.
>>>>
>>>> Then check if the receive can continue with --clone-fallback option.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    .../049-receive-clone-fallback/test.sh        | 60 +++++++++++++++++++
>>>>    1 file changed, 60 insertions(+)
>>>>    create mode 100755 tests/misc-tests/049-receive-clone-fallback/test.sh
>>>>
>>>> diff --git a/tests/misc-tests/049-receive-clone-fallback/test.sh b/tests/misc-tests/049-receive-clone-fallback/test.sh
>>>> new file mode 100755
>>>> index 000000000000..d383c0e08a68
>>>> --- /dev/null
>>>> +++ b/tests/misc-tests/049-receive-clone-fallback/test.sh
>>>> @@ -0,0 +1,60 @@
>>>> +#!/bin/bash
>>>> +# Verify that btrfs receive can fallback to buffered copy when clone
>>>> +# failed
>>>> +
>>>> +source "$TEST_TOP/common"
>>>> +
>>>> +check_prereq mkfs.btrfs
>>>> +check_prereq btrfs
>>>> +setup_root_helper
>>>> +prepare_test_dev
>>>> +
>>>> +tmp=$(mktemp -d --tmpdir btrfs-progs-send-stream.XXXXXX)
>>>> +
>>>> +# Create two send stream, one as the parent and the other as an incremental
>>>
>>> stream -> streams
>>>
>>>> +# stream with one clone operation.
>>>> +run_check_mkfs_test_dev
>>>> +run_check_mount_test_dev -o datacow,datasum
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume create "$TEST_MNT/subv"
>>>
>>> You can use the default subvolume and therefore avoid creating a
>>> subvolume and making the test longer than needed.
>>> Your call.
>>
>> I understand we can use the default fs tree, but I just can't find my
>> mind at peace when doing snapshoting of fs tree.
>>
>> It always remind me of the bad memories using hacky way to solve the
>> qgroup problem for such snapshot.
> 
> I don't get it.
> The fs tree is a subvolume like any other, it was always possible to
> create snapshots of it, and snapshotting it is done the same way as
> for any other subvolume (both in terms of api and at the
> implementation level).
> So I don't see anything "hacky" about it, and it feels very natural and common.

It's already way off-topic now, and it's mostly qgroup related problem.

The problem here is, when snapshotting fs tree, the destination dir is 
also in fs tree itself.

This means for qgroup, it has to do the snapshot dir entry creation 
differently, even they are happening inside the same transaction.
(like doing a mini-transaction commit inside a transaction)

Details can be found in qgroup_account_snapshot().

But that's all qgroup details, should not really trouble end users, just 
some really bad personal memories...

Thanks,
Qu

> 
> So I don't understand the relation with solving some qgroup related
> problems in a "hacky" way.
> 
> You can leave it though.
> 
>>
>> Thus I always try to avoid snapshotting fs tree.
>>
>>>
>>>> +run_check $SUDO_HELPER dd if=/dev/zero bs=1M count=1 of="$TEST_MNT/subv/file1"
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
>>>> +       "$TEST_MNT/snap1"
>>>> +run_check $SUDO_HELPER cp --reflink=always "$TEST_MNT/subv/file1" \
>>>> +       "$TEST_MNT/subv/file2"
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume snapshot -r "$TEST_MNT/subv" \
>>>> +       "$TEST_MNT/snap2"
>>>> +
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/parent_stream" \
>>>> +       "$TEST_MNT/snap1"
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" send -f "$tmp/clone_stream" \
>>>> +       -p "$TEST_MNT/snap1" "$TEST_MNT/snap2"
>>>> +
>>>> +run_check_umount_test_dev
>>>> +run_check_mkfs_test_dev
>>>> +
>>>> +# Now we have the needed stream, try to receive them with different mount
>>>
>>> Reading this is confusing, it mentions receiving them with different
>>> mount options, but they are the same for the first receive.
>>>
>>>> +# options
>>>> +run_check_mount_test_dev -o datacow -o datasum
>>>> +
>>>> +# Receiving the full stream should not fail
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/parent_stream" "$TEST_MNT"
>>>> +
>>>> +# No remount helper, so here we manually unmoutn and re-mount with different
>>>> +# nodatasum option
>>>
>>> Seems pointless to mention there's a lack of a remount helper in the
>>> test framework.
>>> Just say that now we mount the filesystem with nodatasum so that the
>>> new file received through the incremental stream ends up with the
>>> nodatasum flag set.
>>
>> Or maybe I should just add run_check_remount_test().
>>
>> Thanks for the review,
>> Qu
>>
>>>
>>> Thanks.
>>>
>>>> +run_check_umount_test_dev
>>>> +run_check_mount_test_dev -o datacow,nodatasum
>>>> +
>>>> +# Receiving incremental send stream without --clone-fallback should fail.
>>>> +# As the clone source and destination have different NODATASUM flags
>>>> +run_mustfail "receiving clone with different NODATASUM should fail" \
>>>> +       $SUDO_HELPER "$TOP/btrfs" receive -f "$tmp/clone_stream" "$TEST_MNT"
>>>> +
>>>> +# Firstly we need to cleanup the partially received subvolume
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" subvolume delete "$TEST_MNT/snap2"
>>>> +
>>>> +# With --clone-fallback specified, the receive should finish without problem
>>>> +run_check $SUDO_HELPER "$TOP/btrfs" receive --clone-fallback \
>>>> +       -f "$tmp/clone_stream" "$TEST_MNT"
>>>> +run_check_umount_test_dev
>>>> +
>>>> +rm -rf -- "$tmp"
>>>> --
>>>> 2.33.0
>>>>
>>>
>>>
> 
> 
>