Re: [Qemu-devel] [RFC PATCH] drive-backup 'stream' mode

[Eric Blake]

On 10/14/2013 02:10 PM, Wolfgang Richter wrote:

>>
>> Add the designation '(since 1.7)' to make it obvious when this mode was
>> introduced.
> 
> Done.  Is it better to place the updated patch in this thread or start
> a new one?

http://wiki.qemu.org/Contribute/SubmitAPatch suggests submitting a new
top-level thread for each revision of a patch series, along with a
changelog after the --- or in the cover letter to help focus reviewers
on what changed from the earlier revision.

> 
>>
>>>  #
>>>  # Since: 1.3
>>>  ##
>>>  { 'enum': 'MirrorSyncMode',
>>> -  'data': ['top', 'full', 'none'] }
>>> +  'data': ['top', 'full', 'none', 'stream'] }
>>
>> MirrorSyncMode is used by multiple commands; your summary mentions how
>> it would affect 'drive-backup', but what happens to 'drive-mirror'?  For
>> that matter, why isn't 'drive-mirror' with mode 'none' doing what you
>> already want?
> 
> Okay, I think my impression might be wrong, but I thought
> 'drive-mirror' would become deprecated with the new 'drive-backup'
> command and code.

No - drive-mirror and drive-backup are independent, and both useful.
Each fills a niche that the other cannot.

> 
> If we look at what they do (current documentation and code),
> 'drive-backup' AFAIK behaves the same for all modes of 'drive-mirror'
> _except_ mode 'none' with _better_ consistency guarantees.  That is,
> 'drive-backup' clearly provides a point-in-time snapshot, whereas
> 'drive-mirror' may create a point-in-time snapshot, but it can not
> guarantee that.

'drive-backup' creates a point-in-time up front.
'drive-mirror' can be used to create a point-in-time at the tail end
(when you gracefully cancel the job once it is in mirroring phase).  But
it also does not have to be canceled - as long as it is still running,
you are still mirroring data.

> 
> In addition, 'drive-backup's code is cleaner, simpler, and easier to
> work with (in my opinion) than 'drive-mirror's code.  This is because
> of the new hooks in block.c for tracked requests etc. so that the job
> can insert code to be run on every write in a clean manner (I think).
> 
> I think that it would be less confusing to subsume 'drive-mirror' into
> 'drive-backup' so that we have a single command with clear consistency
> guarantees, and also it would prevent overloading (and more confusion)
> with the meaning of the 'MirrorSyncMode's.

You can't break the existing semantics, but if you think you can unify
the code base, be my guest.

> 
> Perhaps a better naming scheme for the modes would then be:
> 
> full - as before (same for both commands AFAIK)
> top - as before (same for both commands AFAIK)
> none - if we only have drive-backup, rename this to 'overlay' as it
> creates a low-overhead CoW overlay point-in-time snapshot
> stream - either keep my name 'stream' to do what 'none' does for
> drive-mirror, or leave this as the 'none' mode with the same
> drive-mirror semantics
> 
> Thus, I think, with a single extra mode, drive-backup can subsume
> drive-mirror.  This reduces the number of commands, the documentation,
> and the code (all duplicating each other in some manner).
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature