| Message ID | 1457353613-8081-1-git-send-email-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, >
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
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,
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(-)