Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH

[tu bo <tubo@linux.vnet.ibm.com>]

Hi Christian:

On 03/23/2016 05:12 PM, Christian Borntraeger wrote:
> On 03/23/2016 10:08 AM, Paolo Bonzini wrote:
>>
>>
>> On 23/03/2016 09:10, Cornelia Huck wrote:
>>>> -    /* Kick right away to begin processing requests already in vring */
>>>> -    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>>>> +    vblk->dataplane_started = true;
>>>>
>>>> -    /* Get this show started by hooking up our callbacks */
>>>> +    /* Get this show started by hooking up our callbacks.  */
>>>>       aio_context_acquire(s->ctx);
>>>>       virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>>>>       aio_context_release(s->ctx);
>>>> +    atomic_dec(&s->starting);
>>>> +
>>>> +    /* Kick right away to begin processing requests already in vring */
>>>> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>>>
>>> I'm wondering whether moving this event_notifier_set() masks something?
>>> IOW, may we run into trouble if the event notifier is set from some
>>> other path before the callbacks are set up properly?
>>
>> The reentrancy check should catch that...  But:
>>
>> 1) the patch really makes no difference, your fix is enough for me
>
>
> Tu Bo, can you test with master + Cornelias 6 refactoring patches and nothing on top?

With qemu master + Cornelia's 6 refactoring patches and nothing on top, 
I did NOT see crash so far.

>
>
>
>> 2) vblk->dataplane_started becomes true before the callbacks are set;
>> that should be enough.
>>
>> 3) this matches what I tested, but it would of course be better if the
>> assertions on s->starting suffice
>>
>>>> -    if (!vblk->dataplane_started || s->stopping) {
>>>> +    if (!vblk->dataplane_started) {
>>>
>>> No fear of reentrancy here?
>>
>> No, because this is only invoked from reset, hence only from the CPU
>> thread and only under the BQL.
>>
>> On start, reentrancy happens between iothread (running outside BQL) and
>> a CPU thread (running within BQL).
>>
>> Paolo
>>
>>>>           return;
>>>>       }
>>>>
>>>> @@ -255,7 +251,7 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>>>>           vblk->dataplane_started = false;
>>>>           return;
>>>>       }
>>>> -    s->stopping = true;
>>>> +
>>>>       trace_virtio_blk_data_plane_stop(s);
>>>>
>>>>       aio_context_acquire(s->ctx);
>>>> @@ -274,5 +270,4 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>>>>       k->set_guest_notifiers(qbus->parent, 1, false);
>>>>
>>>>       vblk->dataplane_started = false;
>>>> -    s->stopping = false;
>>>>   }
>>>>
>>>
>>
>