[Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Alberto Garcia]

This series acquires the AioContext in the _realize() functions of
several devices before making use of their block backends. This fixes
at least a couple of crashes (in virtio-blk and scsi). The other
devices don't currently support iothreads so there's no crashes.

Berto

v2:
- Patch 1: include a test case [Kevin]
- Patch 2: include a test case and extend the locking to protect more
  function calls [Kevin]

v1: https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00259.html
- Initial version

Output of backport-diff against v1:

Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/6:[0095] [FC] 'block: Acquire the AioContext in virtio_blk_device_realize()'
002/6:[0094] [FC] 'block: Acquire the AioContext in scsi_*_realize()'
003/6:[----] [--] 'block: Acquire the AioContext in floppy_drive_realize()'
004/6:[----] [--] 'block: Acquire the AioContext in nvme_realize()'
005/6:[----] [--] 'block: Acquire the AioContext in ide_dev_initfn()'
006/6:[----] [--] 'block: Acquire the AioContext in usb_msd_storage_realize()'

Alberto Garcia (6):
  block: Acquire the AioContext in virtio_blk_device_realize()
  block: Acquire the AioContext in scsi_*_realize()
  block: Acquire the AioContext in floppy_drive_realize()
  block: Acquire the AioContext in nvme_realize()
  block: Acquire the AioContext in ide_dev_initfn()
  block: Acquire the AioContext in usb_msd_storage_realize()

 hw/block/fdc.c             |  15 ++++--
 hw/block/nvme.c            |  13 +++--
 hw/block/virtio-blk.c      |  22 ++++++---
 hw/ide/qdev.c              |  17 +++++--
 hw/scsi/scsi-disk.c        |  23 +++++++--
 hw/usb/dev-storage.c       |   5 ++
 tests/qemu-iotests/236     | 121 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/236.out |  46 +++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 9 files changed, 241 insertions(+), 22 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/6] block: Acquire the AioContext in virtio_blk_device_realize()

[Alberto Garcia]

This fixes a crash when adding a virtio-blk device with a drive that
is using an iothread. Test case included.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/block/virtio-blk.c      | 22 ++++++++-----
 tests/qemu-iotests/236     | 78 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/236.out | 16 ++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 110 insertions(+), 7 deletions(-)
 create mode 100755 tests/qemu-iotests/236
 create mode 100644 tests/qemu-iotests/236.out

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f208c6ddb9..5357da82af 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -912,6 +912,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
     VirtIOBlkConf *conf = &s->conf;
+    AioContext *ctx;
     Error *err = NULL;
     unsigned i;
 
@@ -919,30 +920,34 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         error_setg(errp, "drive property not set");
         return;
     }
+
+    ctx = blk_get_aio_context(conf->conf.blk);
+    aio_context_acquire(ctx);
+
     if (!blk_is_inserted(conf->conf.blk)) {
         error_setg(errp, "Device needs media, but drive is empty");
-        return;
+        goto out;
     }
     if (!conf->num_queues) {
         error_setg(errp, "num-queues property must be larger than 0");
-        return;
+        goto out;
     }
     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;
+        goto out;
     }
 
     if (!blkconf_apply_backend_options(&conf->conf,
                                        blk_is_read_only(conf->conf.blk), true,
                                        errp)) {
-        return;
+        goto out;
     }
     s->original_wce = blk_enable_write_cache(conf->conf.blk);
     if (!blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp)) {
-        return;
+        goto out;
     }
 
     blkconf_blocksizes(&conf->conf);
@@ -951,7 +956,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         conf->conf.physical_block_size) {
         error_setg(errp,
                    "logical_block_size > physical_block_size not supported");
-        return;
+        goto out;
     }
 
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
@@ -968,7 +973,7 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     if (err != NULL) {
         error_propagate(errp, err);
         virtio_cleanup(vdev);
-        return;
+        goto out;
     }
 
     s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
@@ -976,6 +981,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     blk_set_guest_block_size(s->blk, s->conf.conf.logical_block_size);
 
     blk_iostatus_enable(s->blk);
+
+out:
+    aio_context_release(ctx);
 }
 
 static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
new file mode 100755
index 0000000000..c6a3415ffe
--- /dev/null
+++ b/tests/qemu-iotests/236
@@ -0,0 +1,78 @@
+#!/bin/bash
+#
+# Test deletion of devices that are using iothreads
+#
+# Copyright (C) 2019 Igalia, S.L.
+# Author: Alberto Garcia <berto@igalia.com>
+#
+# 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 <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=berto@igalia.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt generic
+_supported_proto generic
+_supported_os Linux
+
+do_run_qemu()
+{
+    echo Testing: "$@"
+    $QEMU -nographic -qmp stdio -serial none "$@"
+    echo
+}
+
+# Remove QMP events from (pretty-printed) output. Doesn't handle
+# nested dicts correctly, but we don't get any of those in this test.
+_filter_qmp_events()
+{
+    tr '\n' '\t' | sed -e \
+	's/{\s*"timestamp":\s*{[^}]*},\s*"event":[^,}]*\(,\s*"data":\s*{[^}]*}\)\?\s*}\s*//g' \
+	| tr '\t' '\n'
+}
+
+run_qemu()
+{
+    do_run_qemu "$@" 2>&1 | _filter_qmp | _filter_qmp_events
+}
+
+echo
+echo === Try adding and removing a virtio-blk device ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0"}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
+{ "execute": "x-blockdev-set-iothread", "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "virtio-blk0", "driver": "virtio-blk", "drive": "hd0"}}
+{ "execute": "device_del", "arguments": {"id": "virtio-blk0"}}
+{ "execute": "system_reset"}
+{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
+{ "execute": "quit"}}
+EOF
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
new file mode 100644
index 0000000000..01ee7b0b0d
--- /dev/null
+++ b/tests/qemu-iotests/236.out
@@ -0,0 +1,16 @@
+QA output created by 236
+
+=== Try adding and removing a virtio-blk device ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 61a6d98ebd..f6b245917a 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -233,3 +233,4 @@
 233 auto quick
 234 auto quick migration
 235 auto quick
+236 auto quick
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/6] block: Acquire the AioContext in scsi_*_realize()

[Alberto Garcia]

This fixes a crash when adding a virtio-scsi device with a drive that
is using an iothread. Test case included.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/scsi-disk.c        | 23 ++++++++++++++++++++---
 tests/qemu-iotests/236     | 43 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/236.out | 30 ++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 0e9027c8f3..b049026219 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2381,10 +2381,13 @@ static void scsi_realize(SCSIDevice *dev, Error **errp)
 static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    AioContext *ctx = NULL;
     /* can happen for devices without drive. The error message for missing
      * backend will be issued in scsi_realize
      */
     if (s->qdev.conf.blk) {
+        ctx = blk_get_aio_context(s->qdev.conf.blk);
+        aio_context_acquire(ctx);
         blkconf_blocksizes(&s->qdev.conf);
     }
     s->qdev.blocksize = s->qdev.conf.logical_block_size;
@@ -2393,11 +2396,15 @@ static void scsi_hd_realize(SCSIDevice *dev, Error **errp)
         s->product = g_strdup("QEMU HARDDISK");
     }
     scsi_realize(&s->qdev, errp);
+    if (ctx) {
+        aio_context_release(ctx);
+    }
 }
 
 static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    AioContext *ctx;
     int ret;
 
     if (!dev->conf.blk) {
@@ -2408,6 +2415,8 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
         assert(ret == 0);
     }
 
+    ctx = blk_get_aio_context(dev->conf.blk);
+    aio_context_acquire(ctx);
     s->qdev.blocksize = 2048;
     s->qdev.type = TYPE_ROM;
     s->features |= 1 << SCSI_DISK_F_REMOVABLE;
@@ -2415,6 +2424,7 @@ static void scsi_cd_realize(SCSIDevice *dev, Error **errp)
         s->product = g_strdup("QEMU CD-ROM");
     }
     scsi_realize(&s->qdev, errp);
+    aio_context_release(ctx);
 }
 
 static void scsi_disk_realize(SCSIDevice *dev, Error **errp)
@@ -2553,6 +2563,7 @@ static int get_device_type(SCSIDiskState *s)
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
+    AioContext *ctx;
     int sg_version;
     int rc;
 
@@ -2567,6 +2578,9 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
                           "be removed in a future version");
     }
 
+    ctx = blk_get_aio_context(s->qdev.conf.blk);
+    aio_context_acquire(ctx);
+
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
@@ -2574,18 +2588,18 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
         if (rc != -EPERM) {
             error_append_hint(errp, "Is this a SCSI device?\n");
         }
-        return;
+        goto out;
     }
     if (sg_version < 30000) {
         error_setg(errp, "scsi generic interface too old");
-        return;
+        goto out;
     }
 
     /* get device type from INQUIRY data */
     rc = get_device_type(s);
     if (rc < 0) {
         error_setg(errp, "INQUIRY failed");
-        return;
+        goto out;
     }
 
     /* Make a guess for the block size, we'll fix it when the guest sends.
@@ -2605,6 +2619,9 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 
     scsi_realize(&s->qdev, errp);
     scsi_generic_read_device_inquiry(&s->qdev);
+
+out:
+    aio_context_release(ctx);
 }
 
 typedef struct SCSIBlockReq {
diff --git a/tests/qemu-iotests/236 b/tests/qemu-iotests/236
index c6a3415ffe..c2bf425491 100755
--- a/tests/qemu-iotests/236
+++ b/tests/qemu-iotests/236
@@ -72,6 +72,49 @@ run_qemu <<EOF
 { "execute": "quit"}}
 EOF
 
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+      virtio_scsi=virtio-scsi-ccw
+      ;;
+  *)
+      virtio_scsi=virtio-scsi-pci
+      ;;
+esac
+
+echo
+echo === Try adding and removing a virtio-scsi device with a hard drive ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0"}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
+{ "execute": "x-blockdev-set-iothread", "arguments": {"node-name": "hd0", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi0"}}
+{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
+{ "execute": "quit"}}
+EOF
+
+echo
+echo === Try adding and removing a virtio-scsi device with a CD ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "cd0"}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
+{ "execute": "x-blockdev-set-iothread", "arguments": {"node-name": "cd0", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-cd0", "driver": "scsi-cd", "drive": "cd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-cd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi0"}}
+{ "execute": "blockdev-del", "arguments": {"node-name": "cd0"}}
+{ "execute": "quit"}}
+EOF
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index 01ee7b0b0d..2ad5aabcb3 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -13,4 +13,34 @@ QMP_VERSION
 {"return": {}}
 {"return": {}}
 {"return": {}}
+
+=== Try adding and removing a virtio-scsi device with a hard drive ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+
+=== Try adding and removing a virtio-scsi device with a CD ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/6] block: Acquire the AioContext in floppy_drive_realize()

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/block/fdc.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6f19f127a5..d9bc80de83 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -512,6 +512,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
 {
     FloppyDrive *dev = FLOPPY_DRIVE(qdev);
     FloppyBus *bus = FLOPPY_BUS(qdev->parent_bus);
+    AioContext *ctx;
     FDrive *drive;
     int ret;
 
@@ -543,13 +544,16 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
         assert(ret == 0);
     }
 
+    ctx = blk_get_aio_context(dev->conf.blk);
+    aio_context_acquire(ctx);
+
     blkconf_blocksizes(&dev->conf);
     if (dev->conf.logical_block_size != 512 ||
         dev->conf.physical_block_size != 512)
     {
         error_setg(errp, "Physical and logical block size must "
                    "be 512 for floppy");
-        return;
+        goto out;
     }
 
     /* rerror/werror aren't supported by fdc and therefore not even registered
@@ -561,7 +565,7 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     if (!blkconf_apply_backend_options(&dev->conf,
                                        blk_is_read_only(dev->conf.blk),
                                        false, errp)) {
-        return;
+        goto out;
     }
 
     /* 'enospc' is the default for -drive, 'report' is what blk_new() gives us
@@ -569,11 +573,11 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     if (blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_ENOSPC &&
         blk_get_on_error(dev->conf.blk, 0) != BLOCKDEV_ON_ERROR_REPORT) {
         error_setg(errp, "fdc doesn't support drive option werror");
-        return;
+        goto out;
     }
     if (blk_get_on_error(dev->conf.blk, 1) != BLOCKDEV_ON_ERROR_REPORT) {
         error_setg(errp, "fdc doesn't support drive option rerror");
-        return;
+        goto out;
     }
 
     drive->conf = &dev->conf;
@@ -589,6 +593,9 @@ static void floppy_drive_realize(DeviceState *qdev, Error **errp)
     dev->type = drive->drive;
 
     fd_revalidate(drive);
+
+out:
+    aio_context_release(ctx);
 }
 
 static void floppy_drive_class_init(ObjectClass *klass, void *data)
-- 
2.11.0

[Qemu-devel] [PATCH v2 4/6] block: Acquire the AioContext in nvme_realize()

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/block/nvme.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 7c8c63e8f5..72e94aff86 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -1203,6 +1203,7 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
 {
     NvmeCtrl *n = NVME(pci_dev);
     NvmeIdCtrl *id = &n->id_ctrl;
+    AioContext *ctx;
 
     int i;
     int64_t bs_size;
@@ -1213,20 +1214,23 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
         return;
     }
 
+    ctx = blk_get_aio_context(n->conf.blk);
+    aio_context_acquire(ctx);
+
     bs_size = blk_getlength(n->conf.blk);
     if (bs_size < 0) {
         error_setg(errp, "could not get backing file size");
-        return;
+        goto out;
     }
 
     if (!n->serial) {
         error_setg(errp, "serial property not set");
-        return;
+        goto out;
     }
     blkconf_blocksizes(&n->conf);
     if (!blkconf_apply_backend_options(&n->conf, blk_is_read_only(n->conf.blk),
                                        false, errp)) {
-        return;
+        goto out;
     }
 
     pci_conf = pci_dev->config;
@@ -1323,6 +1327,9 @@ static void nvme_realize(PCIDevice *pci_dev, Error **errp)
             cpu_to_le64(n->ns_size >>
                 id_ns->lbaf[NVME_ID_NS_FLBAS_INDEX(ns->id_ns.flbas)].ds);
     }
+
+out:
+    aio_context_release(ctx);
 }
 
 static void nvme_exit(PCIDevice *pci_dev)
-- 
2.11.0

[Qemu-devel] [PATCH v2 5/6] block: Acquire the AioContext in ide_dev_initfn()

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/ide/qdev.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/ide/qdev.c b/hw/ide/qdev.c
index 573b022e1e..f355f2a352 100644
--- a/hw/ide/qdev.c
+++ b/hw/ide/qdev.c
@@ -160,6 +160,7 @@ 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;
+    AioContext *ctx;
     int ret;
 
     if (!dev->conf.blk) {
@@ -174,36 +175,39 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
         }
     }
 
+    ctx = blk_get_aio_context(dev->conf.blk);
+    aio_context_acquire(ctx);
+
     if (dev->conf.discard_granularity == -1) {
         dev->conf.discard_granularity = 512;
     } else if (dev->conf.discard_granularity &&
                dev->conf.discard_granularity != 512) {
         error_setg(errp, "discard_granularity must be 512 for ide");
-        return;
+        goto out;
     }
 
     blkconf_blocksizes(&dev->conf);
     if (dev->conf.logical_block_size != 512) {
         error_setg(errp, "logical_block_size must be 512 for IDE");
-        return;
+        goto out;
     }
 
     if (kind != IDE_CD) {
         if (!blkconf_geometry(&dev->conf, &dev->chs_trans, 65535, 16, 255,
                               errp)) {
-            return;
+            goto out;
         }
     }
     if (!blkconf_apply_backend_options(&dev->conf, kind == IDE_CD,
                                        kind != IDE_CD, errp)) {
-        return;
+        goto out;
     }
 
     if (ide_init_drive(s, dev->conf.blk, kind,
                        dev->version, dev->serial, dev->model, dev->wwn,
                        dev->conf.cyls, dev->conf.heads, dev->conf.secs,
                        dev->chs_trans, errp) < 0) {
-        return;
+        goto out;
     }
 
     if (!dev->version) {
@@ -215,6 +219,9 @@ static void ide_dev_initfn(IDEDevice *dev, IDEDriveKind kind, Error **errp)
 
     add_boot_device_path(dev->conf.bootindex, &dev->qdev,
                          dev->unit ? "/disk@1" : "/disk@0");
+
+out:
+    aio_context_release(ctx);
 }
 
 static void ide_dev_get_bootindex(Object *obj, Visitor *v, const char *name,
-- 
2.11.0

[Qemu-devel] [PATCH v2 6/6] block: Acquire the AioContext in usb_msd_storage_realize()

[Alberto Garcia]

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/usb/dev-storage.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index cd5551d94f..fa6e552f0f 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -593,13 +593,18 @@ static void usb_msd_storage_realize(USBDevice *dev, Error **errp)
     MSDState *s = USB_STORAGE_DEV(dev);
     BlockBackend *blk = s->conf.blk;
     SCSIDevice *scsi_dev;
+    AioContext *ctx;
 
     if (!blk) {
         error_setg(errp, "drive property not set");
         return;
     }
 
+    ctx = blk_get_aio_context(blk);
+    aio_context_acquire(ctx);
     blkconf_blocksizes(&s->conf);
+    aio_context_release(ctx);
+
     if (!blkconf_apply_backend_options(&s->conf, blk_is_read_only(blk), true,
                                        errp)) {
         return;
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]

On Mon, Jan 14, 2019 at 04:23:58PM +0200, Alberto Garcia wrote:
> This series acquires the AioContext in the _realize() functions of
> several devices before making use of their block backends. This fixes
> at least a couple of crashes (in virtio-blk and scsi). The other
> devices don't currently support iothreads so there's no crashes.
> 
> Berto

My assumption has been that drives are in the main loop AioContext until
the device model (e.g. virtio-blk) decides to move them into the
IOThread configured by the user.

(And when the VM is stopped or the device is removed, the drive is moved
back into the main loop AioContext by the device.)

The x-blockdev-set-iothread command is for low-level tests so I don't
expect users to invoke it.  So I'm not sure which real-world scenario is
being tested here?

This patch series makes virtio-blk and virtio-scsi more robust, although
I haven't checked what happens if the drive is attached to a different
IOThread than the device - will the switchover work?

What I'm unclear about is why fdc, ide, usb-msd, and nvme should worry
about IOThreads/AioContext in .realize() since they don't support it in
their data path.  What happens when you submit I/O requests to devices
configured like this?  I guess they would crash or hit assertion
failures since they invoke blk_aio_*() APIs from outside the appropriate
AioContext.

Maybe fdc and friends should forbid this scenario:

  /* Fail if @blk is attached to an IOThread */
  static bool blk_check_main_aio_context(BlockBackend *blk, Error **errp) {
      if (blk_get_aio_context(blk) != qemu_get_aio_context()) {
          error_setg(errp, "Device does not support iothreads");
          return false;
      }
      return true;
  }

  ...in a realize() function...
  if (!blk_check_main_aio_context(blk, errp)) {
      return;
  }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Alberto Garcia]

On Wed 16 Jan 2019 02:54:44 PM CET, Stefan Hajnoczi wrote:
> The x-blockdev-set-iothread command is for low-level tests so I don't
> expect users to invoke it.

As I said in a different e-mail maybe this is not necessary then.

> This patch series makes virtio-blk and virtio-scsi more robust,
> although I haven't checked what happens if the drive is attached to a
> different IOThread than the device - will the switchover work?

You mean if you use x-blockdev-set-iothread with a drive and then attach
it to a device using a different thread? That seems to work (with my
patch).

> What I'm unclear about is why fdc, ide, usb-msd, and nvme should worry
> about IOThreads/AioContext in .realize() since they don't support it
> in their data path.  What happens when you submit I/O requests to
> devices configured like this?  I guess they would crash or hit
> assertion failures since they invoke blk_aio_*() APIs from outside the
> appropriate AioContext.
>
> Maybe fdc and friends should forbid this scenario:

Yeah, that seems like a simple solution.

Berto

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On Thu, Jan 17, 2019 at 02:24:10PM +0100, Alberto Garcia wrote:
> On Wed 16 Jan 2019 02:54:44 PM CET, Stefan Hajnoczi wrote:
> > This patch series makes virtio-blk and virtio-scsi more robust,
> > although I haven't checked what happens if the drive is attached to a
> > different IOThread than the device - will the switchover work?
> 
> You mean if you use x-blockdev-set-iothread with a drive and then attach
> it to a device using a different thread? That seems to work (with my
> patch).

Does it work with a running VM that will do I/O?  If yes, then that's
great and we should merge the virtio-blk/scsi patches to make them
robust for this case.  I'm a little surprised it works though :-)
because the code wasn't designed or tested for this.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Kevin Wolf]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

Am 18.01.2019 um 10:56 hat Stefan Hajnoczi geschrieben:
> On Thu, Jan 17, 2019 at 02:24:10PM +0100, Alberto Garcia wrote:
> > On Wed 16 Jan 2019 02:54:44 PM CET, Stefan Hajnoczi wrote:
> > > This patch series makes virtio-blk and virtio-scsi more robust,
> > > although I haven't checked what happens if the drive is attached to a
> > > different IOThread than the device - will the switchover work?
> > 
> > You mean if you use x-blockdev-set-iothread with a drive and then attach
> > it to a device using a different thread? That seems to work (with my
> > patch).
> 
> Does it work with a running VM that will do I/O?  If yes, then that's
> great and we should merge the virtio-blk/scsi patches to make them
> robust for this case.  I'm a little surprised it works though :-)
> because the code wasn't designed or tested for this.

There are two ways to trigger the crash even without
x-blockdev-set-iothread:

* device_del, then device_add for a device with iothread (virtio-scsi;
  may or may not exist with virtio-blk)
  https://bugzilla.redhat.com/show_bug.cgi?id=1656276

* Simply attach two devices with iothread to the the same node
  https://bugzilla.redhat.com/show_bug.cgi?id=1662508

Maybe this series is actually papering over the real problems, though.
Unplug should move the BDS back to the main AioContext, and attaching
two users with different AioContexts must fail. If we don't take care
there, the locking will be wrong.

Note that even though there are more problems, we still need this series
so we can allow attaching two devices to the same node while using the
same iothread.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Kevin Wolf]

Am 14.01.2019 um 15:23 hat Alberto Garcia geschrieben:
> This series acquires the AioContext in the _realize() functions of
> several devices before making use of their block backends. This fixes
> at least a couple of crashes (in virtio-blk and scsi). The other
> devices don't currently support iothreads so there's no crashes.

>  tests/qemu-iotests/236     | 121 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/236.out |  46 +++++++++++++++++

236 is already taken by two other series. :-)

I'll use 239 and 239.out for the conflict resolution, so I think the
next free one for you is 240.

Kevin

Re: [Qemu-devel] [PATCH v2 0/6] Acquire the AioContext during _realize()

[Alberto Garcia]

On Fri 18 Jan 2019 11:14:15 AM CET, Kevin Wolf wrote:
> There are two ways to trigger the crash even without
> x-blockdev-set-iothread:
>
> * device_del, then device_add for a device with iothread (virtio-scsi;
>   may or may not exist with virtio-blk)
>   https://bugzilla.redhat.com/show_bug.cgi?id=1656276
>
> * Simply attach two devices with iothread to the the same node
>   https://bugzilla.redhat.com/show_bug.cgi?id=1662508

While having a look at this I found another crash. Here's how to
reproduce it (wait for the events after each system_reset):

   { "execute": "qmp_capabilities" }
   { "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0"}}
   { "execute": "device_add", "arguments": {"id": "vb0", "driver": "virtio-blk", "drive": "hd0"}}
   { "execute": "system_reset"}

   { "execute": "device_del", "arguments": {"id": "vb0"}}
   { "execute": "system_reset"}

   { "execute": "device_add", "arguments": {"id": "vb0", "driver": "virtio-blk", "drive": "hd0"}}
   { "execute": "system_reset"}

   { "execute": "device_del", "arguments": {"id": "vb0"}}
   { "execute": "system_reset"}

   { "execute": "device_add", "arguments": {"id": "vb0", "driver": "virtio-blk", "drive": "hd0"}}
   { "execute": "system_reset"}

kvm_mem_ioeventfd_add: error adding ioeventfd: No space left on device
Aborted

git-bisect points to this commit:

   commit 3ac7d43a6fbb5d4a3d01fc9a055c218030af3727
   Author: Paolo Bonzini <pbonzini@redhat.com>
   Date:   Wed Nov 28 17:28:45 2018 +0100

       memory: update coalesced_range on transaction_commit

       The e1000 driver calls memory_region_add_coalescing but
       kvm_coalesce_mmio_region is never called for those regions.  The bug
       dates back to the introduction of the memory region API; to fix it,
       delete and re-add coalesced MMIO ranges when building the FlatViews.
       
       Because coalesced MMIO regions apply to all address spaces, the
       has_coalesced_range flag has to be changed into an int.

Berto