| Message ID | 1381821901-1486-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 10/15/2013 01:25 AM, Fam Zheng wrote: > There is only one failure point: bdrv_change_backing_file in this > function, so we can drop the qlist and try to change the backing file > before deleting anything. > > This way bdrv_drop_intermediate is simplified while keeping the > operation transactional. A bonus is dropping an active BDS is supported > too by swapping the base and top. Although no caller uses this yet, the > comment is updated to reflect the change. > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > v2: check for active, top and base being in a backing chain. (Jeff) > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 103 ++++++++++++++++++++------------------------------------- > block/commit.c | 1 + > 2 files changed, 37 insertions(+), 67 deletions(-) > > > if (!top->drv || !base->drv) { > goto exit; > } So base->drv is non-NULL if we get here... > + if (active != top) { > + /* If there's an overlay, its backing_hd points to top's BDS now, > + * the top image is dropped but this BDS structure is kept and swapped > + * with base, this way we keep the pointers valid after dropping top */ > + overlay = bdrv_find_overlay(active, top); > + if (!overlay) { > + goto exit; > + } > + ret = bdrv_change_backing_file(overlay, base->filename, > + base->drv ? ...yet you are checking it for NULL here.
On Tue, Oct 15, 2013 at 03:25:00PM +0800, Fam Zheng wrote: > There is only one failure point: bdrv_change_backing_file in this > function, so we can drop the qlist and try to change the backing file > before deleting anything. > > This way bdrv_drop_intermediate is simplified while keeping the > operation transactional. A bonus is dropping an active BDS is supported > too by swapping the base and top. Although no caller uses this yet, the > comment is updated to reflect the change. > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > v2: check for active, top and base being in a backing chain. (Jeff) This does check for that, but it doesn't catch all errors. It will verify: [base] -> [active] And verifies: [top] -> [active] (when active is != top) However, it does not verify that the following is true: [base] -> [top] (e.g., it will pass on [top] -> [base] -> [active]) Rather than add another call to bdrv_find_overlay to verify the last case, would just adding the bdrv_swap() and a check for active == top to the existing function do what you need for the active layer support? > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 103 ++++++++++++++++++++------------------------------------- > block/commit.c | 1 + > 2 files changed, 37 insertions(+), 67 deletions(-) > > diff --git a/block.c b/block.c > index fd05a80..9ead554 100644 > --- a/block.c > +++ b/block.c > @@ -2130,18 +2130,11 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, > return overlay; > } > > -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. > - * > - * Requires that the overlay to 'top' is opened r/w, so that the backing file > - * information in 'bs' can be properly updated. > + * Drops images above 'base' up to and including 'top', and sets new 'base' > + * as backing_hd of top_overlay (the image orignally has 'top' as backing > + * file). top_overlay may be NULL if 'top' is active, no such update needed. > + * Requires that the top_overlay to 'top' is opened r/w. > * > * E.g., this will convert the following chain: > * bottom <- base <- intermediate <- top <- active > @@ -2158,86 +2151,62 @@ typedef struct BlkIntermediateStates { > * > * base <- active > * > - * Error conditions: > - * if active == top, that is considered an error > + * It also allows active==top, in which case it converts: > + * > + * base <- intermediate <- active (also top) > + * > + * to > + * > + * base == active == top, i.e. only base remains: *top == *base when return. > * > */ > int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, > BlockDriverState *base) > { > - BlockDriverState *intermediate; > + BlockDriverState *pbs; > + BlockDriverState *overlay = NULL; > 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); > + int ret = -EINVAL; > > if (!top->drv || !base->drv) { > goto exit; > } > > - new_top_bs = bdrv_find_overlay(active, top); > - > - if (new_top_bs == NULL) { > - /* we could not find the image above 'top', this is an error */ > - goto exit; > - } > - > - /* special case of new_top_bs->backing_hd already pointing to base - nothing > - * to do, no intermediate images */ > - if (new_top_bs->backing_hd == base) { > - ret = 0; > + if (!bdrv_find_overlay(active, base)) { > 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_malloc0(sizeof(BlkIntermediateStates)); > - intermediate_state->bs = intermediate; > - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry); > - > - if (intermediate->backing_hd == base) { > - base_bs = intermediate->backing_hd; > - break; > + if (active != top) { > + /* If there's an overlay, its backing_hd points to top's BDS now, > + * the top image is dropped but this BDS structure is kept and swapped > + * with base, this way we keep the pointers valid after dropping top */ > + overlay = bdrv_find_overlay(active, top); > + if (!overlay) { > + goto exit; > + } > + ret = bdrv_change_backing_file(overlay, base->filename, > + base->drv ? > + base->drv->format_name : ""); > + if (ret) { > + goto exit; > } > - intermediate = intermediate->backing_hd; > - } > - if (base_bs == NULL) { > - /* something went wrong, we did not end at the base. safely > - * unravel everything, and exit with error */ > - goto exit; > } > > - /* success - we can delete the intermediate states, and link top->base */ > - ret = bdrv_change_backing_file(new_top_bs, base_bs->filename, > - base_bs->drv ? base_bs->drv->format_name : ""); > - if (ret) { > - goto exit; > + for (pbs = top->backing_hd; pbs != base; pbs = base_bs) { > + assert(pbs); > + base_bs = pbs->backing_hd; > + pbs->backing_hd = NULL; > + bdrv_unref(pbs); > } > - new_top_bs->backing_hd = base_bs; > - > > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { > - /* so that bdrv_close() does not recursively close the chain */ > - intermediate_state->bs->backing_hd = NULL; > - bdrv_unref(intermediate_state->bs); > - } > - ret = 0; > + bdrv_swap(base, top); > > + base->backing_hd = NULL; > + bdrv_unref(base); > exit: > - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { > - g_free(intermediate_state); > - } > return ret; > } > > - > static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, > size_t size) > { > diff --git a/block/commit.c b/block/commit.c > index d4090cb..4d8cd05 100644 > --- a/block/commit.c > +++ b/block/commit.c > @@ -142,6 +142,7 @@ wait: > if (!block_job_is_cancelled(&s->common) && sector_num == end) { > /* success */ > ret = bdrv_drop_intermediate(active, top, base); > + base = top; > } > > exit_free_buf: > -- > 1.8.3.1 > >
diff --git a/block.c b/block.c index fd05a80..9ead554 100644 --- a/block.c +++ b/block.c @@ -2130,18 +2130,11 @@ BlockDriverState *bdrv_find_overlay(BlockDriverState *active, return overlay; } -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. - * - * Requires that the overlay to 'top' is opened r/w, so that the backing file - * information in 'bs' can be properly updated. + * Drops images above 'base' up to and including 'top', and sets new 'base' + * as backing_hd of top_overlay (the image orignally has 'top' as backing + * file). top_overlay may be NULL if 'top' is active, no such update needed. + * Requires that the top_overlay to 'top' is opened r/w. * * E.g., this will convert the following chain: * bottom <- base <- intermediate <- top <- active @@ -2158,86 +2151,62 @@ typedef struct BlkIntermediateStates { * * base <- active * - * Error conditions: - * if active == top, that is considered an error + * It also allows active==top, in which case it converts: + * + * base <- intermediate <- active (also top) + * + * to + * + * base == active == top, i.e. only base remains: *top == *base when return. * */ int bdrv_drop_intermediate(BlockDriverState *active, BlockDriverState *top, BlockDriverState *base) { - BlockDriverState *intermediate; + BlockDriverState *pbs; + BlockDriverState *overlay = NULL; 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); + int ret = -EINVAL; if (!top->drv || !base->drv) { goto exit; } - new_top_bs = bdrv_find_overlay(active, top); - - if (new_top_bs == NULL) { - /* we could not find the image above 'top', this is an error */ - goto exit; - } - - /* special case of new_top_bs->backing_hd already pointing to base - nothing - * to do, no intermediate images */ - if (new_top_bs->backing_hd == base) { - ret = 0; + if (!bdrv_find_overlay(active, base)) { 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_malloc0(sizeof(BlkIntermediateStates)); - intermediate_state->bs = intermediate; - QSIMPLEQ_INSERT_TAIL(&states_to_delete, intermediate_state, entry); - - if (intermediate->backing_hd == base) { - base_bs = intermediate->backing_hd; - break; + if (active != top) { + /* If there's an overlay, its backing_hd points to top's BDS now, + * the top image is dropped but this BDS structure is kept and swapped + * with base, this way we keep the pointers valid after dropping top */ + overlay = bdrv_find_overlay(active, top); + if (!overlay) { + goto exit; + } + ret = bdrv_change_backing_file(overlay, base->filename, + base->drv ? + base->drv->format_name : ""); + if (ret) { + goto exit; } - intermediate = intermediate->backing_hd; - } - if (base_bs == NULL) { - /* something went wrong, we did not end at the base. safely - * unravel everything, and exit with error */ - goto exit; } - /* success - we can delete the intermediate states, and link top->base */ - ret = bdrv_change_backing_file(new_top_bs, base_bs->filename, - base_bs->drv ? base_bs->drv->format_name : ""); - if (ret) { - goto exit; + for (pbs = top->backing_hd; pbs != base; pbs = base_bs) { + assert(pbs); + base_bs = pbs->backing_hd; + pbs->backing_hd = NULL; + bdrv_unref(pbs); } - new_top_bs->backing_hd = base_bs; - - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { - /* so that bdrv_close() does not recursively close the chain */ - intermediate_state->bs->backing_hd = NULL; - bdrv_unref(intermediate_state->bs); - } - ret = 0; + bdrv_swap(base, top); + base->backing_hd = NULL; + bdrv_unref(base); exit: - QSIMPLEQ_FOREACH_SAFE(intermediate_state, &states_to_delete, entry, next) { - g_free(intermediate_state); - } return ret; } - static int bdrv_check_byte_request(BlockDriverState *bs, int64_t offset, size_t size) { diff --git a/block/commit.c b/block/commit.c index d4090cb..4d8cd05 100644 --- a/block/commit.c +++ b/block/commit.c @@ -142,6 +142,7 @@ wait: if (!block_job_is_cancelled(&s->common) && sector_num == end) { /* success */ ret = bdrv_drop_intermediate(active, top, base); + base = top; } exit_free_buf: