From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33432) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z39XZ-0006xB-Ao for qemu-devel@nongnu.org; Thu, 11 Jun 2015 16:57:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z39XW-0000iS-0B for qemu-devel@nongnu.org; Thu, 11 Jun 2015 16:57:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45510) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z39XV-0000ea-ML for qemu-devel@nongnu.org; Thu, 11 Jun 2015 16:57:29 -0400 Message-ID: <5579F636.4070501@redhat.com> Date: Thu, 11 Jun 2015 16:57:26 -0400 From: John Snow MIME-Version: 1.0 References: <1433776886-27239-1-git-send-email-vsementsov@virtuozzo.com> <1433776886-27239-8-git-send-email-vsementsov@virtuozzo.com> In-Reply-To: <1433776886-27239-8-git-send-email-vsementsov@virtuozzo.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 7/8] qemu: command line option for dirty bitmaps List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladimir Sementsov-Ogievskiy , qemu-devel@nongnu.org Cc: kwolf@redhat.com, pbonzini@redhat.com, Vladimir Sementsov-Ogievskiy , stefanha@redhat.com, den@openvz.org On 06/08/2015 11:21 AM, Vladimir Sementsov-Ogievskiy wrote: > From: Vladimir Sementsov-Ogievskiy >=20 > The patch adds the following command line option: >=20 > -dirty-bitmap [option1=3Dval1][,option2=3Dval2]... > Available options are: > name The name for the bitmap (necessary). >=20 > file The file to load the bitmap from. >=20 > 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. >=20 > drive The drive to bind the bitmap to. It should be specifie= d > as 'id' suboption of one of -drive options. If nor > 'file' neither 'file_id' are specified, then the bitma= p > will be loaded from that drive (internal dirty bitmap)= . >=20 > granularity The granularity for the bitmap. Not necessary, the > default value may be used. >=20 > enabled on|off. Default is 'on'. Disabled bitmaps are not > changing regardless of writes to corresponding drive. >=20 > Signed-off-by: Vladimir Sementsov-Ogievskiy > --- > 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(+) >=20 > 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); > } > =20 > +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 *fi= le, > const char *optstr) > { > @@ -3093,6 +3098,39 @@ BlockJobInfoList *qmp_query_block_jobs(Error **e= rrp) > return head; > } > =20 > +QemuOptsList qemu_dirty_bitmap_opts =3D { > + .name =3D "dirty-bitmap", > + .head =3D QTAILQ_HEAD_INITIALIZER(qemu_dirty_bitmap_opts.head), > + .desc =3D { > + { > + .name =3D "name", > + .type =3D QEMU_OPT_STRING, > + .help =3D "Name of the dirty bitmap", > + },{ > + .name =3D "file", > + .type =3D QEMU_OPT_STRING, > + .help =3D "file name to load the bitmap from", > + },{ > + .name =3D "file_id", > + .type =3D QEMU_OPT_STRING, > + .help =3D "node name to load the bitmap from (or to set id= for" > + " for file, opened by previous option)", > + },{ > + .name =3D "drive", > + .type =3D QEMU_OPT_STRING, > + .help =3D "drive id to bind the bitmap to", > + },{ > + .name =3D "granularity", > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "granularity", > + },{ > + .name =3D "enabled", > + .type =3D QEMU_OPT_BOOL, > + .help =3D "enabled flag (default is 'on')", > + } > + } > +}; > + > QemuOptsList qemu_common_drive_opts =3D { > .name =3D "drive", > .head =3D 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); > =20 > QemuOpts *drive_def(const char *optstr); > +QemuOpts *dirty_bitmap_def(const char *optstr); > QemuOpts *drive_add(BlockInterfaceType type, int index, const char *fi= le, > const char *optstr); > DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType block_default_t= ype); > 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); > =20 > 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 > =20 > +DEF("dirty-bitmap", HAS_ARG, QEMU_OPTION_dirty_bitmap, > + "-dirty-bitmap name=3Dname[,file=3Dfile][,file_id=3Dfile_id][,driv= e=3D@var{id}]\n" > + " [,granularity=3Dgranularity][,enabled=3Don|off]\n", > + QEMU_ARCH_ALL) > +STEXI > +@item -dirty-bitmap @var{option}[,@var{option}[,@var{option}[,...]]] > +@findex -dirty-bitmap > + > +Define a dirty-bitmap. Valid options are: > + > +@table @option > +@item name=3D@var{name} > +The name of the bitmap. Should be unique per @var{file}/@var{drive} an= d per > +@var{for_drive}. > +@item file=3D@var{file} > +The separate qcow2 file for loading the bitmap @var{name} from it. > +@item file_id=3D@var{file_id} > +When specified with @var{file} option, then this @var{file} will be av= ailable > +through this @var{file_id} for other @option{-dirty-bitmap} options. > +When specified without @var{file} option, then it is a reference to @v= ar{file}, > +specified with another @option{-dirty-bitmap} option, and it will be u= sed to > +load the bitmap from. > +@item drive=3D@var{drive} > +The drive to bind the bitmap to. It should be specified as @var{id} su= boption > +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). > +@item granularity=3D@var{granularity} > +Granularity (in bytes) for created dirty bitmap. If the bitmap is alre= ady > +exists in specified @var{file}/@var{file_id}/@var{device} it's granula= rity will > +not be changed but only checked (an error will be generated if this ch= eck > +fails). > +@item enabled=3D@var{enabled} > +Enabled flag for the bitmap. By default the bitmap will be enabled. > +@end 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 "" > =20 > +static int dirty_bitmap_func(QemuOpts *opts, void *opaque) > +{ > + Error *local_err =3D NULL; > + Error **errp =3D &local_err; > + BlockDriverState *file_bs =3D NULL, *for_bs =3D NULL; > + BdrvDirtyBitmap *bitmap =3D NULL; > + > + const char *name =3D qemu_opt_get(opts, "name"); > + const char *drive =3D qemu_opt_get(opts, "drive"); > + const char *file =3D qemu_opt_get(opts, "file"); > + const char *file_id =3D qemu_opt_get(opts, "file_id"); > + > + uint64_t granularity =3D qemu_opt_get_number(opts, "granularity", = 0); > + bool enabled =3D qemu_opt_get_bool(opts, "enabled", true); > + > + if (name =3D=3D NULL) { > + error_setg(errp, "'name' option is necessary"); > + goto fail; > + } > + > + if (drive =3D=3D NULL) { > + error_setg(errp, "'drive' option is necessary"); > + goto fail; > + } > + > + for_bs =3D 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 =3D=3D NULL) { > + goto fail; > + } > + > + if (file !=3D NULL) { > + QDict *options =3D NULL; > + if (file_id !=3D NULL) { > + options =3D 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 =3D=3D NULL) { > + goto fail; > + } > + } else if (file_id !=3D NULL) { > + file_bs =3D bdrv_find_node(file_id); > + if (file_bs =3D=3D NULL) { > + error_setg(errp, "node '%s' is not found", drive); > + goto fail; > + } > + } else { > + file_bs =3D for_bs; > + } > + > + if (granularity =3D=3D 0) { > + granularity =3D bdrv_get_default_bitmap_granularity(for_bs); > + } > + > + bitmap =3D bdrv_load_dirty_bitmap(for_bs, file_bs, granularity, na= me, > + errp); > + if (*errp !=3D NULL) { > + goto fail; > + } > + > + if (bitmap =3D=3D NULL) { > + /* bitmap is not found in file_bs */ > + bitmap =3D 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 !=3D NULL) { > + bdrv_close(file_bs); > + } > + return -1; > +} > + > static int drive_init_func(QemuOpts *opts, void *opaque) > { > BlockInterfaceType *block_default_type =3D opaque; > @@ -2790,6 +2879,7 @@ int main(int argc, char **argv, char **envp) > module_call_init(MODULE_INIT_QOM); > =20 > 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) =3D=3D NULL) { > + exit(1); > + } > + break; > case QEMU_OPTION_set: > if (qemu_set_option(optarg) !=3D 0) > exit(1); > @@ -4198,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > =20 > parse_numa_opts(machine_class); > =20 > + if (qemu_opts_foreach(qemu_find_opts("dirty-bitmap"), dirty_bitmap= _func, > + NULL, 1) !=3D 0) { > + exit(1); > + } > + > if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, = 1) !=3D 0) { > exit(1); > } >=20 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.