From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41914) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RMFtJ-0002HD-LH for qemu-devel@nongnu.org; Fri, 04 Nov 2011 05:16:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RMFtH-0002sb-BZ for qemu-devel@nongnu.org; Fri, 04 Nov 2011 05:16:49 -0400 Received: from mtagate3.uk.ibm.com ([194.196.100.163]:34230) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RMFtH-0002s2-2t for qemu-devel@nongnu.org; Fri, 04 Nov 2011 05:16:47 -0400 Received: from d06nrmr1707.portsmouth.uk.ibm.com (d06nrmr1707.portsmouth.uk.ibm.com [9.149.39.225]) by mtagate3.uk.ibm.com (8.13.1/8.13.1) with ESMTP id pA49Ghuj011002 for ; Fri, 4 Nov 2011 09:16:43 GMT Received: from d06av08.portsmouth.uk.ibm.com (d06av08.portsmouth.uk.ibm.com [9.149.37.249]) by d06nrmr1707.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id pA49Gg692166954 for ; Fri, 4 Nov 2011 09:16:43 GMT Received: from d06av08.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av08.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id pA49GfH4024702 for ; Fri, 4 Nov 2011 09:16:42 GMT Date: Fri, 4 Nov 2011 08:03:25 +0000 From: Stefan Hajnoczi Message-ID: <20111104080325.GA4749@stefanha-thinkpad.localdomain> References: <1319728975-6069-1-git-send-email-stefanha@linux.vnet.ibm.com> <1319728975-6069-4-git-send-email-stefanha@linux.vnet.ibm.com> <20111101180612.GA13205@amt.cnet> <20111103163424.GA21743@amt.cnet> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <20111103163424.GA21743@amt.cnet> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 3/8] block: add image streaming block job List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Marcelo Tosatti Cc: Kevin Wolf , Stefan Hajnoczi , Anthony Liguori , qemu-devel@nongnu.org On Thu, Nov 03, 2011 at 02:34:24PM -0200, Marcelo Tosatti wrote: > On Wed, Nov 02, 2011 at 03:43:49PM +0000, Stefan Hajnoczi wrote: > > On Tue, Nov 1, 2011 at 6:06 PM, Marcelo Tosatti = wrote: > > > On Thu, Oct 27, 2011 at 04:22:50PM +0100, Stefan Hajnoczi wrote: > > >> + > > >> + =A0 =A0for (sector_num =3D 0; sector_num < end; sector_num +=3D = n) { > > >> + =A0 =A0 =A0 =A0if (block_job_is_cancelled(&s->common)) { > > >> + =A0 =A0 =A0 =A0 =A0 =A0break; > > >> + =A0 =A0 =A0 =A0} > > > > > > If cancellation is seen here in the last loop iteration, > > > bdrv_change_backing_file below should not be executed. > >=20 > > I documented this case in the QMP API. I'm not sure if it's possible > > to guarantee that the operation isn't just completing as you cancel > > it. Any blocking point between completion of the last iteration and > > completing the operation is vulnerable to missing the cancel. It's > > easier to explicitly say the operation might just have completed when > > you canceled, rather than trying to protect the completion path. Do > > you think it's a problem to have these loose semantics that I > > described? >=20 > No, that is ok. I'm referring to bdrv_change_backing_file() being > executed without the entire image being streamed. >=20 > "if (sector_num =3D=3D end && ret =3D=3D 0)" includes both all sectors = being=20 > streamed and all sectors except the last iteration being streamed (due > to job cancelled break). I don't see the case you mention. Here is the code again: for (sector_num =3D 0; sector_num < end; sector_num +=3D n) { =A0 =A0if (block_job_is_cancelled(&s->common)) { =A0 =A0 =A0 =A0break; =A0 =A0} If we are on the last iteration, then sector_num =3D end - m, where m > 0 and is the number of sectors we are about to stream. If we are cancelled during this last iteration then sector_num =3D=3D end - m. Therefore the "if (sector_num =3D=3D end && ret =3D=3D 0)" case doe= s not evaluate to true. The only way we can reach sector_num =3D=3D end is by having successfully streamed those last m sectors. Why? Because sector_num is a 0-based index and not a 1-based index, so it excludes end. > > >> + > > >> + =A0 =A0 =A0 =A0/* TODO rate-limit */ > > >> + =A0 =A0 =A0 =A0/* Note that even when no rate limit is applied w= e need to yield with > > >> + =A0 =A0 =A0 =A0 * no pending I/O here so that qemu_aio_flush() i= s able to return. > > >> + =A0 =A0 =A0 =A0 */ > > >> + =A0 =A0 =A0 =A0co_sleep_ns(rt_clock, 0); > > > > > > How do you plan to implement rate limit? > >=20 > > It was implemented in the QED-specific image streaming series: > >=20 > > http://repo.or.cz/w/qemu/stefanha.git/commitdiff/22f2c09d2fcfe5e49ac4= 604fd23e4744f549a476 > >=20 > > That implementation works fine and is small but I'd like to reuse the > > migration speed limit, if possible. That way we don't have 3 > > different rate-limiting implementations in QEMU :). >=20 > One possibility would be to create a "virtual" block device for > streaming, sitting on top of the real block device. Then enforce block > I/O limits on the virtual block device, the guest would remain accessin= g > the real block device. That's an interesting idea. I have also experimented with rate-limiting and it seems the common code is really small - the rate-limiting code is quite short to begin with. So I'm now tending to reimplementing it. Stefan