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