From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46089) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgVby-0002sT-AK for qemu-devel@nongnu.org; Fri, 10 Apr 2015 05:52:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YgVbq-0007dl-L9 for qemu-devel@nongnu.org; Fri, 10 Apr 2015 05:52:30 -0400 Received: from mail-wg0-x22b.google.com ([2a00:1450:400c:c00::22b]:34700) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YgVbq-0007dQ-Ci for qemu-devel@nongnu.org; Fri, 10 Apr 2015 05:52:22 -0400 Received: by wgso17 with SMTP id o17so12485561wgs.1 for ; Fri, 10 Apr 2015 02:52:21 -0700 (PDT) Date: Fri, 10 Apr 2015 10:52:18 +0100 From: Stefan Hajnoczi Message-ID: <20150410095218.GA23555@stefanha-thinkpad.redhat.com> References: <3060954d506be499c26d07ba4624dd33cd7b002d.1427732020.git.berto@igalia.com> <20150409142257.GE2783@stefanha-thinkpad.redhat.com> <20150410075834.GA18121@igalia.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="9amGYk9869ThD9tj" Content-Disposition: inline In-Reply-To: <20150410075834.GA18121@igalia.com> Subject: Re: [Qemu-devel] [PATCH 4/7] throttle: Add throttle group support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alberto Garcia Cc: Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi --9amGYk9869ThD9tj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Apr 10, 2015 at 09:58:34AM +0200, Alberto Garcia wrote: > On Thu, Apr 09, 2015 at 03:22:57PM +0100, Stefan Hajnoczi wrote: >=20 > > > @@ -1941,9 +1951,11 @@ void qmp_block_set_io_throttle(const char *dev= ice, int64_t bps, int64_t bps_rd, > > > aio_context_acquire(aio_context); > > > =20 > > > if (!bs->io_limits_enabled && throttle_enabled(&cfg)) { > > > - bdrv_io_limits_enable(bs); > > > + bdrv_io_limits_enable(bs, has_group ? group : device); > > > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) { > > > bdrv_io_limits_disable(bs); > > > + } else if (bs->io_limits_enabled && throttle_enabled(&cfg)) { > > > + bdrv_io_limits_update_group(bs, has_group ? group : device); > > > } > > > =20 > > > if (bs->io_limits_enabled) { > >=20 > > The semantics are inconsistent: > >=20 > > 1. Create drive0 with throttle group "mygroup". > > 2. Issue block-set-io-throttle device=3D"drive0" > >=20 > > The result is that a new throttle group called "drive0" is created. > > I expected to modify the throttling configuration for drive0 (i.e. > > "mygroup"). >=20 > That's right. What we can do is choose the group to update using the > following criteria, in order of precedence: >=20 > 1) The 'group' parameter from block-set-io-throttle. > 2) The group the device is already member of. > 3) A new group ("drive0" in this example). >=20 > Currently we're not doing 2). Great, it makes sense to add 2). > > Now let's disable the throttle group: > >=20 > > 3. Issue block-set-io-throttle with 0 values for device=3D"drive0" > >=20 > > The result is that "mygroup" is changed to all 0s. >=20 > No, that simply removes the device from the group without touching its > configuration: >=20 > > > } else if (bs->io_limits_enabled && !throttle_enabled(&cfg)) { > > > bdrv_io_limits_disable(bs); >=20 > The group name passed by the user is actually not used in this case. You are right. Stefan --9amGYk9869ThD9tj Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVJ51SAAoJEJykq7OBq3PIuLMIAKlxiXX4niXv9slnVmHgmrOJ 0G56fRj7QxO+I1pc6ivHIGhYFD2CmVfSh9RNw4AJ320X5JyrIBR+CvgPsikmNZoo WYEDu8qzELrGWGQVXPIBc7MFNogwkVqbFpt34YMUWCS4XCK3rq27cUMU2ZCh2g4d V+3+3TU+ur6hoClDhXW/rB4EpDzKaI59eDIWT+hHDDZG8PdtCM8msRVu8ea6nEFC l+i5cjLDnWJWgU/owN3pVpwopfcHBKIDUFYlegc5zCJx0uxLBVeYm7Sypvf/H0pw oIrjbhUQLOdVk2CMh+dTCoGsDdtc2WY2/N+anvZPXF99ciUma2j/AHC4jmTS1so= =XK9I -----END PGP SIGNATURE----- --9amGYk9869ThD9tj--