[PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[Filipe Manana]

Marc reported a problem where the receiving end of an incremental send
was performing clone operations that failed with -EINVAL. This happened
because, unlike for uncompressed extents, we were not checking if the
source clone offset and length, after summing the data offset, falls
within the source file's boundaries.

So make sure we do such checks when attempting to issue clone operations
for compressed extents.

Problem reproducible with the following steps:

  $ mkfs.btrfs -f /dev/sdb
  $ mount -o compress /dev/sdb /mnt
  $ mkfs.btrfs -f /dev/sdc
  $ mount -o compress /dev/sdc /mnt2

  # Create the file with a single extent of 128K. This creates a metadata file
  # extent item with a data start offset of 0 and a logical length of 128K.
  $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo

  # Now rewrite the range 64K to 112K of our file. This will make the inode's
  # metadata continue to point to the 128K extent we created before, but now
  # with an extent item that points to the extent with a data start offset of
  # 112K and a logical length of 16K.
  # That metadata file extent item is associated with the logical file offset
  # at 176K and covers the logical file range 176K to 192K.
  $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo

  # Now rewrite the range 180K to 12K. This will make the inode's metadata
  # continue to point the the 128K extent we created earlier, with a single
  # extent item that points to it with a start offset of 112K and a logical
  # length of 4K.
  # That metadata file extent item is associated with the logical file offset
  # at 176K and covers the logical file range 176K to 180K.
  $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo

  $ btrfs subvolume snapshot -r /mnt /mnt/snap1

  $ touch /mnt/bar
  # Calls the btrfs clone ioctl.
  $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \
    -l $((4 * 1024)) /mnt/foo /mnt/bar

  $ btrfs subvolume snapshot -r /mnt /mnt/snap2

  $ btrfs send /mnt/snap1 | btrfs receive /mnt2
  At subvol /mnt/snap1
  At subvol snap1

  $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2
  At subvol /mnt/snap2
  At snapshot snap2
  ERROR: failed to clone extents to bar
  Invalid argument

A test case for fstests follows soon.

Reported-by: Marc MERLIN <marc@merlins.org>
Tested-by: Marc MERLIN <marc@merlins.org>
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/send.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 895f1b1..50ebc62 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1159,6 +1159,9 @@ struct backref_ctx {
 	/* may be truncated in case it's the last extent in a file */
 	u64 extent_len;
 
+	/* data offset in the file extent item */
+	u64 data_offset;
+
 	/* Just to check for bugs in backref resolving */
 	int found_itself;
 };
@@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_)
 	if (ret < 0)
 		return ret;
 
-	if (offset + bctx->extent_len > i_size)
+	if (offset + bctx->data_offset + bctx->extent_len > i_size)
 		return 0;
 
 	/*
@@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx,
 	backref_ctx->cur_offset = data_offset;
 	backref_ctx->found_itself = 0;
 	backref_ctx->extent_len = num_bytes;
+	/*
+	 * For non-compressed extents iterate_extent_inodes() gives us extent
+	 * offsets that already take into account the data offset, but not for
+	 * compressed extents, since the offset is logical and not relative to
+	 * the physical extent locations. We must take this into account to
+	 * avoid sending clone offsets that go beyond the source file's size,
+	 * which would result in the clone ioctl failing with -EINVAL on the
+	 * receiving end.
+	 */
+	if (compressed == BTRFS_COMPRESS_NONE)
+		backref_ctx->data_offset = 0;
+	else
+		backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi);
 
 	/*
 	 * The last extent of a file may be too large due to page alignment.
-- 
2.1.3

Re: [PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[David Sterba]

On Sun, May 03, 2015 at 01:56:00AM +0100, Filipe Manana wrote:
> Marc reported a problem where the receiving end of an incremental send
> was performing clone operations that failed with -EINVAL. This happened
> because, unlike for uncompressed extents, we were not checking if the
> source clone offset and length, after summing the data offset, falls
> within the source file's boundaries.
> 
> So make sure we do such checks when attempting to issue clone operations
> for compressed extents.
> 
> Problem reproducible with the following steps:
> 
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount -o compress /dev/sdb /mnt
>   $ mkfs.btrfs -f /dev/sdc
>   $ mount -o compress /dev/sdc /mnt2
> 
>   # Create the file with a single extent of 128K. This creates a metadata file
>   # extent item with a data start offset of 0 and a logical length of 128K.
>   $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo
> 
>   # Now rewrite the range 64K to 112K of our file. This will make the inode's
>   # metadata continue to point to the 128K extent we created before, but now
>   # with an extent item that points to the extent with a data start offset of
>   # 112K and a logical length of 16K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 192K.
>   $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo
> 
>   # Now rewrite the range 180K to 12K. This will make the inode's metadata
>   # continue to point the the 128K extent we created earlier, with a single
>   # extent item that points to it with a start offset of 112K and a logical
>   # length of 4K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 180K.
>   $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo
> 
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap1
> 
>   $ touch /mnt/bar
>   # Calls the btrfs clone ioctl.
>   $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \
>     -l $((4 * 1024)) /mnt/foo /mnt/bar
> 
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap2
> 
>   $ btrfs send /mnt/snap1 | btrfs receive /mnt2
>   At subvol /mnt/snap1
>   At subvol snap1
> 
>   $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2
>   At subvol /mnt/snap2
>   At snapshot snap2
>   ERROR: failed to clone extents to bar
>   Invalid argument
> 
> A test case for fstests follows soon.
> 
> Reported-by: Marc MERLIN <marc@merlins.org>
> Tested-by: Marc MERLIN <marc@merlins.org>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Tested-by: David Sterba <dsterba@suse.cz>

Re: [PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[Jan Alexander Steffens]

On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote:
> Marc reported a problem where the receiving end of an incremental send
> was performing clone operations that failed with -EINVAL. This happened
> because, unlike for uncompressed extents, we were not checking if the
> source clone offset and length, after summing the data offset, falls
> within the source file's boundaries.
>
> So make sure we do such checks when attempting to issue clone operations
> for compressed extents.
>
> Problem reproducible with the following steps:
>
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount -o compress /dev/sdb /mnt
>   $ mkfs.btrfs -f /dev/sdc
>   $ mount -o compress /dev/sdc /mnt2
>
>   # Create the file with a single extent of 128K. This creates a metadata file
>   # extent item with a data start offset of 0 and a logical length of 128K.
>   $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo
>
>   # Now rewrite the range 64K to 112K of our file. This will make the inode's
>   # metadata continue to point to the 128K extent we created before, but now
>   # with an extent item that points to the extent with a data start offset of
>   # 112K and a logical length of 16K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 192K.
>   $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo
>
>   # Now rewrite the range 180K to 12K. This will make the inode's metadata
>   # continue to point the the 128K extent we created earlier, with a single
>   # extent item that points to it with a start offset of 112K and a logical
>   # length of 4K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 180K.
>   $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo
>
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap1
>
>   $ touch /mnt/bar
>   # Calls the btrfs clone ioctl.
>   $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \
>     -l $((4 * 1024)) /mnt/foo /mnt/bar
>
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap2
>
>   $ btrfs send /mnt/snap1 | btrfs receive /mnt2
>   At subvol /mnt/snap1
>   At subvol snap1
>
>   $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2
>   At subvol /mnt/snap2
>   At snapshot snap2
>   ERROR: failed to clone extents to bar
>   Invalid argument
>
> A test case for fstests follows soon.
>
> Reported-by: Marc MERLIN <marc@merlins.org>
> Tested-by: Marc MERLIN <marc@merlins.org>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Tested-by: Jan Alexander Steffens (heftig) <jan.steffens@gmail.com>

Re: [PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[Jan Alexander Steffens]

On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote:
> Marc reported a problem where the receiving end of an incremental send
> was performing clone operations that failed with -EINVAL. This happened
> because, unlike for uncompressed extents, we were not checking if the
> source clone offset and length, after summing the data offset, falls
> within the source file's boundaries.
>
> So make sure we do such checks when attempting to issue clone operations
> for compressed extents.
>
> Problem reproducible with the following steps:
>
>   $ mkfs.btrfs -f /dev/sdb
>   $ mount -o compress /dev/sdb /mnt
>   $ mkfs.btrfs -f /dev/sdc
>   $ mount -o compress /dev/sdc /mnt2
>
>   # Create the file with a single extent of 128K. This creates a metadata file
>   # extent item with a data start offset of 0 and a logical length of 128K.
>   $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo
>
>   # Now rewrite the range 64K to 112K of our file. This will make the inode's
>   # metadata continue to point to the 128K extent we created before, but now
>   # with an extent item that points to the extent with a data start offset of
>   # 112K and a logical length of 16K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 192K.
>   $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo
>
>   # Now rewrite the range 180K to 12K. This will make the inode's metadata
>   # continue to point the the 128K extent we created earlier, with a single
>   # extent item that points to it with a start offset of 112K and a logical
>   # length of 4K.
>   # That metadata file extent item is associated with the logical file offset
>   # at 176K and covers the logical file range 176K to 180K.
>   $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo
>
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap1
>
>   $ touch /mnt/bar
>   # Calls the btrfs clone ioctl.
>   $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \
>     -l $((4 * 1024)) /mnt/foo /mnt/bar
>
>   $ btrfs subvolume snapshot -r /mnt /mnt/snap2
>
>   $ btrfs send /mnt/snap1 | btrfs receive /mnt2
>   At subvol /mnt/snap1
>   At subvol snap1
>
>   $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2
>   At subvol /mnt/snap2
>   At snapshot snap2
>   ERROR: failed to clone extents to bar
>   Invalid argument
>
> A test case for fstests follows soon.
>
> Reported-by: Marc MERLIN <marc@merlins.org>
> Tested-by: Marc MERLIN <marc@merlins.org>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/send.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 895f1b1..50ebc62 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1159,6 +1159,9 @@ struct backref_ctx {
>         /* may be truncated in case it's the last extent in a file */
>         u64 extent_len;
>
> +       /* data offset in the file extent item */
> +       u64 data_offset;
> +
>         /* Just to check for bugs in backref resolving */
>         int found_itself;
>  };
> @@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_)
>         if (ret < 0)
>                 return ret;
>
> -       if (offset + bctx->extent_len > i_size)
> +       if (offset + bctx->data_offset + bctx->extent_len > i_size)
>                 return 0;
>
>         /*
> @@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx,
>         backref_ctx->cur_offset = data_offset;
>         backref_ctx->found_itself = 0;
>         backref_ctx->extent_len = num_bytes;
> +       /*
> +        * For non-compressed extents iterate_extent_inodes() gives us extent
> +        * offsets that already take into account the data offset, but not for
> +        * compressed extents, since the offset is logical and not relative to
> +        * the physical extent locations. We must take this into account to
> +        * avoid sending clone offsets that go beyond the source file's size,
> +        * which would result in the clone ioctl failing with -EINVAL on the
> +        * receiving end.
> +        */
> +       if (compressed == BTRFS_COMPRESS_NONE)
> +               backref_ctx->data_offset = 0;
> +       else
> +               backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi);
>
>         /*
>          * The last extent of a file may be too large due to page alignment.
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Is this patch still useful? It applies to 4.1.

Re: [PATCH] Btrfs: incremental send, fix clone operations for compressed extents

[Filipe David Manana]

On Fri, Jul 17, 2015 at 4:49 AM, Jan Alexander Steffens
<jan.steffens@gmail.com> wrote:
> On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@suse.com> wrote:
>> Marc reported a problem where the receiving end of an incremental send
>> was performing clone operations that failed with -EINVAL. This happened
>> because, unlike for uncompressed extents, we were not checking if the
>> source clone offset and length, after summing the data offset, falls
>> within the source file's boundaries.
>>
>> So make sure we do such checks when attempting to issue clone operations
>> for compressed extents.
>>
>> Problem reproducible with the following steps:
>>
>>   $ mkfs.btrfs -f /dev/sdb
>>   $ mount -o compress /dev/sdb /mnt
>>   $ mkfs.btrfs -f /dev/sdc
>>   $ mount -o compress /dev/sdc /mnt2
>>
>>   # Create the file with a single extent of 128K. This creates a metadata file
>>   # extent item with a data start offset of 0 and a logical length of 128K.
>>   $ xfs_io -f -c "pwrite -S 0xaa 64K 128K" -c "fsync" /mnt/foo
>>
>>   # Now rewrite the range 64K to 112K of our file. This will make the inode's
>>   # metadata continue to point to the 128K extent we created before, but now
>>   # with an extent item that points to the extent with a data start offset of
>>   # 112K and a logical length of 16K.
>>   # That metadata file extent item is associated with the logical file offset
>>   # at 176K and covers the logical file range 176K to 192K.
>>   $ xfs_io -c "pwrite -S 0xbb 64K 112K" -c "fsync" /mnt/foo
>>
>>   # Now rewrite the range 180K to 12K. This will make the inode's metadata
>>   # continue to point the the 128K extent we created earlier, with a single
>>   # extent item that points to it with a start offset of 112K and a logical
>>   # length of 4K.
>>   # That metadata file extent item is associated with the logical file offset
>>   # at 176K and covers the logical file range 176K to 180K.
>>   $ xfs_io -c "pwrite -S 0xcc 180K 12K" -c "fsync" /mnt/foo
>>
>>   $ btrfs subvolume snapshot -r /mnt /mnt/snap1
>>
>>   $ touch /mnt/bar
>>   # Calls the btrfs clone ioctl.
>>   $ ~/xfstests/src/cloner -s $((176 * 1024)) -d $((176 * 1024)) \
>>     -l $((4 * 1024)) /mnt/foo /mnt/bar
>>
>>   $ btrfs subvolume snapshot -r /mnt /mnt/snap2
>>
>>   $ btrfs send /mnt/snap1 | btrfs receive /mnt2
>>   At subvol /mnt/snap1
>>   At subvol snap1
>>
>>   $ btrfs send -p /mnt/snap1 /mnt/snap2 | btrfs receive /mnt2
>>   At subvol /mnt/snap2
>>   At snapshot snap2
>>   ERROR: failed to clone extents to bar
>>   Invalid argument
>>
>> A test case for fstests follows soon.
>>
>> Reported-by: Marc MERLIN <marc@merlins.org>
>> Tested-by: Marc MERLIN <marc@merlins.org>
>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>> ---
>>  fs/btrfs/send.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index 895f1b1..50ebc62 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -1159,6 +1159,9 @@ struct backref_ctx {
>>         /* may be truncated in case it's the last extent in a file */
>>         u64 extent_len;
>>
>> +       /* data offset in the file extent item */
>> +       u64 data_offset;
>> +
>>         /* Just to check for bugs in backref resolving */
>>         int found_itself;
>>  };
>> @@ -1222,7 +1225,7 @@ static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_)
>>         if (ret < 0)
>>                 return ret;
>>
>> -       if (offset + bctx->extent_len > i_size)
>> +       if (offset + bctx->data_offset + bctx->extent_len > i_size)
>>                 return 0;
>>
>>         /*
>> @@ -1364,6 +1367,19 @@ static int find_extent_clone(struct send_ctx *sctx,
>>         backref_ctx->cur_offset = data_offset;
>>         backref_ctx->found_itself = 0;
>>         backref_ctx->extent_len = num_bytes;
>> +       /*
>> +        * For non-compressed extents iterate_extent_inodes() gives us extent
>> +        * offsets that already take into account the data offset, but not for
>> +        * compressed extents, since the offset is logical and not relative to
>> +        * the physical extent locations. We must take this into account to
>> +        * avoid sending clone offsets that go beyond the source file's size,
>> +        * which would result in the clone ioctl failing with -EINVAL on the
>> +        * receiving end.
>> +        */
>> +       if (compressed == BTRFS_COMPRESS_NONE)
>> +               backref_ctx->data_offset = 0;
>> +       else
>> +               backref_ctx->data_offset = btrfs_file_extent_offset(eb, fi);
>>
>>         /*
>>          * The last extent of a file may be too large due to page alignment.
>> --
>> 2.1.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> Is this patch still useful? It applies to 4.1.

Yes, Chris sent it in the pull request to Linus for the 4.2-rc1.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."