[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes |
Date: |
Tue, 8 Mar 2016 10:50:01 +0100 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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 <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.
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 <address@hidden>
Thanks.
Kevin
pgpyFjxjeoZGz.pgp
Description: PGP signature