From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33363) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aczq8-0006LT-3y for qemu-devel@nongnu.org; Mon, 07 Mar 2016 13:25:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aczq2-0007Ns-LB for qemu-devel@nongnu.org; Mon, 07 Mar 2016 13:25:08 -0500 References: <1457353613-8081-1-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <56DDC774.3020107@redhat.com> Date: Mon, 7 Mar 2016 19:24:52 +0100 MIME-Version: 1.0 In-Reply-To: <1457353613-8081-1-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="CFPhosvtUFgstduaSG5dMnPRhm4TBjeQo" 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: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --CFPhosvtUFgstduaSG5dMnPRhm4TBjeQo Content-Type: multipart/mixed; boundary="Cau0p6hTs0SGlIfadPDo0B3w7h8WmMgPs" From: Max Reitz To: Kevin Wolf , qemu-block@nongnu.org Cc: qemu-devel@nongnu.org Message-ID: <56DDC774.3020107@redhat.com> Subject: Re: [Qemu-block] [PATCH] block: Fix snapshot=on cache modes References: <1457353613-8081-1-git-send-email-kwolf@redhat.com> In-Reply-To: <1457353613-8081-1-git-send-email-kwolf@redhat.com> --Cau0p6hTs0SGlIfadPDo0B3w7h8WmMgPs Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable 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 backin= g > 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 i= s > 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 th= e > - * 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 */ This comment reads a bit weird now. Rest looks good and this is not exactly critical, so: Reviewed-by: Max Reitz > - snapshot_options =3D 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 *b= s, int flags, Error **errp) > =20 > ret =3D bdrv_open(&bs_snapshot, NULL, NULL, snapshot_options, > flags, &local_err); > + snapshot_options =3D NULL; > if (ret < 0) { > error_propagate(errp, local_err); > goto out; > @@ -1485,6 +1491,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *b= s, int flags, Error **errp) > bdrv_append(bs_snapshot, bs); > =20 > out: > + QDECREF(snapshot_options); > g_free(tmp_filename); > return ret; > } > @@ -1516,6 +1523,7 @@ static int bdrv_open_inherit(BlockDriverState **p= bs, const char *filename, > const char *drvname; > const char *backing; > Error *local_err =3D NULL; > + QDict *snapshot_options =3D NULL; > int snapshot_flags =3D 0; > =20 > assert(pbs); > @@ -1607,7 +1615,9 @@ static int bdrv_open_inherit(BlockDriverState **p= bs, const char *filename, > flags |=3D BDRV_O_ALLOW_RDWR; > } > if (flags & BDRV_O_SNAPSHOT) { > - snapshot_flags =3D bdrv_temp_snapshot_flags(flags); > + snapshot_options =3D qdict_new(); > + bdrv_temp_snapshot_options(&snapshot_flags, snapshot_optio= ns, > + flags, options); > bdrv_backing_options(&flags, options, flags, options); > } > =20 > @@ -1709,7 +1719,9 @@ static int bdrv_open_inherit(BlockDriverState **p= bs, const char *filename, > /* For snapshot=3Don, create a temporary qcow2 overlay. bs points = to the > * temporary snapshot afterwards. */ > if (snapshot_flags) { > - ret =3D bdrv_append_temp_snapshot(bs, snapshot_flags, &local_e= rr); > + ret =3D bdrv_append_temp_snapshot(bs, snapshot_flags, snapshot= _options, > + &local_err); > + snapshot_options =3D NULL; > if (local_err) { > goto close_and_fail; > } > @@ -1721,6 +1733,7 @@ fail: > if (file !=3D 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 *fil= e, 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")= ; > =20 > - if (snapshot) { > - /* always use cache=3Dunsafe 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_s= tr("on")); > - } > - > if (runstate_check(RUN_STATE_INMIGRATE)) { > bdrv_flags |=3D 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 *backi= ng_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, >=20 --Cau0p6hTs0SGlIfadPDo0B3w7h8WmMgPs-- --CFPhosvtUFgstduaSG5dMnPRhm4TBjeQo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJW3cd1AAoJEDuxQgLoOKytk0oIAI5xxDa+dl2Xg+tIq1mEQTaN kM8FHhDqGGJS7KE1UaAhWBnMDNyA5NLdOZWrOWA1En7o/rF9vFfN6ODdsAdTVlpr hmBqOu4cRzZX1hQSon1ns/0bF03NdiDyvGgk+AnFJBpaJpmE9OfUiuEWQ2HQBNAO yx7cbAv2wcHH4oJbDU516huZhENEodW9X+Ru2vpk7vDTOkbxUMLvdXkK8vJEtF18 L2jkn+nyS4iX7+m1zyPTy0R08tye9MINXvI7uWbH7Q7GhJBUuUnVE56NUrnAzIW2 eAf9HeOe4Xxu1PQh8T6S4puR+lie4MOUy2klrKiOHt4ubIr8IjmhWvicJ1Hpc5Y= =xhs5 -----END PGP SIGNATURE----- --CFPhosvtUFgstduaSG5dMnPRhm4TBjeQo--