Re: [Qemu-devel] [PATCH 7/8] qemu: command line option for dirty bitmaps

[John Snow]

On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote:
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
> 
> The patch adds the following command line option:
> 
> -dirty-bitmap [option1=val1][,option2=val2]...
>     Available options are:
>     name         The name for the bitmap (necessary).
> 
>     file         The file to load the bitmap from.
> 
>     file_id      When specified with 'file' option, then this file will
>                  be available through this id for other -dirty-bitmap
>                  options when specified without 'file' option, then it
>                  is a reference to 'file', specified with another
>                  -dirty-bitmap option, and it will be used to load the
>                  bitmap from.
> 
>     drive        The drive to bind the bitmap to. It should be specified
>                  as 'id' suboption of one of -drive options. If nor
>                  'file' neither 'file_id' are specified, then the bitmap
>                  will be loaded from that drive (internal dirty bitmap).
> 
>     granularity  The granularity for the bitmap. Not necessary, the
>                  default value may be used.
> 
>     enabled      on|off. Default is 'on'. Disabled bitmaps are not
>                  changing regardless of writes to corresponding drive.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  blockdev.c                |  38 ++++++++++++++++++
>  include/sysemu/blockdev.h |   1 +
>  include/sysemu/sysemu.h   |   1 +
>  qemu-options.hx           |  37 +++++++++++++++++
>  vl.c                      | 100 
> ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 177 insertions(+)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 5eaf77e..2a74395 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -176,6 +176,11 @@ QemuOpts *drive_def(const char *optstr)
>      return qemu_opts_parse(qemu_find_opts("drive"), optstr, 0);
>  }
>  
> +QemuOpts *dirty_bitmap_def(const char *optstr)
> +{
> +    return qemu_opts_parse(qemu_find_opts("dirty-bitmap"), optstr, 0);
> +}
> +
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr)
>  {
> @@ -3093,6 +3098,39 @@ BlockJobInfoList *qmp_query_block_jobs(Error **errp)
>      return head;
>  }
>  
> +QemuOptsList qemu_dirty_bitmap_opts = {
> +    .name = "dirty-bitmap",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_dirty_bitmap_opts.head),
> +    .desc = {
> +        {
> +            .name = "name",
> +            .type = QEMU_OPT_STRING,
> +            .help = "Name of the dirty bitmap",
> +        },{
> +            .name = "file",
> +            .type = QEMU_OPT_STRING,
> +            .help = "file name to load the bitmap from",
> +        },{
> +            .name = "file_id",
> +            .type = QEMU_OPT_STRING,
> +            .help = "node name to load the bitmap from (or to set id for"
> +                    " for file, opened by previous option)",
> +        },{
> +            .name = "drive",
> +            .type = QEMU_OPT_STRING,
> +            .help = "drive id to bind the bitmap to",
> +        },{
> +            .name = "granularity",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "granularity",
> +        },{
> +            .name = "enabled",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "enabled flag (default is 'on')",
> +        }
> +    }
> +};
> +
>  QemuOptsList qemu_common_drive_opts = {
>      .name = "drive",
>      .head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
> diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
> index 7ca59b5..5b101b8 100644
> --- a/include/sysemu/blockdev.h
> +++ b/include/sysemu/blockdev.h
> @@ -57,6 +57,7 @@ int drive_get_max_devs(BlockInterfaceType type);
>  DriveInfo *drive_get_next(BlockInterfaceType type);
>  
>  QemuOpts *drive_def(const char *optstr);
> +QemuOpts *dirty_bitmap_def(const char *optstr);
>  QemuOpts *drive_add(BlockInterfaceType type, int index, const char *file,
>                      const char *optstr);
>  DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_type);
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 8a52934..681a8f3 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -207,6 +207,7 @@ bool usb_enabled(void);
>  
>  extern QemuOptsList qemu_legacy_drive_opts;
>  extern QemuOptsList qemu_common_drive_opts;
> +extern QemuOptsList qemu_dirty_bitmap_opts;
>  extern QemuOptsList qemu_drive_opts;
>  extern QemuOptsList qemu_chardev_opts;
>  extern QemuOptsList qemu_device_opts;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index ec356f6..5e93122 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -614,6 +614,43 @@ qemu-system-i386 -hda a -hdb b
>  @end example
>  ETEXI
>  
> +DEF("dirty-bitmap", HAS_ARG, QEMU_OPTION_dirty_bitmap,
> +    "-dirty-bitmap name=name[,file=file][,file_id=file_id][,address@hidden"
> +    "              [,granularity=granularity][,enabled=on|off]\n",
> +    QEMU_ARCH_ALL)
> +STEXI
> address@hidden -dirty-bitmap @var{option}[,@var{option}[,@var{option}[,...]]]
> address@hidden -dirty-bitmap
> +
> +Define a dirty-bitmap. Valid options are:
> +
> address@hidden @option
> address@hidden address@hidden
> +The name of the bitmap. Should be unique per @var{file}/@var{drive} and per
> address@hidden
> address@hidden address@hidden
> +The separate qcow2 file for loading the bitmap @var{name} from it.
> address@hidden address@hidden
> +When specified with @var{file} option, then this @var{file} will be available
> +through this @var{file_id} for other @option{-dirty-bitmap} options.
> +When specified without @var{file} option, then it is a reference to 
> @var{file},
> +specified with another @option{-dirty-bitmap} option, and it will be used to
> +load the bitmap from.
> address@hidden address@hidden
> +The drive to bind the bitmap to. It should be specified as @var{id} suboption
> +of one of @option{-drive} options.
> +If nor @var{file} neither @var{file_id} are specified, then the bitmap will 
> be
> +loaded from that drive (internal dirty bitmap).
> address@hidden address@hidden
> +Granularity (in bytes) for created dirty bitmap. If the bitmap is already
> +exists in specified @var{file}/@var{file_id}/@var{device} it's granularity 
> will
> +not be changed but only checked (an error will be generated if this check
> +fails).
> address@hidden address@hidden
> +Enabled flag for the bitmap. By default the bitmap will be enabled.
> address@hidden table
> +ETEXI
> +
>  DEF("mtdblock", HAS_ARG, QEMU_OPTION_mtdblock,
>      "-mtdblock file  use 'file' as on-board Flash memory image\n",
>      QEMU_ARCH_ALL)
> diff --git a/vl.c b/vl.c
> index 83871f5..fb16d0c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1091,6 +1091,95 @@ static int cleanup_add_fd(QemuOpts *opts, void *opaque)
>  #define MTD_OPTS ""
>  #define SD_OPTS ""
>  
> +static int dirty_bitmap_func(QemuOpts *opts, void *opaque)
> +{
> +    Error *local_err = NULL;
> +    Error **errp = &local_err;
> +    BlockDriverState *file_bs = NULL, *for_bs = NULL;
> +    BdrvDirtyBitmap *bitmap = NULL;
> +
> +    const char *name = qemu_opt_get(opts, "name");
> +    const char *drive = qemu_opt_get(opts, "drive");
> +    const char *file = qemu_opt_get(opts, "file");
> +    const char *file_id = qemu_opt_get(opts, "file_id");
> +
> +    uint64_t granularity = qemu_opt_get_number(opts, "granularity", 0);
> +    bool enabled = qemu_opt_get_bool(opts, "enabled", true);
> +
> +    if (name == NULL) {
> +        error_setg(errp, "'name' option is necessary");
> +        goto fail;
> +    }
> +
> +    if (drive == NULL) {
> +        error_setg(errp, "'drive' option is necessary");
> +        goto fail;
> +    }
> +
> +    for_bs = bdrv_lookup_bs(drive, NULL, errp);

I think in this case we can support arbitrary nodes as well as full
drives, since we can create bitmaps for these nodes. If there's another
reason why we couldn't for our first pass at this feature, we should
document it and remember to come back and fix it.

To that end, maybe "node" is better than "drive" for the CLI here, to
match the QMP interface.

> +    if (for_bs == NULL) {
> +        goto fail;
> +    }
> +
> +    if (file != NULL) {
> +        QDict *options = NULL;
> +        if (file_id != NULL) {
> +            options = qdict_new();
> +            qdict_put(options, "node-name", qstring_from_str(file_id));
> +        }
> +
> +        bdrv_open(&file_bs, file, NULL, options, 0, NULL, errp);
> +        if (options) {
> +            QDECREF(options);
> +        }
> +        if (file_bs == NULL) {
> +            goto fail;
> +        }
> +    } else if (file_id != NULL) {
> +        file_bs = bdrv_find_node(file_id);
> +        if (file_bs == NULL) {
> +            error_setg(errp, "node '%s' is not found", drive);
> +            goto fail;
> +        }
> +    } else {
> +        file_bs = for_bs;
> +    }
> +
> +    if (granularity == 0) {
> +        granularity = bdrv_get_default_bitmap_granularity(for_bs);
> +    }
> +
> +    bitmap = bdrv_load_dirty_bitmap(for_bs, file_bs, granularity, name,
> +                                    errp);
> +    if (*errp != NULL) {
> +        goto fail;
> +    }
> +
> +    if (bitmap == NULL) {
> +        /* bitmap is not found in file_bs */
> +        bitmap = bdrv_create_dirty_bitmap(for_bs, granularity, name, errp);
> +        if (!bitmap) {
> +            goto fail;
> +        }
> +    }
> +

I don't think we should auto-create the persistent file unless we are
explicitly trying to do that. I'll elaborate a little more in a larger
summary email for the whole series by the end of tomorrow.

> +    bdrv_dirty_bitmap_set_file(bitmap, file_bs);
> +
> +    if (!enabled) {
> +        bdrv_disable_dirty_bitmap(bitmap);
> +    }
> +
> +    return 0;
> +
> +fail:
> +    error_report("-dirty-bitmap: %s", error_get_pretty(local_err));
> +    error_free(local_err);
> +    if (file_bs != NULL) {
> +        bdrv_close(file_bs);
> +    }
> +    return -1;
> +}
> +
>  static int drive_init_func(QemuOpts *opts, void *opaque)
>  {
>      BlockInterfaceType *block_default_type = opaque;
> @@ -2790,6 +2879,7 @@ int main(int argc, char **argv, char **envp)
>      module_call_init(MODULE_INIT_QOM);
>  
>      qemu_add_opts(&qemu_drive_opts);
> +    qemu_add_opts(&qemu_dirty_bitmap_opts);
>      qemu_add_drive_opts(&qemu_legacy_drive_opts);
>      qemu_add_drive_opts(&qemu_common_drive_opts);
>      qemu_add_drive_opts(&qemu_drive_opts);
> @@ -2918,6 +3008,11 @@ int main(int argc, char **argv, char **envp)
>                      exit(1);
>                  }
>                  break;
> +            case QEMU_OPTION_dirty_bitmap:
> +                if (dirty_bitmap_def(optarg) == NULL) {
> +                    exit(1);
> +                }
> +                break;
>              case QEMU_OPTION_set:
>                  if (qemu_set_option(optarg) != 0)
>                      exit(1);
> @@ -4198,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
>  
>      parse_numa_opts(machine_class);
>  
> +    if (qemu_opts_foreach(qemu_find_opts("dirty-bitmap"), dirty_bitmap_func,
> +                          NULL, 1) != 0) {
> +        exit(1);
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, 1) != 
> 0) {
>          exit(1);
>      }
> 

Seems okay overall, but I don't like the idea of needing to specify the
granularity to fetch a bitmap out of a file.

I suppose this parameter existed because of the overload with the desire
to CREATE a persistent bitmap at startup, where you would need to
specify a granularity, and perhaps that's fine.

I see that if you leave it un-set it defaults to 0 and we call the
default granularity fn() to get 64K, but we'll still fail to find the
bitmap if the granularity in file is not the default.

I'd like to see the granularity not play a role in the lookup at all.