Re: [Qemu-devel] [patch 2/7] Add blkmirror block driver

[Avi Kivity]

On 05/24/2011 12:31 AM, Marcelo Tosatti wrote:
> Mirrored writes are used by live block copy.
>
>
> +
> +typedef struct {
> +    BlockDriverState *bs[2];
> +} BdrvMirrorState;

Can be generalized one day to N.

> +
> +/* Valid blkmirror filenames look like blkmirror:path/to/image1:path/to/image2 
> */
> +static int blkmirror_open(BlockDriverState *bs, const char *filename, int 
> flags)
> +{
> +    BdrvMirrorState *m = bs->opaque;
> +    int ret;
> +    char *raw, *c;
> +
> +    /* Parse the blkmirror: prefix */
> +    if (strncmp(filename, "blkmirror:", strlen("blkmirror:"))) {

Actual format should be documented somewhere.

> +        return -EINVAL;
> +    }
> +    filename += strlen("blkmirror:");
> +
> +    /* Parse the raw image filename */
> +    c = strchr(filename, ':');
> +    if (c == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    raw = strdup(filename);
> +    raw[c - filename] = '\0';
> +    ret = bdrv_file_open(&m->bs[0], raw, flags);
> +    free(raw);
> +    if (ret<  0) {
> +        return ret;
> +    }
> +    filename = c + 1;
> +
> +    ret = bdrv_file_open(&m->bs[1], filename, flags);
> +    if (ret<  0) {
> +        bdrv_delete(m->bs[0]);
> +        return ret;
> +    }
> +
> +    return 0;
> +}
> +
>
> +
> +static BlockDriverAIOCB *blkmirror_aio_readv(BlockDriverState *bs,
> +                                             int64_t sector_num,
> +                                             QEMUIOVector *qiov,
> +                                             int nb_sectors,
> +                                             BlockDriverCompletionFunc *cb,
> +                                             void *opaque)
> +{
> +    BdrvMirrorState *m = bs->opaque;
> +    return bdrv_aio_readv(m->bs[0], sector_num, qiov, nb_sectors, cb, opaque);

We could one day be clever and round-robin once we're synced.

> +}
> +
> +typedef struct DupAIOCB {
> +    BlockDriverAIOCB common;
> +    int count;
> +
> +    BlockDriverCompletionFunc *cb;
> +    void *opaque;
> +
> +    BlockDriverAIOCB *src_aiocb;
> +    BlockDriverAIOCB *dest_aiocb;
> +
> +    int ret;
> +} DupAIOCB;
> +
> +static void dup_aio_cancel(BlockDriverAIOCB *blockacb)
> +{
> +    DupAIOCB *acb = container_of(blockacb, DupAIOCB, common);
> +
> +    bdrv_aio_cancel(acb->src_aiocb);
> +    bdrv_aio_cancel(acb->dest_aiocb);

Shouldn't we cancel just the ones that haven't completed yet?

> +    qemu_aio_release(acb);
> +}
> +
> +static AIOPool dup_aio_pool = {
> +    .aiocb_size         = sizeof(DupAIOCB),
> +    .cancel             = dup_aio_cancel,
> +};
> +
> +static void blkmirror_aio_cb(void *opaque, int ret)
> +{
> +    DupAIOCB *dcb = opaque;
> +
> +    dcb->count--;
> +    assert(dcb->count>= 0);
> +    if (dcb->count == 0) {
> +        if (dcb->ret<  0) {
> +            ret = dcb->ret;
> +        }
> +        dcb->common.cb(dcb->opaque, ret);
> +        qemu_aio_release(dcb);
> +    }
> +    dcb->ret = ret;
> +}

It would be nicer to do

    if (ret < 0) {
        dcb->ret = ret;
    }

unconditionally.  The current code works, but only for N=2, and is a 
little obfuscated.

I see you aren't keeping sync/unsync state here.  Will read on.

--
error compiling committee.c: too many arguments to function