From 78f1d3d6a6904459b0b6cfd735278e4e2b7b68c5 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 9 Nov 2017 10:26:52 +0000 Subject: coroutine: simplify co_aio_sleep_ns() prototype The AioContext pointer argument to co_aio_sleep_ns() is only used for the sleep timer. It does not affect where the caller coroutine is resumed. Due to changes to coroutine and AIO APIs it is now possible to drop the AioContext pointer argument. This is safe to do since no caller has specific requirements for which AioContext the timer must run in. This patch drops the AioContext pointer argument and renames the function to simplify the API. Reported-by: Paolo Bonzini Reported-by: Eric Blake Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171109102652.6360-1-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- block/null.c | 3 +-- block/sheepdog.c | 3 +-- include/qemu/coroutine.h | 6 +----- util/qemu-coroutine-sleep.c | 4 ++-- 4 files changed, 5 insertions(+), 11 deletions(-) diff --git a/block/null.c b/block/null.c index dd9c13f..0cdabaa 100644 --- a/block/null.c +++ b/block/null.c @@ -110,8 +110,7 @@ static coroutine_fn int null_co_common(BlockDriverState *bs) BDRVNullState *s = bs->opaque; if (s->latency_ns) { - co_aio_sleep_ns(bdrv_get_aio_context(bs), QEMU_CLOCK_REALTIME, - s->latency_ns); + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, s->latency_ns); } return 0; } diff --git a/block/sheepdog.c b/block/sheepdog.c index 696a714..a1edb99 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -776,8 +776,7 @@ static coroutine_fn void reconnect_to_sdog(void *opaque) if (s->fd < 0) { DPRINTF("Wait for connection to be established\n"); error_report_err(local_err); - co_aio_sleep_ns(bdrv_get_aio_context(s->bs), QEMU_CLOCK_REALTIME, - 1000000000ULL); + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000000ULL); } }; diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h index 9aff9a73..ce2eb73 100644 --- a/include/qemu/coroutine.h +++ b/include/qemu/coroutine.h @@ -261,12 +261,8 @@ void qemu_co_rwlock_unlock(CoRwlock *lock); /** * Yield the coroutine for a given duration - * - * Behaves similarly to co_sleep_ns(), but the sleeping coroutine will be - * resumed when using aio_poll(). */ -void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type, - int64_t ns); +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns); /** * Yield until a file descriptor becomes readable diff --git a/util/qemu-coroutine-sleep.c b/util/qemu-coroutine-sleep.c index 254349c..afb678f 100644 --- a/util/qemu-coroutine-sleep.c +++ b/util/qemu-coroutine-sleep.c @@ -31,9 +31,9 @@ static void co_sleep_cb(void *opaque) aio_co_wake(sleep_cb->co); } -void coroutine_fn co_aio_sleep_ns(AioContext *ctx, QEMUClockType type, - int64_t ns) +void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns) { + AioContext *ctx = qemu_get_current_aio_context(); CoSleepCB sleep_cb = { .co = qemu_coroutine_self(), }; -- cgit v1.1 From e01d6a415bec6fb93a7413929fb1438a22d28419 Mon Sep 17 00:00:00 2001 From: Mao Zhongyi Date: Wed, 22 Nov 2017 11:08:43 +0800 Subject: hw/block/nvme: Convert to realize Convert nvme_init() to realize and rename it to nvme_realize(). Cc: John Snow Cc: Keith Busch Cc: Kevin Wolf Cc: Max Reitz Cc: Markus Armbruster Signed-off-by: Mao Zhongyi Message-id: 2882e72d795e04cbe2120f569d551aef2467ac60.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi --- hw/block/nvme.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index 441e21e..e530ba7 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -920,7 +920,7 @@ static const MemoryRegionOps nvme_cmb_ops = { }, }; -static int nvme_init(PCIDevice *pci_dev) +static void nvme_realize(PCIDevice *pci_dev, Error **errp) { NvmeCtrl *n = NVME(pci_dev); NvmeIdCtrl *id = &n->id_ctrl; @@ -931,24 +931,27 @@ static int nvme_init(PCIDevice *pci_dev) Error *local_err = NULL; if (!n->conf.blk) { - return -1; + error_setg(errp, "drive property not set"); + return; } bs_size = blk_getlength(n->conf.blk); if (bs_size < 0) { - return -1; + error_setg(errp, "could not get backing file size"); + return; } blkconf_serial(&n->conf, &n->serial); if (!n->serial) { - return -1; + error_setg(errp, "serial property not set"); + return; } blkconf_blocksizes(&n->conf); blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), false, &local_err); if (local_err) { - error_report_err(local_err); - return -1; + error_propagate(errp, local_err); + return; } pci_conf = pci_dev->config; @@ -1046,7 +1049,6 @@ static int nvme_init(PCIDevice *pci_dev) cpu_to_le64(n->ns_size >> id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds); } - return 0; } static void nvme_exit(PCIDevice *pci_dev) @@ -1081,7 +1083,7 @@ static void nvme_class_init(ObjectClass *oc, void *data) DeviceClass *dc = DEVICE_CLASS(oc); PCIDeviceClass *pc = PCI_DEVICE_CLASS(oc); - pc->init = nvme_init; + pc->realize = nvme_realize; pc->exit = nvme_exit; pc->class_id = PCI_CLASS_STORAGE_EXPRESS; pc->vendor_id = PCI_VENDOR_ID_INTEL; -- cgit v1.1 From 9d3b155186c2788570f4802700d030c84e0f0746 Mon Sep 17 00:00:00 2001 From: Mao Zhongyi Date: Wed, 22 Nov 2017 11:08:44 +0800 Subject: hw/block: Fix the return type When the function no success value to transmit, it usually make the function return void. It has turned out not to be a success, because it means that the extra local_err variable and error_propagate() will be needed. It leads to cumbersome code, therefore, transmit success/ failure in the return value is worth. So fix the return type of blkconf_apply_backend_options(), blkconf_geometry() and virtio_blk_data_plane_create() to avoid it. Cc: John Snow Cc: Kevin Wolf Cc: Max Reitz Cc: Stefan Hajnoczi Signed-off-by: Mao Zhongyi Reviewed-by: Stefan Hajnoczi Message-id: ac0edc1fc70c4457e5cec94405eb7d1f89f9c2c1.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi --- hw/block/block.c | 15 +++++++++------ hw/block/dataplane/virtio-blk.c | 12 +++++++----- hw/block/dataplane/virtio-blk.h | 2 +- include/hw/block/block.h | 4 ++-- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/hw/block/block.c b/hw/block/block.c index 27878d0..b0269c8 100644 --- a/hw/block/block.c +++ b/hw/block/block.c @@ -51,7 +51,7 @@ void blkconf_blocksizes(BlockConf *conf) } } -void blkconf_apply_backend_options(BlockConf *conf, bool readonly, +bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, bool resizable, Error **errp) { BlockBackend *blk = conf->blk; @@ -76,7 +76,7 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly, ret = blk_set_perm(blk, perm, shared_perm, errp); if (ret < 0) { - return; + return false; } switch (conf->wce) { @@ -99,9 +99,11 @@ void blkconf_apply_backend_options(BlockConf *conf, bool readonly, blk_set_enable_write_cache(blk, wce); blk_set_on_error(blk, rerror, werror); + + return true; } -void blkconf_geometry(BlockConf *conf, int *ptrans, +bool blkconf_geometry(BlockConf *conf, int *ptrans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp) { @@ -129,15 +131,16 @@ void blkconf_geometry(BlockConf *conf, int *ptrans, if (conf->cyls || conf->heads || conf->secs) { if (conf->cyls < 1 || conf->cyls > cyls_max) { error_setg(errp, "cyls must be between 1 and %u", cyls_max); - return; + return false; } if (conf->heads < 1 || conf->heads > heads_max) { error_setg(errp, "heads must be between 1 and %u", heads_max); - return; + return false; } if (conf->secs < 1 || conf->secs > secs_max) { error_setg(errp, "secs must be between 1 and %u", secs_max); - return; + return false; } } + return true; } diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 5556f0e..f6fc639 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -76,7 +76,7 @@ static void notify_guest_bh(void *opaque) } /* Context: QEMU global mutex held */ -void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, +bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, Error **errp) { @@ -91,11 +91,11 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, error_setg(errp, "device is incompatible with iothread " "(transport does not support notifiers)"); - return; + return false; } if (!virtio_device_ioeventfd_enabled(vdev)) { error_setg(errp, "ioeventfd is required for iothread"); - return; + return false; } /* If dataplane is (re-)enabled while the guest is running there could @@ -103,12 +103,12 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, */ if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) { error_prepend(errp, "cannot start virtio-blk dataplane: "); - return; + return false; } } /* Don't try if transport does not support notifiers. */ if (!virtio_device_ioeventfd_enabled(vdev)) { - return; + return false; } s = g_new0(VirtIOBlockDataPlane, 1); @@ -126,6 +126,8 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, s->batch_notify_vqs = bitmap_new(conf->num_queues); *dataplane = s; + + return true; } /* Context: QEMU global mutex held */ diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h index db3f47b..5e18bb9 100644 --- a/hw/block/dataplane/virtio-blk.h +++ b/hw/block/dataplane/virtio-blk.h @@ -19,7 +19,7 @@ typedef struct VirtIOBlockDataPlane VirtIOBlockDataPlane; -void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, +bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf, VirtIOBlockDataPlane **dataplane, Error **errp); void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s); diff --git a/include/hw/block/block.h b/include/hw/block/block.h index f3f6e8e..64b9298 100644 --- a/include/hw/block/block.h +++ b/include/hw/block/block.h @@ -72,11 +72,11 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf) /* Configuration helpers */ void blkconf_serial(BlockConf *conf, char **serial); -void blkconf_geometry(BlockConf *conf, int *trans, +bool blkconf_geometry(BlockConf *conf, int *trans, unsigned cyls_max, unsigned heads_max, unsigned secs_max, Error **errp); void blkconf_blocksizes(BlockConf *conf); -void blkconf_apply_backend_options(BlockConf *conf, bool readonly, +bool blkconf_apply_backend_options(BlockConf *conf, bool readonly, bool resizable, Error **errp); /* Hard disk geometry */ -- cgit v1.1 From ceff3e1f01ee8c0d77de7e3750e7297c3af7e2ec Mon Sep 17 00:00:00 2001 From: Mao Zhongyi Date: Wed, 22 Nov 2017 11:08:45 +0800 Subject: hw/block: Use errp directly rather than local_err [Drop virtio_blk_data_plane_create() change that misinterprets return value when the virtio transport does not support dataplane. --Stefan] Cc: John Snow Cc: Kevin Wolf Cc: Max Reitz Cc: Keith Busch Cc: Stefan Hajnoczi Cc: "Michael S. Tsirkin" Cc: Paolo Bonzini Cc: Gerd Hoffmann Cc: Markus Armbruster Signed-off-by: Mao Zhongyi Reviewed-by: Stefan Hajnoczi Message-id: e77848d3735ba590f23ffbf8094379c646c33d79.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi --- hw/block/fdc.c | 17 ++++++----------- hw/block/nvme.c | 7 ++----- hw/block/virtio-blk.c | 13 +++++-------- hw/ide/qdev.c | 12 ++++-------- hw/scsi/scsi-disk.c | 13 ++++--------- hw/usb/dev-storage.c | 9 +++------ 6 files changed, 24 insertions(+), 47 deletions(-) diff --git a/hw/block/fdc.c b/hw/block/fdc.c index 67f78ac..7b7dd41 100644 --- a/hw/block/fdc.c +++ b/hw/block/fdc.c @@ -473,16 +473,13 @@ static void fd_revalidate(FDrive *drv) static void fd_change_cb(void *opaque, bool load, Error **errp) { FDrive *drive = opaque; - Error *local_err = NULL; if (!load) { blk_set_perm(drive->blk, 0, BLK_PERM_ALL, &error_abort); } else { - blkconf_apply_backend_options(drive->conf, - blk_is_read_only(drive->blk), false, - &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(drive->conf, + blk_is_read_only(drive->blk), false, + errp)) { return; } } @@ -522,7 +519,6 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) FloppyDrive *dev = FLOPPY_DRIVE(qdev); FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus); FDrive *drive; - Error *local_err = NULL; int ret; if (dev->unit == -1) { @@ -568,10 +564,9 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp) dev->conf.rerror = BLOCKDEV_ON_ERROR_AUTO; dev->conf.werror = BLOCKDEV_ON_ERROR_AUTO; - blkconf_apply_backend_options(&dev->conf, blk_is_read_only(dev->conf.blk), - false, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(&dev->conf, + blk_is_read_only(dev->conf.blk), + false, errp)) { return; } diff --git a/hw/block/nvme.c b/hw/block/nvme.c index e530ba7..e529e88 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -928,7 +928,6 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) int i; int64_t bs_size; uint8_t *pci_conf; - Error *local_err = NULL; if (!n->conf.blk) { error_setg(errp, "drive property not set"); @@ -947,10 +946,8 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp) return; } blkconf_blocksizes(&n->conf); - blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), - false, &local_err); - if (local_err) { - error_propagate(errp, local_err); + if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk), + false, errp)) { return; } diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 05d1440..41a4ccded 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -930,19 +930,16 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) } blkconf_serial(&conf->conf, &conf->serial); - blkconf_apply_backend_options(&conf->conf, - blk_is_read_only(conf->conf.blk), true, - &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&conf->conf, + blk_is_read_only(conf->conf.blk), true, + errp)) { return; } s->original_wce = blk_enable_write_cache(conf->conf.blk); - blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp)) { return; } + blkconf_blocksizes(&conf->conf); virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c index a5181b4..f395d24 100644 --- a/hw/ide/qdev.c +++ b/hw/ide/qdev.c @@ -160,7 +160,6 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) { IDEBus *bus = DO_UPCAST(IDEBus, qbus, dev->qdev.parent_bus); IDEState *s = bus->ifs + dev->unit; - Error *err = NULL; int ret; if (!dev->conf.blk) { @@ -191,16 +190,13 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp) blkconf_serial(&dev->conf, &dev->serial); if (kind != IDE_CD) { - blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255, + errp)) { return; } } - blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, kind != IDE_CD, - &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD, + kind != IDE_CD, errp)) { return; } diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c index 1243117..870d9ae 100644 --- a/hw/scsi/scsi-disk.c +++ b/hw/scsi/scsi-disk.c @@ -2332,7 +2332,6 @@ static void scsi_disk_unit_attention_reported(SCSIDevice *dev) static void scsi_realize(SCSIDevice *dev, Error **errp) { SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev); - Error *err = NULL; if (!s->qdev.conf.blk) { error_setg(errp, "drive property not set"); @@ -2356,17 +2355,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp) } if (dev->type == TYPE_DISK) { - blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_geometry(&dev->conf, NULL, 65535, 255, 255, errp)) { return; } } - blkconf_apply_backend_options(&dev->conf, - blk_is_read_only(s->qdev.conf.blk), - dev->type == TYPE_DISK, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&dev->conf, + blk_is_read_only(s->qdev.conf.blk), + dev->type == TYPE_DISK, errp)) { return; } diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index 8a61ec9..a9bcc67 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -601,7 +601,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) MSDState *s = USB_STORAGE_DEV(dev); BlockBackend *blk = s->conf.blk; SCSIDevice *scsi_dev; - Error *err = NULL; if (!blk) { error_setg(errp, "drive property not set"); @@ -610,9 +609,8 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) blkconf_serial(&s->conf, &dev->serial); blkconf_blocksizes(&s->conf); - blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true, &err); - if (err) { - error_propagate(errp, err); + if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true, + errp)) { return; } @@ -636,10 +634,9 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) &usb_msd_scsi_info_storage, NULL); scsi_dev = scsi_bus_legacy_add_drive(&s->bus, blk, 0, !!s->removable, s->conf.bootindex, dev->serial, - &err); + errp); blk_unref(blk); if (!scsi_dev) { - error_propagate(errp, err); return; } usb_msd_handle_reset(dev); -- cgit v1.1 From 6db3ea39e2650dc4fbcd18e075510f162265512f Mon Sep 17 00:00:00 2001 From: Mao Zhongyi Date: Wed, 22 Nov 2017 11:08:46 +0800 Subject: dev-storage: Fix the unusual function name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The function name of usb_msd_{realize,unrealize}_*, usb_msd_class_initfn_* are unusual. Rename it to usb_msd_*_{realize,unrealize}, usb_msd_class_*_initfn. Cc: Gerd Hoffmann Signed-off-by: Mao Zhongyi Reviewed-by: Philippe Mathieu-Daudé Message-id: 11e6003433abce35f3f4970e1acc71ee92dbcf51.1511317952.git.maozy.fnst@cn.fujitsu.com Signed-off-by: Stefan Hajnoczi --- hw/usb/dev-storage.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index a9bcc67..9722ac8 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -596,7 +596,7 @@ static void usb_msd_unrealize_storage(USBDevice *dev, Error **errp) object_unref(OBJECT(&s->bus)); } -static void usb_msd_realize_storage(USBDevice *dev, Error **errp) +static void usb_msd_storage_realize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); BlockBackend *blk = s->conf.blk; @@ -643,14 +643,14 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp) s->scsi_dev = scsi_dev; } -static void usb_msd_unrealize_bot(USBDevice *dev, Error **errp) +static void usb_msd_bot_unrealize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); object_unref(OBJECT(&s->bus)); } -static void usb_msd_realize_bot(USBDevice *dev, Error **errp) +static void usb_msd_bot_realize(USBDevice *dev, Error **errp) { MSDState *s = USB_STORAGE_DEV(dev); DeviceState *d = DEVICE(dev); @@ -764,12 +764,12 @@ static void usb_msd_class_initfn_common(ObjectClass *klass, void *data) dc->vmsd = &vmstate_usb_msd; } -static void usb_msd_class_initfn_storage(ObjectClass *klass, void *data) +static void usb_msd_class_storage_initfn(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); USBDeviceClass *uc = USB_DEVICE_CLASS(klass); - uc->realize = usb_msd_realize_storage; + uc->realize = usb_msd_storage_realize; uc->unrealize = usb_msd_unrealize_storage; dc->props = msd_properties; } @@ -828,26 +828,26 @@ static void usb_msd_instance_init(Object *obj) object_property_set_int(obj, -1, "bootindex", NULL); } -static void usb_msd_class_initfn_bot(ObjectClass *klass, void *data) +static void usb_msd_class_bot_initfn(ObjectClass *klass, void *data) { USBDeviceClass *uc = USB_DEVICE_CLASS(klass); - uc->realize = usb_msd_realize_bot; - uc->unrealize = usb_msd_unrealize_bot; + uc->realize = usb_msd_bot_realize; + uc->unrealize = usb_msd_bot_unrealize; uc->attached_settable = true; } static const TypeInfo msd_info = { .name = "usb-storage", .parent = TYPE_USB_STORAGE, - .class_init = usb_msd_class_initfn_storage, + .class_init = usb_msd_class_storage_initfn, .instance_init = usb_msd_instance_init, }; static const TypeInfo bot_info = { .name = "usb-bot", .parent = TYPE_USB_STORAGE, - .class_init = usb_msd_class_initfn_bot, + .class_init = usb_msd_class_bot_initfn, }; static void usb_msd_register_types(void) -- cgit v1.1 From a12aef24fa3e3312fac024ac22809ee9deb4d98a Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Tue, 5 Dec 2017 13:39:54 +0000 Subject: qdev: drop unused #include "sysemu/iothread.h" Commit 1351d1ec89eabebc9fdff20451a62c413d7accc1 ("qdev: drop iothread property type") forgot to remove this include. Signed-off-by: Stefan Hajnoczi Message-id: 20171205133954.31006-1-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- hw/core/qdev-properties-system.c | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index c173646..46b3843 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -22,7 +22,6 @@ #include "qapi/visitor.h" #include "chardev/char-fe.h" #include "sysemu/tpm_backend.h" -#include "sysemu/iothread.h" static void get_pointer(Object *obj, Visitor *v, Property *prop, char *(*print)(void *ptr), -- cgit v1.1 From b9464ba19f821ea6b29969104dc48dcdb26243dd Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:42 +0000 Subject: blockdev: hold AioContext for bdrv_unref() in external_snapshot_clean() bdrv_unref() requires the AioContext lock because bdrv_flush() uses BDRV_POLL_WHILE(), which assumes the AioContext is currently held. If BDRV_POLL_WHILE() runs without AioContext held the pthread_mutex_unlock() call in aio_context_release() fails. This patch moves bdrv_unref() into the AioContext locked region to solve the following pthread_mutex_unlock() failure: #0 0x00007f566181969b in raise () at /lib64/libc.so.6 #1 0x00007f566181b3b1 in abort () at /lib64/libc.so.6 #2 0x00005592cd590458 in error_exit (err=, msg=msg@entry=0x5592cdaf6d60 <__func__.23977> "qemu_mutex_unlock") at util/qemu-thread-posix.c:36 #3 0x00005592cd96e738 in qemu_mutex_unlock (mutex=mutex@entry=0x5592ce9505e0) at util/qemu-thread-posix.c:96 #4 0x00005592cd969b69 in aio_context_release (ctx=ctx@entry=0x5592ce950580) at util/async.c:507 #5 0x00005592cd8ead78 in bdrv_flush (bs=bs@entry=0x5592cfa87210) at block/io.c:2478 #6 0x00005592cd89df30 in bdrv_close (bs=0x5592cfa87210) at block.c:3207 #7 0x00005592cd89df30 in bdrv_delete (bs=0x5592cfa87210) at block.c:3395 #8 0x00005592cd89df30 in bdrv_unref (bs=0x5592cfa87210) at block.c:4418 #9 0x00005592cd6b7f86 in qmp_transaction (dev_list=, has_props=, props=, errp=errp@entry=0x7ffe4a1fc9d8) at blockdev.c:2308 Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-2-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/blockdev.c b/blockdev.c index 56a6b24..3c8d994 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1812,8 +1812,8 @@ static void external_snapshot_clean(BlkActionState *common) DO_UPCAST(ExternalSnapshotState, common, common); if (state->aio_context) { bdrv_drained_end(state->old_bs); - aio_context_release(state->aio_context); bdrv_unref(state->new_bs); + aio_context_release(state->aio_context); } } -- cgit v1.1 From 2d24b60b7747f7bf40fd00b0375b6bd988d4f0d9 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:43 +0000 Subject: block: don't keep AioContext acquired after external_snapshot_prepare() It is not necessary to hold AioContext across transactions anymore since bdrv_drained_begin/end() is used to keep the nodes quiesced. In fact, using the AioContext lock for this purpose was always buggy. This patch reduces the scope of AioContext locked regions. This is not just a cleanup but also fixes hangs that occur in BDRV_POLL_WHILE() because it is unware of recursive locking and does not release the AioContext the necessary number of times to allow progress to be made. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-3-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 71 ++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 23 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3c8d994..3b598f8 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1606,7 +1606,6 @@ typedef struct ExternalSnapshotState { BlkActionState common; BlockDriverState *old_bs; BlockDriverState *new_bs; - AioContext *aio_context; bool overlay_appended; } ExternalSnapshotState; @@ -1626,6 +1625,7 @@ static void external_snapshot_prepare(BlkActionState *common, ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); TransactionAction *action = common->action; + AioContext *aio_context; /* 'blockdev-snapshot' and 'blockdev-snapshot-sync' have similar * purpose but a different set of parameters */ @@ -1662,31 +1662,32 @@ static void external_snapshot_prepare(BlkActionState *common, return; } - /* Acquire AioContext now so any threads operating on old_bs stop */ - state->aio_context = bdrv_get_aio_context(state->old_bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ bdrv_drained_begin(state->old_bs); if (!bdrv_is_inserted(state->old_bs)) { error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device); - return; + goto out; } if (bdrv_op_is_blocked(state->old_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (!bdrv_is_read_only(state->old_bs)) { if (bdrv_flush(state->old_bs)) { error_setg(errp, QERR_IO_ERROR); - return; + goto out; } } if (!bdrv_is_first_non_filter(state->old_bs)) { error_setg(errp, QERR_FEATURE_DISABLED, "snapshot"); - return; + goto out; } if (action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC) { @@ -1698,13 +1699,13 @@ static void external_snapshot_prepare(BlkActionState *common, if (node_name && !snapshot_node_name) { error_setg(errp, "New snapshot node name missing"); - return; + goto out; } if (snapshot_node_name && bdrv_lookup_bs(snapshot_node_name, snapshot_node_name, NULL)) { error_setg(errp, "New snapshot node name already in use"); - return; + goto out; } flags = state->old_bs->open_flags; @@ -1717,7 +1718,7 @@ static void external_snapshot_prepare(BlkActionState *common, int64_t size = bdrv_getlength(state->old_bs); if (size < 0) { error_setg_errno(errp, -size, "bdrv_getlength failed"); - return; + goto out; } bdrv_img_create(new_image_file, format, state->old_bs->filename, @@ -1725,7 +1726,7 @@ static void external_snapshot_prepare(BlkActionState *common, NULL, size, flags, false, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } } @@ -1740,30 +1741,30 @@ static void external_snapshot_prepare(BlkActionState *common, errp); /* We will manually add the backing_hd field to the bs later */ if (!state->new_bs) { - return; + goto out; } if (bdrv_has_blk(state->new_bs)) { error_setg(errp, "The snapshot is already in use"); - return; + goto out; } if (bdrv_op_is_blocked(state->new_bs, BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (state->new_bs->backing != NULL) { error_setg(errp, "The snapshot already has a backing image"); - return; + goto out; } if (!state->new_bs->drv->supports_backing) { error_setg(errp, "The snapshot does not support backing images"); - return; + goto out; } - bdrv_set_aio_context(state->new_bs, state->aio_context); + bdrv_set_aio_context(state->new_bs, aio_context); /* This removes our old bs and adds the new bs. This is an operation that * can fail, so we need to do it in .prepare; undoing it for abort is @@ -1772,15 +1773,22 @@ static void external_snapshot_prepare(BlkActionState *common, bdrv_append(state->new_bs, state->old_bs, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } state->overlay_appended = true; + +out: + aio_context_release(aio_context); } static void external_snapshot_commit(BlkActionState *common) { ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); /* We don't need (or want) to use the transactional * bdrv_reopen_multiple() across all the entries at once, because we @@ -1789,6 +1797,8 @@ static void external_snapshot_commit(BlkActionState *common) bdrv_reopen(state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR, NULL); } + + aio_context_release(aio_context); } static void external_snapshot_abort(BlkActionState *common) @@ -1797,11 +1807,18 @@ static void external_snapshot_abort(BlkActionState *common) DO_UPCAST(ExternalSnapshotState, common, common); if (state->new_bs) { if (state->overlay_appended) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + bdrv_ref(state->old_bs); /* we can't let bdrv_set_backind_hd() close state->old_bs; we need it */ bdrv_set_backing_hd(state->new_bs, NULL, &error_abort); bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */ + + aio_context_release(aio_context); } } } @@ -1810,11 +1827,19 @@ static void external_snapshot_clean(BlkActionState *common) { ExternalSnapshotState *state = DO_UPCAST(ExternalSnapshotState, common, common); - if (state->aio_context) { - bdrv_drained_end(state->old_bs); - bdrv_unref(state->new_bs); - aio_context_release(state->aio_context); + AioContext *aio_context; + + if (!state->old_bs) { + return; } + + aio_context = bdrv_get_aio_context(state->old_bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->old_bs); + bdrv_unref(state->new_bs); + + aio_context_release(aio_context); } typedef struct DriveBackupState { -- cgit v1.1 From 66d56054bca3c1c45861d18ea97f147f7d376d21 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:44 +0000 Subject: block: don't keep AioContext acquired after drive_backup_prepare() Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-4-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 42 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 8 deletions(-) diff --git a/blockdev.c b/blockdev.c index 3b598f8..5a56a1a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1845,7 +1845,6 @@ static void external_snapshot_clean(BlkActionState *common) typedef struct DriveBackupState { BlkActionState common; BlockDriverState *bs; - AioContext *aio_context; BlockJob *job; } DriveBackupState; @@ -1857,6 +1856,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); BlockDriverState *bs; DriveBackup *backup; + AioContext *aio_context; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_DRIVE_BACKUP); @@ -1867,24 +1867,36 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp) return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); + + /* Paired with .clean() */ bdrv_drained_begin(bs); + state->bs = bs; state->job = do_drive_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } + +out: + aio_context_release(aio_context); } static void drive_backup_commit(BlkActionState *common) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + assert(state->job); block_job_start(state->job); + + aio_context_release(aio_context); } static void drive_backup_abort(BlkActionState *common) @@ -1892,18 +1904,32 @@ static void drive_backup_abort(BlkActionState *common) DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); if (state->job) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + block_job_cancel_sync(state->job); + + aio_context_release(aio_context); } } static void drive_backup_clean(BlkActionState *common) { DriveBackupState *state = DO_UPCAST(DriveBackupState, common, common); + AioContext *aio_context; - if (state->aio_context) { - bdrv_drained_end(state->bs); - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } typedef struct BlockdevBackupState { -- cgit v1.1 From edd5adeecddf665e7954bc146ff458bb30f178ae Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:45 +0000 Subject: block: don't keep AioContext acquired after blockdev_backup_prepare() Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-5-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 10 deletions(-) diff --git a/blockdev.c b/blockdev.c index 5a56a1a..d7ad764 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1936,7 +1936,6 @@ typedef struct BlockdevBackupState { BlkActionState common; BlockDriverState *bs; BlockJob *job; - AioContext *aio_context; } BlockdevBackupState; static BlockJob *do_blockdev_backup(BlockdevBackup *backup, BlockJobTxn *txn, @@ -1947,6 +1946,7 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); BlockdevBackup *backup; BlockDriverState *bs, *target; + AioContext *aio_context; Error *local_err = NULL; assert(common->action->type == TRANSACTION_ACTION_KIND_BLOCKDEV_BACKUP); @@ -1962,29 +1962,39 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - if (state->aio_context != bdrv_get_aio_context(target)) { - state->aio_context = NULL; + aio_context = bdrv_get_aio_context(bs); + if (aio_context != bdrv_get_aio_context(target)) { error_setg(errp, "Backup between two IO threads is not implemented"); return; } - aio_context_acquire(state->aio_context); + aio_context_acquire(aio_context); state->bs = bs; + + /* Paired with .clean() */ bdrv_drained_begin(state->bs); state->job = do_blockdev_backup(backup, common->block_job_txn, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } + +out: + aio_context_release(aio_context); } static void blockdev_backup_commit(BlkActionState *common) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + assert(state->job); block_job_start(state->job); + + aio_context_release(aio_context); } static void blockdev_backup_abort(BlkActionState *common) @@ -1992,18 +2002,32 @@ static void blockdev_backup_abort(BlkActionState *common) BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); if (state->job) { + AioContext *aio_context; + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + block_job_cancel_sync(state->job); + + aio_context_release(aio_context); } } static void blockdev_backup_clean(BlkActionState *common) { BlockdevBackupState *state = DO_UPCAST(BlockdevBackupState, common, common); + AioContext *aio_context; - if (state->aio_context) { - bdrv_drained_end(state->bs); - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } typedef struct BlockDirtyBitmapState { -- cgit v1.1 From a36e458cdda0196911c1cbe7cfe6f9530f2280e3 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:46 +0000 Subject: block: don't keep AioContext acquired after internal_snapshot_prepare() Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-6-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 47 +++++++++++++++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/blockdev.c b/blockdev.c index d7ad764..6332a24 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1454,7 +1454,6 @@ struct BlkActionState { typedef struct InternalSnapshotState { BlkActionState common; BlockDriverState *bs; - AioContext *aio_context; QEMUSnapshotInfo sn; bool created; } InternalSnapshotState; @@ -1485,6 +1484,7 @@ static void internal_snapshot_prepare(BlkActionState *common, qemu_timeval tv; BlockdevSnapshotInternal *internal; InternalSnapshotState *state; + AioContext *aio_context; int ret1; g_assert(common->action->type == @@ -1506,32 +1506,33 @@ static void internal_snapshot_prepare(BlkActionState *common, return; } - /* AioContext is released in .clean() */ - state->aio_context = bdrv_get_aio_context(bs); - aio_context_acquire(state->aio_context); + aio_context = bdrv_get_aio_context(bs); + aio_context_acquire(aio_context); state->bs = bs; + + /* Paired with .clean() */ bdrv_drained_begin(bs); if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)) { - return; + goto out; } if (bdrv_is_read_only(bs)) { error_setg(errp, "Device '%s' is read only", device); - return; + goto out; } if (!bdrv_can_snapshot(bs)) { error_setg(errp, "Block format '%s' used by device '%s' " "does not support internal snapshots", bs->drv->format_name, device); - return; + goto out; } if (!strlen(name)) { error_setg(errp, "Name is empty"); - return; + goto out; } /* check whether a snapshot with name exist */ @@ -1539,12 +1540,12 @@ static void internal_snapshot_prepare(BlkActionState *common, &local_err); if (local_err) { error_propagate(errp, local_err); - return; + goto out; } else if (ret) { error_setg(errp, "Snapshot with name '%s' already exists on device '%s'", name, device); - return; + goto out; } /* 3. take the snapshot */ @@ -1560,11 +1561,14 @@ static void internal_snapshot_prepare(BlkActionState *common, error_setg_errno(errp, -ret1, "Failed to create snapshot '%s' on device '%s'", name, device); - return; + goto out; } /* 4. succeed, mark a snapshot is created */ state->created = true; + +out: + aio_context_release(aio_context); } static void internal_snapshot_abort(BlkActionState *common) @@ -1573,12 +1577,16 @@ static void internal_snapshot_abort(BlkActionState *common) DO_UPCAST(InternalSnapshotState, common, common); BlockDriverState *bs = state->bs; QEMUSnapshotInfo *sn = &state->sn; + AioContext *aio_context; Error *local_error = NULL; if (!state->created) { return; } + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) { error_reportf_err(local_error, "Failed to delete snapshot with id '%s' and " @@ -1586,19 +1594,26 @@ static void internal_snapshot_abort(BlkActionState *common) sn->id_str, sn->name, bdrv_get_device_name(bs)); } + + aio_context_release(aio_context); } static void internal_snapshot_clean(BlkActionState *common) { InternalSnapshotState *state = DO_UPCAST(InternalSnapshotState, common, common); + AioContext *aio_context; - if (state->aio_context) { - if (state->bs) { - bdrv_drained_end(state->bs); - } - aio_context_release(state->aio_context); + if (!state->bs) { + return; } + + aio_context = bdrv_get_aio_context(state->bs); + aio_context_acquire(aio_context); + + bdrv_drained_end(state->bs); + + aio_context_release(aio_context); } /* external snapshot private data */ -- cgit v1.1 From 5016f21b705152d138395f90219b665ddce89c1b Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:47 +0000 Subject: block: drop unused BlockDirtyBitmapState->aio_context field The dirty bitmap actions in qmp_transaction have not used AioContext since the dirty bitmap locking discipline was introduced in commit 2119882c7eb7e2c612b24fc0c8d86f5887d6f1c3 ("block: introduce dirty_bitmap_mutex"). Remove the unused field. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-7-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/blockdev.c b/blockdev.c index 6332a24..e865ae4 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2049,7 +2049,6 @@ typedef struct BlockDirtyBitmapState { BlkActionState common; BdrvDirtyBitmap *bitmap; BlockDriverState *bs; - AioContext *aio_context; HBitmap *backup; bool prepared; } BlockDirtyBitmapState; @@ -2128,7 +2127,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, } bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); - /* AioContext is released in .clean() */ } static void block_dirty_bitmap_clear_abort(BlkActionState *common) @@ -2149,16 +2147,6 @@ static void block_dirty_bitmap_clear_commit(BlkActionState *common) hbitmap_free(state->backup); } -static void block_dirty_bitmap_clear_clean(BlkActionState *common) -{ - BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, - common, common); - - if (state->aio_context) { - aio_context_release(state->aio_context); - } -} - static void abort_prepare(BlkActionState *common, Error **errp) { error_setg(errp, "Transaction aborted using Abort action"); @@ -2219,7 +2207,6 @@ static const BlkActionOps actions[] = { .prepare = block_dirty_bitmap_clear_prepare, .commit = block_dirty_bitmap_clear_commit, .abort = block_dirty_bitmap_clear_abort, - .clean = block_dirty_bitmap_clear_clean, } }; -- cgit v1.1 From fbcc6923b00c2b468a7470fec7863f0403a65736 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:48 +0000 Subject: iothread: add iothread_by_id() API Encapsulate IOThread QOM object lookup so that callers don't need to know how and where IOThread objects live. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-8-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- include/sysemu/iothread.h | 1 + iothread.c | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index 110329b..55de171 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -42,6 +42,7 @@ typedef struct { OBJECT_CHECK(IOThread, obj, TYPE_IOTHREAD) char *iothread_get_id(IOThread *iothread); +IOThread *iothread_by_id(const char *id); AioContext *iothread_get_aio_context(IOThread *iothread); void iothread_stop_all(void); GMainContext *iothread_get_g_main_context(IOThread *iothread); diff --git a/iothread.c b/iothread.c index 27a4288..e7b93e0 100644 --- a/iothread.c +++ b/iothread.c @@ -380,3 +380,10 @@ void iothread_destroy(IOThread *iothread) { object_unparent(OBJECT(iothread)); } + +/* Lookup IOThread by its id. Only finds user-created objects, not internal + * iothread_create() objects. */ +IOThread *iothread_by_id(const char *id) +{ + return IOTHREAD(object_resolve_path_type(id, TYPE_IOTHREAD, NULL)); +} -- cgit v1.1 From ca00bbb153d03c5338d8c8136812163f463f841e Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:49 +0000 Subject: blockdev: add x-blockdev-set-iothread testing command Currently there is no easy way for iotests to ensure that a BDS is bound to a particular IOThread. Normally the virtio-blk device calls blk_set_aio_context() when dataplane is enabled during guest driver initialization. This never happens in iotests since -machine accel=qtest means there is no guest activity (including device driver initialization). This patch adds a QMP command to explicitly assign IOThreads in test cases. See qapi/block-core.json for a description of the command. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-9-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 41 +++++++++++++++++++++++++++++++++++++++++ qapi/block-core.json | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/blockdev.c b/blockdev.c index e865ae4..f75c01f 100644 --- a/blockdev.c +++ b/blockdev.c @@ -45,6 +45,7 @@ #include "qapi/qmp/qerror.h" #include "qapi/qobject-output-visitor.h" #include "sysemu/sysemu.h" +#include "sysemu/iothread.h" #include "block/block_int.h" #include "qmp-commands.h" #include "block/trace.h" @@ -4129,6 +4130,46 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) return head; } +void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, + Error **errp) +{ + AioContext *old_context; + AioContext *new_context; + BlockDriverState *bs; + + bs = bdrv_find_node(node_name); + if (!bs) { + error_setg(errp, "Cannot find node %s", node_name); + return; + } + + /* If we want to allow more extreme test scenarios this guard could be + * removed. For now it protects against accidents. */ + if (bdrv_has_blk(bs)) { + error_setg(errp, "Node %s is in use", node_name); + return; + } + + if (iothread->type == QTYPE_QSTRING) { + IOThread *obj = iothread_by_id(iothread->u.s); + if (!obj) { + error_setg(errp, "Cannot find iothread %s", iothread->u.s); + return; + } + + new_context = iothread_get_aio_context(obj); + } else { + new_context = qemu_get_aio_context(); + } + + old_context = bdrv_get_aio_context(bs); + aio_context_acquire(old_context); + + bdrv_set_aio_context(bs, new_context); + + aio_context_release(old_context); +} + QemuOptsList qemu_common_drive_opts = { .name = "drive", .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head), diff --git a/qapi/block-core.json b/qapi/block-core.json index dd763dc..741d6c4 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3949,3 +3949,39 @@ 'data' : { 'parent': 'str', '*child': 'str', '*node': 'str' } } + +## +# @x-blockdev-set-iothread: +# +# Move @node and its children into the @iothread. If @iothread is null then +# move @node and its children into the main loop. +# +# The node must not be attached to a BlockBackend. +# +# @node-name: the name of the block driver node +# +# @iothread: the name of the IOThread object or null for the main loop +# +# Note: this command is experimental and intended for test cases that need +# control over IOThreads only. +# +# Since: 2.12 +# +# Example: +# +# 1. Move a node into an IOThread +# -> { "execute": "x-blockdev-set-iothread", +# "arguments": { "node-name": "disk1", +# "iothread": "iothread0" } } +# <- { "return": {} } +# +# 2. Move a node into the main loop +# -> { "execute": "x-blockdev-set-iothread", +# "arguments": { "node-name": "disk1", +# "iothread": null } } +# <- { "return": {} } +# +## +{ 'command': 'x-blockdev-set-iothread', + 'data' : { 'node-name': 'str', + 'iothread': 'StrOrNull' } } -- cgit v1.1 From 6dd64919ea36db9fd7e754ed394c25ced45ea39a Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Wed, 6 Dec 2017 14:45:50 +0000 Subject: qemu-iotests: add 202 external snapshots IOThread test QMP 'transaction' blockdev-snapshot-sync with multiple disks in an IOThread is an untested code path. Several bugs have been found in connection with this command. This patch adds a test case to prevent future regressions. Signed-off-by: Stefan Hajnoczi Reviewed-by: Kevin Wolf Reviewed-by: Eric Blake Message-id: 20171206144550.22295-10-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/202 | 95 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/202.out | 11 ++++++ tests/qemu-iotests/group | 1 + 3 files changed, 107 insertions(+) create mode 100755 tests/qemu-iotests/202 create mode 100644 tests/qemu-iotests/202.out diff --git a/tests/qemu-iotests/202 b/tests/qemu-iotests/202 new file mode 100755 index 0000000..581ca34 --- /dev/null +++ b/tests/qemu-iotests/202 @@ -0,0 +1,95 @@ +#!/usr/bin/env python +# +# Copyright (C) 2017 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Creator/Owner: Stefan Hajnoczi +# +# Check that QMP 'transaction' blockdev-snapshot-sync with multiple drives on a +# single IOThread completes successfully. This particular command triggered a +# hang due to recursive AioContext locking and BDRV_POLL_WHILE(). Protect +# against regressions. + +import iotests + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +with iotests.FilePath('disk0.img') as disk0_img_path, \ + iotests.FilePath('disk1.img') as disk1_img_path, \ + iotests.FilePath('disk0-snap.img') as disk0_snap_img_path, \ + iotests.FilePath('disk1-snap.img') as disk1_snap_img_path, \ + iotests.VM() as vm: + + img_size = '10M' + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size) + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size) + + iotests.log('Launching VM...') + vm.launch() + + iotests.log('Adding IOThread...') + iotests.log(vm.qmp('object-add', + qom_type='iothread', + id='iothread0')) + + iotests.log('Adding blockdevs...') + iotests.log(vm.qmp('blockdev-add', + driver=iotests.imgfmt, + node_name='disk0', + file={ + 'driver': 'file', + 'filename': disk0_img_path, + })) + iotests.log(vm.qmp('blockdev-add', + driver=iotests.imgfmt, + node_name='disk1', + file={ + 'driver': 'file', + 'filename': disk1_img_path, + })) + + iotests.log('Setting iothread...') + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='disk0', + iothread='iothread0')) + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='disk1', + iothread='iothread0')) + + iotests.log('Creating external snapshots...') + iotests.log(vm.qmp( + 'transaction', + actions=[ + { + 'data': { + 'node-name': 'disk0', + 'snapshot-file': disk0_snap_img_path, + 'snapshot-node-name': 'disk0-snap', + 'mode': 'absolute-paths', + 'format': iotests.imgfmt, + }, + 'type': 'blockdev-snapshot-sync' + }, { + 'data': { + 'node-name': 'disk1', + 'snapshot-file': disk1_snap_img_path, + 'snapshot-node-name': 'disk1-snap', + 'mode': 'absolute-paths', + 'format': iotests.imgfmt + }, + 'type': 'blockdev-snapshot-sync' + } + ])) diff --git a/tests/qemu-iotests/202.out b/tests/qemu-iotests/202.out new file mode 100644 index 0000000..d5ea374 --- /dev/null +++ b/tests/qemu-iotests/202.out @@ -0,0 +1,11 @@ +Launching VM... +Adding IOThread... +{u'return': {}} +Adding blockdevs... +{u'return': {}} +{u'return': {}} +Setting iothread... +{u'return': {}} +{u'return': {}} +Creating external snapshots... +{u'return': {}} diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 3e68867..d0ee1e2 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -197,3 +197,4 @@ 197 rw auto quick 198 rw auto 200 rw auto +202 rw auto quick -- cgit v1.1 From 6040aedddb5f474a9c2304b6a432a652d82b3d3c Mon Sep 17 00:00:00 2001 From: Mark Kanda Date: Mon, 11 Dec 2017 09:16:24 -0600 Subject: virtio-blk: make queue size configurable Depending on the configuration, it can be beneficial to adjust the virtio-blk queue size to something other than the current default of 128. Add a new property to make the queue size configurable. Signed-off-by: Mark Kanda Reviewed-by: Karl Heubaum Reviewed-by: Martin K. Petersen Reviewed-by: Ameya More Message-id: 52e6d742811f10dbd16e996e86cf375b9577c187.1513005190.git.mark.kanda@oracle.com Signed-off-by: Stefan Hajnoczi --- hw/block/virtio-blk.c | 10 +++++++++- include/hw/virtio/virtio-blk.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 41a4ccded..66ab0b1 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -928,6 +928,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) error_setg(errp, "num-queues property must be larger than 0"); return; } + if (!is_power_of_2(conf->queue_size) || + conf->queue_size > VIRTQUEUE_MAX_SIZE) { + error_setg(errp, "invalid queue-size property (%" PRIu16 "), " + "must be a power of 2 (max %d)", + conf->queue_size, VIRTQUEUE_MAX_SIZE); + return; + } blkconf_serial(&conf->conf, &conf->serial); if (!blkconf_apply_backend_options(&conf->conf, @@ -950,7 +957,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1; for (i = 0; i < conf->num_queues; i++) { - virtio_add_queue(vdev, 128, virtio_blk_handle_output); + virtio_add_queue(vdev, conf->queue_size, virtio_blk_handle_output); } virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err); if (err != NULL) { @@ -1009,6 +1016,7 @@ static Property virtio_blk_properties[] = { DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0, true), DEFINE_PROP_UINT16("num-queues", VirtIOBlock, conf.num_queues, 1), + DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128), DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD, IOThread *), DEFINE_PROP_END_OF_LIST(), diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index d3c8a6f..5117431 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -39,6 +39,7 @@ struct VirtIOBlkConf uint32_t config_wce; uint32_t request_merging; uint16_t num_queues; + uint16_t queue_size; }; struct VirtIOBlockDataPlane; -- cgit v1.1 From 0a75b60cdb685fed09ac0c0783bc0d6052a00e5d Mon Sep 17 00:00:00 2001 From: Mark Kanda Date: Mon, 11 Dec 2017 09:16:25 -0600 Subject: virtio-blk: reject configs with logical block size > physical block size virtio-blk logical block size should never be larger than physical block size because it doesn't make sense to have such configurations. QEMU doesn't have a way to effectively express this condition; the best it can do is report the physical block exponent as 0 - indicating the logical block size equals the physical block size. This is identical to commit 3da023b5827543ee4c022986ea2ad9d1274410b2 but applied to virtio-blk (instead of virtio-scsi). Signed-off-by: Mark Kanda Reviewed-by: Konrad Rzeszutek Wilk Reviewed-by: Ameya More Reviewed-by: Martin K. Petersen Reviewed-by: Stefan Hajnoczi Message-id: 773169891f9f2deb4cb7c4ef2655580dbe24c1d1.1513005190.git.mark.kanda@oracle.com Signed-off-by: Stefan Hajnoczi --- hw/block/virtio-blk.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index 66ab0b1..b1532e4 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -949,6 +949,13 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp) blkconf_blocksizes(&conf->conf); + if (conf->conf.logical_block_size > + conf->conf.physical_block_size) { + error_setg(errp, + "logical_block_size > physical_block_size not supported"); + return; + } + virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, sizeof(struct virtio_blk_config)); -- cgit v1.1 From bd6458e410c1e7d2912357aeb399fe7d8ee9f9a3 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Thu, 7 Dec 2017 20:13:15 +0000 Subject: block: avoid recursive AioContext acquire in bdrv_inactivate_all() BDRV_POLL_WHILE() does not support recursive AioContext locking. It only releases the AioContext lock once regardless of how many times the caller has acquired it. This results in a hang since the IOThread does not make progress while the AioContext is still locked. The following steps trigger the hang: $ qemu-system-x86_64 -M accel=kvm -m 1G -cpu host \ -object iothread,id=iothread0 \ -device virtio-scsi-pci,iothread=iothread0 \ -drive if=none,id=drive0,file=test.img,format=raw \ -device scsi-hd,drive=drive0 \ -drive if=none,id=drive1,file=test.img,format=raw \ -device scsi-hd,drive=drive1 $ qemu-system-x86_64 ...same options... \ -incoming tcp::1234 (qemu) migrate tcp:127.0.0.1:1234 ...hang... Tested-by: Stefan Hajnoczi Signed-off-by: Paolo Bonzini Reviewed-by: Eric Blake Message-id: 20171207201320.19284-2-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- block.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/block.c b/block.c index 9a1a0d1..1c37ce4 100644 --- a/block.c +++ b/block.c @@ -4320,9 +4320,15 @@ int bdrv_inactivate_all(void) BdrvNextIterator it; int ret = 0; int pass; + GSList *aio_ctxs = NULL, *ctx; for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { - aio_context_acquire(bdrv_get_aio_context(bs)); + AioContext *aio_context = bdrv_get_aio_context(bs); + + if (!g_slist_find(aio_ctxs, aio_context)) { + aio_ctxs = g_slist_prepend(aio_ctxs, aio_context); + aio_context_acquire(aio_context); + } } /* We do two passes of inactivation. The first pass calls to drivers' @@ -4340,9 +4346,11 @@ int bdrv_inactivate_all(void) } out: - for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) { - aio_context_release(bdrv_get_aio_context(bs)); + for (ctx = aio_ctxs; ctx != NULL; ctx = ctx->next) { + AioContext *aio_context = ctx->data; + aio_context_release(aio_context); } + g_slist_free(aio_ctxs); return ret; } -- cgit v1.1 From d02d8dde0c0fe3d77a31046e104a55f2e6c50bf6 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 7 Dec 2017 20:13:16 +0000 Subject: docs: mark nested AioContext locking as a legacy API See the patch for why nested AioContext locking is no longer allowed. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171207201320.19284-3-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- docs/devel/multiple-iothreads.txt | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/devel/multiple-iothreads.txt b/docs/devel/multiple-iothreads.txt index e4d340b..4f9012d 100644 --- a/docs/devel/multiple-iothreads.txt +++ b/docs/devel/multiple-iothreads.txt @@ -1,4 +1,4 @@ -Copyright (c) 2014 Red Hat Inc. +Copyright (c) 2014-2017 Red Hat Inc. This work is licensed under the terms of the GNU GPL, version 2 or later. See the COPYING file in the top-level directory. @@ -92,8 +92,9 @@ aio_context_acquire()/aio_context_release() for mutual exclusion. Once the context is acquired no other thread can access it or run event loop iterations in this AioContext. -aio_context_acquire()/aio_context_release() calls may be nested. This -means you can call them if you're not sure whether #2 applies. +Legacy code sometimes nests aio_context_acquire()/aio_context_release() calls. +Do not use nesting anymore, it is incompatible with the BDRV_POLL_WHILE() macro +used in the block layer and can lead to hangs. There is currently no lock ordering rule if a thread needs to acquire multiple AioContexts simultaneously. Therefore, it is only safe for code holding the -- cgit v1.1 From 882e9b89af7c1086d97cee11b2437337e756fa00 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 7 Dec 2017 20:13:17 +0000 Subject: blockdev: add x-blockdev-set-iothread force boolean When a node is already associated with a BlockBackend the x-blockdev-set-iothread command refuses to set the IOThread. This is to prevent accidentally changing the IOThread when the nodes are in use. When the nodes are created with -drive they automatically get a BlockBackend. In that case we know nothing is using them yet and it's safe to set the IOThread. Add a force boolean to override the check. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171207201320.19284-4-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- blockdev.c | 11 ++++++----- qapi/block-core.json | 6 +++++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/blockdev.c b/blockdev.c index f75c01f..9c3a430 100644 --- a/blockdev.c +++ b/blockdev.c @@ -4131,7 +4131,7 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp) } void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, - Error **errp) + bool has_force, bool force, Error **errp) { AioContext *old_context; AioContext *new_context; @@ -4143,10 +4143,11 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread, return; } - /* If we want to allow more extreme test scenarios this guard could be - * removed. For now it protects against accidents. */ - if (bdrv_has_blk(bs)) { - error_setg(errp, "Node %s is in use", node_name); + /* Protects against accidents. */ + if (!(has_force && force) && bdrv_has_blk(bs)) { + error_setg(errp, "Node %s is associated with a BlockBackend and could " + "be in use (use force=true to override this check)", + node_name); return; } diff --git a/qapi/block-core.json b/qapi/block-core.json index 741d6c4..a8cdbc3 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -3962,6 +3962,9 @@ # # @iothread: the name of the IOThread object or null for the main loop # +# @force: true if the node and its children should be moved when a BlockBackend +# is already attached +# # Note: this command is experimental and intended for test cases that need # control over IOThreads only. # @@ -3984,4 +3987,5 @@ ## { 'command': 'x-blockdev-set-iothread', 'data' : { 'node-name': 'str', - 'iothread': 'StrOrNull' } } + 'iothread': 'StrOrNull', + '*force': 'bool' } } -- cgit v1.1 From ccc15f7dafff63cbcc3f6a7c51e380025d67f7f4 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 7 Dec 2017 20:13:18 +0000 Subject: iotests: add VM.add_object() The VM.add_object() method can be used to add IOThreads or memory backend objects. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171207201320.19284-5-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/iotests.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 6f05790..44477e9 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -197,6 +197,11 @@ class VM(qtest.QEMUQtestMachine): socket_scm_helper=socket_scm_helper) self._num_drives = 0 + def add_object(self, opts): + self._args.append('-object') + self._args.append(opts) + return self + def add_device(self, opts): self._args.append('-device') self._args.append(opts) -- cgit v1.1 From 2362a28ea11c145e1a13ae79342d76dc118a72a6 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 7 Dec 2017 20:13:19 +0000 Subject: iothread: fix iothread_stop() race condition There is a small chance that iothread_stop() hangs as follows: Thread 3 (Thread 0x7f63eba5f700 (LWP 16105)): #0 0x00007f64012c09b6 in ppoll () at /lib64/libc.so.6 #1 0x000055959992eac9 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/bits/poll2.h:77 #2 0x000055959992eac9 in qemu_poll_ns (fds=, nfds=, timeout=) at util/qemu-timer.c:322 #3 0x0000559599930711 in aio_poll (ctx=0x55959bdb83c0, blocking=blocking@entry=true) at util/aio-posix.c:629 #4 0x00005595996806fe in iothread_run (opaque=0x55959bd78400) at iothread.c:59 #5 0x00007f640159f609 in start_thread () at /lib64/libpthread.so.0 #6 0x00007f64012cce6f in clone () at /lib64/libc.so.6 Thread 1 (Thread 0x7f640b45b280 (LWP 16103)): #0 0x00007f64015a0b6d in pthread_join () at /lib64/libpthread.so.0 #1 0x00005595999332ef in qemu_thread_join (thread=) at util/qemu-thread-posix.c:547 #2 0x00005595996808ae in iothread_stop (iothread=) at iothread.c:91 #3 0x000055959968094d in iothread_stop_iter (object=, opaque=) at iothread.c:102 #4 0x0000559599857d97 in do_object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 , opaque=opaque@entry=0x0, recurse=recurse@entry=false) at qom/object.c:852 #5 0x0000559599859477 in object_child_foreach (obj=obj@entry=0x55959bdb8100, fn=fn@entry=0x559599680930 , opaque=opaque@entry=0x0) at qom/object.c:867 #6 0x0000559599680a6e in iothread_stop_all () at iothread.c:341 #7 0x000055959955b1d5 in main (argc=, argv=, envp=) at vl.c:4913 The relevant code from iothread_run() is: while (!atomic_read(&iothread->stopping)) { aio_poll(iothread->ctx, true); and iothread_stop(): iothread->stopping = true; aio_notify(iothread->ctx); ... qemu_thread_join(&iothread->thread); The following scenario can occur: 1. IOThread: while (!atomic_read(&iothread->stopping)) -> stopping=false 2. Main loop: iothread->stopping = true; aio_notify(iothread->ctx); 3. IOThread: aio_poll(iothread->ctx, true); -> hang The bug is explained by the AioContext->notify_me doc comments: "If this field is 0, everything (file descriptors, bottom halves, timers) will be re-evaluated before the next blocking poll(), thus the event_notifier_set call can be skipped." The problem is that "everything" does not include checking iothread->stopping. This means iothread_run() will block in aio_poll() if aio_notify() was called just before aio_poll(). This patch fixes the hang by replacing aio_notify() with aio_bh_schedule_oneshot(). This makes aio_poll() or g_main_loop_run() to return. Implementing this properly required a new bool running flag. The new flag prevents races that are tricky if we try to use iothread->stopping. Now iothread->stopping is purely for iothread_stop() and iothread->running is purely for the iothread_run() thread. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171207201320.19284-6-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- include/sysemu/iothread.h | 3 ++- iothread.c | 20 +++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/include/sysemu/iothread.h b/include/sysemu/iothread.h index 55de171..799614f 100644 --- a/include/sysemu/iothread.h +++ b/include/sysemu/iothread.h @@ -29,7 +29,8 @@ typedef struct { GOnce once; QemuMutex init_done_lock; QemuCond init_done_cond; /* is thread initialization done? */ - bool stopping; + bool stopping; /* has iothread_stop() been called? */ + bool running; /* should iothread_run() continue? */ int thread_id; /* AioContext poll parameters */ diff --git a/iothread.c b/iothread.c index e7b93e0..d8b6c1f 100644 --- a/iothread.c +++ b/iothread.c @@ -55,7 +55,7 @@ static void *iothread_run(void *opaque) qemu_cond_signal(&iothread->init_done_cond); qemu_mutex_unlock(&iothread->init_done_lock); - while (!atomic_read(&iothread->stopping)) { + while (iothread->running) { aio_poll(iothread->ctx, true); if (atomic_read(&iothread->worker_context)) { @@ -78,16 +78,25 @@ static void *iothread_run(void *opaque) return NULL; } +/* Runs in iothread_run() thread */ +static void iothread_stop_bh(void *opaque) +{ + IOThread *iothread = opaque; + + iothread->running = false; /* stop iothread_run() */ + + if (iothread->main_loop) { + g_main_loop_quit(iothread->main_loop); + } +} + void iothread_stop(IOThread *iothread) { if (!iothread->ctx || iothread->stopping) { return; } iothread->stopping = true; - aio_notify(iothread->ctx); - if (atomic_read(&iothread->main_loop)) { - g_main_loop_quit(iothread->main_loop); - } + aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread); qemu_thread_join(&iothread->thread); } @@ -134,6 +143,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) char *name, *thread_name; iothread->stopping = false; + iothread->running = true; iothread->thread_id = -1; iothread->ctx = aio_context_new(&local_error); if (!iothread->ctx) { -- cgit v1.1 From 7a9dda0d7f9831c2432620dcfefdadbb7ae888dc Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 7 Dec 2017 20:13:20 +0000 Subject: qemu-iotests: add 203 savevm with IOThreads test This test case will prevent future regressions with savevm and IOThreads. Signed-off-by: Stefan Hajnoczi Reviewed-by: Eric Blake Message-id: 20171207201320.19284-7-stefanha@redhat.com Signed-off-by: Stefan Hajnoczi --- tests/qemu-iotests/203 | 59 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/203.out | 6 +++++ tests/qemu-iotests/group | 1 + 3 files changed, 66 insertions(+) create mode 100755 tests/qemu-iotests/203 create mode 100644 tests/qemu-iotests/203.out diff --git a/tests/qemu-iotests/203 b/tests/qemu-iotests/203 new file mode 100755 index 0000000..2c81191 --- /dev/null +++ b/tests/qemu-iotests/203 @@ -0,0 +1,59 @@ +#!/usr/bin/env python +# +# Copyright (C) 2017 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# +# Creator/Owner: Stefan Hajnoczi +# +# Check that QMP 'migrate' with multiple drives on a single IOThread completes +# successfully. This particular command triggered a hang in the source QEMU +# process due to recursive AioContext locking in bdrv_invalidate_all() and +# BDRV_POLL_WHILE(). + +import iotests + +iotests.verify_image_format(supported_fmts=['qcow2']) +iotests.verify_platform(['linux']) + +with iotests.FilePath('disk0.img') as disk0_img_path, \ + iotests.FilePath('disk1.img') as disk1_img_path, \ + iotests.VM() as vm: + + img_size = '10M' + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk0_img_path, img_size) + iotests.qemu_img_pipe('create', '-f', iotests.imgfmt, disk1_img_path, img_size) + + iotests.log('Launching VM...') + (vm.add_object('iothread,id=iothread0') + .add_drive(disk0_img_path, 'node-name=drive0-node', interface='none') + .add_drive(disk1_img_path, 'node-name=drive1-node', interface='none') + .launch()) + + iotests.log('Setting IOThreads...') + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='drive0-node', iothread='iothread0', + force=True)) + iotests.log(vm.qmp('x-blockdev-set-iothread', + node_name='drive1-node', iothread='iothread0', + force=True)) + + iotests.log('Starting migration...') + iotests.log(vm.qmp('migrate', uri='exec:cat >/dev/null')) + while True: + vm.get_qmp_event(wait=60.0) + result = vm.qmp('query-migrate') + status = result.get('return', {}).get('status', None) + if status == 'completed': + break diff --git a/tests/qemu-iotests/203.out b/tests/qemu-iotests/203.out new file mode 100644 index 0000000..3f1ff90 --- /dev/null +++ b/tests/qemu-iotests/203.out @@ -0,0 +1,6 @@ +Launching VM... +Setting IOThreads... +{u'return': {}} +{u'return': {}} +Starting migration... +{u'return': {}} diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index d0ee1e2..93d96fb 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -198,3 +198,4 @@ 198 rw auto 200 rw auto 202 rw auto quick +203 rw auto -- cgit v1.1