On 07.03.2016 13:26, Kevin Wolf wrote: > 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 <kw...@redhat.com> > --- > 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 */
This comment reads a bit weird now. Rest looks good and this is not exactly critical, so: Reviewed-by: Max Reitz <mre...@redhat.com> > - 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, >
signature.asc
Description: OpenPGP digital signature