[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes |
Date: |
Mon, 7 Mar 2016 19:24:52 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 |
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 <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 */
This comment reads a bit weird now.
Rest looks good and this is not exactly critical, so:
Reviewed-by: Max Reitz <address@hidden>
> - 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