| Message ID | 56C34414.90809@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, 16 Feb 2016 16:45:24 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 15/02/2016 18:58, Cornelia Huck wrote: > > It seems a bit odd to me that ->started is the only state that is not > > inside the dataplane struct... this approach saves a function call for > > an accessor, though. > > Actually, I can do better by moving the flag entirely within > hw/block/virtio-blk.c: > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index e04c8f5..34bae8e 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -590,6 +590,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) > * dataplane here instead of waiting for .set_status(). > */ > if (s->dataplane && !s->dataplane_started) { > + s->dataplane_started = true; > virtio_blk_data_plane_start(s->dataplane); > return; > } > @@ -658,8 +659,9 @@ static void virtio_blk_reset(VirtIODevice *vdev) > aio_context_acquire(ctx); > blk_drain(s->blk); > > - if (s->dataplane) { > + if (s->dataplane && s->dataplane_started) { > virtio_blk_data_plane_stop(s->dataplane); > + s->dataplane_started = false; > } > aio_context_release(ctx); > > > Does it look better? I think yes. Hm... this seems to guarantee that _start()/_stop() never runs concurrently, doesn't it? Could we get rid of the ->starting/->stopping flags as well? ...and ->disabled as well, since we try just once until we stop?
On 16/02/2016 17:15, Cornelia Huck wrote: > Hm... this seems to guarantee that _start()/_stop() never runs > concurrently, doesn't it? Could we get rid of the ->starting/->stopping > flags as well? > > ...and ->disabled as well, since we try just once until we stop? Sounds like that, yes. Paolo
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index cc521c1..29db94b 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -263,7 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) VirtQueue *vq; int r; - if (vblk->dataplane_started || s->starting) { + if (s->starting) { return; } @@ -295,7 +295,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) vblk->complete_request = complete_request_vring; s->starting = false; - vblk->dataplane_started = true; trace_virtio_blk_data_plane_start(s); blk_set_aio_context(s->conf->conf.blk, s->ctx); @@ -317,7 +316,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) fail_vring: s->disabled = true; s->starting = false; - vblk->dataplane_started = true; } /* Context: QEMU global mutex held */ @@ -327,14 +325,13 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); VirtIOBlock *vblk = VIRTIO_BLK(s->vdev); - if (!vblk->dataplane_started || s->stopping) { + if (s->stopping) { return; } /* Better luck next time. */ if (s->disabled) { s->disabled = false; - vblk->dataplane_started = false; return; } s->stopping = true; @@ -361,6 +358,5 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) /* Clean up guest notifier (irq) */ k->set_guest_notifiers(qbus->parent, 1, false); - vblk->dataplane_started = false; s->stopping = false; } diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index e04c8f5..34bae8e 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -590,6 +590,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq) * dataplane here instead of waiting for .set_status(). */ if (s->dataplane && !s->dataplane_started) { + s->dataplane_started = true; virtio_blk_data_plane_start(s->dataplane); return; } @@ -658,8 +659,9 @@ static void virtio_blk_reset(VirtIODevice *vdev) aio_context_acquire(ctx); blk_drain(s->blk); - if (s->dataplane) { + if (s->dataplane && s->dataplane_started) { virtio_blk_data_plane_stop(s->dataplane); + s->dataplane_started = false; } aio_context_release(ctx);