From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34058) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZquTm-00020T-22 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 22:59:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZquTi-0001au-01 for qemu-devel@nongnu.org; Mon, 26 Oct 2015 22:59:17 -0400 References: <1443161858-20533-1-git-send-email-wency@cn.fujitsu.com> <1443161858-20533-9-git-send-email-wency@cn.fujitsu.com> <20151012162714.GC4053@stefanha-thinkpad.redhat.com> <561CCA01.7000001@cn.fujitsu.com> <20151014142751.GD16162@stefanha-thinkpad.redhat.com> <561F0D25.9000409@cn.fujitsu.com> <20151015145558.GA21733@stefanha-thinkpad.redhat.com> <56205F4D.5030604@cn.fujitsu.com> <20151016113757.GA10205@stefanha-thinkpad.redhat.com> From: Wen Congyang Message-ID: <562EE826.7000902@cn.fujitsu.com> Date: Tue, 27 Oct 2015 10:57:42 +0800 MIME-Version: 1.0 In-Reply-To: <20151016113757.GA10205@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v10 08/10] Implement new driver for block replication List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Fam Zheng , zhanghailiang , qemu block , Stefan Hajnoczi , Jeff Cody , Jiang Yunhong , Dong Eddie , qemu devel , "Michael R. Hines" , Max Reitz , Gonglei , Paolo Bonzini , Yang Hongyang , "Dr. David Alan Gilbert" 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 > . >