qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting
@ 2019-06-12 22:08 Max Reitz
  2019-06-12 22:08 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Max Reitz @ 2019-06-12 22:08 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, 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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting
  2019-06-12 22:08 [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
@ 2019-06-12 22:08 ` Max Reitz
       [not found]   ` <b110b753-8546-0d34-f6ef-06c5726766ce@virtuozzo.com>
  2019-06-12 22:08 ` [Qemu-devel] [PATCH 2/2] iotests: Test quitting with job on throttled node Max Reitz
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-06-12 22:08 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, 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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [Qemu-devel] [PATCH 2/2] iotests: Test quitting with job on throttled node
  2019-06-12 22:08 [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
  2019-06-12 22:08 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
@ 2019-06-12 22:08 ` Max Reitz
  2019-06-12 22:31 ` [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
  2019-07-19 13:26 ` Kevin Wolf
  3 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-06-12 22:08 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, 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



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting
  2019-06-12 22:08 [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
  2019-06-12 22:08 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
  2019-06-12 22:08 ` [Qemu-devel] [PATCH 2/2] iotests: Test quitting with job on throttled node Max Reitz
@ 2019-06-12 22:31 ` Max Reitz
  2019-07-19 13:26 ` Kevin Wolf
  3 siblings, 0 replies; 9+ messages in thread
From: Max Reitz @ 2019-06-12 22:31 UTC (permalink / raw)
  To: qemu-block; +Cc: Kevin Wolf, Paolo Bonzini, qemu-devel


[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting
       [not found]       ` <57ae7f82-ae02-a382-74f6-cb96672b2058@virtuozzo.com>
@ 2019-06-13 16:03         ` Max Reitz
  2019-06-14  9:22           ` Vladimir Sementsov-Ogievskiy
  0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-06-13 16:03 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: Kevin Wolf, Paolo Bonzini, qemu-devel, Qemu-block


[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting
  2019-06-13 16:03         ` Max Reitz
@ 2019-06-14  9:22           ` Vladimir Sementsov-Ogievskiy
  2019-06-19 19:40             ` Max Reitz
  0 siblings, 1 reply; 9+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-06-14  9:22 UTC (permalink / raw)
  To: Max Reitz
  Cc: Kevin Wolf, Paolo Bonzini, Denis Plotnikov, qemu-devel, Qemu-block

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting
  2019-06-14  9:22           ` Vladimir Sementsov-Ogievskiy
@ 2019-06-19 19:40             ` Max Reitz
  2019-07-16  7:00               ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 9+ messages in thread
From: Max Reitz @ 2019-06-19 19:40 UTC (permalink / raw)
  To: Vladimir Sementsov-Ogievskiy
  Cc: Kevin Wolf, Paolo Bonzini, Denis Plotnikov, qemu-devel, Qemu-block


[-- 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 --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] vl: Drain before (block) job cancel when quitting
  2019-06-19 19:40             ` Max Reitz
@ 2019-07-16  7:00               ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-07-16  7:00 UTC (permalink / raw)
  To: Max Reitz, Vladimir Sementsov-Ogievskiy
  Cc: Kevin Wolf, Paolo Bonzini, Denis Plotnikov, qemu-devel, Qemu-block

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
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting
  2019-06-12 22:08 [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
                   ` (2 preceding siblings ...)
  2019-06-12 22:31 ` [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
@ 2019-07-19 13:26 ` Kevin Wolf
  3 siblings, 0 replies; 9+ messages in thread
From: Kevin Wolf @ 2019-07-19 13:26 UTC (permalink / raw)
  To: Max Reitz; +Cc: Paolo Bonzini, qemu-devel, qemu-block

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2019-07-19 13:27 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-12 22:08 [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
2019-06-12 22:08 ` [Qemu-devel] [PATCH 1/2] " Max Reitz
     [not found]   ` <b110b753-8546-0d34-f6ef-06c5726766ce@virtuozzo.com>
     [not found]     ` <c1fbf12a-77af-d939-4266-67b822e5a923@redhat.com>
     [not found]       ` <57ae7f82-ae02-a382-74f6-cb96672b2058@virtuozzo.com>
2019-06-13 16:03         ` Max Reitz
2019-06-14  9:22           ` Vladimir Sementsov-Ogievskiy
2019-06-19 19:40             ` Max Reitz
2019-07-16  7:00               ` Philippe Mathieu-Daudé
2019-06-12 22:08 ` [Qemu-devel] [PATCH 2/2] iotests: Test quitting with job on throttled node Max Reitz
2019-06-12 22:31 ` [Qemu-devel] [PATCH 0/2] vl: Drain before (block) job cancel when quitting Max Reitz
2019-07-19 13:26 ` Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).