[PULL 0/2] Block patches for 5.1.0-rc4

[PULL 0/2] Block patches for 5.1.0-rc4

[Max Reitz]

Hi,

There is a bug in the backup job that breaks backups from images whose
size is not aligned to the job's cluster size (i.e., qemu crashes
because of a failed assertion).  If this bug makes it into the release,
it would be a regression from 5.0.

On one hand, this is probably a rare configuration that should not
happen in practice.  On the other, it is a regression, and the fix
(patch 1) is simple.  So I think it would be good to have this in 5.1.


The following changes since commit e1d322c40524d2c544d1fcd37b267d106d16d328:

  Update version for v5.1.0-rc3 release (2020-08-05 17:37:17 +0100)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-08-11

for you to fetch changes up to 1f3765b652930a3b485f1a67542c2410c3774abe:

  iotests: add test for unaligned granularity bitmap backup (2020-08-11 09:29:31 +0200)

----------------------------------------------------------------
Block patches for 5.1.0-rc4:
- Fix abort when running a backup job on an image whose size is not
  aligned to the backup job's cluster size

----------------------------------------------------------------
Stefan Reiter (2):
  block/block-copy: always align copied region to cluster size
  iotests: add test for unaligned granularity bitmap backup

 block/block-copy.c         |  3 ++
 tests/qemu-iotests/304     | 60 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/304.out |  2 ++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 66 insertions(+)
 create mode 100755 tests/qemu-iotests/304
 create mode 100644 tests/qemu-iotests/304.out

-- 
2.26.2

[PULL 1/2] block/block-copy: always align copied region to cluster size

[Max Reitz]

From: Stefan Reiter <s.reiter@proxmox.com>

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 <vsementsov@virtuozzo.com>
Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
Message-Id: <20200810095523.15071-1-s.reiter@proxmox.com>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/block-copy.c | 3 +++
 1 file changed, 3 insertions(+)

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));
 
-- 
2.26.2

[PULL 2/2] iotests: add test for unaligned granularity bitmap backup

[Max Reitz]

From: Stefan Reiter <s.reiter@proxmox.com>

Start a VM with a 4097 byte image attached, add a 4096 byte granularity
dirty bitmap, mark it dirty, and then do a backup.

This used to run into an assert and fail, check that it works as
expected and also check the created image to ensure that misaligned
backups in general work correctly.

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
Message-Id: <20200810095523.15071-2-s.reiter@proxmox.com>
[mreitz: Drop bitmap, and do not write past the image's end]
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/304     | 60 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/304.out |  2 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 63 insertions(+)
 create mode 100755 tests/qemu-iotests/304
 create mode 100644 tests/qemu-iotests/304.out

diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
new file mode 100755
index 0000000000..aaf9e14617
--- /dev/null
+++ b/tests/qemu-iotests/304
@@ -0,0 +1,60 @@
+#!/usr/bin/env python3
+#
+# Tests dirty-bitmap backup with unaligned bitmap granularity
+#
+# Copyright (c) 2020 Proxmox Server Solutions
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+# owner=s.reiter@proxmox.com
+
+import iotests
+from iotests import qemu_img_create, qemu_img_log, file_path
+
+iotests.script_initialize(supported_fmts=['qcow2'],
+                          supported_protocols=['file'])
+
+test_img = file_path('test.qcow2')
+target_img = file_path('target.qcow2')
+
+# unaligned by one byte
+image_len = 4097
+bitmap_granularity = 4096
+
+qemu_img_create('-f', iotests.imgfmt, test_img, str(image_len))
+
+# create VM
+vm = iotests.VM().add_drive(test_img)
+vm.launch()
+
+# write to the entire image
+vm.hmp_qemu_io('drive0', 'write -P0x16 0 4096');
+vm.hmp_qemu_io('drive0', 'write -P0x17 4096 1');
+
+# do backup and wait for completion
+vm.qmp('drive-backup', **{
+    'device': 'drive0',
+    'sync': 'full',
+    'target': target_img
+})
+
+event = vm.event_wait(name='BLOCK_JOB_COMPLETED',
+                      match={'data': {'device': 'drive0'}},
+                      timeout=5.0)
+
+# shutdown to sync images
+vm.shutdown()
+
+# backup succeeded, check if image is correct
+qemu_img_log('compare', test_img, target_img)
diff --git a/tests/qemu-iotests/304.out b/tests/qemu-iotests/304.out
new file mode 100644
index 0000000000..381cc056f7
--- /dev/null
+++ b/tests/qemu-iotests/304.out
@@ -0,0 +1,2 @@
+Images are identical.
+
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 025ed5238d..7f76066640 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -309,3 +309,4 @@
 299 auto quick
 301 backing quick
 302 quick
+304 rw quick
-- 
2.26.2

Re: [PULL 0/2] Block patches for 5.1.0-rc4

[Peter Maydell]

On Tue, 11 Aug 2020 at 10:35, Max Reitz <mreitz@redhat.com> wrote:
>
> Hi,
>
> There is a bug in the backup job that breaks backups from images whose
> size is not aligned to the job's cluster size (i.e., qemu crashes
> because of a failed assertion).  If this bug makes it into the release,
> it would be a regression from 5.0.
>
> On one hand, this is probably a rare configuration that should not
> happen in practice.  On the other, it is a regression, and the fix
> (patch 1) is simple.  So I think it would be good to have this in 5.1.

I'm really reluctant to have to roll an rc4...

thanks
-- PMM

Re: [PULL 0/2] Block patches for 5.1.0-rc4

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 880 bytes --]

On 11.08.20 11:39, Peter Maydell wrote:
> On Tue, 11 Aug 2020 at 10:35, Max Reitz <mreitz@redhat.com> wrote:
>>
>> Hi,
>>
>> There is a bug in the backup job that breaks backups from images whose
>> size is not aligned to the job's cluster size (i.e., qemu crashes
>> because of a failed assertion).  If this bug makes it into the release,
>> it would be a regression from 5.0.
>>
>> On one hand, this is probably a rare configuration that should not
>> happen in practice.  On the other, it is a regression, and the fix
>> (patch 1) is simple.  So I think it would be good to have this in 5.1.
> 
> I'm really reluctant to have to roll an rc4...

Well, that’s the information there is on this: Regression, simple fix,
but little relevance in practice, and late to the party.
If, given this, you don’t want to roll an rc4, then that’s how it is.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PULL 0/2] Block patches for 5.1.0-rc4

[Peter Maydell]

On Tue, 11 Aug 2020 at 10:35, Max Reitz <mreitz@redhat.com> wrote:
>
> Hi,
>
> There is a bug in the backup job that breaks backups from images whose
> size is not aligned to the job's cluster size (i.e., qemu crashes
> because of a failed assertion).  If this bug makes it into the release,
> it would be a regression from 5.0.
>
> On one hand, this is probably a rare configuration that should not
> happen in practice.  On the other, it is a regression, and the fix
> (patch 1) is simple.  So I think it would be good to have this in 5.1.
>
>
> The following changes since commit e1d322c40524d2c544d1fcd37b267d106d16d328:
>
>   Update version for v5.1.0-rc3 release (2020-08-05 17:37:17 +0100)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2020-08-11
>
> for you to fetch changes up to 1f3765b652930a3b485f1a67542c2410c3774abe:
>
>   iotests: add test for unaligned granularity bitmap backup (2020-08-11 09:29:31 +0200)
>
> ----------------------------------------------------------------
> Block patches for 5.1.0-rc4:
> - Fix abort when running a backup job on an image whose size is not
>   aligned to the backup job's cluster size
>


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM

Re: [PULL 0/2] Block patches for 5.1.0-rc4

[Vladimir Sementsov-Ogievskiy]

11.08.2020 12:54, Max Reitz wrote:
> On 11.08.20 11:39, Peter Maydell wrote:
>> On Tue, 11 Aug 2020 at 10:35, Max Reitz <mreitz@redhat.com> wrote:
>>>
>>> Hi,
>>>
>>> There is a bug in the backup job that breaks backups from images whose
>>> size is not aligned to the job's cluster size (i.e., qemu crashes
>>> because of a failed assertion).  If this bug makes it into the release,
>>> it would be a regression from 5.0.
>>>
>>> On one hand, this is probably a rare configuration that should not
>>> happen in practice.  On the other, it is a regression, and the fix
>>> (patch 1) is simple.  So I think it would be good to have this in 5.1.
>>
>> I'm really reluctant to have to roll an rc4...
> 
> Well, that’s the information there is on this: Regression, simple fix,
> but little relevance in practice, and late to the party.
> If, given this, you don’t want to roll an rc4, then that’s how it is.
> 

Recently bug was reproduced by accidentally starting backup with source = cdrom (image was not 64k-cluster aligned).

Fedora 33, Rhel/Centos 8 are affected.

Could this go to stable branch?

-- 
Best regards,
Vladimir