From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57651) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsRcN-000380-9P for qemu-devel@nongnu.org; Mon, 16 Dec 2013 01:25:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VsRcE-0005Tz-Ux for qemu-devel@nongnu.org; Mon, 16 Dec 2013 01:25:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:11113) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsRcE-0005Tm-NP for qemu-devel@nongnu.org; Mon, 16 Dec 2013 01:25:18 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBG6PHBE025120 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 16 Dec 2013 01:25:17 -0500 Message-ID: <52AE9CC3.20600@redhat.com> Date: Mon, 16 Dec 2013 14:25:07 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1385444708-19439-1-git-send-email-famz@redhat.com> <1385444708-19439-5-git-send-email-famz@redhat.com> <20131213182643.GN3916@dhcp-200-207.str.redhat.com> In-Reply-To: <20131213182643.GN3916@dhcp-200-207.str.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 4/6] commit: support commit active layer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: pbonzini@redhat.com, jcody@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On 2013=E5=B9=B412=E6=9C=8814=E6=97=A5 02:26, Kevin Wolf wrote: > Am 26.11.2013 um 06:45 hat Fam Zheng geschrieben: >> If active is top, it will be mirrored to base, (with block/mirror.c >> code), then the image is switched when user completes the block job. >> >> QMP documentation is updated. >> >> Signed-off-by: Fam Zheng >> --- >> block/mirror.c | 11 +++++++++++ >> blockdev.c | 9 +++++++-- >> qapi-schema.json | 5 +++-- >> 3 files changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/block/mirror.c b/block/mirror.c >> index 9be741a..86ffac8 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -478,6 +478,13 @@ immediate_exit: >> bdrv_reopen(s->target, bdrv_get_flags(s->common.bs), NUL= L); >> } >> bdrv_swap(s->target, s->common.bs); >> + if (s->common.driver->job_type =3D=3D BLOCK_JOB_TYPE_COMMIT) = { >> + /* drop the bs loop chain formed by the swap: break the l= oop then >> + * trigger the unref from the top one */ >> + BlockDriverState *p =3D s->base->backing_hd; >> + s->base->backing_hd =3D NULL; >> + bdrv_unref(p); Dropping "p" here (points to sn1 in your example) recursively results in = ... > > I'm not sure about the refcounts... > > Let's assume we have the following backing file chain, refcount in > brackets: > > +------ NBD server > v > base (1) <-- sn1 (1) <-- sn2 (3) <-- guest > ^ > +------ something else ;-) > >> + } >> } >> bdrv_unref(s->target); >> block_job_completed(&s->common, ret); >> @@ -619,6 +626,10 @@ void commit_active_start(BlockDriverState *bs, Bl= ockDriverState *base, >> BlockDriverCompletionFunc *cb, >> void *opaque, Error **errp) >> { >> + if (bdrv_reopen(base, bs->open_flags, errp)) { >> + return; >> + } >> + bdrv_ref(base); > > So when we start, the refcount changes as follows: > > +--- commit job +------ NBD server > v v > base (2) <-- sn1 (1) <-- sn2 (3) <-- guest > ^ > +------ something else > > > Once all the data is copied over, we get o the code above, and first do > a bdrv_swap, which results in the following: > > +--- commit job +------ NBD server > v v > sn2 (2) <-- sn1 (1) base (3) <-- guest > | ^ ^ > +------------+ +------ something else > > > Break the loop (but no refcount update!): > > +--- commit job +------ NBD server > v v > sn2 (2) <-- sn1 (1) base (3) <-- guest > ^ > +------ something else > ... sn2(1) left alone. It is also reference by s->target and is=20 bdrv_unref'ed right before block_job_completed(). So... > > Drop the commit job's reference: > > +------ NBD server > v > sn2 (1) <-- sn1 (1) base (3) <-- guest > ^ > +------ something else > you mean sn2 (1) <-- sn1 (1) is leaked but I double checked it is not.=20 (Did you overlooked the bdrv_unref(p)?) Fam