From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35140) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aidrw-0002wc-Cb for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:10:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aidrt-0006dP-I5 for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:10:20 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:52304) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aidrt-0006bZ-A6 for qemu-devel@nongnu.org; Wed, 23 Mar 2016 04:10:17 -0400 Received: from localhost by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 23 Mar 2016 08:10:15 -0000 Date: Wed, 23 Mar 2016 09:10:09 +0100 From: Cornelia Huck Message-ID: <20160323091009.64eb4cd8.cornelia.huck@de.ibm.com> In-Reply-To: <56F18955.4060005@redhat.com> References: <1458123018-18651-1-git-send-email-famz@redhat.com> <1458123018-18651-5-git-send-email-famz@redhat.com> <20160317150057.GP14062@stefanha-x1.localdomain> <56EAC82A.50907@redhat.com> <20160322125254.GB9749@ad.usersys.redhat.com> <56F18955.4060005@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] virtio-blk: Clean up start/stop with mutex and BH List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Kevin Wolf , Fam Zheng , qemu-block@nongnu.org, "Michael S. Tsirkin" , Stefan Hajnoczi , qemu-devel@nongnu.org, tubo@linux.vnet.ibm.com, Stefan Hajnoczi , borntraeger@de.ibm.com On Tue, 22 Mar 2016 19:05:09 +0100 Paolo Bonzini 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; > } >