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

[Wen Congyang]

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.

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

I agree with it.

Thanks
Wen Congyang

> 
> 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
> .
>