[v2] Btrfs: send, fix emission of invalid clone operations within the same file

Commit Message

Filipe Manana Jan. 29, 2020, 5:09 p.m. UTC

From: Filipe Manana <[e-mail]>

When doing an incremental send and a file has extents shared with itself
at different file offsets, it's possible for send to emit clone operations
that will fail at the destination because the source range goes beyond the
file's current size. This happens when the file size has increased in the
send snapshot, there is a hole between the shared extents and both shared
extents are at file offsets which are greater the file's size in the
parent snapshot.

Example:

  $ mkfs.btrfs -f /dev/sdb
  $ mount /dev/sdb /mnt/sdb

  $ xfs_io -f -c "pwrite -S 0xf1 0 64K" /mnt/sdb/foobar
  $ btrfs subvolume snapshot -r /mnt/sdb /mnt/sdb/base
  $ btrfs send -f /tmp/1.snap /mnt/sdb/base

  # Create a 320K extent at file offset 512K.
  $ xfs_io -c "pwrite -S 0xab 512K 64K" /mnt/sdb/foobar
  $ xfs_io -c "pwrite -S 0xcd 576K 64K" /mnt/sdb/foobar
  $ xfs_io -c "pwrite -S 0xef 640K 64K" /mnt/sdb/foobar
  $ xfs_io -c "pwrite -S 0x64 704K 64K" /mnt/sdb/foobar
  $ xfs_io -c "pwrite -S 0x73 768K 64K" /mnt/sdb/foobar

  # Clone part of that 320K extent into a lower file offset (192K).
  # This file offset is greater than the file's size in the parent
  # snapshot (64K). Also the clone range is a bit behind the offset of
  # the 320K extent so that we leave a hole between the shared extents.
  $ xfs_io -c "reflink /mnt/sdb/foobar 448K 192K 192K" /mnt/sdb/foobar

  $ btrfs subvolume snapshot -r /mnt/sdb /mnt/sdb/incr
  $ btrfs send -p /mnt/sdb/base -f /tmp/2.snap /mnt/sdb/incr

  $ mkfs.btrfs -f /dev/sdc
  $ mount /dev/sdc /mnt/sdc

  $ btrfs receive -f /tmp/1.snap /mnt/sdc
  $ btrfs receive -f /tmp/2.snap /mnt/sdc
  ERROR: failed to clone extents to foobar: Invalid argument

The problem is that after processing the extent at file offset 256K, which
refers to the first 128K of the 320K extent created by the buffered write
operations, we have 'cur_inode_next_write_offset' set to 384K, which
corresponds to the end offset of the partially shared extent (256K + 128K)
and to the current file size in the receiver. Then when we process the
extent at offset 512K, we do extent backreference iteration to figure out
if we can clone the extent from some other inode or from the same inode,
and we consider the extent at offset 256K of the same inode as a valid
source for a clone operation, which is not correct because at that point
the current file size in the receiver is 384K, which corresponds to the
end of last processed extent (at file offset 256K), so using a clone
source range from 256K to 256K + 320K is invalid because that goes past
the current size of the file (384K) - this makes the receiver get an
-EINVAL error when attempting the clone operation.

So fix this by excluding clone sources that have a range that goes beyond
the current file size in the receiver when iterating extent backreferences.

A test case for fstests follows soon.

Fixes: 11f2069c113e02 ("Btrfs: send, allow clone operations within the same file")
Reviewed-by: Josef Bacik <[e-mail]>
Signed-off-by: Filipe Manana <[e-mail]>
---

v2: Removed unncessary hunk, which sey sctx->cur_inode_next_write_offset, not
    because it's not necessary to fix this issue, but also caused problems
    for files that got a prealloc extents beyond their size in the send
    snapshot, got their size increased and have a trailing hole (ending at
    the new file size).

 fs/btrfs/send.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

David Sterba Jan. 30, 2020, 4:31 p.m. UTC | #1

On Wed, Jan 29, 2020 at 05:09:53PM +0000, [e-mail] wrote:
> From: Filipe Manana <[e-mail]>
[...]

Added to misc-next, thanks.

Christoph Anton Mitterer Feb. 11, 2020, 2:32 p.m. UTC | #2

Hey.

Just wanted to ask whether this is going to be backported to 5.5?

Cheers,
Chris.

Filipe Manana Feb. 11, 2020, 2:46 p.m. UTC | #3

On Tue, Feb 11, 2020 at 2:41 PM Christoph Anton Mitterer
<[e-mail]> wrote:
>
> Hey.
>
> Just wanted to ask whether this is going to be backported to 5.5?

It's a bug fix, so yes. In fact you can check that yourself:

https://cdn.kernel.org/pub/linux/kernel/v5.x/ChangeLog-5.5.3

thanks
>
> Cheers,
> Chris.
>

Christoph Anton Mitterer Feb. 12, 2020, 7:20 p.m. UTC | #4

On Tue, 2020-02-11 at 14:46 +0000, Filipe Manana wrote:
> It's a bug fix, so yes. In fact you can check that yourself:

Thanks... actually I did check it directly in linux-stable.git ... but
it must have been merged shortly afterwards ^^

Cheers,
Chris.

Patch

diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 091e5bc8c7ea..a055b657cb85 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1269,7 +1269,8 @@  static int __iterate_backrefs(u64 ino, u64 offset, u64 root, void *ctx_)
 		 * destination of the stream.
 		 */
 		if (ino == bctx->cur_objectid &&
-		    offset >= bctx->sctx->cur_inode_next_write_offset)
+		    offset + bctx->extent_len >
+		    bctx->sctx->cur_inode_next_write_offset)
 			return 0;
 	}