[Qemu-devel] [PATCH 0/2] introduce bdrv_swap, implement bdrv_append on top

[Qemu-devel] [PATCH 0/2] introduce bdrv_swap, implement bdrv_append on top

[Paolo Bonzini]

Yet another tiny bit extracted from block mirroring, looks like it
should be useful for block commit too.

Paolo Bonzini (2):
  block: copy over job and dirty bitmap fields in bdrv_append
  block: introduce bdrv_swap, implement bdrv_append on top of it

 block.c |  175 +++++++++++++++++++++++++++++++++++++--------------------------
 block.h |    1 +
 2 files changed, 103 insertions(+), 73 deletions(-)

-- 
1.7.10.2

[Qemu-devel] [PATCH 1/2] block: copy over job and dirty bitmap fields in bdrv_append

[Paolo Bonzini]

While these should not be in use at the time a transaction is started,
a command in the prepare phase of a transaction might have added them,
so they need to be brought over.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block.c b/block.c
index 0acdcac..702821d 100644
--- a/block.c
+++ b/block.c
@@ -1027,6 +1027,16 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     tmp.iostatus_enabled  = bs_top->iostatus_enabled;
     tmp.iostatus          = bs_top->iostatus;
 
+    /* dirty bitmap */
+    tmp.dirty_count       = bs_top->dirty_count;
+    tmp.dirty_bitmap      = bs_top->dirty_bitmap;
+    assert(bs_new->dirty_bitmap == NULL);
+
+    /* job */
+    tmp.in_use            = bs_top->in_use;
+    tmp.job               = bs_top->job;
+    assert(bs_new->job == NULL);
+
     /* keep the same entry in bdrv_states */
     pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
     tmp.list = bs_top->list;
@@ -1051,6 +1061,11 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
     /* clear the copied fields in the new backing file */
     bdrv_detach_dev(bs_new, bs_new->dev);
 
+    bs_new->job                = NULL;
+    bs_new->in_use             = 0;
+    bs_new->dirty_bitmap       = NULL;
+    bs_new->dirty_count        = 0;
+
     qemu_co_queue_init(&bs_new->throttled_reqs);
     memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
     memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
-- 
1.7.10.2

[Qemu-devel] [PATCH 2/2] block: introduce bdrv_swap, implement bdrv_append on top of it

[Paolo Bonzini]

The new function can be made a bit nicer than bdrv_append.  It swaps the
whole contents, and then swaps back (using the usual t=a;a=b;b=t idiom)
the fields that need to stay on top.  Thus, it does not need explicit
bdrv_detach_dev, bdrv_iostatus_disable, etc.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block.c |  184 ++++++++++++++++++++++++++++++++++-----------------------------
 block.h |    1 +
 2 files changed, 100 insertions(+), 85 deletions(-)

diff --git a/block.c b/block.c
index 702821d..0991ded 100644
--- a/block.c
+++ b/block.c
@@ -971,116 +971,130 @@ static void bdrv_rebind(BlockDriverState *bs)
     }
 }
 
-/*
- * Add new bs contents at the top of an image chain while the chain is
- * live, while keeping required fields on the top layer.
- *
- * This will modify the BlockDriverState fields, and swap contents
- * between bs_new and bs_top. Both bs_new and bs_top are modified.
- *
- * bs_new is required to be anonymous.
- *
- * This function does not create any image files.
- */
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
+static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
+                                     BlockDriverState *bs_src)
 {
-    BlockDriverState tmp;
-
-    /* bs_new must be anonymous */
-    assert(bs_new->device_name[0] == '\0');
-
-    tmp = *bs_new;
-
-    /* there are some fields that need to stay on the top layer: */
-    tmp.open_flags        = bs_top->open_flags;
+    /* move some fields that need to stay attached to the device */
+    bs_dest->open_flags         = bs_src->open_flags;
 
     /* dev info */
-    tmp.dev_ops           = bs_top->dev_ops;
-    tmp.dev_opaque        = bs_top->dev_opaque;
-    tmp.dev               = bs_top->dev;
-    tmp.buffer_alignment  = bs_top->buffer_alignment;
-    tmp.copy_on_read      = bs_top->copy_on_read;
+    bs_dest->dev_ops            = bs_src->dev_ops;
+    bs_dest->dev_opaque         = bs_src->dev_opaque;
+    bs_dest->dev                = bs_src->dev;
+    bs_dest->buffer_alignment   = bs_src->buffer_alignment;
+    bs_dest->copy_on_read       = bs_src->copy_on_read;
 
-    tmp.enable_write_cache = bs_top->enable_write_cache;
+    bs_dest->enable_write_cache = bs_src->enable_write_cache;
 
     /* i/o timing parameters */
-    tmp.slice_time        = bs_top->slice_time;
-    tmp.slice_start       = bs_top->slice_start;
-    tmp.slice_end         = bs_top->slice_end;
-    tmp.io_limits         = bs_top->io_limits;
-    tmp.io_base           = bs_top->io_base;
-    tmp.throttled_reqs    = bs_top->throttled_reqs;
-    tmp.block_timer       = bs_top->block_timer;
-    tmp.io_limits_enabled = bs_top->io_limits_enabled;
+    bs_dest->slice_time         = bs_src->slice_time;
+    bs_dest->slice_start        = bs_src->slice_start;
+    bs_dest->slice_end          = bs_src->slice_end;
+    bs_dest->io_limits          = bs_src->io_limits;
+    bs_dest->io_base            = bs_src->io_base;
+    bs_dest->throttled_reqs     = bs_src->throttled_reqs;
+    bs_dest->block_timer        = bs_src->block_timer;
+    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
 
     /* geometry */
-    tmp.cyls              = bs_top->cyls;
-    tmp.heads             = bs_top->heads;
-    tmp.secs              = bs_top->secs;
-    tmp.translation       = bs_top->translation;
+    bs_dest->cyls               = bs_src->cyls;
+    bs_dest->heads              = bs_src->heads;
+    bs_dest->secs               = bs_src->secs;
+    bs_dest->translation        = bs_src->translation;
 
     /* r/w error */
-    tmp.on_read_error     = bs_top->on_read_error;
-    tmp.on_write_error    = bs_top->on_write_error;
+    bs_dest->on_read_error      = bs_src->on_read_error;
+    bs_dest->on_write_error     = bs_src->on_write_error;
 
     /* i/o status */
-    tmp.iostatus_enabled  = bs_top->iostatus_enabled;
-    tmp.iostatus          = bs_top->iostatus;
+    bs_dest->iostatus_enabled   = bs_src->iostatus_enabled;
+    bs_dest->iostatus           = bs_src->iostatus;
 
     /* dirty bitmap */
-    tmp.dirty_count       = bs_top->dirty_count;
-    tmp.dirty_bitmap      = bs_top->dirty_bitmap;
-    assert(bs_new->dirty_bitmap == NULL);
+    bs_dest->dirty_count        = bs_src->dirty_count;
+    bs_dest->dirty_bitmap       = bs_src->dirty_bitmap;
 
     /* job */
-    tmp.in_use            = bs_top->in_use;
-    tmp.job               = bs_top->job;
-    assert(bs_new->job == NULL);
+    bs_dest->in_use             = bs_src->in_use;
+    bs_dest->job                = bs_src->job;
 
     /* keep the same entry in bdrv_states */
-    pstrcpy(tmp.device_name, sizeof(tmp.device_name), bs_top->device_name);
-    tmp.list = bs_top->list;
+    pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name),
+            bs_src->device_name);
+    bs_dest->list = bs_src->list;
+}
 
-    /* The contents of 'tmp' will become bs_top, as we are
-     * swapping bs_new and bs_top contents. */
-    tmp.backing_hd = bs_new;
-    pstrcpy(tmp.backing_file, sizeof(tmp.backing_file), bs_top->filename);
-    pstrcpy(tmp.backing_format, sizeof(tmp.backing_format),
-            bs_top->drv ? bs_top->drv->format_name : "");
-
-    /* swap contents of the fixed new bs and the current top */
-    *bs_new = *bs_top;
-    *bs_top = tmp;
-
-    /* device_name[] was carried over from the old bs_top.  bs_new
-     * shouldn't be in bdrv_states, so we need to make device_name[]
-     * reflect the anonymity of bs_new
-     */
-    bs_new->device_name[0] = '\0';
+/*
+ * Swap bs contents for two image chains while they are live,
+ * while keeping required fields on the BlockDriverState that is
+ * actually attached to a device.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_old. Both bs_new and bs_old are modified.
+ *
+ * bs_new is required to be anonymous.
+ *
+ * This function does not create any image files.
+ */
+void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old)
+{
+    BlockDriverState tmp;
 
-    /* clear the copied fields in the new backing file */
-    bdrv_detach_dev(bs_new, bs_new->dev);
+    /* bs_new must be anonymous and shouldn't have anything fancy enabled */
+    assert(bs_new->device_name[0] == '\0');
+    assert(bs_new->dirty_bitmap == NULL);
+    assert(bs_new->job == NULL);
+    assert(bs_new->dev == NULL);
+    assert(bs_new->in_use == 0);
+    assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->block_timer == NULL);
 
-    bs_new->job                = NULL;
-    bs_new->in_use             = 0;
-    bs_new->dirty_bitmap       = NULL;
-    bs_new->dirty_count        = 0;
+    tmp = *bs_new;
+    *bs_new = *bs_old;
+    *bs_old = tmp;
 
-    qemu_co_queue_init(&bs_new->throttled_reqs);
-    memset(&bs_new->io_base,   0, sizeof(bs_new->io_base));
-    memset(&bs_new->io_limits, 0, sizeof(bs_new->io_limits));
-    bdrv_iostatus_disable(bs_new);
+    /* there are some fields that should not be swapped, move them back */
+    bdrv_move_feature_fields(&tmp, bs_old);
+    bdrv_move_feature_fields(bs_old, bs_new);
+    bdrv_move_feature_fields(bs_new, &tmp);
 
-    /* we don't use bdrv_io_limits_disable() for this, because we don't want
-     * to affect or delete the block_timer, as it has been moved to bs_top */
-    bs_new->io_limits_enabled = false;
-    bs_new->block_timer       = NULL;
-    bs_new->slice_time        = 0;
-    bs_new->slice_start       = 0;
-    bs_new->slice_end         = 0;
+    /* bs_new shouldn't be in bdrv_states even after the swap!  */
+    assert(bs_new->device_name[0] == '\0');
+
+    /* Check a few fields that should remain attached to the device */
+    assert(bs_new->dev == NULL);
+    assert(bs_new->job == NULL);
+    assert(bs_new->in_use == 0);
+    assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->block_timer == NULL);
 
     bdrv_rebind(bs_new);
-    bdrv_rebind(bs_top);
+    bdrv_rebind(bs_old);
+}
+
+/*
+ * Add new bs contents at the top of an image chain while the chain is
+ * live, while keeping required fields on the top layer.
+ *
+ * This will modify the BlockDriverState fields, and swap contents
+ * between bs_new and bs_top. Both bs_new and bs_top are modified.
+ *
+ * bs_new is required to be anonymous.
+ *
+ * This function does not create any image files.
+ */
+void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
+{
+    bdrv_swap(bs_new, bs_top);
+
+    /* The contents of 'tmp' will become bs_top, as we are
+     * swapping bs_new and bs_top contents. */
+    bs_top->backing_hd = bs_new;
+    bs_top->open_flags &= ~BDRV_O_NO_BACKING;
+    pstrcpy(bs_top->backing_file, sizeof(bs_top->backing_file),
+            bs_new->filename);
+    pstrcpy(bs_top->backing_format, sizeof(bs_top->backing_format),
+            bs_new->drv ? bs_new->drv->format_name : "");
 }
 
 void bdrv_delete(BlockDriverState *bs)
diff --git a/block.h b/block.h
index 42e30d6..3af93c6 100644
--- a/block.h
+++ b/block.h
@@ -122,6 +122,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
+void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
-- 
1.7.10.2

Re: [Qemu-devel] [PATCH 0/2] introduce bdrv_swap, implement bdrv_append on top

[Kevin Wolf]

Am 14.06.2012 16:55, schrieb Paolo Bonzini:
> Yet another tiny bit extracted from block mirroring, looks like it
> should be useful for block commit too.
> 
> Paolo Bonzini (2):
>   block: copy over job and dirty bitmap fields in bdrv_append
>   block: introduce bdrv_swap, implement bdrv_append on top of it
> 
>  block.c |  175 +++++++++++++++++++++++++++++++++++++--------------------------
>  block.h |    1 +
>  2 files changed, 103 insertions(+), 73 deletions(-)
> 

I was really hoping we could get rid of bdrv_append() rather than extend
it and spread its use...

What exactly do we need this for? I'm sure you have good reasons, but
with such hackish approaches the justification should be explicit.

Kevin

Re: [Qemu-devel] [PATCH 0/2] introduce bdrv_swap, implement bdrv_append on top

[Paolo Bonzini]

Il 15/06/2012 11:38, Kevin Wolf ha scritto:
>> > Yet another tiny bit extracted from block mirroring, looks like it
>> > should be useful for block commit too.
>> > 
>> > Paolo Bonzini (2):
>> >   block: copy over job and dirty bitmap fields in bdrv_append
>> >   block: introduce bdrv_swap, implement bdrv_append on top of it
>> > 
>> >  block.c |  175 +++++++++++++++++++++++++++++++++++++--------------------------
>> >  block.h |    1 +
>> >  2 files changed, 103 insertions(+), 73 deletions(-)
>> > 
> I was really hoping we could get rid of bdrv_append() rather than extend
> it and spread its use...
> 
> What exactly do we need this for? I'm sure you have good reasons, but
> with such hackish approaches the justification should be explicit.

It's part of the replacement for drive_reopen.  It is used by a new
command called block-job-complete when switching the device from the
mirroring source to the target.  Unlike drive_reopen, it is safe (it
just reuses the target BDS without closing it) and asynchronous, so
overall an improvement.

Paolo

Re: [Qemu-devel] [PATCH 2/2] block: introduce bdrv_swap, implement bdrv_append on top of it

[Kevin Wolf]

Am 14.06.2012 16:55, schrieb Paolo Bonzini:
> +/*
> + * Add new bs contents at the top of an image chain while the chain is
> + * live, while keeping required fields on the top layer.
> + *
> + * This will modify the BlockDriverState fields, and swap contents
> + * between bs_new and bs_top. Both bs_new and bs_top are modified.
> + *
> + * bs_new is required to be anonymous.
> + *
> + * This function does not create any image files.
> + */
> +void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    bdrv_swap(bs_new, bs_top);
> +
> +    /* The contents of 'tmp' will become bs_top, as we are
> +     * swapping bs_new and bs_top contents. */

This comment looks outdated, there's not tmp in this function.

Kevin