From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34670) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZmAMT-0008HD-6Q for qemu-devel@nongnu.org; Tue, 13 Oct 2015 20:56:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZmAMP-0005hg-56 for qemu-devel@nongnu.org; Tue, 13 Oct 2015 20:56:09 -0400 References: <1443161858-20533-1-git-send-email-wency@cn.fujitsu.com> <1443161858-20533-9-git-send-email-wency@cn.fujitsu.com> <20151012162539.GB4053@stefanha-thinkpad.redhat.com> <561CC804.6000602@cn.fujitsu.com> <20151013094112.GA28309@ad.nay.redhat.com> <561CD2EA.6070701@cn.fujitsu.com> <20151013101209.GB28309@ad.nay.redhat.com> From: Wen Congyang Message-ID: <561DA814.7000903@cn.fujitsu.com> Date: Wed, 14 Oct 2015 08:55:48 +0800 MIME-Version: 1.0 In-Reply-To: <20151013101209.GB28309@ad.nay.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: Fam Zheng Cc: Kevin Wolf , zhanghailiang , qemu block , Stefan Hajnoczi , Jiang Yunhong , Dong Eddie , qemu devel , "Michael R. Hines" , Max Reitz , Gonglei , Stefan Hajnoczi , Paolo Bonzini , Yang Hongyang , "Dr. David Alan Gilbert" On 10/13/2015 06:12 PM, Fam Zheng wrote: > On Tue, 10/13 17:46, Wen Congyang wrote: >> On 10/13/2015 05:41 PM, Fam Zheng wrote: >>> On Tue, 10/13 16:59, Wen Congyang wrote: >>>> On 10/13/2015 12:25 AM, Stefan Hajnoczi wrote: >>>>> On Fri, Sep 25, 2015 at 02:17:36PM +0800, Wen Congyang wrote: >>>>>> +static void backup_job_completed(void *opaque, int ret) >>>>>> +{ >>>>>> + BDRVReplicationState *s = opaque; >>>>>> + >>>>>> + if (s->replication_state != BLOCK_REPLICATION_DONE) { >>>>>> + /* The backup job is cancelled unexpectedly */ >>>>>> + s->error = -EIO; >>>>>> + } >>>>>> + >>>>>> + bdrv_op_block(s->hidden_disk, BLOCK_OP_TYPE_BACKUP_TARGET, >>>>>> + s->active_disk->backing_blocker); >>>>>> + bdrv_op_block(s->secondary_disk, BLOCK_OP_TYPE_BACKUP_SOURCE, >>>>>> + s->hidden_disk->backing_blocker); >>>>>> + >>>>>> + bdrv_put_ref_bh_schedule(s->secondary_disk); >>>>> >>>>> Why is bdrv_put_ref_bh_schedule() necessary? >>>> >>>> It is copied from block_job_cb(). According to the comments in bdrv_put_ref_bh_schedule(): >>>> /* >>>> * Release a BDS reference in a BH >>>> * >>>> * It is not safe to use bdrv_unref() from a callback function when the callers >>>> * still need the BlockDriverState. In such cases we schedule a BH to release >>>> * the reference. >>>> */ >>>> >>>> If the comment is right, I think it is necessary to call bdrv_put_ref_bh_schedule() here. >>>> Because the job is created on the BDS s->secondary disk, backup_job_completed() is >>>> called in block_job_completed(), and we will still use s->secondary_disk in block_job_release(). >>> >>> Where is the matching bdrv_ref called? >> >> It is in block_job_create().... >> >> source: we call in bdrv_ref() in block_job_create(), and the user should unref it. >> target: the user call bdrv_ref(), and we will unref it in the job >> >> I don't know why we design it like this... >> > > Maybe it's better to unref it in block_job_release. Then we can simply drop > bdrv_put_ref_bh_schedule. I agree with it. Thanks Wen Congyang > > Fam > . >