On Fri, Jul 17, 2015 at 4:49 AM, Jan Alexander Steffens
<jan.steffens@xxxxxxxxx> wrote:
> On Sun, May 3, 2015 at 2:56 AM, Filipe Manana <fdmanana@xxxxxxxx> 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@xxxxxxxxxxx>
>> Tested-by: Marc MERLIN <marc@xxxxxxxxxxx>
>> Signed-off-by: Filipe Manana <fdmanana@xxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxxxxxxxx
> 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."
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
