[Qemu-block] [PATCH] block: Fix snapshot=on cache modes

[Kevin Wolf]

Since commit 91a097e, we end up with a somewhat weird cache mode
configuration with snapshot=on: The commit broke the cache mode
inheritance for the snapshot overlay so that it is opened as
writethrough instead of unsafe now. The following bdrv_append() call to
put it on top of the tree swaps the WCE flag with the snapshot's backing
file (i.e. the originally given file), so what we eventually get is
cache=writeback on the temporary overlay and
cache=writethrough,cache.no-flush=on on the real image file.

This patch changes things so that the temporary overlay gets
cache=unsafe again like it used to, and the real images get whatever the
user specified. This means that cache.direct is now respected even with
snapshot=on, and in the case of committing changes, the final flush is
no longer ignored except explicitly requested by the user.

Signed-off-by: Kevin Wolf <address@hidden>
---
 block.c               | 34 ++++++++++++++++++++++++----------
 blockdev.c            |  7 -------
 include/block/block.h |  1 -
 3 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index ba24b8e..e3fe8ed 100644
--- a/block.c
+++ b/block.c
@@ -687,13 +687,19 @@ int bdrv_parse_cache_flags(const char *mode, int *flags)
 }
 
 /*
- * Returns the flags that a temporary snapshot should get, based on the
- * originally requested flags (the originally requested image will have flags
- * like a backing file)
+ * Returns the options and flags that a temporary snapshot should get, based on
+ * the originally requested flags (the originally requested image will have
+ * flags like a backing file)
  */
-static int bdrv_temp_snapshot_flags(int flags)
+static void bdrv_temp_snapshot_options(int *child_flags, QDict *child_options,
+                                       int parent_flags, QDict *parent_options)
 {
-    return (flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY;
+    *child_flags = (parent_flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPORARY;
+
+    /* For temporary files, unconditional cache=unsafe is fine */
+    qdict_set_default_str(child_options, BDRV_OPT_CACHE_WB, "on");
+    qdict_set_default_str(child_options, BDRV_OPT_CACHE_DIRECT, "off");
+    qdict_set_default_str(child_options, BDRV_OPT_CACHE_NO_FLUSH, "on");
 }
 
 /*
@@ -1424,13 +1430,13 @@ done:
     return c;
 }
 
-int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
+static int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags,
+                                     QDict *snapshot_options, Error **errp)
 {
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);
     int64_t total_size;
     QemuOpts *opts = NULL;
-    QDict *snapshot_options;
     BlockDriverState *bs_snapshot;
     Error *local_err = NULL;
     int ret;
@@ -1465,7 +1471,6 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int 
flags, Error **errp)
     }
 
     /* Prepare a new options QDict for the temporary file */
-    snapshot_options = qdict_new();
     qdict_put(snapshot_options, "file.driver",
               qstring_from_str("file"));
     qdict_put(snapshot_options, "file.filename",
@@ -1477,6 +1482,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int 
flags, Error **errp)
 
     ret = bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options,
                     flags, &local_err);
+    snapshot_options = NULL;
     if (ret < 0) {
         error_propagate(errp, local_err);
         goto out;
@@ -1485,6 +1491,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int 
flags, Error **errp)
     bdrv_append(bs_snapshot, bs);
 
 out:
+    QDECREF(snapshot_options);
     g_free(tmp_filename);
     return ret;
 }
@@ -1516,6 +1523,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
     const char *drvname;
     const char *backing;
     Error *local_err = NULL;
+    QDict *snapshot_options = NULL;
     int snapshot_flags = 0;
 
     assert(pbs);
@@ -1607,7 +1615,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
             flags |= BDRV_O_ALLOW_RDWR;
         }
         if (flags & BDRV_O_SNAPSHOT) {
-            snapshot_flags = bdrv_temp_snapshot_flags(flags);
+            snapshot_options = qdict_new();
+            bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
+                                       flags, options);
             bdrv_backing_options(&flags, options, flags, options);
         }
 
@@ -1709,7 +1719,9 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
     /* For snapshot=on, create a temporary qcow2 overlay. bs points to the
      * temporary snapshot afterwards. */
     if (snapshot_flags) {
-        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, &local_err);
+        ret = bdrv_append_temp_snapshot(bs, snapshot_flags, snapshot_options,
+                                        &local_err);
+        snapshot_options = NULL;
         if (local_err) {
             goto close_and_fail;
         }
@@ -1721,6 +1733,7 @@ fail:
     if (file != NULL) {
         bdrv_unref_child(bs, file);
     }
+    QDECREF(snapshot_options);
     QDECREF(bs->explicit_options);
     QDECREF(bs->options);
     QDECREF(options);
@@ -1743,6 +1756,7 @@ close_and_fail:
     } else {
         bdrv_unref(bs);
     }
+    QDECREF(snapshot_options);
     QDECREF(options);
     if (local_err) {
         error_propagate(errp, local_err);
diff --git a/blockdev.c b/blockdev.c
index ced3993..4508798 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -593,13 +593,6 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
         qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_DIRECT, "off");
         qdict_set_default_str(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, "off");
 
-        if (snapshot) {
-            /* always use cache=unsafe with snapshot */
-            qdict_put(bs_opts, BDRV_OPT_CACHE_WB, qstring_from_str("on"));
-            qdict_put(bs_opts, BDRV_OPT_CACHE_DIRECT, qstring_from_str("off"));
-            qdict_put(bs_opts, BDRV_OPT_CACHE_NO_FLUSH, 
qstring_from_str("on"));
-        }
-
         if (runstate_check(RUN_STATE_INMIGRATE)) {
             bdrv_flags |= BDRV_O_INACTIVE;
         }
diff --git a/include/block/block.h b/include/block/block.h
index 1c4f4d8..3900c4d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -215,7 +215,6 @@ BdrvChild *bdrv_open_child(const char *filename,
 void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd);
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);
-int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp);
 int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-- 
1.8.3.1