From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51517) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrJSn-0005UK-AW for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:30:59 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VrJSh-0005QG-8b for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:30:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36208) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrJSh-0005QA-0e for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:30:47 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBD3UkDo009585 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 12 Dec 2013 22:30:46 -0500 Message-ID: <52AA7F56.9010608@redhat.com> Date: Fri, 13 Dec 2013 11:30:30 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1386836626-6436-1-git-send-email-famz@redhat.com> <1386836626-6436-5-git-send-email-famz@redhat.com> <20131212132457.GJ10007@dhcp-200-207.str.redhat.com> In-Reply-To: <20131212132457.GJ10007@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 v7 04/10] block: support dropping active in bdrv_drop_intermediate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: armbru@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com On 2013=E5=B9=B412=E6=9C=8812=E6=97=A5 21:24, Kevin Wolf wrote: > Am 12.12.2013 um 09:23 hat Fam Zheng geschrieben: >> Dropping intermediate could be useful both for commit and stream, and >> BDS refcnt plus bdrv_swap could do most of the job nicely. It also nee= d >> some improvements in preparation for op blockers. >> >> Signed-off-by: Fam Zheng >> --- >> block.c | 152 +++++++++++++++++++++++++++--------------------= ---------- >> block/commit.c | 1 + >> 2 files changed, 74 insertions(+), 79 deletions(-) >> >> diff --git a/block.c b/block.c >> index 41562fd..681d3be 100644 >> --- a/block.c >> +++ b/block.c >> @@ -2163,114 +2163,108 @@ BlockDriverState *bdrv_find_overlay(BlockDri= verState *active, >> return overlay; >> } >> >> -typedef struct BlkIntermediateStates { >> - BlockDriverState *bs; >> - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; >> -} BlkIntermediateStates; >> - >> +static void bdrv_set_backing_hd(BlockDriverState *bs, >> + BlockDriverState *new_backing) >> +{ >> + if (bs->backing_hd) { >> + bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); >> + } >> + bs->backing_hd =3D new_backing; >> + if (new_backing) { >> + bdrv_op_block_all(bs->backing_hd, bs->backing_blocker); > > What about unblocking commit, like you did in patch 3? > > Should bdrv_open_backing_file() be using this function? > Yes, will do that. >> + } >> +} >> >> /* >> - * Drops images above 'base' up to and including 'top', and sets the = image >> - * above 'top' to have base as its backing file. >> + * Drops images above 'base' up to and including 'top', and sets new = 'base' >> + * as backing_hd of top_overlay (the image orignally has 'top' as bac= king >> + * file). top_overlay may be NULL if 'top' is active, no such update = needed. >> + * Requires that the top_overlay to 'top' is opened r/w. >> * >> - * Requires that the overlay to 'top' is opened r/w, so that the back= ing file >> - * information in 'bs' can be properly updated. >> + * 1) This will convert the following chain: >> + * ... <- base <- ... <- top <- overlay <-... <- active >> * >> - * E.g., this will convert the following chain: >> - * bottom <- base <- intermediate <- top <- active >> + * to >> + * >> + * ... <- base <- overlay <- active >> + * >> + * 2) It is allowed for bottom=3D=3Dbase, in which case it converts: >> + * >> + * ... <- base <- ... <- top <- overlay <- ... <- active >> * >> * to >> * >> - * bottom <- base <- active >> + * base <- overlay <- active >> * >> - * It is allowed for bottom=3D=3Dbase, in which case it converts: >> + * 2) It also allows active=3D=3Dtop, in which case it converts: >> * >> - * base <- intermediate <- top <- active >> + * ... <- base <- ... <- top (active) >> * >> * to >> * >> - * base <- active >> + * base =3D=3D active =3D=3D top, i.e. only base and lower remains: *= top =3D=3D *base when >> + * return. >> + * >> + * 3) If base=3D=3DNULL, it will drop all the BDS below overlay and s= et its >> + * backing_hd to NULL. I.e.: >> * >> - * Error conditions: >> - * if active =3D=3D top, that is considered an error >> + * base(NULL) <- ... <- overlay <- ... <- active >> * >> + * to >> + * >> + * overlay <- ... <- active >> */ >> int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverStat= e *top, >> BlockDriverState *base) >> { >> - BlockDriverState *intermediate; >> - BlockDriverState *base_bs =3D NULL; >> - BlockDriverState *new_top_bs =3D NULL; >> - BlkIntermediateStates *intermediate_state, *next; >> - int ret =3D -EIO; >> - >> - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_= delete; >> - QSIMPLEQ_INIT(&states_to_delete); >> - >> - if (!top->drv || !base->drv) { >> - goto exit; >> - } >> + BlockDriverState *drop_start, *overlay; >> + int ret =3D -EINVAL; >> >> - new_top_bs =3D bdrv_find_overlay(active, top); >> - >> - if (new_top_bs =3D=3D NULL) { >> - /* we could not find the image above 'top', this is an error = */ >> + if (!top->drv || (base && !base->drv)) { >> goto exit; >> } >> - >> - /* special case of new_top_bs->backing_hd already pointing to bas= e - nothing >> - * to do, no intermediate images */ >> - if (new_top_bs->backing_hd =3D=3D base) { >> + if (top =3D=3D base) { >> ret =3D 0; >> - goto exit; >> - } >> - >> - intermediate =3D top; >> - >> - /* now we will go down through the list, and add each BDS we find >> - * into our deletion queue, until we hit the 'base' >> - */ >> - while (intermediate) { >> - intermediate_state =3D g_malloc0(sizeof(BlkIntermediateStates= )); >> - intermediate_state->bs =3D intermediate; >> - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, e= ntry); >> - >> - if (intermediate->backing_hd =3D=3D base) { >> - base_bs =3D intermediate->backing_hd; >> - break; >> + } else if (top =3D=3D active) { >> + assert(base); >> + drop_start =3D active->backing_hd; >> + bdrv_swap(active, base); >> + base->backing_hd =3D NULL; >> + bdrv_unref(drop_start); >> + ret =3D 0; >> + } else { >> + /* If there's an overlay, its backing_hd points to top's BDS = now, >> + * the top image is dropped but this BDS structure is kept an= d swapped >> + * with base, this way we keep the pointers valid after dropp= ing top */ >> + overlay =3D bdrv_find_overlay(active, top); >> + if (!overlay) { >> + goto exit; >> + } >> + if (base) { >> + ret =3D bdrv_change_backing_file(overlay, base->filename, >> + base->drv->format_name); >> + } else { >> + ret =3D bdrv_change_backing_file(overlay, "", ""); > > This should be NULL, NULL instead of empty strings. > OK, thanks. Fam