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
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;
>> }
>>
>
