[PATCH 0/2] btrfs-progs: receive: workaround full file clone quirk

[PATCH 0/2] btrfs-progs: receive: workaround full file clone quirk

[Qu Wenruo]

Since kernel commit 46a6e10a1ab1 ("btrfs: send: allow cloning
non-aligned extent if it ends at i_size"), we can have clone commands to
clone a full file.

However such full file clone can have an unaligned length, and such
clone is only allowed if the destination file is no larger than the
clone range.

But we can create a case where such clone called on a destination file
which has a larger size.
This can lead to kernel reject such clone, and fail the receive.

This patchset will handle such quirk by truncate the file size to 0 if
we detect this is a full file clone.

Also add a new test case to verify the behaivor.

Qu Wenruo (2):
  btrfs-progs: receive: workaround unaligned full file clone
  btrfs-progs: misc-tests: new test case check the handling of full file
    clone

 cmds/receive.c                                |  47 ++++++++++++++++++
 .../ro_snap1.stream.zst                       | Bin 0 -> 293 bytes
 .../ro_subv1.stream.zst                       | Bin 0 -> 413 bytes
 .../066-receive-full-file-clone/test.sh       |  24 +++++++++
 4 files changed, 71 insertions(+)
 create mode 100644 tests/misc-tests/066-receive-full-file-clone/ro_snap1.stream.zst
 create mode 100644 tests/misc-tests/066-receive-full-file-clone/ro_subv1.stream.zst
 create mode 100755 tests/misc-tests/066-receive-full-file-clone/test.sh

--
2.46.1

[PATCH 1/2] btrfs-progs: receive: workaround unaligned full file clone

[Qu Wenruo]

[BUG]
The following script can lead to receive failure with latest kernel:

  dev="/dev/test/scratch1"
  mnt="/mnt/btrfs"

  mkfs.btrfs -f $dev
  mount $dev $mnt
  btrfs subv create $mnt/subv1
  xfs_io -f -c "pwrite 0 4000" $mnt/subv1/source
  xfs_io -f -c "reflink $mnt/subv1/source" $mnt/subv1/dest
  btrfs subv snap -r $mnt/subv1 $mnt/ro_subv1
  btrfs subv snap $mnt/subv1 $mnt/snap1
  xfs_io -f -c "pwrite -S 0xff 0 3900" -c "truncate 3900" $mnt/snap1/source
  truncate -s 0 $mnt/snap1/dest
  xfs_io -f -c "reflink $mnt/snap1/source" $mnt/snap1/dest
  btrfs subv snap -r $mnt/snap1 $mnt/ro_snap1
  btrfs send $mnt/ro_subv1 -f /tmp/ro_subv1.stream
  btrfs send -p $mnt/ro_subv1 $mnt/ro_snap1 -f /tmp/ro_snap1.stream
  umount $mnt
  mkfs.btrfs -f $dev
  mount $dev $mnt
  btrfs receive -f /tmp/ro_subv1.stream $mnt
  btrfs receive -f /tmp/ro_snap1.stream $mnt
  At snapshot ro_snap1
  ERROR: failed to clone extents to dest: Invalid argument

[CAUSE]
Since kernel commit 46a6e10a1ab1 ("btrfs: send: allow cloning
non-aligned extent if it ends at i_size"), kernel can send out clone
stream if we're cloning a full file, even if the size of the file is not
sector aligned, like this one:

  snapshot        ./ro_snap1                      uuid=2a3e2b70-c606-d446-b60b-baab458be6da transid=9 parent_uuid=d8ff9b9e-3ffc-6343-b53e-e22f8bbb7c25 parent_transid=7
  write           ./ro_snap1/source               offset=0 len=4700
  truncate        ./ro_snap1/source               size=4700
  utimes          ./ro_snap1/source               atime=2024-09-27T13:08:54+0800 mtime=2024-09-27T13:08:54+0800 ctime=2024-09-27T13:08:54+0800
  clone           ./ro_snap1/dest                 offset=0 len=4700 from=./ro_snap1/source clone_offset=0
  truncate        ./ro_snap1/dest                 size=4700
  utimes          ./ro_snap1/dest                 atime=2024-09-27T13:08:54+0800 mtime=2024-09-27T13:08:54+0800 ctime=2024-09-27T13:08:54+0800

However for the clone command, if the file inside the source subvolume
is larger than the new size, kernel will reject the clone operation, as
the resulted layout may read beyond the EOF of the clone source.

This should be addressed by the kernel, by doing the truncation before
the clone to ensure the destination file is no larger than the source.

[FIX]
It won't hurt for "btrfs receive" command to workaround the
problem, by truncating the destination file first.

Here we choose to truncate the file size to 0, other than the
source/destination file size.
As truncating to an unaligned size can cause the fs to do extra page
dirty and zero the tailing part.

Since we know it's a full file clone, truncating the file to size 0 will
avoid the extra page dirty, and allow the later clone to be done.

Reported-by: Ben Millwood <thebenmachine@gmail.com>
Link: https://lore.kernel.org/linux-btrfs/CAJhrHS2z+WViO2h=ojYvBPDLsATwLbg+7JaNCyYomv0fUxEpQQ@mail.gmail.com/
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/receive.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/cmds/receive.c b/cmds/receive.c
index 4cc5b9009442..9bda5485d895 100644
--- a/cmds/receive.c
+++ b/cmds/receive.c
@@ -56,6 +56,8 @@
 #include "cmds/commands.h"
 #include "cmds/receive-dump.h"
 
+static u32 g_sectorsize;
+
 struct btrfs_receive
 {
 	int mnt_fd;
@@ -739,6 +741,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
 	struct btrfs_receive *rctx = user;
 	struct btrfs_ioctl_clone_range_args clone_args;
 	struct subvol_info *si = NULL;
+	struct stat st = { 0 };
 	char full_path[PATH_MAX];
 	const char *subvol_path;
 	char full_clone_path[PATH_MAX];
@@ -802,11 +805,36 @@ static int process_clone(const char *path, u64 offset, u64 len,
 		error("cannot open %s: %m", full_clone_path);
 		goto out;
 	}
+	ret = fstat(clone_fd, &st);
+	if (ret < 0) {
+		errno = -ret;
+		error("cannot stat %s: %m", full_clone_path);
+		goto out;
+	}
 
 	if (bconf.verbose >= 2)
 		fprintf(stderr,
 			"clone %s - source=%s source offset=%llu offset=%llu length=%llu\n",
 			path, clone_path, clone_offset, offset, len);
+	/*
+	 * Since kernel commit 46a6e10a1ab1 ("btrfs: send: allow cloning
+	 * non-aligned extent if it ends at i_size"), we can have send
+	 * stream cloning a full file even its size is not sector aligned.
+	 *
+	 * But if we're cloning this unaligned range into an existing file,
+	 * which has a larger i_size, the clone will fail.
+	 *
+	 * Address this quirk by introducing an extra truncation to size 0.
+	 */
+	if (clone_offset == 0 && !IS_ALIGNED(len, g_sectorsize) &&
+	    len == st.st_size) {
+		ret = ftruncate(rctx->write_fd, 0);
+		if (ret < 0) {
+			errno = - ret;
+			error("failed to truncate %s: %m", path);
+			goto out;
+		}
+	}
 
 	clone_args.src_fd = clone_fd;
 	clone_args.src_offset = clone_offset;
@@ -1468,6 +1496,18 @@ static struct btrfs_send_ops send_ops = {
 	.enable_verity = process_enable_verity,
 };
 
+static int get_fs_sectorsize(int fd, u32 *sectorsize_ret)
+{
+	struct btrfs_ioctl_fs_info_args args = { 0 };
+	int ret;
+
+	ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
+	if (ret < 0)
+		return -errno;
+	*sectorsize_ret = args.sectorsize;
+	return 0;
+}
+
 static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 		      char *realmnt, int r_fd, u64 max_errors)
 {
@@ -1491,6 +1531,13 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
 			dest_dir_full_path);
 		goto out;
 	}
+	ret = get_fs_sectorsize(rctx->dest_dir_fd, &g_sectorsize);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to get sectorsize of the destination directory %s: %m",
+			dest_dir_full_path);
+		goto out;
+	}
 
 	if (realmnt[0]) {
 		rctx->root_path = realmnt;
-- 
2.46.1

[PATCH 2/2] btrfs-progs: misc-tests: new test case check the handling of full file clone

[Qu Wenruo]

The new test case will utilize the following send stream:

Stream 1:

subvol          ./ro_subv1                      uuid=769f87d1-98b2-824f-bf18-9d98178ad2e2 transid=7
chown           ./ro_subv1/                     gid=0 uid=0
chmod           ./ro_subv1/                     mode=755
mkfile          ./ro_subv1/o257-7-0
rename          ./ro_subv1/o257-7-0             dest=./ro_subv1/source
write           ./ro_subv1/source               offset=0 len=4000
chown           ./ro_subv1/source               gid=0 uid=0
chmod           ./ro_subv1/source               mode=600
utimes          ./ro_subv1/source               atime=2024-09-27T13:26:25+0800 mtime=2024-09-27T13:26:25+0800 ctime=2024-09-27T13:26:25+0800
mkfile          ./ro_subv1/o258-7-0
rename          ./ro_subv1/o258-7-0             dest=./ro_subv1/dest
clone           ./ro_subv1/dest                 offset=0 len=4000 from=./ro_subv1/source clone_offset=0
chown           ./ro_subv1/dest                 gid=0 uid=0
chmod           ./ro_subv1/dest                 mode=600
utimes          ./ro_subv1/dest                 atime=2024-09-27T13:26:25+0800 mtime=2024-09-27T13:26:25+0800 ctime=2024-09-27T13:26:25+0800
utimes          ./ro_subv1/                     atime=2024-09-27T13:26:25+0800 mtime=2024-09-27T13:26:25+0800 ctime=2024-09-27T13:26:25+0800

Stream 2:

snapshot        ./ro_snap1                      uuid=e78c9b7c-1bea-fc48-aaf3-6a4d98eba473 transid=9 parent_uuid=769f87d1-98b2-824f-bf18-9d98178ad2e2 parent_transid=7
write           ./ro_snap1/source               offset=0 len=3900
truncate        ./ro_snap1/source               size=3900
utimes          ./ro_snap1/source               atime=2024-09-27T13:26:25+0800 mtime=2024-09-27T13:26:25+0800 ctime=2024-09-27T13:26:25+0800
clone           ./ro_snap1/dest                 offset=0 len=3900 from=./ro_snap1/source clone_offset=0
truncate        ./ro_snap1/dest                 size=3900
utimes          ./ro_snap1/dest                 atime=2024-09-27T13:26:25+0800 mtime=2024-09-27T13:26:25+0800 ctime=2024-09-27T13:26:25+0800

The stream is generated using v6.11 kernel, as upstream commit
46a6e10a1ab1 ("btrfs: send: allow cloning non-aligned extent if it ends
at i_size") allows full file clone to further reduce the stream size.

Verify that "btrfs receive" command has the proper workaround to handle
such full file clone correctly.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 .../ro_snap1.stream.zst                       | Bin 0 -> 293 bytes
 .../ro_subv1.stream.zst                       | Bin 0 -> 413 bytes
 .../066-receive-full-file-clone/test.sh       |  24 ++++++++++++++++++
 3 files changed, 24 insertions(+)
 create mode 100644 tests/misc-tests/066-receive-full-file-clone/ro_snap1.stream.zst
 create mode 100644 tests/misc-tests/066-receive-full-file-clone/ro_subv1.stream.zst
 create mode 100755 tests/misc-tests/066-receive-full-file-clone/test.sh

diff --git a/tests/misc-tests/066-receive-full-file-clone/ro_snap1.stream.zst b/tests/misc-tests/066-receive-full-file-clone/ro_snap1.stream.zst
new file mode 100644
index 0000000000000000000000000000000000000000..e190b6ca7f050ae16ed11a6b278658fdea5c9adb
GIT binary patch
literal 293
zcmdPcs{c3TEB{^&1{2<-lA^R?-Qtp>)Wlo{Mg|53A0TF8@Z107AU^{KLs5Qwab9A9
zAtQqT!}FfmHPWyCc&z%I<vZi`l42&HJST$)P^N5t`^6cXn*8@m%$*_Lb?K2PP==i$
zjGuvlpTV_b)mnZAHiqK-(xT*4A)t&fgAM<GF`zYq47Z-7Okn{E+3;Hd`63Jx^d!G>
zGw?7lI(<vq7xY+w%T8>q5N8-rPJkiab{?|?KLZOxN@{V5h^Uyjh-*u5sKt8Y^aYtb
zo`U<<xC)Ch1mv8YE5(q*F+-B!fZ&6S3=57fj&<5n{N4hxZ{vj9WPJn^?nvw?$u!7&
luh{Nz-PpJJK>IC@9;31yigTFPGv;#`@UaQ7R&Bgf1OO`|ST_Iw

literal 0
HcmV?d00001

diff --git a/tests/misc-tests/066-receive-full-file-clone/ro_subv1.stream.zst b/tests/misc-tests/066-receive-full-file-clone/ro_subv1.stream.zst
new file mode 100644
index 0000000000000000000000000000000000000000..f70aeebb7cf5161d0fe3db25496ed29d7e12430b
GIT binary patch
literal 413
zcmdPcs{c2oU$B;k;fO#|Nl{v{ZgELbYGN(}BLf424iGak?0Y}=5kCV5Ls5QwacNSS
zAtQqTL)rZHi!(Mg`R|vQJ43wd(jz9IJUfF7P`xk%leumWKaj-+5)%M2gcu@2OHEmU
z+_#JpKt3}=PTp<id?QnHU2|OnW}rBu7*Lp#VF$n976G8X;{4L0<kVe2L--jU-1uw9
zFC@&cfd8ygns{>;o7n1Ov5On5fEq*?9+uT#<7VJtV08MHwlC<h02gO*tMN7qCK26Z
zc|H~_3@NF_C1F670t{dNZ~H9)v}pl@h^Uyj!f9o>RU#b+y}w&Hi9A=x<na_V`PE^n
zCo;2EK$IaM=j2=qg#%0zHH9Ygg)k?~VLH~Z+O6<c#9@gx)^mZUEjff<?wbDjg36^*
zL;gdX+>I62Nw&%AE-Fmkc;syI)(Z(Njvq`8$w)j}^0w@aaM=`2gBkD6UE5N#Bd{{m
zLHgd(?cZ4{6N>xaFIDk)`_$&`*ZC*p-p(s${q$V-*Ay0JxnfmCi8PV8k2@{_07?#w
AHUIzs

literal 0
HcmV?d00001

diff --git a/tests/misc-tests/066-receive-full-file-clone/test.sh b/tests/misc-tests/066-receive-full-file-clone/test.sh
new file mode 100755
index 000000000000..63fa922a5600
--- /dev/null
+++ b/tests/misc-tests/066-receive-full-file-clone/test.sh
@@ -0,0 +1,24 @@
+#!/bin/bash
+# Verify "btrfs-receive" can handle a full file clone (clone length is not
+# aligned but matches inode size) into a larger destination file.
+#
+# Such clone stream can be generated since kernel commit 46a6e10a1ab1
+# ("btrfs: send: allow cloning non-aligned extent if it ends at i_size").
+
+source "$TEST_TOP/common" || exit
+source "$TEST_TOP/common.convert" || exit
+
+tmp=$(_mktemp "receive-full-file-clone")
+
+setup_root_helper
+prepare_test_dev
+check_global_prereq zstd
+
+run_check_mkfs_test_dev
+run_check_mount_test_dev
+run_check zstd -d -f ./ro_subv1.stream.zst -o "$tmp"
+run_check "$TOP/btrfs" receive -f "$tmp" "$TEST_MNT"
+run_check zstd -d -f ./ro_snap1.stream.zst -o "$tmp"
+run_check "$TOP/btrfs" receive -f "$tmp" "$TEST_MNT"
+run_check_umount_test_dev
+rm -f -- "$tmp"
-- 
2.46.1

Re: [PATCH 1/2] btrfs-progs: receive: workaround unaligned full file clone

[Filipe Manana]

On Fri, Sep 27, 2024 at 6:54 AM Qu Wenruo <wqu@suse.com> wrote:
>
> [BUG]
> The following script can lead to receive failure with latest kernel:
>
>   dev="/dev/test/scratch1"
>   mnt="/mnt/btrfs"
>
>   mkfs.btrfs -f $dev
>   mount $dev $mnt
>   btrfs subv create $mnt/subv1
>   xfs_io -f -c "pwrite 0 4000" $mnt/subv1/source
>   xfs_io -f -c "reflink $mnt/subv1/source" $mnt/subv1/dest
>   btrfs subv snap -r $mnt/subv1 $mnt/ro_subv1
>   btrfs subv snap $mnt/subv1 $mnt/snap1
>   xfs_io -f -c "pwrite -S 0xff 0 3900" -c "truncate 3900" $mnt/snap1/source
>   truncate -s 0 $mnt/snap1/dest
>   xfs_io -f -c "reflink $mnt/snap1/source" $mnt/snap1/dest
>   btrfs subv snap -r $mnt/snap1 $mnt/ro_snap1
>   btrfs send $mnt/ro_subv1 -f /tmp/ro_subv1.stream
>   btrfs send -p $mnt/ro_subv1 $mnt/ro_snap1 -f /tmp/ro_snap1.stream
>   umount $mnt
>   mkfs.btrfs -f $dev
>   mount $dev $mnt
>   btrfs receive -f /tmp/ro_subv1.stream $mnt
>   btrfs receive -f /tmp/ro_snap1.stream $mnt
>   At snapshot ro_snap1
>   ERROR: failed to clone extents to dest: Invalid argument
>
> [CAUSE]
> Since kernel commit 46a6e10a1ab1 ("btrfs: send: allow cloning
> non-aligned extent if it ends at i_size"), kernel can send out clone
> stream if we're cloning a full file, even if the size of the file is not
> sector aligned, like this one:
>
>   snapshot        ./ro_snap1                      uuid=2a3e2b70-c606-d446-b60b-baab458be6da transid=9 parent_uuid=d8ff9b9e-3ffc-6343-b53e-e22f8bbb7c25 parent_transid=7
>   write           ./ro_snap1/source               offset=0 len=4700
>   truncate        ./ro_snap1/source               size=4700
>   utimes          ./ro_snap1/source               atime=2024-09-27T13:08:54+0800 mtime=2024-09-27T13:08:54+0800 ctime=2024-09-27T13:08:54+0800
>   clone           ./ro_snap1/dest                 offset=0 len=4700 from=./ro_snap1/source clone_offset=0
>   truncate        ./ro_snap1/dest                 size=4700
>   utimes          ./ro_snap1/dest                 atime=2024-09-27T13:08:54+0800 mtime=2024-09-27T13:08:54+0800 ctime=2024-09-27T13:08:54+0800
>
> However for the clone command, if the file inside the source subvolume
> is larger than the new size, kernel will reject the clone operation, as
> the resulted layout may read beyond the EOF of the clone source.
>
> This should be addressed by the kernel, by doing the truncation before
> the clone to ensure the destination file is no larger than the source.
>
> [FIX]
> It won't hurt for "btrfs receive" command to workaround the
> problem, by truncating the destination file first.
>
> Here we choose to truncate the file size to 0, other than the
> source/destination file size.
> As truncating to an unaligned size can cause the fs to do extra page
> dirty and zero the tailing part.
>
> Since we know it's a full file clone, truncating the file to size 0 will
> avoid the extra page dirty, and allow the later clone to be done.
>
> Reported-by: Ben Millwood <thebenmachine@gmail.com>
> Link: https://lore.kernel.org/linux-btrfs/CAJhrHS2z+WViO2h=ojYvBPDLsATwLbg+7JaNCyYomv0fUxEpQQ@mail.gmail.com/
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  cmds/receive.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/cmds/receive.c b/cmds/receive.c
> index 4cc5b9009442..9bda5485d895 100644
> --- a/cmds/receive.c
> +++ b/cmds/receive.c
> @@ -56,6 +56,8 @@
>  #include "cmds/commands.h"
>  #include "cmds/receive-dump.h"
>
> +static u32 g_sectorsize;
> +
>  struct btrfs_receive
>  {
>         int mnt_fd;
> @@ -739,6 +741,7 @@ static int process_clone(const char *path, u64 offset, u64 len,
>         struct btrfs_receive *rctx = user;
>         struct btrfs_ioctl_clone_range_args clone_args;
>         struct subvol_info *si = NULL;
> +       struct stat st = { 0 };
>         char full_path[PATH_MAX];
>         const char *subvol_path;
>         char full_clone_path[PATH_MAX];
> @@ -802,11 +805,36 @@ static int process_clone(const char *path, u64 offset, u64 len,
>                 error("cannot open %s: %m", full_clone_path);
>                 goto out;
>         }
> +       ret = fstat(clone_fd, &st);
> +       if (ret < 0) {
> +               errno = -ret;
> +               error("cannot stat %s: %m", full_clone_path);
> +               goto out;
> +       }
>
>         if (bconf.verbose >= 2)
>                 fprintf(stderr,
>                         "clone %s - source=%s source offset=%llu offset=%llu length=%llu\n",
>                         path, clone_path, clone_offset, offset, len);
> +       /*
> +        * Since kernel commit 46a6e10a1ab1 ("btrfs: send: allow cloning
> +        * non-aligned extent if it ends at i_size"), we can have send
> +        * stream cloning a full file even its size is not sector aligned.
> +        *
> +        * But if we're cloning this unaligned range into an existing file,
> +        * which has a larger i_size, the clone will fail.
> +        *
> +        * Address this quirk by introducing an extra truncation to size 0.

No, this is incorrect and will cause data loss in case the file has
multiple extents, and we're trying to clone the last one which is not
sector size aligned.
The assumption that this is always for a full file, consisting of a
single extent is wrong.

The problem is easy to fix in the kernel and it's where it should
always be fixed - any problem with the order of operations in the send
stream should be fixed in the kernel.
We've had at least one problem in the past by adding this type of
workarounds to btrfs-progs when the problem should be fixed in the
kernel (a case with special xattrs).

I'll send the kernel fix soon.

Thanks.

> +        */
> +       if (clone_offset == 0 && !IS_ALIGNED(len, g_sectorsize) &&
> +           len == st.st_size) {
> +               ret = ftruncate(rctx->write_fd, 0);
> +               if (ret < 0) {
> +                       errno = - ret;
> +                       error("failed to truncate %s: %m", path);
> +                       goto out;
> +               }
> +       }
>
>         clone_args.src_fd = clone_fd;
>         clone_args.src_offset = clone_offset;
> @@ -1468,6 +1496,18 @@ static struct btrfs_send_ops send_ops = {
>         .enable_verity = process_enable_verity,
>  };
>
> +static int get_fs_sectorsize(int fd, u32 *sectorsize_ret)
> +{
> +       struct btrfs_ioctl_fs_info_args args = { 0 };
> +       int ret;
> +
> +       ret = ioctl(fd, BTRFS_IOC_FS_INFO, &args);
> +       if (ret < 0)
> +               return -errno;
> +       *sectorsize_ret = args.sectorsize;
> +       return 0;
> +}
> +
>  static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
>                       char *realmnt, int r_fd, u64 max_errors)
>  {
> @@ -1491,6 +1531,13 @@ static int do_receive(struct btrfs_receive *rctx, const char *tomnt,
>                         dest_dir_full_path);
>                 goto out;
>         }
> +       ret = get_fs_sectorsize(rctx->dest_dir_fd, &g_sectorsize);
> +       if (ret < 0) {
> +               errno = -ret;
> +               error("failed to get sectorsize of the destination directory %s: %m",
> +                       dest_dir_full_path);
> +               goto out;
> +       }
>
>         if (realmnt[0]) {
>                 rctx->root_path = realmnt;
> --
> 2.46.1
>
>