Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Mon, Mar 27, 2017 at 6:04 PM, ashish mittal wrote: > On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi wrote: >> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: >> >> Have you tested live migration? >> >> If live migration is not supported then a migration blocker should be >> added using migrate_add_blocker(). >> > > We do support live migration. We have been testing a fork of this code > (slightly different version) with live migration. > >>> v10 changelog: >>> (1) Implemented accepting TLS creds per block device via the CLI >>> (see 3rd e.g in commit log). Corresponding changes made to the >>> libqnio library. >>> (2) iio_open() changed to accept TLS creds and use these internally >>> to set up SSL connections. >>> (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used >>> for authentication in any way. >> >> Why does the code still access qemu_uuid and pass the UUID string to >> iio_init()? >> > > I was of the opinion that knowing what instance (for qemu-kvm case) > was opening a block device could be a useful piece of information for > the block device to have in the future. > >> In libqnio.git (66698ca47bc594a9f623c240d63ea535f5a42b47) the 'instance' >> field is unused and not sent over the wire. Please drop it. >> > > It is not used at present. I will drop it. > >>> diff --git a/block/vxhs.c b/block/vxhs.c >>> new file mode 100644 >>> index 000..b98b535 >>> --- /dev/null >>> +++ b/block/vxhs.c >>> @@ -0,0 +1,595 @@ >>> +/* >>> + * QEMU Block driver for Veritas HyperScale (VxHS) >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>> later. >>> + * See the COPYING file in the top-level directory. >>> + * >>> + */ >>> + >>> +#include "qemu/osdep.h" >>> +#include >>> +#include >>> +#include "block/block_int.h" >>> +#include "qapi/qmp/qerror.h" >>> +#include "qapi/qmp/qdict.h" >>> +#include "qapi/qmp/qstring.h" >>> +#include "trace.h" >>> +#include "qemu/uri.h" >>> +#include "qapi/error.h" >>> +#include "qemu/uuid.h" >>> +#include "crypto/tlscredsx509.h" >>> + >>> +#define VXHS_OPT_FILENAME "filename" >>> +#define VXHS_OPT_VDISK_ID "vdisk-id" >>> +#define VXHS_OPT_SERVER "server" >>> +#define VXHS_OPT_HOST "host" >>> +#define VXHS_OPT_PORT "port" >>> + >>> +QemuUUID qemu_uuid __attribute__ ((weak)); >>> + >>> +static uint32_t vxhs_ref; >> >> It would be nice to add: >> /* Only accessed under QEMU global mutex */ >> > Will do. > >>> +/* >>> + * Parse the incoming URI and populate *options with the host information. >>> + * URI syntax has the limitation of supporting only one host info. >>> + * To pass multiple host information, use the JSON syntax. >> >> References to multiple hosts are out of date. The driver only supports >> a single host now. >> > Will change. > >>> + */ >>> +static int vxhs_parse_uri(const char *filename, QDict *options) >>> +{ >>> +URI *uri = NULL; >>> +char *hoststr, *portstr; >>> +char *port; >>> +int ret = 0; >>> + >>> +trace_vxhs_parse_uri_filename(filename); >>> +uri = uri_parse(filename); >>> +if (!uri || !uri->server || !uri->path) { >>> +uri_free(uri); >>> +return -EINVAL; >>> +} >>> + >>> +hoststr = g_strdup(VXHS_OPT_SERVER".host"); >>> +qdict_put(options, hoststr, qstring_from_str(uri->server)); >>> +g_free(hoststr); >>> + >>> +portstr = g_strdup(VXHS_OPT_SERVER".port"); >>> +if (uri->port) { >>> +port = g_strdup_printf("%d", uri->port); >>> +qdict_put(options, portstr, qstring_from_str(port)); >>> +g_free(port); >>> +} >>> +g_free(portstr); >> >> The g_strdup()/g_free() isn't necessary for the qdict_put() key >> argument. The key belongs to the caller so we can pass a string >> literal: > > Will Change. > >> >> qdict_put(options, VXHS_OPT_SERVER ".host", qstring_from_str(uri->server)); >> if (uri->port) { >> port = g_strdup_printf("%d", uri->port); >> qdict_put(options, VXHS_OPT_SERVER ".port", qstring_from_str(port)); >> g_free(port); >> } >> >>> + >>> +if (strstr(uri->path, "vxhs") == NULL) { >> >> What does this check do? >> > > Not sure about the history, but it's been there since first code > draft. Will check if it serves any purpose, or remove it. > >>> +static int vxhs_open(BlockDriverState *bs, QDict *options, >>> + int bdrv_flags, Error **errp) >>> +{ >>> +BDRVVXHSState *s = bs->opaque; >>> +void *dev_handlep = NULL; >>> +QDict *backing_options = NULL; >>> +QemuOpts *opts, *tcp_opts; >>> +char *of_vsa_addr = NULL; >>> +Error *local_err = NULL; >>> +const char *vdisk_id_opt; >>> +const char *server_host_opt; >>> +char *str = NULL; >>> +int ret = 0; >>> +char *cacert = NULL; >>> +char *client_key = NULL; >>> +char *client_cert = NULL; >>> + >>> +ret = vxhs_init_and_ref(); >>> +if (ret < 0) { >>> +return
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Fri, Mar 31, 2017 at 11:25:02AM -0700, ashish mittal wrote: > On Mon, Mar 27, 2017 at 6:04 PM, ashish mittal wrote: > > On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi > > wrote: > >> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: > >> > >> Have you tested live migration? > >> > >> If live migration is not supported then a migration blocker should be > >> added using migrate_add_blocker(). > >> > > > > We do support live migration. We have been testing a fork of this code > > (slightly different version) with live migration. The reason I ask is because this patch doesn't implement the BlockDriver bdrv_invalidate_cache()/bdrv_inactivate() callbacks. These functions are invoked during live migration so that the block driver can ensure data consistency. Since the destination QEMU process is launched while the source QEMU is still running and making changes to the disk, some block drivers need to discard metadata at migration handover time that was read upon opening the image on the destination. The guarantees that they see the latest metadata. Not sure if libvxhs caches anything that might get stale during live migration, but I wanted to raise this question? Regarding a fork of this code that you haven't posted to the mailing list, it doesn't exist as far as anyone here is concerned :). Therefore either the code on the mailing list needs to support migration or it must register a migration blocker to prevent migration. > >>> +static BlockDriver bdrv_vxhs = { > >>> +.format_name = "vxhs", > >>> +.protocol_name= "vxhs", > >>> +.instance_size= sizeof(BDRVVXHSState), > >>> +.bdrv_file_open = vxhs_open, > >>> +.bdrv_parse_filename = vxhs_parse_filename, > >>> +.bdrv_close = vxhs_close, > >>> +.bdrv_getlength = vxhs_getlength, > >>> +.bdrv_aio_readv = vxhs_aio_readv, > >>> +.bdrv_aio_writev = vxhs_aio_writev, > >> > >> Missing .bdrv_aio_flush(). Does VxHS promise that every completed write > >> request is persistent? > >> > > > > Yes, every acknowledged write request is persistent. > > > >> In that case it may be better to disable the emulated disk write cache > >> so the guest operating system and application know not to send flush > >> commands. > > > > We do pass "cache=none" on the qemu command line for every block > > device. Are there any other code changes necessary? Any pointers will > > help. > > > > Upon further reading, I now understand that cache=none will not > disable the emulated disk write cache. I am trying to understand if - > (1) It should still not be a problem since flush will just be a no-op for us. The guest operating system and applications may take different code paths depending on the state of the disk write cache. Useless vmexits can be eliminated if the guest doesn't need to send flush commands. Hence the file system and applications may perform better. > (2) Is there a way, or reason, to disable the emulated disk write > cache in the code for vxhs? I think passing WCE=0 to the guest has > something to do with this, although I have yet to figure out what that > means. Right, WCE == "Write Cache Enable". If you disable the write cache then the guest's SCSI disk or virtio-blk drivers will notice that the disk does not require flush commands. Try launching a guest with -drive if=none,id=drive0,cache=directsync,... and you should see that the write cache is disabled: # cat /sys/block/vda/queue/write_cache > (3) Is this a must for merge? This doesn't affect the block driver code so no change is necessary. signature.asc Description: PGP signature
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Mon, Apr 3, 2017 at 8:11 AM, Stefan Hajnoczi wrote: > On Fri, Mar 31, 2017 at 11:25:02AM -0700, ashish mittal wrote: >> On Mon, Mar 27, 2017 at 6:04 PM, ashish mittal wrote: >> > On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi >> > wrote: >> >> On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: >> >> >> >> Have you tested live migration? >> >> >> >> If live migration is not supported then a migration blocker should be >> >> added using migrate_add_blocker(). >> >> >> > >> > We do support live migration. We have been testing a fork of this code >> > (slightly different version) with live migration. > > The reason I ask is because this patch doesn't implement the > BlockDriver bdrv_invalidate_cache()/bdrv_inactivate() callbacks. These > functions are invoked during live migration so that the block driver can > ensure data consistency. > > Since the destination QEMU process is launched while the source QEMU is > still running and making changes to the disk, some block drivers need to > discard metadata at migration handover time that was read upon opening > the image on the destination. The guarantees that they see the latest > metadata. > Thanks! During live migration, we point the vdisks to the same host as the migration source (specified on the vxhs command line), thereby insuring consistent view of data. > Not sure if libvxhs caches anything that might get stale during live > migration, but I wanted to raise this question? > > Regarding a fork of this code that you haven't posted to the mailing > list, it doesn't exist as far as anyone here is concerned :). Therefore > either the code on the mailing list needs to support migration or it > must register a migration blocker to prevent migration. > The code on the mailing list does support live migration. Live migration requires the support of proprietary vxhs bits and also our orchestration code within OpenStack. We have been testing live migration and it works without any data consistency issues. >> >>> +static BlockDriver bdrv_vxhs = { >> >>> +.format_name = "vxhs", >> >>> +.protocol_name= "vxhs", >> >>> +.instance_size= sizeof(BDRVVXHSState), >> >>> +.bdrv_file_open = vxhs_open, >> >>> +.bdrv_parse_filename = vxhs_parse_filename, >> >>> +.bdrv_close = vxhs_close, >> >>> +.bdrv_getlength = vxhs_getlength, >> >>> +.bdrv_aio_readv = vxhs_aio_readv, >> >>> +.bdrv_aio_writev = vxhs_aio_writev, >> >> >> >> Missing .bdrv_aio_flush(). Does VxHS promise that every completed write >> >> request is persistent? >> >> >> > >> > Yes, every acknowledged write request is persistent. >> > >> >> In that case it may be better to disable the emulated disk write cache >> >> so the guest operating system and application know not to send flush >> >> commands. >> > >> > We do pass "cache=none" on the qemu command line for every block >> > device. Are there any other code changes necessary? Any pointers will >> > help. >> > >> >> Upon further reading, I now understand that cache=none will not >> disable the emulated disk write cache. I am trying to understand if - >> (1) It should still not be a problem since flush will just be a no-op for us. > > The guest operating system and applications may take different code > paths depending on the state of the disk write cache. > > Useless vmexits can be eliminated if the guest doesn't need to send > flush commands. Hence the file system and applications may perform > better. > >> (2) Is there a way, or reason, to disable the emulated disk write >> cache in the code for vxhs? I think passing WCE=0 to the guest has >> something to do with this, although I have yet to figure out what that >> means. > > Right, WCE == "Write Cache Enable". If you disable the write cache then > the guest's SCSI disk or virtio-blk drivers will notice that the disk > does not require flush commands. > > Try launching a guest with -drive if=none,id=drive0,cache=directsync,... > and you should see that the write cache is disabled: > > # cat /sys/block/vda/queue/write_cache > Thanks! Will try this. As you mentioned, this might give us a slightly better performance as it will avoid unnecessary flush from the guest OS/app. I was wondering if there was a separate command line option (other than cache=directsync) for passing WCE=0? Reading some of the articles out there almost suggested as if there was! Maybe I was just confused. >> (3) Is this a must for merge? > > This doesn't affect the block driver code so no change is necessary.
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On 03/26/2017 09:50 PM, Ashish Mittal wrote: > Source code for the qnio library that this code loads can be downloaded from: > https://github.com/VeritasHyperScale/libqnio.git When sending a multi-patch series, please include a 0/2 cover letter ('git config format.coverletter auto' can help). > > Sample command line using JSON syntax: > ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0 > -k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 > -msg timestamp=on > 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", > "server":{"host":"172.172.17.4","port":""}}' > > Sample command line using URI syntax: > qemu-img convert -f raw -O raw -n > /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad > vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0 Do we really need URI syntax, now that we have -blockdev-add going into 2.9? > > Sample command line using TLS credentials (run in secure mode): > ./qemu-io --object > tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read > -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "", > "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}' > > Signed-off-by: Ashish Mittal > --- > I'm just doing a high-level review of the interface, and leaving the actual code review to others. > +++ b/block/vxhs.c > @@ -0,0 +1,595 @@ > +/* > + * QEMU Block driver for Veritas HyperScale (VxHS) > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * No Copyright notice? The GPL works by virtue of copyright law, so generally a copyright holder should be mentioned. > +++ b/qapi/block-core.json > @@ -2118,6 +2118,7 @@ > # @iscsi: Since 2.9 > # @rbd: Since 2.9 > # @sheepdog: Since 2.9 > +# @vxhs: Since 2.10 > # > # Since: 2.0 > ## > @@ -2127,7 +2128,7 @@ > 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs', > 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', > 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh', > -'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } > +'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] } Markus has patches pending (to promote x-blockdev-del over to blockdev-del) that will conflict in context with this patch. You may have to rebase again, but hopefully the conflicts should be easy to figure out. > > ## > # @BlockdevOptionsFile: > @@ -2820,6 +2821,22 @@ >'data': { '*offset': 'int', '*size': 'int' } } > > ## > +# @BlockdevOptionsVxHS: > +# > +# Driver specific block device options for VxHS > +# > +# @vdisk-id:UUID of VxHS volume > +# @server: vxhs server IP, port > +# @tls-creds: TLS credentials ID > +# > +# Since: 2.10 > +## > +{ 'struct': 'BlockdevOptionsVxHS', > + 'data': { 'vdisk-id': 'str', > +'server': 'InetSocketAddress', Do you want to use the new InetSocketAddressBase (just host and port, eliminating things like 'to' that don't make much sense in this context)? > +'*tls-creds': 'str' } } > + > +## > # @BlockdevOptions: > # > # Options for creating a block device. Many options are available for all > @@ -2881,7 +2898,8 @@ >'vhdx': 'BlockdevOptionsGenericFormat', >'vmdk': 'BlockdevOptionsGenericCOWFormat', >'vpc':'BlockdevOptionsGenericFormat', > - 'vvfat': 'BlockdevOptionsVVFAT' > + 'vvfat': 'BlockdevOptionsVVFAT', > + 'vxhs': 'BlockdevOptionsVxHS' >} } > > ## > -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: Have you tested live migration? If live migration is not supported then a migration blocker should be added using migrate_add_blocker(). > v10 changelog: > (1) Implemented accepting TLS creds per block device via the CLI > (see 3rd e.g in commit log). Corresponding changes made to the > libqnio library. > (2) iio_open() changed to accept TLS creds and use these internally > to set up SSL connections. > (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used > for authentication in any way. Why does the code still access qemu_uuid and pass the UUID string to iio_init()? In libqnio.git (66698ca47bc594a9f623c240d63ea535f5a42b47) the 'instance' field is unused and not sent over the wire. Please drop it. > diff --git a/block/vxhs.c b/block/vxhs.c > new file mode 100644 > index 000..b98b535 > --- /dev/null > +++ b/block/vxhs.c > @@ -0,0 +1,595 @@ > +/* > + * QEMU Block driver for Veritas HyperScale (VxHS) > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + * > + */ > + > +#include "qemu/osdep.h" > +#include > +#include > +#include "block/block_int.h" > +#include "qapi/qmp/qerror.h" > +#include "qapi/qmp/qdict.h" > +#include "qapi/qmp/qstring.h" > +#include "trace.h" > +#include "qemu/uri.h" > +#include "qapi/error.h" > +#include "qemu/uuid.h" > +#include "crypto/tlscredsx509.h" > + > +#define VXHS_OPT_FILENAME "filename" > +#define VXHS_OPT_VDISK_ID "vdisk-id" > +#define VXHS_OPT_SERVER "server" > +#define VXHS_OPT_HOST "host" > +#define VXHS_OPT_PORT "port" > + > +QemuUUID qemu_uuid __attribute__ ((weak)); > + > +static uint32_t vxhs_ref; It would be nice to add: /* Only accessed under QEMU global mutex */ > +/* > + * Parse the incoming URI and populate *options with the host information. > + * URI syntax has the limitation of supporting only one host info. > + * To pass multiple host information, use the JSON syntax. References to multiple hosts are out of date. The driver only supports a single host now. > + */ > +static int vxhs_parse_uri(const char *filename, QDict *options) > +{ > +URI *uri = NULL; > +char *hoststr, *portstr; > +char *port; > +int ret = 0; > + > +trace_vxhs_parse_uri_filename(filename); > +uri = uri_parse(filename); > +if (!uri || !uri->server || !uri->path) { > +uri_free(uri); > +return -EINVAL; > +} > + > +hoststr = g_strdup(VXHS_OPT_SERVER".host"); > +qdict_put(options, hoststr, qstring_from_str(uri->server)); > +g_free(hoststr); > + > +portstr = g_strdup(VXHS_OPT_SERVER".port"); > +if (uri->port) { > +port = g_strdup_printf("%d", uri->port); > +qdict_put(options, portstr, qstring_from_str(port)); > +g_free(port); > +} > +g_free(portstr); The g_strdup()/g_free() isn't necessary for the qdict_put() key argument. The key belongs to the caller so we can pass a string literal: qdict_put(options, VXHS_OPT_SERVER ".host", qstring_from_str(uri->server)); if (uri->port) { port = g_strdup_printf("%d", uri->port); qdict_put(options, VXHS_OPT_SERVER ".port", qstring_from_str(port)); g_free(port); } > + > +if (strstr(uri->path, "vxhs") == NULL) { What does this check do? > +static int vxhs_open(BlockDriverState *bs, QDict *options, > + int bdrv_flags, Error **errp) > +{ > +BDRVVXHSState *s = bs->opaque; > +void *dev_handlep = NULL; > +QDict *backing_options = NULL; > +QemuOpts *opts, *tcp_opts; > +char *of_vsa_addr = NULL; > +Error *local_err = NULL; > +const char *vdisk_id_opt; > +const char *server_host_opt; > +char *str = NULL; > +int ret = 0; > +char *cacert = NULL; > +char *client_key = NULL; > +char *client_cert = NULL; > + > +ret = vxhs_init_and_ref(); > +if (ret < 0) { > +return ret; > +} > + > +/* Create opts info from runtime_opts and runtime_tcp_opts list */ > +opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > +tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0, &error_abort); > + > +qemu_opts_absorb_qdict(opts, options, &local_err); > +if (local_err) { > +ret = -EINVAL; > +goto out; > +} > + > +/* vdisk-id is the disk UUID */ > +vdisk_id_opt = qemu_opt_get(opts, VXHS_OPT_VDISK_ID); > +if (!vdisk_id_opt) { > +error_setg(&local_err, QERR_MISSING_PARAMETER, VXHS_OPT_VDISK_ID); > +ret = -EINVAL; > +goto out; > +} > + > +/* vdisk-id may contain a leading '/' */ > +if (strlen(vdisk_id_opt) > UUID_FMT_LEN + 1) { > +error_setg(&local_err, "vdisk-id cannot be more than %d characters", > + UUID_FMT_LEN); > +ret = -EINVAL; > +goto out; > +} > + > +s->vdisk_guid =
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Mon, Mar 27, 2017 at 8:56 AM, Eric Blake wrote: > On 03/26/2017 09:50 PM, Ashish Mittal wrote: >> Source code for the qnio library that this code loads can be downloaded from: >> https://github.com/VeritasHyperScale/libqnio.git > > When sending a multi-patch series, please include a 0/2 cover letter > ('git config format.coverletter auto' can help). > Will do! >> >> Sample command line using JSON syntax: >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0 >> -k en-us -vga cirrus -device >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 >> -msg timestamp=on >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", >> "server":{"host":"172.172.17.4","port":""}}' >> >> Sample command line using URI syntax: >> qemu-img convert -f raw -O raw -n >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0 > > Do we really need URI syntax, now that we have -blockdev-add going into 2.9? > Having it just makes manual testing with CLI easier (less to type). Would like to retain it for now, unless having this is a problem. Removing it will also require rework and retesting of the qemu-iotests patch. >> >> Sample command line using TLS credentials (run in secure mode): >> ./qemu-io --object >> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read >> -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "", >> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}' >> >> Signed-off-by: Ashish Mittal >> --- >> > > I'm just doing a high-level review of the interface, and leaving the > actual code review to others. > >> +++ b/block/vxhs.c >> @@ -0,0 +1,595 @@ >> +/* >> + * QEMU Block driver for Veritas HyperScale (VxHS) >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + * > > No Copyright notice? The GPL works by virtue of copyright law, so > generally a copyright holder should be mentioned. > I guess what's missing is the "Copyright (c) Veritas, LLC. 2017" line. Is that correct? > >> +++ b/qapi/block-core.json >> @@ -2118,6 +2118,7 @@ >> # @iscsi: Since 2.9 >> # @rbd: Since 2.9 >> # @sheepdog: Since 2.9 >> +# @vxhs: Since 2.10 >> # >> # Since: 2.0 >> ## >> @@ -2127,7 +2128,7 @@ >> 'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd', 'nfs', >> 'null-aio', 'null-co', 'parallels', 'qcow', 'qcow2', 'qed', >> 'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh', >> -'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat' ] } >> +'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] } > > Markus has patches pending (to promote x-blockdev-del over to > blockdev-del) that will conflict in context with this patch. You may > have to rebase again, but hopefully the conflicts should be easy to > figure out. > I will rebase again as needed. Not having vxhs code merged yet does create conflicts while rebasing. It would have been much easier to just send out patches for the review comments :) >> >> ## >> # @BlockdevOptionsFile: >> @@ -2820,6 +2821,22 @@ >>'data': { '*offset': 'int', '*size': 'int' } } >> >> ## >> +# @BlockdevOptionsVxHS: >> +# >> +# Driver specific block device options for VxHS >> +# >> +# @vdisk-id:UUID of VxHS volume >> +# @server: vxhs server IP, port >> +# @tls-creds: TLS credentials ID >> +# >> +# Since: 2.10 >> +## >> +{ 'struct': 'BlockdevOptionsVxHS', >> + 'data': { 'vdisk-id': 'str', >> +'server': 'InetSocketAddress', > > Do you want to use the new InetSocketAddressBase (just host and port, > eliminating things like 'to' that don't make much sense in this context)? > Don't see InetSocketAddressBase in the latest code. Could you please give me a usage example for reference? Thanks! >> +'*tls-creds': 'str' } } >> + >> +## >> # @BlockdevOptions: >> # >> # Options for creating a block device. Many options are available for all >> @@ -2881,7 +2898,8 @@ >>'vhdx': 'BlockdevOptionsGenericFormat', >>'vmdk': 'BlockdevOptionsGenericCOWFormat', >>'vpc':'BlockdevOptionsGenericFormat', >> - 'vvfat': 'BlockdevOptionsVVFAT' >> + 'vvfat': 'BlockdevOptionsVVFAT', >> + 'vxhs': 'BlockdevOptionsVxHS' >>} } >> >> ## >> > > -- > Eric Blake eblake redhat com+1-919-301-3266 > Libvirt virtualization library http://libvirt.org >
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On 03/27/2017 01:25 PM, ashish mittal wrote: >>> >>> Sample command line using URI syntax: >>> qemu-img convert -f raw -O raw -n >>> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad >>> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0 >> >> Do we really need URI syntax, now that we have -blockdev-add going into 2.9? >> > > Having it just makes manual testing with CLI easier (less to type). > Would like to retain it for now, unless having this is a problem. > Removing it will also require rework and retesting of the qemu-iotests > patch. At this point, it's a maintenance burden. We're trying to get rid of QemuOpts madness in favor of sane QMP design, and every new driver that adds yet another URI hack on top of QemuOpts makes it that much harder to get rid of the cruft. I'm not going to be the one to outright reject a URI scheme as long as the QMP works with equal features. But given the churn currently going on about RBD support for 2.9, I want to make absolutely sure we get the QMP part exactly right, and don't have to maintain any extra translation layers to URI than absolutely necessary. >> >> No Copyright notice? The GPL works by virtue of copyright law, so >> generally a copyright holder should be mentioned. >> > > I guess what's missing is the "Copyright (c) Veritas, LLC. 2017" line. > Is that correct? Yes. (Your company's legal team may have specific requirements or advice on exactly how to format the line; I can't speak to whether you have complied with those checklists, but from my perspective your proposal looks fine). >>> +## >>> +{ 'struct': 'BlockdevOptionsVxHS', >>> + 'data': { 'vdisk-id': 'str', >>> +'server': 'InetSocketAddress', >> >> Do you want to use the new InetSocketAddressBase (just host and port, >> eliminating things like 'to' that don't make much sense in this context)? >> > > Don't see InetSocketAddressBase in the latest code. Could you please > give me a usage example for reference? Thanks! Part of Markus' pending series that you'd have to rebase on ;) https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg05215.html -- Eric Blake eblake redhat com+1-919-301-3266 Libvirt virtualization library http://libvirt.org signature.asc Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Mon, Mar 27, 2017 at 10:27 AM, Stefan Hajnoczi wrote: > On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: > > Have you tested live migration? > > If live migration is not supported then a migration blocker should be > added using migrate_add_blocker(). > We do support live migration. We have been testing a fork of this code (slightly different version) with live migration. >> v10 changelog: >> (1) Implemented accepting TLS creds per block device via the CLI >> (see 3rd e.g in commit log). Corresponding changes made to the >> libqnio library. >> (2) iio_open() changed to accept TLS creds and use these internally >> to set up SSL connections. >> (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used >> for authentication in any way. > > Why does the code still access qemu_uuid and pass the UUID string to > iio_init()? > I was of the opinion that knowing what instance (for qemu-kvm case) was opening a block device could be a useful piece of information for the block device to have in the future. > In libqnio.git (66698ca47bc594a9f623c240d63ea535f5a42b47) the 'instance' > field is unused and not sent over the wire. Please drop it. > It is not used at present. I will drop it. >> diff --git a/block/vxhs.c b/block/vxhs.c >> new file mode 100644 >> index 000..b98b535 >> --- /dev/null >> +++ b/block/vxhs.c >> @@ -0,0 +1,595 @@ >> +/* >> + * QEMU Block driver for Veritas HyperScale (VxHS) >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + * >> + */ >> + >> +#include "qemu/osdep.h" >> +#include >> +#include >> +#include "block/block_int.h" >> +#include "qapi/qmp/qerror.h" >> +#include "qapi/qmp/qdict.h" >> +#include "qapi/qmp/qstring.h" >> +#include "trace.h" >> +#include "qemu/uri.h" >> +#include "qapi/error.h" >> +#include "qemu/uuid.h" >> +#include "crypto/tlscredsx509.h" >> + >> +#define VXHS_OPT_FILENAME "filename" >> +#define VXHS_OPT_VDISK_ID "vdisk-id" >> +#define VXHS_OPT_SERVER "server" >> +#define VXHS_OPT_HOST "host" >> +#define VXHS_OPT_PORT "port" >> + >> +QemuUUID qemu_uuid __attribute__ ((weak)); >> + >> +static uint32_t vxhs_ref; > > It would be nice to add: > /* Only accessed under QEMU global mutex */ > Will do. >> +/* >> + * Parse the incoming URI and populate *options with the host information. >> + * URI syntax has the limitation of supporting only one host info. >> + * To pass multiple host information, use the JSON syntax. > > References to multiple hosts are out of date. The driver only supports > a single host now. > Will change. >> + */ >> +static int vxhs_parse_uri(const char *filename, QDict *options) >> +{ >> +URI *uri = NULL; >> +char *hoststr, *portstr; >> +char *port; >> +int ret = 0; >> + >> +trace_vxhs_parse_uri_filename(filename); >> +uri = uri_parse(filename); >> +if (!uri || !uri->server || !uri->path) { >> +uri_free(uri); >> +return -EINVAL; >> +} >> + >> +hoststr = g_strdup(VXHS_OPT_SERVER".host"); >> +qdict_put(options, hoststr, qstring_from_str(uri->server)); >> +g_free(hoststr); >> + >> +portstr = g_strdup(VXHS_OPT_SERVER".port"); >> +if (uri->port) { >> +port = g_strdup_printf("%d", uri->port); >> +qdict_put(options, portstr, qstring_from_str(port)); >> +g_free(port); >> +} >> +g_free(portstr); > > The g_strdup()/g_free() isn't necessary for the qdict_put() key > argument. The key belongs to the caller so we can pass a string > literal: Will Change. > > qdict_put(options, VXHS_OPT_SERVER ".host", qstring_from_str(uri->server)); > if (uri->port) { > port = g_strdup_printf("%d", uri->port); > qdict_put(options, VXHS_OPT_SERVER ".port", qstring_from_str(port)); > g_free(port); > } > >> + >> +if (strstr(uri->path, "vxhs") == NULL) { > > What does this check do? > Not sure about the history, but it's been there since first code draft. Will check if it serves any purpose, or remove it. >> +static int vxhs_open(BlockDriverState *bs, QDict *options, >> + int bdrv_flags, Error **errp) >> +{ >> +BDRVVXHSState *s = bs->opaque; >> +void *dev_handlep = NULL; >> +QDict *backing_options = NULL; >> +QemuOpts *opts, *tcp_opts; >> +char *of_vsa_addr = NULL; >> +Error *local_err = NULL; >> +const char *vdisk_id_opt; >> +const char *server_host_opt; >> +char *str = NULL; >> +int ret = 0; >> +char *cacert = NULL; >> +char *client_key = NULL; >> +char *client_cert = NULL; >> + >> +ret = vxhs_init_and_ref(); >> +if (ret < 0) { >> +return ret; >> +} >> + >> +/* Create opts info from runtime_opts and runtime_tcp_opts list */ >> +opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); >> +tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0,
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: > Source code for the qnio library that this code loads can be downloaded from: > https://github.com/VeritasHyperScale/libqnio.git > > Sample command line using JSON syntax: > ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0 > -k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 > -msg timestamp=on > 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", > "server":{"host":"172.172.17.4","port":""}}' > > Sample command line using URI syntax: > qemu-img convert -f raw -O raw -n > /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad > vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0 > > Sample command line using TLS credentials (run in secure mode): > ./qemu-io --object > tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read > -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "", > "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}' > > Signed-off-by: Ashish Mittal > --- > > v10 changelog: > (1) Implemented accepting TLS creds per block device via the CLI > (see 3rd e.g in commit log). Corresponding changes made to the > libqnio library. > (2) iio_open() changed to accept TLS creds and use these internally > to set up SSL connections. > (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used > for authentication in any way. > (4) Removed unnecessary qdict_del(backing_options, str). > (5) Added '*tls-creds' to BlockdevOptionsVxHS. > > v9 changelog: > (1) Fixes for all the review comments from v8. I have left the definition > of VXHS_UUID_DEF unchanged pending a better suggestion. > (2) qcow2 tests now pass on the vxhs test server. > (3) Packaging changes for libvxhs will be checked in to the git repo soon. > (4) I have not moved extern QemuUUID qemu_uuid to a separate header file. > > v8 changelog: > (1) Security implementation for libqnio present in branch 'securify'. > Please use 'securify' branch for building libqnio and testing > with this patch. > (2) Renamed libqnio to libvxhs. > (3) Pass instance ID to libvxhs for SSL authentication. > > v7 changelog: > (1) IO failover code has moved out to the libqnio library. > (2) Fixes for issues reported by Stefan on v6. > (3) Incorporated the QEMUBH patch provided by Stefan. > This is a replacement for the pipe mechanism used earlier. > (4) Fixes to the buffer overflows reported in libqnio. > (5) Input validations in vxhs.c to prevent any buffer overflows for > arguments passed to libqnio. > > v6 changelog: > (1) Added qemu-iotests for VxHS as a new patch in the series. > (2) Replaced release version from 2.8 to 2.9 in block-core.json. > > v5 changelog: > (1) Incorporated v4 review comments. > > v4 changelog: > (1) Incorporated v3 review comments on QAPI changes. > (2) Added refcounting for device open/close. > Free library resources on last device close. > > v3 changelog: > (1) Added QAPI schema for the VxHS driver. > > v2 changelog: > (1) Changes done in response to v1 comments. > > block/Makefile.objs | 2 + > block/trace-events | 17 ++ > block/vxhs.c | 595 > +++ > configure| 39 > qapi/block-core.json | 22 +- > 5 files changed, 673 insertions(+), 2 deletions(-) > create mode 100644 block/vxhs.c > > diff --git a/block/Makefile.objs b/block/Makefile.objs > index de96f8e..ea95530 100644 > --- a/block/Makefile.objs > +++ b/block/Makefile.objs > @@ -19,6 +19,7 @@ block-obj-$(CONFIG_LIBNFS) += nfs.o > block-obj-$(CONFIG_CURL) += curl.o > block-obj-$(CONFIG_RBD) += rbd.o > block-obj-$(CONFIG_GLUSTERFS) += gluster.o > +block-obj-$(CONFIG_VXHS) += vxhs.o > block-obj-$(CONFIG_LIBSSH2) += ssh.o > block-obj-y += accounting.o dirty-bitmap.o > block-obj-y += write-threshold.o > @@ -38,6 +39,7 @@ rbd.o-cflags := $(RBD_CFLAGS) > rbd.o-libs := $(RBD_LIBS) > gluster.o-cflags := $(GLUSTERFS_CFLAGS) > gluster.o-libs := $(GLUSTERFS_LIBS) > +vxhs.o-libs:= $(VXHS_LIBS) > ssh.o-cflags := $(LIBSSH2_CFLAGS) > ssh.o-libs := $(LIBSSH2_LIBS) > block-obj-$(if $(CONFIG_BZIP2),m,n) += dmg-bz2.o > diff --git a/block/trace-events b/block/trace-events > index 0bc5c0a..7758ec3 100644 > --- a/block/trace-events > +++ b/block/trace-events > @@ -110,3 +110,20 @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t > offset, size_t len) "s > qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, > uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64 > qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, > uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64 > qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) > "s %p acb %p ret %d offset %"PRIu64" len %zu" > + > +# block/vxh
Re: [Qemu-devel] [PATCH v10 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"
Hi Jeff, Will incorporate all your review comments with two small dependent changes as follows: @@ -302,14 +286,14 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, int bdrv_flags, Error **errp) { BDRVVXHSState *s = bs->opaque; -void *dev_handlep = NULL; +void *dev_handlep; QDict *backing_options = NULL; -QemuOpts *opts, *tcp_opts; <== These two have to be init to NULL because we potentially gfree them before assignment.. +QemuOpts *opts = NULL; +QemuOpts *tcp_opts = NULL; char *of_vsa_addr = NULL; Error *local_err = NULL; const char *vdisk_id_opt; const char *server_host_opt; -char *str = NULL; int ret = 0; char *cacert = NULL; char *client_key = NULL; @@ -317,7 +301,8 @@ static int vxhs_open(BlockDriverState *bs, QDict *options, ret = vxhs_init_and_ref(); if (ret < 0) { -return ret; +ret = -EINVAL; +goto out; <== have to call vxhs_unref() in case of an error to decrement the refcount.. } Would it help if I send out a diff over the last patch (vxhs.c only) after incorporating changes from all three reviews? I can then send v11 series if everyone okays the diff? Regards, Ashish On Tue, Mar 28, 2017 at 10:03 AM, Jeff Cody wrote: > On Sun, Mar 26, 2017 at 07:50:35PM -0700, Ashish Mittal wrote: >> Source code for the qnio library that this code loads can be downloaded from: >> https://github.com/VeritasHyperScale/libqnio.git >> >> Sample command line using JSON syntax: >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0 >> -k en-us -vga cirrus -device >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 >> -msg timestamp=on >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", >> "server":{"host":"172.172.17.4","port":""}}' >> >> Sample command line using URI syntax: >> qemu-img convert -f raw -O raw -n >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0 >> >> Sample command line using TLS credentials (run in secure mode): >> ./qemu-io --object >> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read >> -v 66000 2.5k' 'json:{"server.host": "127.0.0.1", "server.port": "", >> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}' >> >> Signed-off-by: Ashish Mittal >> --- >> >> v10 changelog: >> (1) Implemented accepting TLS creds per block device via the CLI >> (see 3rd e.g in commit log). Corresponding changes made to the >> libqnio library. >> (2) iio_open() changed to accept TLS creds and use these internally >> to set up SSL connections. >> (3) Got rid of hard-coded VXHS_UUID_DEF. qemu_uuid is no longer used >> for authentication in any way. >> (4) Removed unnecessary qdict_del(backing_options, str). >> (5) Added '*tls-creds' to BlockdevOptionsVxHS. >> >> v9 changelog: >> (1) Fixes for all the review comments from v8. I have left the definition >> of VXHS_UUID_DEF unchanged pending a better suggestion. >> (2) qcow2 tests now pass on the vxhs test server. >> (3) Packaging changes for libvxhs will be checked in to the git repo soon. >> (4) I have not moved extern QemuUUID qemu_uuid to a separate header file. >> >> v8 changelog: >> (1) Security implementation for libqnio present in branch 'securify'. >> Please use 'securify' branch for building libqnio and testing >> with this patch. >> (2) Renamed libqnio to libvxhs. >> (3) Pass instance ID to libvxhs for SSL authentication. >> >> v7 changelog: >> (1) IO failover code has moved out to the libqnio library. >> (2) Fixes for issues reported by Stefan on v6. >> (3) Incorporated the QEMUBH patch provided by Stefan. >> This is a replacement for the pipe mechanism used earlier. >> (4) Fixes to the buffer overflows reported in libqnio. >> (5) Input validations in vxhs.c to prevent any buffer overflows for >> arguments passed to libqnio. >> >> v6 changelog: >> (1) Added qemu-iotests for VxHS as a new patch in the series. >> (2) Replaced release version from 2.8 to 2.9 in block-core.json. >> >> v5 changelog: >> (1) Incorporated v4 review comments. >> >> v4 changelog: >> (1) Incorporated v3 review comments on QAPI changes. >> (2) Added refcounting for device open/close. >> Free library resources on last device close. >> >> v3 changelog: >> (1) Added QAPI schema for the VxHS driver. >> >> v2 changelog: >> (1) Changes done in response to v1 comments. >> >> block/Makefile.objs | 2 + >> block/trace-events | 17 ++ >> block/vxhs.c | 595 >> +++ >> configure| 39 >> qapi/block-core.json | 22 +- >> 5 files changed, 673 insertions(+), 2 deletions(-) >> create mode 100644 block/vxhs.c >> >> diff --git a/block/Makefile.objs b/block/Makefile.objs >> index de96f8e..ea95530 100644 >> --- a/block/Makefile.objs >> +++ b/block/Makefile.objs