On 10/16/2015 07:37 PM, Stefan Hajnoczi wrote:
> 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'.
What about this?
diff --git a/block.c b/block.c
index e9f40dc..b181d67 100644
--- a/block.c
+++ b/block.c
@@ -1162,6 +1162,24 @@ void bdrv_set_backing_hd(BlockDriverState *bs,
BlockDriverState *backing_hd)
/* Otherwise we won't be able to commit due to check in bdrv_commit */
bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_COMMIT_TARGET,
bs->backing_blocker);
+ /*
+ * We do backup in 3 ways:
+ * 1. drive backup
+ * The target bs is new opened, and the source is top BDS
+ * 2. blockdev backup
+ * Both the source and the target are top BDSes.
+ * 3. internal backup(used for block replication)
+ * Both the source and the target are backing file
+ *
+ * In case 1, and 2, the backing file is neither the source nor
+ * the target.
+ * In case 3, we will block the top BDS, so there is only one block
+ * job for the top BDS and its backing chain.
+ */
+ bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_SOURCE,
+ bs->backing_blocker);
+ bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET,
+ bs->backing_blocker);
out:
bdrv_refresh_limits(bs, NULL);
}
>
> Stefan
> .
>
