Re: [Qemu-devel] [Patch v12 resend 08/10] Implement new driver for block replication

[Stefan Hajnoczi]

On Wed, Dec 02, 2015 at 01:37:25PM +0800, Wen Congyang wrote:
> +    /*
> +     * Only write to active disk if the sectors have
> +     * already been allocated in active disk/hidden disk.
> +     */
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +    while (remaining_sectors > 0) {
> +        ret = bdrv_is_allocated_above(top, base, sector_num,
> +                                      remaining_sectors, &n);

There is a race condition here since multiple I/O requests can be in
flight at the same time.   If two requests touch the same cluster
between top->base then the result of these checks could be unreliable.

The simple but slow solution is to use a CoMutex to serialize requests.

> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        qemu_iovec_reset(&hd_qiov);
> +        qemu_iovec_concat(&hd_qiov, qiov, bytes_done, n * 512);
> +
> +        target = ret ? top : base;
> +        ret = bdrv_co_writev(target, sector_num, n, &hd_qiov);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        remaining_sectors -= n;
> +        sector_num += n;
> +        bytes_done += n * BDRV_SECTOR_SIZE;
> +    }

I think this can be replaced with an active commit block job that copies
data down from the hidden/active disk to the secondary disk.  It is okay
to keep writing to the secondary disk while the block job is running and
then switch over to the secondary disk once it completes.

> +
> +    return 0;
> +}
> +
> +static coroutine_fn int replication_co_discard(BlockDriverState *bs,
> +                                               int64_t sector_num,
> +                                               int nb_sectors)
> +{
> +    BDRVReplicationState *s = bs->opaque;
> +    int ret;
> +
> +    ret = replication_get_io_status(s);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    if (ret == 1) {
> +        /* It is secondary qemu and we are after failover */
> +        ret = bdrv_co_discard(s->secondary_disk, sector_num, nb_sectors);

What if the clusters are still allocated in the hidden/active disk?

signature.asc
Description: PGP signature