[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/sto
From: |
Cornelia Huck |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH |
Date: |
Wed, 23 Mar 2016 09:10:09 +0100 |
On Tue, 22 Mar 2016 19:05:09 +0100
Paolo Bonzini <address@hidden> wrote:
> On 22/03/2016 13:52, Fam Zheng wrote:
> >> You're right. After unrealizing virtio_blk_data_plane_stop has set of
> >> vblk->dataplane_started = false, so that's covered. However, you still
> >> need an object_ref/object_object_unref pair.
> >
> > Is it safe to call object_unref outside BQL?
>
> Hmm, no.
>
> However, perhaps we can fix the code without a bottom half, using the
> assertion in virtio_blk_data_plane_start to ensure that there is no
> unwanted reentrancy.
>
> Conny's patches are also enough to mask the bug for me, so my tests
> do not say much. But in any case the following patch works here too
> instead of Fam's 4/4; it is a mess including some other experiments,
> but I'm including it as is because that's what I tested and it's
> dinner time now.
>
> Even if it fails for you or Tu Bo, perhaps the backtraces say
> something.
>
> Thanks,
>
> Paolo
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 1b2d5fa..5f72671 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -26,8 +26,7 @@
> #include "qom/object_interfaces.h"
>
> struct VirtIOBlockDataPlane {
> - bool starting;
> - bool stopping;
> + int starting;
> bool disabled;
>
> VirtIOBlkConf *conf;
> @@ -192,11 +191,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> int r;
>
> - if (vblk->dataplane_started || s->starting) {
> - return;
> - }
> -
> - s->starting = true;
> + assert(atomic_fetch_inc(&s->starting) == 0);
> s->vq = virtio_get_queue(s->vdev, 0);
>
> /* Set up guest notifier (irq) */
> @@ -215,27 +210,28 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane
> *s)
> goto fail_host_notifier;
> }
>
> - s->starting = false;
> - vblk->dataplane_started = true;
> trace_virtio_blk_data_plane_start(s);
>
> blk_set_aio_context(s->conf->conf.blk, s->ctx);
>
> - /* 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?
> return;
>
> fail_host_notifier:
> k->set_guest_notifiers(qbus->parent, 1, false);
> fail_guest_notifiers:
> s->disabled = true;
> - s->starting = false;
> vblk->dataplane_started = true;
> + atomic_dec(&s->starting);
> }
>
> /* Context: QEMU global mutex held */
> @@ -245,7 +241,7 @@ 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 (!vblk->dataplane_started) {
No fear of reentrancy here?
> 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;
> }
>
- [Qemu-devel] [PATCH 1/4] block: Use drained section in bdrv_set_aio_context, (continued)
- [Qemu-devel] [PATCH 1/4] block: Use drained section in bdrv_set_aio_context, Fam Zheng, 2016/03/16
- [Qemu-devel] [PATCH 2/4] block-backend: Introduce blk_drained_begin/end, Fam Zheng, 2016/03/16
- [Qemu-devel] [PATCH 3/4] virtio-blk: Use blk_drained_begin/end around dataplane stop, Fam Zheng, 2016/03/16
- [Qemu-devel] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Fam Zheng, 2016/03/16
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Stefan Hajnoczi, 2016/03/17
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Paolo Bonzini, 2016/03/17
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Fam Zheng, 2016/03/22
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Paolo Bonzini, 2016/03/22
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH,
Cornelia Huck <=
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Paolo Bonzini, 2016/03/23
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Christian Borntraeger, 2016/03/23
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, tu bo, 2016/03/24
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Cornelia Huck, 2016/03/24
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Cornelia Huck, 2016/03/24
- Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH, Cornelia Huck, 2016/03/24
Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop, Paolo Bonzini, 2016/03/16