[Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options

[Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options

[Kevin Wolf]

This is part two of what I had sent earlier as "[PATCH 00/34] block:
Cache mode for children, reopen overhaul and more". Most of the patches
were actually already reviewed in v1.

Apart from a few addressed review comments, there are no functional
changes compared to v1. Some rebasing was necessary; also the
qemu-iotests cases are new.

Kevin Wolf (13):
  block: Allow specifying driver-specific options to reopen
  qemu-io: Remove duplicate 'open' error message
  qemu-io: Add command 'reopen'
  qcow2: Improve error message
  qcow2: Factor out qcow2_update_options()
  qcow2: Move qcow2_update_options() call up
  qcow2: Move rest of option handling to qcow2_update_options()
  qcow2: Leave s unchanged on qcow2_update_options() failure
  qcow2: Fix memory leak in qcow2_update_options() error path
  qcow2: Make qcow2_update_options() suitable for transactions
  qcow2: Support updating driver-specific options in reopen
  qemu-iotests: Reopen qcow2 with lazy-refcounts change
  qemu-iotests: More qcow2 reopen tests

 block.c                    |  42 +++++-
 block/commit.c             |   4 +-
 block/qcow2.c              | 362 ++++++++++++++++++++++++++++++---------------
 include/block/block.h      |   4 +-
 qemu-io-cmds.c             |  90 +++++++++++
 qemu-io.c                  |   1 -
 tests/qemu-iotests/039     |  27 ++++
 tests/qemu-iotests/039.out |  18 +++
 tests/qemu-iotests/137     | 143 ++++++++++++++++++
 tests/qemu-iotests/137.out |  43 ++++++
 tests/qemu-iotests/group   |   1 +
 11 files changed, 612 insertions(+), 123 deletions(-)
 create mode 100755 tests/qemu-iotests/137
 create mode 100644 tests/qemu-iotests/137.out

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 01/13] block: Allow specifying driver-specific options to reopen

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 42 +++++++++++++++++++++++++++++++++++++++---
 block/commit.c        |  4 ++--
 include/block/block.h |  4 +++-
 3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index 090923c..a2cd9b8 100644
--- a/block.c
+++ b/block.c
@@ -1660,6 +1660,9 @@ typedef struct BlockReopenQueueEntry {
  *
  * bs is the BlockDriverState to add to the reopen queue.
  *
+ * options contains the changed options for the associated bs
+ * (the BlockReopenQueue takes ownership)
+ *
  * flags contains the open flags for the associated bs
  *
  * returns a pointer to bs_queue, which is either the newly allocated
@@ -1667,18 +1670,28 @@ typedef struct BlockReopenQueueEntry {
  *
  */
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-                                    BlockDriverState *bs, int flags)
+                                    BlockDriverState *bs,
+                                    QDict *options, int flags)
 {
     assert(bs != NULL);
 
     BlockReopenQueueEntry *bs_entry;
     BdrvChild *child;
+    QDict *old_options;
 
     if (bs_queue == NULL) {
         bs_queue = g_new0(BlockReopenQueue, 1);
         QSIMPLEQ_INIT(bs_queue);
     }
 
+    if (!options) {
+        options = qdict_new();
+    }
+
+    old_options = qdict_clone_shallow(bs->options);
+    qdict_join(options, old_options, false);
+    QDECREF(old_options);
+
     /* bdrv_open() masks this flag out */
     flags &= ~BDRV_O_PROTOCOL;
 
@@ -1690,13 +1703,15 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
         }
 
         child_flags = child->role->inherit_flags(flags);
-        bdrv_reopen_queue(bs_queue, child->bs, child_flags);
+        /* TODO Pass down child flags (backing.*, extents.*, ...) */
+        bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
     }
 
     bs_entry = g_new0(BlockReopenQueueEntry, 1);
     QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
 
     bs_entry->state.bs = bs;
+    bs_entry->state.options = options;
     bs_entry->state.flags = flags;
 
     return bs_queue;
@@ -1749,6 +1764,7 @@ cleanup:
         if (ret && bs_entry->prepared) {
             bdrv_reopen_abort(&bs_entry->state);
         }
+        QDECREF(bs_entry->state.options);
         g_free(bs_entry);
     }
     g_free(bs_queue);
@@ -1761,7 +1777,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
 {
     int ret = -1;
     Error *local_err = NULL;
-    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+    BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
 
     ret = bdrv_reopen_multiple(queue, &local_err);
     if (local_err != NULL) {
@@ -1837,6 +1853,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
         goto error;
     }
 
+    /* Options that are not handled are only okay if they are unchanged
+     * compared to the old state. It is expected that some options are only
+     * used for the initial open, but not reopen (e.g. filename) */
+    if (qdict_size(reopen_state->options)) {
+        const QDictEntry *entry = qdict_first(reopen_state->options);
+
+        do {
+            QString *new_obj = qobject_to_qstring(entry->value);
+            const char *new = qstring_get_str(new_obj);
+            const char *old = qdict_get_try_str(reopen_state->bs->options,
+                                                entry->key);
+
+            if (!old || strcmp(new, old)) {
+                error_setg(errp, "Cannot change the option '%s'", entry->key);
+                ret = -EINVAL;
+                goto error;
+            }
+        } while ((entry = qdict_next(reopen_state->options, entry)));
+    }
+
     ret = 0;
 
 error:
diff --git a/block/commit.c b/block/commit.c
index 7312a5b..d12e26f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,11 +236,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
 
     /* convert base & overlay_bs to r/w, if necessary */
     if (!(orig_base_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+        reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
                                          orig_base_flags | BDRV_O_RDWR);
     }
     if (!(orig_overlay_flags & BDRV_O_RDWR)) {
-        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+        reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
                                          orig_overlay_flags | BDRV_O_RDWR);
     }
     if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index 608cd4e..555a2ce 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -147,6 +147,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
 typedef struct BDRVReopenState {
     BlockDriverState *bs;
     int flags;
+    QDict *options;
     void *opaque;
 } BDRVReopenState;
 
@@ -221,7 +222,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
               const char *reference, QDict *options, int flags,
               BlockDriver *drv, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-                                    BlockDriverState *bs, int flags);
+                                    BlockDriverState *bs,
+                                    QDict *options, int flags);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
 int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
 int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 02/13] qemu-io: Remove duplicate 'open' error message

[Kevin Wolf]

qemu_opts_parse_noisily() already prints an error message with the exact
reason why the parsing failed. No need to add another less specific one.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/qemu-io.c b/qemu-io.c
index f1e3a67..269f17c 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -156,7 +156,6 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
             break;
         case 'o':
             if (!qemu_opts_parse_noisily(&empty_opts, optarg, false)) {
-                printf("could not parse option list -- %s\n", optarg);
                 qemu_opts_reset(&empty_opts);
                 return 0;
             }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 03/13] qemu-io: Add command 'reopen'

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 qemu-io-cmds.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 53477e1..d6572a8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1979,6 +1979,95 @@ static const cmdinfo_t map_cmd = {
        .oneline        = "prints the allocated areas of a file",
 };
 
+static void reopen_help(void)
+{
+    printf(
+"\n"
+" Changes the open options of an already opened image\n"
+"\n"
+" Example:\n"
+" 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 image\n"
+"\n"
+" -r, -- Reopen the image read-only\n"
+" -c, -- Change the cache mode to the given value\n"
+" -o, -- Changes block driver options (cf. 'open' command)\n"
+"\n");
+}
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv);
+
+static QemuOptsList reopen_opts = {
+    .name = "reopen",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(reopen_opts.head),
+    .desc = {
+        /* no elements => accept any params */
+        { /* end of list */ }
+    },
+};
+
+static const cmdinfo_t reopen_cmd = {
+       .name           = "reopen",
+       .argmin         = 0,
+       .argmax         = -1,
+       .cfunc          = reopen_f,
+       .args           = "[-r] [-c cache] [-o options]",
+       .oneline        = "reopens an image with new options",
+       .help           = reopen_help,
+};
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv)
+{
+    BlockDriverState *bs = blk_bs(blk);
+    QemuOpts *qopts;
+    QDict *opts;
+    int c;
+    int flags = bs->open_flags;
+
+    BlockReopenQueue *brq;
+    Error *local_err = NULL;
+
+    while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+        switch (c) {
+        case 'c':
+            if (bdrv_parse_cache_flags(optarg, &flags) < 0) {
+                error_report("Invalid cache option: %s", optarg);
+                return 0;
+            }
+            break;
+        case 'o':
+            if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
+                qemu_opts_reset(&reopen_opts);
+                return 0;
+            }
+            break;
+        case 'r':
+            flags &= ~BDRV_O_RDWR;
+            break;
+        default:
+            qemu_opts_reset(&reopen_opts);
+            return qemuio_command_usage(&reopen_cmd);
+        }
+    }
+
+    if (optind != argc) {
+        qemu_opts_reset(&reopen_opts);
+        return qemuio_command_usage(&reopen_cmd);
+    }
+
+    qopts = qemu_opts_find(&reopen_opts, NULL);
+    opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+    qemu_opts_reset(&reopen_opts);
+
+    brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+    bdrv_reopen_multiple(brq, &local_err);
+    if (local_err) {
+        error_report_err(local_err);
+    }
+
+    return 0;
+}
+
 static int break_f(BlockBackend *blk, int argc, char **argv)
 {
     int ret;
@@ -2266,6 +2355,7 @@ static void __attribute((constructor)) init_qemuio_commands(void)
     qemuio_add_command(&discard_cmd);
     qemuio_add_command(&alloc_cmd);
     qemuio_add_command(&map_cmd);
+    qemuio_add_command(&reopen_cmd);
     qemuio_add_command(&break_cmd);
     qemuio_add_command(&remove_break_cmd);
     qemuio_add_command(&resume_cmd);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 04/13] qcow2: Improve error message

[Kevin Wolf]

Eric says that "any" sounds better than "either", and my non-native
feeling says the same, so let's change it.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 76c331b..e120ed3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -971,7 +971,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         overlap_check_template = QCOW2_OL_ALL;
     } else {
         error_setg(errp, "Unsupported value '%s' for qcow2 option "
-                   "'overlap-check'. Allowed are either of the following: "
+                   "'overlap-check'. Allowed are any of the following: "
                    "none, constant, cached, all", opt_overlap_check);
         ret = -EINVAL;
         goto fail;
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 05/13] qcow2: Factor out qcow2_update_options()

[Kevin Wolf]

Eventually we want to be able to change options at runtime. As a first
step towards that goal, separate some option handling code from the
general initialisation code in qcow2_open().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 76 insertions(+), 59 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index e120ed3..dc07d35 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,6 +541,80 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     }
 }
 
+static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+                                int flags, Error **errp)
+{
+    BDRVQcowState *s = bs->opaque;
+    const char *opt_overlap_check, *opt_overlap_check_template;
+    int overlap_check_template = 0;
+    int i;
+    int ret;
+
+    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+        (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+
+    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+                          flags & BDRV_O_UNMAP);
+    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
+
+    opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
+    opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
+    if (opt_overlap_check_template && opt_overlap_check &&
+        strcmp(opt_overlap_check_template, opt_overlap_check))
+    {
+        error_setg(errp, "Conflicting values for qcow2 options '"
+                   QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
+                   "' ('%s')", opt_overlap_check, opt_overlap_check_template);
+        ret = -EINVAL;
+        goto fail;
+    }
+    if (!opt_overlap_check) {
+        opt_overlap_check = opt_overlap_check_template ?: "cached";
+    }
+
+    if (!strcmp(opt_overlap_check, "none")) {
+        overlap_check_template = 0;
+    } else if (!strcmp(opt_overlap_check, "constant")) {
+        overlap_check_template = QCOW2_OL_CONSTANT;
+    } else if (!strcmp(opt_overlap_check, "cached")) {
+        overlap_check_template = QCOW2_OL_CACHED;
+    } else if (!strcmp(opt_overlap_check, "all")) {
+        overlap_check_template = QCOW2_OL_ALL;
+    } else {
+        error_setg(errp, "Unsupported value '%s' for qcow2 option "
+                   "'overlap-check'. Allowed are any of the following: "
+                   "none, constant, cached, all", opt_overlap_check);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    s->overlap_check = 0;
+    for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
+        /* overlap-check defines a template bitmask, but every flag may be
+         * overwritten through the associated boolean option */
+        s->overlap_check |=
+            qemu_opt_get_bool(opts, overlap_bool_option_names[i],
+                              overlap_check_template & (1 << i)) << i;
+    }
+
+    if (s->use_lazy_refcounts && s->qcow_version < 3) {
+        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+                   "qemu 1.1 compatibility level");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    ret = 0;
+fail:
+    return ret;
+}
+
 static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
                       Error **errp)
 {
@@ -552,8 +626,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
-    const char *opt_overlap_check, *opt_overlap_check_template;
-    int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
@@ -933,69 +1005,14 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
-        (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-
-    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
-                          flags & BDRV_O_UNMAP);
-    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
-    opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
-    opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
-    if (opt_overlap_check_template && opt_overlap_check &&
-        strcmp(opt_overlap_check_template, opt_overlap_check))
-    {
-        error_setg(errp, "Conflicting values for qcow2 options '"
-                   QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
-                   "' ('%s')", opt_overlap_check, opt_overlap_check_template);
-        ret = -EINVAL;
-        goto fail;
-    }
-    if (!opt_overlap_check) {
-        opt_overlap_check = opt_overlap_check_template ?: "cached";
-    }
-
-    if (!strcmp(opt_overlap_check, "none")) {
-        overlap_check_template = 0;
-    } else if (!strcmp(opt_overlap_check, "constant")) {
-        overlap_check_template = QCOW2_OL_CONSTANT;
-    } else if (!strcmp(opt_overlap_check, "cached")) {
-        overlap_check_template = QCOW2_OL_CACHED;
-    } else if (!strcmp(opt_overlap_check, "all")) {
-        overlap_check_template = QCOW2_OL_ALL;
-    } else {
-        error_setg(errp, "Unsupported value '%s' for qcow2 option "
-                   "'overlap-check'. Allowed are any of the following: "
-                   "none, constant, cached, all", opt_overlap_check);
-        ret = -EINVAL;
+    ret = qcow2_update_options(bs, opts, flags, errp);
+    if (ret < 0) {
         goto fail;
     }
 
-    s->overlap_check = 0;
-    for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
-        /* overlap-check defines a template bitmask, but every flag may be
-         * overwritten through the associated boolean option */
-        s->overlap_check |=
-            qemu_opt_get_bool(opts, overlap_bool_option_names[i],
-                              overlap_check_template & (1 << i)) << i;
-    }
-
     qemu_opts_del(opts);
     opts = NULL;
 
-    if (s->use_lazy_refcounts && s->qcow_version < 3) {
-        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
-                   "qemu 1.1 compatibility level");
-        ret = -EINVAL;
-        goto fail;
-    }
-
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 06/13] qcow2: Move qcow2_update_options() call up

[Kevin Wolf]

qcow2_update_options() only updates some variables in BDRVQcowState and
doesn't really depend on other parts of it being initialised yet, so it
can be moved so that it immediately follows the other half of option
handling code in qcow2_open().

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

diff --git a/block/qcow2.c b/block/qcow2.c
index dc07d35..2903481 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -920,6 +920,15 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
+    /* Enable lazy_refcounts according to image and command line options */
+    ret = qcow2_update_options(bs, opts, flags, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    qemu_opts_del(opts);
+    opts = NULL;
+
     s->cluster_cache = g_malloc(s->cluster_size);
     /* one more sector for decompressed data alignment */
     s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS
@@ -1004,15 +1013,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    /* Enable lazy_refcounts according to image and command line options */
-    ret = qcow2_update_options(bs, opts, flags, errp);
-    if (ret < 0) {
-        goto fail;
-    }
-
-    qemu_opts_del(opts);
-    opts = NULL;
-
 #ifdef DEBUG_ALLOC
     {
         BdrvCheckResult result = {0};
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 07/13] qcow2: Move rest of option handling to qcow2_update_options()

[Kevin Wolf]

With this commit, the handling of driver-specific options in
qcow2_open() is completely separated out into qcow2_update_options().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 111 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 56 insertions(+), 55 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2903481..bad377d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,15 +541,65 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     }
 }
 
-static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+static int qcow2_update_options(BlockDriverState *bs, QDict *options,
                                 int flags, Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
+    QemuOpts *opts = NULL;
     const char *opt_overlap_check, *opt_overlap_check_template;
     int overlap_check_template = 0;
+    uint64_t l2_cache_size, refcount_cache_size;
     int i;
+    Error *local_err = NULL;
     int ret;
 
+    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
+    qemu_opts_absorb_qdict(opts, options, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* get L2 table/refcount block cache size from command line options */
+    read_cache_sizes(bs, opts, &l2_cache_size, &refcount_cache_size,
+                     &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    l2_cache_size /= s->cluster_size;
+    if (l2_cache_size < MIN_L2_CACHE_SIZE) {
+        l2_cache_size = MIN_L2_CACHE_SIZE;
+    }
+    if (l2_cache_size > INT_MAX) {
+        error_setg(errp, "L2 cache size too big");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    refcount_cache_size /= s->cluster_size;
+    if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
+        refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
+    }
+    if (refcount_cache_size > INT_MAX) {
+        error_setg(errp, "Refcount cache size too big");
+        ret = -EINVAL;
+        goto fail;
+    }
+
+    /* alloc L2 table/refcount block cache */
+    s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+    s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+    if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+        error_setg(errp, "Could not allocate metadata caches");
+        ret = -ENOMEM;
+        goto fail;
+    }
+
+    /* Enable lazy_refcounts according to image and command line options */
     s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
 
@@ -612,6 +662,9 @@ static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
 
     ret = 0;
 fail:
+    qemu_opts_del(opts);
+    opts = NULL;
+
     return ret;
 }
 
@@ -622,11 +675,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     unsigned int len, i;
     int ret = 0;
     QCowHeader header;
-    QemuOpts *opts = NULL;
     Error *local_err = NULL;
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
-    uint64_t l2_cache_size, refcount_cache_size;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
@@ -874,61 +925,12 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         }
     }
 
-    /* get L2 table/refcount block cache size from command line options */
-    opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    read_cache_sizes(bs, opts, &l2_cache_size, &refcount_cache_size,
-                     &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    l2_cache_size /= s->cluster_size;
-    if (l2_cache_size < MIN_L2_CACHE_SIZE) {
-        l2_cache_size = MIN_L2_CACHE_SIZE;
-    }
-    if (l2_cache_size > INT_MAX) {
-        error_setg(errp, "L2 cache size too big");
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    refcount_cache_size /= s->cluster_size;
-    if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
-        refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
-    }
-    if (refcount_cache_size > INT_MAX) {
-        error_setg(errp, "Refcount cache size too big");
-        ret = -EINVAL;
-        goto fail;
-    }
-
-    /* alloc L2 table/refcount block cache */
-    s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
-    s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
-    if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
-        error_setg(errp, "Could not allocate metadata caches");
-        ret = -ENOMEM;
-        goto fail;
-    }
-
-    /* Enable lazy_refcounts according to image and command line options */
-    ret = qcow2_update_options(bs, opts, flags, errp);
+    /* Parse driver-specific options */
+    ret = qcow2_update_options(bs, options, flags, errp);
     if (ret < 0) {
         goto fail;
     }
 
-    qemu_opts_del(opts);
-    opts = NULL;
-
     s->cluster_cache = g_malloc(s->cluster_size);
     /* one more sector for decompressed data alignment */
     s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS
@@ -1022,7 +1024,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     return ret;
 
  fail:
-    qemu_opts_del(opts);
     g_free(s->unknown_header_fields);
     cleanup_unknown_header_ext(bs);
     qcow2_free_snapshots(bs);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 08/13] qcow2: Leave s unchanged on qcow2_update_options() failure

[Kevin Wolf]

On return, either all new options should be applied to BDRVQcowState (on
success), or all of the old settings should be preserved (on failure).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 52 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 32 insertions(+), 20 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index bad377d..9b67334 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -549,6 +549,9 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     const char *opt_overlap_check, *opt_overlap_check_template;
     int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
+    Qcow2Cache *l2_table_cache;
+    Qcow2Cache *refcount_block_cache;
+    bool use_lazy_refcounts;
     int i;
     Error *local_err = NULL;
     int ret;
@@ -591,28 +594,25 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     }
 
     /* alloc L2 table/refcount block cache */
-    s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
-    s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
-    if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+    l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+    refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+    if (l2_table_cache == NULL || refcount_block_cache == NULL) {
         error_setg(errp, "Could not allocate metadata caches");
         ret = -ENOMEM;
         goto fail;
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+    use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+    if (use_lazy_refcounts && s->qcow_version < 3) {
+        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+                   "qemu 1.1 compatibility level");
+        ret = -EINVAL;
+        goto fail;
+    }
 
-    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
-                          flags & BDRV_O_UNMAP);
-    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
-        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
+    /* Overlap check options */
     opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
     opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
     if (opt_overlap_check_template && opt_overlap_check &&
@@ -644,6 +644,10 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
+    /*
+     * Start updating fields in BDRVQcowState.
+     * After this point no failure is allowed any more.
+     */
     s->overlap_check = 0;
     for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
         /* overlap-check defines a template bitmask, but every flag may be
@@ -653,12 +657,20 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
                               overlap_check_template & (1 << i)) << i;
     }
 
-    if (s->use_lazy_refcounts && s->qcow_version < 3) {
-        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
-                   "qemu 1.1 compatibility level");
-        ret = -EINVAL;
-        goto fail;
-    }
+    s->l2_table_cache = l2_table_cache;
+    s->refcount_block_cache = refcount_block_cache;
+
+    s->use_lazy_refcounts = use_lazy_refcounts;
+
+    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+                          flags & BDRV_O_UNMAP);
+    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+        qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
     ret = 0;
 fail:
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 09/13] qcow2: Fix memory leak in qcow2_update_options() error path

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 9b67334..24acefd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -549,8 +549,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     const char *opt_overlap_check, *opt_overlap_check_template;
     int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
-    Qcow2Cache *l2_table_cache;
-    Qcow2Cache *refcount_block_cache;
+    Qcow2Cache *l2_table_cache = NULL;
+    Qcow2Cache *refcount_block_cache = NULL;
     bool use_lazy_refcounts;
     int i;
     Error *local_err = NULL;
@@ -674,6 +674,14 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
 
     ret = 0;
 fail:
+    if (ret < 0) {
+        if (l2_table_cache) {
+            qcow2_cache_destroy(bs, l2_table_cache);
+        }
+        if (refcount_block_cache) {
+            qcow2_cache_destroy(bs, refcount_block_cache);
+        }
+    }
     qemu_opts_del(opts);
     opts = NULL;
 
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 10/13] qcow2: Make qcow2_update_options() suitable for transactions

[Kevin Wolf]

Before we can allow updating options at runtime with bdrv_reopen(), we
need to split the function into prepare/commit/abort parts.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 34 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 24acefd..b498cf1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,17 +541,24 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
     }
 }
 
-static int qcow2_update_options(BlockDriverState *bs, QDict *options,
-                                int flags, Error **errp)
+typedef struct Qcow2ReopenState {
+    Qcow2Cache *l2_table_cache;
+    Qcow2Cache *refcount_block_cache;
+    bool use_lazy_refcounts;
+    int overlap_check;
+    bool discard_passthrough[QCOW2_DISCARD_MAX];
+} Qcow2ReopenState;
+
+static int qcow2_update_options_prepare(BlockDriverState *bs,
+                                        Qcow2ReopenState *r,
+                                        QDict *options, int flags,
+                                        Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QemuOpts *opts = NULL;
     const char *opt_overlap_check, *opt_overlap_check_template;
     int overlap_check_template = 0;
     uint64_t l2_cache_size, refcount_cache_size;
-    Qcow2Cache *l2_table_cache = NULL;
-    Qcow2Cache *refcount_block_cache = NULL;
-    bool use_lazy_refcounts;
     int i;
     Error *local_err = NULL;
     int ret;
@@ -594,18 +601,18 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     }
 
     /* alloc L2 table/refcount block cache */
-    l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
-    refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
-    if (l2_table_cache == NULL || refcount_block_cache == NULL) {
+    r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+    r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+    if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
         error_setg(errp, "Could not allocate metadata caches");
         ret = -ENOMEM;
         goto fail;
     }
 
     /* Enable lazy_refcounts according to image and command line options */
-    use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+    r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-    if (use_lazy_refcounts && s->qcow_version < 3) {
+    if (r->use_lazy_refcounts && s->qcow_version < 3) {
         error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
                    "qemu 1.1 compatibility level");
         ret = -EINVAL;
@@ -644,46 +651,72 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
         goto fail;
     }
 
-    /*
-     * Start updating fields in BDRVQcowState.
-     * After this point no failure is allowed any more.
-     */
-    s->overlap_check = 0;
+    r->overlap_check = 0;
     for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
         /* overlap-check defines a template bitmask, but every flag may be
          * overwritten through the associated boolean option */
-        s->overlap_check |=
+        r->overlap_check |=
             qemu_opt_get_bool(opts, overlap_bool_option_names[i],
                               overlap_check_template & (1 << i)) << i;
     }
 
-    s->l2_table_cache = l2_table_cache;
-    s->refcount_block_cache = refcount_block_cache;
-
-    s->use_lazy_refcounts = use_lazy_refcounts;
-
-    s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
-    s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
-    s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+    r->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+    r->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+    r->discard_passthrough[QCOW2_DISCARD_REQUEST] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
                           flags & BDRV_O_UNMAP);
-    s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+    r->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
-    s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+    r->discard_passthrough[QCOW2_DISCARD_OTHER] =
         qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
 
     ret = 0;
 fail:
-    if (ret < 0) {
-        if (l2_table_cache) {
-            qcow2_cache_destroy(bs, l2_table_cache);
-        }
-        if (refcount_block_cache) {
-            qcow2_cache_destroy(bs, refcount_block_cache);
-        }
-    }
     qemu_opts_del(opts);
     opts = NULL;
+    return ret;
+}
+
+static void qcow2_update_options_commit(BlockDriverState *bs,
+                                        Qcow2ReopenState *r)
+{
+    BDRVQcowState *s = bs->opaque;
+    int i;
+
+    s->l2_table_cache = r->l2_table_cache;
+    s->refcount_block_cache = r->refcount_block_cache;
+
+    s->overlap_check = r->overlap_check;
+    s->use_lazy_refcounts = r->use_lazy_refcounts;
+
+    for (i = 0; i < QCOW2_DISCARD_MAX; i++) {
+        s->discard_passthrough[i] = r->discard_passthrough[i];
+    }
+}
+
+static void qcow2_update_options_abort(BlockDriverState *bs,
+                                       Qcow2ReopenState *r)
+{
+    if (r->l2_table_cache) {
+        qcow2_cache_destroy(bs, r->l2_table_cache);
+    }
+    if (r->refcount_block_cache) {
+        qcow2_cache_destroy(bs, r->refcount_block_cache);
+    }
+}
+
+static int qcow2_update_options(BlockDriverState *bs, QDict *options,
+                                int flags, Error **errp)
+{
+    Qcow2ReopenState r = {};
+    int ret;
+
+    ret = qcow2_update_options_prepare(bs, &r, options, flags, errp);
+    if (ret >= 0) {
+        qcow2_update_options_commit(bs, &r);
+    } else {
+        qcow2_update_options_abort(bs, &r);
+    }
 
     return ret;
 }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 11/13] qcow2: Support updating driver-specific options in reopen

[Kevin Wolf]

For updating the cache sizes or disabling lazy refcounts there is a bit
more to do than just changing the variables, but otherwise we're all set
for changing options during bdrv_reopen().

Just implement the missing pieces and hook the functions up in
bdrv_reopen().

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 6 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b498cf1..8d52766 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -600,7 +600,24 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
-    /* alloc L2 table/refcount block cache */
+    /* alloc new L2 table/refcount block cache, flush old one */
+    if (s->l2_table_cache) {
+        ret = qcow2_cache_flush(bs, s->l2_table_cache);
+        if (ret) {
+            error_setg_errno(errp, -ret, "Failed to flush the L2 table cache");
+            goto fail;
+        }
+    }
+
+    if (s->refcount_block_cache) {
+        ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+        if (ret) {
+            error_setg_errno(errp, -ret,
+                             "Failed to flush the refcount block cache");
+            goto fail;
+        }
+    }
+
     r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
     r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
     if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
@@ -609,7 +626,7 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
-    /* Enable lazy_refcounts according to image and command line options */
+    /* lazy-refcounts; flush if going from enabled to disabled */
     r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
         (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
     if (r->use_lazy_refcounts && s->qcow_version < 3) {
@@ -619,6 +636,14 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
         goto fail;
     }
 
+    if (s->use_lazy_refcounts && !r->use_lazy_refcounts) {
+        ret = qcow2_mark_clean(bs);
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "Failed to disable lazy refcounts");
+            goto fail;
+        }
+    }
+
     /* Overlap check options */
     opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
     opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
@@ -683,6 +708,12 @@ static void qcow2_update_options_commit(BlockDriverState *bs,
     BDRVQcowState *s = bs->opaque;
     int i;
 
+    if (s->l2_table_cache) {
+        qcow2_cache_destroy(bs, s->l2_table_cache);
+    }
+    if (s->refcount_block_cache) {
+        qcow2_cache_destroy(bs, s->refcount_block_cache);
+    }
     s->l2_table_cache = r->l2_table_cache;
     s->refcount_block_cache = r->refcount_block_cache;
 
@@ -1137,26 +1168,52 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
     return 0;
 }
 
-/* We have no actual commit/abort logic for qcow2, but we need to write out any
- * unwritten data if we reopen read-only. */
 static int qcow2_reopen_prepare(BDRVReopenState *state,
                                 BlockReopenQueue *queue, Error **errp)
 {
+    Qcow2ReopenState *r;
     int ret;
 
+    r = g_new0(Qcow2ReopenState, 1);
+    state->opaque = r;
+
+    ret = qcow2_update_options_prepare(state->bs, r, state->options,
+                                       state->flags, errp);
+    if (ret < 0) {
+        goto fail;
+    }
+
+    /* We need to write out any unwritten data if we reopen read-only. */
     if ((state->flags & BDRV_O_RDWR) == 0) {
         ret = bdrv_flush(state->bs);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
 
         ret = qcow2_mark_clean(state->bs);
         if (ret < 0) {
-            return ret;
+            goto fail;
         }
     }
 
     return 0;
+
+fail:
+    qcow2_update_options_abort(state->bs, r);
+    g_free(r);
+    return ret;
+}
+
+static void qcow2_reopen_commit(BDRVReopenState *state)
+{
+    qcow2_update_options_commit(state->bs, state->opaque);
+    g_free(state->opaque);
+}
+
+static void qcow2_reopen_abort(BDRVReopenState *state)
+{
+    qcow2_update_options_abort(state->bs, state->opaque);
+    g_free(state->opaque);
 }
 
 static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
@@ -3013,6 +3070,8 @@ BlockDriver bdrv_qcow2 = {
     .bdrv_open          = qcow2_open,
     .bdrv_close         = qcow2_close,
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
+    .bdrv_reopen_commit   = qcow2_reopen_commit,
+    .bdrv_reopen_abort    = qcow2_reopen_abort,
     .bdrv_create        = qcow2_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/039     | 27 +++++++++++++++++++++++++++
 tests/qemu-iotests/039.out | 18 ++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 617f397..9e9b379 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -147,6 +147,33 @@ $PYTHON qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
 _check_test_img
 TEST_IMG="$TEST_IMG".base _check_test_img
 
+echo
+echo "== Changing lazy_refcounts setting at runtime =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=off"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=on" \
+         -c "write -P 0x5a 0 512" \
+         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+    | _filter_qemu_io
+
+# The dirty bit must be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=off" \
+         -c "write -P 0x5a 0 512" \
+         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+    | _filter_qemu_io
+
+# The dirty bit must not be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
 
 # success, all done
 echo "*** done"
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index b055670..39859b2 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -74,4 +74,22 @@ incompatible_features     0x0
 incompatible_features     0x0
 No errors were found on the image.
 No errors were found on the image.
+
+== Changing lazy_refcounts setting at runtime ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features     0x1
+ERROR cluster 5 refcount=0 reference=1
+ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
+
+2 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features     0x0
+No errors were found on the image.
 *** done
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/137     | 143 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/137.out |  43 ++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 187 insertions(+)
 create mode 100755 tests/qemu-iotests/137
 create mode 100644 tests/qemu-iotests/137.out

diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
new file mode 100755
index 0000000..ad52f71
--- /dev/null
+++ b/tests/qemu-iotests/137
@@ -0,0 +1,143 @@
+#!/bin/bash
+#
+# Test qcow2 reopen
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+_make_test_img 64M
+
+echo === Try setting valid values for all options ===
+echo
+
+# Try all options and then check that all of the basic I/O operations still
+# work on this image.
+$QEMU_IO \
+    -c "reopen -o lazy-refcounts=on,pass-discard-request=on" \
+    -c "reopen -o lazy-refcounts=off,pass-discard-request=off" \
+    -c "reopen -o pass-discard-snapshot=on,pass-discard-other=on" \
+    -c "reopen -o pass-discard-snapshot=off,pass-discard-other=off" \
+    -c "reopen -o overlap-check=all" \
+    -c "reopen -o overlap-check=none" \
+    -c "reopen -o overlap-check=cached" \
+    -c "reopen -o overlap-check=constant" \
+    -c "reopen -o overlap-check.template=all" \
+    -c "reopen -o overlap-check.template=none" \
+    -c "reopen -o overlap-check.template=cached" \
+    -c "reopen -o overlap-check.template=constant" \
+    -c "reopen -o overlap-check.main-header=on" \
+    -c "reopen -o overlap-check.main-header=off" \
+    -c "reopen -o overlap-check.active-l1=on" \
+    -c "reopen -o overlap-check.active-l1=off" \
+    -c "reopen -o overlap-check.active-l2=on" \
+    -c "reopen -o overlap-check.active-l2=off" \
+    -c "reopen -o overlap-check.refcount-table=on" \
+    -c "reopen -o overlap-check.refcount-table=off" \
+    -c "reopen -o overlap-check.refcount-block=on" \
+    -c "reopen -o overlap-check.refcount-block=off" \
+    -c "reopen -o overlap-check.snapshot-table=on" \
+    -c "reopen -o overlap-check.snapshot-table=off" \
+    -c "reopen -o overlap-check.inactive-l1=on" \
+    -c "reopen -o overlap-check.inactive-l1=off" \
+    -c "reopen -o overlap-check.inactive-l2=on" \
+    -c "reopen -o overlap-check.inactive-l2=off" \
+    -c "reopen -o cache-size=1M" \
+    -c "reopen -o l2-cache-size=512k" \
+    -c "reopen -o refcount-cache-size=128k" \
+    \
+    -c "write -P 55 0 32M" \
+    -c "read -P 55 0 32M" \
+    -c "discard 0 32M" \
+    -c "write -z 0 32M" \
+    -c "read -P 0 0 32M" \
+    \
+    "$TEST_IMG" | _filter_qemu_io
+
+
+echo
+echo === Try setting some invalid values ===
+echo
+
+$QEMU_IO \
+    -c "reopen -o lazy-refcounts=42" \
+    -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
+    -c "reopen -o cache-size=1M,l2-cache-size=2M" \
+    -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
+    -c "reopen -o l2-cache-size=256T" \
+    -c "reopen -o l2-cache-size=64G" \
+    -c "reopen -o refcount-cache-size=256T" \
+    -c "reopen -o refcount-cache-size=64G" \
+    -c "reopen -o overlap-check=constant,overlap-check.template=all" \
+    -c "reopen -o overlap-check=blubb" \
+    -c "reopen -o overlap-check.template=blubb" \
+    "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo === Test transaction semantics ===
+echo
+
+# Whether lazy-refcounts was actually enabled can easily be tested: Check if
+# the dirty bit is set after a crash
+$QEMU_IO \
+    -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
+    -c "write -P 0x5a 0 512" \
+    -c "sigraise $(kill -l KILL)" \
+    "$TEST_IMG" 2>&1 | _filter_qemu_io
+
+# The dirty bit must not be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Similarly we can test whether corruption detection has been enabled:
+# Create L1/L2, overwrite first entry in refcount block, allocate something.
+# Disabling the checks should fail, so the corruption must be detected.
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00"
+$QEMU_IO \
+    -c "reopen -o overlap-check=none,lazy-refcounts=42" \
+    -c "write 64k 64k" \
+    "$TEST_IMG" 2>&1 | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
new file mode 100644
index 0000000..b1a1565
--- /dev/null
+++ b/tests/qemu-iotests/137.out
@@ -0,0 +1,43 @@
+QA output created by 137
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+=== Try setting valid values for all options ===
+
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Try setting some invalid values ===
+
+Parameter 'lazy-refcounts' expects 'on' or 'off'
+cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+l2-cache-size may not exceed cache-size
+refcount-cache-size may not exceed cache-size
+L2 cache size too big
+Could not allocate metadata caches
+L2 cache size too big
+Could not allocate metadata caches
+Conflicting values for qcow2 options 'overlap-check' ('constant') and 'overlap-check.template' ('all')
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+
+=== Test transaction semantics ===
+
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features     0x0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Parameter 'lazy-refcounts' expects 'on' or 'off'
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with qcow2_header); further corruption events will be suppressed
+write failed: Input/output error
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c430b6c..3a6a8f0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -134,3 +134,4 @@
 132 rw auto quick
 134 rw auto quick
 135 rw auto
+137 rw auto
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2 02/13] qemu-io: Remove duplicate 'open' error message

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> qemu_opts_parse_noisily() already prints an error message with the exact
> reason why the parsing failed. No need to add another less specific one.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 03/13] qemu-io: Add command 'reopen'

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  qemu-io-cmds.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 04/13] qcow2: Improve error message

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> Eric says that "any" sounds better than "either", and my non-native
> feeling says the same, so let's change it.
> 
> Suggested-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 11/13] qcow2: Support updating driver-specific options in reopen

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> For updating the cache sizes or disabling lazy refcounts there is a bit
> more to do than just changing the variables, but otherwise we're all set
> for changing options during bdrv_reopen().
> 
> Just implement the missing pieces and hook the functions up in
> bdrv_reopen().
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 65 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/039     | 27 +++++++++++++++++++++++++++
>  tests/qemu-iotests/039.out | 18 ++++++++++++++++++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 617f397..9e9b379 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -147,6 +147,33 @@ $PYTHON qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
>  _check_test_img
>  TEST_IMG="$TEST_IMG".base _check_test_img
>  
> +echo
> +echo "== Changing lazy_refcounts setting at runtime =="
> +
> +IMGOPTS="compat=1.1,lazy_refcounts=off"
> +_make_test_img $size
> +
> +$QEMU_IO -c "reopen -o lazy-refcounts=on" \
> +         -c "write -P 0x5a 0 512" \
> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +    | _filter_qemu_io
> +
> +# The dirty bit must be set
> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> +_check_test_img
> +
> +IMGOPTS="compat=1.1,lazy_refcounts=on"
> +_make_test_img $size
> +
> +$QEMU_IO -c "reopen -o lazy-refcounts=off" \
> +         -c "write -P 0x5a 0 512" \

Maybe it would be nicer to test this the other way around, write first
and then disable lazy refcounts in order to test whether the refcounts
are actually flushed during the reopen.

Either way:

Reviewed-by: Max Reitz <mreitz@redhat.com>

> +         -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> +    | _filter_qemu_io
> +
> +# The dirty bit must not be set
> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> +_check_test_img
> +
>  
>  # success, all done
>  echo "*** done"
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index b055670..39859b2 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -74,4 +74,22 @@ incompatible_features     0x0
>  incompatible_features     0x0
>  No errors were found on the image.
>  No errors were found on the image.
> +
> +== Changing lazy_refcounts setting at runtime ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
> +incompatible_features     0x1
> +ERROR cluster 5 refcount=0 reference=1
> +ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> +
> +2 errors were found on the image.
> +Data may be corrupted, or further writes to the image may corrupt it.
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Killed                  ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
> +incompatible_features     0x0
> +No errors were found on the image.
>  *** done
> 



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

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests

[Max Reitz]

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

On 04.09.2015 19:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  tests/qemu-iotests/137     | 143 +++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/137.out |  43 ++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  3 files changed, 187 insertions(+)
>  create mode 100755 tests/qemu-iotests/137
>  create mode 100644 tests/qemu-iotests/137.out
> 
> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> new file mode 100755
> index 0000000..ad52f71
> --- /dev/null
> +++ b/tests/qemu-iotests/137


[snip]

> +echo
> +echo === Try setting some invalid values ===
> +echo
> +
> +$QEMU_IO \
> +    -c "reopen -o lazy-refcounts=42" \
> +    -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> +    -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> +    -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> +    -c "reopen -o l2-cache-size=256T" \
> +    -c "reopen -o l2-cache-size=64G" \

Testing this seems risky. The rest looks fine, but I'm not sure whether
this is really needed so much that we should risk the test failing on
some machines, especially in the future.

Or actually even on my machine:

$ touch ~/tmp/swap
$ truncate ~/tmp/swap --size 128G
$ fallocate -l 128G ~/tmp/swap
# mkswap ~maxx/tmp/swap
# swapon ~max/tmp/swap

$ cd /to/qemu/build
$ ./qemu-io -c 'reopen -o l2-cache-size=64G' test.qcow2
$

(Running the test itself is apparently a bad idea, because then for some
reason Linux thinks I actually want to use the swap and everything slows
down to a crawl)

Max

> +    -c "reopen -o refcount-cache-size=256T" \
> +    -c "reopen -o refcount-cache-size=64G" \
> +    -c "reopen -o overlap-check=constant,overlap-check.template=all" \
> +    -c "reopen -o overlap-check=blubb" \
> +    -c "reopen -o overlap-check.template=blubb" \
> +    "$TEST_IMG" | _filter_qemu_io
> +

[snip]


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

Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests

[Kevin Wolf]

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

Am 04.09.2015 um 21:52 hat Max Reitz geschrieben:
> On 04.09.2015 19:18, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  tests/qemu-iotests/137     | 143 +++++++++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/137.out |  43 ++++++++++++++
> >  tests/qemu-iotests/group   |   1 +
> >  3 files changed, 187 insertions(+)
> >  create mode 100755 tests/qemu-iotests/137
> >  create mode 100644 tests/qemu-iotests/137.out
> > 
> > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > new file mode 100755
> > index 0000000..ad52f71
> > --- /dev/null
> > +++ b/tests/qemu-iotests/137
> 
> 
> [snip]
> 
> > +echo
> > +echo === Try setting some invalid values ===
> > +echo
> > +
> > +$QEMU_IO \
> > +    -c "reopen -o lazy-refcounts=42" \
> > +    -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> > +    -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> > +    -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> > +    -c "reopen -o l2-cache-size=256T" \
> > +    -c "reopen -o l2-cache-size=64G" \
> 
> Testing this seems risky. The rest looks fine, but I'm not sure whether
> this is really needed so much that we should risk the test failing on
> some machines, especially in the future.

I think we should be testing the "too large, but not much too large"
case. A risk of false positives is better than a risk to break things
without noticing. I could make it something like 64T, though, that
should last a bit longer. The "much too large" case starts at 128T
(INT_MAX * s->cluster_size == 2G * 64k).

> Or actually even on my machine:
> 
> $ touch ~/tmp/swap
> $ truncate ~/tmp/swap --size 128G
> $ fallocate -l 128G ~/tmp/swap
> # mkswap ~maxx/tmp/swap
> # swapon ~max/tmp/swap

This looks faked. :-P

> $ cd /to/qemu/build
> $ ./qemu-io -c 'reopen -o l2-cache-size=64G' test.qcow2
> $
> 
> (Running the test itself is apparently a bad idea, because then for some
> reason Linux thinks I actually want to use the swap and everything slows
> down to a crawl)

Well, if you do that intentionally, I guess you deserve it...

Kevin

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests

[Kevin Wolf]

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

Am 07.09.2015 um 14:08 hat Kevin Wolf geschrieben:
> Am 04.09.2015 um 21:52 hat Max Reitz geschrieben:
> > On 04.09.2015 19:18, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > >  tests/qemu-iotests/137     | 143 +++++++++++++++++++++++++++++++++++++++++++++
> > >  tests/qemu-iotests/137.out |  43 ++++++++++++++
> > >  tests/qemu-iotests/group   |   1 +
> > >  3 files changed, 187 insertions(+)
> > >  create mode 100755 tests/qemu-iotests/137
> > >  create mode 100644 tests/qemu-iotests/137.out
> > > 
> > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > > new file mode 100755
> > > index 0000000..ad52f71
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/137
> > 
> > 
> > [snip]
> > 
> > > +echo
> > > +echo === Try setting some invalid values ===
> > > +echo
> > > +
> > > +$QEMU_IO \
> > > +    -c "reopen -o lazy-refcounts=42" \
> > > +    -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> > > +    -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> > > +    -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> > > +    -c "reopen -o l2-cache-size=256T" \
> > > +    -c "reopen -o l2-cache-size=64G" \
> > 
> > Testing this seems risky. The rest looks fine, but I'm not sure whether
> > this is really needed so much that we should risk the test failing on
> > some machines, especially in the future.
> 
> I think we should be testing the "too large, but not much too large"
> case. A risk of false positives is better than a risk to break things
> without noticing. I could make it something like 64T, though, that
> should last a bit longer. The "much too large" case starts at 128T
> (INT_MAX * s->cluster_size == 2G * 64k).

...except that qcow2_cache_create() also allocates a Qcow2CachedTable
array and that one is small enough in comparison that we'd end up again
in the same range as before, where allocations might not fail right
away, but fill up the swap.

Fair enough, I'll remove the lines. Do you want a v3 on the list or can
I fix it while merging?

Kevin

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