[Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting

[Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

Quitting qemu should lead to qemu exiting pretty much immediately.  That
means if you have a block job running on a throttled block node, the
node should ignore its throttling and the job should be cancelled
immediately.

Unfortunately, that is not what happens.  Currently, the node will be
drained (with a bdrv_drain_all()), and then again unquiesced (because
bdrv_drain_all() ends).  Then, the block job is cancelled; but at this
point, the node is no longer drained, so it will block, as it befits a
throttling node.

To fix this issue, we have to keep all nodes drained while we cancel all
block jobs when quitting qemu.  This will make the throttle node ignore
its throttling and thus let the block job cancel immediately.


Max Reitz (2):
  vl: Drain before (block) job cancel when quitting
  iotests: Test quitting with job on throttled node

 vl.c                       | 11 ++++++++
 tests/qemu-iotests/218     | 55 ++++++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/218.out |  4 +++
 3 files changed, 68 insertions(+), 2 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

If the main loop cancels all block jobs while the block layer is not
drained, this cancelling may not happen instantaneously.  We can start a
drained section before vm_shutdown(), which entails another
bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
basically.

We do not have to end the drained section, because we actually do not
want any requests to happen from this point on.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
I don't know whether it actually makes sense to never end this drained
section.  It makes sense to me.  Please correct me if I'm wrong.
---
 vl.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/vl.c b/vl.c
index cd1fbc4cdc..3f8b3f74f5 100644
--- a/vl.c
+++ b/vl.c
@@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
      */
     migration_shutdown();
 
+    /*
+     * We must cancel all block jobs while the block layer is drained,
+     * or cancelling will be affected by throttling and thus may block
+     * for an extended period of time.
+     * vm_shutdown() will bdrv_drain_all(), so we may as well include
+     * it in the drained section.
+     * We do not need to end this section, because we do not want any
+     * requests happening from here on anyway.
+     */
+    bdrv_drain_all_begin();
+
     /* No more vcpu or device emulation activity beyond this point */
     vm_shutdown();
 
-- 
2.21.0

Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

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

[re-adding the original CCs, why not]

On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 17:21, Max Reitz wrote:
>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.06.2019 1:08, Max Reitz wrote:
>>>> If the main loop cancels all block jobs while the block layer is not
>>>> drained, this cancelling may not happen instantaneously.  We can start a
>>>> drained section before vm_shutdown(), which entails another
>>>> bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
>>>> basically.
>>>>
>>>> We do not have to end the drained section, because we actually do not
>>>> want any requests to happen from this point on.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>> I don't know whether it actually makes sense to never end this drained
>>>> section.  It makes sense to me.  Please correct me if I'm wrong.
>>>> ---
>>>>    vl.c | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/vl.c b/vl.c
>>>> index cd1fbc4cdc..3f8b3f74f5 100644
>>>> --- a/vl.c
>>>> +++ b/vl.c
>>>> @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
>>>>         */
>>>>        migration_shutdown();
>>>>    
>>>> +    /*
>>>> +     * We must cancel all block jobs while the block layer is drained,
>>>> +     * or cancelling will be affected by throttling and thus may block
>>>> +     * for an extended period of time.
>>>> +     * vm_shutdown() will bdrv_drain_all(), so we may as well include
>>>> +     * it in the drained section.
>>>> +     * We do not need to end this section, because we do not want any
>>>> +     * requests happening from here on anyway.
>>>> +     */
>>>> +    bdrv_drain_all_begin();
>>>> +
>>>>        /* No more vcpu or device emulation activity beyond this point */
>>>>        vm_shutdown();
>>>>    
>>>>
>>>
>>> So, actually, the problem is that we may wait for job requests twice:
>>> on drain and then on cancel.
>>
>> We don’t wait on drain.  When the throttle node is drained, it will
>> ignore throttling (as noted in the cover letter).
>>
>> We do wait when cancelling a job while the throttle node isn’t drained,
>> though.  That’s the problem.
> 
> Ah, understand now.
> 
> Is it safe to drain_begin before stopping cpus? We may finish up then with some queued
> somewhere IO requests..

Hm...  Aren’t guest devices prohibited from issuing requests to the
block layer while their respective block device is drained?

Otherwise, I suppose I’ll have to move the bdrv_drain_all_begin() below
the vm_shutdown().  That wouldn’t be too big of a problem.

Max


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

Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Vladimir Sementsov-Ogievskiy]

13.06.2019 19:03, Max Reitz wrote:
> [re-adding the original CCs, why not]
> 
> On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 17:21, Max Reitz wrote:
>>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
>>>> 13.06.2019 1:08, Max Reitz wrote:
>>>>> If the main loop cancels all block jobs while the block layer is not
>>>>> drained, this cancelling may not happen instantaneously.  We can start a
>>>>> drained section before vm_shutdown(), which entails another
>>>>> bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
>>>>> basically.
>>>>>
>>>>> We do not have to end the drained section, because we actually do not
>>>>> want any requests to happen from this point on.
>>>>>
>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>> ---
>>>>> I don't know whether it actually makes sense to never end this drained
>>>>> section.  It makes sense to me.  Please correct me if I'm wrong.
>>>>> ---
>>>>>     vl.c | 11 +++++++++++
>>>>>     1 file changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/vl.c b/vl.c
>>>>> index cd1fbc4cdc..3f8b3f74f5 100644
>>>>> --- a/vl.c
>>>>> +++ b/vl.c
>>>>> @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
>>>>>          */
>>>>>         migration_shutdown();
>>>>>     
>>>>> +    /*
>>>>> +     * We must cancel all block jobs while the block layer is drained,
>>>>> +     * or cancelling will be affected by throttling and thus may block
>>>>> +     * for an extended period of time.
>>>>> +     * vm_shutdown() will bdrv_drain_all(), so we may as well include
>>>>> +     * it in the drained section.
>>>>> +     * We do not need to end this section, because we do not want any
>>>>> +     * requests happening from here on anyway.
>>>>> +     */
>>>>> +    bdrv_drain_all_begin();
>>>>> +
>>>>>         /* No more vcpu or device emulation activity beyond this point */
>>>>>         vm_shutdown();
>>>>>     
>>>>>
>>>>
>>>> So, actually, the problem is that we may wait for job requests twice:
>>>> on drain and then on cancel.
>>>
>>> We don’t wait on drain.  When the throttle node is drained, it will
>>> ignore throttling (as noted in the cover letter).
>>>
>>> We do wait when cancelling a job while the throttle node isn’t drained,
>>> though.  That’s the problem.
>>
>> Ah, understand now.
>>
>> Is it safe to drain_begin before stopping cpus? We may finish up then with some queued
>> somewhere IO requests..
> 
> Hm...  Aren’t guest devices prohibited from issuing requests to the
> block layer while their respective block device is drained?

It's at least a buggy place, I remember Denis Plotnikov sent patch to fix it and had a huge
discussion with Kevin.
And here it is:
https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00732.html

> 
> Otherwise, I suppose I’ll have to move the bdrv_drain_all_begin() below
> the vm_shutdown().  That wouldn’t be too big of a problem.
> 
> Max
> 


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

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

On 14.06.19 11:22, Vladimir Sementsov-Ogievskiy wrote:
> 13.06.2019 19:03, Max Reitz wrote:
>> [re-adding the original CCs, why not]
>>
>> On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
>>> 13.06.2019 17:21, Max Reitz wrote:
>>>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
>>>>> 13.06.2019 1:08, Max Reitz wrote:
>>>>>> If the main loop cancels all block jobs while the block layer is not
>>>>>> drained, this cancelling may not happen instantaneously.  We can start a
>>>>>> drained section before vm_shutdown(), which entails another
>>>>>> bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
>>>>>> basically.
>>>>>>
>>>>>> We do not have to end the drained section, because we actually do not
>>>>>> want any requests to happen from this point on.
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>> ---
>>>>>> I don't know whether it actually makes sense to never end this drained
>>>>>> section.  It makes sense to me.  Please correct me if I'm wrong.
>>>>>> ---
>>>>>>     vl.c | 11 +++++++++++
>>>>>>     1 file changed, 11 insertions(+)
>>>>>>
>>>>>> diff --git a/vl.c b/vl.c
>>>>>> index cd1fbc4cdc..3f8b3f74f5 100644
>>>>>> --- a/vl.c
>>>>>> +++ b/vl.c
>>>>>> @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
>>>>>>          */
>>>>>>         migration_shutdown();
>>>>>>     
>>>>>> +    /*
>>>>>> +     * We must cancel all block jobs while the block layer is drained,
>>>>>> +     * or cancelling will be affected by throttling and thus may block
>>>>>> +     * for an extended period of time.
>>>>>> +     * vm_shutdown() will bdrv_drain_all(), so we may as well include
>>>>>> +     * it in the drained section.
>>>>>> +     * We do not need to end this section, because we do not want any
>>>>>> +     * requests happening from here on anyway.
>>>>>> +     */
>>>>>> +    bdrv_drain_all_begin();
>>>>>> +
>>>>>>         /* No more vcpu or device emulation activity beyond this point */
>>>>>>         vm_shutdown();
>>>>>>     
>>>>>>
>>>>>
>>>>> So, actually, the problem is that we may wait for job requests twice:
>>>>> on drain and then on cancel.
>>>>
>>>> We don’t wait on drain.  When the throttle node is drained, it will
>>>> ignore throttling (as noted in the cover letter).
>>>>
>>>> We do wait when cancelling a job while the throttle node isn’t drained,
>>>> though.  That’s the problem.
>>>
>>> Ah, understand now.
>>>
>>> Is it safe to drain_begin before stopping cpus? We may finish up then with some queued
>>> somewhere IO requests..
>>
>> Hm...  Aren’t guest devices prohibited from issuing requests to the
>> block layer while their respective block device is drained?
> 
> It's at least a buggy place, I remember Denis Plotnikov sent patch to fix it and had a huge
> discussion with Kevin.
> And here it is:
> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00732.html

Ah, I even have that in my inbox...  The latest reply I see came in April:

https://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00243.html

Where Kevin asked for an RFC patch in the current state.

I’m not sure whether I should work around a potential bug here, if we
can agree that it is a bug, and if it isn’t clear whether this place
would actually be affected.

Max


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

Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting

[Philippe Mathieu-Daudé]

On 6/19/19 9:40 PM, Max Reitz wrote:
> On 14.06.19 11:22, Vladimir Sementsov-Ogievskiy wrote:
>> 13.06.2019 19:03, Max Reitz wrote:
>>> [re-adding the original CCs, why not]
>>>
>>> On 13.06.19 16:30, Vladimir Sementsov-Ogievskiy wrote:
>>>> 13.06.2019 17:21, Max Reitz wrote:
>>>>> On 13.06.19 16:19, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 13.06.2019 1:08, Max Reitz wrote:
>>>>>>> If the main loop cancels all block jobs while the block layer is not
>>>>>>> drained, this cancelling may not happen instantaneously.  We can start a
>>>>>>> drained section before vm_shutdown(), which entails another
>>>>>>> bdrv_drain_all(); this nested bdrv_drain_all() will thus be a no-op,
>>>>>>> basically.
>>>>>>>
>>>>>>> We do not have to end the drained section, because we actually do not
>>>>>>> want any requests to happen from this point on.

Your cover description was more helpful to understand the big picture
than this commit description.

>>>>>>>
>>>>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>>>>> ---
>>>>>>> I don't know whether it actually makes sense to never end this drained
>>>>>>> section.  It makes sense to me.  Please correct me if I'm wrong.
>>>>>>> ---
>>>>>>>     vl.c | 11 +++++++++++
>>>>>>>     1 file changed, 11 insertions(+)
>>>>>>>
>>>>>>> diff --git a/vl.c b/vl.c
>>>>>>> index cd1fbc4cdc..3f8b3f74f5 100644
>>>>>>> --- a/vl.c
>>>>>>> +++ b/vl.c
>>>>>>> @@ -4538,6 +4538,17 @@ int main(int argc, char **argv, char **envp)
>>>>>>>          */
>>>>>>>         migration_shutdown();
>>>>>>>     
>>>>>>> +    /*
>>>>>>> +     * We must cancel all block jobs while the block layer is drained,
>>>>>>> +     * or cancelling will be affected by throttling and thus may block
>>>>>>> +     * for an extended period of time.
>>>>>>> +     * vm_shutdown() will bdrv_drain_all(), so we may as well include
>>>>>>> +     * it in the drained section.
>>>>>>> +     * We do not need to end this section, because we do not want any
>>>>>>> +     * requests happening from here on anyway.
>>>>>>> +     */
>>>>>>> +    bdrv_drain_all_begin();

This change looks sane, but I'm not expert with the 'block layer'.

>>>>>>> +
>>>>>>>         /* No more vcpu or device emulation activity beyond this point */
>>>>>>>         vm_shutdown();
>>>>>>>     
>>>>>>>
>>>>>>
>>>>>> So, actually, the problem is that we may wait for job requests twice:
>>>>>> on drain and then on cancel.
>>>>>
>>>>> We don’t wait on drain.  When the throttle node is drained, it will
>>>>> ignore throttling (as noted in the cover letter).
>>>>>
>>>>> We do wait when cancelling a job while the throttle node isn’t drained,
>>>>> though.  That’s the problem.
>>>>
>>>> Ah, understand now.
>>>>
>>>> Is it safe to drain_begin before stopping cpus? We may finish up then with some queued
>>>> somewhere IO requests..
>>>
>>> Hm...  Aren’t guest devices prohibited from issuing requests to the
>>> block layer while their respective block device is drained?
>>
>> It's at least a buggy place, I remember Denis Plotnikov sent patch to fix it and had a huge
>> discussion with Kevin.
>> And here it is:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg00732.html
> 
> Ah, I even have that in my inbox...  The latest reply I see came in April:
> 
> https://lists.nongnu.org/archive/html/qemu-block/2019-04/msg00243.html
> 
> Where Kevin asked for an RFC patch in the current state.
> 
> I’m not sure whether I should work around a potential bug here, if we
> can agree that it is a bug, and if it isn’t clear whether this place
> would actually be affected.

Hmmm so what is the outcome of this thread?

Apparently this is not a bug for Max, but a potential one for Vladimir?

Vladimir are you NACKing this patch?

Thanks,

Phil.

> 
> Max
> 

[Qemu-devel] [PATCH 2/2] iotests: Test quitting with job on throttled node

[Max Reitz]

When qemu quits, all throttling should be ignored.  That means, if there
is a mirror job running from a throttled node, it should be cancelled
immediately and qemu close without blocking.

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

diff --git a/tests/qemu-iotests/218 b/tests/qemu-iotests/218
index 92c331b6fb..2554d84581 100755
--- a/tests/qemu-iotests/218
+++ b/tests/qemu-iotests/218
@@ -27,9 +27,9 @@
 # Creator/Owner: Max Reitz <mreitz@redhat.com>
 
 import iotests
-from iotests import log
+from iotests import log, qemu_img, qemu_io_silent
 
-iotests.verify_platform(['linux'])
+iotests.verify_image_format(supported_fmts=['qcow2', 'raw'])
 
 
 # Launches the VM, adds two null-co nodes (source and target), and
@@ -136,3 +136,54 @@ with iotests.VM() as vm:
 
     log(vm.event_wait('BLOCK_JOB_CANCELLED'),
         filters=[iotests.filter_qmp_event])
+
+log('')
+log('=== Cancel mirror job from throttled node by quitting ===')
+log('')
+
+with iotests.VM() as vm, \
+     iotests.FilePath('src.img') as src_img_path:
+
+    assert qemu_img('create', '-f', iotests.imgfmt, src_img_path, '64M') == 0
+    assert qemu_io_silent('-f', iotests.imgfmt, src_img_path,
+                          '-c', 'write -P 42 0M 64M') == 0
+
+    vm.launch()
+
+    ret = vm.qmp('object-add', qom_type='throttle-group', id='tg',
+                 props={'x-bps-read': 4096})
+    assert ret['return'] == {}
+
+    ret = vm.qmp('blockdev-add',
+                 node_name='source',
+                 driver=iotests.imgfmt,
+                 file={
+                     'driver': 'file',
+                     'filename': src_img_path
+                 })
+    assert ret['return'] == {}
+
+    ret = vm.qmp('blockdev-add',
+                 node_name='throttled-source',
+                 driver='throttle',
+                 throttle_group='tg',
+                 file='source')
+    assert ret['return'] == {}
+
+    ret = vm.qmp('blockdev-add',
+                 node_name='target',
+                 driver='null-co',
+                 size=(64 * 1048576))
+    assert ret['return'] == {}
+
+    ret = vm.qmp('blockdev-mirror',
+                 job_id='mirror',
+                 device='throttled-source',
+                 target='target',
+                 sync='full')
+    assert ret['return'] == {}
+
+    log(vm.qmp('quit'))
+
+    with iotests.Timeout(5, 'Timeout waiting for VM to quit'):
+        vm.shutdown(has_quit=True)
diff --git a/tests/qemu-iotests/218.out b/tests/qemu-iotests/218.out
index 825a657081..5a86a97550 100644
--- a/tests/qemu-iotests/218.out
+++ b/tests/qemu-iotests/218.out
@@ -28,3 +28,7 @@ Cancelling job
 Cancelling job
 {"return": {}}
 {"data": {"device": "mirror", "len": 1048576, "offset": 1048576, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_CANCELLED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}
+
+=== Cancel mirror job from throttled node by quitting ===
+
+{"return": {}}
-- 
2.21.0

Re: [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting

[Max Reitz]

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

On 13.06.19 00:08, Max Reitz wrote:
> Quitting qemu should lead to qemu exiting pretty much immediately.  That
> means if you have a block job running on a throttled block node, the
> node should ignore its throttling and the job should be cancelled
> immediately.
> 
> Unfortunately, that is not what happens.  Currently, the node will be
> drained (with a bdrv_drain_all()), and then again unquiesced (because
> bdrv_drain_all() ends).  Then, the block job is cancelled; but at this
> point, the node is no longer drained, so it will block, as it befits a
> throttling node.
> 
> To fix this issue, we have to keep all nodes drained while we cancel all
> block jobs when quitting qemu.  This will make the throttle node ignore
> its throttling and thus let the block job cancel immediately.

I forgot to mention: This series depends on “block: Keep track of parent
quiescing”, specifically patch 3 (“iotests: Add @has_quit to
vm.shutdown()”).

Based-on: <20190605161118.14544-1-mreitz@redhat.com>

Max

> Max Reitz (2):
>   vl: Drain before (block) job cancel when quitting
>   iotests: Test quitting with job on throttled node
> 
>  vl.c                       | 11 ++++++++
>  tests/qemu-iotests/218     | 55 ++++++++++++++++++++++++++++++++++++--
>  tests/qemu-iotests/218.out |  4 +++
>  3 files changed, 68 insertions(+), 2 deletions(-)
> 



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

Re: [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting

[Kevin Wolf]

Am 13.06.2019 um 00:08 hat Max Reitz geschrieben:
> Quitting qemu should lead to qemu exiting pretty much immediately.  That
> means if you have a block job running on a throttled block node, the
> node should ignore its throttling and the job should be cancelled
> immediately.
> 
> Unfortunately, that is not what happens.  Currently, the node will be
> drained (with a bdrv_drain_all()), and then again unquiesced (because
> bdrv_drain_all() ends).  Then, the block job is cancelled; but at this
> point, the node is no longer drained, so it will block, as it befits a
> throttling node.
> 
> To fix this issue, we have to keep all nodes drained while we cancel all
> block jobs when quitting qemu.  This will make the throttle node ignore
> its throttling and thus let the block job cancel immediately.

Thanks, applied to the block branch.

Kevin