Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes

[Max Reitz]

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