From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34104) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adEHI-0008Ug-RD for qemu-devel@nongnu.org; Tue, 08 Mar 2016 04:50:09 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adEHH-0005WA-NQ for qemu-devel@nongnu.org; Tue, 08 Mar 2016 04:50:08 -0500 Date: Tue, 8 Mar 2016 10:50:01 +0100 From: Kevin Wolf Message-ID: <20160308095001.GA5807@noname.str.redhat.com> References: <1457353613-8081-1-git-send-email-kwolf@redhat.com> <56DDC774.3020107@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="fdj2RfSjLxBAspz7" Content-Disposition: inline In-Reply-To: <56DDC774.3020107@redhat.com> Subject: Re: [Qemu-devel] [Qemu-block] [PATCH] block: Fix snapshot=on cache modes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, qemu-block@nongnu.org --fdj2RfSjLxBAspz7 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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=3Don: 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=3Dwriteback on the temporary overlay and > > cache=3Dwritethrough,cache.no-flush=3Don on the real image file. > >=20 > > This patch changes things so that the temporary overlay gets > > cache=3Dunsafe 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=3Don, and in the case of committing changes, the final flush is > > no longer ignored except explicitly requested by the user. > >=20 > > Signed-off-by: Kevin Wolf > > --- > > block.c | 34 ++++++++++++++++++++++++---------- > > blockdev.c | 7 ------- > > include/block/block.h | 1 - > > 3 files changed, 24 insertions(+), 18 deletions(-) > >=20 > > 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) > > } > > =20 > > /* > > - * Returns the flags that a temporary snapshot should get, based on the > > - * originally requested flags (the originally requested image will hav= e 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 =3D (parent_flags & ~BDRV_O_SNAPSHOT) | BDRV_O_TEMPOR= ARY; > > + > > + /* For temporary files, unconditional cache=3Dunsafe 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"= ); > > } > > =20 > > /* > > @@ -1424,13 +1430,13 @@ done: > > return c; > > } > > =20 > > -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 =3D g_malloc0(PATH_MAX + 1); > > int64_t total_size; > > QemuOpts *opts =3D NULL; > > - QDict *snapshot_options; > > BlockDriverState *bs_snapshot; > > Error *local_err =3D NULL; > > int ret; > > @@ -1465,7 +1471,6 @@ int bdrv_append_temp_snapshot(BlockDriverState *b= s, int flags, Error **errp) > > } > > =20 > > /* Prepare a new options QDict for the temporary file */ >=20 > 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: >=20 > Reviewed-by: Max Reitz Thanks. Kevin --fdj2RfSjLxBAspz7 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJW3qBIAAoJEH8JsnLIjy/WetkQAKyIDK9LkMi4BylcpP3CLLa+ kcPreFtr7sF4pTC3vU6Q0y5crBWWURpFZWDf9lhkA7l/1Zm+G9nQ/rVcqW2Nlmry QuRPA76RKches68H6q/2+IijaNmct7up4PGlyHWk7JpGbcLd0qAgU3cdJw57bzgV JmJPSB7/dzLkJ8kYN9KNoYY2YSw90sYIz1/hMMzkTRPSJ1D07cUFahTf4wk0mvE1 hJbr7EghrYBPztowiWZa/BHxL6d3kcHl35KdfDaw4od/cDhMSazSY1UCU2s6vpMz frwLDn5WvRuJT2c2bUyltOjMz8isW/Ze0TgI9cke+QKNB1Lik36UzpaWvQZk5C7p RjMwx5E8QRrtLqXq4L8BcmwEUnAk026JjeH8CM3m5MiCul8iE9FBAMfknDECmyQd qoSQD16mchOWZog0G2XsnE851tSZ9iW8+scIyH5h35/OEp6kcd4f8uLF3QcnFaHl TcMdi3vqXFVS4xN4zHJep06RH34LCDjLbhBByJoWaLSNK/aTffQTHkwxU+czv0bb +URNDgJc24uPj8pgY2H6xK/nBf5IMxmzg2EfkR4avzY42YLWQmMe4UHZBAt9Gu6W sWBH3qf1Xv3vUE3Ya6nHMtzsHWt5ZHf83177LpBBxb+OG4ayXNg/5ACJd0lYKX1E MUcjeIkBdP3u9+7aAvUH =p27q -----END PGP SIGNATURE----- --fdj2RfSjLxBAspz7--