| Message ID | 1443705214-9304-9-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 01.10.2015 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 <kwolf@redhat.com> > --- > 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(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On Thu 01 Oct 2015 03:13:26 PM CEST, Kevin Wolf wrote: > @@ -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); The purpose of this while() loop in the original code was to make a list of all the intermediate states whose references need to be dropped after the bdrv_set_backing_hd() call. Since now bdrv_set_backing_hd() is the one who manages the backing references, this list is no longer necessary, and you are actually leaking the BlkIntermediateStates objects here (see the rest of the patch below). The loop is also useful to check that 'base' is indeed part of the backing chain, so that we should keep. > @@ -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; > } Berto
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 <kwolf@redhat.com> > --- > 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 > >
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(). + */ 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);
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 <kwolf@redhat.com> --- 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(-)