[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH v3] blkverify: Add block driver for verifyin
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] Re: [PATCH v3] blkverify: Add block driver for verifying I/O |
Date: |
Mon, 20 Sep 2010 17:43:11 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.12) Gecko/20100907 Fedora/3.0.7-1.fc12 Thunderbird/3.0.7 |
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 <address@hidden> 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