From patchwork Thu Oct 1 13:13:33 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 525175 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id D930A14016A for ; Fri, 2 Oct 2015 02:34:31 +1000 (AEST) Received: from localhost ([::1]:52963 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhgoP-0007kV-Jz for incoming@patchwork.ozlabs.org; Thu, 01 Oct 2015 12:34:29 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32847) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zhdgr-0003m1-3R for qemu-devel@nongnu.org; Thu, 01 Oct 2015 09:14:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zhdgp-0002ff-RM for qemu-devel@nongnu.org; Thu, 01 Oct 2015 09:14:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:55559) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zhdge-0002cJ-Tl; Thu, 01 Oct 2015 09:14:17 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 7A31DA3810; Thu, 1 Oct 2015 13:14:16 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-91.ams2.redhat.com [10.36.116.91]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t91DDbtl016338; Thu, 1 Oct 2015 09:14:14 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Thu, 1 Oct 2015 15:13:33 +0200 Message-Id: <1443705214-9304-16-git-send-email-kwolf@redhat.com> In-Reply-To: <1443705214-9304-1-git-send-email-kwolf@redhat.com> References: <1443705214-9304-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.23 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, jcody@redhat.com, armbru@redhat.com, mreitz@redhat.com, stefanha@redhat.com Subject: [Qemu-devel] [PATCH v2 15/16] block: Add and use bdrv_replace_in_backing_chain() X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org This cleans up the mess we left behind in the mirror code after the previous patch. Instead of using bdrv_swap(), just change pointers. The interface change of the mirror job that callers must consider is that after job completion, their local BDS pointers still point to the same node now. qemu-img must change its code accordingly (which makes it easier to understand); the other callers stays unchanged because after completion they don't do anything with the BDS, but just with the job, and the job is still owned by the source BDS. Signed-off-by: Kevin Wolf Reviewed-by: Max Reitz --- block.c | 32 +++++++++++++++++++++++++++++++- block/mirror.c | 23 +++++++---------------- include/block/block.h | 4 +++- qemu-img.c | 16 ++++++++-------- 4 files changed, 49 insertions(+), 26 deletions(-) diff --git a/block.c b/block.c index 54ef5de..5e0ad24 100644 --- a/block.c +++ b/block.c @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, return child; } -void bdrv_detach_child(BdrvChild *child) +static void bdrv_detach_child(BdrvChild *child) { QLIST_REMOVE(child, next); QLIST_REMOVE(child, next_parent); @@ -2232,6 +2232,36 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) bdrv_unref(bs_new); } +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new) +{ + assert(!bdrv_requests_pending(old)); + assert(!bdrv_requests_pending(new)); + + bdrv_ref(old); + + if (old->blk) { + /* As long as these fields aren't in BlockBackend, but in the top-level + * BlockDriverState, it's not possible for a BDS to have two BBs. + * + * We really want to copy the fields from old to new, but we go for a + * swap instead so that pointers aren't duplicated and cause trouble. + * (Also, bdrv_swap() used to do the same.) */ + assert(!new->blk); + swap_feature_fields(old, new); + } + change_parent_backing_link(old, new); + + /* Change backing files if a previously independent node is added to the + * chain. For active commit, we replace top by its own (indirect) backing + * file and don't do anything here so we don't build a loop. */ + if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) { + bdrv_set_backing_hd(new, backing_bs(old)); + bdrv_set_backing_hd(old, NULL); + } + + bdrv_unref(old); +} + static void bdrv_delete(BlockDriverState *bs) { assert(!bs->job); diff --git a/block/mirror.c b/block/mirror.c index 0597ad7..78f4c3f 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -353,6 +353,11 @@ static void mirror_exit(BlockJob *job, void *opaque) MirrorBlockJob *s = container_of(job, MirrorBlockJob, common); MirrorExitData *data = opaque; AioContext *replace_aio_context = NULL; + BlockDriverState *src = s->common.bs; + + /* Make sure that the source BDS doesn't go away before we called + * block_job_completed(). */ + bdrv_ref(src); if (s->to_replace) { replace_aio_context = bdrv_get_aio_context(s->to_replace); @@ -367,22 +372,7 @@ static void mirror_exit(BlockJob *job, void *opaque) if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) { bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL); } - bdrv_swap(s->target, to_replace); - if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) { - /* drop the bs loop chain formed by the swap: break the loop then - * trigger the unref */ - /* FIXME This duplicates bdrv_set_backing_hd(), except for the - * actual detach/unref so that the loop can be broken. When - * bdrv_swap() gets replaced, this will become sane again. */ - BlockDriverState *backing = s->base->backing->bs; - assert(s->base->backing_blocker); - bdrv_op_unblock_all(backing, s->base->backing_blocker); - error_free(s->base->backing_blocker); - s->base->backing_blocker = NULL; - bdrv_detach_child(s->base->backing); - s->base->backing = NULL; - bdrv_unref(backing); - } + bdrv_replace_in_backing_chain(to_replace, s->target); } if (s->to_replace) { bdrv_op_unblock_all(s->to_replace, s->replace_blocker); @@ -396,6 +386,7 @@ static void mirror_exit(BlockJob *job, void *opaque) bdrv_unref(s->target); block_job_completed(&s->common, data->ret); g_free(data); + bdrv_unref(src); } static void coroutine_fn mirror_run(void *opaque) diff --git a/include/block/block.h b/include/block/block.h index 5d9092c..1520dee 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -203,6 +203,9 @@ BlockDriverState *bdrv_new(void); void bdrv_make_anon(BlockDriverState *bs); void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old); void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top); +void bdrv_replace_in_backing_chain(BlockDriverState *old, + BlockDriverState *new); + int bdrv_parse_cache_flags(const char *mode, int *flags); int bdrv_parse_discard_flags(const char *mode, int *flags); BdrvChild *bdrv_open_child(const char *filename, @@ -513,7 +516,6 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs); void bdrv_ref(BlockDriverState *bs); void bdrv_unref(BlockDriverState *bs); void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child); -void bdrv_detach_child(BdrvChild *child); bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); diff --git a/qemu-img.c b/qemu-img.c index e678fdc..3025776 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -766,12 +766,12 @@ static int img_commit(int argc, char **argv) goto done; } - /* The block job will swap base_bs and bs (which is not what we really want - * here, but okay) and unref base_bs (after the swap, i.e., the old top - * image). In order to still be able to empty that top image afterwards, - * increment the reference counter here preemptively. */ + /* When the block job completes, the BlockBackend reference will point to + * the old backing file. In order to avoid that the top image is already + * deleted, so we can still empty it afterwards, increment the reference + * counter here preemptively. */ if (!drop) { - bdrv_ref(base_bs); + bdrv_ref(bs); } run_block_job(bs->job, &local_err); @@ -779,8 +779,8 @@ static int img_commit(int argc, char **argv) goto unref_backing; } - if (!drop && base_bs->drv->bdrv_make_empty) { - ret = base_bs->drv->bdrv_make_empty(base_bs); + if (!drop && bs->drv->bdrv_make_empty) { + ret = bs->drv->bdrv_make_empty(bs); if (ret) { error_setg_errno(&local_err, -ret, "Could not empty %s", filename); @@ -790,7 +790,7 @@ static int img_commit(int argc, char **argv) unref_backing: if (!drop) { - bdrv_unref(base_bs); + bdrv_unref(bs); } done: