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

[Kevin Wolf <kwolf@redhat.com>]

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