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
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
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
© 2016 - 2024 Red Hat, Inc.