[Qemu-devel] [PATCH for-4.1 0/2] backup: Copy only dirty areas

[Qemu-devel] [PATCH for-4.1 0/2] backup: Copy only dirty areas

[Max Reitz]

Hi,

In a discussion with Vladimir today, we noticed that the backup job
currently is pretty broken when using copy offloading.  I don’t know
about you, but my local filesystem (XFS) supports copy offloading, so
the job uses it automatically.  That means, that backup is broken and
has been broken for a year on my local FS.

The last working version was 2.12, so this isn’t a regression in 4.1.
We could thus justify moving it to 4.2.  But I think this should really
go into 4.1, because this is not really an edge case and as far as I
know users cannot do anything to prevent the backup job from performing
copy offloading if the system and all involved block drivers support it.
I just wonder why nobody has noticed...


Max Reitz (2):
  backup: Copy only dirty areas
  iotests: Test backup job with two guest writes

 block/backup.c             | 13 +++++++++++--
 tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/056.out |  4 ++--
 3 files changed, 47 insertions(+), 4 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH for-4.1 1/2] backup: Copy only dirty areas

[Max Reitz]

The backup job must only copy areas that the copy_bitmap reports as
dirty.  This is always the case when using traditional non-offloading
backup, because it copies each cluster separately.  When offloading the
copy operation, we sometimes copy more than one cluster at a time, but
we only check whether the first one is dirty.

Therefore, whenever copy offloading is possible, the backup job
currently produces wrong output when the guest writes to an area of
which an inner part has already been backed up, because that inner part
will be re-copied.

Fixes: 9ded4a0114968e98b41494fc035ba14f84cdf700
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/backup.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 715e1d3be8..1ee271f9f1 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -202,22 +202,31 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
     cow_request_begin(&cow_request, job, start, end);
 
     while (start < end) {
+        int64_t dirty_end;
+
         if (!hbitmap_get(job->copy_bitmap, start)) {
             trace_backup_do_cow_skip(job, start);
             start += job->cluster_size;
             continue; /* already copied */
         }
 
+        dirty_end = hbitmap_next_zero(job->copy_bitmap, start, (end - start));
+        if (dirty_end < 0) {
+            dirty_end = end;
+        }
+
         trace_backup_do_cow_process(job, start);
 
         if (job->use_copy_range) {
-            ret = backup_cow_with_offload(job, start, end, is_write_notifier);
+            ret = backup_cow_with_offload(job, start, dirty_end,
+                                          is_write_notifier);
             if (ret < 0) {
                 job->use_copy_range = false;
             }
         }
         if (!job->use_copy_range) {
-            ret = backup_cow_with_bounce_buffer(job, start, end, is_write_notifier,
+            ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
+                                                is_write_notifier,
                                                 error_is_read, &bounce_buffer);
         }
         if (ret < 0) {
-- 
2.21.0

Re: [Qemu-devel] [PATCH for-4.1 1/2] backup: Copy only dirty areas

[Vladimir Sementsov-Ogievskiy]

01.08.2019 18:12, Max Reitz wrote:
> The backup job must only copy areas that the copy_bitmap reports as
> dirty.  This is always the case when using traditional non-offloading
> backup, because it copies each cluster separately.  When offloading the
> copy operation, we sometimes copy more than one cluster at a time, but
> we only check whether the first one is dirty.
> 
> Therefore, whenever copy offloading is possible, the backup job
> currently produces wrong output when the guest writes to an area of
> which an inner part has already been backed up, because that inner part
> will be re-copied.
> 
> Fixes: 9ded4a0114968e98b41494fc035ba14f84cdf700
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/backup.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 715e1d3be8..1ee271f9f1 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -202,22 +202,31 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>       cow_request_begin(&cow_request, job, start, end);
>   
>       while (start < end) {
> +        int64_t dirty_end;
> +
>           if (!hbitmap_get(job->copy_bitmap, start)) {
>               trace_backup_do_cow_skip(job, start);
>               start += job->cluster_size;
>               continue; /* already copied */
>           }
>   
> +        dirty_end = hbitmap_next_zero(job->copy_bitmap, start, (end - start));
> +        if (dirty_end < 0) {
> +            dirty_end = end;
> +        }

1. hbitmap_next_dirty_area is better I think
2. we can do it only in use_copy_range case, as backup_cow_with_bounce_buffer copies
    only one cluster anyway

But this all should be refactored anyway, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

> +
>           trace_backup_do_cow_process(job, start);
>   
>           if (job->use_copy_range) {
> -            ret = backup_cow_with_offload(job, start, end, is_write_notifier);
> +            ret = backup_cow_with_offload(job, start, dirty_end,
> +                                          is_write_notifier);
>               if (ret < 0) {
>                   job->use_copy_range = false;
>               }
>           }
>           if (!job->use_copy_range) {
> -            ret = backup_cow_with_bounce_buffer(job, start, end, is_write_notifier,
> +            ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
> +                                                is_write_notifier,
>                                                   error_is_read, &bounce_buffer);
>           }
>           if (ret < 0) {
> 


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Max Reitz]

Perform two guest writes to not yet backed up areas of an image, where
the former touches an inner area of the latter.

Before HEAD^, copy offloading broke this in two ways:
(1) The output differs from the reference output (what the source was
    before the guest writes).
(2) But you will not see that in the failing output, because the job
    offset is reported as being greater than the job length.  This is
    because one cluster is copied twice, and thus accounted for twice,
    but of course the job length does not increase.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/056.out |  4 ++--
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
index f40fc11a09..d7198507f5 100755
--- a/tests/qemu-iotests/056
+++ b/tests/qemu-iotests/056
@@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
         self.vm = iotests.VM()
         self.test_img = img_create('test')
         self.dest_img = img_create('dest')
+        self.ref_img = img_create('ref')
         self.vm.add_drive(self.test_img)
         self.vm.launch()
 
@@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
         self.vm.shutdown()
         try_remove(self.test_img)
         try_remove(self.dest_img)
+        try_remove(self.ref_img)
 
     def hmp_io_writes(self, drive, patterns):
         for pattern in patterns:
@@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
             self.assert_qmp(event, 'data/error', qerror)
             return False
 
+    def test_overlapping_writes(self):
+        # Write something to back up
+        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
+
+        # Create a reference backup
+        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
+                                 sync='full', target=self.ref_img)
+
+        # Now to the test backup: We simulate the following guest
+        # writes:
+        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
+        #     area should be in the target image, and we must not copy
+        #     it again (because the source image has changed now)
+        #     (64k is the job's cluster size)
+        # (2) [1M, 2M): The backup job must not get overeager.  It
+        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
+        #     but not the area in between.
+
+        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
+                        target=self.dest_img, speed=1)
+
+        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
+                                      ('66', '1M', '1M')])
+
+        # Let the job complete
+        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
+        self.assert_qmp(res, 'return', {})
+        self.qmp_backup_wait('drive0')
+
+        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
+                        'target image does not match reference image')
+
     def test_dismiss_false(self):
         res = self.vm.qmp('query-block-jobs')
         self.assert_qmp(res, 'return', [])
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
index dae404e278..36376bed87 100644
--- a/tests/qemu-iotests/056.out
+++ b/tests/qemu-iotests/056.out
@@ -1,5 +1,5 @@
-.........
+..........
 ----------------------------------------------------------------------
-Ran 9 tests
+Ran 10 tests
 
 OK
-- 
2.21.0

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Vladimir Sementsov-Ogievskiy]

01.08.2019 18:12, Max Reitz wrote:
> Perform two guest writes to not yet backed up areas of an image, where
> the former touches an inner area of the latter.
> 
> Before HEAD^, copy offloading broke this in two ways:
> (1) The output differs from the reference output (what the source was
>      before the guest writes).
> (2) But you will not see that in the failing output, because the job
>      offset is reported as being greater than the job length.  This is
>      because one cluster is copied twice, and thus accounted for twice,
>      but of course the job length does not increase.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/056.out |  4 ++--
>   2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
> index f40fc11a09..d7198507f5 100755
> --- a/tests/qemu-iotests/056
> +++ b/tests/qemu-iotests/056
> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>           self.vm = iotests.VM()
>           self.test_img = img_create('test')
>           self.dest_img = img_create('dest')
> +        self.ref_img = img_create('ref')
>           self.vm.add_drive(self.test_img)
>           self.vm.launch()
>   
> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>           self.vm.shutdown()
>           try_remove(self.test_img)
>           try_remove(self.dest_img)
> +        try_remove(self.ref_img)
>   
>       def hmp_io_writes(self, drive, patterns):
>           for pattern in patterns:
> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>               self.assert_qmp(event, 'data/error', qerror)
>               return False
>   
> +    def test_overlapping_writes(self):
> +        # Write something to back up
> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
> +
> +        # Create a reference backup
> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
> +                                 sync='full', target=self.ref_img)
> +
> +        # Now to the test backup: We simulate the following guest
> +        # writes:
> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
> +        #     area should be in the target image, and we must not copy
> +        #     it again (because the source image has changed now)
> +        #     (64k is the job's cluster size)
> +        # (2) [1M, 2M): The backup job must not get overeager.  It
> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
> +        #     but not the area in between.
> +
> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
> +                        target=self.dest_img, speed=1)
> +
> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
> +                                      ('66', '1M', '1M')])
> +
> +        # Let the job complete
> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
> +        self.assert_qmp(res, 'return', {})
> +        self.qmp_backup_wait('drive0')
> +
> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
> +                        'target image does not match reference image')
> +
>       def test_dismiss_false(self):
>           res = self.vm.qmp('query-block-jobs')
>           self.assert_qmp(res, 'return', [])
> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
> index dae404e278..36376bed87 100644
> --- a/tests/qemu-iotests/056.out
> +++ b/tests/qemu-iotests/056.out
> @@ -1,5 +1,5 @@
> -.........
> +..........
>   ----------------------------------------------------------------------
> -Ran 9 tests
> +Ran 10 tests
>   
>   OK
> 

Failed for me:
-..........
+qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
+Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
+......F...
+======================================================================
+FAIL: test_overlapping_writes (__main__.BackupTest)
+----------------------------------------------------------------------
+Traceback (most recent call last):
+  File "056", line 212, in test_overlapping_writes
+    'target image does not match reference image')
+AssertionError: False is not true : target image does not match reference image
+
  ----------------------------------------------------------------------
  Ran 10 tests

-OK
+FAILED (failures=1)



So, with applied

@@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
          res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
          self.assert_qmp(res, 'return', {})
          self.qmp_backup_wait('drive0')
+        self.vm.shutdown()

          self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
                          'target image does not match reference image')



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

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Max Reitz]

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

On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 18:12, Max Reitz wrote:
>> Perform two guest writes to not yet backed up areas of an image, where
>> the former touches an inner area of the latter.
>>
>> Before HEAD^, copy offloading broke this in two ways:
>> (1) The output differs from the reference output (what the source was
>>      before the guest writes).
>> (2) But you will not see that in the failing output, because the job
>>      offset is reported as being greater than the job length.  This is
>>      because one cluster is copied twice, and thus accounted for twice,
>>      but of course the job length does not increase.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/056.out |  4 ++--
>>   2 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>> index f40fc11a09..d7198507f5 100755
>> --- a/tests/qemu-iotests/056
>> +++ b/tests/qemu-iotests/056
>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>           self.vm = iotests.VM()
>>           self.test_img = img_create('test')
>>           self.dest_img = img_create('dest')
>> +        self.ref_img = img_create('ref')
>>           self.vm.add_drive(self.test_img)
>>           self.vm.launch()
>>   
>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>           self.vm.shutdown()
>>           try_remove(self.test_img)
>>           try_remove(self.dest_img)
>> +        try_remove(self.ref_img)
>>   
>>       def hmp_io_writes(self, drive, patterns):
>>           for pattern in patterns:
>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>               self.assert_qmp(event, 'data/error', qerror)
>>               return False
>>   
>> +    def test_overlapping_writes(self):
>> +        # Write something to back up
>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>> +
>> +        # Create a reference backup
>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>> +                                 sync='full', target=self.ref_img)
>> +
>> +        # Now to the test backup: We simulate the following guest
>> +        # writes:
>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>> +        #     area should be in the target image, and we must not copy
>> +        #     it again (because the source image has changed now)
>> +        #     (64k is the job's cluster size)
>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>> +        #     but not the area in between.
>> +
>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>> +                        target=self.dest_img, speed=1)
>> +
>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>> +                                      ('66', '1M', '1M')])
>> +
>> +        # Let the job complete
>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>> +        self.assert_qmp(res, 'return', {})
>> +        self.qmp_backup_wait('drive0')
>> +
>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>> +                        'target image does not match reference image')
>> +
>>       def test_dismiss_false(self):
>>           res = self.vm.qmp('query-block-jobs')
>>           self.assert_qmp(res, 'return', [])
>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>> index dae404e278..36376bed87 100644
>> --- a/tests/qemu-iotests/056.out
>> +++ b/tests/qemu-iotests/056.out
>> @@ -1,5 +1,5 @@
>> -.........
>> +..........
>>   ----------------------------------------------------------------------
>> -Ran 9 tests
>> +Ran 10 tests
>>   
>>   OK
>>
> 
> Failed for me:
> -..........
> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
> +......F...
> +======================================================================
> +FAIL: test_overlapping_writes (__main__.BackupTest)
> +----------------------------------------------------------------------
> +Traceback (most recent call last):
> +  File "056", line 212, in test_overlapping_writes
> +    'target image does not match reference image')
> +AssertionError: False is not true : target image does not match reference image
> +
>   ----------------------------------------------------------------------
>   Ran 10 tests
> 
> -OK
> +FAILED (failures=1)

Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.

> So, with applied
> 
> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>           res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>           self.assert_qmp(res, 'return', {})
>           self.qmp_backup_wait('drive0')
> +        self.vm.shutdown()
> 
>           self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>                           'target image does not match reference image')

I’d personally prefer auto_dismiss=False and then block-job-dismiss.
Although I can’t give a reason why.

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

In any case, thanks!

Max


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

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Vladimir Sementsov-Ogievskiy]

01.08.2019 20:06, Max Reitz wrote:
> On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
>> 01.08.2019 18:12, Max Reitz wrote:
>>> Perform two guest writes to not yet backed up areas of an image, where
>>> the former touches an inner area of the latter.
>>>
>>> Before HEAD^, copy offloading broke this in two ways:
>>> (1) The output differs from the reference output (what the source was
>>>       before the guest writes).
>>> (2) But you will not see that in the failing output, because the job
>>>       offset is reported as being greater than the job length.  This is
>>>       because one cluster is copied twice, and thus accounted for twice,
>>>       but of course the job length does not increase.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>    tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>>    tests/qemu-iotests/056.out |  4 ++--
>>>    2 files changed, 36 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>>> index f40fc11a09..d7198507f5 100755
>>> --- a/tests/qemu-iotests/056
>>> +++ b/tests/qemu-iotests/056
>>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>>            self.vm = iotests.VM()
>>>            self.test_img = img_create('test')
>>>            self.dest_img = img_create('dest')
>>> +        self.ref_img = img_create('ref')
>>>            self.vm.add_drive(self.test_img)
>>>            self.vm.launch()
>>>    
>>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>>            self.vm.shutdown()
>>>            try_remove(self.test_img)
>>>            try_remove(self.dest_img)
>>> +        try_remove(self.ref_img)
>>>    
>>>        def hmp_io_writes(self, drive, patterns):
>>>            for pattern in patterns:
>>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>>                self.assert_qmp(event, 'data/error', qerror)
>>>                return False
>>>    
>>> +    def test_overlapping_writes(self):
>>> +        # Write something to back up
>>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>>> +
>>> +        # Create a reference backup
>>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>>> +                                 sync='full', target=self.ref_img)
>>> +
>>> +        # Now to the test backup: We simulate the following guest
>>> +        # writes:
>>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>>> +        #     area should be in the target image, and we must not copy
>>> +        #     it again (because the source image has changed now)
>>> +        #     (64k is the job's cluster size)
>>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>>> +        #     but not the area in between.
>>> +
>>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>>> +                        target=self.dest_img, speed=1)
>>> +
>>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>>> +                                      ('66', '1M', '1M')])
>>> +
>>> +        # Let the job complete
>>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>> +        self.assert_qmp(res, 'return', {})
>>> +        self.qmp_backup_wait('drive0')
>>> +
>>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>> +                        'target image does not match reference image')
>>> +
>>>        def test_dismiss_false(self):
>>>            res = self.vm.qmp('query-block-jobs')
>>>            self.assert_qmp(res, 'return', [])
>>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>>> index dae404e278..36376bed87 100644
>>> --- a/tests/qemu-iotests/056.out
>>> +++ b/tests/qemu-iotests/056.out
>>> @@ -1,5 +1,5 @@
>>> -.........
>>> +..........
>>>    ----------------------------------------------------------------------
>>> -Ran 9 tests
>>> +Ran 10 tests
>>>    
>>>    OK
>>>
>>
>> Failed for me:
>> -..........
>> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
>> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
>> +......F...
>> +======================================================================
>> +FAIL: test_overlapping_writes (__main__.BackupTest)
>> +----------------------------------------------------------------------
>> +Traceback (most recent call last):
>> +  File "056", line 212, in test_overlapping_writes
>> +    'target image does not match reference image')
>> +AssertionError: False is not true : target image does not match reference image
>> +
>>    ----------------------------------------------------------------------
>>    Ran 10 tests
>>
>> -OK
>> +FAILED (failures=1)
> 
> Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.

The problem is higher: "Failed to get shared "write" lock". Because of this iotests.compare_images
can't work. So, because of locking, we need to shutdown qemu before starting qemu-img.
And it's done so in test_complete_top() (I just looked at it as it's the only other user of compare_images
in 056)

> 
>> So, with applied
>>
>> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>>            res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>            self.assert_qmp(res, 'return', {})
>>            self.qmp_backup_wait('drive0')
>> +        self.vm.shutdown()
>>
>>            self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>                            'target image does not match reference image')
> 
> I’d personally prefer auto_dismiss=False and then block-job-dismiss.
> Although I can’t give a reason why.
> 
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> In any case, thanks!
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Max Reitz]

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

On 01.08.19 19:25, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 20:06, Max Reitz wrote:
>> On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>> 01.08.2019 18:12, Max Reitz wrote:
>>>> Perform two guest writes to not yet backed up areas of an image, where
>>>> the former touches an inner area of the latter.
>>>>
>>>> Before HEAD^, copy offloading broke this in two ways:
>>>> (1) The output differs from the reference output (what the source was
>>>>       before the guest writes).
>>>> (2) But you will not see that in the failing output, because the job
>>>>       offset is reported as being greater than the job length.  This is
>>>>       because one cluster is copied twice, and thus accounted for twice,
>>>>       but of course the job length does not increase.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>>>    tests/qemu-iotests/056.out |  4 ++--
>>>>    2 files changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>>>> index f40fc11a09..d7198507f5 100755
>>>> --- a/tests/qemu-iotests/056
>>>> +++ b/tests/qemu-iotests/056
>>>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>            self.vm = iotests.VM()
>>>>            self.test_img = img_create('test')
>>>>            self.dest_img = img_create('dest')
>>>> +        self.ref_img = img_create('ref')
>>>>            self.vm.add_drive(self.test_img)
>>>>            self.vm.launch()
>>>>    
>>>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>            self.vm.shutdown()
>>>>            try_remove(self.test_img)
>>>>            try_remove(self.dest_img)
>>>> +        try_remove(self.ref_img)
>>>>    
>>>>        def hmp_io_writes(self, drive, patterns):
>>>>            for pattern in patterns:
>>>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>>>                self.assert_qmp(event, 'data/error', qerror)
>>>>                return False
>>>>    
>>>> +    def test_overlapping_writes(self):
>>>> +        # Write something to back up
>>>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>>>> +
>>>> +        # Create a reference backup
>>>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>>>> +                                 sync='full', target=self.ref_img)
>>>> +
>>>> +        # Now to the test backup: We simulate the following guest
>>>> +        # writes:
>>>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>>>> +        #     area should be in the target image, and we must not copy
>>>> +        #     it again (because the source image has changed now)
>>>> +        #     (64k is the job's cluster size)
>>>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>>>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>>>> +        #     but not the area in between.
>>>> +
>>>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>>>> +                        target=self.dest_img, speed=1)
>>>> +
>>>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>>>> +                                      ('66', '1M', '1M')])
>>>> +
>>>> +        # Let the job complete
>>>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>> +        self.assert_qmp(res, 'return', {})
>>>> +        self.qmp_backup_wait('drive0')
>>>> +
>>>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>> +                        'target image does not match reference image')
>>>> +
>>>>        def test_dismiss_false(self):
>>>>            res = self.vm.qmp('query-block-jobs')
>>>>            self.assert_qmp(res, 'return', [])
>>>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>>>> index dae404e278..36376bed87 100644
>>>> --- a/tests/qemu-iotests/056.out
>>>> +++ b/tests/qemu-iotests/056.out
>>>> @@ -1,5 +1,5 @@
>>>> -.........
>>>> +..........
>>>>    ----------------------------------------------------------------------
>>>> -Ran 9 tests
>>>> +Ran 10 tests
>>>>    
>>>>    OK
>>>>
>>>
>>> Failed for me:
>>> -..........
>>> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
>>> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
>>> +......F...
>>> +======================================================================
>>> +FAIL: test_overlapping_writes (__main__.BackupTest)
>>> +----------------------------------------------------------------------
>>> +Traceback (most recent call last):
>>> +  File "056", line 212, in test_overlapping_writes
>>> +    'target image does not match reference image')
>>> +AssertionError: False is not true : target image does not match reference image
>>> +
>>>    ----------------------------------------------------------------------
>>>    Ran 10 tests
>>>
>>> -OK
>>> +FAILED (failures=1)
>>
>> Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.
> 
> The problem is higher: "Failed to get shared "write" lock". Because of this iotests.compare_images
> can't work. So, because of locking, we need to shutdown qemu before starting qemu-img.
> And it's done so in test_complete_top() (I just looked at it as it's the only other user of compare_images
> in 056)

The destination image shouldn’t be in use by qemu after the block job is
done, though.  Therefore, there shouldn’t be a lock issue.  That’s what
I meant.

Max

>>> So, with applied
>>>
>>> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>>>            res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>            self.assert_qmp(res, 'return', {})
>>>            self.qmp_backup_wait('drive0')
>>> +        self.vm.shutdown()
>>>
>>>            self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>                            'target image does not match reference image')
>>
>> I’d personally prefer auto_dismiss=False and then block-job-dismiss.
>> Although I can’t give a reason why.
>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> In any case, thanks!
>>
>> Max
>>
> 
> 



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

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Vladimir Sementsov-Ogievskiy]

01.08.2019 20:35, Max Reitz wrote:
> On 01.08.19 19:25, Vladimir Sementsov-Ogievskiy wrote:
>> 01.08.2019 20:06, Max Reitz wrote:
>>> On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>>> 01.08.2019 18:12, Max Reitz wrote:
>>>>> Perform two guest writes to not yet backed up areas of an image, where
>>>>> the former touches an inner area of the latter.
>>>>>
>>>>> Before HEAD^, copy offloading broke this in two ways:
>>>>> (1) The output differs from the reference output (what the source was
>>>>>        before the guest writes).
>>>>> (2) But you will not see that in the failing output, because the job
>>>>>        offset is reported as being greater than the job length.  This is
>>>>>        because one cluster is copied twice, and thus accounted for twice,
>>>>>        but of course the job length does not increase.
>>>>>
>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>> ---
>>>>>     tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>>>>     tests/qemu-iotests/056.out |  4 ++--
>>>>>     2 files changed, 36 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>>>>> index f40fc11a09..d7198507f5 100755
>>>>> --- a/tests/qemu-iotests/056
>>>>> +++ b/tests/qemu-iotests/056
>>>>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>>             self.vm = iotests.VM()
>>>>>             self.test_img = img_create('test')
>>>>>             self.dest_img = img_create('dest')
>>>>> +        self.ref_img = img_create('ref')
>>>>>             self.vm.add_drive(self.test_img)
>>>>>             self.vm.launch()
>>>>>     
>>>>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>>             self.vm.shutdown()
>>>>>             try_remove(self.test_img)
>>>>>             try_remove(self.dest_img)
>>>>> +        try_remove(self.ref_img)
>>>>>     
>>>>>         def hmp_io_writes(self, drive, patterns):
>>>>>             for pattern in patterns:
>>>>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>>>>                 self.assert_qmp(event, 'data/error', qerror)
>>>>>                 return False
>>>>>     
>>>>> +    def test_overlapping_writes(self):
>>>>> +        # Write something to back up
>>>>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>>>>> +
>>>>> +        # Create a reference backup
>>>>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>>>>> +                                 sync='full', target=self.ref_img)
>>>>> +
>>>>> +        # Now to the test backup: We simulate the following guest
>>>>> +        # writes:
>>>>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>>>>> +        #     area should be in the target image, and we must not copy
>>>>> +        #     it again (because the source image has changed now)
>>>>> +        #     (64k is the job's cluster size)
>>>>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>>>>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>>>>> +        #     but not the area in between.
>>>>> +
>>>>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>>>>> +                        target=self.dest_img, speed=1)
>>>>> +
>>>>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>>>>> +                                      ('66', '1M', '1M')])
>>>>> +
>>>>> +        # Let the job complete
>>>>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>>> +        self.assert_qmp(res, 'return', {})
>>>>> +        self.qmp_backup_wait('drive0')
>>>>> +
>>>>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>>> +                        'target image does not match reference image')
>>>>> +
>>>>>         def test_dismiss_false(self):
>>>>>             res = self.vm.qmp('query-block-jobs')
>>>>>             self.assert_qmp(res, 'return', [])
>>>>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>>>>> index dae404e278..36376bed87 100644
>>>>> --- a/tests/qemu-iotests/056.out
>>>>> +++ b/tests/qemu-iotests/056.out
>>>>> @@ -1,5 +1,5 @@
>>>>> -.........
>>>>> +..........
>>>>>     ----------------------------------------------------------------------
>>>>> -Ran 9 tests
>>>>> +Ran 10 tests
>>>>>     
>>>>>     OK
>>>>>
>>>>
>>>> Failed for me:
>>>> -..........
>>>> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
>>>> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
>>>> +......F...
>>>> +======================================================================
>>>> +FAIL: test_overlapping_writes (__main__.BackupTest)
>>>> +----------------------------------------------------------------------
>>>> +Traceback (most recent call last):
>>>> +  File "056", line 212, in test_overlapping_writes
>>>> +    'target image does not match reference image')
>>>> +AssertionError: False is not true : target image does not match reference image
>>>> +
>>>>     ----------------------------------------------------------------------
>>>>     Ran 10 tests
>>>>
>>>> -OK
>>>> +FAILED (failures=1)
>>>
>>> Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.
>>
>> The problem is higher: "Failed to get shared "write" lock". Because of this iotests.compare_images
>> can't work. So, because of locking, we need to shutdown qemu before starting qemu-img.
>> And it's done so in test_complete_top() (I just looked at it as it's the only other user of compare_images
>> in 056)
> 
> The destination image shouldn’t be in use by qemu after the block job is
> done, though.  Therefore, there shouldn’t be a lock issue.  That’s what
> I meant.
> 

Ah, understand. Hmm, then it's strange that I see that error..

Clearly, in job_finalize_single(), job_clean() is called first, which should call backup_clean to unref target,
then job_event_completed() is called...



> 
>>>> So, with applied
>>>>
>>>> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>             res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>>             self.assert_qmp(res, 'return', {})
>>>>             self.qmp_backup_wait('drive0')
>>>> +        self.vm.shutdown()
>>>>
>>>>             self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>>                             'target image does not match reference image')
>>>
>>> I’d personally prefer auto_dismiss=False and then block-job-dismiss.
>>> Although I can’t give a reason why.
>>>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>> In any case, thanks!
>>>
>>> Max
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Vladimir Sementsov-Ogievskiy]

01.08.2019 20:43, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 20:35, Max Reitz wrote:
>> On 01.08.19 19:25, Vladimir Sementsov-Ogievskiy wrote:
>>> 01.08.2019 20:06, Max Reitz wrote:
>>>> On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 01.08.2019 18:12, Max Reitz wrote:
>>>>>> Perform two guest writes to not yet backed up areas of an image, where
>>>>>> the former touches an inner area of the latter.
>>>>>>
>>>>>> Before HEAD^, copy offloading broke this in two ways:
>>>>>> (1) The output differs from the reference output (what the source was
>>>>>>        before the guest writes).
>>>>>> (2) But you will not see that in the failing output, because the job
>>>>>>        offset is reported as being greater than the job length.  This is
>>>>>>        because one cluster is copied twice, and thus accounted for twice,
>>>>>>        but of course the job length does not increase.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>> ---
>>>>>>     tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>>>>>     tests/qemu-iotests/056.out |  4 ++--
>>>>>>     2 files changed, 36 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>>>>>> index f40fc11a09..d7198507f5 100755
>>>>>> --- a/tests/qemu-iotests/056
>>>>>> +++ b/tests/qemu-iotests/056
>>>>>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>>>             self.vm = iotests.VM()
>>>>>>             self.test_img = img_create('test')
>>>>>>             self.dest_img = img_create('dest')
>>>>>> +        self.ref_img = img_create('ref')
>>>>>>             self.vm.add_drive(self.test_img)
>>>>>>             self.vm.launch()
>>>>>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>>>             self.vm.shutdown()
>>>>>>             try_remove(self.test_img)
>>>>>>             try_remove(self.dest_img)
>>>>>> +        try_remove(self.ref_img)
>>>>>>         def hmp_io_writes(self, drive, patterns):
>>>>>>             for pattern in patterns:
>>>>>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>>>>>                 self.assert_qmp(event, 'data/error', qerror)
>>>>>>                 return False
>>>>>> +    def test_overlapping_writes(self):
>>>>>> +        # Write something to back up
>>>>>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>>>>>> +
>>>>>> +        # Create a reference backup
>>>>>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>>>>>> +                                 sync='full', target=self.ref_img)
>>>>>> +
>>>>>> +        # Now to the test backup: We simulate the following guest
>>>>>> +        # writes:
>>>>>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>>>>>> +        #     area should be in the target image, and we must not copy
>>>>>> +        #     it again (because the source image has changed now)
>>>>>> +        #     (64k is the job's cluster size)
>>>>>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>>>>>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>>>>>> +        #     but not the area in between.
>>>>>> +
>>>>>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>>>>>> +                        target=self.dest_img, speed=1)
>>>>>> +
>>>>>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>>>>>> +                                      ('66', '1M', '1M')])
>>>>>> +
>>>>>> +        # Let the job complete
>>>>>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>>>> +        self.assert_qmp(res, 'return', {})
>>>>>> +        self.qmp_backup_wait('drive0')
>>>>>> +
>>>>>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>>>> +                        'target image does not match reference image')
>>>>>> +
>>>>>>         def test_dismiss_false(self):
>>>>>>             res = self.vm.qmp('query-block-jobs')
>>>>>>             self.assert_qmp(res, 'return', [])
>>>>>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>>>>>> index dae404e278..36376bed87 100644
>>>>>> --- a/tests/qemu-iotests/056.out
>>>>>> +++ b/tests/qemu-iotests/056.out
>>>>>> @@ -1,5 +1,5 @@
>>>>>> -.........
>>>>>> +..........
>>>>>>     ----------------------------------------------------------------------
>>>>>> -Ran 9 tests
>>>>>> +Ran 10 tests
>>>>>>     OK
>>>>>>
>>>>>
>>>>> Failed for me:
>>>>> -..........
>>>>> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
>>>>> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
>>>>> +......F...
>>>>> +======================================================================
>>>>> +FAIL: test_overlapping_writes (__main__.BackupTest)
>>>>> +----------------------------------------------------------------------
>>>>> +Traceback (most recent call last):
>>>>> +  File "056", line 212, in test_overlapping_writes
>>>>> +    'target image does not match reference image')
>>>>> +AssertionError: False is not true : target image does not match reference image
>>>>> +
>>>>>     ----------------------------------------------------------------------
>>>>>     Ran 10 tests
>>>>>
>>>>> -OK
>>>>> +FAILED (failures=1)
>>>>
>>>> Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.
>>>
>>> The problem is higher: "Failed to get shared "write" lock". Because of this iotests.compare_images
>>> can't work. So, because of locking, we need to shutdown qemu before starting qemu-img.
>>> And it's done so in test_complete_top() (I just looked at it as it's the only other user of compare_images
>>> in 056)
>>
>> The destination image shouldn’t be in use by qemu after the block job is
>> done, though.  Therefore, there shouldn’t be a lock issue.  That’s what
>> I meant.
>>
> 
> Ah, understand. Hmm, then it's strange that I see that error..
> 
> Clearly, in job_finalize_single(), job_clean() is called first, which should call backup_clean to unref target,
> then job_event_completed() is called...
> 
> 

But BlockJob.nodes are removed at very-end from block_job_free, so it seems to be another reference to target,
which is still alive when event is sent.

> 
>>
>>>>> So, with applied
>>>>>
>>>>> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>>             res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>>>             self.assert_qmp(res, 'return', {})
>>>>>             self.qmp_backup_wait('drive0')
>>>>> +        self.vm.shutdown()
>>>>>
>>>>>             self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>>>                             'target image does not match reference image')
>>>>
>>>> I’d personally prefer auto_dismiss=False and then block-job-dismiss.
>>>> Although I can’t give a reason why.
>>>>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>
>>>> In any case, thanks!
>>>>
>>>> Max
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH for-4.1 0/2] backup: Copy only dirty areas

[Vladimir Sementsov-Ogievskiy]

01.08.2019 18:12, Max Reitz wrote:
> Hi,
> 
> In a discussion with Vladimir today, we noticed that the backup job
> currently is pretty broken when using copy offloading.  I don’t know
> about you, but my local filesystem (XFS) supports copy offloading, so
> the job uses it automatically.  That means, that backup is broken and
> has been broken for a year on my local FS.
> 
> The last working version was 2.12, so this isn’t a regression in 4.1.
> We could thus justify moving it to 4.2.  But I think this should really
> go into 4.1, because this is not really an edge case and as far as I
> know users cannot do anything to prevent the backup job from performing
> copy offloading if the system and all involved block drivers support it.
> I just wonder why nobody has noticed...
> 

Agree. And if this goes into 4.1, I suggest to add
[PATCH 2/3] block/backup: disable copy_range for compressed backup

as for now compressed backup just don't compress anything on FS with
copy offloading supported.

> 
> Max Reitz (2):
>    backup: Copy only dirty areas
>    iotests: Test backup job with two guest writes
> 
>   block/backup.c             | 13 +++++++++++--
>   tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/056.out |  4 ++--
>   3 files changed, 47 insertions(+), 4 deletions(-)
> 


-- 
Best regards,
Vladimir