From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KuSTu-0001bC-IM for qemu-devel@nongnu.org; Mon, 27 Oct 2008 09:50:06 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KuSTt-0001ay-DC for qemu-devel@nongnu.org; Mon, 27 Oct 2008 09:50:05 -0400 Received: from [199.232.76.173] (port=55624 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KuSTt-0001av-9R for qemu-devel@nongnu.org; Mon, 27 Oct 2008 09:50:05 -0400 Received: from rn-out-0910.google.com ([64.233.170.188]:53226) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KuSTs-0007xA-Vw for qemu-devel@nongnu.org; Mon, 27 Oct 2008 09:50:05 -0400 Received: by rn-out-0910.google.com with SMTP id m61so1036313rnd.8 for ; Mon, 27 Oct 2008 06:50:02 -0700 (PDT) Message-ID: <4905C706.7040408@codemonkey.ws> Date: Mon, 27 Oct 2008 08:49:58 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] fix bdrv_aio_read API breakage in qcow2 References: <20080829135249.GI24884@duo.random> <20080901104356.GD25764@duo.random> <20080901105314.GE25764@duo.random> <20081022141459.GA26426@duo.random> In-Reply-To: <20081022141459.GA26426@duo.random> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Andrea Arcangeli wrote: > From: Andrea Arcangeli > > I noticed the qemu_aio_flush was doing nothing at all. And a flood of > cmd_writeb commands leading to a noop-invocation of qemu_aio_flush > were executed. > > In short all 'memset;goto redo' places must be fixed to use the bh and > not to call the callback in the context of bdrv_aio_read or the > bdrv_aio_read model falls apart. Reading from qcow2 holes is possible > with phyisical readahead (kind of breada in linux buffer cache). > > This is needed at least for scsi, ide is lucky (or it has been > band-aided against this API breakage by fixing the symptom and not the > real bug). > > Same bug exists in qcow of course, can be fixed later as it's less > urgent. > > Signed-off-by: Andrea Arcangeli > You've got whitespace damage (tabs instead of spaces). Regards, Anthony Liguori > -- > > Index: block-qcow2.c > =================================================================== > --- block-qcow2.c (revision 5506) > +++ block-qcow2.c (working copy) > @@ -1165,8 +1165,18 @@ > uint64_t cluster_offset; > uint8_t *cluster_data; > BlockDriverAIOCB *hd_aiocb; > + QEMUBH *bh; > } QCowAIOCB; > > +static void qcow_aio_read_cb(void *opaque, int ret); > +static void qcow_aio_read_bh(void *opaque) > +{ > + QCowAIOCB *acb = opaque; > + qemu_bh_delete(acb->bh); > + acb->bh = NULL; > + qcow_aio_read_cb(opaque, 0); > +} > + > static void qcow_aio_read_cb(void *opaque, int ret) > { > QCowAIOCB *acb = opaque; > @@ -1182,7 +1192,6 @@ > return; > } > > - redo: > /* post process the read buffer */ > if (!acb->cluster_offset) { > /* nothing to do */ > @@ -1223,12 +1232,30 @@ > if (acb->hd_aiocb == NULL) > goto fail; > } else { > - goto redo; > + if (acb->bh) { > + ret = -EIO; > + goto fail; > + } > + acb->bh = qemu_bh_new(qcow_aio_read_bh, acb); > + if (!acb->bh) { > + ret = -EIO; > + goto fail; > + } > + qemu_bh_schedule(acb->bh); > } > } else { > /* Note: in this case, no need to wait */ > memset(acb->buf, 0, 512 * acb->n); > - goto redo; > + if (acb->bh) { > + ret = -EIO; > + goto fail; > + } > + acb->bh = qemu_bh_new(qcow_aio_read_bh, acb); > + if (!acb->bh) { > + ret = -EIO; > + goto fail; > + } > + qemu_bh_schedule(acb->bh); > } > } else if (acb->cluster_offset & QCOW_OFLAG_COMPRESSED) { > /* add AIO support for compressed blocks ? */ > @@ -1236,7 +1263,16 @@ > goto fail; > memcpy(acb->buf, > s->cluster_cache + index_in_cluster * 512, 512 * acb->n); > - goto redo; > + if (acb->bh) { > + ret = -EIO; > + goto fail; > + } > + acb->bh = qemu_bh_new(qcow_aio_read_bh, acb); > + if (!acb->bh) { > + ret = -EIO; > + goto fail; > + } > + qemu_bh_schedule(acb->bh); > } else { > if ((acb->cluster_offset & 511) != 0) { > ret = -EIO; > > >