From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=43689 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OUgMO-0007gf-TS for qemu-devel@nongnu.org; Fri, 02 Jul 2010 09:32:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OUgMN-0006lx-Ot for qemu-devel@nongnu.org; Fri, 02 Jul 2010 09:32:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:23172) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OUgMN-0006lj-Hc for qemu-devel@nongnu.org; Fri, 02 Jul 2010 09:32:51 -0400 Message-ID: <4C2DEA79.3020600@redhat.com> Date: Fri, 02 Jul 2010 15:32:41 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1278072457-8156-1-git-send-email-kwolf@redhat.com> <1278072457-8156-2-git-send-email-kwolf@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v2 1/2] block: Fix early failure in multiwrite List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel@nongnu.org Am 02.07.2010 15:18, schrieb Stefan Hajnoczi: > On Fri, Jul 2, 2010 at 1:07 PM, Kevin Wolf wrote: >> bdrv_aio_writev may call the callback immediately (and it will commonly do so >> in error cases). Current code doesn't consider this. For details see the >> comment added by this patch. >> >> Signed-off-by: Kevin Wolf >> --- >> block.c | 35 +++++++++++++++++++++++++++++------ >> 1 files changed, 29 insertions(+), 6 deletions(-) >> >> diff --git a/block.c b/block.c >> index 9176dec..e65971c 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2183,8 +2183,29 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs) >> // Check for mergable requests >> num_reqs = multiwrite_merge(bs, reqs, num_reqs, mcb); >> >> - // Run the aio requests >> + /* >> + * Run the aio requests. As soon as one request can't be submitted >> + * successfully, fail all requests that are not yet submitted (we must >> + * return failure for all requests anyway) >> + * >> + * num_requests cannot be set to the right value immediately: If >> + * bdrv_aio_writev fails for some request, num_requests would be too high >> + * and therefore multiwrite_cb() would never recognize the multiwrite >> + * request as completed. We also cannot use the loop variable i to set it >> + * when the first request fails because the callback may already have been >> + * called for previously submitted requests. Thus, num_requests must be >> + * incremented for each request that is submitted. >> + * >> + * The problem that callbacks may be called early also means that we need >> + * to take care that num_requests doesn't become 0 before all requests are >> + * submitted - multiwrite_cb() would consider the multiwrite request >> + * completed. A dummy request that is "completed" by a manual call to >> + * multiwrite_cb() takes care of this. >> + */ >> + mcb->num_requests = 1; >> + >> for (i = 0; i < num_reqs; i++) { >> + mcb->num_requests++; >> acb = bdrv_aio_writev(bs, reqs[i].sector, reqs[i].qiov, >> reqs[i].nb_sectors, multiwrite_cb, mcb); >> >> @@ -2192,22 +2213,24 @@ int bdrv_aio_multiwrite(BlockDriverState *bs, BlockRequest *reqs, int num_reqs) >> // We can only fail the whole thing if no request has been >> // submitted yet. Otherwise we'll wait for the submitted AIOs to >> // complete and report the error in the callback. >> - if (mcb->num_requests == 0) { >> - reqs[i].error = -EIO; >> + if (i == 0) { >> goto fail; >> } else { >> - mcb->num_requests++; >> multiwrite_cb(mcb, -EIO); > > When bdrv_aio_writev() fails we don't know if the callback has been > invoked by the block driver. Qcow2 will invoke the callback in some > cases. This is a problem because num_requests will be decremented > twice if we unconditionally call it here. Talked to Stefan on IRC and we came to the conclusion that it's not a problem in fact: qcow_aio_writev() either returns NULL or calls a callback, but it never does both. If a block driver returned NULL and called a callback for the same request that would be a bug in the block driver. Kevin