Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 10/03/2016 10:40, Christian Borntraeger wrote:
> On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
>> On 03/10/2016 02:51 AM, Fam Zheng wrote:
>> [...]
>>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
>>> suspicious:
>>>
>>>        main thread                                          iothread
>>> ----------------------------------------------------------------------------
>>>     virtio_blk_handle_output()
>>>      virtio_blk_data_plane_start()
>>>       vblk->dataplane_started = true;
>>>       blk_set_aio_context()
>>>        bdrv_set_aio_context()
>>>         bdrv_drain()
>>>          aio_poll()
>>>           <snip...>
>>>            virtio_blk_handle_output()
>>>             /* s->dataplane_started is true */
>>> !!!   ->    virtio_blk_handle_request()
>>>          event_notifier_set(ioeventfd)
>>>                                                     aio_poll()
>>>                                                      
>>> virtio_blk_handle_request()
>>>
>>> Christian, could you try the followed patch? The aio_poll above is replaced
>>> with a "limited aio_poll" that doesn't disptach ioeventfd.
>>>
>>> (Note: perhaps moving "vblk->dataplane_started = true;" after
>>> blk_set_aio_context() also *works around* this.)
>>>
>>> ---
>>>
>>> diff --git a/block.c b/block.c
>>> index ba24b8e..e37e8f7 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>>
>>>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>>  {
>>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>>> +    /* ensure there are no in-flight requests */
>>> +    bdrv_drained_begin(bs);
>>> +    bdrv_drained_end(bs);
>>>
>>>      bdrv_detach_aio_context(bs);
>>>
>>
>> That seems to do the trick. 
> 
> Or not. Crashed again :-(

I would put bdrv_drained_end just before aio_context_release.

But secondarily, I'm thinking of making the logic simpler to understand 
in two ways:

1) adding a mutex around virtio_blk_data_plane_start/stop.

2) moving

    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);

to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
true starts the processing.

Thanks,

Paolo