From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43687) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YxaRg-000603-Dk for qemu-devel@nongnu.org; Wed, 27 May 2015 08:28:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YxaRf-0004oy-Dg for qemu-devel@nongnu.org; Wed, 27 May 2015 08:28:28 -0400 Message-ID: <5565B837.3090300@redhat.com> Date: Wed, 27 May 2015 06:27:35 -0600 From: Eric Blake MIME-Version: 1.0 References: <8007efe81120cd72f7c4145b8bbc3f4bc558e62d.1432719752.git.berto@igalia.com> In-Reply-To: <8007efe81120cd72f7c4145b8bbc3f4bc558e62d.1432719752.git.berto@igalia.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="63FT3pm7NHL2SHTXokBRdS9oGxrgJHVgC" Subject: Re: [Qemu-devel] [PATCH 2/3] qcow2: add option to clean unused cache entries after some time List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia , qemu-devel@nongnu.org Cc: Kevin Wolf , qemu-block@nongnu.org, Stefan Hajnoczi , Max Reitz This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --63FT3pm7NHL2SHTXokBRdS9oGxrgJHVgC Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/27/2015 03:46 AM, Alberto Garcia wrote: > This adds a new 'cache-clean-interval' option that cleans all qcow2 > cache entries that haven't been used in a certain interval, given in > seconds. >=20 > This allows setting a large L2 cache size so it can handle scenarios > with lots of I/O and at the same time use little memory during periods > of inactivity. >=20 > This feature currently relies on MADV_DONTNEED to free that memory, so > it is not useful in systems that don't follow that behavior. >=20 > Signed-off-by: Alberto Garcia > Cc: Max Reitz > --- > block/qcow2-cache.c | 35 ++++++++++++++++++++++++++++ > block/qcow2.c | 66 ++++++++++++++++++++++++++++++++++++++++++++= ++++++++ > block/qcow2.h | 4 ++++ > qapi/block-core.json | 13 +++++++++-- > 4 files changed, 116 insertions(+), 2 deletions(-) >=20 > +static void cache_clean_timer_init(BlockDriverState *bs, AioContext *c= ontext) > +{ > + BDRVQcowState *s =3D bs->opaque; > + if (s->cache_clean_interval > 0) { > + s->cache_clean_timer =3D aio_timer_new(context, QEMU_CLOCK_VIR= TUAL, > + SCALE_MS, cache_clean_tim= er_cb, > + bs); > + timer_mod(s->cache_clean_timer, qemu_clock_get_ms(QEMU_CLOCK_V= IRTUAL) + > + (int64_t) s->cache_clean_interval * 1000); > + } > +} > + This function sets up a timer for non-zero interval, but does nothing if interval is zero. [1] > @@ -839,6 +888,16 @@ static int qcow2_open(BlockDriverState *bs, QDict = *options, int flags, > goto fail; > } > =20 > + cache_clean_interval =3D > + qemu_opt_get_number(opts, QCOW2_OPT_CACHE_CLEAN_INTERVAL, 0); > + if (cache_clean_interval > UINT_MAX) { > + error_setg(errp, "Cache clean interval too big"); > + ret =3D -EINVAL; > + goto fail; > + } If you type the qapi code as 'uint32' rather than 'int', you could skip the error checking here because the parser would have already clamped things. But I can live with this as-is. > + s->cache_clean_interval =3D cache_clean_interval; > + cache_clean_timer_init(bs, bdrv_get_aio_context(bs)); [1] But here, you are unconditionally calling init, whether the new value is 0 or nonzero. Can a block reopen ever cause an existing BDS to change its interval, in which case I could create a BDS originally with a timer, then reopen it without a timer, and init() would have to remove the existing timer? If I'm reading this patch correctly, right now the interval is a write-once deal (no way to change it after the fact), so your code is okay; but should a separate patch be added to allow adjusting the interval, via a reopen operation? Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --63FT3pm7NHL2SHTXokBRdS9oGxrgJHVgC 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJVZbg3AAoJEKeha0olJ0Nqer8H/1iwsayLtFEs5D2sfWQ/gf7n QKqst2HDvIc4IfTi8iIrOBdoc6nNoanGGuj1NoLtKHscsW1I3uuKnJAkbPZHBurE DBU4U9EeSHouIWdtgxo33Qh9AZffHWYFP7o7tFp4OpqYESwgBi6BwM5Z1zk8RN6r 9hQwVCFZul18NU3xKKBWXOMvLDlC0LPOZBkPla8/tFwrvpOymt5Bp+wD3EPQ9s05 8GackmYF0EpVb+zrWFtFb+D9d5dJzEH0031QKU7XM5QrzT4KZ6GO5eICQvk82pIE ts2XQnwHN2GptDT6YKBDQa1qkB2o41OBFA8eMmkfxSpq0wfkuTvFOX719/4D/EI= =otCE -----END PGP SIGNATURE----- --63FT3pm7NHL2SHTXokBRdS9oGxrgJHVgC--