Re: [Qemu-block] [PATCH v10 08/10] Implement new driver for block replication

[Stefan Hajnoczi]

On Fri, Oct 16, 2015 at 10:22:05AM +0800, Wen Congyang wrote:
> On 10/15/2015 10:55 PM, Stefan Hajnoczi wrote:
> > On Thu, Oct 15, 2015 at 10:19:17AM +0800, Wen Congyang wrote:
> >> On 10/14/2015 10:27 PM, Stefan Hajnoczi wrote:
> >>> On Tue, Oct 13, 2015 at 05:08:17PM +0800, Wen Congyang wrote:
> >>>> On 10/13/2015 12:27 AM, Stefan Hajnoczi wrote:
> >>>>> On Fri, Sep 25, 2015 at 02:17:36PM +0800, Wen Congyang wrote:
> >>>>>> +        /* start backup job now */
> >>>>>> +        bdrv_op_unblock(s->hidden_disk, BLOCK_OP_TYPE_BACKUP_TARGET,
> >>>>>> +                        s->active_disk->backing_blocker);
> >>>>>> +        bdrv_op_unblock(s->secondary_disk, 
> >>>>>> BLOCK_OP_TYPE_BACKUP_SOURCE,
> >>>>>> +                        s->hidden_disk->backing_blocker);
> >>>>>
> >>>>> Why is it safe to unblock these operations?
> >>>>>
> >>>>> Why do they have to be blocked for non-replication users?
> >>>>
> >>>> hidden_disk and secondary disk are opened as backing file, so it is 
> >>>> blocked for
> >>>> non-replication users.
> >>>> What can I do if I don't unblock it and want to do backup?
> >>>
> >>> CCing Jeff Cody, block jobs maintainer
> >>>
> >>> You need to explain why it is safe remove this protection.  We can't
> >>> merge code that may be unsafe.
> >>>
> >>> I think we can investigate further by asking: when does QEMU code assume
> >>> the backing file is read-only?
> >>
> >> The backing file is opened in read-only mode. I want to reopen it in 
> >> read-write
> >> mode here in the next version(So the patch 1 will be dropped)
> >>
> >>>
> >>> I haven't checked but these cases come to mind:
> >>>
> >>> Operations that move data between BDS in the backing chain (e.g. commit
> >>> and stream block jobs) will lose or overwrite data if the backing file
> >>> is being written to by another coroutine.
> >>>
> >>> We need to prevent users from running these operations at the same time.
> >>
> >> Yes, but qemu doesn't provide such API.
> > 
> > This series can't be merged unless it is safe.
> > 
> > Have you looked at op blockers and thought about how to prevent unsafe
> > operations?
> 
> What about this solution:
> 1. unblock it in bdrv_set_backing_hd()
> 2. block it in qmp_block_commit(), qmp_block_stream(), qmp_block_backup()..., 
> to
>    prevent unsafe operations

Come to think of it, currently QEMU only supports 1 block job per BDS.

This means that as long as COLO has a backup job running, no other block
jobs can interfere.

There still might be a risk with monitor commands like 'commit'.

Stefan