[Qemu-devel] [PATCH 0/3] iothread-related fixes for virtio-scsi

[Qemu-devel] [PATCH 0/3] iothread-related fixes for virtio-scsi

[Alberto Garcia]

Hi,

here are three patches with iothread-related fixes for virtio-scsi,
with their test cases.

This series fixes the following bugs:

   https://bugzilla.redhat.com/show_bug.cgi?id=1656276

   https://bugzilla.redhat.com/show_bug.cgi?id=1662508

I also wanted to do prepare a similar one for virtio-blk but I'm
unsure about how to proceed, because I'm unable to detect during
realize() that the blockdev is using a different iothread. This
crashes QEMU:

{ "execute": "qmp_capabilities" }
{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}}
{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}}
{ "execute": "device_add", "arguments": {"id": "virtio-blk0", "driver": "virtio-blk", "iothread": "iothread0", "drive": "hd0"}}
{ "execute": "system_reset"}
  (wait for the events)
{ "execute": "device_add", "arguments": {"id": "virtio-blk1", "driver": "virtio-blk", "iothread": "iothread1", "drive": "hd0"}}

Regards,

Berto

Alberto Garcia (3):
  virtio-scsi: Move BlockBackend back to the main AioContext on unplug
  scsi-disk: Acquire the AioContext in scsi_*_realize()
  virtio-scsi: Forbid devices with different iothreads sharing a
    blockdev

 hw/scsi/scsi-disk.c        |  23 ++++++--
 hw/scsi/virtio-scsi.c      |  13 +++++
 tests/qemu-iotests/240     | 129 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/240.out |  54 +++++++++++++++++++
 tests/qemu-iotests/group   |   1 +
 5 files changed, 217 insertions(+), 3 deletions(-)
 create mode 100755 tests/qemu-iotests/240
 create mode 100644 tests/qemu-iotests/240.out

-- 
2.11.0

[Qemu-devel] [PATCH 1/3] virtio-scsi: Move BlockBackend back to the main AioContext on unplug

[Alberto Garcia]

This fixes a crash when attaching a disk to a SCSI device using
iothreads, then detaching it and reattaching it again. Test case
included.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/virtio-scsi.c      |  6 ++++
 tests/qemu-iotests/240     | 89 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/240.out | 18 ++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 114 insertions(+)
 create mode 100755 tests/qemu-iotests/240
 create mode 100644 tests/qemu-iotests/240.out

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 3aa99717e2..e1f7b208c7 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -824,6 +824,12 @@ static void virtio_scsi_hotunplug(HotplugHandler *hotplug_dev, DeviceState *dev,
         virtio_scsi_release(s);
     }
 
+    if (s->ctx) {
+        virtio_scsi_acquire(s);
+        blk_set_aio_context(sd->conf.blk, qemu_get_aio_context());
+        virtio_scsi_release(s);
+    }
+
     qdev_simple_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
new file mode 100755
index 0000000000..ead7ee08eb
--- /dev/null
+++ b/tests/qemu-iotests/240
@@ -0,0 +1,89 @@
+#!/bin/bash
+#
+# Test hot plugging and unplugging with 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
+}
+
+case "$QEMU_DEFAULT_MACHINE" in
+  s390-ccw-virtio)
+      virtio_scsi=virtio-scsi-ccw
+      ;;
+  *)
+      virtio_scsi=virtio-scsi-pci
+      ;;
+esac
+
+echo
+echo === Unplug a SCSI disk and then plug it again ===
+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": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
+{ "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
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out
new file mode 100644
index 0000000000..432d98111c
--- /dev/null
+++ b/tests/qemu-iotests/240.out
@@ -0,0 +1,18 @@
+QA output created by 240
+
+=== Unplug a SCSI disk and then plug it again ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index f6b245917a..4671d7a733 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -234,3 +234,4 @@
 234 auto quick migration
 235 auto quick
 236 auto quick
+240 auto quick
-- 
2.11.0

[Qemu-devel] [PATCH 2/3] scsi-disk: Acquire the AioContext in scsi_*_realize()

[Alberto Garcia]

This fixes a crash when attaching two disks with the same blockdev to
a SCSI device that is using iothreads. Test case included.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/scsi-disk.c        | 23 ++++++++++++++++++++---
 tests/qemu-iotests/240     | 18 ++++++++++++++++++
 tests/qemu-iotests/240.out | 16 ++++++++++++++++
 3 files changed, 54 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/240 b/tests/qemu-iotests/240
index ead7ee08eb..5d499c9a00 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -83,6 +83,24 @@ run_qemu <<EOF
 { "execute": "quit"}
 EOF
 
+echo
+echo === Attach two SCSI disks using the same block device and the same iothread ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}}
+{ "execute": "device_del", "arguments": {"id": "scsi0"}}
+{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
+{ "execute": "quit"}
+EOF
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out
index 432d98111c..701cb5c7d2 100644
--- a/tests/qemu-iotests/240.out
+++ b/tests/qemu-iotests/240.out
@@ -15,4 +15,20 @@ QMP_VERSION
 {"return": {}}
 {"return": {}}
 {"return": {}}
+
+=== Attach two SCSI disks using the same block device and the same iothread ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done
-- 
2.11.0

[Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

This patch forbids attaching a disk to a SCSI device if its using a
different AioContext. Test case included.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 hw/scsi/virtio-scsi.c      |  7 +++++++
 tests/qemu-iotests/240     | 22 ++++++++++++++++++++++
 tests/qemu-iotests/240.out | 20 ++++++++++++++++++++
 3 files changed, 49 insertions(+)

diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index e1f7b208c7..eb90288f47 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -791,9 +791,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
     SCSIDevice *sd = SCSI_DEVICE(dev);
 
     if (s->ctx && !s->dataplane_fenced) {
+        AioContext *ctx;
         if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
             return;
         }
+        ctx = blk_get_aio_context(sd->conf.blk);
+        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
+            error_setg(errp, "Cannot attach a blockdev that is using "
+                       "a different iothread");
+            return;
+        }
         virtio_scsi_acquire(s);
         blk_set_aio_context(sd->conf.blk, s->ctx);
         virtio_scsi_release(s);
diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
index 5d499c9a00..65cc3b39b1 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -101,6 +101,28 @@ run_qemu <<EOF
 { "execute": "quit"}
 EOF
 
+echo
+echo === Attach two SCSI disks using the same block device but different iothreads ===
+echo
+
+run_qemu <<EOF
+{ "execute": "qmp_capabilities" }
+{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
+{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}}
+{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi1", "driver": "${virtio_scsi}", "iothread": "iothread1"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
+{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}}
+{ "execute": "device_del", "arguments": {"id": "scsi0"}}
+{ "execute": "device_del", "arguments": {"id": "scsi1"}}
+{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
+{ "execute": "quit"}
+EOF
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out
index 701cb5c7d2..d76392966c 100644
--- a/tests/qemu-iotests/240.out
+++ b/tests/qemu-iotests/240.out
@@ -31,4 +31,24 @@ QMP_VERSION
 {"return": {}}
 {"return": {}}
 {"return": {}}
+
+=== Attach two SCSI disks using the same block device but different iothreads ===
+
+Testing:
+QMP_VERSION
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Cannot attach a blockdev that is using a different iothread"}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
 *** done
-- 
2.11.0

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Paolo Bonzini]

On 22/01/19 16:53, Alberto Garcia wrote:
> This patch forbids attaching a disk to a SCSI device if its using a
> different AioContext. Test case included.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>

Should this be handled in common code?  However, everything else looks
good.  Thanks!

Paolo

> ---
>  hw/scsi/virtio-scsi.c      |  7 +++++++
>  tests/qemu-iotests/240     | 22 ++++++++++++++++++++++
>  tests/qemu-iotests/240.out | 20 ++++++++++++++++++++
>  3 files changed, 49 insertions(+)
> 
> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
> index e1f7b208c7..eb90288f47 100644
> --- a/hw/scsi/virtio-scsi.c
> +++ b/hw/scsi/virtio-scsi.c
> @@ -791,9 +791,16 @@ static void virtio_scsi_hotplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      SCSIDevice *sd = SCSI_DEVICE(dev);
>  
>      if (s->ctx && !s->dataplane_fenced) {
> +        AioContext *ctx;
>          if (blk_op_is_blocked(sd->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
>              return;
>          }
> +        ctx = blk_get_aio_context(sd->conf.blk);
> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
> +            error_setg(errp, "Cannot attach a blockdev that is using "
> +                       "a different iothread");
> +            return;
> +        }
>          virtio_scsi_acquire(s);
>          blk_set_aio_context(sd->conf.blk, s->ctx);
>          virtio_scsi_release(s);
> diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
> index 5d499c9a00..65cc3b39b1 100755
> --- a/tests/qemu-iotests/240
> +++ b/tests/qemu-iotests/240
> @@ -101,6 +101,28 @@ run_qemu <<EOF
>  { "execute": "quit"}
>  EOF
>  
> +echo
> +echo === Attach two SCSI disks using the same block device but different iothreads ===
> +echo
> +
> +run_qemu <<EOF
> +{ "execute": "qmp_capabilities" }
> +{ "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}}
> +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
> +{ "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}}
> +{ "execute": "device_add", "arguments": {"id": "scsi0", "driver": "${virtio_scsi}", "iothread": "iothread0"}}
> +{ "execute": "device_add", "arguments": {"id": "scsi1", "driver": "${virtio_scsi}", "iothread": "iothread1"}}
> +{ "execute": "device_add", "arguments": {"id": "scsi-hd0", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi0.0"}}
> +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
> +{ "execute": "device_del", "arguments": {"id": "scsi-hd0"}}
> +{ "execute": "device_add", "arguments": {"id": "scsi-hd1", "driver": "scsi-hd", "drive": "hd0", "bus": "scsi1.0"}}
> +{ "execute": "device_del", "arguments": {"id": "scsi-hd1"}}
> +{ "execute": "device_del", "arguments": {"id": "scsi0"}}
> +{ "execute": "device_del", "arguments": {"id": "scsi1"}}
> +{ "execute": "blockdev-del", "arguments": {"node-name": "hd0"}}
> +{ "execute": "quit"}
> +EOF
> +
>  # success, all done
>  echo "*** done"
>  rm -f $seq.full
> diff --git a/tests/qemu-iotests/240.out b/tests/qemu-iotests/240.out
> index 701cb5c7d2..d76392966c 100644
> --- a/tests/qemu-iotests/240.out
> +++ b/tests/qemu-iotests/240.out
> @@ -31,4 +31,24 @@ QMP_VERSION
>  {"return": {}}
>  {"return": {}}
>  {"return": {}}
> +
> +=== Attach two SCSI disks using the same block device but different iothreads ===
> +
> +Testing:
> +QMP_VERSION
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"error": {"class": "GenericError", "desc": "Cannot attach a blockdev that is using a different iothread"}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
> +{"return": {}}
>  *** done
> 

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

On Wed 23 Jan 2019 10:46:54 AM CET, Paolo Bonzini wrote:
> On 22/01/19 16:53, Alberto Garcia wrote:
>> This patch forbids attaching a disk to a SCSI device if its using a
>> different AioContext. Test case included.
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>
> Should this be handled in common code?  However, everything else looks
> good.  Thanks!

You mean a common function with the code below?

>> +        ctx = blk_get_aio_context(sd->conf.blk);
>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
>> +            error_setg(errp, "Cannot attach a blockdev that is using "
>> +                       "a different iothread");
>> +            return;
>> +        }

Who else would be the user?

Berto

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Paolo Bonzini]

On 23/01/19 16:09, Alberto Garcia wrote:
> On Wed 23 Jan 2019 10:46:54 AM CET, Paolo Bonzini wrote:
>> On 22/01/19 16:53, Alberto Garcia wrote:
>>> This patch forbids attaching a disk to a SCSI device if its using a
>>> different AioContext. Test case included.
>>>
>>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>>
>> Should this be handled in common code?  However, everything else looks
>> good.  Thanks!
> 
> You mean a common function with the code below?
> 
>>> +        ctx = blk_get_aio_context(sd->conf.blk);
>>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
>>> +            error_setg(errp, "Cannot attach a blockdev that is using "
>>> +                       "a different iothread");
>>> +            return;
>>> +        }
> 
> Who else would be the user?

If it isn't, probably virtio-blk should be using too.

Paolo

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote:
>> You mean a common function with the code below?
>> 
>>>> +        ctx = blk_get_aio_context(sd->conf.blk);
>>>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
>>>> +            error_setg(errp, "Cannot attach a blockdev that is using "
>>>> +                       "a different iothread");
>>>> +            return;
>>>> +        }
>> 
>> Who else would be the user?
>
> If it isn't, probably virtio-blk should be using too.

I agree, but that's the problem that I mentioned on the cover letter.

In the virtio-blk case I don't know how to tell that the block device is
using a different iothread during device_add, because the iothread is
not set during realize() but by virtio_blk_data_plane_start() that
happens only when I reboot the guest.

Berto

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Kevin Wolf]

Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben:
> On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote:
> >> You mean a common function with the code below?
> >> 
> >>>> +        ctx = blk_get_aio_context(sd->conf.blk);
> >>>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
> >>>> +            error_setg(errp, "Cannot attach a blockdev that is using "
> >>>> +                       "a different iothread");
> >>>> +            return;
> >>>> +        }
> >> 
> >> Who else would be the user?
> >
> > If it isn't, probably virtio-blk should be using too.
> 
> I agree, but that's the problem that I mentioned on the cover letter.
> 
> In the virtio-blk case I don't know how to tell that the block device is
> using a different iothread during device_add, because the iothread is
> not set during realize() but by virtio_blk_data_plane_start() that
> happens only when I reboot the guest.

I think the proper solution on the block layer level would be that
AioContext is managed per BdrvChild and only BdrvChild objects with the
same AioContext can be attached to the same node. bdrv_set_aio_context()
would then fail if another parent is already using the node.

Now, Paolo keeps talking about how AioContexts are going to disappear
soon (TM), so I wonder whether it's still worth fixing this properly
now?

Kevin

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

On Thu 24 Jan 2019 10:12:22 AM CET, Kevin Wolf wrote:
> Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben:
>> On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote:
>> >> You mean a common function with the code below?
>> >> 
>> >>>> +        ctx = blk_get_aio_context(sd->conf.blk);
>> >>>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
>> >>>> +            error_setg(errp, "Cannot attach a blockdev that is using "
>> >>>> +                       "a different iothread");
>> >>>> +            return;
>> >>>> +        }
>> >> 
>> >> Who else would be the user?
>> >
>> > If it isn't, probably virtio-blk should be using too.
>> 
>> I agree, but that's the problem that I mentioned on the cover letter.
>> 
>> In the virtio-blk case I don't know how to tell that the block device
>> is using a different iothread during device_add, because the iothread
>> is not set during realize() but by virtio_blk_data_plane_start() that
>> happens only when I reboot the guest.
>
> I think the proper solution on the block layer level would be that
> AioContext is managed per BdrvChild and only BdrvChild objects with
> the same AioContext can be attached to the same node.
> bdrv_set_aio_context() would then fail if another parent is already
> using the node.

How would that solve the virtio-blk problem, though? :-?

> Now, Paolo keeps talking about how AioContexts are going to disappear
> soon (TM), so I wonder whether it's still worth fixing this properly
> now?

Either way, should we commit this series already and later update it
once we have a proper solution for virtio-blk?

Berto

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Kevin Wolf]

Am 28.01.2019 um 15:42 hat Alberto Garcia geschrieben:
> On Thu 24 Jan 2019 10:12:22 AM CET, Kevin Wolf wrote:
> > Am 23.01.2019 um 17:16 hat Alberto Garcia geschrieben:
> >> On Wed 23 Jan 2019 04:47:30 PM CET, Paolo Bonzini wrote:
> >> >> You mean a common function with the code below?
> >> >> 
> >> >>>> +        ctx = blk_get_aio_context(sd->conf.blk);
> >> >>>> +        if (ctx != s->ctx && ctx != qemu_get_aio_context()) {
> >> >>>> +            error_setg(errp, "Cannot attach a blockdev that is using "
> >> >>>> +                       "a different iothread");
> >> >>>> +            return;
> >> >>>> +        }
> >> >> 
> >> >> Who else would be the user?
> >> >
> >> > If it isn't, probably virtio-blk should be using too.
> >> 
> >> I agree, but that's the problem that I mentioned on the cover letter.
> >> 
> >> In the virtio-blk case I don't know how to tell that the block device
> >> is using a different iothread during device_add, because the iothread
> >> is not set during realize() but by virtio_blk_data_plane_start() that
> >> happens only when I reboot the guest.
> >
> > I think the proper solution on the block layer level would be that
> > AioContext is managed per BdrvChild and only BdrvChild objects with
> > the same AioContext can be attached to the same node.
> > bdrv_set_aio_context() would then fail if another parent is already
> > using the node.
> 
> How would that solve the virtio-blk problem, though? :-?

It wouldn't, but it would make clear that bdrv_set_aio_context() can
fail and we'd have to handle the problem exactly there. So probably we
need to do something to move the data plane initialisation into realize?

The only other option I see is downgrading to non-dataplane mode, but
this would have to be silent because during machine reset we have no way
to report errors.

> > Now, Paolo keeps talking about how AioContexts are going to disappear
> > soon (TM), so I wonder whether it's still worth fixing this properly
> > now?
> 
> Either way, should we commit this series already and later update it
> once we have a proper solution for virtio-blk?

Fine with me.

Kevin

Re: [Qemu-devel] [PATCH 0/3] iothread-related fixes for virtio-scsi

[Kevin Wolf]

Am 22.01.2019 um 16:53 hat Alberto Garcia geschrieben:
> Hi,
> 
> here are three patches with iothread-related fixes for virtio-scsi,
> with their test cases.
> 
> This series fixes the following bugs:
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=1656276
> 
>    https://bugzilla.redhat.com/show_bug.cgi?id=1662508
> 
> I also wanted to do prepare a similar one for virtio-blk but I'm
> unsure about how to proceed, because I'm unable to detect during
> realize() that the blockdev is using a different iothread. This
> crashes QEMU:
> 
> { "execute": "qmp_capabilities" }
> { "execute": "blockdev-add", "arguments": {"driver": "null-co", "node-name": "hd0", "read-only": true}}
> { "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread0"}}
> { "execute": "object-add", "arguments": {"qom-type": "iothread", "id": "iothread1"}}
> { "execute": "device_add", "arguments": {"id": "virtio-blk0", "driver": "virtio-blk", "iothread": "iothread0", "drive": "hd0"}}
> { "execute": "system_reset"}
>   (wait for the events)
> { "execute": "device_add", "arguments": {"id": "virtio-blk1", "driver": "virtio-blk", "iothread": "iothread1", "drive": "hd0"}}

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

On Mon 28 Jan 2019 03:57:43 PM CET, Kevin Wolf wrote:

>> > I think the proper solution on the block layer level would be that
>> > AioContext is managed per BdrvChild and only BdrvChild objects with
>> > the same AioContext can be attached to the same node.
>> > bdrv_set_aio_context() would then fail if another parent is already
>> > using the node.
>> 
>> How would that solve the virtio-blk problem, though? :-?
>
> It wouldn't, but it would make clear that bdrv_set_aio_context() can
> fail and we'd have to handle the problem exactly there.

Yes, that sound like the proper solution.

> The only other option I see is downgrading to non-dataplane mode, but
> this would have to be silent because during machine reset we have no
> way to report errors.

Yes, I actually had a patch doing that, but as you said the user has no
way of knowing that it went wrong.

If you have 2 or 3 virtio-blk devices with different iothreads using the
same BDS then after the guest reboot only one would succeed, but you
can't see which one, or can you?

Berto

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Kevin Wolf]

Am 28.01.2019 um 16:46 hat Alberto Garcia geschrieben:
> On Mon 28 Jan 2019 03:57:43 PM CET, Kevin Wolf wrote:
> 
> >> > I think the proper solution on the block layer level would be that
> >> > AioContext is managed per BdrvChild and only BdrvChild objects with
> >> > the same AioContext can be attached to the same node.
> >> > bdrv_set_aio_context() would then fail if another parent is already
> >> > using the node.
> >> 
> >> How would that solve the virtio-blk problem, though? :-?
> >
> > It wouldn't, but it would make clear that bdrv_set_aio_context() can
> > fail and we'd have to handle the problem exactly there.
> 
> Yes, that sound like the proper solution.
> 
> > The only other option I see is downgrading to non-dataplane mode, but
> > this would have to be silent because during machine reset we have no
> > way to report errors.
> 
> Yes, I actually had a patch doing that, but as you said the user has no
> way of knowing that it went wrong.
> 
> If you have 2 or 3 virtio-blk devices with different iothreads using the
> same BDS then after the guest reboot only one would succeed, but you
> can't see which one, or can you?

I suppose you could add a QOM property that tells whether the assigned
iothread is actually in use, but it would still be quite obscure. And
you would have to check explicitly, so I don't think that's a good
solution.

And actually... if you move the first virtio-blk device to an iothread,
then downgrading the others isn't going to save us, because that would
still be using the backend from two threads (one successfully enabled
iothread, and the main thread).

Kevin

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Alberto Garcia]

On Mon 28 Jan 2019 05:01:09 PM CET, Kevin Wolf wrote:

> And actually... if you move the first virtio-blk device to an
> iothread, then downgrading the others isn't going to save us, because
> that would still be using the backend from two threads (one
> successfully enabled iothread, and the main thread).

...which makes me think that, since patch 1 from this series moves the
BlockBackend back to the main AioContext on unplug, we could have a
similar problem if the BDS is being used by other devices. Or am I
missing something?

Berto

Re: [Qemu-devel] [PATCH 3/3] virtio-scsi: Forbid devices with different iothreads sharing a blockdev

[Kevin Wolf]

Am 28.01.2019 um 17:15 hat Alberto Garcia geschrieben:
> On Mon 28 Jan 2019 05:01:09 PM CET, Kevin Wolf wrote:
> 
> > And actually... if you move the first virtio-blk device to an
> > iothread, then downgrading the others isn't going to save us, because
> > that would still be using the backend from two threads (one
> > successfully enabled iothread, and the main thread).
> 
> ...which makes me think that, since patch 1 from this series moves the
> BlockBackend back to the main AioContext on unplug, we could have a
> similar problem if the BDS is being used by other devices. Or am I
> missing something?

Oops. :-)

Maybe we do need to implement the proper solution?

Kevin