From 5db15a57697063b9062a015dbc6d5d38211f2df1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 14 Sep 2015 15:33:33 +0200 Subject: block: Manage backing file references in bdrv_set_backing_hd() 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); -- cgit v1.1