From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51107) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a2P99-0006fe-Mp for qemu-devel@nongnu.org; Fri, 27 Nov 2015 14:57:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a2P98-0006Zs-CU for qemu-devel@nongnu.org; Fri, 27 Nov 2015 14:57:31 -0500 References: <1448294400-476-1-git-send-email-kwolf@redhat.com> <1448294400-476-18-git-send-email-kwolf@redhat.com> From: Max Reitz Message-ID: <5658B5A1.3070106@redhat.com> Date: Fri, 27 Nov 2015 20:57:21 +0100 MIME-Version: 1.0 In-Reply-To: <1448294400-476-18-git-send-email-kwolf@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Hw5Lp4TXPKlo6DVQL300trPnGb46PFhjP" Subject: Re: [Qemu-devel] [PATCH v2 17/21] block: Move cache options into options QDict 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) --Hw5Lp4TXPKlo6DVQL300trPnGb46PFhjP Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 23.11.2015 16:59, Kevin Wolf wrote: > This adds the cache mode options to the QDict, so that they can be > specified for child nodes (e.g. backing.cache.direct=3Doff). >=20 > The cache modes are not removed from the flags at this point; instead, > options and flags are kept in sync. If the user specifies both flags an= d > options, the options take precedence. >=20 > Child node inherit cache modes as options now, they don't use flags any= > more. >=20 > Note that this forbids specifying the cache mode for empty drives. It > didn't make sense anyway to specify it there, because it didn't have an= y > effect. blockdev_init() considers the cache options now bdrv_open() > options and therefore doesn't create an empty drive any more but calls > into bdrv_open(). This in turn will fail with no driver and filename > specified. >=20 > Signed-off-by: Kevin Wolf > --- > block.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++++++++= +++++--- > blockdev.c | 52 ++++++++++---------------------- > 2 files changed, 111 insertions(+), 41 deletions(-) >=20 > diff --git a/block.c b/block.c > index eff0c19..397014a 100644 > --- a/block.c > +++ b/block.c [...] > @@ -1747,12 +1816,22 @@ static BlockReopenQueue *bdrv_reopen_queue_chil= d(BlockReopenQueue *bs_queue, > /* > * Precedence of options: > * 1. Explicitly passed in options (highest) > - * 2. TODO Set in flags (only for top level) > + * 2. Set in flags (only for top level) > * 3. Retained from explicitly set options of bs > * 4. Inherited from parent node > * 5. Retained from effective options of bs > */ > =20 > + if (!parent_options) { > + /* > + * Any setting represented by flags is always updated. If the > + * corresponding QDict option is set, it takes precedence. Oth= erwise > + * the flag is translated into a QDict option. The old setting= of bs is > + * not considered. > + */ > + update_options_from_flags(options, flags); > + } > + > /* Old explicitly set values (don't overwrite by inherited value) = */ > old_options =3D qdict_clone_shallow(bs->explicit_options); > bdrv_join_options(bs, options, old_options); > @@ -1923,6 +2002,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_= state, BlockReopenQueue *queue, > goto error; > } > =20 > + update_flags_from_options(&reopen_state->flags, opts); > + > + /* If a guest device is attached, it owns WCE */ > + if (reopen_state->bs->blk && blk_get_attached_dev(reopen_state->bs= ->blk)) { > + bool old_wce =3D bdrv_enable_write_cache(reopen_state->bs); > + bool new_wce =3D (reopen_state->flags & BDRV_O_CACHE_WB); > + if (old_wce !=3D new_wce) { > + error_setg(errp, "Cannot change cache.writeback: Device at= tached"); > + ret =3D -EINVAL; > + goto error; > + } > + } > + Time to get back to my question regarding bdrv_set_enable_write_cache(): 1. bdrv_set_enable_write_cache() sets/unsets BDRV_O_CACHE_WB in bs->open_flags so that it is preserved through a reopen. 2. On reopen, bdrv_reopen_queue_child() calls update_options_from_flags() unless @parent_options is set. If that function is called, it will set the appropriate caching options in @options as dictated by bdrv_set_enable_write_cache(). 3. If @parent_options was NULL, the update_flags_from_options() call here in bdrv_reopen_prepare() will set BDRV_O_CACHE_WB just as dictated by bdrv_set_enable_write_cache() (unless overwritten). That is what we want. 4. If @parent_options was not NULL, the caching options in bs->open_flags are completely overwritten, discarding everything that had been set by bdrv_set_enable_write_cache(). That's not so good. @parent_options is tested so that update_options_from_flags() is called only for root BDSs. It is possible to call bdrv_set_enable_write_cache() on non-root BDSs (e.g. commit_active_start() does it on the commit base via mirror_start_job()), so I do think overriding the setting made by bdrv_set_enable_write_cache() on reopen for any non-root BDSs is not correct. Maybe bdrv_set_enable_write_cache() should set cache.writeback in bs->explicit_options to on or off instead of using bs->open_flags. But in that case, we can no longer use bdrv_set_enable_write_cache() in bdrv_open_common(), because the cache setting there may be implicit. Max > /* node-name and driver must be unchanged. Put them back into the = QDict, so > * that they are checked at the end of this function. */ > value =3D qemu_opt_get(opts, "node-name"); --Hw5Lp4TXPKlo6DVQL300trPnGb46PFhjP 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 iQEcBAEBCAAGBQJWWLWhAAoJEDuxQgLoOKytjJEH/3QtU+AHfugexBjOG75vjEhJ hTnw9u6bTXGs0Ms3M+RddBvZ/nQNaDqnx0j/Lj+OnpSak+t2UbSXwdVfnFqUjDqm +i/WtwIpNqiUj3aIGQ2G9Ksy/G19S3nAt6P162eMAhzEj2RVXTZBKpp3CBxw4Qwt Bo7X4xLdgv3+ChlxZs2CJg8YmFIm/ALxrJPLucrNLB2C6I45nXLHU2B2+QUuKANs EYGuO8SeGHhwOHWtFoTsXgPg7BLK/o9/ldn6KNBRYQ2qRgXmQlb2OwgItgv9+h/0 JYvfb7QECaz0hESekmB+gZGLQQmmPa2RdKNQj2wyJdWePJKF0nJegDfX4J6FUdU= =VwQF -----END PGP SIGNATURE----- --Hw5Lp4TXPKlo6DVQL300trPnGb46PFhjP--