From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46972) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1adUn1-0003WX-VV for qemu-devel@nongnu.org; Tue, 08 Mar 2016 22:28:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1adUn1-0006Mh-2C for qemu-devel@nongnu.org; Tue, 08 Mar 2016 22:27:59 -0500 Message-ID: <56DF9891.4010207@cn.fujitsu.com> Date: Wed, 9 Mar 2016 11:29:21 +0800 From: Changlong Xie MIME-Version: 1.0 References: <1454645888-28826-1-git-send-email-xiecl.fnst@cn.fujitsu.com> <1454645888-28826-9-git-send-email-xiecl.fnst@cn.fujitsu.com> <20160304173954.GF9130@stefanha-x1.localdomain> In-Reply-To: <20160304173954.GF9130@stefanha-x1.localdomain> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v15 8/9] 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 , Jiang Yunhong , Dong Eddie , qemu devel , "Michael R. Hines" , Max Reitz , Gonglei , Stefan Hajnoczi , Paolo Bonzini , "Dr. David Alan Gilbert" On 03/05/2016 01:39 AM, Stefan Hajnoczi wrote: > On Fri, Feb 05, 2016 at 12:18:07PM +0800, Changlong Xie wrote: >> +static void replication_start(ReplicationState *rs, ReplicationMode mode, >> + Error **errp) >> +{ >> + BlockDriverState *bs = rs->opaque; >> + BDRVReplicationState *s = bs->opaque; >> + int64_t active_length, hidden_length, disk_length; >> + AioContext *aio_context; >> + Error *local_err = NULL; >> + >> + if (s->replication_state != BLOCK_REPLICATION_NONE) { > > Dereferencing bs and s is not thread-safe since the AioContext isn't > acquired here. Unless you have other locking rules, the default is that > everything in BlockDriverState and bs->opaque must be accessed with the > AioContext acquired. > > Please apply this comment to the rest of the patch series, too. > Functions that are not part of BlockDriver are probably called outside > the AioContext and must acquire it before they are allowed to access > anything else in bs. > >> + s->top_bs = get_top_bs(bs); >> + if (!s->top_bs) { >> + error_setg(errp, "Cannot get the top block driver state to do" >> + " internal backup"); >> + return; >> + } > > Traversing the BDS graph using the parent pointer is not safe - you > cannot stash the parent pointer because it changes if a parent node is > inserted/removed. > > I suggest passing the drive name as an argument so that bdrv_lookup_bs() > can be used when installing the op blocker. Then the BdrvChild.parent > pointer patch and get_top_bs() can be deleted. > > Jeff Cody is currently working on a new op blocker system. Hopefully > this system will allow you to install blockers on bs instead of on the > drive's top BDS. But let's not worry about that for now and just use > the drive name as an argument. > >> + /* >> + * Must protect backup target if backup job was stopped/cancelled >> + * unexpectedly >> + */ >> + bdrv_ref(s->hidden_disk->bs); > > Where is the matching bdrv_unref() call? Two conditions 1. job is cancelled, so "local_err != 0" 449 if (local_err) { 450 error_propagate(errp, local_err); 451 backup_job_cleanup(s); 452 bdrv_unref(s->hidden_disk->bs); 453 return; 454 } 2. backup completed backup_complete bdrv_unref(s->target); If i'm wrong, pls correct me. Thanks -Xie >