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

[Stefan Hajnoczi]

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?

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.

Also, accessing bs->backing_blocker is a layering violation.  No one
outside block.c:bdrv_set_backing_hd() is supposed to access this field.

Let's figure out the safety concerns first and then the
bs->backing_blocker access will probably be eliminated as part of the
solution.

Stefan