[Qemu-devel] [PATCH 0/7] block: Make more blockdev-add options work

[Qemu-devel] [PATCH 0/7] block: Make more blockdev-add options work

[Kevin Wolf]

This series moves a few more options from flags to the appropriate place. This
doesn't only result in cleaner code, but also allows using these options in
nested node definitions.

After this series, bds_tree_init() is only a thin wrapper around bdrv_open()
which sets the right defaults for cache modes and the BDRV_O_INACTIVE flag if
necessary.

Depends on my block branch, specifically Berto's 'read-only' patches.

Kevin Wolf (7):
  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

 block.c                        |  50 ++++++++++++++++++-
 block/block-backend.c          |   9 ++--
 block/raw-posix.c              |  45 ++++++++++-------
 block/raw-win32.c              |  50 +++++++++++++++++--
 blockdev.c                     | 110 +++--------------------------------------
 include/block/block.h          |   1 +
 include/sysemu/block-backend.h |   2 +-
 qapi/block-core.json           |  31 ++++++++----
 tests/qemu-iotests/087         |   4 +-
 tests/qemu-iotests/087.out     |   2 +-
 10 files changed, 159 insertions(+), 145 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/7] 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>
---
 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 a51434c..a611cc8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3834,21 +3834,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 f2d6f96..6cd5e37 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] [PATCH 2/7] 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>
---
 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 46661e6..0939011 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] [PATCH 3/7] 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>
---
 block/raw-posix.c      | 45 ++++++++++++++++++++++++++++-----------------
 block/raw-win32.c      | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 qapi/block-core.json   |  6 +++---
 tests/qemu-iotests/087 |  4 ++--
 4 files changed, 78 insertions(+), 27 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index 6ed7547..beb86a3 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;
+    const char *aio;
     int fd, ret;
     struct stat st;
 
@@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
+    aio = qemu_opt_get(opts, "aio");
+    if (!aio) {
+        s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
+    } else if (!strcmp(aio, "native")) {
+        s->use_linux_aio = true;
+    } else if (!strcmp(aio, "threads")) {
+        s->use_linux_aio = false;
+    } else {
+       error_setg(errp, "invalid aio option");
+       ret = -EINVAL;
+       goto fail;
+    }
+
     s->open_flags = open_flags;
     raw_parse_flags(bdrv_flags, &s->open_flags);
 
@@ -444,14 +452,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 +1265,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 +1294,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 +1305,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..61bb78e 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -252,7 +252,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 +265,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 +293,30 @@ 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)
+{
+    const char *aio = qemu_opt_get(opts, "aio");
+    if (!aio) {
+        return !!(flags & BDRV_O_NATIVE_AIO);
+    } else if (!strcmp(aio, "native")) {
+        return true;
+    } else if (!strcmp(aio, "threads")) {
+        return false;
+    }
+
+    error_setg(errp, "invalid aio option");
+    return false;
+}
+
 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
                     Error **errp)
 {
@@ -305,6 +326,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 +341,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 +375,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 +676,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 +689,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 +717,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 0939011..126b9ca 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: 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] [PATCH 4/7] block: Parse 'detect-zeroes' in bdrv_open_common()

[Kevin Wolf]

Amonst 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>
---
 block.c    | 33 +++++++++++++++++++++++++++++++++
 blockdev.c |  9 +--------
 2 files changed, 34 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 1e9d66b..b9c780e 100644
--- a/block.c
+++ b/block.c
@@ -41,6 +41,7 @@
 #include "qapi-event.h"
 #include "qemu/cutils.h"
 #include "qemu/id.h"
+#include "qapi/util.h"
 
 #ifdef CONFIG_BSD
 #include <sys/ioctl.h>
@@ -906,6 +907,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 */ }
     },
 };
@@ -922,6 +928,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;
@@ -990,6 +997,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,
+                            qemu_opt_get(opts, "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 a611cc8..b847ee9 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;
@@ -4140,10 +4137,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] [PATCH 5/7] 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>
---
 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 0bd19ab..bc14f96 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 b847ee9..2e0bc46 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2548,6 +2548,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;
@@ -2587,8 +2588,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));
     }
 
@@ -2625,8 +2630,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 3b29317..07339b7 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -199,7 +199,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] [PATCH 6/7] 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>
---
 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 b9c780e..5c019db 100644
--- a/block.c
+++ b/block.c
@@ -717,7 +717,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 |
@@ -912,6 +912,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 */ }
     },
 };
@@ -928,6 +933,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;
@@ -997,6 +1003,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 2e0bc46..34e7b2d 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;
-        }
     }
 }
 
@@ -3994,10 +3977,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)",
@@ -4129,10 +4108,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 e18233a..e572b31 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

[Qemu-devel] [PATCH 7/7] 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>
---
 blockdev.c | 54 +-----------------------------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 34e7b2d..d216caf 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)
@@ -4103,23 +4068,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

Re: [Qemu-devel] [PATCH 1/7] block: Drop aio/cache consistency check from qmp_blockdev_add()

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> 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>
> ---
>  blockdev.c                 | 15 ---------------
>  tests/qemu-iotests/087.out |  2 +-
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 2/7] block/qapi: Use separate options type for curl driver

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> 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>
> ---
>  qapi/block-core.json | 25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> 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>
> ---
>  block/raw-posix.c      | 45 ++++++++++++++++++++++++++++-----------------
>  block/raw-win32.c      | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
>  qapi/block-core.json   |  6 +++---
>  tests/qemu-iotests/087 |  4 ++--
>  4 files changed, 78 insertions(+), 27 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index 6ed7547..beb86a3 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;

Not this patch's concern, but why are some of our bools packed in a
bitfield and others used with a full byte?  Does the performance vs.
size tradeoff even matter for any of the members concerned?

>  } 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

Nice; we're getting rid of some awkward grammar.

> @@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
>          goto fail;
>      }
>  
> +    aio = qemu_opt_get(opts, "aio");
> +    if (!aio) {
> +        s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
> +    } else if (!strcmp(aio, "native")) {
> +        s->use_linux_aio = true;
> +    } else if (!strcmp(aio, "threads")) {
> +        s->use_linux_aio = false;
> +    } else {
> +       error_setg(errp, "invalid aio option");
> +       ret = -EINVAL;
> +       goto fail;
> +    }
> +

> +++ b/block/raw-win32.c

>  
> +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
> +{
> +    const char *aio = qemu_opt_get(opts, "aio");
> +    if (!aio) {
> +        return !!(flags & BDRV_O_NATIVE_AIO);
> +    } else if (!strcmp(aio, "native")) {
> +        return true;
> +    } else if (!strcmp(aio, "threads")) {
> +        return false;
> +    }
> +
> +    error_setg(errp, "invalid aio option");
> +    return false;
> +}

Is there somewhere common to share this, to avoid duplication?

> +++ 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: 1.7
>  ##
>  { 'struct': 'BlockdevOptionsFile',
> -  'data': { 'filename': 'str' } }
> +  'data': { 'filename': 'str',
> +            '*aio': 'BlockdevAioOptions' } }
>  

> +++ 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"


Looks like a reasonable relocation. And more evidence that blockdev-add
is not yet usable; hopefully we get there in 2.8.


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/7] block: Parse 'detect-zeroes' in bdrv_open_common()

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> Amonst others, this means that you can now use the 'detect-zeroes'

s/Amonst/Amongst/ (that's UK spelling; US shortens it to Among)

> option for non-top-level nodes in blockdev-add, like the QAPI schema
> promises.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c    | 33 +++++++++++++++++++++++++++++++++
>  blockdev.c |  9 +--------
>  2 files changed, 34 insertions(+), 8 deletions(-)
> 

> @@ -990,6 +997,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,
> +                            qemu_opt_get(opts, "detect-zeroes"),

Why do we have to repeat the qemu_opt_get() here, instead of just
passing in the just-collected detect_zeroes local variable?

Otherwise looks fine.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 5/7] block: Use 'detect-zeroes' option for 'blockdev-change-medium'

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> 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>
> ---
>  block/block-backend.c          | 9 ++++-----
>  blockdev.c                     | 9 ++++++---
>  include/sysemu/block-backend.h | 2 +-
>  3 files changed, 11 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 6/7] block: Move 'discard' option to bdrv_open_common()

[Eric Blake]

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

On 09/20/2016 04:08 PM, 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.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c               | 17 ++++++++++++++++-
>  blockdev.c            | 25 -------------------------
>  include/block/block.h |  1 +
>  3 files changed, 17 insertions(+), 26 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 7/7] block: Remove qemu_root_bds_opts

[Eric Blake]

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

On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> 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>
> ---
>  blockdev.c | 54 +-----------------------------------------------------
>  1 file changed, 1 insertion(+), 53 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Kevin Wolf]

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

Am 21.09.2016 um 00:27 hat Eric Blake geschrieben:
> On 09/20/2016 04:08 PM, Kevin Wolf wrote:
> > 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>
> > ---
> >  block/raw-posix.c      | 45 ++++++++++++++++++++++++++++-----------------
> >  block/raw-win32.c      | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
> >  qapi/block-core.json   |  6 +++---
> >  tests/qemu-iotests/087 |  4 ++--
> >  4 files changed, 78 insertions(+), 27 deletions(-)
> > 
> > diff --git a/block/raw-posix.c b/block/raw-posix.c
> > index 6ed7547..beb86a3 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;
> 
> Not this patch's concern, but why are some of our bools packed in a
> bitfield and others used with a full byte?  Does the performance vs.
> size tradeoff even matter for any of the members concerned?

Probably not. Also, we have 5 bytes of padding at the end of this
struct, so not using a bitfield would make it exactly the same size. And
if we care about the size, we are already wasting 4 bytes with the
misaligned size_t buf_align.

Anyway, I just did what most other fields are doing.

> >  } 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
> 
> Nice; we're getting rid of some awkward grammar.
> 
> > @@ -429,6 +424,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> >          goto fail;
> >      }
> >  
> > +    aio = qemu_opt_get(opts, "aio");
> > +    if (!aio) {
> > +        s->use_linux_aio = !!(bdrv_flags & BDRV_O_NATIVE_AIO);
> > +    } else if (!strcmp(aio, "native")) {
> > +        s->use_linux_aio = true;
> > +    } else if (!strcmp(aio, "threads")) {
> > +        s->use_linux_aio = false;
> > +    } else {
> > +       error_setg(errp, "invalid aio option");
> > +       ret = -EINVAL;
> > +       goto fail;
> > +    }
> > +
> 
> > +++ b/block/raw-win32.c
> 
> >  
> > +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
> > +{
> > +    const char *aio = qemu_opt_get(opts, "aio");
> > +    if (!aio) {
> > +        return !!(flags & BDRV_O_NATIVE_AIO);
> > +    } else if (!strcmp(aio, "native")) {
> > +        return true;
> > +    } else if (!strcmp(aio, "threads")) {
> > +        return false;
> > +    }
> > +
> > +    error_setg(errp, "invalid aio option");
> > +    return false;
> > +}
> 
> Is there somewhere common to share this, to avoid duplication?

I don't know where I would put it. This is a driver-specific option, so
it doesn't belong in the generic block layer. It's just that two drivers
happen to provide the same option currently. If we add another backend
to raw-posix, raw-win32 wouldn't get the new option, so maybe leaving
them separate is the best anyway.

I guess I could do something like this to make the "duplicated" code
look somewhat smaller, or at least condensed into a single statement:

    BlockdevAioOptions aio =
        qapi_enum_parse(BlockdevAioOptions_lookup,
                        qemu_opt_get(opts, "aio"),
                        BLOCKDEV_AIO_OPTIONS__MAX,
                        (flags & BDRV_O_NATIVE_AIO) ?
                            BLOCKDEV_AIO_OPTIONS_NATIVE :
                            BLOCKDEV_AIO_OPTIONS_THREADS);
    s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);

Would you consider this an improvement?

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 3/7] block/qapi: Move 'aio' option to file driver

[Eric Blake]

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

On 09/22/2016 05:25 AM, Kevin Wolf wrote:

>>>  
>>> +static bool get_aio_option(QemuOpts *opts, int flags, Error **errp)
>>> +{
>>> +    const char *aio = qemu_opt_get(opts, "aio");
>>> +    if (!aio) {
>>> +        return !!(flags & BDRV_O_NATIVE_AIO);
>>> +    } else if (!strcmp(aio, "native")) {
>>> +        return true;
>>> +    } else if (!strcmp(aio, "threads")) {
>>> +        return false;
>>> +    }
>>> +
>>> +    error_setg(errp, "invalid aio option");
>>> +    return false;
>>> +}
>>
>> Is there somewhere common to share this, to avoid duplication?
> 
> I don't know where I would put it. This is a driver-specific option, so
> it doesn't belong in the generic block layer. It's just that two drivers
> happen to provide the same option currently. If we add another backend
> to raw-posix, raw-win32 wouldn't get the new option, so maybe leaving
> them separate is the best anyway.
> 

Fair enough...

> I guess I could do something like this to make the "duplicated" code
> look somewhat smaller, or at least condensed into a single statement:
> 
>     BlockdevAioOptions aio =
>         qapi_enum_parse(BlockdevAioOptions_lookup,
>                         qemu_opt_get(opts, "aio"),
>                         BLOCKDEV_AIO_OPTIONS__MAX,
>                         (flags & BDRV_O_NATIVE_AIO) ?
>                             BLOCKDEV_AIO_OPTIONS_NATIVE :
>                             BLOCKDEV_AIO_OPTIONS_THREADS);
>     s->use_linux_aio = (aio == BLOCKDEV_AIO_OPTIONS_NATIVE);
> 
> Would you consider this an improvement?

Yes, that looks nicer, because it's not hand-rolling the parse. If, in
the future, we do diverge and add a mode in posix that is not available
in win32, that just means we would have two separate QAPI enums, but
keep the qapi_enum_parse() in place for no additional if/else branches.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]