[Qemu-devel] [PATCH] block: don't set the same context

[Qemu-devel] [PATCH] block: don't set the same context

[Denis Plotnikov]

Adds a fast path on aio context setting preventing
unnecessary context setting routine.
Also, it prevents issues with cyclic walk of child
bds-es appeared because of registring aio walking
notifiers:

Call stack:

0  __GI_raise
1  __GI_abort
2  __assert_fail_base
3  __GI___assert_fail
4  bdrv_detach_aio_context (bs=0x55f54d65c000)      <<<
5  bdrv_detach_aio_context (bs=0x55f54fc8a800)
6  bdrv_set_aio_context (bs=0x55f54fc8a800, ...)
7  block_job_attached_aio_context
8  bdrv_attach_aio_context (bs=0x55f54d65c000, ...) <<<
9  bdrv_set_aio_context (bs=0x55f54d65c000)
10 blk_set_aio_context
11 virtio_blk_data_plane_stop
12 virtio_bus_stop_ioeventfd
13 virtio_vmstate_change
14 vm_state_notify (running=0, state=RUN_STATE_SHUTDOWN)
15 do_vm_stop (state=RUN_STATE_SHUTDOWN, send_stop=true)
16 vm_stop (state=RUN_STATE_SHUTDOWN)
17 main_loop_should_exit
18 main_loop
19 main

This can happen because of "new" context attachment to VM disk bds.
When attaching a new context the corresponding aio context handler is
called for each of aio_notifiers registered on the VM disk bds context.
Among those handlers, there is the block_job_attached_aio_context handler
which sets a new aio context for the block job bds. When doing so,
the old context is detached from all the block job bds children and one of
them is the VM disk bds, serving as backing store for the blockjob bds,
although the VM disk bds is actually the initializer of that process.
Since the VM disk bds is protected with walking_aio_notifiers flag
from double processing in recursive calls, the assert fires.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 block.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index adda221c2c..5742545e7a 100644
--- a/block.c
+++ b/block.c
@@ -5263,6 +5263,10 @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
     AioContext *ctx = bdrv_get_aio_context(bs);
 
+    if (ctx == new_context) {
+        return;
+    }
+
     aio_disable_external(ctx);
     bdrv_parent_drained_begin(bs, NULL, false);
     bdrv_drain(bs); /* ensure there are no in-flight requests */
-- 
2.17.0

Re: [Qemu-devel] [PATCH] block: don't set the same context

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1490 bytes --]

On 2/15/19 3:19 AM, Denis Plotnikov wrote:
> Adds a fast path on aio context setting preventing
> unnecessary context setting routine.
> Also, it prevents issues with cyclic walk of child
> bds-es appeared because of registring aio walking

registering

> notifiers:
> 

> This can happen because of "new" context attachment to VM disk bds.
> When attaching a new context the corresponding aio context handler is
> called for each of aio_notifiers registered on the VM disk bds context.
> Among those handlers, there is the block_job_attached_aio_context handler
> which sets a new aio context for the block job bds. When doing so,
> the old context is detached from all the block job bds children and one of
> them is the VM disk bds, serving as backing store for the blockjob bds,
> although the VM disk bds is actually the initializer of that process.
> Since the VM disk bds is protected with walking_aio_notifiers flag
> from double processing in recursive calls, the assert fires.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  block.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 

Naive question - do we have to always call
attach_aio_context/detach_aio_context in pairs?  If so, will
short-circuiting a nested attach cause reference count problems when we
come to the matching detach calls?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH] block: don't set the same context

[Denis Plotnikov]

On 15.02.2019 15:29, Eric Blake wrote:
> On 2/15/19 3:19 AM, Denis Plotnikov wrote:
>> Adds a fast path on aio context setting preventing
>> unnecessary context setting routine.
>> Also, it prevents issues with cyclic walk of child
>> bds-es appeared because of registring aio walking
> 
> registering
will fix it
> 
>> notifiers:
>>
> 
>> This can happen because of "new" context attachment to VM disk bds.
>> When attaching a new context the corresponding aio context handler is
>> called for each of aio_notifiers registered on the VM disk bds context.
>> Among those handlers, there is the block_job_attached_aio_context handler
>> which sets a new aio context for the block job bds. When doing so,
>> the old context is detached from all the block job bds children and one of
>> them is the VM disk bds, serving as backing store for the blockjob bds,
>> although the VM disk bds is actually the initializer of that process.
>> Since the VM disk bds is protected with walking_aio_notifiers flag
>> from double processing in recursive calls, the assert fires.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>>   block.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
> 
> Naive question - do we have to always call
> attach_aio_context/detach_aio_context in pairs?
seems, to be so. the only user doing attach/detach is bdrv_set_aio_context
   If so, will
> short-circuiting a nested attach cause reference count problems when we
> come to the matching detach calls?
I don't quite understand. I suggested the short circuit for 
bdrv_set_aio_context which actually pairs attach/detach context. So, the 
pairing here should be a problem
> 

-- 
Best,
Denis