Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

2013-10-24 Thread Jeff Cody 
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 f...@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 f...@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;
 -

Re: [Qemu-devel] [PATCH v2] block: support dropping active in bdrv_drop_intermediate

2013-10-18 Thread Eric Blake 
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 f...@redhat.com
 
 ---
 v2: check for active, top and base being in a backing chain. (Jeff)
 
 Signed-off-by: Fam Zheng f...@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.

-- 
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

2013-10-15 Thread Fam Zheng 
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 f...@redhat.com

---
v2: check for active, top and base being in a backing chain. (Jeff)

Signed-off-by: Fam Zheng f...@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;
-
 
-