Hmm, now I have trouble getting the whole thing started (Dont know how I was able to start the guest from below). The problem seems to be that qdev->name is always "virtio-blk".
So this code in virtio_blk_data_plane_create will always add a child called "virtio-blk", which obviously doesnt work so great with more that one disk. } else { /* Create per-device IOThread if none specified */ Error *local_err = NULL; s->internal_iothread = true; object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err); if (error_is_set(&local_err)) { error_propagate(errp, local_err); g_free(s); return; } s->iothread = iothread_find(vdev->name); assert(s->iothread); } s->ctx = iothread_get_aio_context(s->iothread); On 17/03/14 16:52, Christian Borntraeger wrote: > This causes the following bug during managedsave of s390 with a guest that > has multiple disks with dataplane. > > (gdb) bt > #0 object_deinit (type=0x0, obj=0x807e8750) at > /home/cborntra/REPOS/qemu/qom/object.c:410 > #1 object_finalize (data=0x807e8750) at > /home/cborntra/REPOS/qemu/qom/object.c:424 > #2 object_unref (obj=0x807e8750) at > /home/cborntra/REPOS/qemu/qom/object.c:726 > #3 0x000000008011da60 in object_property_del (obj=0x807e8d80, > name=0x807e8d60 "virtio-blk", errp=<optimized out>) at > /home/cborntra/REPOS/qemu/qom/object.c:783 > #4 0x000000008011dc0e in object_property_del_child (errp=0x0, > child=0x807e8750, obj=0x807e8d80) at > /home/cborntra/REPOS/qemu/qom/object.c:386 > #5 object_unparent (obj=0x807e8750) at > /home/cborntra/REPOS/qemu/qom/object.c:403 > #6 0x0000000080183898 in virtio_blk_data_plane_destroy (s=0x8088a750) at > /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:430 > #7 0x00000000801845a0 in virtio_blk_migration_state_changed > (notifier=0x807ebf20, data=0x80314088 <current_migration.25446>) at > /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:666 > #8 0x00000000802161f8 in notifier_list_notify (list=list@entry=0x80726310 > <migration_state_notifiers>, data=data@entry=0x80314088 > <current_migration.25446>) > at /home/cborntra/REPOS/qemu/util/notify.c:39 > #9 0x00000000800ccecc in migrate_fd_connect (s=s@entry=0x80314088 > <current_migration.25446>) at /home/cborntra/REPOS/qemu/migration.c:696 > #10 0x00000000800cae9c in fd_start_outgoing_migration (s=s@entry=0x80314088 > <current_migration.25446>, fdname=<optimized out>, > errp=errp@entry=0x3fffff298c0) > at /home/cborntra/REPOS/qemu/migration-fd.c:42 > #11 0x00000000800cc986 in qmp_migrate (uri=0x808f6190 "fd:migrate", > has_blk=<optimized out>, blk=<optimized out>, has_inc=<optimized out>, > inc=inc@entry=false, has_detach=true, detach=true, errp= > 0x3fffff29998) at /home/cborntra/REPOS/qemu/migration.c:450 > #12 0x00000000801166f6 in qmp_marshal_input_migrate (mon=<optimized out>, > qdict=<optimized out>, ret=<optimized out>) at qmp-marshal.c:3285 > #13 0x00000000801b252a in qmp_call_cmd (cmd=<optimized out>, > params=0x808f6280, mon=0x808150d0) at /home/cborntra/REPOS/qemu/monitor.c:4760 > #14 handle_qmp_command (parser=<optimized out>, tokens=<optimized out>) at > /home/cborntra/REPOS/qemu/monitor.c:4826 > #15 0x0000000080202f66 in json_message_process_token (lexer=0x80815050, > token=0x808f3e20, type=<optimized out>, x=<optimized out>, y=9) at > /home/cborntra/REPOS/qemu/qobject/json-streamer.c:87 > #16 0x000000008021b36a in json_lexer_feed_char (lexer=lexer@entry=0x80815050, > ch=<optimized out>, flush=flush@entry=false) at > /home/cborntra/REPOS/qemu/qobject/json-lexer.c:303 > #17 0x000000008021b4ec in json_lexer_feed (lexer=0x80815050, > buffer=<optimized out>, size=<optimized out>) at > /home/cborntra/REPOS/qemu/qobject/json-lexer.c:356 > #18 0x00000000802031a2 in json_message_parser_feed (parser=<optimized out>, > buffer=<optimized out>, size=<optimized out>) at > /home/cborntra/REPOS/qemu/qobject/json-streamer.c:110 > #19 0x00000000801b015a in monitor_control_read (opaque=<optimized out>, > buf=<optimized out>, size=<optimized out>) at > /home/cborntra/REPOS/qemu/monitor.c:4847 > #20 0x00000000801023e2 in qemu_chr_be_write (len=1, buf=0x3fffff29e18 "}[A", > s=0x807cdf40) at /home/cborntra/REPOS/qemu/qemu-char.c:165 > #21 tcp_chr_read (chan=<optimized out>, cond=<optimized out>, > opaque=0x807cdf40) at /home/cborntra/REPOS/qemu/qemu-char.c:2487 > #22 0x000003fffd54005a in g_main_context_dispatch () from > /lib64/libglib-2.0.so.0 > #23 0x00000000800ca9b2 in glib_pollfds_poll () at > /home/cborntra/REPOS/qemu/main-loop.c:190 > #24 os_host_main_loop_wait (timeout=<optimized out>) at > /home/cborntra/REPOS/qemu/main-loop.c:235 > #25 main_loop_wait (nonblocking=nonblocking@entry=0) at > /home/cborntra/REPOS/qemu/main-loop.c:484 > #26 0x0000000080014f2a in main_loop () at /home/cborntra/REPOS/qemu/vl.c:2053 > #27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) > at /home/cborntra/REPOS/qemu/vl.c:4524 > > > Reverting commit 48ff269272f18d2b8fa53cb08365df417588f585 seems to fix this > particualar bug. > Any ideas? > > Christian > > On 13/03/14 15:10, Stefan Hajnoczi wrote: >> Today virtio-blk dataplane uses a 1:1 device-per-thread model. Now that >> IOThreads have been introduced we can generalize this to N:M devices per >> threads. >> >> This patch drops thread code from dataplane in favor of running inside >> an IOThread AioContext. >> >> As a bonus we solve the case where a guest keeps submitting I/O requests >> while dataplane is trying to stop. Previously the dataplane thread >> would continue to process requests until the request gave it a break. >> Now we can shut down in bounded time thanks to >> aio_context_acquire/release. >> >> Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com> >> --- >> hw/block/dataplane/virtio-blk.c | 96 >> +++++++++++++++++++++++------------------ >> include/hw/virtio/virtio-blk.h | 8 +++- >> 2 files changed, 60 insertions(+), 44 deletions(-) >> >> diff --git a/hw/block/dataplane/virtio-blk.c >> b/hw/block/dataplane/virtio-blk.c >> index d1c7ad4..a5afc21 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -23,6 +23,7 @@ >> #include "virtio-blk.h" >> #include "block/aio.h" >> #include "hw/virtio/virtio-bus.h" >> +#include "monitor/monitor.h" /* for object_add() */ >> >> enum { >> SEG_MAX = 126, /* maximum number of I/O segments */ >> @@ -44,8 +45,6 @@ struct VirtIOBlockDataPlane { >> bool started; >> bool starting; >> bool stopping; >> - QEMUBH *start_bh; >> - QemuThread thread; >> >> VirtIOBlkConf *blk; >> int fd; /* image file descriptor */ >> @@ -59,12 +58,14 @@ struct VirtIOBlockDataPlane { >> * (because you don't own the file descriptor or handle; you just >> * use it). >> */ >> + IOThread *iothread; >> + bool internal_iothread; >> AioContext *ctx; >> EventNotifier io_notifier; /* Linux AIO completion */ >> EventNotifier host_notifier; /* doorbell */ >> >> IOQueue ioqueue; /* Linux AIO queue (should really be per >> - dataplane thread) */ >> + IOThread) */ >> VirtIOBlockRequest requests[REQ_MAX]; /* pool of requests, managed by >> the >> queue */ >> >> @@ -342,26 +343,7 @@ static void handle_io(EventNotifier *e) >> } >> } >> >> -static void *data_plane_thread(void *opaque) >> -{ >> - VirtIOBlockDataPlane *s = opaque; >> - >> - while (!s->stopping || s->num_reqs > 0) { >> - aio_poll(s->ctx, true); >> - } >> - return NULL; >> -} >> - >> -static void start_data_plane_bh(void *opaque) >> -{ >> - VirtIOBlockDataPlane *s = opaque; >> - >> - qemu_bh_delete(s->start_bh); >> - s->start_bh = NULL; >> - qemu_thread_create(&s->thread, "data_plane", data_plane_thread, >> - s, QEMU_THREAD_JOINABLE); >> -} >> - >> +/* Context: QEMU global mutex held */ >> void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk, >> VirtIOBlockDataPlane **dataplane, >> Error **errp) >> @@ -408,12 +390,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, >> VirtIOBlkConf *blk, >> s->fd = fd; >> s->blk = blk; >> >> + if (blk->iothread) { >> + s->internal_iothread = false; >> + s->iothread = blk->iothread; >> + object_ref(OBJECT(s->iothread)); >> + } else { >> + /* Create per-device IOThread if none specified */ >> + Error *local_err = NULL; >> + >> + s->internal_iothread = true; >> + object_add(TYPE_IOTHREAD, vdev->name, NULL, NULL, &local_err); >> + if (error_is_set(&local_err)) { >> + error_propagate(errp, local_err); >> + g_free(s); >> + return; >> + } >> + s->iothread = iothread_find(vdev->name); >> + assert(s->iothread); >> + } >> + s->ctx = iothread_get_aio_context(s->iothread); >> + >> /* Prevent block operations that conflict with data plane thread */ >> bdrv_set_in_use(blk->conf.bs, 1); >> >> *dataplane = s; >> } >> >> +/* Context: QEMU global mutex held */ >> void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s) >> { >> if (!s) { >> @@ -422,9 +425,14 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane >> *s) >> >> virtio_blk_data_plane_stop(s); >> bdrv_set_in_use(s->blk->conf.bs, 0); >> + object_unref(OBJECT(s->iothread)); >> + if (s->internal_iothread) { >> + object_unparent(OBJECT(s->iothread)); >> + } >> g_free(s); >> } >> >> +/* Context: QEMU global mutex held */ >> void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> { >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev))); >> @@ -448,8 +456,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> return; >> } >> >> - s->ctx = aio_context_new(); >> - >> /* Set up guest notifier (irq) */ >> if (k->set_guest_notifiers(qbus->parent, 1, true) != 0) { >> fprintf(stderr, "virtio-blk failed to set guest notifier, " >> @@ -464,7 +470,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> exit(1); >> } >> s->host_notifier = *virtio_queue_get_host_notifier(vq); >> - aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify); >> >> /* Set up ioqueue */ >> ioq_init(&s->ioqueue, s->fd, REQ_MAX); >> @@ -472,7 +477,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) >> ioq_put_iocb(&s->ioqueue, &s->requests[i].iocb); >> } >> s->io_notifier = *ioq_get_notifier(&s->ioqueue); >> - aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io); >> >> s->starting = false; >> s->started = true; >> @@ -481,11 +485,14 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane >> *s) >> /* Kick right away to begin processing requests already in vring */ >> event_notifier_set(virtio_queue_get_host_notifier(vq)); >> >> - /* Spawn thread in BH so it inherits iothread cpusets */ >> - s->start_bh = qemu_bh_new(start_data_plane_bh, s); >> - qemu_bh_schedule(s->start_bh); >> + /* Get this show started by hooking up our callbacks */ >> + aio_context_acquire(s->ctx); >> + aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify); >> + aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io); >> + aio_context_release(s->ctx); >> } >> >> +/* Context: QEMU global mutex held */ >> void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) >> { >> BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev))); >> @@ -496,27 +503,32 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane >> *s) >> s->stopping = true; >> trace_virtio_blk_data_plane_stop(s); >> >> - /* Stop thread or cancel pending thread creation BH */ >> - if (s->start_bh) { >> - qemu_bh_delete(s->start_bh); >> - s->start_bh = NULL; >> - } else { >> - aio_notify(s->ctx); >> - qemu_thread_join(&s->thread); >> + aio_context_acquire(s->ctx); >> + >> + /* Stop notifications for new requests from guest */ >> + aio_set_event_notifier(s->ctx, &s->host_notifier, NULL); >> + >> + /* Complete pending requests */ >> + while (s->num_reqs > 0) { >> + aio_poll(s->ctx, true); >> } >> >> + /* Stop ioq callbacks (there are no pending requests left) */ >> aio_set_event_notifier(s->ctx, &s->io_notifier, NULL); >> - ioq_cleanup(&s->ioqueue); >> >> - aio_set_event_notifier(s->ctx, &s->host_notifier, NULL); >> - k->set_host_notifier(qbus->parent, 0, false); >> + aio_context_release(s->ctx); >> >> - aio_context_unref(s->ctx); >> + /* Sync vring state back to virtqueue so that non-dataplane request >> + * processing can continue when we disable the host notifier below. >> + */ >> + vring_teardown(&s->vring, s->vdev, 0); >> + >> + ioq_cleanup(&s->ioqueue); >> + k->set_host_notifier(qbus->parent, 0, false); >> >> /* Clean up guest notifier (irq) */ >> k->set_guest_notifiers(qbus->parent, 1, false); >> >> - vring_teardown(&s->vring, s->vdev, 0); >> s->started = false; >> s->stopping = false; >> } >> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h >> index 41885da..e4c41ff 100644 >> --- a/include/hw/virtio/virtio-blk.h >> +++ b/include/hw/virtio/virtio-blk.h >> @@ -16,6 +16,7 @@ >> >> #include "hw/virtio/virtio.h" >> #include "hw/block/block.h" >> +#include "sysemu/iothread.h" >> >> #define TYPE_VIRTIO_BLK "virtio-blk-device" >> #define VIRTIO_BLK(obj) \ >> @@ -106,6 +107,7 @@ struct virtio_scsi_inhdr >> struct VirtIOBlkConf >> { >> BlockConf conf; >> + IOThread *iothread; >> char *serial; >> uint32_t scsi; >> uint32_t config_wce; >> @@ -140,13 +142,15 @@ typedef struct VirtIOBlock { >> DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), >> \ >> DEFINE_PROP_STRING("serial", _state, _field.serial), >> \ >> DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), >> \ >> - DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true) >> + DEFINE_PROP_BIT("scsi", _state, _field.scsi, 0, true), >> \ >> + DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread) >> #else >> #define DEFINE_VIRTIO_BLK_PROPERTIES(_state, _field) >> \ >> DEFINE_BLOCK_PROPERTIES(_state, _field.conf), >> \ >> DEFINE_BLOCK_CHS_PROPERTIES(_state, _field.conf), >> \ >> DEFINE_PROP_STRING("serial", _state, _field.serial), >> \ >> - DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true) >> + DEFINE_PROP_BIT("config-wce", _state, _field.config_wce, 0, true), >> \ >> + DEFINE_PROP_IOTHREAD("x-iothread", _state, _field.iothread) >> #endif /* __linux__ */ >> >> void virtio_blk_set_conf(DeviceState *dev, VirtIOBlkConf *blk); >> >