diff mbox

[16/18] blockdev: Split up 'cache' option

Message ID 1374584606-5615-17-git-send-email-kwolf@redhat.com
State New
Headers show

Commit Message

Kevin Wolf July 23, 2013, 1:03 p.m. UTC 
The old 'cache' option really encodes three different boolean flags into
a cache mode name, without providing all combinations. Make them three
separate options instead and translate the old option to the new ones
for drive_init().

The specific boolean options take precedence if the old cache option is
specified as well, so the following options are equivalent:

-drive file=x,cache=none,cache.no-flush=true
-drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 47 insertions(+), 10 deletions(-)

Comments

Eric Blake July 26, 2013, 4:30 p.m. UTC | #1 
On 07/23/2013 07:03 AM, Kevin Wolf wrote:
> The old 'cache' option really encodes three different boolean flags into
> a cache mode name, without providing all combinations. Make them three
> separate options instead and translate the old option to the new ones
> for drive_init().
> 
> The specific boolean options take precedence if the old cache option is
> specified as well, so the following options are equivalent:
> 
> -drive file=x,cache=none,cache.no-flush=true
> -drive file=x,cache.writeback=true,cache.direct=true,cache.no-flush=true
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  blockdev.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 47 insertions(+), 10 deletions(-)
> 

> +++ b/blockdev.c
> @@ -452,12 +452,15 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
>          }
>      }
>  
> -    bdrv_flags |= BDRV_O_CACHE_WB;
> -    if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
> -        if (bdrv_parse_cache_flags(buf, &bdrv_flags) != 0) {

The old code calls bdrv_parse_cache_flags() with non-zero bdrv_flags.

> @@ -751,6 +756,31 @@ DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
>  
>      qemu_opt_rename(all_opts, "readonly", "read-only");
>  
> +    value = qemu_opt_get(all_opts, "cache");
> +    if (value) {
> +        int flags = 0;
> +
> +        if (bdrv_parse_cache_flags(value, &flags) != 0) {

The new code calls it with flags starting at 0.  But looking at
bdrv_parse_cache_flags(), the first thing that code did was zero out
BDRV_O_CACHE_MASK, which includes BDRV_O_CACHE_WB, so I think that in
reality you still end up with the same bits set at the end of that parse
call.  Phew - that means you are actually getting rid of a dead |=
assignment.  [And you proved that the block code is a twisty maze of
back-compat hacks.]

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

Patch

diff --git a/blockdev.c b/blockdev.c
index 3b05e29..ef55b1a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -452,12 +452,15 @@  static DriveInfo *blockdev_init(QemuOpts *all_opts,
         }
     }
 
-    bdrv_flags |= BDRV_O_CACHE_WB;
-    if ((buf = qemu_opt_get(opts, "cache")) != NULL) {
-        if (bdrv_parse_cache_flags(buf, &bdrv_flags) != 0) {
-            error_report("invalid cache option");
-            return NULL;
-        }
+    bdrv_flags = 0;
+    if (qemu_opt_get_bool(opts, "cache.writeback", true)) {
+        bdrv_flags |= BDRV_O_CACHE_WB;
+    }
+    if (qemu_opt_get_bool(opts, "cache.direct", false)) {
+        bdrv_flags |= BDRV_O_NOCACHE;
+    }
+    if (qemu_opt_get_bool(opts, "cache.no-flush", true)) {
+        bdrv_flags |= BDRV_O_NO_FLUSH;
     }
 
 #ifdef CONFIG_LINUX_AIO
@@ -740,6 +743,8 @@  static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to)
 
 DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 {
+    const char *value;
+
     /* Change legacy command line options into QMP ones */
     qemu_opt_rename(all_opts, "iops", "throttling.iops-total");
     qemu_opt_rename(all_opts, "iops_rd", "throttling.iops-read");
@@ -751,6 +756,31 @@  DriveInfo *drive_init(QemuOpts *all_opts, BlockInterfaceType block_default_type)
 
     qemu_opt_rename(all_opts, "readonly", "read-only");
 
+    value = qemu_opt_get(all_opts, "cache");
+    if (value) {
+        int flags = 0;
+
+        if (bdrv_parse_cache_flags(value, &flags) != 0) {
+            error_report("invalid cache option");
+            return NULL;
+        }
+
+        /* Specific options take precedence */
+        if (!qemu_opt_get(all_opts, "cache.writeback")) {
+            qemu_opt_set_bool(all_opts, "cache.writeback",
+                              !!(flags & BDRV_O_CACHE_WB));
+        }
+        if (!qemu_opt_get(all_opts, "cache.direct")) {
+            qemu_opt_set_bool(all_opts, "cache.direct",
+                              !!(flags & BDRV_O_NOCACHE));
+        }
+        if (!qemu_opt_get(all_opts, "cache.no-flush")) {
+            qemu_opt_set_bool(all_opts, "cache.no-flush",
+                              !!(flags & BDRV_O_NO_FLUSH));
+        }
+        qemu_opt_unset(all_opts, "cache");
+    }
+
     return blockdev_init(all_opts, block_default_type);
 }
 
@@ -1850,10 +1880,17 @@  QemuOptsList qemu_common_drive_opts = {
             .type = QEMU_OPT_STRING,
             .help = "discard operation (ignore/off, unmap/on)",
         },{
-            .name = "cache",
-            .type = QEMU_OPT_STRING,
-            .help = "host cache usage (none, writeback, writethrough, "
-                    "directsync, unsafe)",
+            .name = "cache.writeback",
+            .type = QEMU_OPT_BOOL,
+            .help = "enables writeback mode for any caches",
+        },{
+            .name = "cache.direct",
+            .type = QEMU_OPT_BOOL,
+            .help = "enables use of O_DIRECT (bypass the host page cache)",
+        },{
+            .name = "cache.no-flush",
+            .type = QEMU_OPT_BOOL,
+            .help = "ignore any flush requests for the device",
         },{
             .name = "aio",
             .type = QEMU_OPT_STRING,