Re: [Qemu-devel] [PATCH V6 0/3] Implement sync modes for drive-backup.

[Fam Zheng]

On Tue, 07/23 12:55, Ian Main wrote:
> On Tue, Jul 23, 2013 at 01:53:51PM +0200, Stefan Hajnoczi wrote:
> > On Mon, Jul 22, 2013 at 03:09:17PM -0700, Ian Main wrote:
> > > This patch adds sync modes on top of the work that Stefan Hajnoczi has 
> > > done.
> > > 
> > > These patches apply on kevin/block.
> > > 
> > > Hopefully all is in order as this is my first QEMU patch.  Many thanks to
> > > Stephan and Fam Zheng for their help.
> > > 
> > > V2:
> > > 
> > > - No longer poll, instead use qemu_coroutine_yield().
> > > - Use bdrv_co_is_allocated().
> > > - Much better SYNC_MODE_NONE test.
> > > 
> > > V3:
> > > 
> > > - A few style fixes.
> > > - Better commit message explaining how TOP and NONE operate.
> > > - Verified using checkpatch.pl.
> > > 
> > > V4:
> > > 
> > > - Add patch to use the source as a backing hd during backup.
> > > - Add patch to default sync mode none to qcow2.
> > > 
> > > V5:
> > > 
> > > - Fix qcow2 patch.  Forgot to git add final version.
> > > 
> > > V6:
> > > 
> > > - Default to requiring 'format' when mode is absolute-paths.
> > > - Removed one bad hunk that was misapplying.
> > > - Fixed docs, examples and tests to match changes.
> > > - Added tests for format bad/missing.
> > > - Added bdrv_set_in_use() to target.
> > > - Default to qcow2 patch not required.
> > > 
> > > Ian Main (3):
> > >   Implement sync modes for drive-backup.
> > >   Add tests for sync modes 'TOP' and 'NONE'
> > >   Add backing drive while performing backup.
> > > 
> > >  block/backup.c                | 107 
> > > +++++++++++++++++++++++++++++------------
> > >  blockdev.c                    |  36 +++++++++-----
> > >  include/block/block_int.h     |   4 +-
> > >  qapi-schema.json              |   4 +-
> > >  qmp-commands.hx               |   2 +
> > >  tests/qemu-iotests/055        | 108 
> > > +++++++++++++++++++++++++++++++++++++-----
> > >  tests/qemu-iotests/055.out    |   4 +-
> > >  tests/qemu-iotests/group      |   2 +-
> > >  tests/qemu-iotests/iotests.py |   5 ++
> > >  9 files changed, 211 insertions(+), 61 deletions(-)
> > 
> > This patch mostly takes care of image fleecing except it does not give
> > the target a device name which can be used by nbd-server-add.
> 
> It's not clear to me how to do that.  I looked through Fams series and
> was hoping to find a clue there but I may have missed it.

In short, my patches added QMP, the device id version of drive-backup:

    blockdev-backup device=source-id target=target-id

where it needs the target already there:

    (HMP) drive_add ...,backing=source-id"

"backing=source-id" is added there too, it's just the same purpose to
override backing_hd, as you added in block/backup.c

Then we have the target-id to use with NBD.

>  
> > Fam's series tackles the target device name and some of the overlapping
> > problems with your series.
> 
> Yes which is great.
>  
> > The core feature in your series is sync=top|none and that needs to be
> > merged.
> > 
> > Now we need to figure out which patches to take and what must be
> > changed.  Please see the sub-threads on Fam's series.  Perhaps we can
> > reach a consensus there.
> 
> Yes I'm happy to offer whatever help this is.  It does seem like
> applying this is the next logical move however.  Fam could then base his
> patch on top of this one.  At some point soon we need to reconcile
> everything and to me that seems the logical way.  There actually doesn't
> seem to be too much overlap in effort from what I can see.

The overlapping I can see is overriding backing_hd, but I'm not sure
which way will end merged, or both.

> 
> Fam, what do you think?  I'm happy to make changes to make it easier for
> your patchset too.
> 
I think it's fine till now, the hard and unresolved parts are adding
bits to control interface, which is independent on your patches.

Thanks.

-- 
Fam