[Qemu-devel] [PATCH v2 1/1] nbd: add zero-init parameter

[Qemu-devel] [PATCH v2 1/1] nbd: add zero-init parameter

[Denis V. Lunev]

When using a nbd block device, the info about necessity of prior disk
zeroing could significantly improve the speed of certain operations
(e.g. backups).

This patch also will allow to preserve QCOW2 images during migration.
Management software now may specify zero-init option and thus abscent
areas in the original QCOW2 image will not be marked as zeroes in the
target image. This is tight distiction but it is here.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
---
 block/nbd.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c
index 6bc06d6..eed06d1 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -45,6 +45,7 @@ typedef struct BDRVNBDState {
 
     /* For nbd_refresh_filename() */
     char *path, *host, *port, *export, *tlscredsid;
+    bool zero_init;
 } BDRVNBDState;
 
 static int nbd_parse_uri(const char *filename, QDict *options)
@@ -194,6 +195,7 @@ out:
 static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
 {
     SocketAddress *saddr;
+    const char *zero_init;
 
     s->path = g_strdup(qemu_opt_get(opts, "path"));
     s->host = g_strdup(qemu_opt_get(opts, "host"));
@@ -232,6 +234,11 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
 
     s->export = g_strdup(qemu_opt_get(opts, "export"));
 
+    zero_init = qemu_opt_get(opts, "zero-init");
+    if (zero_init != NULL) {
+        s->zero_init = strcmp(zero_init, "on") == 0;
+    }
+
     return saddr;
 }
 
@@ -322,6 +329,11 @@ static QemuOptsList nbd_runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "ID of the TLS credentials to use",
         },
+        {
+            .name = "zero-init",
+            .type = QEMU_OPT_BOOL,
+            .help = "Zero-initialized image flag",
+        },
     },
 };
 
@@ -483,6 +495,12 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     bs->full_open_options = opts;
 }
 
+static int nbd_has_zero_init(BlockDriverState *bs)
+{
+    BDRVNBDState *s = bs->opaque;
+    return s->zero_init;
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -499,6 +517,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_has_zero_init,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -517,6 +536,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_has_zero_init,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -535,6 +555,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_has_zero_init,
 };
 
 static void bdrv_nbd_init(void)
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 1/1] nbd: add zero-init parameter

[Kevin Wolf]

Am 05.10.2016 um 12:57 hat Denis V. Lunev geschrieben:
> When using a nbd block device, the info about necessity of prior disk
> zeroing could significantly improve the speed of certain operations
> (e.g. backups).
> 
> This patch also will allow to preserve QCOW2 images during migration.
> Management software now may specify zero-init option and thus abscent
> areas in the original QCOW2 image will not be marked as zeroes in the
> target image. This is tight distiction but it is here.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 6bc06d6..eed06d1 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -45,6 +45,7 @@ typedef struct BDRVNBDState {
>  
>      /* For nbd_refresh_filename() */
>      char *path, *host, *port, *export, *tlscredsid;
> +    bool zero_init;
>  } BDRVNBDState;
>  
>  static int nbd_parse_uri(const char *filename, QDict *options)
> @@ -194,6 +195,7 @@ out:
>  static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
>  {
>      SocketAddress *saddr;
> +    const char *zero_init;
>  
>      s->path = g_strdup(qemu_opt_get(opts, "path"));
>      s->host = g_strdup(qemu_opt_get(opts, "host"));
> @@ -232,6 +234,11 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
>  
>      s->export = g_strdup(qemu_opt_get(opts, "export"));
>  
> +    zero_init = qemu_opt_get(opts, "zero-init");
> +    if (zero_init != NULL) {
> +        s->zero_init = strcmp(zero_init, "on") == 0;
> +    }

We don't need a local char* zero_init and parse it manually:

    s->zero_init = qemu_opt_get_bool(opts, "zero-init", false);

Much easier.

> +
>      return saddr;
>  }
>  
> @@ -322,6 +329,11 @@ static QemuOptsList nbd_runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "ID of the TLS credentials to use",
>          },
> +        {
> +            .name = "zero-init",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "Zero-initialized image flag",

We can be a bit more descriptive here:

    "If enabled, assume the image to be completely zeroed on startup"

(Or maybe you can think of something better than that.)

> +        },
>      },
>  };
>  
> @@ -483,6 +495,12 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>      bs->full_open_options = opts;
>  }
>  
> +static int nbd_has_zero_init(BlockDriverState *bs)
> +{
> +    BDRVNBDState *s = bs->opaque;
> +    return s->zero_init;
> +}
> +
>  static BlockDriver bdrv_nbd = {
>      .format_name                = "nbd",
>      .protocol_name              = "nbd",
> @@ -499,6 +517,7 @@ static BlockDriver bdrv_nbd = {
>      .bdrv_detach_aio_context    = nbd_detach_aio_context,
>      .bdrv_attach_aio_context    = nbd_attach_aio_context,
>      .bdrv_refresh_filename      = nbd_refresh_filename,
> +    .bdrv_has_zero_init         = nbd_has_zero_init,
>  };
>  
>  static BlockDriver bdrv_nbd_tcp = {
> @@ -517,6 +536,7 @@ static BlockDriver bdrv_nbd_tcp = {
>      .bdrv_detach_aio_context    = nbd_detach_aio_context,
>      .bdrv_attach_aio_context    = nbd_attach_aio_context,
>      .bdrv_refresh_filename      = nbd_refresh_filename,
> +    .bdrv_has_zero_init         = nbd_has_zero_init,
>  };
>  
>  static BlockDriver bdrv_nbd_unix = {
> @@ -535,6 +555,7 @@ static BlockDriver bdrv_nbd_unix = {
>      .bdrv_detach_aio_context    = nbd_detach_aio_context,
>      .bdrv_attach_aio_context    = nbd_attach_aio_context,
>      .bdrv_refresh_filename      = nbd_refresh_filename,
> +    .bdrv_has_zero_init         = nbd_has_zero_init,
>  };

I believe nbd_refresh_filename() needs an update, too.

Kevin

Re: [Qemu-devel] [PATCH v2 1/1] nbd: add zero-init parameter

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1662 bytes --]

On 10/05/2016 05:57 AM, Denis V. Lunev wrote:
> When using a nbd block device, the info about necessity of prior disk
> zeroing could significantly improve the speed of certain operations
> (e.g. backups).
> 
> This patch also will allow to preserve QCOW2 images during migration.

'allow to' is not idiomatic English; and your placement of 'also' before
'will' is awkward. You want either of:

This patch will also allow preservation of QCOW2 images...
This patch will also preserve QCOW2 images...

> Management software now may specify zero-init option and thus abscent

s/now may/may now/
s/abscent/absent/

> areas in the original QCOW2 image will not be marked as zeroes in the
> target image. This is tight distiction but it is here.

s/distiction/distinction/

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> CC: Max Reitz <mreitz@redhat.com>
> ---
>  block/nbd.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 

> @@ -232,6 +234,11 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QemuOpts *opts, Error **errp)
>  
>      s->export = g_strdup(qemu_opt_get(opts, "export"));
>  
> +    zero_init = qemu_opt_get(opts, "zero-init");
> +    if (zero_init != NULL) {
> +        s->zero_init = strcmp(zero_init, "on") == 0;

As Kevin pointed out, a manual parse makes the command line less
consistent; reusing the common parser also means that you will support
things like zero-init=true for free.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]