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

[Kevin Wolf <kwolf@redhat.com>]

Am 04.09.2010 17:34, 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 <stefanha@linux.vnet.ibm.com>
> ---
> This is embarassing, I sent out the v2 patch instead of the v3 patch the first
> time!
> 
> v3:
>  * Fix compile error in blkverify_aio_cancel()
> 
> v2:
>  * Implement aio_cancel() by waiting for pending requests
>  * Fix conflict in Makefile.objs
> 
>  Makefile.objs      |    2 +-
>  block/blkverify.c  |  324 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  docs/blkverify.txt |   69 +++++++++++
>  3 files changed, 394 insertions(+), 1 deletions(-)
>  create mode 100644 block/blkverify.c
>  create mode 100644 docs/blkverify.txt
> 
> diff --git a/Makefile.objs b/Makefile.objs
> index 4a1eaa1..b640ec8 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 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
> diff --git a/block/blkverify.c b/block/blkverify.c
> new file mode 100644
> index 0000000..97717a6
> --- /dev/null
> +++ b/block/blkverify.c
> @@ -0,0 +1,324 @@
> +/*
> + * 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_cb(void *opaque, int ret)
> +{
> +    *(bool *)opaque = true;
> +}
> +
> +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.

> +    acb->cb = blkverify_aio_cancel_cb;
> +    acb->opaque = &done;
> +    while (!done) {
> +        qemu_aio_wait();
> +    }

qemu_aio_release(acb)?

> +}
> +
> +static AIOPool blkverify_aio_pool = {
> +    .aiocb_size         = sizeof(BlkverifyAIOCB),
> +    .cancel             = blkverify_aio_cancel,
> +};
> +
> +static void blkverify_err(BlkverifyAIOCB *acb, const char *fmt, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, fmt);
> +    fprintf(stderr, "blkverify: %s sector_num=%ld nb_sectors=%d ",
> +            acb->is_write ? "write" : "read", acb->sector_num,
> +            acb->nb_sectors);
> +    vfprintf(stderr, fmt, ap);
> +    fprintf(stderr, "\n");
> +    va_end(ap);
> +    exit(1);
> +}
> +
> +/* Valid blkverify filenames look like blkverify:path/to/raw_image:path/to/image */
> +static int blkverify_open(BlockDriverState *bs, const char *filename, int flags)
> +{
> +    BDRVBlkverifyState *s = bs->opaque;
> +    int ret;
> +    char *raw, *c;
> +
> +    /* Parse the blkverify: prefix */
> +    if (strncmp(filename, "blkverify:", strlen("blkverify:"))) {
> +        return -EINVAL;
> +    }
> +    filename += strlen("blkverify:");
> +
> +    /* Parse the raw image filename */
> +    c = strchr(filename, ':');
> +    if (c == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    raw = strdup(filename);
> +    raw[c - filename] = '\0';
> +    ret = bdrv_file_open(&bs->file, raw, flags);
> +    free(raw);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    filename = c + 1;
> +
> +    /* 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.

Kevin