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
