[PATCH 0/3] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'

[PATCH 0/3] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'

[Fiona Ebner]

If s->replace_blocker was already set by an earlier invocation of
mirror_complete(), then there will be an assertion failure when
error_setg() is called for it a second time. The bdrv_ref() operation
should only be done a single time too.

I thought about using an early return with an error, but that might
break existing users that (accidentally) issue duplicate
'job-complete' without using 'replaces'.

A paused job does not accept the 'complete' verb, so using an
externally throttled node was the best I could come up with for the
test.

Fiona Ebner (3):
  block/mirror: fix assertion failure upon duplicate complete for job
    using 'replaces'
  iotests/041: add test for mirror with throttled NBD export as target
  iotests/041: add test for duplicate job-complete with throttled target

 block/mirror.c             |  28 +++----
 tests/qemu-iotests/041     | 153 ++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/041.out |   4 +-
 3 files changed, 169 insertions(+), 16 deletions(-)

-- 
2.47.3

[PATCH 1/3] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'

[Fiona Ebner]

If s->replace_blocker was already set by an earlier invocation of
mirror_complete(), then there will be an assertion failure when
error_setg() is called for it a second time. The bdrv_op_block_all()
and bdrv_ref() operations should only be done a single time too.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/mirror.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index fa1d975eb9..2fcded9e93 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1276,23 +1276,25 @@ static void mirror_complete(Job *job, Error **errp)
         return;
     }
 
-    /* block all operations on to_replace bs */
-    if (s->replaces) {
-        s->to_replace = bdrv_find_node(s->replaces);
-        if (!s->to_replace) {
-            error_setg(errp, "Node name '%s' not found", s->replaces);
-            return;
+    if (!s->should_complete) {
+        /* block all operations on to_replace bs */
+        if (s->replaces) {
+            s->to_replace = bdrv_find_node(s->replaces);
+            if (!s->to_replace) {
+                error_setg(errp, "Node name '%s' not found", s->replaces);
+                return;
+            }
+
+            /* TODO Translate this into child freeze system. */
+            error_setg(&s->replace_blocker,
+                       "block device is in use by block-job-complete");
+            bdrv_op_block_all(s->to_replace, s->replace_blocker);
+            bdrv_ref(s->to_replace);
         }
 
-        /* TODO Translate this into child freeze system. */
-        error_setg(&s->replace_blocker,
-                   "block device is in use by block-job-complete");
-        bdrv_op_block_all(s->to_replace, s->replace_blocker);
-        bdrv_ref(s->to_replace);
+        s->should_complete = true;
     }
 
-    s->should_complete = true;
-
     /* If the job is paused, it will be re-entered when it is resumed */
     WITH_JOB_LOCK_GUARD() {
         if (!job->paused) {
-- 
2.47.3

[PATCH 2/3] iotests/041: add test for mirror with throttled NBD export as target

[Fiona Ebner]

Mostly in preparation for commit "iotests/041: add test for duplicate
job-complete with throttled target".

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 tests/qemu-iotests/041     | 112 ++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/041.out |   4 +-
 2 files changed, 113 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index 8452845f44..a7e1980f13 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -24,7 +24,7 @@ import os
 import re
 import json
 import iotests
-from iotests import qemu_img, qemu_img_map, qemu_io
+from iotests import qemu_img, qemu_img_map, qemu_io, QemuStorageDaemon
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 target_backing_img = os.path.join(iotests.test_dir, 'target-backing.img')
@@ -1373,6 +1373,116 @@ class TestFilters(iotests.QMPTestCase):
 
         self.complete_and_wait('mirror')
 
+# Tests for mirror where the target is a throttled NBD export.
+class TestThrottledNBDTarget(iotests.QMPTestCase):
+    image_len = 3 * 1024 * 1024
+    bps_total = 1 * 1024 * 1024
+
+    def setUp(self):
+        iotests.create_image(backing_img, self.image_len)
+        iotests.create_image(target_backing_img, self.image_len)
+        qemu_img('create', '-f', iotests.imgfmt,
+                 '-o', f'backing_file={backing_img}', '-F', 'raw', test_img)
+        qemu_img('create', '-f', iotests.imgfmt,
+                 '-o', f'backing_file={target_backing_img}', '-F', 'raw',
+                 target_img)
+        qemu_io('-c', 'write -P 23 0 3M', test_img)
+
+        self.qsd = QemuStorageDaemon('--nbd-server',
+                                     f'addr.type=unix,addr.path={nbd_sock_path}',
+                                     qmp=True)
+
+        self.qsd.cmd('object-add', {
+            'qom-type': 'throttle-group',
+            'id': 'thrgr-target',
+            'limits': {
+                'bps-total': self.bps_total,
+            }
+        })
+
+        self.qsd.cmd('blockdev-add', {
+            'node-name': 'target',
+            'driver': 'throttle',
+            'throttle-group': 'thrgr-target',
+            'file': {
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': target_img
+                }
+            }
+        })
+
+        self.qsd.cmd('block-export-add', {
+            'id': 'exp0',
+            'type': 'nbd',
+            'node-name': 'target',
+            'writable': True
+        })
+
+        self.vm = iotests.VM().add_device('virtio-scsi,id=vio-scsi')
+        self.vm.launch()
+
+        self.vm.cmd('blockdev-add',{
+            'node-name': 'source',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': test_img
+            },
+            'backing': {
+                'node-name': 'backing',
+                'driver': 'raw',
+                'file': {
+                    'driver': 'file',
+                    'filename': backing_img
+                }
+            }
+        })
+
+        self.vm.cmd('device_add',
+                    driver='scsi-hd',
+                    id='virtio',
+                    bus='vio-scsi.0',
+                    drive='source')
+
+        self.vm.cmd('blockdev-add', {
+            'node-name': 'target',
+            'driver': 'nbd',
+            'export': 'target',
+            'server': {
+                'type': 'unix',
+                'path': nbd_sock_path
+            }
+        })
+
+    def tearDown(self):
+        os.remove(test_img)
+        os.remove(target_img)
+        os.remove(backing_img)
+        os.remove(target_backing_img)
+
+    def test_throttled(self):
+        self.vm.cmd('blockdev-mirror',
+                    job_id='mirror',
+                    device='source',
+                    target='target',
+                    sync='full')
+
+        time.sleep(1)
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/ready', False)
+        self.vm.qtest(f'clock_step {4 * 1000 * 1000 * 1000}')
+        time.sleep(1) # in real time, the rest should now comlpete fast
+        result = self.vm.qmp('query-block-jobs')
+        self.assert_qmp(result, 'return[0]/ready', True)
+
+        self.complete_and_wait('mirror')
+        self.vm.shutdown()
+        self.qsd.stop()
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 46651953e8..5273ce86c3 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-...........................................................................................................
+............................................................................................................
 ----------------------------------------------------------------------
-Ran 107 tests
+Ran 108 tests
 
 OK
-- 
2.47.3

[PATCH 3/3] iotests/041: add test for duplicate job-complete with throttled target

[Fiona Ebner]

This would fail without commit "block/mirror: fix assertion failure
upon duplicate complete for job using 'replaces'".

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 tests/qemu-iotests/041     | 41 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/041.out |  4 ++--
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index a7e1980f13..a09fdd4ad6 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -1456,6 +1456,8 @@ class TestThrottledNBDTarget(iotests.QMPTestCase):
             }
         })
 
+        self.drive_qdev = '/machine/peripheral/virtio'
+
     def tearDown(self):
         os.remove(test_img)
         os.remove(target_img)
@@ -1483,6 +1485,45 @@ class TestThrottledNBDTarget(iotests.QMPTestCase):
         self.assertTrue(iotests.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
+    def test_duplicate_complete_with_replaces(self):
+        self.qsd.cmd('qom-set', {
+            'path': 'thrgr-target',
+            'property': 'limits',
+            'value' : {},
+        })
+
+        self.vm.cmd('blockdev-mirror',
+                    job_id='mirror',
+                    device='source',
+                    target='target',
+                    replaces='source',
+                    sync='full')
+        self.wait_ready(drive='mirror')
+
+        self.qsd.cmd('qom-set', {
+            'path': 'thrgr-target',
+            'property': 'limits',
+            'value' : {
+                'bps-total': self.bps_total,
+            }
+        })
+
+        self.vm.hmp_qemu_io(self.drive_qdev, 'aio_write -P 7 0 1M', qdev=True)
+        self.vm.hmp_qemu_io(self.drive_qdev, 'aio_write -P 7 1M 1M', qdev=True)
+        self.vm.hmp_qemu_io(self.drive_qdev, 'aio_write -P 7 2M 1M', qdev=True)
+
+        self.vm.cmd('job-complete', id='mirror')
+        # The fact that the target is externally throttled ensures that the job
+        # won't be finished yet when the second command is issued.
+        self.vm.cmd('job-complete', id='mirror')
+
+        self.vm.qtest(f'clock_step {4 * 1000 * 1000 * 1000}')
+        self.wait_until_completed('mirror')
+
+        self.vm.shutdown()
+        self.qsd.stop()
+        self.assertTrue(iotests.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
 
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index 5273ce86c3..96a0752f44 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-............................................................................................................
+.............................................................................................................
 ----------------------------------------------------------------------
-Ran 108 tests
+Ran 109 tests
 
 OK
-- 
2.47.3

Re: [PATCH 1/3] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'

[Hanna Czenczek]

On 11.03.26 15:54, Fiona Ebner wrote:
> If s->replace_blocker was already set by an earlier invocation of
> mirror_complete(), then there will be an assertion failure when
> error_setg() is called for it a second time. The bdrv_op_block_all()
> and bdrv_ref() operations should only be done a single time too.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   block/mirror.c | 28 +++++++++++++++-------------
>   1 file changed, 15 insertions(+), 13 deletions(-)

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Re: [PATCH 2/3] iotests/041: add test for mirror with throttled NBD export as target

[Hanna Czenczek]

On 11.03.26 15:54, Fiona Ebner wrote:
> Mostly in preparation for commit "iotests/041: add test for duplicate
> job-complete with throttled target".
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   tests/qemu-iotests/041     | 112 ++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/041.out |   4 +-
>   2 files changed, 113 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
> index 8452845f44..a7e1980f13 100755
> --- a/tests/qemu-iotests/041
> +++ b/tests/qemu-iotests/041

[...]


> +    def tearDown(self):
> +        os.remove(test_img)
> +        os.remove(target_img)
> +        os.remove(backing_img)
> +        os.remove(target_backing_img)

It would be good if tearDown() could still contain the VM/QSD shutdown/stop.

> +
> +    def test_throttled(self):
> +        self.vm.cmd('blockdev-mirror',
> +                    job_id='mirror',
> +                    device='source',
> +                    target='target',
> +                    sync='full')
> +
> +        time.sleep(1)
> +        result = self.vm.qmp('query-block-jobs')
> +        self.assert_qmp(result, 'return[0]/ready', False)
> +        self.vm.qtest(f'clock_step {4 * 1000 * 1000 * 1000}')
> +        time.sleep(1) # in real time, the rest should now comlpete fast

I think the clock_step belongs on the export side, the QSD.  On the 
other hand, QSD has no way to enable qtest, I believe, so it won’t work 
well as an export here.  I think it works accidentally because throttle 
allows a burst, and can allow to write 3 MB in 2 seconds.

This whole test is also a bit fiddly. It needs to write 3 MB in one 
second, I believe, which seems prone to being flakey.  I’d be happy with 
a null export on the target side, if that still did the job (pun intended).

(Also *complete)

Hanna

> +        result = self.vm.qmp('query-block-jobs')
> +        self.assert_qmp(result, 'return[0]/ready', True)
> +
> +        self.complete_and_wait('mirror')
> +        self.vm.shutdown()
> +        self.qsd.stop()
> +        self.assertTrue(iotests.compare_images(test_img, target_img),
> +                        'target image does not match source after mirroring')
> +
>   
>   if __name__ == '__main__':
>       iotests.main(supported_fmts=['qcow2', 'qed'],
> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
> index 46651953e8..5273ce86c3 100644
> --- a/tests/qemu-iotests/041.out
> +++ b/tests/qemu-iotests/041.out
> @@ -1,5 +1,5 @@
> -...........................................................................................................
> +............................................................................................................
>   ----------------------------------------------------------------------
> -Ran 107 tests
> +Ran 108 tests
>   
>   OK

Re: [PATCH 3/3] iotests/041: add test for duplicate job-complete with throttled target

[Hanna Czenczek]

On 11.03.26 15:54, Fiona Ebner wrote:
> This would fail without commit "block/mirror: fix assertion failure
> upon duplicate complete for job using 'replaces'".
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>   tests/qemu-iotests/041     | 41 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/041.out |  4 ++--
>   2 files changed, 43 insertions(+), 2 deletions(-)

Looks good in principle, but there is the same problem with QSD not 
speaking qtest here.

Hanna

Re: [PATCH 0/3] block/mirror: fix assertion failure upon duplicate complete for job using 'replaces'

[Hanna Czenczek]

On 11.03.26 15:54, Fiona Ebner wrote:
> If s->replace_blocker was already set by an earlier invocation of
> mirror_complete(), then there will be an assertion failure when
> error_setg() is called for it a second time. The bdrv_ref() operation
> should only be done a single time too.
>
> I thought about using an early return with an error, but that might
> break existing users that (accidentally) issue duplicate
> 'job-complete' without using 'replaces'.
>
> A paused job does not accept the 'complete' verb, so using an
> externally throttled node was the best I could come up with for the
> test.
>
> Fiona Ebner (3):
>    block/mirror: fix assertion failure upon duplicate complete for job
>      using 'replaces'

Thanks, I’ve applied this patch 1 to my block branch (for rc0):

https://gitlab.com/hreitz/qemu/-/commits/block

Better to have the code in earlier, and add the test later.

Hanna

Re: [PATCH 2/3] iotests/041: add test for mirror with throttled NBD export as target

[Fiona Ebner]

Am 16.03.26 um 6:27 PM schrieb Hanna Czenczek:
> On 11.03.26 15:54, Fiona Ebner wrote:
>> Mostly in preparation for commit "iotests/041: add test for duplicate
>> job-complete with throttled target".
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>   tests/qemu-iotests/041     | 112 ++++++++++++++++++++++++++++++++++++-
>>   tests/qemu-iotests/041.out |   4 +-
>>   2 files changed, 113 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
>> index 8452845f44..a7e1980f13 100755
>> --- a/tests/qemu-iotests/041
>> +++ b/tests/qemu-iotests/041
> 
> [...]
> 
> 
>> +    def tearDown(self):
>> +        os.remove(test_img)
>> +        os.remove(target_img)
>> +        os.remove(backing_img)
>> +        os.remove(target_backing_img)
> 
> It would be good if tearDown() could still contain the VM/QSD shutdown/
> stop.

Will do!

>> +
>> +    def test_throttled(self):
>> +        self.vm.cmd('blockdev-mirror',
>> +                    job_id='mirror',
>> +                    device='source',
>> +                    target='target',
>> +                    sync='full')
>> +
>> +        time.sleep(1)
>> +        result = self.vm.qmp('query-block-jobs')
>> +        self.assert_qmp(result, 'return[0]/ready', False)
>> +        self.vm.qtest(f'clock_step {4 * 1000 * 1000 * 1000}')
>> +        time.sleep(1) # in real time, the rest should now comlpete fast
> 
> I think the clock_step belongs on the export side, the QSD.  On the
> other hand, QSD has no way to enable qtest, I believe, so it won’t work
> well as an export here.  I think it works accidentally because throttle
> allows a burst, and can allow to write 3 MB in 2 seconds.

Ah, good catch! I originally did everything with just the VM, but
draining during mirror completion disables the throttling, so the issue
with the duplicate complete wouldn't trigger for the test in patch 3/3.
That's why an external target is needed.

> This whole test is also a bit fiddly. It needs to write 3 MB in one
> second, I believe, which seems prone to being flakey.  I’d be happy with
> a null export on the target side, if that still did the job (pun intended).

The one from patch 3/3 relies on the same kind of behavior. I guess
using a second VM instance rather than QSD for the export could work?
And adapting the test to make throttling actually have an effect.

Best Regards,
Fiona

> (Also *complete)
> 
> Hanna
> 
>> +        result = self.vm.qmp('query-block-jobs')
>> +        self.assert_qmp(result, 'return[0]/ready', True)
>> +
>> +        self.complete_and_wait('mirror')
>> +        self.vm.shutdown()
>> +        self.qsd.stop()
>> +        self.assertTrue(iotests.compare_images(test_img, target_img),
>> +                        'target image does not match source after
>> mirroring')
>> +
>>     if __name__ == '__main__':
>>       iotests.main(supported_fmts=['qcow2', 'qed'],
>> diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
>> index 46651953e8..5273ce86c3 100644
>> --- a/tests/qemu-iotests/041.out
>> +++ b/tests/qemu-iotests/041.out
>> @@ -1,5 +1,5 @@
>> -...........................................................................................................
>> +............................................................................................................
>>   ----------------------------------------------------------------------
>> -Ran 107 tests
>> +Ran 108 tests
>>     OK
> 
> 

Re: [PATCH 2/3] iotests/041: add test for mirror with throttled NBD export as target

[Hanna Czenczek]

On 18.03.26 15:51, Fiona Ebner wrote:
> Am 16.03.26 um 6:27 PM schrieb Hanna Czenczek:
>> On 11.03.26 15:54, Fiona Ebner wrote:
>>> Mostly in preparation for commit "iotests/041: add test for duplicate
>>> job-complete with throttled target".
>>>
>>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>>> ---
>>>    tests/qemu-iotests/041     | 112 ++++++++++++++++++++++++++++++++++++-
>>>    tests/qemu-iotests/041.out |   4 +-
>>>    2 files changed, 113 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
>>> index 8452845f44..a7e1980f13 100755
>>> --- a/tests/qemu-iotests/041
>>> +++ b/tests/qemu-iotests/041
>> [...]
>>
>>
>>> +    def tearDown(self):
>>> +        os.remove(test_img)
>>> +        os.remove(target_img)
>>> +        os.remove(backing_img)
>>> +        os.remove(target_backing_img)
>> It would be good if tearDown() could still contain the VM/QSD shutdown/
>> stop.
> Will do!
>
>>> +
>>> +    def test_throttled(self):
>>> +        self.vm.cmd('blockdev-mirror',
>>> +                    job_id='mirror',
>>> +                    device='source',
>>> +                    target='target',
>>> +                    sync='full')
>>> +
>>> +        time.sleep(1)
>>> +        result = self.vm.qmp('query-block-jobs')
>>> +        self.assert_qmp(result, 'return[0]/ready', False)
>>> +        self.vm.qtest(f'clock_step {4 * 1000 * 1000 * 1000}')
>>> +        time.sleep(1) # in real time, the rest should now comlpete fast
>> I think the clock_step belongs on the export side, the QSD.  On the
>> other hand, QSD has no way to enable qtest, I believe, so it won’t work
>> well as an export here.  I think it works accidentally because throttle
>> allows a burst, and can allow to write 3 MB in 2 seconds.
> Ah, good catch! I originally did everything with just the VM, but
> draining during mirror completion disables the throttling, so the issue
> with the duplicate complete wouldn't trigger for the test in patch 3/3.
> That's why an external target is needed.
>
>> This whole test is also a bit fiddly. It needs to write 3 MB in one
>> second, I believe, which seems prone to being flakey.  I’d be happy with
>> a null export on the target side, if that still did the job (pun intended).
> The one from patch 3/3 relies on the same kind of behavior. I guess
> using a second VM instance rather than QSD for the export could work?
> And adapting the test to make throttling actually have an effect.

Yes, that’s what I had in mind.

Hanna