From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59394) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZlwZC-00076l-IL for qemu-devel@nongnu.org; Tue, 13 Oct 2015 06:12:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZlwZB-00052C-Il for qemu-devel@nongnu.org; Tue, 13 Oct 2015 06:12:22 -0400 Date: Tue, 13 Oct 2015 18:12:09 +0800 From: Fam Zheng Message-ID: <20151013101209.GB28309@ad.nay.redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <561CD2EA.6070701@cn.fujitsu.com> 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: Wen Congyang 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 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. Fam