[Qemu-devel] [PULL 00/18] Block layer patches

[Qemu-devel] [PULL 00/18] Block layer patches

[Kevin Wolf]

The following changes since commit 7cfdc02dae0d2ff58c897496cfdbbafc0eda0f3f:

  Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-26 19:47:00 +0100)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 3b856cebe5e93547852c156ca2119d075e62aed7:

  coroutine: reduce stack size to 60kB (2016-09-27 14:05:21 +0200)

----------------------------------------------------------------
Block layer patches

----------------------------------------------------------------
John Snow (3):
      block: reintroduce bdrv_flush_all
      qemu: use bdrv_flush_all for vm_stop et al
      block-backend: remove blk_flush_all

Kevin Wolf (8):
      block: Fix error path in qmp_blockdev_change_medium()
      block: Drop aio/cache consistency check from qmp_blockdev_add()
      block/qapi: Use separate options type for curl driver
      block/qapi: Move 'aio' option to file driver
      block: Parse 'detect-zeroes' in bdrv_open_common()
      block: Use 'detect-zeroes' option for 'blockdev-change-medium'
      block: Move 'discard' option to bdrv_open_common()
      block: Remove qemu_root_bds_opts

Peter Lieven (7):
      oslib-posix: add helpers for stack alloc and free
      coroutine-sigaltstack: rename coroutine struct appropriately
      coroutine: add a macro for the coroutine stack size
      coroutine-ucontext: use helper for allocating stack memory
      coroutine-sigaltstack: use helper for allocating stack memory
      oslib-posix: add a configure switch to debug stack usage
      coroutine: reduce stack size to 60kB

 block.c                        |  50 +++++++++++++++++-
 block/block-backend.c          |  31 ++----------
 block/io.c                     |  25 +++++++++
 block/raw-posix.c              |  44 +++++++++-------
 block/raw-win32.c              |  56 +++++++++++++++++++--
 blockdev.c                     | 112 +++--------------------------------------
 configure                      |  19 +++++++
 cpus.c                         |   4 +-
 hw/i386/xen/xen_platform.c     |   2 -
 hw/ide/piix.c                  |   4 ++
 include/block/block.h          |   2 +
 include/qemu/coroutine_int.h   |   2 +
 include/sysemu/block-backend.h |   3 +-
 include/sysemu/os-posix.h      |  27 ++++++++++
 qapi/block-core.json           |  31 ++++++++----
 tests/qemu-iotests/087         |   4 +-
 tests/qemu-iotests/087.out     |   2 +-
 util/coroutine-sigaltstack.c   |  25 ++++-----
 util/coroutine-ucontext.c      |  11 ++--
 util/coroutine-win32.c         |   2 +-
 util/oslib-posix.c             |  77 ++++++++++++++++++++++++++++
 21 files changed, 342 insertions(+), 191 deletions(-)

[Qemu-devel] [PULL 01/18] block: reintroduce bdrv_flush_all

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Commit fe1a9cbc moved the flush_all routine from the bdrv layer to the
block-backend layer. In doing so, however, the semantics of the routine
changed slightly such that flush_all now used blk_flush instead of
bdrv_flush.

blk_flush can fail if the attached device model reports that it is not
"available," (i.e. the tray is open.) This changed the semantics of
flush_all such that it can now fail for e.g. open CDROM drives.

Reintroduce bdrv_flush_all to regain the old semantics without having to
alter the behavior of blk_flush or blk_flush_all, which are already
'doing the right thing.'

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Acked-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c            | 25 +++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/block/io.c b/block/io.c
index fdf7080..57a2eeb 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1619,6 +1619,31 @@ int coroutine_fn bdrv_co_pwrite_zeroes(BdrvChild *child, int64_t offset,
                            BDRV_REQ_ZERO_WRITE | flags);
 }
 
+/*
+ * Flush ALL BDSes regardless of if they are reachable via a BlkBackend or not.
+ */
+int bdrv_flush_all(void)
+{
+    BdrvNextIterator it;
+    BlockDriverState *bs = NULL;
+    int result = 0;
+
+    for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
+        AioContext *aio_context = bdrv_get_aio_context(bs);
+        int ret;
+
+        aio_context_acquire(aio_context);
+        ret = bdrv_flush(bs);
+        if (ret < 0 && !result) {
+            result = ret;
+        }
+        aio_context_release(aio_context);
+    }
+
+    return result;
+}
+
+
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index e18233a..811b060 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -333,6 +333,7 @@ int bdrv_inactivate_all(void);
 /* Ensure contents are flushed to disk.  */
 int bdrv_flush(BlockDriverState *bs);
 int coroutine_fn bdrv_co_flush(BlockDriverState *bs);
+int bdrv_flush_all(void);
 void bdrv_close_all(void);
 void bdrv_drain(BlockDriverState *bs);
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs);
-- 
1.8.3.1

[Qemu-devel] [PULL 02/18] qemu: use bdrv_flush_all for vm_stop et al

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

Reimplement bdrv_flush_all for vm_stop. In contrast to blk_flush_all,
bdrv_flush_all does not have device model restrictions. This allows
us to flush and halt unconditionally without error.

This allows us to do things like migrate when we have a device with
an open tray, but has a node that may need to be flushed, or nodes
that aren't currently attached to any device and need to be flushed.

Specifically, this allows us to migrate when we have a CDROM with
an open tray.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Acked-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 cpus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index e39ccb7..96d9352 100644
--- a/cpus.c
+++ b/cpus.c
@@ -751,7 +751,7 @@ static int do_vm_stop(RunState state)
     }
 
     bdrv_drain_all();
-    ret = blk_flush_all();
+    ret = bdrv_flush_all();
 
     return ret;
 }
@@ -1494,7 +1494,7 @@ int vm_stop_force_state(RunState state)
         bdrv_drain_all();
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
-        return blk_flush_all();
+        return bdrv_flush_all();
     }
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 03/18] block-backend: remove blk_flush_all

[Kevin Wolf]

From: John Snow <jsnow@redhat.com>

We can teach Xen to drain and flush each device as it needs to, instead
of trying to flush ALL devices. This removes the last user of
blk_flush_all.

The function is therefore removed under the premise that any new uses
of blk_flush_all would be the wrong paradigm: either flush the single
device that requires flushing, or use an appropriate flush_all mechanism
from outside of the BlkBackend layer.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Acked-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c          | 22 ----------------------
 hw/i386/xen/xen_platform.c     |  2 --
 hw/ide/piix.c                  |  4 ++++
 include/sysemu/block-backend.h |  1 -
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0bd19ab..f34bad5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1640,28 +1640,6 @@ int blk_commit_all(void)
     return 0;
 }
 
-int blk_flush_all(void)
-{
-    BlockBackend *blk = NULL;
-    int result = 0;
-
-    while ((blk = blk_all_next(blk)) != NULL) {
-        AioContext *aio_context = blk_get_aio_context(blk);
-        int ret;
-
-        aio_context_acquire(aio_context);
-        if (blk_is_inserted(blk)) {
-            ret = blk_flush(blk);
-            if (ret < 0 && !result) {
-                result = ret;
-            }
-        }
-        aio_context_release(aio_context);
-    }
-
-    return result;
-}
-
 
 /* throttling disk I/O limits */
 void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index aa78393..f85635c 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -134,8 +134,6 @@ static void platform_fixed_ioport_writew(void *opaque, uint32_t addr, uint32_t v
            devices, and bit 2 the non-primary-master IDE devices. */
         if (val & UNPLUG_ALL_IDE_DISKS) {
             DPRINTF("unplug disks\n");
-            blk_drain_all();
-            blk_flush_all();
             pci_unplug_disks(pci_dev->bus);
         }
         if (val & UNPLUG_ALL_NICS) {
diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index c190fca..d5777fd 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -179,6 +179,10 @@ int pci_piix3_xen_ide_unplug(DeviceState *dev)
         if (di != NULL && !di->media_cd) {
             BlockBackend *blk = blk_by_legacy_dinfo(di);
             DeviceState *ds = blk_get_attached_dev(blk);
+
+            blk_drain(blk);
+            blk_flush(blk);
+
             if (ds) {
                 blk_detach_dev(blk, ds);
             }
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3b29317..24d1d85 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -152,7 +152,6 @@ BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
 int blk_co_pdiscard(BlockBackend *blk, int64_t offset, int count);
 int blk_co_flush(BlockBackend *blk);
 int blk_flush(BlockBackend *blk);
-int blk_flush_all(void);
 int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
-- 
1.8.3.1

[Qemu-devel] [PULL 04/18] block: Fix error path in qmp_blockdev_change_medium()

[Kevin Wolf]

Commit 00949bab incorrectly changed one instance of &err into errp while
touching the line. Change it back.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 29c6561..62d0dd0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2614,7 +2614,7 @@ void qmp_blockdev_change_medium(bool has_device, const char *device,
     error_free(err);
     err = NULL;
 
-    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, errp);
+    qmp_x_blockdev_remove_medium(has_device, device, has_id, id, &err);
     if (err) {
         error_propagate(errp, err);
         goto fail;
-- 
1.8.3.1

[Qemu-devel] [PULL 05/18] block: Drop aio/cache consistency check from qmp_blockdev_add()

[Kevin Wolf]

The TODO comment has been addressed a while ago and this is now checked
in raw-posix, so we don't have to special case this in blockdev-add any
more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c                 | 15 ---------------
 tests/qemu-iotests/087.out |  2 +-
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 62d0dd0..7820f42 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3832,21 +3832,6 @@ void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
     QDict *qdict;
     Error *local_err = NULL;
 
-    /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
-     * cache.direct=false instead of silently switching to aio=threads, except
-     * when called from drive_new().
-     *
-     * For now, simply forbidding the combination for all drivers will do. */
-    if (options->has_aio && options->aio == BLOCKDEV_AIO_OPTIONS_NATIVE) {
-        bool direct = options->has_cache &&
-                      options->cache->has_direct &&
-                      options->cache->direct;
-        if (!direct) {
-            error_setg(errp, "aio=native requires cache.direct=true");
-            goto fail;
-        }
-    }
-
     visit_type_BlockdevOptions(v, NULL, &options, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
index bef6862..cd02eae 100644
--- a/tests/qemu-iotests/087.out
+++ b/tests/qemu-iotests/087.out
@@ -27,7 +27,7 @@ QMP_VERSION
 Testing:
 QMP_VERSION
 {"return": {}}
-{"error": {"class": "GenericError", "desc": "aio=native requires cache.direct=true"}}
+{"error": {"class": "GenericError", "desc": "aio=native was specified, but it requires cache.direct=on, which was not specified."}}
 {"return": {}}
 {"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN"}
 
-- 
1.8.3.1

[Qemu-devel] [PULL 06/18] block/qapi: Use separate options type for curl driver

[Kevin Wolf]

We're going to add an option to the file drivers which doesn't apply to
the curl drivers, so give them a separate option type.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 qapi/block-core.json | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 92193ab..b5fdd42 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1721,8 +1721,7 @@
 ##
 # @BlockdevOptionsFile
 #
-# Driver specific block device options for the file backend and similar
-# protocols.
+# Driver specific block device options for the file backend.
 #
 # @filename:    path to the image file
 #
@@ -2211,6 +2210,18 @@
             '*top-id': 'str' } }
 
 ##
+# @BlockdevOptionsCurl
+#
+# Driver specific block device options for the curl backend.
+#
+# @filename:    path to the image file
+#
+# Since: 1.7
+##
+{ 'struct': 'BlockdevOptionsCurl',
+  'data': { 'filename': 'str' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2248,13 +2259,13 @@
       'cloop':      'BlockdevOptionsGenericFormat',
       'dmg':        'BlockdevOptionsGenericFormat',
       'file':       'BlockdevOptionsFile',
-      'ftp':        'BlockdevOptionsFile',
-      'ftps':       'BlockdevOptionsFile',
+      'ftp':        'BlockdevOptionsCurl',
+      'ftps':       'BlockdevOptionsCurl',
       'gluster':    'BlockdevOptionsGluster',
       'host_cdrom': 'BlockdevOptionsFile',
       'host_device':'BlockdevOptionsFile',
-      'http':       'BlockdevOptionsFile',
-      'https':      'BlockdevOptionsFile',
+      'http':       'BlockdevOptionsCurl',
+      'https':      'BlockdevOptionsCurl',
 # TODO iscsi: Wait for structured options
       'luks':       'BlockdevOptionsLUKS',
 # TODO nbd: Should take InetSocketAddress for 'host'?
@@ -2271,7 +2282,7 @@
       'replication':'BlockdevOptionsReplication',
 # TODO sheepdog: Wait for structured options
 # TODO ssh: Should take InetSocketAddress for 'host'?
-      'tftp':       'BlockdevOptionsFile',
+      'tftp':       'BlockdevOptionsCurl',
       'vdi':        'BlockdevOptionsGenericFormat',
       'vhdx':       'BlockdevOptionsGenericFormat',
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
-- 
1.8.3.1

[Qemu-devel] [PULL 07/18] block/qapi: Move 'aio' option to file driver

[Kevin Wolf]

The option whether or not to use a native AIO interface really isn't a
generic option for all drivers, but only applies to the native file
protocols. This patch moves the option in blockdev-add to the
appropriate places (raw-posix and raw-win32).

We still have to keep the flag BDRV_O_NATIVE_AIO for compatibility
because so far the AIO option was usually specified on the wrong layer
(the top-level format driver, which didn't even look at it) and then
inherited by the protocol driver (where it was actually used). We can't
forbid this use except in new interfaces.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/raw-posix.c      | 44 ++++++++++++++++++++++++---------------
 block/raw-win32.c      | 56 +++++++++++++++++++++++++++++++++++++++++++++-----
 qapi/block-core.json   |  6 +++---
 tests/qemu-iotests/087 |  4 ++--
 4 files changed, 83 insertions(+), 27 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6ed7547..166e9d1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -143,6 +143,7 @@ typedef struct BDRVRawState {
     bool has_discard:1;
     bool has_write_zeroes:1;
     bool discard_zeroes:1;
+    bool use_linux_aio:1;
     bool has_fallocate;
     bool needs_alignment;
 } BDRVRawState;
@@ -367,18 +368,6 @@ static void raw_parse_flags(int bdrv_flags, int *open_flags)
     }
 }
 
-#ifdef CONFIG_LINUX_AIO
-static bool raw_use_aio(int bdrv_flags)
-{
-    /*
-     * Currently Linux do AIO only for files opened with O_DIRECT
-     * specified so check NOCACHE flag too
-     */
-    return (bdrv_flags & (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO)) ==
-                         (BDRV_O_NOCACHE|BDRV_O_NATIVE_AIO);
-}
-#endif
-
 static void raw_parse_filename(const char *filename, QDict *options,
                                Error **errp)
 {
@@ -399,6 +388,11 @@ static QemuOptsList raw_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "File name of the image",
         },
+        {
+            .name = "aio",
+            .type = QEMU_OPT_STRING,
+            .help = "host AIO implementation (threads, native)",
+        },
         { /* end of list */ }
     },
 };
@@ -410,6 +404,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename = NULL;
+    BlockdevAioOptions aio, aio_default;
     int fd, ret;
     struct stat st;
 
@@ -429,6 +424,18 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
+    aio_default = (bdrv_flags & BDRV_O_NATIVE_AIO)
+                  ? BLOCKDEV_AIO_OPTIONS_NATIVE
+                  : BLOCKDEV_AIO_OPTIONS_THREADS;
+    aio = qapi_enum_parse(BlockdevAioOptions_lookup, qemu_opt_get(opts, "aio"),
+                          BLOCKDEV_AIO_OPTIONS__MAX, aio_default, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+    s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
+
     s->open_flags = open_flags;
     raw_parse_flags(bdrv_flags, &s->open_flags);
 
@@ -444,14 +451,15 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     s->fd = fd;
 
 #ifdef CONFIG_LINUX_AIO
-    if (!raw_use_aio(bdrv_flags) && (bdrv_flags & BDRV_O_NATIVE_AIO)) {
+     /* Currently Linux does AIO only for files opened with O_DIRECT */
+    if (s->use_linux_aio && !(s->open_flags & O_DIRECT)) {
         error_setg(errp, "aio=native was specified, but it requires "
                          "cache.direct=on, which was not specified.");
         ret = -EINVAL;
         goto fail;
     }
 #else
-    if (bdrv_flags & BDRV_O_NATIVE_AIO) {
+    if (s->use_linux_aio) {
         error_setg(errp, "aio=native was specified, but is not supported "
                          "in this build.");
         ret = -EINVAL;
@@ -1256,7 +1264,7 @@ static int coroutine_fn raw_co_prw(BlockDriverState *bs, uint64_t offset,
         if (!bdrv_qiov_is_aligned(bs, qiov)) {
             type |= QEMU_AIO_MISALIGNED;
 #ifdef CONFIG_LINUX_AIO
-        } else if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+        } else if (s->use_linux_aio) {
             LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
             assert(qiov->size == bytes);
             return laio_co_submit(bs, aio, s->fd, offset, qiov, type);
@@ -1285,7 +1293,8 @@ static int coroutine_fn raw_co_pwritev(BlockDriverState *bs, uint64_t offset,
 static void raw_aio_plug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
-    if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+    BDRVRawState *s = bs->opaque;
+    if (s->use_linux_aio) {
         LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
         laio_io_plug(bs, aio);
     }
@@ -1295,7 +1304,8 @@ static void raw_aio_plug(BlockDriverState *bs)
 static void raw_aio_unplug(BlockDriverState *bs)
 {
 #ifdef CONFIG_LINUX_AIO
-    if (bs->open_flags & BDRV_O_NATIVE_AIO) {
+    BDRVRawState *s = bs->opaque;
+    if (s->use_linux_aio) {
         LinuxAioState *aio = aio_get_linux_aio(bdrv_get_aio_context(bs));
         laio_io_unplug(bs, aio);
     }
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 56f45fe..734bb10 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -32,6 +32,7 @@
 #include "block/thread-pool.h"
 #include "qemu/iov.h"
 #include "qapi/qmp/qstring.h"
+#include "qapi/util.h"
 #include <windows.h>
 #include <winioctl.h>
 
@@ -252,7 +253,8 @@ static void raw_probe_alignment(BlockDriverState *bs, Error **errp)
     }
 }
 
-static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)
+static void raw_parse_flags(int flags, bool use_aio, int *access_flags,
+                            DWORD *overlapped)
 {
     assert(access_flags != NULL);
     assert(overlapped != NULL);
@@ -264,7 +266,7 @@ static void raw_parse_flags(int flags, int *access_flags, DWORD *overlapped)
     }
 
     *overlapped = FILE_ATTRIBUTE_NORMAL;
-    if (flags & BDRV_O_NATIVE_AIO) {
+    if (use_aio) {
         *overlapped |= FILE_FLAG_OVERLAPPED;
     }
     if (flags & BDRV_O_NOCACHE) {
@@ -292,10 +294,35 @@ static QemuOptsList raw_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "File name of the image",
         },
+        {
+            .name = "aio",
+            .type = QEMU_OPT_STRING,
+            .help = "host AIO implementation (threads, native)",
+        },
         { /* end of list */ }
     },
 };
 
+static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
+{
+    BlockdevAioOptions aio, aio_default;
+
+    aio_default = (flags & BDRV_O_NATIVE_AIO) ? BLOCKDEV_AIO_OPTIONS_NATIVE
+                                              : BLOCKDEV_AIO_OPTIONS_THREADS;
+    aio = qapi_enum_parse(BlockdevAioOptions_lookup, qemu_opt_get(opts, "aio"),
+                          BLOCKDEV_AIO_OPTIONS__MAX, aio_default, errp);
+
+    switch (aio) {
+    case BLOCKDEV_AIO_OPTIONS_NATIVE:
+        return true;
+    case BLOCKDEV_AIO_OPTIONS_THREADS:
+        return false;
+    default:
+        error_setg(errp, "Invalid AIO option");
+    }
+    return false;
+}
+
 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -305,6 +332,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
     QemuOpts *opts;
     Error *local_err = NULL;
     const char *filename;
+    bool use_aio;
     int ret;
 
     s->type = FTYPE_FILE;
@@ -319,7 +347,14 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
 
     filename = qemu_opt_get(opts, "filename");
 
-    raw_parse_flags(flags, &access_flags, &overlapped);
+    use_aio = get_aio_option(opts, flags, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    raw_parse_flags(flags, use_aio, &access_flags, &overlapped);
 
     if (filename[0] && filename[1] == ':') {
         snprintf(s->drive_path, sizeof(s->drive_path), "%c:\\", filename[0]);
@@ -346,7 +381,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    if (flags & BDRV_O_NATIVE_AIO) {
+    if (use_aio) {
         s->aio = win32_aio_init();
         if (s->aio == NULL) {
             CloseHandle(s->hfile);
@@ -647,6 +682,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
 
     Error *local_err = NULL;
     const char *filename;
+    bool use_aio;
 
     QemuOpts *opts = qemu_opts_create(&raw_runtime_opts, NULL, 0,
                                       &error_abort);
@@ -659,6 +695,16 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
 
     filename = qemu_opt_get(opts, "filename");
 
+    use_aio = get_aio_option(opts, flags, &local_err);
+    if (!local_err && use_aio) {
+        error_setg(&local_err, "AIO is not supported on Windows host devices");
+    }
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto done;
+    }
+
     if (strstart(filename, "/dev/cdrom", NULL)) {
         if (find_cdrom(device_name, sizeof(device_name)) < 0) {
             error_setg(errp, "Could not open CD-ROM drive");
@@ -677,7 +723,7 @@ static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->type = find_device_type(bs, filename);
 
-    raw_parse_flags(flags, &access_flags, &overlapped);
+    raw_parse_flags(flags, use_aio, &access_flags, &overlapped);
 
     create_flags = OPEN_EXISTING;
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b5fdd42..9d797b8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1724,11 +1724,13 @@
 # Driver specific block device options for the file backend.
 #
 # @filename:    path to the image file
+# @aio:         #optional AIO backend (default: threads) (since: 2.8)
 #
 # Since: 1.7
 ##
 { 'struct': 'BlockdevOptionsFile',
-  'data': { 'filename': 'str' } }
+  'data': { 'filename': 'str',
+            '*aio': 'BlockdevAioOptions' } }
 
 ##
 # @BlockdevOptionsNull
@@ -2232,7 +2234,6 @@
 #                 This option is required on the top level of blockdev-add.
 # @discard:       #optional discard-related options (default: ignore)
 # @cache:         #optional cache-related options
-# @aio:           #optional AIO backend (default: threads)
 # @read-only:     #optional whether the block device should be read-only
 #                 (default: false)
 # @detect-zeroes: #optional detect and optimize zero writes (Since 2.1)
@@ -2247,7 +2248,6 @@
             '*node-name': 'str',
             '*discard': 'BlockdevDiscardOptions',
             '*cache': 'BlockdevCacheOptions',
-            '*aio': 'BlockdevAioOptions',
             '*read-only': 'bool',
             '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
   'discriminator': 'driver',
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 5c04577..b1ac71f 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -117,10 +117,10 @@ run_qemu <<EOF
       "options": {
         "driver": "$IMGFMT",
         "node-name": "disk",
-        "aio": "native",
         "file": {
             "driver": "file",
-            "filename": "$TEST_IMG"
+            "filename": "$TEST_IMG",
+            "aio": "native"
         }
       }
     }
-- 
1.8.3.1

[Qemu-devel] [PULL 08/18] block: Parse 'detect-zeroes' in bdrv_open_common()

[Kevin Wolf]

Amongst others, this means that you can now use the 'detect-zeroes'
option for non-top-level nodes in blockdev-add, like the QAPI schema
promises.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c    | 33 +++++++++++++++++++++++++++++++++
 blockdev.c |  9 +--------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 493ecf3..1f10457 100644
--- a/block.c
+++ b/block.c
@@ -42,6 +42,7 @@
 #include "qapi-event.h"
 #include "qemu/cutils.h"
 #include "qemu/id.h"
+#include "qapi/util.h"
 
 #ifdef CONFIG_BSD
 #include <sys/ioctl.h>
@@ -954,6 +955,11 @@ static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "Node is opened in read-only mode",
         },
+        {
+            .name = "detect-zeroes",
+            .type = QEMU_OPT_STRING,
+            .help = "try to optimize zero writes (off, on, unmap)",
+        },
         { /* end of list */ }
     },
 };
@@ -970,6 +976,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     const char *filename;
     const char *driver_name = NULL;
     const char *node_name = NULL;
+    const char *detect_zeroes;
     QemuOpts *opts;
     BlockDriver *drv;
     Error *local_err = NULL;
@@ -1038,6 +1045,32 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         }
     }
 
+    detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
+    if (detect_zeroes) {
+        BlockdevDetectZeroesOptions value =
+            qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+                            detect_zeroes,
+                            BLOCKDEV_DETECT_ZEROES_OPTIONS__MAX,
+                            BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+                            &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto fail_opts;
+        }
+
+        if (value == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+            !(bs->open_flags & BDRV_O_UNMAP))
+        {
+            error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+                             "without setting discard operation to unmap");
+            ret = -EINVAL;
+            goto fail_opts;
+        }
+
+        bs->detect_zeroes = value;
+    }
+
     if (filename != NULL) {
         pstrcpy(bs->filename, sizeof(bs->filename), filename);
     } else {
diff --git a/blockdev.c b/blockdev.c
index 7820f42..511260c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -658,7 +658,6 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
     BlockDriverState *bs;
     QemuOpts *opts;
     Error *local_error = NULL;
-    BlockdevDetectZeroesOptions detect_zeroes;
     int bdrv_flags = 0;
 
     opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
@@ -673,7 +672,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
     }
 
     extract_common_blockdev_options(opts, &bdrv_flags, NULL, NULL,
-                                    &detect_zeroes, &local_error);
+                                    NULL, &local_error);
     if (local_error) {
         error_propagate(errp, local_error);
         goto fail;
@@ -695,8 +694,6 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
         goto fail_no_bs_opts;
     }
 
-    bs->detect_zeroes = detect_zeroes;
-
 fail_no_bs_opts:
     qemu_opts_del(opts);
     return bs;
@@ -4136,10 +4133,6 @@ static QemuOptsList qemu_root_bds_opts = {
             .name = "copy-on-read",
             .type = QEMU_OPT_BOOL,
             .help = "copy read data from backing file into image file",
-        },{
-            .name = "detect-zeroes",
-            .type = QEMU_OPT_STRING,
-            .help = "try to optimize zero writes (off, on, unmap)",
         },
         { /* end of list */ }
     },
-- 
1.8.3.1

[Qemu-devel] [PULL 09/18] block: Use 'detect-zeroes' option for 'blockdev-change-medium'

[Kevin Wolf]

Instead of modifying the new BDS after it has been opened, use the newly
supported 'detect-zeroes' option in bdrv_open_common() so that all
requirements are checked (detect-zeroes=unmap requires discard=unmap).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/block-backend.c          | 9 ++++-----
 blockdev.c                     | 9 ++++++---
 include/sysemu/block-backend.h | 2 +-
 3 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f34bad5..639294b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1592,13 +1592,12 @@ void blk_update_root_state(BlockBackend *blk)
 }
 
 /*
- * Applies the information in the root state to the given BlockDriverState. This
- * does not include the flags which have to be specified for bdrv_open(), use
- * blk_get_open_flags_from_root_state() to inquire them.
+ * Returns the detect-zeroes setting to be used for bdrv_open() of a
+ * BlockDriverState which is supposed to inherit the root state.
  */
-void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs)
+bool blk_get_detect_zeroes_from_root_state(BlockBackend *blk)
 {
-    bs->detect_zeroes = blk->root_state.detect_zeroes;
+    return blk->root_state.detect_zeroes;
 }
 
 /*
diff --git a/blockdev.c b/blockdev.c
index 511260c..7b87bd8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2546,6 +2546,7 @@ void qmp_blockdev_change_medium(bool has_device, const char *device,
     BlockBackend *blk;
     BlockDriverState *medium_bs = NULL;
     int bdrv_flags;
+    bool detect_zeroes;
     int rc;
     QDict *options = NULL;
     Error *err = NULL;
@@ -2585,8 +2586,12 @@ void qmp_blockdev_change_medium(bool has_device, const char *device,
         abort();
     }
 
+    options = qdict_new();
+    detect_zeroes = blk_get_detect_zeroes_from_root_state(blk);
+    qdict_put(options, "detect-zeroes",
+              qstring_from_str(detect_zeroes ? "on" : "off"));
+
     if (has_format) {
-        options = qdict_new();
         qdict_put(options, "driver", qstring_from_str(format));
     }
 
@@ -2623,8 +2628,6 @@ void qmp_blockdev_change_medium(bool has_device, const char *device,
         goto fail;
     }
 
-    blk_apply_root_state(blk, medium_bs);
-
     qmp_blockdev_close_tray(has_device, device, has_id, id, errp);
 
 fail:
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 24d1d85..a7993af 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -198,7 +198,7 @@ void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
 BlockBackendRootState *blk_get_root_state(BlockBackend *blk);
 void blk_update_root_state(BlockBackend *blk);
-void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs);
+bool blk_get_detect_zeroes_from_root_state(BlockBackend *blk);
 int blk_get_open_flags_from_root_state(BlockBackend *blk);
 
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
-- 
1.8.3.1

[Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Kevin Wolf]

This enables its use for nested child nodes. The compatibility
between the 'discard' and 'detect-zeroes' setting is checked in
bdrv_open_common() now as the former setting isn't available before
calling bdrv_open() any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 17 ++++++++++++++++-
 blockdev.c            | 25 -------------------------
 include/block/block.h |  1 +
 3 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/block.c b/block.c
index 1f10457..bb1f1ec 100644
--- a/block.c
+++ b/block.c
@@ -765,7 +765,7 @@ static void bdrv_inherited_options(int *child_flags, QDict *child_options,
     /* Our block drivers take care to send flushes and respect unmap policy,
      * so we can default to enable both on lower layers regardless of the
      * corresponding parent options. */
-    flags |= BDRV_O_UNMAP;
+    qdict_set_default_str(child_options, BDRV_OPT_DISCARD, "unmap");
 
     /* Clear flags that only apply to the top layer */
     flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ |
@@ -960,6 +960,11 @@ static QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "try to optimize zero writes (off, on, unmap)",
         },
+        {
+            .name = "discard",
+            .type = QEMU_OPT_STRING,
+            .help = "discard operation (ignore/off, unmap/on)",
+        },
         { /* end of list */ }
     },
 };
@@ -976,6 +981,7 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
     const char *filename;
     const char *driver_name = NULL;
     const char *node_name = NULL;
+    const char *discard;
     const char *detect_zeroes;
     QemuOpts *opts;
     BlockDriver *drv;
@@ -1045,6 +1051,15 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
         }
     }
 
+    discard = qemu_opt_get(opts, "discard");
+    if (discard != NULL) {
+        if (bdrv_parse_discard_flags(discard, &bs->open_flags) != 0) {
+            error_setg(errp, "Invalid discard option");
+            ret = -EINVAL;
+            goto fail_opts;
+        }
+    }
+
     detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
     if (detect_zeroes) {
         BlockdevDetectZeroesOptions value =
diff --git a/blockdev.c b/blockdev.c
index 7b87bd8..e2ace04 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -356,7 +356,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
     const char **throttling_group, ThrottleConfig *throttle_cfg,
     BlockdevDetectZeroesOptions *detect_zeroes, Error **errp)
 {
-    const char *discard;
     Error *local_error = NULL;
     const char *aio;
 
@@ -365,13 +364,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             *bdrv_flags |= BDRV_O_COPY_ON_READ;
         }
 
-        if ((discard = qemu_opt_get(opts, "discard")) != NULL) {
-            if (bdrv_parse_discard_flags(discard, bdrv_flags) != 0) {
-                error_setg(errp, "Invalid discard option");
-                return;
-            }
-        }
-
         if ((aio = qemu_opt_get(opts, "aio")) != NULL) {
             if (!strcmp(aio, "native")) {
                 *bdrv_flags |= BDRV_O_NATIVE_AIO;
@@ -449,15 +441,6 @@ static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
             error_propagate(errp, local_error);
             return;
         }
-
-        if (bdrv_flags &&
-            *detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-            !(*bdrv_flags & BDRV_O_UNMAP))
-        {
-            error_setg(errp, "setting detect-zeroes to unmap is not allowed "
-                             "without setting discard operation to unmap");
-            return;
-        }
     }
 }
 
@@ -3990,10 +3973,6 @@ QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "enable/disable snapshot mode",
         },{
-            .name = "discard",
-            .type = QEMU_OPT_STRING,
-            .help = "discard operation (ignore/off, unmap/on)",
-        },{
             .name = "aio",
             .type = QEMU_OPT_STRING,
             .help = "host AIO implementation (threads, native)",
@@ -4125,10 +4104,6 @@ static QemuOptsList qemu_root_bds_opts = {
     .head = QTAILQ_HEAD_INITIALIZER(qemu_root_bds_opts.head),
     .desc = {
         {
-            .name = "discard",
-            .type = QEMU_OPT_STRING,
-            .help = "discard operation (ignore/off, unmap/on)",
-        },{
             .name = "aio",
             .type = QEMU_OPT_STRING,
             .help = "host AIO implementation (threads, native)",
diff --git a/include/block/block.h b/include/block/block.h
index 811b060..107c603 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -108,6 +108,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_CACHE_DIRECT   "cache.direct"
 #define BDRV_OPT_CACHE_NO_FLUSH "cache.no-flush"
 #define BDRV_OPT_READ_ONLY      "read-only"
+#define BDRV_OPT_DISCARD        "discard"
 
 
 #define BDRV_SECTOR_BITS   9
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Gerd Hoffmann]

On Di, 2016-09-27 at 15:53 +0200, Kevin Wolf wrote:
> This enables its use for nested child nodes. The compatibility
> between the 'discard' and 'detect-zeroes' setting is checked in
> bdrv_open_common() now as the former setting isn't available before
> calling bdrv_open() any more.

Seems this change makes libvirt not see the discard option any more,
"virsh start" throws this error now ...

error: Failed to start domain fedora-org-xhci
error: unsupported configuration: discard is not supported by this QEMU
binary

... and git bisect found this commit.

cheers,
  Gerd

Re: [Qemu-devel] [PULL 10/18] block: Move 'discard' option to bdrv_open_common()

[Kevin Wolf]

Am 07.10.2016 um 11:01 hat Gerd Hoffmann geschrieben:
> On Di, 2016-09-27 at 15:53 +0200, Kevin Wolf wrote:
> > This enables its use for nested child nodes. The compatibility
> > between the 'discard' and 'detect-zeroes' setting is checked in
> > bdrv_open_common() now as the former setting isn't available before
> > calling bdrv_open() any more.
> 
> Seems this change makes libvirt not see the discard option any more,
> "virsh start" throws this error now ...
> 
> error: Failed to start domain fedora-org-xhci
> error: unsupported configuration: discard is not supported by this QEMU
> binary
> 
> ... and git bisect found this commit.

The fix is already on the list:

    [PATCH] block: Add bdrv_runtime_opts to query-command-line-options

Kevin

[Qemu-devel] [PULL 11/18] block: Remove qemu_root_bds_opts

[Kevin Wolf]

The remaining options in qemu_root_bds_opts (aio and copy-on-read)
aren't used any more, the QAPI schema doesn't contain them. Therefore
all the code processing qemu_root_bds_opts options is dead and can be
removed.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 54 +-----------------------------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e2ace04..07ec733 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -633,34 +633,11 @@ err_no_opts:
     return NULL;
 }
 
-static QemuOptsList qemu_root_bds_opts;
-
 /* Takes the ownership of bs_opts */
 static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
 {
-    BlockDriverState *bs;
-    QemuOpts *opts;
-    Error *local_error = NULL;
     int bdrv_flags = 0;
 
-    opts = qemu_opts_create(&qemu_root_bds_opts, NULL, 1, errp);
-    if (!opts) {
-        goto fail;
-    }
-
-    qemu_opts_absorb_qdict(opts, bs_opts, &local_error);
-    if (local_error) {
-        error_propagate(errp, local_error);
-        goto fail;
-    }
-
-    extract_common_blockdev_options(opts, &bdrv_flags, NULL, NULL,
-                                    NULL, &local_error);
-    if (local_error) {
-        error_propagate(errp, local_error);
-        goto fail;
-    }
-
     /* bdrv_open() defaults to the values in bdrv_flags (for compatibility
      * with other callers) rather than what we want as the real defaults.
      * Apply the defaults here instead. */
@@ -672,19 +649,7 @@ static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
         bdrv_flags |= BDRV_O_INACTIVE;
     }
 
-    bs = bdrv_open(NULL, NULL, bs_opts, bdrv_flags, errp);
-    if (!bs) {
-        goto fail_no_bs_opts;
-    }
-
-fail_no_bs_opts:
-    qemu_opts_del(opts);
-    return bs;
-
-fail:
-    qemu_opts_del(opts);
-    QDECREF(bs_opts);
-    return NULL;
+    return bdrv_open(NULL, NULL, bs_opts, bdrv_flags, errp);
 }
 
 void blockdev_close_all_bdrv_states(void)
@@ -4099,23 +4064,6 @@ QemuOptsList qemu_common_drive_opts = {
     },
 };
 
-static QemuOptsList qemu_root_bds_opts = {
-    .name = "root-bds",
-    .head = QTAILQ_HEAD_INITIALIZER(qemu_root_bds_opts.head),
-    .desc = {
-        {
-            .name = "aio",
-            .type = QEMU_OPT_STRING,
-            .help = "host AIO implementation (threads, native)",
-        },{
-            .name = "copy-on-read",
-            .type = QEMU_OPT_BOOL,
-            .help = "copy read data from backing file into image file",
-        },
-        { /* end of list */ }
-    },
-};
-
 QemuOptsList qemu_drive_opts = {
     .name = "drive",
     .head = QTAILQ_HEAD_INITIALIZER(qemu_drive_opts.head),
-- 
1.8.3.1

[Qemu-devel] [PULL 12/18] oslib-posix: add helpers for stack alloc and free

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

the allocated stack will be adjusted to the minimum supported stack size
by the OS and rounded up to be a multiple of the system pagesize.
Additionally an architecture dependent guard page is added to the stack
to catch stack overflows.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/sysemu/os-posix.h | 27 +++++++++++++++++++++++++++
 util/oslib-posix.c        | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 9c7dfdf..3cfedbc 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -60,4 +60,31 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
 
 bool is_daemonized(void);
 
+/**
+ * qemu_alloc_stack:
+ * @sz: pointer to a size_t holding the requested usable stack size
+ *
+ * Allocate memory that can be used as a stack, for instance for
+ * coroutines. If the memory cannot be allocated, this function
+ * will abort (like g_malloc()). This function also inserts an
+ * additional guard page to catch a potential stack overflow.
+ * Note that the memory required for the guard page and alignment
+ * and minimal stack size restrictions will increase the value of sz.
+ *
+ * The allocated stack must be freed with qemu_free_stack().
+ *
+ * Returns: pointer to (the lowest address of) the stack memory.
+ */
+void *qemu_alloc_stack(size_t *sz);
+
+/**
+ * qemu_free_stack:
+ * @stack: stack to free
+ * @sz: size of stack in bytes
+ *
+ * Free a stack allocated via qemu_alloc_stack(). Note that sz must
+ * be exactly the adjusted stack size returned by qemu_alloc_stack.
+ */
+void qemu_free_stack(void *stack, size_t sz);
+
 #endif
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f2d4e9e..d950c34 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -499,3 +499,45 @@ pid_t qemu_fork(Error **errp)
     }
     return pid;
 }
+
+void *qemu_alloc_stack(size_t *sz)
+{
+    void *ptr, *guardpage;
+    size_t pagesz = getpagesize();
+#ifdef _SC_THREAD_STACK_MIN
+    /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
+    long min_stack_sz = sysconf(_SC_THREAD_STACK_MIN);
+    *sz = MAX(MAX(min_stack_sz, 0), *sz);
+#endif
+    /* adjust stack size to a multiple of the page size */
+    *sz = ROUND_UP(*sz, pagesz);
+    /* allocate one extra page for the guard page */
+    *sz += pagesz;
+
+    ptr = mmap(NULL, *sz, PROT_READ | PROT_WRITE,
+               MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+    if (ptr == MAP_FAILED) {
+        abort();
+    }
+
+#if defined(HOST_IA64)
+    /* separate register stack */
+    guardpage = ptr + (((*sz - pagesz) / 2) & ~pagesz);
+#elif defined(HOST_HPPA)
+    /* stack grows up */
+    guardpage = ptr + *sz - pagesz;
+#else
+    /* stack grows down */
+    guardpage = ptr;
+#endif
+    if (mprotect(guardpage, pagesz, PROT_NONE) != 0) {
+        abort();
+    }
+
+    return ptr;
+}
+
+void qemu_free_stack(void *stack, size_t sz)
+{
+    munmap(stack, sz);
+}
-- 
1.8.3.1

[Qemu-devel] [PULL 13/18] coroutine-sigaltstack: rename coroutine struct appropriately

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

The name of the sigaltstack coroutine struct was misleading.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 util/coroutine-sigaltstack.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index a7c3366..171cd44 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -34,7 +34,7 @@ typedef struct {
     Coroutine base;
     void *stack;
     sigjmp_buf env;
-} CoroutineUContext;
+} CoroutineSigAltStack;
 
 /**
  * Per-thread coroutine bookkeeping
@@ -44,7 +44,7 @@ typedef struct {
     Coroutine *current;
 
     /** The default coroutine */
-    CoroutineUContext leader;
+    CoroutineSigAltStack leader;
 
     /** Information for the signal handler (trampoline) */
     sigjmp_buf tr_reenter;
@@ -89,7 +89,7 @@ static void __attribute__((constructor)) coroutine_init(void)
  * (from the signal handler when it is not signal handling, read ahead
  * for more information).
  */
-static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
+static void coroutine_bootstrap(CoroutineSigAltStack *self, Coroutine *co)
 {
     /* Initialize longjmp environment and switch back the caller */
     if (!sigsetjmp(self->env, 0)) {
@@ -109,7 +109,7 @@ static void coroutine_bootstrap(CoroutineUContext *self, Coroutine *co)
  */
 static void coroutine_trampoline(int signal)
 {
-    CoroutineUContext *self;
+    CoroutineSigAltStack *self;
     Coroutine *co;
     CoroutineThreadState *coTS;
 
@@ -144,7 +144,7 @@ static void coroutine_trampoline(int signal)
 Coroutine *qemu_coroutine_new(void)
 {
     const size_t stack_size = 1 << 20;
-    CoroutineUContext *co;
+    CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
     struct sigaction osa;
@@ -251,7 +251,7 @@ Coroutine *qemu_coroutine_new(void)
 
 void qemu_coroutine_delete(Coroutine *co_)
 {
-    CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
+    CoroutineSigAltStack *co = DO_UPCAST(CoroutineSigAltStack, base, co_);
 
     g_free(co->stack);
     g_free(co);
@@ -260,8 +260,8 @@ void qemu_coroutine_delete(Coroutine *co_)
 CoroutineAction qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
                                       CoroutineAction action)
 {
-    CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
-    CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
+    CoroutineSigAltStack *from = DO_UPCAST(CoroutineSigAltStack, base, from_);
+    CoroutineSigAltStack *to = DO_UPCAST(CoroutineSigAltStack, base, to_);
     CoroutineThreadState *s = coroutine_get_thread_state();
     int ret;
 
-- 
1.8.3.1

[Qemu-devel] [PULL 14/18] coroutine: add a macro for the coroutine stack size

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

Signed-off-by: Peter Lieven <pl@kamp.de>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Richard Henderson <rth@twiddle.net>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/coroutine_int.h | 2 ++
 util/coroutine-sigaltstack.c | 2 +-
 util/coroutine-ucontext.c    | 2 +-
 util/coroutine-win32.c       | 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 6df9d33..14d4f1d 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,6 +28,8 @@
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
 
+#define COROUTINE_STACK_SIZE (1 << 20)
+
 typedef enum {
     COROUTINE_YIELD = 1,
     COROUTINE_TERMINATE = 2,
diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index 171cd44..a5bcb7e 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -143,7 +143,7 @@ static void coroutine_trampoline(int signal)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 2bb7e10..31254ab 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -82,7 +82,7 @@ static void coroutine_trampoline(int i0, int i1)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineUContext *co;
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
diff --git a/util/coroutine-win32.c b/util/coroutine-win32.c
index 02e28e8..de6bd4f 100644
--- a/util/coroutine-win32.c
+++ b/util/coroutine-win32.c
@@ -71,7 +71,7 @@ static void CALLBACK coroutine_trampoline(void *co_)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = 1 << 20;
+    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineWin32 *co;
 
     co = g_malloc0(sizeof(*co));
-- 
1.8.3.1

[Qemu-devel] [PULL 15/18] coroutine-ucontext: use helper for allocating stack memory

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 util/coroutine-ucontext.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index 31254ab..6621f3f 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -34,6 +34,7 @@
 typedef struct {
     Coroutine base;
     void *stack;
+    size_t stack_size;
     sigjmp_buf env;
 
 #ifdef CONFIG_VALGRIND_H
@@ -82,7 +83,6 @@ static void coroutine_trampoline(int i0, int i1)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineUContext *co;
     ucontext_t old_uc, uc;
     sigjmp_buf old_env;
@@ -101,17 +101,18 @@ Coroutine *qemu_coroutine_new(void)
     }
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack_size = COROUTINE_STACK_SIZE;
+    co->stack = qemu_alloc_stack(&co->stack_size);
     co->base.entry_arg = &old_env; /* stash away our jmp_buf */
 
     uc.uc_link = &old_uc;
     uc.uc_stack.ss_sp = co->stack;
-    uc.uc_stack.ss_size = stack_size;
+    uc.uc_stack.ss_size = co->stack_size;
     uc.uc_stack.ss_flags = 0;
 
 #ifdef CONFIG_VALGRIND_H
     co->valgrind_stack_id =
-        VALGRIND_STACK_REGISTER(co->stack, co->stack + stack_size);
+        VALGRIND_STACK_REGISTER(co->stack, co->stack + co->stack_size);
 #endif
 
     arg.p = co;
@@ -149,7 +150,7 @@ void qemu_coroutine_delete(Coroutine *co_)
     valgrind_stack_deregister(co);
 #endif
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, co->stack_size);
     g_free(co);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 16/18] coroutine-sigaltstack: use helper for allocating stack memory

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 util/coroutine-sigaltstack.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/util/coroutine-sigaltstack.c b/util/coroutine-sigaltstack.c
index a5bcb7e..f6fc49a 100644
--- a/util/coroutine-sigaltstack.c
+++ b/util/coroutine-sigaltstack.c
@@ -33,6 +33,7 @@
 typedef struct {
     Coroutine base;
     void *stack;
+    size_t stack_size;
     sigjmp_buf env;
 } CoroutineSigAltStack;
 
@@ -143,7 +144,6 @@ static void coroutine_trampoline(int signal)
 
 Coroutine *qemu_coroutine_new(void)
 {
-    const size_t stack_size = COROUTINE_STACK_SIZE;
     CoroutineSigAltStack *co;
     CoroutineThreadState *coTS;
     struct sigaction sa;
@@ -164,7 +164,8 @@ Coroutine *qemu_coroutine_new(void)
      */
 
     co = g_malloc0(sizeof(*co));
-    co->stack = g_malloc(stack_size);
+    co->stack_size = COROUTINE_STACK_SIZE;
+    co->stack = qemu_alloc_stack(&co->stack_size);
     co->base.entry_arg = &old_env; /* stash away our jmp_buf */
 
     coTS = coroutine_get_thread_state();
@@ -189,7 +190,7 @@ Coroutine *qemu_coroutine_new(void)
      * Set the new stack.
      */
     ss.ss_sp = co->stack;
-    ss.ss_size = stack_size;
+    ss.ss_size = co->stack_size;
     ss.ss_flags = 0;
     if (sigaltstack(&ss, &oss) < 0) {
         abort();
@@ -253,7 +254,7 @@ void qemu_coroutine_delete(Coroutine *co_)
 {
     CoroutineSigAltStack *co = DO_UPCAST(CoroutineSigAltStack, base, co_);
 
-    g_free(co->stack);
+    qemu_free_stack(co->stack, co->stack_size);
     g_free(co);
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 17/18] oslib-posix: add a configure switch to debug stack usage

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

this adds a knob to track the maximum stack usage of stacks
created by qemu_alloc_stack.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 configure          | 19 +++++++++++++++++++
 util/oslib-posix.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/configure b/configure
index 8fa62ad..93ef00a 100755
--- a/configure
+++ b/configure
@@ -296,6 +296,7 @@ libiscsi=""
 libnfs=""
 coroutine=""
 coroutine_pool=""
+debug_stack_usage="no"
 seccomp=""
 glusterfs=""
 glusterfs_xlator_opt="no"
@@ -1004,6 +1005,8 @@ for opt do
   ;;
   --enable-coroutine-pool) coroutine_pool="yes"
   ;;
+  --enable-debug-stack-usage) debug_stack_usage="yes"
+  ;;
   --disable-docs) docs="no"
   ;;
   --enable-docs) docs="yes"
@@ -4276,6 +4279,17 @@ if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
   error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
 fi
 
+if test "$debug_stack_usage" = "yes"; then
+  if test "$cpu" = "ia64" -o "$cpu" = "hppa"; then
+    error_exit "stack usage debugging is not supported for $cpu"
+  fi
+  if test "$coroutine_pool" = "yes"; then
+    echo "WARN: disabling coroutine pool for stack usage debugging"
+    coroutine_pool=no
+  fi
+fi
+
+
 ##########################################
 # check if we have open_by_handle_at
 
@@ -4861,6 +4875,7 @@ echo "QGA MSI support   $guest_agent_msi"
 echo "seccomp support   $seccomp"
 echo "coroutine backend $coroutine"
 echo "coroutine pool    $coroutine_pool"
+echo "debug stack usage $debug_stack_usage"
 echo "GlusterFS support $glusterfs"
 echo "Archipelago support $archipelago"
 echo "gcov              $gcov_tool"
@@ -5330,6 +5345,10 @@ else
   echo "CONFIG_COROUTINE_POOL=0" >> $config_host_mak
 fi
 
+if test "$debug_stack_usage" = "yes" ; then
+  echo "CONFIG_DEBUG_STACK_USAGE=y" >> $config_host_mak
+fi
+
 if test "$open_by_handle_at" = "yes" ; then
   echo "CONFIG_OPEN_BY_HANDLE=y" >> $config_host_mak
 fi
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index d950c34..aaec189 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -50,6 +50,10 @@
 
 #include "qemu/mmap-alloc.h"
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+#include "qemu/error-report.h"
+#endif
+
 int qemu_get_thread_id(void)
 {
 #if defined(__linux__)
@@ -503,6 +507,9 @@ pid_t qemu_fork(Error **errp)
 void *qemu_alloc_stack(size_t *sz)
 {
     void *ptr, *guardpage;
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    void *ptr2;
+#endif
     size_t pagesz = getpagesize();
 #ifdef _SC_THREAD_STACK_MIN
     /* avoid stacks smaller than _SC_THREAD_STACK_MIN */
@@ -534,10 +541,38 @@ void *qemu_alloc_stack(size_t *sz)
         abort();
     }
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    for (ptr2 = ptr + pagesz; ptr2 < ptr + *sz; ptr2 += sizeof(uint32_t)) {
+        *(uint32_t *)ptr2 = 0xdeadbeaf;
+    }
+#endif
+
     return ptr;
 }
 
+#ifdef CONFIG_DEBUG_STACK_USAGE
+static __thread unsigned int max_stack_usage;
+#endif
+
 void qemu_free_stack(void *stack, size_t sz)
 {
+#ifdef CONFIG_DEBUG_STACK_USAGE
+    unsigned int usage;
+    void *ptr;
+
+    for (ptr = stack + getpagesize(); ptr < stack + sz;
+         ptr += sizeof(uint32_t)) {
+        if (*(uint32_t *)ptr != 0xdeadbeaf) {
+            break;
+        }
+    }
+    usage = sz - (uintptr_t) (ptr - stack);
+    if (usage > max_stack_usage) {
+        error_report("thread %d max stack usage increased from %u to %u",
+                     qemu_get_thread_id(), max_stack_usage, usage);
+        max_stack_usage = usage;
+    }
+#endif
+
     munmap(stack, sz);
 }
-- 
1.8.3.1

[Qemu-devel] [PULL 18/18] coroutine: reduce stack size to 60kB

[Kevin Wolf]

From: Peter Lieven <pl@kamp.de>

evaluation with the recently introduced maximum stack usage monitoring revealed
that the actual used stack size was never above 4kB so allocating 1MB stack
for each coroutine is a lot of wasted memory. So reduce the stack size to
60kB which should still give enough head room. The guard page added
in qemu_alloc_stack will catch a potential stack overflow introduced
by this commit. The 60kB + guard page will result in an allocation of
64kB per coroutine on systems where a page is 4kB.

Signed-off-by: Peter Lieven <pl@kamp.de>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/coroutine_int.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 14d4f1d..be14260 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -28,7 +28,7 @@
 #include "qemu/queue.h"
 #include "qemu/coroutine.h"
 
-#define COROUTINE_STACK_SIZE (1 << 20)
+#define COROUTINE_STACK_SIZE 61440
 
 typedef enum {
     COROUTINE_YIELD = 1,
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Peter Maydell]

On 27 September 2016 at 06:53, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit 7cfdc02dae0d2ff58c897496cfdbbafc0eda0f3f:
>
>   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-26 19:47:00 +0100)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 3b856cebe5e93547852c156ca2119d075e62aed7:
>
>   coroutine: reduce stack size to 60kB (2016-09-27 14:05:21 +0200)
>
> ----------------------------------------------------------------
> Block layer patches
>
> ----------------------------------------------------------------

I see 'make check' failures on x86-64 host, clang Linux:

  /i386/ahci/migrate/ncq/simple:                                       OK
  /i386/ahci/migrate/ncq/halted:                                       OK
  /i386/ahci/cdrom/dma/single:                                         OK
  /i386/ahci/cdrom/dma/multi:                                          OK
  /i386/ahci/cdrom/pio/single:
Broken pipe
FAIL
GTester: last random seed: R02Sa8f729848b07c3b3e5ee67368f9d0350
(pid=10590)
  /i386/ahci/cdrom/pio/multi:
Broken pipe
FAIL
GTester: last random seed: R02Se85704e04bbd382223983c878723b811
(pid=10598)
FAIL: tests/ahci-test
TEST: tests/hd-geo-test... (pid=10601)
  /i386/hd-geo/ide/none:                                               OK


thanks
-- PMM

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Kevin Wolf]

Am 27.09.2016 um 21:42 hat Peter Maydell geschrieben:
> On 27 September 2016 at 06:53, Kevin Wolf <kwolf@redhat.com> wrote:
> > The following changes since commit 7cfdc02dae0d2ff58c897496cfdbbafc0eda0f3f:
> >
> >   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-26 19:47:00 +0100)
> >
> > are available in the git repository at:
> >
> >
> >   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> >
> > for you to fetch changes up to 3b856cebe5e93547852c156ca2119d075e62aed7:
> >
> >   coroutine: reduce stack size to 60kB (2016-09-27 14:05:21 +0200)
> >
> > ----------------------------------------------------------------
> > Block layer patches
> >
> > ----------------------------------------------------------------
> 
> I see 'make check' failures on x86-64 host, clang Linux:
> 
>   /i386/ahci/migrate/ncq/simple:                                       OK
>   /i386/ahci/migrate/ncq/halted:                                       OK
>   /i386/ahci/cdrom/dma/single:                                         OK
>   /i386/ahci/cdrom/dma/multi:                                          OK
>   /i386/ahci/cdrom/pio/single:
> Broken pipe
> FAIL
> GTester: last random seed: R02Sa8f729848b07c3b3e5ee67368f9d0350
> (pid=10590)
>   /i386/ahci/cdrom/pio/multi:
> Broken pipe
> FAIL
> GTester: last random seed: R02Se85704e04bbd382223983c878723b811
> (pid=10598)
> FAIL: tests/ahci-test
> TEST: tests/hd-geo-test... (pid=10601)
>   /i386/hd-geo/ide/none:                                               OK

I asked on IRC, but as you don't seem to be around at the moment, I'll
keep things on the list instead.

Can you tell me the exact 'configure' and 'make check' command lines
you're using? I always run the tests with clang/Linux on x86_64 before
sending a pull request, and I repeated it now, but the tests pass for
me.

This is what I'm doing (yesterday I built all targets, today only
i386-softmmu because that seems to be what fails for you):

$ clang --version
clang version 3.4.2 (tags/RELEASE_34/dot2-final)
Target: x86_64-redhat-linux-gnu
Thread model: posix
$ ../qemu/configure --cc=clang --host-cc=clang --cxx=clang++ --target-list=i386-softmmu
$ make -j4
$ make check

Kevin

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Peter Maydell]

On 28 September 2016 at 02:37, Kevin Wolf <kwolf@redhat.com> wrote:
> [what clang config?]

This is with Ubuntu Xenial's clang. configure args are

'--cc=clang' '--cxx=clang++' '--enable-gtk'
'--extra-cflags=-fsanitize=undefined -Werror'

and the build is just 'make all -j8 && make check' in the build
directory.

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Peter Maydell]

On 28 September 2016 at 02:37, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 27.09.2016 um 21:42 hat Peter Maydell geschrieben:
>> On 27 September 2016 at 06:53, Kevin Wolf <kwolf@redhat.com> wrote:
>> > The following changes since commit 7cfdc02dae0d2ff58c897496cfdbbafc0eda0f3f:
>> >
>> >   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-26 19:47:00 +0100)
>> >
>> > are available in the git repository at:
>> >
>> >
>> >   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>> >
>> > for you to fetch changes up to 3b856cebe5e93547852c156ca2119d075e62aed7:
>> >
>> >   coroutine: reduce stack size to 60kB (2016-09-27 14:05:21 +0200)
>> >
>> > ----------------------------------------------------------------
>> > Block layer patches
>> >
>> > ----------------------------------------------------------------
>>
>> I see 'make check' failures on x86-64 host, clang Linux:
>>
>>   /i386/ahci/migrate/ncq/simple:                                       OK
>>   /i386/ahci/migrate/ncq/halted:                                       OK
>>   /i386/ahci/cdrom/dma/single:                                         OK
>>   /i386/ahci/cdrom/dma/multi:                                          OK
>>   /i386/ahci/cdrom/pio/single:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02Sa8f729848b07c3b3e5ee67368f9d0350
>> (pid=10590)
>>   /i386/ahci/cdrom/pio/multi:
>> Broken pipe
>> FAIL
>> GTester: last random seed: R02Se85704e04bbd382223983c878723b811
>> (pid=10598)
>> FAIL: tests/ahci-test
>> TEST: tests/hd-geo-test... (pid=10601)
>>   /i386/hd-geo/ide/none:                                               OK
>
> I asked on IRC, but as you don't seem to be around at the moment, I'll
> keep things on the list instead.

I got a gdb backtrace:

Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
0x00005555561dea15 in address_space_translate (as=0x55555a46bfc0,
addr=1106048, xlat=0x7ffff7e0d050, plen=0x7ffff7e0d058,
    is_write=false) at /home/petmay01/linaro/qemu-for-merges/exec.c:423
423     {


Backtrace suggests we've run out of stack due to some infinite
recursion:

#0  0x00005555561dea15 in address_space_translate (as=0x55555a46bfc0,
addr=1106048, xlat=0x7ffff7e0d050, plen=0x7ffff7e0d058,
is_write=false) at /home/petmay01/linaro/qemu-for-merges/exec.c:423
#1  0x00005555561edeab in address_space_map (as=<optimised out>,
addr=1106048, plen=<optimised out>, is_write=false)
    at /home/petmay01/linaro/qemu-for-merges/exec.c:2909
#2  0x0000555556840b9b in ahci_populate_sglist (as=0x55555a46bfc0,
addr=1106048, dir=DMA_DIRECTION_TO_DEVICE, len=<optimised out>)
    at /home/petmay01/linaro/qemu-for-merges/include/sysemu/dma.h:135
#3  0x0000555556840b9b in ahci_populate_sglist (ad=<optimised out>,
sglist=<optimised out>, cmd=<optimised out>, limit=<optimised out>,
offset=1592) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:863
#4  0x0000555556844de4 in ahci_dma_prepare_buf (dma=0x55555a475b48,
limit=<optimised out>)
    at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1366
#5  0x000055555684354c in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1295
#6  0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#7  0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#8  0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#9  0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#10 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#11 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#12 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#13 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#14 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#15 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#16 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#17 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#18 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#19 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#20 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#21 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#22 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#23 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#24 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#25 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#26 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#27 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#28 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#29 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#30 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#31 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#32 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#33 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#34 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#35 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#36 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#37 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#38 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#39 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#40 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#41 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
/home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318

[skip a lot of repeated stack frames]

#393 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#394 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#395 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#396 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#397 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#398 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#399 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#400 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#401 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#402 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#403 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
#404 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
#405 0x0000555556809cfc in ide_buffered_readv_cb
(opaque=0x5555594f57e0, ret=<optimised out>)
    at /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:605
#406 0x0000555556df7f73 in blk_aio_complete (acb=0x55555a4387c0) at
/home/petmay01/linaro/qemu-for-merges/block/block-backend.c:943
#407 0x0000555556f676f1 in coroutine_trampoline (i0=<optimised out>,
i1=<optimised out>)
    at /home/petmay01/linaro/qemu-for-merges/util/coroutine-ucontext.c:79
#408 0x00007fffdca05590 in __start_context () at /lib/x86_64-linux-gnu/libc.so.6
#409 0x00007fffffffc318 in  ()
#410 0x0000000000000000 in  ()


thanks
-- PMM

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Kevin Wolf]

Am 28.09.2016 um 21:03 hat Peter Maydell geschrieben:
> On 28 September 2016 at 02:37, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 27.09.2016 um 21:42 hat Peter Maydell geschrieben:
> >> On 27 September 2016 at 06:53, Kevin Wolf <kwolf@redhat.com> wrote:
> >> > The following changes since commit 7cfdc02dae0d2ff58c897496cfdbbafc0eda0f3f:
> >> >
> >> >   Merge remote-tracking branch 'remotes/mst/tags/for_upstream' into staging (2016-09-26 19:47:00 +0100)
> >> >
> >> > are available in the git repository at:
> >> >
> >> >
> >> >   git://repo.or.cz/qemu/kevin.git tags/for-upstream
> >> >
> >> > for you to fetch changes up to 3b856cebe5e93547852c156ca2119d075e62aed7:
> >> >
> >> >   coroutine: reduce stack size to 60kB (2016-09-27 14:05:21 +0200)
> >> >
> >> > ----------------------------------------------------------------
> >> > Block layer patches
> >> >
> >> > ----------------------------------------------------------------
> >>
> >> I see 'make check' failures on x86-64 host, clang Linux:
> >>
> >>   /i386/ahci/migrate/ncq/simple:                                       OK
> >>   /i386/ahci/migrate/ncq/halted:                                       OK
> >>   /i386/ahci/cdrom/dma/single:                                         OK
> >>   /i386/ahci/cdrom/dma/multi:                                          OK
> >>   /i386/ahci/cdrom/pio/single:
> >> Broken pipe
> >> FAIL
> >> GTester: last random seed: R02Sa8f729848b07c3b3e5ee67368f9d0350
> >> (pid=10590)
> >>   /i386/ahci/cdrom/pio/multi:
> >> Broken pipe
> >> FAIL
> >> GTester: last random seed: R02Se85704e04bbd382223983c878723b811
> >> (pid=10598)
> >> FAIL: tests/ahci-test
> >> TEST: tests/hd-geo-test... (pid=10601)
> >>   /i386/hd-geo/ide/none:                                               OK
> >
> > I asked on IRC, but as you don't seem to be around at the moment, I'll
> > keep things on the list instead.
> 
> I got a gdb backtrace:
> 
> Thread 1 "qemu-system-i38" received signal SIGSEGV, Segmentation fault.
> 0x00005555561dea15 in address_space_translate (as=0x55555a46bfc0,
> addr=1106048, xlat=0x7ffff7e0d050, plen=0x7ffff7e0d058,
>     is_write=false) at /home/petmay01/linaro/qemu-for-merges/exec.c:423
> 423     {
> 
> 
> Backtrace suggests we've run out of stack due to some infinite
> recursion:

Thanks, Peter, this is useful.

The series contains a patch that reduces the coroutine stack size, so I
guess it's not quite infinite, but pretty deep recursion anyway. I will
drop that final patch that reduces the stack size and hope that the rest
will pass your testing (I tried some more to reproduce it, but I still
didn't manage to).

John, can you have a look at the IDE code and check whether we can get
rid of the deep recursion? It seems that the test issues a large request
that is then split into many small requests. But it should be possible
to do this iteratively rather than recursively.

Kevin

> #0  0x00005555561dea15 in address_space_translate (as=0x55555a46bfc0,
> addr=1106048, xlat=0x7ffff7e0d050, plen=0x7ffff7e0d058,
> is_write=false) at /home/petmay01/linaro/qemu-for-merges/exec.c:423
> #1  0x00005555561edeab in address_space_map (as=<optimised out>,
> addr=1106048, plen=<optimised out>, is_write=false)
>     at /home/petmay01/linaro/qemu-for-merges/exec.c:2909
> #2  0x0000555556840b9b in ahci_populate_sglist (as=0x55555a46bfc0,
> addr=1106048, dir=DMA_DIRECTION_TO_DEVICE, len=<optimised out>)
>     at /home/petmay01/linaro/qemu-for-merges/include/sysemu/dma.h:135
> #3  0x0000555556840b9b in ahci_populate_sglist (ad=<optimised out>,
> sglist=<optimised out>, cmd=<optimised out>, limit=<optimised out>,
> offset=1592) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:863
> #4  0x0000555556844de4 in ahci_dma_prepare_buf (dma=0x55555a475b48,
> limit=<optimised out>)
>     at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1366
> #5  0x000055555684354c in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1295
> #6  0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #7  0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #8  0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #9  0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #10 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #11 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #12 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #13 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #14 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #15 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #16 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #17 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #18 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #19 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #20 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #21 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #22 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #23 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #24 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #25 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #26 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #27 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #28 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #29 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #30 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #31 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #32 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #33 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #34 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #35 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #36 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #37 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #38 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #39 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #40 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #41 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>) at
> /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> 
> [skip a lot of repeated stack frames]
> 
> #393 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #394 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #395 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #396 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #397 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #398 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #399 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #400 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #401 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #402 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #403 0x0000555556843662 in ahci_start_transfer (dma=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/ahci.c:1318
> #404 0x00005555568250cb in ide_atapi_cmd_reply_end (s=<optimised out>)
> at /home/petmay01/linaro/qemu-for-merges/hw/ide/atapi.c:324
> #405 0x0000555556809cfc in ide_buffered_readv_cb
> (opaque=0x5555594f57e0, ret=<optimised out>)
>     at /home/petmay01/linaro/qemu-for-merges/hw/ide/core.c:605
> #406 0x0000555556df7f73 in blk_aio_complete (acb=0x55555a4387c0) at
> /home/petmay01/linaro/qemu-for-merges/block/block-backend.c:943
> #407 0x0000555556f676f1 in coroutine_trampoline (i0=<optimised out>,
> i1=<optimised out>)
>     at /home/petmay01/linaro/qemu-for-merges/util/coroutine-ucontext.c:79
> #408 0x00007fffdca05590 in __start_context () at /lib/x86_64-linux-gnu/libc.so.6
> #409 0x00007fffffffc318 in  ()
> #410 0x0000000000000000 in  ()
> 
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[John Snow]

On 09/29/2016 06:25 AM, Kevin Wolf wrote:
> John, can you have a look at the IDE code and check whether we can get
> rid of the deep recursion? It seems that the test issues a large request
> that is then split into many small requests. But it should be possible
> to do this iteratively rather than recursively.

http://wiki.qemu.org/Features/IDE

"Rewrite ide_atapi_cmd_reply_end to be fully non-recursive; and ideally 
less confusing."

Guess I have to float this one to the top.

--js

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Paolo Bonzini]

On 29/09/2016 19:02, John Snow wrote:
> 
> 
> On 09/29/2016 06:25 AM, Kevin Wolf wrote:
>> John, can you have a look at the IDE code and check whether we can get
>> rid of the deep recursion? It seems that the test issues a large request
>> that is then split into many small requests. But it should be possible
>> to do this iteratively rather than recursively.
> 
> http://wiki.qemu.org/Features/IDE
> 
> "Rewrite ide_atapi_cmd_reply_end to be fully non-recursive; and ideally
> less confusing."
> 
> Guess I have to float this one to the top.

Would it be enough to hide the call to s->bus->dma->ops->start_transfer
behind a bottom half?

Paolo

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[John Snow]

On 09/29/2016 02:17 PM, Paolo Bonzini wrote:
>
>
> On 29/09/2016 19:02, John Snow wrote:
>>
>>
>> On 09/29/2016 06:25 AM, Kevin Wolf wrote:
>>> John, can you have a look at the IDE code and check whether we can get
>>> rid of the deep recursion? It seems that the test issues a large request
>>> that is then split into many small requests. But it should be possible
>>> to do this iteratively rather than recursively.
>>
>> http://wiki.qemu.org/Features/IDE
>>
>> "Rewrite ide_atapi_cmd_reply_end to be fully non-recursive; and ideally
>> less confusing."
>>
>> Guess I have to float this one to the top.
>
> Would it be enough to hide the call to s->bus->dma->ops->start_transfer
> behind a bottom half?
>
> Paolo
>

Probably the simplest way, even if not the prettiest.

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[Peter Maydell]

On 29 September 2016 at 03:25, Kevin Wolf <kwolf@redhat.com> wrote:
> The series contains a patch that reduces the coroutine stack size, so I
> guess it's not quite infinite, but pretty deep recursion anyway. I will
> drop that final patch that reduces the stack size and hope that the rest
> will pass your testing (I tried some more to reproduce it, but I still
> didn't manage to).

Ah, I see. I suspect the clang build (since it has the sanitizer
enabled) is a worst-case for stack usage.

Is it possible for a guest to issue a sufficiently large
request that it blows the stack, or is that capped
somehow?

thanks
-- PMM

Re: [Qemu-devel] [PULL 00/18] Block layer patches

[John Snow]

On 09/29/2016 01:18 PM, Peter Maydell wrote:
> On 29 September 2016 at 03:25, Kevin Wolf <kwolf@redhat.com> wrote:
>> The series contains a patch that reduces the coroutine stack size, so I
>> guess it's not quite infinite, but pretty deep recursion anyway. I will
>> drop that final patch that reduces the stack size and hope that the rest
>> will pass your testing (I tried some more to reproduce it, but I still
>> didn't manage to).
>
> Ah, I see. I suspect the clang build (since it has the sanitizer
> enabled) is a worst-case for stack usage.
>
> Is it possible for a guest to issue a sufficiently large
> request that it blows the stack, or is that capped
> somehow?
>
> thanks
> -- PMM
>

If the qtest can blow the stack, so can a guest. In practice it does not 
happen that guests send requests so large, but mum's the word on a 
naughty guest.

--js