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

[Kevin Wolf]

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

pgp6PJCJRT34I.pgp
Description: PGP signature