[PATCH 0/3] backup: Make sure that source and target size match

[PATCH 0/3] backup: Make sure that source and target size match

[Kevin Wolf]

Kevin Wolf (3):
  backup: Improve error for bdrv_getlength() failure
  backup: Make sure that source and target size match
  iotests: Backup with different source/target size

 block/backup-top.c         | 12 +++++---
 block/backup.c             | 18 ++++++++++--
 tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/055.out |  4 +--
 4 files changed, 83 insertions(+), 11 deletions(-)

-- 
2.25.3

[PATCH 1/3] backup: Improve error for bdrv_getlength() failure

[Kevin Wolf]

bdrv_get_device_name() will be an empty string with modern management
tools that don't use -drive. Use bdrv_get_device_or_node_name() instead
so that the node name is used if the BlockBackend is anonymous.

While at it, start with upper case to make the message consistent with
the rest of the function.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index a7a7dcaf4c..c4c3b8cd46 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -400,8 +400,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
 
     len = bdrv_getlength(bs);
     if (len < 0) {
-        error_setg_errno(errp, -len, "unable to get length for '%s'",
-                         bdrv_get_device_name(bs));
+        error_setg_errno(errp, -len, "Unable to get length for '%s'",
+                         bdrv_get_device_or_node_name(bs));
         goto error;
     }
 
-- 
2.25.3

[PATCH 2/3] backup: Make sure that source and target size match

[Kevin Wolf]

Since the introduction of a backup filter node, the backup block job
crashes when the target image is smaller than the source image because
it will try to write after the end of the target node without having
BLK_PERM_RESIZE. (Previously, the BlockBackend layer would have caught
this and errored out gracefully.)

We can fix this and even do better than the old behaviour: Check that
source and target have the same image size at the start of the block job
and unshare BLK_PERM_RESIZE. This will immediately error out when
starting the job instead of only when writing to a block that doesn't
exist in the target.

Longer target than source would technically work because we would never
write to blocks that don't exist, but semantically these are invalid,
too, because a backup is supposed to create a copy, not just an image
that starts with a copy.

The bugs were introduced in commits 2c8074c45 (BLK_PERM_RESIZE is shared
since this commit) and 00e30f05d (BdrvChild instead of BlockBackend
turns I/O errors into assertion failures).

Fixes: 2c8074c453ff13a94bd08ec26061917670ec03be
Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/backup-top.c | 12 ++++++++----
 block/backup.c     | 14 +++++++++++++-
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/block/backup-top.c b/block/backup-top.c
index 3b50c06e2c..0e515a7705 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
          *
          * Share write to target (child_file), to not interfere
          * with guest writes to its disk which may be in target backing chain.
+         * Can't resize during a backup block job because we check the size
+         * only upfront.
          */
-        *nshared = BLK_PERM_ALL;
+        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
         *nperm = BLK_PERM_WRITE;
     } else {
         /* Source child */
@@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
 {
     Error *local_err = NULL;
     BDRVBackupTopState *state;
-    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
-                                                 filter_node_name,
-                                                 BDRV_O_RDWR, errp);
+    BlockDriverState *top;
     bool appended = false;
 
+    assert(source->total_sectors == target->total_sectors);
+
+    top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name,
+                              BDRV_O_RDWR, errp);
     if (!top) {
         return NULL;
     }
diff --git a/block/backup.c b/block/backup.c
index c4c3b8cd46..4f13bb20a5 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -340,7 +340,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
                   BlockCompletionFunc *cb, void *opaque,
                   JobTxn *txn, Error **errp)
 {
-    int64_t len;
+    int64_t len, target_len;
     BackupBlockJob *job = NULL;
     int64_t cluster_size;
     BdrvRequestFlags write_flags;
@@ -405,6 +405,18 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
         goto error;
     }
 
+    target_len = bdrv_getlength(target);
+    if (target_len < 0) {
+        error_setg_errno(errp, -target_len, "Unable to get length for '%s'",
+                         bdrv_get_device_or_node_name(bs));
+        goto error;
+    }
+
+    if (target_len != len) {
+        error_setg(errp, "Source and target image have different sizes");
+        goto error;
+    }
+
     cluster_size = backup_calculate_cluster_size(target, errp);
     if (cluster_size < 0) {
         goto error;
-- 
2.25.3

[PATCH 3/3] iotests: Backup with different source/target size

[Kevin Wolf]

This tests that the backup jobs catches situations where the target node
has a different size than the source node. It must also forbid resize
operations when the job is already running.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/055.out |  4 +--
 2 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
index 82b9f5f47d..243d66a62e 100755
--- a/tests/qemu-iotests/055
+++ b/tests/qemu-iotests/055
@@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
     def setUp(self):
         qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
 
-        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
-        self.vm.add_drive(blockdev_target_img, interface="none")
+        self.vm = iotests.VM()
+        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
+        self.vm.add_drive(blockdev_target_img, 'node-name=target',
+                          interface="none")
         if iotests.qemu_default_machine == 'pc':
             self.vm.add_drive(None, 'media=cdrom', 'ide')
         self.vm.launch()
@@ -112,6 +114,60 @@ class TestSingleDrive(iotests.QMPTestCase):
     def test_pause_blockdev_backup(self):
         self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
 
+    def test_source_resize_blockdev_backup(self):
+        self.assert_no_active_block_jobs()
+
+        def pre_finalize():
+            result = self.vm.qmp('block_resize', device='drive0', size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+            result = self.vm.qmp('block_resize', node_name='source', size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full', auto_finalize=False,
+                             auto_dismiss=False)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
+                        use_log=False)
+
+    def test_target_resize_blockdev_backup(self):
+        self.assert_no_active_block_jobs()
+
+        def pre_finalize():
+            result = self.vm.qmp('block_resize', device='drive1', size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+            result = self.vm.qmp('block_resize', node_name='target', size=65536)
+            self.assert_qmp(result, 'error/class', 'GenericError')
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full', auto_finalize=False,
+                             auto_dismiss=False)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
+                        use_log=False)
+
+    def test_small_target(self):
+        short_len = image_len // 2
+        result = self.vm.qmp('block_resize', device='drive1', size=short_len)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_large_target(self):
+        short_len = image_len * 2
+        result = self.vm.qmp('block_resize', device='drive1', size=short_len)
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
+                             target='drive1', sync='full')
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
     def test_medium_not_found(self):
         if iotests.qemu_default_machine != 'pc':
             return
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
index 5ce2f9a2ed..88bf7fa73a 100644
--- a/tests/qemu-iotests/055.out
+++ b/tests/qemu-iotests/055.out
@@ -1,5 +1,5 @@
-..............................
+..................................
 ----------------------------------------------------------------------
-Ran 30 tests
+Ran 34 tests
 
 OK
-- 
2.25.3

Re: [PATCH 1/3] backup: Improve error for bdrv_getlength() failure

[Vladimir Sementsov-Ogievskiy]

29.04.2020 14:15, Kevin Wolf wrote:
> bdrv_get_device_name() will be an empty string with modern management
> tools that don't use -drive. Use bdrv_get_device_or_node_name() instead
> so that the node name is used if the BlockBackend is anonymous.
> 
> While at it, start with upper case to make the message consistent with
> the rest of the function.
> 
> Signed-off-by: Kevin Wolf<kwolf@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir

Re: [PATCH 2/3] backup: Make sure that source and target size match

[Vladimir Sementsov-Ogievskiy]

29.04.2020 14:15, Kevin Wolf wrote:
> Since the introduction of a backup filter node, the backup block job
> crashes when the target image is smaller than the source image because
> it will try to write after the end of the target node without having
> BLK_PERM_RESIZE. (Previously, the BlockBackend layer would have caught
> this and errored out gracefully.)
> 
> We can fix this and even do better than the old behaviour: Check that
> source and target have the same image size at the start of the block job
> and unshare BLK_PERM_RESIZE. This will immediately error out when
> starting the job instead of only when writing to a block that doesn't
> exist in the target.
> 
> Longer target than source would technically work because we would never
> write to blocks that don't exist, but semantically these are invalid,
> too, because a backup is supposed to create a copy, not just an image
> that starts with a copy.
> 
> The bugs were introduced in commits 2c8074c45 (BLK_PERM_RESIZE is shared

no, it was unshared by blks in block-copy

> since this commit) and 00e30f05d (BdrvChild instead of BlockBackend
> turns I/O errors into assertion failures).

and here becomes shared.

So, seems only 00e30f05d is broken and introduces both problems

> 
> Fixes: 2c8074c453ff13a94bd08ec26061917670ec03be
> Fixes: 00e30f05de1d19586345ec373970ef4c192c6270
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1778593
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/backup-top.c | 12 ++++++++----
>   block/backup.c     | 14 +++++++++++++-
>   2 files changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/block/backup-top.c b/block/backup-top.c
> index 3b50c06e2c..0e515a7705 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -148,8 +148,10 @@ static void backup_top_child_perm(BlockDriverState *bs, BdrvChild *c,
>            *
>            * Share write to target (child_file), to not interfere
>            * with guest writes to its disk which may be in target backing chain.
> +         * Can't resize during a backup block job because we check the size
> +         * only upfront.
>            */
> -        *nshared = BLK_PERM_ALL;
> +        *nshared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
>           *nperm = BLK_PERM_WRITE;
>       } else {
>           /* Source child */
> @@ -192,11 +194,13 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
>   {
>       Error *local_err = NULL;
>       BDRVBackupTopState *state;
> -    BlockDriverState *top = bdrv_new_open_driver(&bdrv_backup_top_filter,
> -                                                 filter_node_name,
> -                                                 BDRV_O_RDWR, errp);
> +    BlockDriverState *top;
>       bool appended = false;
>   
> +    assert(source->total_sectors == target->total_sectors);

Is it correct to use directly total_sectors and not bdrv_getlenght()?
Anyway, using bdrv_getlength() seems safer, and will help us if we move
to byte-accurate block-layer at some moment in future.

Hmm but total_sectors used directly anyway in this function, so OK


> +
> +    top = bdrv_new_open_driver(&bdrv_backup_top_filter, filter_node_name,
> +                              BDRV_O_RDWR, errp);

alignment broken. With it fixed:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

>       if (!top) {
>           return NULL;
>       }
> diff --git a/block/backup.c b/block/backup.c
> index c4c3b8cd46..4f13bb20a5 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -340,7 +340,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>                     BlockCompletionFunc *cb, void *opaque,
>                     JobTxn *txn, Error **errp)
>   {
> -    int64_t len;
> +    int64_t len, target_len;
>       BackupBlockJob *job = NULL;
>       int64_t cluster_size;
>       BdrvRequestFlags write_flags;
> @@ -405,6 +405,18 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
>           goto error;
>       }
>   
> +    target_len = bdrv_getlength(target);
> +    if (target_len < 0) {
> +        error_setg_errno(errp, -target_len, "Unable to get length for '%s'",
> +                         bdrv_get_device_or_node_name(bs));
> +        goto error;
> +    }
> +
> +    if (target_len != len) {
> +        error_setg(errp, "Source and target image have different sizes");
> +        goto error;
> +    }
> +
>       cluster_size = backup_calculate_cluster_size(target, errp);
>       if (cluster_size < 0) {
>           goto error;
> 


-- 
Best regards,
Vladimir

Re: [PATCH 3/3] iotests: Backup with different source/target size

[Vladimir Sementsov-Ogievskiy]

29.04.2020 14:15, Kevin Wolf wrote:
> This tests that the backup jobs catches situations where the target node
> has a different size than the source node. It must also forbid resize
> operations when the job is already running.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
>   tests/qemu-iotests/055.out |  4 +--
>   2 files changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> index 82b9f5f47d..243d66a62e 100755
> --- a/tests/qemu-iotests/055
> +++ b/tests/qemu-iotests/055
> @@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
>       def setUp(self):
>           qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
>   
> -        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
> -        self.vm.add_drive(blockdev_target_img, interface="none")
> +        self.vm = iotests.VM()
> +        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
> +        self.vm.add_drive(blockdev_target_img, 'node-name=target',
> +                          interface="none")
>           if iotests.qemu_default_machine == 'pc':
>               self.vm.add_drive(None, 'media=cdrom', 'ide')
>           self.vm.launch()
> @@ -112,6 +114,60 @@ class TestSingleDrive(iotests.QMPTestCase):
>       def test_pause_blockdev_backup(self):
>           self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
>   
> +    def test_source_resize_blockdev_backup(self):
> +        self.assert_no_active_block_jobs()

this will never fire, as vm is created a moment before, I'd drop it.

> +
> +        def pre_finalize():
> +            result = self.vm.qmp('block_resize', device='drive0', size=65536)
> +            self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +            result = self.vm.qmp('block_resize', node_name='source', size=65536)
> +            self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> +                             target='drive1', sync='full', auto_finalize=False,
> +                             auto_dismiss=False)
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> +                        use_log=False)
> +
> +    def test_target_resize_blockdev_backup(self):
> +        self.assert_no_active_block_jobs()
> +
> +        def pre_finalize():
> +            result = self.vm.qmp('block_resize', device='drive1', size=65536)
> +            self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +            result = self.vm.qmp('block_resize', node_name='target', size=65536)
> +            self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> +                             target='drive1', sync='full', auto_finalize=False,
> +                             auto_dismiss=False)
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> +                        use_log=False)

these two functions are almost identical.. worth refactoring to be use common helper?

> +
> +    def test_small_target(self):
> +        short_len = image_len // 2
> +        result = self.vm.qmp('block_resize', device='drive1', size=short_len)
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> +                             target='drive1', sync='full')
> +        self.assert_qmp(result, 'error/class', 'GenericError')
> +
> +    def test_large_target(self):
> +        short_len = image_len * 2
> +        result = self.vm.qmp('block_resize', device='drive1', size=short_len)
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> +                             target='drive1', sync='full')
> +        self.assert_qmp(result, 'error/class', 'GenericError')

these are identical too, still they are smaller and hardly needs refactoring

> +
>       def test_medium_not_found(self):
>           if iotests.qemu_default_machine != 'pc':
>               return
> diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
> index 5ce2f9a2ed..88bf7fa73a 100644
> --- a/tests/qemu-iotests/055.out
> +++ b/tests/qemu-iotests/055.out
> @@ -1,5 +1,5 @@
> -..............................
> +..................................
>   ----------------------------------------------------------------------
> -Ran 30 tests
> +Ran 34 tests
>   
>   OK
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/3] backup: Improve error for bdrv_getlength() failure

[Alberto Garcia]

On Wed 29 Apr 2020 01:15:37 PM CEST, Kevin Wolf wrote:
> bdrv_get_device_name() will be an empty string with modern management
> tools that don't use -drive. Use bdrv_get_device_or_node_name() instead
> so that the node name is used if the BlockBackend is anonymous.
>
> While at it, start with upper case to make the message consistent with
> the rest of the function.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [PATCH 3/3] iotests: Backup with different source/target size

[Kevin Wolf]

Am 29.04.2020 um 14:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 29.04.2020 14:15, Kevin Wolf wrote:
> > This tests that the backup jobs catches situations where the target node
> > has a different size than the source node. It must also forbid resize
> > operations when the job is already running.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
> >   tests/qemu-iotests/055.out |  4 +--

One general remark and question that came up while I was running 055 a
lot and really got annonyed by the long time it takes:

TestDriveCompression is quite unconventional in that 055 is raw/qcow2
only per se, but some of the test cases always test qcow2 and vmdk. The
slow one is vmdk.

I found out that zero writes in vmdk are completely broken (I'll send
patches), but even after fixing this, it's still slow. I think this is
the combination of VMDK not having writeback metadata caching and
backup serving lots of tiny 64k zero writes.

Has anyone ever looked into making backup use more reasonable request
sizes for the background copy like mirror does?

> >   2 files changed, 60 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> > index 82b9f5f47d..243d66a62e 100755
> > --- a/tests/qemu-iotests/055
> > +++ b/tests/qemu-iotests/055
> > @@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
> >       def setUp(self):
> >           qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
> > -        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
> > -        self.vm.add_drive(blockdev_target_img, interface="none")
> > +        self.vm = iotests.VM()
> > +        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
> > +        self.vm.add_drive(blockdev_target_img, 'node-name=target',
> > +                          interface="none")
> >           if iotests.qemu_default_machine == 'pc':
> >               self.vm.add_drive(None, 'media=cdrom', 'ide')
> >           self.vm.launch()
> > @@ -112,6 +114,60 @@ class TestSingleDrive(iotests.QMPTestCase):
> >       def test_pause_blockdev_backup(self):
> >           self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
> > +    def test_source_resize_blockdev_backup(self):
> > +        self.assert_no_active_block_jobs()
> 
> this will never fire, as vm is created a moment before, I'd drop it.

This pattern exists all over the place in 055, but you're right, it's
kind of pointless.

> > +
> > +        def pre_finalize():
> > +            result = self.vm.qmp('block_resize', device='drive0', size=65536)
> > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > +
> > +            result = self.vm.qmp('block_resize', node_name='source', size=65536)
> > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > +
> > +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> > +                             target='drive1', sync='full', auto_finalize=False,
> > +                             auto_dismiss=False)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> > +                        use_log=False)
> > +
> > +    def test_target_resize_blockdev_backup(self):
> > +        self.assert_no_active_block_jobs()
> > +
> > +        def pre_finalize():
> > +            result = self.vm.qmp('block_resize', device='drive1', size=65536)
> > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > +
> > +            result = self.vm.qmp('block_resize', node_name='target', size=65536)
> > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > +
> > +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> > +                             target='drive1', sync='full', auto_finalize=False,
> > +                             auto_dismiss=False)
> > +        self.assert_qmp(result, 'return', {})
> > +
> > +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> > +                        use_log=False)
> 
> these two functions are almost identical.. worth refactoring to be use common helper?

Ok, I'll see what I can do.

Kevin

Re: [PATCH 3/3] iotests: Backup with different source/target size

[Vladimir Sementsov-Ogievskiy]

30.04.2020 14:41, Kevin Wolf wrote:
> Am 29.04.2020 um 14:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 29.04.2020 14:15, Kevin Wolf wrote:
>>> This tests that the backup jobs catches situations where the target node
>>> has a different size than the source node. It must also forbid resize
>>> operations when the job is already running.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
>>>    tests/qemu-iotests/055.out |  4 +--
> 
> One general remark and question that came up while I was running 055 a
> lot and really got annonyed by the long time it takes:
> 
> TestDriveCompression is quite unconventional in that 055 is raw/qcow2
> only per se, but some of the test cases always test qcow2 and vmdk.

Yes, that's bad. Oh, seems I have a patch for it not still sent. Will do soon.

> The
> slow one is vmdk.
> 
> I found out that zero writes in vmdk are completely broken (I'll send
> patches), but even after fixing this, it's still slow. I think this is
> the combination of VMDK not having writeback metadata caching and
> backup serving lots of tiny 64k zero writes.
> 
> Has anyone ever looked into making backup use more reasonable request
> sizes for the background copy like mirror does?

I am :)

The whole series improving backup is
  "[RFC 00/24] backup performance: block_status + async"
First part is already merged, current chunk close to be finished:
  "[PATCH v4 0/5] block-copy: use aio-task-pool"

> 
>>>    2 files changed, 60 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
>>> index 82b9f5f47d..243d66a62e 100755
>>> --- a/tests/qemu-iotests/055
>>> +++ b/tests/qemu-iotests/055
>>> @@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
>>>        def setUp(self):
>>>            qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, str(image_len))
>>> -        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
>>> -        self.vm.add_drive(blockdev_target_img, interface="none")
>>> +        self.vm = iotests.VM()
>>> +        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
>>> +        self.vm.add_drive(blockdev_target_img, 'node-name=target',
>>> +                          interface="none")
>>>            if iotests.qemu_default_machine == 'pc':
>>>                self.vm.add_drive(None, 'media=cdrom', 'ide')
>>>            self.vm.launch()
>>> @@ -112,6 +114,60 @@ class TestSingleDrive(iotests.QMPTestCase):
>>>        def test_pause_blockdev_backup(self):
>>>            self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
>>> +    def test_source_resize_blockdev_backup(self):
>>> +        self.assert_no_active_block_jobs()
>>
>> this will never fire, as vm is created a moment before, I'd drop it.
> 
> This pattern exists all over the place in 055, but you're right, it's
> kind of pointless.
> 
>>> +
>>> +        def pre_finalize():
>>> +            result = self.vm.qmp('block_resize', device='drive0', size=65536)
>>> +            self.assert_qmp(result, 'error/class', 'GenericError')
>>> +
>>> +            result = self.vm.qmp('block_resize', node_name='source', size=65536)
>>> +            self.assert_qmp(result, 'error/class', 'GenericError')
>>> +
>>> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
>>> +                             target='drive1', sync='full', auto_finalize=False,
>>> +                             auto_dismiss=False)
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
>>> +                        use_log=False)
>>> +
>>> +    def test_target_resize_blockdev_backup(self):
>>> +        self.assert_no_active_block_jobs()
>>> +
>>> +        def pre_finalize():
>>> +            result = self.vm.qmp('block_resize', device='drive1', size=65536)
>>> +            self.assert_qmp(result, 'error/class', 'GenericError')
>>> +
>>> +            result = self.vm.qmp('block_resize', node_name='target', size=65536)
>>> +            self.assert_qmp(result, 'error/class', 'GenericError')
>>> +
>>> +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
>>> +                             target='drive1', sync='full', auto_finalize=False,
>>> +                             auto_dismiss=False)
>>> +        self.assert_qmp(result, 'return', {})
>>> +
>>> +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
>>> +                        use_log=False)
>>
>> these two functions are almost identical.. worth refactoring to be use common helper?
> 
> Ok, I'll see what I can do.
> 
> Kevin
> 


-- 
Best regards,
Vladimir