[PATCH] block/block-copy: always align copied region to cluster size

[PATCH] block/block-copy: always align copied region to cluster size

[Stefan Reiter]

Since commit 42ac214406e0 (block/block-copy: refactor task creation)
block_copy_task_create calculates the area to be copied via
bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
count if the backing image's last cluster end is not aligned to the
bitmap's granularity.

Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
which requires the 'bytes' parameter to be aligned to cluster size.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---

This causes backups with unaligned image sizes to fail on the last block in my
testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
bytes).

Alternatively one could remove the
  assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
from block_copy_do_copy, but I'd wager that's there for a reason?

 block/block-copy.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block/block-copy.c b/block/block-copy.c
index f7428a7c08..023cb03200 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
         return NULL;
     }
 
+    bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
+
     /* region is dirty, so no existent tasks possible in it */
     assert(!find_conflicting_task(s, offset, bytes));
 
-- 
2.20.1

Re: [PATCH] block/block-copy: always align copied region to cluster size

[Vladimir Sementsov-Ogievskiy]

06.08.2020 16:57, Stefan Reiter wrote:
> Since commit 42ac214406e0 (block/block-copy: refactor task creation)
> block_copy_task_create calculates the area to be copied via
> bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
> count if the backing image's last cluster end is not aligned to the

Hmm, I assume you mean not "backing image" but just "image"? Backing seems unrelated, the problem is just unaligned image

> bitmap's granularity.
> 
> Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
> which requires the 'bytes' parameter to be aligned to cluster size.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
> 
> This causes backups with unaligned image sizes to fail on the last block in my
> testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
> bytes).

Ohh. Sorry for this :(

I think, we want this case covered by some iotest.. Are you going to add a test? Or I can do it..

> 
> Alternatively one could remove the
>    assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
> from block_copy_do_copy, but I'd wager that's there for a reason?

Looking at the code I think, that the reason is just a convention. It may be changed, but it will need an audit of the code. For me your fix looks the correct thing to do.

> 
>   block/block-copy.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index f7428a7c08..023cb03200 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>           return NULL;
>       }
>   

For readability, I'd also add an assertion:

       assert(QEMU_IS_ALIGNED(offset, s->cluster_size));

> +    bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
> +
>       /* region is dirty, so no existent tasks possible in it */
>       assert(!find_conflicting_task(s, offset, bytes));
>   
> 

Thanks for fixing!
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH for-5.1?] block/block-copy: always align copied region to cluster size

[Eric Blake]

On 8/6/20 8:57 AM, Stefan Reiter wrote:
> Since commit 42ac214406e0 (block/block-copy: refactor task creation)
> block_copy_task_create calculates the area to be copied via
> bdrv_dirty_bitmap_next_dirty_area, but that can return an unaligned byte
> count if the backing image's last cluster end is not aligned to the
> bitmap's granularity.
> 
> Always ALIGN_UP the resulting bytes value to satisfy block_copy_do_copy,
> which requires the 'bytes' parameter to be aligned to cluster size.
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---

As this is an assertion failure in a feature new to 5.1, this might be a 
candidate for inclusion if we have other reasons to go with -rc4.  But 
it's awfully late, I don't think this bug is sufficient on its own to 
delay the release.

> 
> This causes backups with unaligned image sizes to fail on the last block in my
> testing (e.g. a backup job with 4k cluster size fails on a drive with 4097
> bytes).
> 
> Alternatively one could remove the
>    assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
> from block_copy_do_copy, but I'd wager that's there for a reason?
> 
>   block/block-copy.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/block/block-copy.c b/block/block-copy.c
> index f7428a7c08..023cb03200 100644
> --- a/block/block-copy.c
> +++ b/block/block-copy.c
> @@ -142,6 +142,8 @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
>           return NULL;
>       }
>   
> +    bytes = QEMU_ALIGN_UP(bytes, s->cluster_size);
> +
>       /* region is dirty, so no existent tasks possible in it */
>       assert(!find_conflicting_task(s, offset, bytes));
>   
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org