From patchwork Fri Oct 16 15:05:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kevin Wolf X-Patchwork-Id: 531393 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 CF3001402B0 for ; Sat, 17 Oct 2015 02:22:24 +1100 (AEDT) Received: from localhost ([::1]:54364 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zn6pq-00069y-Ov for incoming@patchwork.ozlabs.org; Fri, 16 Oct 2015 11:22:22 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58717) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zn6aq-0003Qi-PF for qemu-devel@nongnu.org; Fri, 16 Oct 2015 11:06:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zn6al-0007Yo-8d for qemu-devel@nongnu.org; Fri, 16 Oct 2015 11:06:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46632) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zn6ad-0007TY-Ox; Fri, 16 Oct 2015 11:06:39 -0400 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 (Postfix) with ESMTPS id 404D5C0BFD24; Fri, 16 Oct 2015 15:06:39 +0000 (UTC) Received: from noname.redhat.com (ovpn-116-19.ams2.redhat.com [10.36.116.19]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t9GF6HWp001639; Fri, 16 Oct 2015 11:06:37 -0400 From: Kevin Wolf To: qemu-block@nongnu.org Date: Fri, 16 Oct 2015 17:05:57 +0200 Message-Id: <1445007974-11433-13-git-send-email-kwolf@redhat.com> In-Reply-To: <1445007974-11433-1-git-send-email-kwolf@redhat.com> References: <1445007974-11433-1-git-send-email-kwolf@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 209.132.183.28 Cc: kwolf@redhat.com, qemu-devel@nongnu.org Subject: [Qemu-devel] [PULL 12/29] block: Manage backing file references in bdrv_set_backing_hd() 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 simplifies the code somewhat, especially when dropping whole backing file subchains. The exception is the mirroring code that does adventurous things with bdrv_swap() and in order to keep it working, I had to duplicate most of bdrv_set_backing_hd() locally. We'll get rid again of this ugliness shortly. Signed-off-by: Kevin Wolf Reviewed-by: Alberto Garcia Reviewed-by: Max Reitz Reviewed-by: Stefan Hajnoczi --- block.c | 68 ++++++++++++++------------------------------------- block/mirror.c | 16 +++++++++--- block/stream.c | 30 +---------------------- block/vvfat.c | 6 ++++- include/block/block.h | 1 + 5 files changed, 37 insertions(+), 84 deletions(-) diff --git a/block.c b/block.c index ecc0885..a9c7ea6 100644 --- a/block.c +++ b/block.c @@ -1094,7 +1094,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs, return child; } -static void bdrv_detach_child(BdrvChild *child) +void bdrv_detach_child(BdrvChild *child) { QLIST_REMOVE(child, next); g_free(child); @@ -1112,13 +1112,20 @@ void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child) bdrv_unref(child_bs); } +/* + * Sets the backing file link of a BDS. A new reference is created; callers + * which don't need their own reference any more must call bdrv_unref(). + */ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) { + if (backing_hd) { + bdrv_ref(backing_hd); + } if (bs->backing) { assert(bs->backing_blocker); bdrv_op_unblock_all(bs->backing->bs, bs->backing_blocker); - bdrv_detach_child(bs->backing); + bdrv_unref_child(bs, bs->backing); } else if (backing_hd) { error_setg(&bs->backing_blocker, "node is used as backing hd of '%s'", @@ -1214,7 +1221,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp) goto free_exit; } + /* Hook up the backing file link; drop our reference, bs owns the + * backing_hd reference now */ bdrv_set_backing_hd(bs, backing_hd); + bdrv_unref(backing_hd); free_exit: g_free(backing_filename); @@ -1891,11 +1901,7 @@ void bdrv_close(BlockDriverState *bs) bs->drv->bdrv_close(bs); bs->drv = NULL; - if (bs->backing) { - BlockDriverState *backing_hd = bs->backing->bs; - bdrv_set_backing_hd(bs, NULL); - bdrv_unref(backing_hd); - } + bdrv_set_backing_hd(bs, NULL); if (bs->file != NULL) { bdrv_unref_child(bs, bs->file); @@ -2378,12 +2384,6 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) return bdrv_find_overlay(bs, NULL); } -typedef struct BlkIntermediateStates { - BlockDriverState *bs; - QSIMPLEQ_ENTRY(BlkIntermediateStates) entry; -} BlkIntermediateStates; - - /* * Drops images above 'base' up to and including 'top', and sets the image * above 'top' to have base as its backing file. @@ -2416,15 +2416,9 @@ typedef struct BlkIntermediateStates { int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, BlockDriverState *base, const char *backing_file_str) { - BlockDriverState *intermediate; - BlockDriverState *base_bs = NULL; BlockDriverState *new_top_bs = NULL; - BlkIntermediateStates *intermediate_state, *next; int ret = -EIO; - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete; - QSIMPLEQ_INIT(&states_to_delete); - if (!top->drv || !base->drv) { goto exit; } @@ -2443,48 +2437,22 @@ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, goto exit; } - intermediate = 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 = g_new0(BlkIntermediateStates, 1); - intermediate_state->bs = intermediate; - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry); - - if (backing_bs(intermediate) == base) { - base_bs = backing_bs(intermediate); - break; - } - intermediate = backing_bs(intermediate); - } - if (base_bs == NULL) { - /* something went wrong, we did not end at the base. safely - * unravel everything, and exit with error */ + /* Make sure that base is in the backing chain of top */ + if (!bdrv_chain_contains(top, base)) { goto exit; } /* success - we can delete the intermediate states, and link top->base */ - backing_file_str = backing_file_str ? backing_file_str : base_bs->filename; + backing_file_str = backing_file_str ? backing_file_str : base->filename; ret = bdrv_change_backing_file(new_top_bs, backing_file_str, - base_bs->drv ? base_bs->drv->format_name : ""); + base->drv ? base->drv->format_name : ""); if (ret) { goto exit; } - bdrv_set_backing_hd(new_top_bs, base_bs); + bdrv_set_backing_hd(new_top_bs, base); - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { - /* so that bdrv_close() does not recursively close the chain */ - bdrv_set_backing_hd(intermediate_state->bs, NULL); - bdrv_unref(intermediate_state->bs); - } ret = 0; - exit: - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { - g_free(intermediate_state); - } return ret; } diff --git a/block/mirror.c b/block/mirror.c index 5d3b8bb..6247b27 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -370,10 +370,18 @@ static void mirror_exit(BlockJob *job, void *opaque) 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 from the top one */ - BlockDriverState *p = backing_bs(s->base); - bdrv_set_backing_hd(s->base, NULL); - bdrv_unref(p); + * 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); } } if (s->to_replace) { diff --git a/block/stream.c b/block/stream.c index ba535c5..3f64fa2 100644 --- a/block/stream.c +++ b/block/stream.c @@ -52,34 +52,6 @@ static int coroutine_fn stream_populate(BlockDriverState *bs, return bdrv_co_copy_on_readv(bs, sector_num, nb_sectors, &qiov); } -static void close_unused_images(BlockDriverState *top, BlockDriverState *base, - const char *base_id) -{ - BlockDriverState *intermediate; - intermediate = backing_bs(top); - - /* Must assign before bdrv_delete() to prevent traversing dangling pointer - * while we delete backing image instances. - */ - bdrv_set_backing_hd(top, base); - - while (intermediate) { - BlockDriverState *unused; - - /* reached base */ - if (intermediate == base) { - break; - } - - unused = intermediate; - intermediate = backing_bs(intermediate); - bdrv_set_backing_hd(unused, NULL); - bdrv_unref(unused); - } - - bdrv_refresh_limits(top, NULL); -} - typedef struct { int ret; bool reached_end; @@ -101,7 +73,7 @@ static void stream_complete(BlockJob *job, void *opaque) } } data->ret = bdrv_change_backing_file(job->bs, base_id, base_fmt); - close_unused_images(job->bs, base, base_id); + bdrv_set_backing_hd(job->bs, base); } g_free(s->backing_file_str); diff --git a/block/vvfat.c b/block/vvfat.c index 7c4b0f5..b41055a 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -2923,6 +2923,7 @@ static BlockDriver vvfat_write_target = { static int enable_write_target(BDRVVVFATState *s, Error **errp) { BlockDriver *bdrv_qcow = NULL; + BlockDriverState *backing; QemuOpts *opts = NULL; int ret; int size = sector2cluster(s, s->sector_count); @@ -2971,7 +2972,10 @@ static int enable_write_target(BDRVVVFATState *s, Error **errp) unlink(s->qcow_filename); #endif - bdrv_set_backing_hd(s->bs, bdrv_new()); + backing = bdrv_new(); + bdrv_set_backing_hd(s->bs, backing); + bdrv_unref(backing); + s->bs->backing->bs->drv = &vvfat_write_target; s->bs->backing->bs->opaque = g_new(void *, 1); *(void**)s->bs->backing->bs->opaque = s; diff --git a/include/block/block.h b/include/block/block.h index 20d5810..da94bdf 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -513,6 +513,7 @@ 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);