[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 08/16] block: Manage backing fil
From: |
Fam Zheng |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v2 08/16] block: Manage backing file references in bdrv_set_backing_hd() |
Date: |
Fri, 9 Oct 2015 09:49:37 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Thu, 10/01 15:13, Kevin Wolf wrote:
> 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 <address@hidden>
> ---
> block.c | 35 ++++++++++++++---------------------
> block/mirror.c | 16 ++++++++++++----
> block/stream.c | 30 +-----------------------------
> block/vvfat.c | 6 +++++-
> include/block/block.h | 1 +
> 5 files changed, 33 insertions(+), 55 deletions(-)
>
> diff --git a/block.c b/block.c
> index 20a3c5c..c4dcf81 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().
> + */
Should we document the reference to bs->backing->bs as well?
> 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);
> @@ -2428,12 +2434,9 @@ int bdrv_drop_intermediate(BlockDriverState *active,
> BlockDriverState *top,
> BlockDriverState *intermediate;
> BlockDriverState *base_bs = NULL;
> BlockDriverState *new_top_bs = NULL;
> - BlkIntermediateStates *intermediate_state, *next;
> + BlkIntermediateStates *intermediate_state;
> int ret = -EIO;
>
> - QSIMPLEQ_HEAD(states_to_delete, BlkIntermediateStates) states_to_delete;
> - QSIMPLEQ_INIT(&states_to_delete);
> -
> if (!top->drv || !base->drv) {
> goto exit;
> }
> @@ -2460,7 +2463,6 @@ int bdrv_drop_intermediate(BlockDriverState *active,
> BlockDriverState *top,
> 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);
> @@ -2483,17 +2485,8 @@ int bdrv_drop_intermediate(BlockDriverState *active,
> BlockDriverState *top,
> }
> bdrv_set_backing_hd(new_top_bs, base_bs);
>
> - 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 5bb3e36..911c432 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 c6854a6..5d9092c 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);
> --
> 1.8.3.1
>
>
- Re: [Qemu-block] [PATCH v2 03/16] blkverify: Convert s->test_file to BdrvChild, (continued)
- [Qemu-block] [PATCH v2 09/16] block: Split bdrv_move_feature_fields(), Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 07/16] block: Convert bs->backing_hd to BdrvChild, Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 08/16] block: Manage backing file references in bdrv_set_backing_hd(), Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 10/16] block/io: Make bdrv_requests_pending() public, Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 12/16] block: Introduce parents list, Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 13/16] block: Implement bdrv_append() without bdrv_swap(), Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 02/16] vmdk: Use BdrvChild instead of BDS for references to extents, Kevin Wolf, 2015/10/08
- [Qemu-block] [PATCH v2 01/16] block: Introduce BDS.file_child, Kevin Wolf, 2015/10/08
- Re: [Qemu-block] [Qemu-devel] [PATCH v2 00/16] block: Get rid of bdrv_swap(), Fam Zheng, 2015/10/08
- Re: [Qemu-block] [PATCH v2 00/16] block: Get rid of bdrv_swap(), Stefan Hajnoczi, 2015/10/09