On Tue, Oct 27, 2015 at 10:57:42AM +0800, Wen Congyang wrote: > 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);
BLOCK_OP_TYPE_BACKUP_SOURCE does not modify the image so this should be safe. > + bdrv_op_unblock(backing_hd, BLOCK_OP_TYPE_BACKUP_TARGET, > + bs->backing_blocker); This one is trickier since it means write access, but BLOCK_OP_TYPE_COMMIT_TARGET is already unblocked above. At least it should be no worse than allowing BLOCK_OP_TYPE_COMMIT_TARGET. Jeff? Stefan