Re: [Qemu-devel] [PATCH 5/6] Add blkmirror block driver

[Stefan Hajnoczi]

On Wed, Feb 29, 2012 at 1:37 PM, Paolo Bonzini <address@hidden> wrote:
> From: Marcelo Tosatti <address@hidden>
>
> Mirrored writes are used by live block copy.
>
> The blkmirror driver is for internal use only, because it requires
> bdrv_append to set up a backing_hd for it.  It relies on a quirk
> of bdrv_append, which leaves the old image open for writes.
>
> The source is hardcoded as the backing_hd for the destination, so that
> copy-on-write functions properly.  Since the source is not yet available
> at the time blkmirror_open is called, the backing_hd is set later.
>
> Signed-off-by: Marcelo Tosatti <address@hidden>
> Signed-off-by: Federico Simoncelli <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>        This version of the driver is almost entirely rewritten to
>        use bs->backing_hd and bs->file.  This is necessary in order
>        to share as much code as possible with group snapshots.
>
>  Makefile.objs      |    2 +-
>  block/blkmirror.c  |  153 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  docs/blkmirror.txt |   16 ++++++
>  3 files changed, 170 insertions(+), 1 deletions(-)
>  create mode 100644 block/blkmirror.c
>  create mode 100644 docs/blkmirror.txt

Mostly happy here, I just recommend tweaking the name of this block
driver and documenting clearly that this is not a general-purpose
mirroring driver, given that it points image B's backing file at image
A's backing file.  I see this driver as internal functionality and
it's fairly easy for users to misuse it and be surprised by the
results.

> +static int blkmirror_co_writev(BlockDriverState *bs,
> +                               int64_t sector_num, int nb_sectors,
> +                               QEMUIOVector *qiov)
> +{
> +    int ret;
> +
> +    /* bs->backing_hd is set after initialization.  */
> +    bs->file->backing_hd = bs->backing_hd;
> +
> +    ret = bdrv_co_writev(bs->backing_hd, sector_num, nb_sectors, qiov);
> +    if (ret >= 0) {
> +        ret = bdrv_co_writev(bs->file, sector_num, nb_sectors, qiov);
> +    }
> +
> +    return ret;
> +}

Have you done performance tests?  It seems suboptimal to use
.bdrv_co_writev() and perform writes sequentially, even with
cache=unsafe.