* [Qemu-devel] [PATCH] block: Fix snapshot=on cache modes
@ 2016-03-07 12:26 Kevin Wolf
2016-03-07 18:24 ` [Qemu-devel] [Qemu-block] " Max Reitz
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2016-03-07 12:26 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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 <kwolf@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 */
- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes
2016-03-07 12:26 [Qemu-devel] [PATCH] block: Fix snapshot=on cache modes Kevin Wolf
@ 2016-03-07 18:24 ` Max Reitz
2016-03-08 9:50 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Max Reitz @ 2016-03-07 18:24 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 7655 bytes --]
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 <kwolf@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 <mreitz@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,
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes
2016-03-07 18:24 ` [Qemu-devel] [Qemu-block] " Max Reitz
@ 2016-03-08 9:50 ` Kevin Wolf
0 siblings, 0 replies; 3+ messages in thread
From: Kevin Wolf @ 2016-03-08 9:50 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 3683 bytes --]
Am 07.03.2016 um 19:24 hat Max Reitz geschrieben:
> 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 <kwolf@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.
Good catch, will s/a new// before sending a pull request.
> Rest looks good and this is not exactly critical, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
Thanks.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-08 9:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-07 12:26 [Qemu-devel] [PATCH] block: Fix snapshot=on cache modes Kevin Wolf
2016-03-07 18:24 ` [Qemu-devel] [Qemu-block] " Max Reitz
2016-03-08 9:50 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).