From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41778) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afpef-0006rQ-Hq for qemu-devel@nongnu.org; Tue, 15 Mar 2016 10:09:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1afpec-0006oP-A1 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 10:09:01 -0400 Received: from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242]:34564) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1afpeb-0006oL-S4 for qemu-devel@nongnu.org; Tue, 15 Mar 2016 10:08:58 -0400 Received: by mail-wm0-x242.google.com with SMTP id p65so3778743wmp.1 for ; Tue, 15 Mar 2016 07:08:57 -0700 (PDT) Sender: Paolo Bonzini References: <1455470231-5223-1-git-send-email-pbonzini@redhat.com> <1455470231-5223-6-git-send-email-pbonzini@redhat.com> <56E01544.6060305@de.ibm.com> <56E01D3F.1060204@redhat.com> <56E03333.5020601@de.ibm.com> <56E04C9B.7070801@redhat.com> <20160310015154.GD23632@ad.usersys.redhat.com> <56E13849.3060409@de.ibm.com> <56E14101.4030405@de.ibm.com> <56E29DB1.6000200@redhat.com> <20160315124530.GA10718@ad.usersys.redhat.com> <20160315141838.744c2068.cornelia.huck@de.ibm.com> From: Paolo Bonzini Message-ID: <56E81776.2000006@redhat.com> Date: Tue, 15 Mar 2016 15:08:54 +0100 MIME-Version: 1.0 In-Reply-To: <20160315141838.744c2068.cornelia.huck@de.ibm.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck , Fam Zheng Cc: Christian Borntraeger , qemu-devel@nongnu.org, Bo Tu On 15/03/2016 14:18, Cornelia Huck wrote: > On Tue, 15 Mar 2016 20:45:30 +0800 > Fam Zheng wrote: > >> On Fri, 03/11 11:28, Paolo Bonzini wrote: > >>> But secondarily, I'm thinking of making the logic simpler to understand >>> in two ways: >>> >>> 1) adding a mutex around virtio_blk_data_plane_start/stop. >>> >>> 2) moving >>> >>> event_notifier_set(virtio_queue_get_host_notifier(s->vq)); >>> virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); >>> >>> to a bottom half (created with aio_bh_new in s->ctx). The bottom half >>> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's >>> true starts the processing. >> >> Like this? If it captures your idea, could Bo or Christian help test? >> >> --- >> >> From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001 >> From: Fam Zheng >> Date: Thu, 10 Mar 2016 10:26:36 +0800 >> Subject: [PATCH] virtio-blk dataplane start crash fix >> >> Suggested-by: Paolo Bonzini >> Signed-off-by: Fam Zheng >> --- >> block.c | 4 +++- >> hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++------- >> 2 files changed, 35 insertions(+), 8 deletions(-) >> >> diff --git a/block.c b/block.c >> index ba24b8e..e37e8f7 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs, >> >> void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context) >> { >> - bdrv_drain(bs); /* ensure there are no in-flight requests */ >> + /* ensure there are no in-flight requests */ >> + bdrv_drained_begin(bs); >> + bdrv_drained_end(bs); I'm not sure that this is necessary. An empty section should be the same as plain old bdrv_drain. >> bdrv_detach_aio_context(bs); >> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c >> index 36f3d2b..6db5c22 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane { >> >> /* Operation blocker on BDS */ >> Error *blocker; >> + >> + QemuMutex start_lock; >> }; >> >> /* Raise an interrupt to signal guest, if necessary */ >> @@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, >> s = g_new0(VirtIOBlockDataPlane, 1); >> s->vdev = vdev; >> s->conf = conf; >> + qemu_mutex_init(&s->start_lock); >> >> if (conf->iothread) { >> s->iothread = conf->iothread; >> @@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) >> g_free(s); >> } >> >> +typedef struct { >> + VirtIOBlockDataPlane *s; >> + QEMUBH *bh; >> +} VirtIOBlockStartData; >> + >> +static void virtio_blk_data_plane_start_bh_cb(void *opaque) >> +{ >> + VirtIOBlockStartData *data = opaque; >> + VirtIOBlockDataPlane *s = data->s; > > Won't you need to check here whether ->started is still set? Yes. >> + >> + /* Kick right away to begin processing requests already in vring */ >> + event_notifier_set(virtio_queue_get_host_notifier(s->vq)); >> + >> + /* Get this show started by hooking up our callbacks */ >> + virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true); >> + >> + qemu_bh_delete(data->bh); >> + g_free(data); >> +} >> + >> /* Context: QEMU global mutex held */ >> void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> { >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev))); >> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus); >> VirtIOBlock *vblk = VIRTIO_BLK(s->vdev); >> + VirtIOBlockStartData *data; >> int r; >> >> + qemu_mutex_lock(&s->start_lock); >> if (vblk->dataplane_started || s->starting) { > > Do we still need ->starting with the new mutex? No, but really we shouldn't have needed it before either. :) So a task for another day. >> + qemu_mutex_unlock(&s->start_lock); >> return; >> } >> >> /* Context: QEMU global mutex held */ > > Do you also need to do something in _stop()? _stop definitely needs to take the mutex too.