From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41185) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ad8Kh-0003Pb-J1 for qemu-devel@nongnu.org; Mon, 07 Mar 2016 22:29:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ad8Kg-0007ti-Lf for qemu-devel@nongnu.org; Mon, 07 Mar 2016 22:29:15 -0500 Message-ID: <56DE4771.40401@cn.fujitsu.com> Date: Tue, 8 Mar 2016 11:30:57 +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. > Will do implement in replication_start/stop/checkpoint/get_error callbacks. >> + 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. > ok > 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. Good news. Thanks > >> + /* >> + * Must protect backup target if backup job was stopped/cancelled >> + * unexpectedly >> + */ >> + bdrv_ref(s->hidden_disk->bs); > > Where is the matching bdrv_unref() call? >