Re: [Qemu-devel] Re: [PATCH] blkverify: Add block driver for verifying I/O

[Stefan Hajnoczi]

On Fri, Sep 3, 2010 at 4:06 PM, Kevin Wolf <address@hidden> wrote:
> Am 03.09.2010 13:55, schrieb Stefan Hajnoczi:
>> The blkverify block driver makes investigating image format data
>> corruption much easier.  A raw image initialized with the same contents
>> as the test image (e.g. qcow2 file) must be provided.  The raw image
>> mirrors read/write operations and is used to verify that data read from
>> the test image is correct.
>>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>> Please see docs/blkverify.txt below for a full description.
>>
>>  Makefile.objs      |    2 +-
>>  block/blkverify.c  |  309 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  docs/blkverify.txt |   69 ++++++++++++
>>  3 files changed, 379 insertions(+), 1 deletions(-)
>>  create mode 100644 block/blkverify.c
>>  create mode 100644 docs/blkverify.txt
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index b25f573..40a4594 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -14,7 +14,7 @@ block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
>>
>>  block-nested-y += raw.o cow.o cow2.o qcow.o vdi.o vmdk.o cloop.o dmg.o 
>> bochs.o vpc.o vvfat.o
>>  block-nested-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
>> -block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o
>> +block-nested-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>>  block-nested-$(CONFIG_WIN32) += raw-win32.o
>>  block-nested-$(CONFIG_POSIX) += raw-posix.o
>>  block-nested-$(CONFIG_CURL) += curl.o
>
> This hunk doesn't apply, there is no cow2. :-)
>
>> diff --git a/block/blkverify.c b/block/blkverify.c
>> new file mode 100644
>> index 0000000..9cebafe
>> --- /dev/null
>> +++ b/block/blkverify.c
>> @@ -0,0 +1,309 @@
>> +/*
>> + * Block protocol for block driver correctness testing
>> + *
>> + * Copyright (C) 2010 IBM, Corp.
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +
>> +#include <stdarg.h>
>> +#include "block_int.h"
>> +
>> +typedef struct {
>> +    BlockDriverState *test_file;
>> +} BDRVBlkverifyState;
>> +
>> +typedef struct BlkverifyAIOCB BlkverifyAIOCB;
>> +struct BlkverifyAIOCB {
>> +    BlockDriverAIOCB common;
>> +    QEMUBH *bh;
>> +
>> +    /* Request metadata */
>> +    bool is_write;
>> +    int64_t sector_num;
>> +    int nb_sectors;
>> +
>> +    int ret;                    /* first completed request's result */
>> +    unsigned int done;          /* completion counter */
>> +
>> +    QEMUIOVector *qiov;         /* user I/O vector */
>> +    QEMUIOVector raw_qiov;      /* cloned I/O vector for raw file */
>> +    void *buf;                  /* buffer for raw file I/O */
>> +
>> +    void (*verify)(BlkverifyAIOCB *acb);
>> +};
>> +
>> +static void blkverify_aio_cancel(BlockDriverAIOCB *acb)
>> +{
>> +    qemu_aio_release(acb);
>> +}
>
> I would be surprised if this implementation didn't segfault in one way
> or another.
>
> I think you'd better cancel the two requests that are combined in this
> acb. Or wait for their completion actually because you want to have both
> in the same state.

You're right, this needs to be done more carefully.  I'll send a v2.

Thanks for reviewing the patch!

Stefan