diff mbox

[10/20] mirror: add buf-size argument to drive-mirror

Message ID 1355319999-30627-11-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Dec. 12, 2012, 1:46 p.m. UTC
This makes sense when the next commit starts using the extra buffer space
to perform many I/O operations asynchronously.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 block/mirror.c         |  6 +++---
 block_int.h            |  5 +++--
 blockdev.c             |  9 ++++++++-
 hmp.c                  |  2 +-
 qapi-schema.json       |  5 ++++-
 qmp-commands.hx        |  4 +++-
 tests/qemu-iotests/041 | 31 +++++++++++++++++++++++++++++++
 7 files changed, 53 insertions(+), 9 deletions(-)

Comments

Eric Blake Dec. 14, 2012, 10:22 p.m. UTC | #1
On 12/12/2012 06:46 AM, Paolo Bonzini wrote:
> This makes sense when the next commit starts using the extra buffer space
> to perform many I/O operations asynchronously.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/mirror.c         |  6 +++---
>  block_int.h            |  5 +++--
>  blockdev.c             |  9 ++++++++-
>  hmp.c                  |  2 +-
>  qapi-schema.json       |  5 ++++-
>  qmp-commands.hx        |  4 +++-
>  tests/qemu-iotests/041 | 31 +++++++++++++++++++++++++++++++
>  7 files changed, 53 insertions(+), 9 deletions(-)
> 

> @@ -447,7 +447,7 @@ void mirror_start(BlockDriverState *bs, BlockDriverState *target,
>      s->target = target;
>      s->mode = mode;
>      s->granularity = granularity;
> -    s->buf_size = granularity;
> +    s->buf_size = MAX(buf_size, granularity);

So you silently clamp the buffer size if the user gives something larger
than granularity.

> +++ b/qapi-schema.json
> @@ -1641,6 +1641,9 @@
>  #               are smaller than that, else the cluster size.  Must be a
>  #               power of 2 between 512 and 64M.
>  #
> +# @buf-size: #optional maximum amount of data in flight from source to
> +#            target.

Mention that it was added in 1.4.  Also, is it worth mentioning
reasonable bounds (such as granularity), and whether it must be a power
of two?

>  - "granularity": granularity of the dirty bitmap (json-int, optional)
> +- "buf_size": maximum amount of data in flight from source to target
> +  (json-int, default 10M)

Oh, so unlike granularity, this one does NOT have to be a power of 2.
But why is the default 10M if you clamp it to granularity, which
defaults to 64k?  (and again, something else I have to figure out how to
expose from libvirt)
Paolo Bonzini Dec. 15, 2012, 9:09 a.m. UTC | #2
> On 12/12/2012 06:46 AM, Paolo Bonzini wrote:
> > This makes sense when the next commit starts using the extra buffer
> > space
> > to perform many I/O operations asynchronously.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  block/mirror.c         |  6 +++---
> >  block_int.h            |  5 +++--
> >  blockdev.c             |  9 ++++++++-
> >  hmp.c                  |  2 +-
> >  qapi-schema.json       |  5 ++++-
> >  qmp-commands.hx        |  4 +++-
> >  tests/qemu-iotests/041 | 31 +++++++++++++++++++++++++++++++
> >  7 files changed, 53 insertions(+), 9 deletions(-)
> > 
> 
> > @@ -447,7 +447,7 @@ void mirror_start(BlockDriverState *bs,
> > BlockDriverState *target,
> >      s->target = target;
> >      s->mode = mode;
> >      s->granularity = granularity;
> > -    s->buf_size = granularity;
> > +    s->buf_size = MAX(buf_size, granularity);
> 
> So you silently clamp the buffer size if the user gives something
> larger than granularity.

I silently make it large if it gives something *smaller*.  The job
always has to copy one granularity-sized chunk.

> > +++ b/qapi-schema.json
> > @@ -1641,6 +1641,9 @@
> >  #               are smaller than that, else the cluster size.
> >   Must be a
> >  #               power of 2 between 512 and 64M.
> >  #
> > +# @buf-size: #optional maximum amount of data in flight from
> > source to
> > +#            target.
> 
> Mention that it was added in 1.4.  Also, is it worth mentioning
> reasonable bounds (such as granularity), and whether it must be a
> power of two?

It is arbitrary, though making it too big makes little sense.

> >  - "granularity": granularity of the dirty bitmap (json-int,
> >  optional)
> > +- "buf_size": maximum amount of data in flight from source to
> > target
> > +  (json-int, default 10M)
> 
> Oh, so unlike granularity, this one does NOT have to be a power of 2.
> But why is the default 10M if you clamp it to granularity, which
> defaults to 64k?

I don't clamp it, I make it _at least_ as big as the granularity.

Paolo
Stefan Hajnoczi Jan. 14, 2013, 11:41 a.m. UTC | #3
On Wed, Dec 12, 2012 at 02:46:29PM +0100, Paolo Bonzini wrote:
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d8faad0..97c52c9 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -939,7 +939,7 @@ EQMP
>          .name       = "drive-mirror",
>          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>                        "on-source-error:s?,on-target-error:s?,"
> -                      "granularity:i?",
> +                      "buf-size:i?,granularity:i?",

granularity was also added in this patch series, so this doesn't matter,
but in general it's nicer to append optional arguments on the end.  This
way, command option ordering doesn't change arbitrarily - that could
confuse users.

Stefan
diff mbox

Patch

diff --git a/block/mirror.c b/block/mirror.c
index f02ffb3..ed56b86 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -408,8 +408,8 @@  static BlockJobType mirror_job_type = {
 };
 
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed, int64_t granularity, MirrorSyncMode mode,
-                  BlockdevOnError on_source_error,
+                  int64_t speed, int64_t granularity, int64_t buf_size,
+                  MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp)
@@ -447,7 +447,7 @@  void mirror_start(BlockDriverState *bs, BlockDriverState *target,
     s->target = target;
     s->mode = mode;
     s->granularity = granularity;
-    s->buf_size = granularity;
+    s->buf_size = MAX(buf_size, granularity);
 
     bdrv_set_dirty_tracking(bs, granularity);
     bdrv_set_enable_write_cache(s->target, true);
diff --git a/block_int.h b/block_int.h
index 8d038c8..8f54b41 100644
--- a/block_int.h
+++ b/block_int.h
@@ -339,6 +339,7 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @target: Block device to write to.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
  * @granularity: The chosen granularity for the dirty bitmap.
+ * @buf_size: The amount of data that can be in flight at one time.
  * @mode: Whether to collapse all images in the chain to the target.
  * @on_source_error: The action to take upon error reading from the source.
  * @on_target_error: The action to take upon error writing to the target.
@@ -352,8 +353,8 @@  void commit_start(BlockDriverState *bs, BlockDriverState *base,
  * @bs will be switched to read from @target.
  */
 void mirror_start(BlockDriverState *bs, BlockDriverState *target,
-                  int64_t speed, int64_t granularity, MirrorSyncMode mode,
-                  BlockdevOnError on_source_error,
+                  int64_t speed, int64_t granularity, int64_t buf_size,
+                  MirrorSyncMode mode, BlockdevOnError on_source_error,
                   BlockdevOnError on_target_error,
                   BlockDriverCompletionFunc *cb,
                   void *opaque, Error **errp);
diff --git a/blockdev.c b/blockdev.c
index d554c03..dff6c3d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1187,12 +1187,15 @@  void qmp_block_commit(const char *device,
     drive_get_ref(drive_get_by_blockdev(bs));
 }
 
+#define DEFAULT_MIRROR_BUF_SIZE   (10 << 20)
+
 void qmp_drive_mirror(const char *device, const char *target,
                       bool has_format, const char *format,
                       enum MirrorSyncMode sync,
                       bool has_mode, enum NewImageMode mode,
                       bool has_speed, int64_t speed,
                       bool has_granularity, uint32_t granularity,
+                      bool has_buf_size, int64_t buf_size,
                       bool has_on_source_error, BlockdevOnError on_source_error,
                       bool has_on_target_error, BlockdevOnError on_target_error,
                       Error **errp)
@@ -1221,6 +1224,10 @@  void qmp_drive_mirror(const char *device, const char *target,
     if (!has_granularity) {
         granularity = 0;
     }
+    if (!has_buf_size) {
+        buf_size = DEFAULT_MIRROR_BUF_SIZE;
+    }
+
     if (granularity != 0 && (granularity < 512 || granularity > 1048576 * 64)) {
         error_set(errp, QERR_INVALID_PARAMETER, device);
         return;
@@ -1310,7 +1317,7 @@  void qmp_drive_mirror(const char *device, const char *target,
         return;
     }
 
-    mirror_start(bs, target_bs, speed, granularity, sync,
+    mirror_start(bs, target_bs, speed, granularity, buf_size, sync,
                  on_source_error, on_target_error,
                  block_job_cb, bs, &local_err);
     if (local_err != NULL) {
diff --git a/hmp.c b/hmp.c
index e6d2930..428c563 100644
--- a/hmp.c
+++ b/hmp.c
@@ -795,7 +795,7 @@  void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
 
     qmp_drive_mirror(device, filename, !!format, format,
                      full ? MIRROR_SYNC_MODE_FULL : MIRROR_SYNC_MODE_TOP,
-                     true, mode, false, 0, false, 0,
+                     true, mode, false, 0, false, 0, false, 0,
                      false, 0, false, 0, &errp);
     hmp_handle_error(mon, &errp);
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index 465984e..fb38106 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1641,6 +1641,9 @@ 
 #               are smaller than that, else the cluster size.  Must be a
 #               power of 2 between 512 and 64M.
 #
+# @buf-size: #optional maximum amount of data in flight from source to
+#            target.
+#
 # @on-source-error: #optional the action to take on an error on the source,
 #                   default 'report'.  'stop' and 'enospc' can only be used
 #                   if the block device supports io-status (see BlockInfo).
@@ -1658,7 +1661,7 @@ 
   'data': { 'device': 'str', 'target': 'str', '*format': 'str',
             'sync': 'MirrorSyncMode', '*mode': 'NewImageMode',
             '*speed': 'int', '*granularity': 'uint32',
-            '*on-source-error': 'BlockdevOnError',
+            '*buf-size': 'int', '*on-source-error': 'BlockdevOnError',
             '*on-target-error': 'BlockdevOnError' } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d8faad0..97c52c9 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -939,7 +939,7 @@  EQMP
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"
-                      "granularity:i?",
+                      "buf-size:i?,granularity:i?",
         .mhandler.cmd_new = qmp_marshal_input_drive_mirror,
     },
 
@@ -964,6 +964,8 @@  Arguments:
 - "speed": maximum speed of the streaming job, in bytes per second
   (json-int)
 - "granularity": granularity of the dirty bitmap (json-int, optional)
+- "buf_size": maximum amount of data in flight from source to target
+  (json-int, default 10M)
 - "sync": what parts of the disk image should be copied to the destination;
   possibilities include "full" for all the disk, "top" for only the sectors
   allocated in the topmost image, or "none" to only replicate new I/O
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index d0e9a7f..05ff0f8 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -207,6 +207,37 @@  class TestSingleDrive(ImageMirroringTestCase):
         self.assertTrue(self.compare_images(test_img, target_img),
                         'target image does not match source after mirroring')
 
+    def test_small_buffer(self):
+        self.assert_no_active_mirrors()
+
+        # A small buffer is rounded up automatically
+        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
+                             buf_size=4096, target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/inserted/file', target_img)
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
+    def test_small_buffer2(self):
+        self.assert_no_active_mirrors()
+
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d,size=%d'
+                        % (TestSingleDrive.image_len, TestSingleDrive.image_len), target_img)
+        result = self.vm.qmp('drive-mirror', device='drive0', sync='full',
+                             buf_size=65536, mode='existing', target=target_img)
+        self.assert_qmp(result, 'return', {})
+
+        self.complete_and_wait()
+        result = self.vm.qmp('query-block')
+        self.assert_qmp(result, 'return[0]/inserted/file', target_img)
+        self.vm.shutdown()
+        self.assertTrue(self.compare_images(test_img, target_img),
+                        'target image does not match source after mirroring')
+
     def test_large_cluster(self):
         self.assert_no_active_mirrors()