From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=45973 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1OxiW2-0005Uu-RN
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:42:52 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1OxiW1-0006gn-GM
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:42:50 -0400
Received: from mx1.redhat.com ([209.132.183.28]:37939)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <kwolf@redhat.com>) id 1OxiW1-0006gS-93
	for qemu-devel@nongnu.org; Mon, 20 Sep 2010 11:42:49 -0400
Message-ID: <4C97810F.8020106@redhat.com>
Date: Mon, 20 Sep 2010 17:43:11 +0200
From: Kevin Wolf <kwolf@redhat.com>
MIME-Version: 1.0
Subject: Re: [Qemu-devel] Re: [PATCH v3] blkverify: Add block driver for
	verifying I/O
References: <1283614225-23651-1-git-send-email-stefanha@linux.vnet.ibm.com>	<1283614468-25230-1-git-send-email-stefanha@linux.vnet.ibm.com>	<4C97744F.50307@redhat.com>
	<AANLkTimPfSv1QCDQTfnhMvy_usMzMDHDjUa59=fG7uR2@mail.gmail.com>
In-Reply-To: <AANLkTimPfSv1QCDQTfnhMvy_usMzMDHDjUa59=fG7uR2@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
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.nongnu.org/archive/html/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: Stefan Hajnoczi <stefanha@gmail.com>
Cc: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>, qemu-devel@nongnu.org

Am 20.09.2010 17:22, schrieb Stefan Hajnoczi:
> Thanks for your comments Kevin!
> 
> I'd like to merge the overlapping I/O patch I sent today since
> blkverify is not in mainline yet.  Are you okay with that or should I
> keep them separate?

If that works better for you, I don't mind merging them.

> On Mon, Sep 20, 2010 at 3:48 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 04.09.2010 17:34, schrieb Stefan Hajnoczi:
>>> +static void blkverify_aio_cancel(BlockDriverAIOCB *acb)
>>> +{
>>> +    bool done = false;
>>> +
>>> +    /* Allow the request to complete so the raw file and test file stay in
>>> +     * sync.  Hijack the callback (since the request is cancelled anyway) to
>>> +     * wait for completion.
>>> +     */
>>
>> The approach of completing the request sounds okay, but I think you need
>> to call the original callback if you let it complete.
> 
> Neither posix-aio-compat.c nor block/qcow2.c invoke the callback.  Am
> I missing something?

qcow2 calls raw, so it doesn't need to do it itself.

posix-aio-compat invokes the callback indirectly by waiting for the
request to complete using the normal path.

>>> +    acb->cb = blkverify_aio_cancel_cb;
>>> +    acb->opaque = &done;
>>> +    while (!done) {
>>> +        qemu_aio_wait();
>>> +    }
>>
>> qemu_aio_release(acb)?
> 
> Will fix.
> 
>>> +    /* Open the test file */
>>> +    s->test_file = bdrv_new("");
>>> +    ret = bdrv_open(s->test_file, filename, flags, NULL);
>>> +    if (ret < 0) {
>>> +        return ret;
>>
>> This leaks bs->file.
> 
> s->test_file needs bdrv_delete().  block.c:bdrv_open_common() calls
> bdrv_delete(bs->file) on failure to close and delete bs->file, but we
> don't need to rely on that.  I will fix both.

Right, I missed that we're in the same path as with an automatically
allocated bs->file. Not exactly intuitive that the common implementation
cleans up when it didn't create the file. ;-)

Kevin