Re: [Qemu-block] [PATCH 15/18] block/mirror: Add active mirroring

[Max Reitz]

On 2017-09-14 17:57, Stefan Hajnoczi wrote:
> On Wed, Sep 13, 2017 at 08:19:07PM +0200, Max Reitz wrote:
>> This patch implements active synchronous mirroring.  In active mode, the
>> passive mechanism will still be in place and is used to copy all
>> initially dirty clusters off the source disk; but every write request
>> will write data both to the source and the target disk, so the source
>> cannot be dirtied faster than data is mirrored to the target.  Also,
>> once the block job has converged (BLOCK_JOB_READY sent), source and
>> target are guaranteed to stay in sync (unless an error occurs).
>>
>> Optionally, dirty data can be copied to the target disk on read
>> operations, too.
>>
>> Active mode is completely optional and currently disabled at runtime.  A
>> later patch will add a way for users to enable it.
>>
>> Signed-off-by: Max Reitz <address@hidden>
>> ---
>>  qapi/block-core.json |  23 +++++++
>>  block/mirror.c       | 187 
>> +++++++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 205 insertions(+), 5 deletions(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index bb11815608..e072cfa67c 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -938,6 +938,29 @@
>>    'data': ['top', 'full', 'none', 'incremental'] }
>>  
>>  ##
>> +# @MirrorCopyMode:
>> +#
>> +# An enumeration whose values tell the mirror block job when to
>> +# trigger writes to the target.
>> +#
>> +# @passive: copy data in background only.
>> +#
>> +# @active-write: when data is written to the source, write it
>> +#                (synchronously) to the target as well.  In addition,
>> +#                data is copied in background just like in @passive
>> +#                mode.
>> +#
>> +# @active-read-write: write data to the target (synchronously) both
>> +#                     when it is read from and written to the source.
>> +#                     In addition, data is copied in background just
>> +#                     like in @passive mode.
> 
> I'm not sure the terms "active"/"passive" are helpful.  "Active commit"
> means committing the top-most BDS while the guest is accessing it.  The
> "passive" mirror block still works on the top-most BDS while the guest
> is accessing it.
> 
> Calling it "asynchronous" and "synchronous" is clearer to me.  It's also
> the terminology used in disk replication (e.g. DRBD).

I'd be OK with that, too, but I think I remember that in the past at
least Kevin made a clear distinction between active/passive and
sync/async when it comes to mirroring.

> Ideally the user wouldn't have to worry about async vs sync because QEMU
> would switch modes as appropriate in order to converge.  That way
> libvirt also doesn't have to worry about this.

So here you mean async/sync in the way I meant it, i.e., whether the
mirror operations themselves are async/sync?

Maybe we could call the the passive mode "background" and the active
mode "synchronous" (or maybe even "foreground")?  Because sync then
means three things:
(1) The block job sync mode
(2) Synchronous write operations to the target
(3) Active mirroring mode

(1) is relatively easy to distinguish because it's just "the job sync
mode".  If we call the passive mode "background" we can distinguish (2)
and (3) at least based on their antonyms: "sync (as in sync/async)" vs.
"sync (as in sync/background)".

>>  static int coroutine_fn bdrv_mirror_top_preadv(BlockDriverState *bs,
>>      uint64_t offset, uint64_t bytes, QEMUIOVector *qiov, int flags)
>>  {
>> -    return bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
>> +    MirrorOp *op = NULL;
>> +    MirrorBDSOpaque *s = bs->opaque;
>> +    int ret = 0;
>> +    bool copy_to_target;
>> +
>> +    copy_to_target = s->job->ret >= 0 &&
>> +                     s->job->copy_mode == 
>> MIRROR_COPY_MODE_ACTIVE_READ_WRITE;
>> +
>> +    if (copy_to_target) {
>> +        op = active_write_prepare(s->job, offset, bytes);
>> +    }
>> +
>> +    ret = bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
>> +    if (ret < 0) {
>> +        goto out;
>> +    }
>> +
>> +    if (copy_to_target) {
>> +        do_sync_target_write(s->job, offset, bytes, qiov, 0);
>> +    }
> 
> This mode is dangerous.  See bdrv_co_do_copy_on_readv():
> 
>   /* Perform I/O through a temporary buffer so that users who scribble over
>    * their read buffer while the operation is in progress do not end up
>    * modifying the image file.  This is critical for zero-copy guest I/O
>    * where anything might happen inside guest memory.
>    */
>   void *bounce_buffer;

Ooh, yes, right.  I'll just drop it for now, then.

Max

signature.asc
Description: OpenPGP digital signature