[for-5.1,v2,1/2] block/block-copy: always align copied region to cluster size

Commit Message

Stefan Reiter Aug. 10, 2020, 9:55 a.m. UTC

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 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.

Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
Signed-off-by: Stefan Reiter <[e-mail]>
---

I've now marked it for-5.1 since it is just a fix, but it's probably okay if
done later as well.

v2:
* add assert on offset alignment
* remove 'backing image' wording from commit
* collect R-b

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

Comments

Max Reitz Aug. 10, 2020, 3:15 p.m. UTC | #1

On 10.08.20 11:55, 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 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.
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <[e-mail]>
> Signed-off-by: Stefan Reiter <[e-mail]>
> ---
> 
> I've now marked it for-5.1 since it is just a fix, but it's probably okay if
> done later as well.

42ac214406e0 wasn’t in 5.0, so this would be a regression if we don’t
get it in 5.1.  I suppose this is an edge case, because most images
should be aligned to the cluster size, but I think objectively this is
something for 5.1.

So I’ll apply this series to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

And I think I’m going to send a pull request tomorrow morning.  I see
there’s another patch for 5.1 on the list, so it should be OK.

If you want me to act on any of the suggestions I gave on your test,
feel free to say so and I’ll handle those that make sense to you (like,
I hope the s/4097/4096/ thing perhaps).

Thanks for finding and fixing this!

Max

Patch

diff --git a/block/block-copy.c b/block/block-copy.c
index f7428a7c08..a30b9097ef 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -142,6 +142,9 @@  static BlockCopyTask *block_copy_task_create(BlockCopyState *s,
         return NULL;
     }
 
+    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));