From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1JgkZB-000809-Qa
	for qemu-devel@nongnu.org; Tue, 01 Apr 2008 13:46:37 -0400
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1JgkZ7-0007uh-BH
	for qemu-devel@nongnu.org; Tue, 01 Apr 2008 13:46:37 -0400
Received: from [199.232.76.173] (helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1JgkZ6-0007uO-SD
	for qemu-devel@nongnu.org; Tue, 01 Apr 2008 13:46:32 -0400
Received: from mail.codesourcery.com ([65.74.133.4])
	by monty-python.gnu.org with esmtps
	(TLS-1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.60)
	(envelope-from <paul@codesourcery.com>) id 1JgkZ6-00025G-4c
	for qemu-devel@nongnu.org; Tue, 01 Apr 2008 13:46:32 -0400
From: Paul Brook <paul@codesourcery.com>
Subject: Re: [Qemu-devel] [PATCH] Asynchronous reliable and configurable cache
	flush
Date: Tue, 1 Apr 2008 18:46:25 +0100
References: <18418.28291.689482.198484@mariner.uk.xensource.com>
In-Reply-To: <18418.28291.689482.198484@mariner.uk.xensource.com>
MIME-Version: 1.0
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline
Message-Id: <200804011846.25975.paul@codesourcery.com>
Reply-To: qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: qemu-devel@nongnu.org
Cc: Ian Jackson <Ian.Jackson@eu.citrix.com>

On Tuesday 01 April 2008, Ian Jackson wrote:
> + =A0 =A0if (!s->bs) return; /* yikes */

This comment is distinctly unhelpful. I guess this is the condition mention=
ed=20
in ide_device_utterly_broken, but it's hard to tell.

> @@ -1021,6 +1031,13 @@ static void ide_write_dma_cb(void *opaque, int ret)
> =A0
> =A0 =A0 =A0/* end of transfer ? */
> =A0 =A0 =A0if (s->nsector =3D=3D 0) {
> +=A0=A0=A0=A0=A0=A0=A0if (!s->write_cache) {
> +=A0=A0=A0=A0=A0=A0=A0 =A0 =A0ret =3D bdrv_flush(s->bs);
> +=A0=A0=A0=A0=A0=A0=A0 =A0 =A0if (ret !=3D 0) {
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ide_dma_error(s);
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return;
> +=A0=A0=A0=A0=A0=A0=A0 =A0 =A0}
> +=A0=A0=A0=A0=A0=A0=A0}

By my reading this is adding a synchronous flush to the end of an async wri=
te=20
operation, which in practice makes the whole operation synchronous.

> +static void scsi_flush_cb(void *opaque, int ret) {
> + =A0 =A0SCSIRequest *r =3D opaque;
> + =A0 =A0if (ret) {
> + =A0 =A0 =A0 =A0BADF("scsi-disc: IO error on flush: %s\n", strerror(-ret=
));

This is wrong for two reasons: BADF already adds a suitable prefix, and is =
for=20
reporting qemu bugs (i.e. missing features or things that should never=20
happen). This is just a normal IO error, which we correctly report to the=20
guest.

Paul