Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate
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
>
> ---
> 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
> ---
> 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
Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate
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
>
> ---
> v2: check for active, top and base being in a backing chain. (Jeff)
>
> Signed-off-by: Fam Zheng
> ---
> 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.
--
Eric Blake eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate
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
---
v2: check for active, top and base being in a backing chain. (Jeff)
Signed-off-by: Fam Zheng
---
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;
-
-QSIM
