From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59344) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG9Af-0002QY-12 for qemu-devel@nongnu.org; Wed, 19 Feb 2014 10:34:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WG9AY-000275-VP for qemu-devel@nongnu.org; Wed, 19 Feb 2014 10:34:48 -0500 Received: from lnantes-156-75-100-125.w80-12.abo.wanadoo.fr ([80.12.84.125]:50337 helo=paradis.irqsave.net) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WG9AY-00026t-N8 for qemu-devel@nongnu.org; Wed, 19 Feb 2014 10:34:42 -0500 Date: Wed, 19 Feb 2014 16:34:42 +0100 From: =?iso-8859-1?Q?Beno=EEt?= Canet Message-ID: <20140219153442.GG20622@irqsave.net> References: <1392817351-22148-1-git-send-email-famz@redhat.com> <1392817351-22148-9-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <1392817351-22148-9-git-send-email-famz@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v14 08/14] block: Support dropping active in bdrv_drop_intermediate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fam Zheng Cc: kwolf@redhat.com, benoit.canet@irqsave.net, rjones@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com The Wednesday 19 Feb 2014 =E0 21:42:25 (+0800), Fam Zheng wrote : > 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 need= s > to work with op blockers. >=20 > Signed-off-by: Fam Zheng > --- > block.c | 146 +++++++++++++++++++++++++------------------------= -------- > block/commit.c | 1 + > 2 files changed, 66 insertions(+), 81 deletions(-) >=20 > diff --git a/block.c b/block.c > index a2bf24c..cf41f3d 100644 > --- a/block.c > +++ b/block.c > @@ -2485,115 +2485,99 @@ BlockDriverState *bdrv_find_overlay(BlockDrive= rState *active, > return overlay; > } > =20 > -typedef struct BlkIntermediateStates { > - BlockDriverState *bs; > - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; > -} BlkIntermediateStates; > - > - > /* > - * Drops images above 'base' up to and including 'top', and sets the i= mage > - * 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 back= ing > + * file). top_overlay may be NULL if 'top' is active, no such update n= eeded. > + * Requires that the top_overlay to 'top' is opened r/w. > * > - * Requires that the overlay to 'top' is opened r/w, so that the backi= ng file > - * information in 'bs' can be properly updated. > + * 1) This will convert the following chain: > * > - * E.g., this will convert the following chain: > - * bottom <- base <- intermediate <- top <- active > + * ... <- 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 is allowed for bottom=3D=3Dbase, in which case it converts: > * > - * base <- intermediate <- top <- active > + * base <- ... <- top <- overlay <- ... <- active > * > * to > * > - * base <- active > + * base <- overlay <- active > + * > + * 2) It also allows active=3D=3Dtop, in which case it converts: > + * > + * ... <- base <- ... <- top (active) > + * > + * to > + * > + * ... <- 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 se= t its > + * backing_hd to NULL. I.e.: > + * > + * base(NULL) <- ... <- overlay <- ... <- active > + * > + * to > * > - * Error conditions: > - * if active =3D=3D top, that is considered an error > + * overlay <- ... <- active > * > */ > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState = *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_d= elete; > - QSIMPLEQ_INIT(&states_to_delete); > - > - if (!top->drv || !base->drv) { > - goto exit; > - } > - > - new_top_bs =3D bdrv_find_overlay(active, top); > + BlockDriverState *drop_start, *overlay; > + int ret =3D -EINVAL; > =20 > - 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 base= - 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, en= try); > - > - 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 n= ow, > + * the top image is dropped but this BDS structure is kept and= swapped > + * with base, this way we keep the pointers valid after droppi= ng 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, NULL, NULL); > + } > + if (ret) { > + goto exit; > + } > + if (base) { > + drop_start =3D top->backing_hd; > + bdrv_swap(top, base); > + /* Break the loop formed by bdrv_swap */ > + bdrv_set_backing_hd(base, NULL); > + } else { > + bdrv_set_backing_hd(overlay, NULL); > + drop_start =3D top; > } > - intermediate =3D intermediate->backing_hd; > - } > - if (base_bs =3D=3D NULL) { > - /* something went wrong, we did not end at the base. safely > - * unravel everything, and exit with error */ > - goto exit; > - } > - > - /* success - we can delete the intermediate states, and link top->= base */ > - ret =3D bdrv_change_backing_file(new_top_bs, base_bs->filename, > - base_bs->drv ? base_bs->drv->format= _name : ""); > - if (ret) { > - goto exit; > - } > - new_top_bs->backing_hd =3D base_bs; > - > - bdrv_refresh_limits(new_top_bs); > =20 > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry= , next) { > - /* so that bdrv_close() does not recursively close the chain *= / > - intermediate_state->bs->backing_hd =3D NULL; > - bdrv_unref(intermediate_state->bs); > + bdrv_unref(drop_start); > } > - ret =3D 0; > - > exit: > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry= , next) { > - g_free(intermediate_state); > - } > return ret; > } > =20 > - > static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offse= t, > size_t size) > { > diff --git a/block/commit.c b/block/commit.c > index acec4ac..b10eb79 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -142,6 +142,7 @@ wait: > if (!block_job_is_cancelled(&s->common) && sector_num =3D=3D end) = { > /* success */ > ret =3D bdrv_drop_intermediate(active, top, base); > + base =3D top; > } > =20 > exit_free_buf: > --=20 > 1.8.5.4 >=20 >=20 This is where I hit peter principle in the reviewing of this series. Will stop reviewing here and let someone stronger than me continue. Best regards Beno=EEt