qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 31/34] block: Move cache options into options QD


From: Max Reitz
Subject: Re: [Qemu-block] [PATCH 31/34] block: Move cache options into options QDict
Date: Fri, 15 May 2015 20:43:08 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 08.05.2015 19:22, Kevin Wolf wrote:
This adds the cache mode options to the QDict, so that they can be
specified for child nodes (e.g. backing.cache.direct=off).

The cache modes are not removed from the flags at this point; instead,
options and flags are kept in sync. If the user specifies both flags and
options, the options take precedence.

Child node inherit cache modes as options now, they don't use flags any
more.

Signed-off-by: Kevin Wolf <address@hidden>
---
  block.c    | 85 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
  blockdev.c | 27 +++++---------------
  2 files changed, 88 insertions(+), 24 deletions(-)

diff --git a/block.c b/block.c
index 8faa5ce..2430070 100644
--- a/block.c
+++ b/block.c
@@ -27,6 +27,7 @@
  #include "block/block_int.h"
  #include "block/blockjob.h"
  #include "qemu/module.h"
+#include "qapi/qmp/qbool.h"
  #include "qapi/qmp/qjson.h"
  #include "sysemu/block-backend.h"
  #include "sysemu/sysemu.h"
@@ -689,9 +690,15 @@ static void bdrv_inherited_options(int *child_flags, QDict 
*child_options,
      /* Enable protocol handling, disable format probing for bs->file */
      flags |= BDRV_O_PROTOCOL;
+ /* If the cache mode isn't explicitly set, inherit direct and no-flush from
+     * the parent. */
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+

Well, this directly violates what I wanted these functions to guarantee: Not set any option with a dot in them. It's fine anyway because "cache" is never used as the bdref_key for child BDS.

      /* Our block drivers take care to send flushes and respect unmap policy,
       * so we can enable both unconditionally on lower layers. */

The comment reads a bit strange now (because it's referring to two heterogenous lines instead of one line with two flags on it), and also WB is no longer "unconditionally" enabled.

-    flags |= BDRV_O_CACHE_WB | BDRV_O_UNMAP;
+    qdict_set_default_str(child_options, BDRV_OPT_CACHE_WB, "on");
+    flags |= BDRV_O_UNMAP;
/* Clear flags that only apply to the top layer */
      flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
@@ -725,6 +732,11 @@ static void bdrv_backing_options(int *child_flags, QDict 
*child_options,
  {
      int flags = parent_flags;
+ /* The cache mode is inherited unmodified for backing files */
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_WB);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_DIRECT);
+    qdict_copy_default(child_options, parent_options, BDRV_OPT_CACHE_NO_FLUSH);
+
      /* backing files always opened read-only */
      flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
@@ -758,6 +770,42 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
      return open_flags;
  }
+static void update_flags_from_options(int *flags, QemuOpts *opts)
+{
+    *flags &= ~BDRV_O_CACHE_MASK;
+
+    assert(qemu_opt_find(opts, BDRV_OPT_CACHE_WB));
+    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, false)) {
+        *flags |= BDRV_O_CACHE_WB;
+    }
+
+    assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
+    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+        *flags |= BDRV_O_NO_FLUSH;
+    }
+
+    assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
+    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+        *flags |= BDRV_O_NOCACHE;
+    }
+}
+
+static void update_options_from_flags(QDict *options, int flags)
+{
+    if (!qdict_haskey(options, BDRV_OPT_CACHE_WB)) {
+        qdict_put(options, BDRV_OPT_CACHE_WB,
+                  qbool_from_int(flags & BDRV_O_CACHE_WB));

Urgh, qbool_from_int() doesn't cast the int to bool? :-/



+    }
+    if (!qdict_haskey(options, BDRV_OPT_CACHE_DIRECT)) {
+        qdict_put(options, BDRV_OPT_CACHE_DIRECT,
+                  qbool_from_int(flags & BDRV_O_NOCACHE));
+    }
+    if (!qdict_haskey(options, BDRV_OPT_CACHE_NO_FLUSH)) {
+        qdict_put(options, BDRV_OPT_CACHE_NO_FLUSH,
+                  qbool_from_int(flags & BDRV_O_NO_FLUSH));
+    }
+}
+
  static void bdrv_assign_node_name(BlockDriverState *bs,
                                    const char *node_name,
                                    Error **errp)
@@ -804,6 +852,20 @@ static QemuOptsList bdrv_runtime_opts = {
              .type = QEMU_OPT_STRING,
              .help = "Block driver to use for the node",
          },
+        {
+            .name = BDRV_OPT_CACHE_WB,
+            .type = QEMU_OPT_BOOL,
+            .help = "Enable writeback mode",
+        },
+        {
+            .name = BDRV_OPT_CACHE_DIRECT,
+            .type = QEMU_OPT_BOOL,

Why no .help?

+        },
+        {
+            .name = BDRV_OPT_CACHE_NO_FLUSH,
+            .type = QEMU_OPT_BOOL,
+            .help = "Ignore flush requests",
+        },
          { /* end of list */ }
      },
  };
@@ -907,7 +969,9 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
      bs->drv = drv;
      bs->opaque = g_malloc0(drv->instance_size);
- bs->enable_write_cache = !!(flags & BDRV_O_CACHE_WB);
+    /* Apply cache mode options */
+    update_flags_from_options(&bs->open_flags, opts);
+    bdrv_set_enable_write_cache(bs, bs->open_flags & BDRV_O_CACHE_WB);
/* Open the image, either directly or using a protocol */
      if (drv->bdrv_file_open) {
@@ -1025,6 +1089,9 @@ static int bdrv_fill_options(QDict **options, const char 
**pfilename, int flags,
          *pfilename = filename = NULL;
      }
+ /* Translate cache options from flags into options */
+    update_options_from_flags(*options, flags);
+
      /* Fetch the file name from the options QDict if necessary */
      if (protocol && filename) {
          if (!qdict_haskey(*options, "filename")) {
@@ -1650,12 +1717,22 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
      /*
       * Precedence of options:
       * 1. Explicitly passed in options (highest)
-     * 2. TODO Set in flags (only for top level)
+     * 2. Set in flags (only for top level)
       * 3. Retained from explicitly set options of bs
       * 4. Inherited from parent node
       * 5. Retained from effective options of bs
       */
+ if (!parent_options) {
+        /*
+         * Any setting represented by flags is always updated. If the
+         * corresponding QDict option is set, it takes precedence. Otherwise
+         * the flag is translated into a QDict option. The old setting of bs is
+         * not considered.
+         */
+        update_options_from_flags(options, flags);
+    }
+
      /* Old explicitly set values (don't overwrite by inherited value) */
      old_options = qdict_clone_shallow(bs->explicit_options);
      qdict_join(options, old_options, false);
@@ -1824,6 +1901,8 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
          goto error;
      }
+ update_flags_from_options(&reopen_state->flags, opts);
+
      /* node-name and driver must be unchanged. Put them back into the QDict, 
so
       * that they are checked at the end of this function. */
      value = qemu_opt_get(opts, "node-name");
diff --git a/blockdev.c b/blockdev.c
index 77cbe72..90853aa 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -391,15 +391,12 @@ static BlockBackend *blockdev_init(const char *file, 
QDict *bs_opts,
          }
      }
- if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true)) {
-        bdrv_flags |= BDRV_O_CACHE_WB;
-    }
-    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
-        bdrv_flags |= BDRV_O_NOCACHE;
-    }
-    if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
-        bdrv_flags |= BDRV_O_NO_FLUSH;
-    }
+    /* 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. */
+    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_WB, "on");
+    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
+    qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
#ifdef CONFIG_LINUX_AIO
      if ((buf = qemu_opt_get(opts, "aio")) != NULL) {
@@ -3106,18 +3103,6 @@ QemuOptsList qemu_common_drive_opts = {
              .type = QEMU_OPT_STRING,
              .help = "discard operation (ignore/off, unmap/on)",
          },{
-            .name = BDRV_OPT_CACHE_WB,
-            .type = QEMU_OPT_BOOL,
-            .help = "enables writeback mode for any caches",
-        },{
-            .name = BDRV_OPT_CACHE_DIRECT,
-            .type = QEMU_OPT_BOOL,
-            .help = "enables use of O_DIRECT (bypass the host page cache)",
-        },{
-            .name = BDRV_OPT_CACHE_NO_FLUSH,
-            .type = QEMU_OPT_BOOL,
-            .help = "ignore any flush requests for the device",
-        },{
              .name = "aio",
              .type = QEMU_OPT_STRING,
              .help = "host AIO implementation (threads, native)",

Question: bdrv_set_enable_write_cache() changes bs->open_flags so the option (BDRV_O_CACHE_WB) is preserved beyond a reopen (according to the comment there). Is this still the case? Or should it instead modify bs->options now?

Max



reply via email to

[Prev in Thread] Current Thread [Next in Thread]