[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1] block/vxhs: Add Veritas HyperScale VxHS bloc
|
From: |
Jeff Cody |
|
Subject: |
Re: [Qemu-devel] [PATCH v1] block/vxhs: Add Veritas HyperScale VxHS block device support |
|
Date: |
Wed, 26 Oct 2016 21:09:59 -0400 |
|
User-agent: |
Mutt/1.5.24 (2015-08-30) |
On Wed, Oct 26, 2016 at 09:33:30PM +0000, Buddhi Madhav wrote:
>
>
> On 10/25/16, 9:41 PM, "Jeff Cody" <address@hidden> wrote:
>
> >On Tue, Oct 25, 2016 at 03:02:07PM -0700, Ashish Mittal wrote:
> >> This patch adds support for a new block device type called "vxhs".
> >> Source code for the library that this code loads can be downloaded from:
> >> https://github.com/MittalAshish/libqnio.git
> >>
> >
> >I grabbed the latest of libqnio, compiled it (had to disable -Werror), and
> >tried it out. I was able to do a qemu-img info on a raw file, but it
> >would
> >just hang when trying a format such as qcow2. I am assuming
> >this is a limitation of test_server, and not libqnio.
>
> On my build I did not get any build errors.
>
Likely a difference in gcc versions; I am compiling with gcc 6.2.1. Here
are the warnings I get (this is just for your benefit, not really part of
this driver review):
lib/qnio/cJSON.c: In function ‘cJSON_strcasecmp’:
lib/qnio/cJSON.c:41:5: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
if (!s1) return (s1==s2)?0:1;if (!s2) return 1;
^~
lib/qnio/cJSON.c:41:34: note: ...this statement, but the latter is misleadingly
indented as if it is guarded by the ‘if’
if (!s1) return (s1==s2)?0:1;if (!s2) return 1;
^~
lib/qnio/cJSON.c: In function ‘print_object’:
lib/qnio/cJSON.c:440:9: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
if (fmt) *ptr++='\n';*ptr=0;
^~
lib/qnio/cJSON.c:440:30: note: ...this statement, but the latter is
misleadingly indented as if it is guarded by the ‘if’
if (fmt) *ptr++='\n';*ptr=0;
^
lib/qnio/cJSON.c: In function ‘cJSON_DetachItemFromArray’:
lib/qnio/cJSON.c:467:5: warning: this ‘if’ clause does not guard...
[-Wmisleading-indentation]
if (c->prev) c->prev->next=c->next;if (c->next) c->next->prev=c->prev;if
(c==array->child) array->child=c->next;c->prev=c->next=0;return c;}
^~
lib/qnio/cJSON.c:467:40: note: ...this statement, but the latter is
misleadingly indented as if it is guarded by the ‘if’
if (c->prev) c->prev->next=c->next;if (c->next) c->next->prev=c->prev;if
(c==array->child) array->child=c->next;c->prev=c->next=0;return c;}
> The qcow2 issue is to do with the limitation in the test server, which we
> will fix in a seperate patch.
>
> >
> >This will make qemu-iotests more difficult however.
> >
> >I haven't looked at the latest qnio code yet (other than compiling the
> >test-server to test), so the rest of this review is on the qemu driver.
> >
> >> Sample command line using JSON syntax:
> >> ./qemu-system-x86_64 -name instance-00000008 -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":"9999"}]}'
> >>
> >> 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:9999/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
> >>
> >> Signed-off-by: Ashish Mittal <address@hidden>
> >> ---
> >> block/Makefile.objs | 2 +
> >> block/trace-events | 22 ++
> >> block/vxhs.c | 736
> >>++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> configure | 41 +++jj
> >> 4 files changed, 801 insertions(+)
> >> create mode 100644 block/vxhs.c
> >
> >I think this version still does not address Daniel's concerns regarding a
> >QAPI schema for vxhs.
>
> We are working on QAPI schema changes and will submit them in separate
> patch.
>
When you submit them, since there is a dependency with the patches, it
should be submitted as a patch series along with this patch. Same with the
qemu-iotests, once those are ready.
> >
> >We are also still needing qemu-iotests, and a test-server suitable to run
> >the tests.
> >
> >>
> >> diff --git a/block/Makefile.objs b/block/Makefile.objs
> >> index 67a036a..58313a2 100644
> >> --- a/block/Makefile.objs
> >> +++ b/block/Makefile.objs
> >> @@ -18,6 +18,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_ARCHIPELAGO) += archipelago.o
> >> block-obj-$(CONFIG_LIBSSH2) += ssh.o
> >> block-obj-y += accounting.o dirty-bitmap.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)
> >> archipelago.o-libs := $(ARCHIPELAGO_LIBS)
> >> diff --git a/block/trace-events b/block/trace-events
> >> index 05fa13c..aea97cb 100644
> >> --- a/block/trace-events
> >> +++ b/block/trace-events
> >> @@ -114,3 +114,25 @@ 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/vxhs.c
> >> +vxhs_iio_callback(int error, int reason) "ctx is NULL: error %d,
> >>reason %d"
> >> +vxhs_setup_qnio(void *s) "Context to HyperScale IO manager = %p"
> >> +vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no
> >>i/o %d, %d"
> >> +vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d,
> >>errno %d"
> >> +vxhs_open_fail(int ret) "Could not open the device. Error = %d"
> >> +vxhs_open_epipe(int ret) "Could not create a pipe for device. Bailing
> >>out. Error=%d"
> >> +vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d"
> >> +vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off,
> >>void *acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d
> >>size = %lu offset = %lu ACB = %p. Error = %d, errno = %d"
> >> +vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat
> >>ioctl failed, ret = %d, errno = %d"
> >> +vxhs_get_vdisk_stat(char *vdisk_guid, uint64_t vdisk_size) "vDisk %s
> >>stat ioctl returned size %lu"
> >> +vxhs_qnio_iio_open(const char *ip) "Failed to connect to storage agent
> >>on host-ip %s"
> >> +vxhs_qnio_iio_devopen(const char *fname) "Failed to open vdisk device:
> >>%s"
> >> +vxhs_complete_aio(void *acb, uint64_t ret) "aio failed acb %p ret %ld"
> >> +vxhs_parse_uri_filename(const char *filename) "URI passed via
> >>bdrv_parse_filename %s"
> >> +vxhs_qemu_init_vdisk(const char *vdisk_id) "vdisk_id from json %s"
> >> +vxhs_qemu_init_numservers(int num_servers) "Number of servers passed =
> >>%d"
> >> +vxhs_parse_uri_hostinfo(int num, char *host, int port) "Host %d: IP
> >>%s, Port %d"
> >> +vxhs_qemu_init(char *of_vsa_addr, int port) "Adding host %s:%d to
> >>BDRVVXHSState"
> >> +vxhs_qemu_init_filename(const char *filename) "Filename passed as %s"
> >> +vxhs_close(char *vdisk_guid) "Closing vdisk %s"
> >> diff --git a/block/vxhs.c b/block/vxhs.c
> >> new file mode 100644
> >> index 0000000..97fb804
> >> --- /dev/null
> >> +++ b/block/vxhs.c
> >> @@ -0,0 +1,736 @@
> >> +/*
> >> + * 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 "block/block_int.h"
> >> +#include <qnio/qnio_api.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/error-report.h"
> >> +
> >> +#define VDISK_FD_READ 0
> >> +#define VDISK_FD_WRITE 1
> >> +
> >> +#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"
> >> +
> >> +/* qnio client ioapi_ctx */
> >> +static void *global_qnio_ctx;
> >> +
> >
> >
> >Why create a qnio ctx as a static global, rather than confining it to
> >BDRVVXHSState, so that it is unique to each disk instance?
> >
> >Is the following usage supported (i.e. two different vxhs remote servers):
> >
> >qemu-system-x86_64 -drive file=vxhs://server1:9999/test1 \
> > -drive file=vxhs://server2:9999/test2
>
> ³global_qnio_ctx² is a global context, not per-disk context. Removed the
> qnio_ctx field
> From ³BDRVVXHSState² structure. The above command should work.
>
OK, great, thanks for the clarification. I assume then there is no
drive-specific or host-specific stateful information in the global_qnio_ctx
structure.
How about cleanup on close? Looking at the libqnio code, it allocs memory
during the qnio_ctx creation. Currently, I don't think there is anything in
the vxhs driver that frees the global_qnio_ctx and its internal structures.
Looking at qnio_api.h, there also doesn't seem to be an api that will do
that, either.
We need to be able to free all resources once we close the last vxhs drive.
Since multiple drives could all be using the global_qnio_ctx, I think you'll
likely need to wrap it in a struct and create a ref counter. Increment the
ref on every open, and decrement the ref on close (and if zero, free the
resources).
(If my interpretation is incorrect, and it is already freed somewhere, just
let me know).
> >
> >
> >Why store the qnio ctx into this global briefly, and just to move it to
> >where it belongs in the BDRVVXHSState struct? It makes more sense to be
> >in
> >the state struct the whole time, unless I am missing something fundamental
> >about the protocol.
> >
> >
> >> +/* vdisk prefix to pass to qnio */
> >> +static const char vdisk_prefix[] = "/dev/of/vdisk";
> >> +
> >> +typedef enum {
> >> + VXHS_IO_INPROGRESS,
> >> + VXHS_IO_COMPLETED,
> >> + VXHS_IO_ERROR
> >> +} VXHSIOState;
> >> +
> >> +typedef enum {
> >> + VDISK_AIO_READ,
> >> + VDISK_AIO_WRITE,
> >> + VDISK_STAT
> >> +} VDISKAIOCmd;
> >> +
> >> +/*
> >> + * HyperScale AIO callbacks structure
> >> + */
> >> +typedef struct VXHSAIOCB {
> >> + BlockAIOCB common;
> >> + int err;
> >> + int state;
> >> + int direction; /* IO direction (r/w) */
> >> + size_t io_offset;
> >> + size_t size;
> >> + QEMUBH *bh;
> >
> >*bh is unused.
>
> Removed the unused variable.
> >
> >> + QEMUIOVector *qiov;
> >> + QSIMPLEQ_ENTRY(VXHSAIOCB) retry_entry;
> >
> >retry_entry is unused.
>
> Removed the unused variable.
> >
> >> +} VXHSAIOCB;
> >> +
> >> +typedef struct VXHSvDiskHostsInfo {
> >> + int qnio_cfd; /* Channel FD */
> >> + int vdisk_rfd; /* vDisk remote FD */
> >> + char *hostip; /* Host's IP addresses */
> >> + int port; /* Host's port number */
> >> +} VXHSvDiskHostsInfo;
> >> +
> >> +/*
> >> + * Structure per vDisk maintained for state
> >> + */
> >> +typedef struct BDRVVXHSState {
> >> + int fds[2];
> >> + int64_t vdisk_size;
> >> + int64_t vdisk_blocks;
> >
> >vdisk_blocks is unused.
>
> Removed the unused variable.
> >
> >> + int event_reader_pos;
> >> + VXHSAIOCB *qnio_event_acb;
> >> + void *qnio_ctx;
> >> + VXHSvDiskHostsInfo vdisk_hostinfo; /* Per host info */
> >> + char *vdisk_guid;
> >> +} BDRVVXHSState;
> >> +
> >> +static void vxhs_qnio_iio_close(BDRVVXHSState *s)
> >> +{
> >> + /*
> >> + * Close vDisk device
> >> + */
> >> + if (s->vdisk_hostinfo.vdisk_rfd >= 0) {
> >> + iio_devclose(s->qnio_ctx, 0, s->vdisk_hostinfo.vdisk_rfd);
> >> + s->vdisk_hostinfo.vdisk_rfd = -1;
> >> + }
> >> +
> >> + /*
> >> + * Close QNIO channel against cached channel-fd
> >> + */
> >> + if (s->vdisk_hostinfo.qnio_cfd >= 0) {
> >> + iio_close(s->qnio_ctx, s->vdisk_hostinfo.qnio_cfd);
> >> + s->vdisk_hostinfo.qnio_cfd = -1;
> >> + }
> >> +}
> >> +
> >> +static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr,
> >> + int *rfd, const char *file_name)
> >> +{
> >> + /*
> >> + * Open qnio channel to storage agent if not opened before.
> >> + */
> >> + if (*cfd < 0) {
> >> + *cfd = iio_open(global_qnio_ctx, of_vsa_addr, 0);
> >> + if (*cfd < 0) {
> >> + trace_vxhs_qnio_iio_open(of_vsa_addr);
> >> + return -ENODEV;
> >> + }
> >> + }
> >> +
> >> + /*
> >> + * Open vdisk device
> >> + */
> >> + *rfd = iio_devopen(global_qnio_ctx, *cfd, file_name, 0);
> >> + if (*rfd < 0) {
> >> + if (*cfd >= 0) {
> >> + iio_close(global_qnio_ctx, *cfd);
> >> + *cfd = -1;
> >> + *rfd = -1;
> >> + }
> >> +
> >> + trace_vxhs_qnio_iio_devopen(file_name);
> >> + return -ENODEV;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void vxhs_iio_callback(int32_t rfd, uint32_t reason, void *ctx,
> >> + uint32_t error, uint32_t opcode)
> >> +{
> >> + VXHSAIOCB *acb = NULL;
> >> + BDRVVXHSState *s = NULL;
> >> + ssize_t ret;
> >> +
> >> + switch (opcode) {
> >> + case IRP_READ_REQUEST:
> >> + case IRP_WRITE_REQUEST:
> >> +
> >> + /*
> >> + * ctx is VXHSAIOCB*
> >> + * ctx is NULL if error is QNIOERROR_CHANNEL_HUP or
> >> + * reason is IIO_REASON_HUP
> >> + */
> >> + if (ctx) {
> >> + acb = ctx;
> >> + s = acb->common.bs->opaque;
> >> + } else {
> >> + trace_vxhs_iio_callback(error, reason);
> >> + goto out;
> >> + }
> >> +
> >> + if (error) {
> >> + if (!acb->err) {
> >> + acb->err = error;
> >> + }
> >> + trace_vxhs_iio_callback(error, reason);
> >> + }
> >> +
> >> + ret = qemu_write_full(s->fds[VDISK_FD_WRITE], &acb,
> >>sizeof(acb));
> >> + g_assert(ret == sizeof(acb));
> >> + break;
> >> +
> >> + default:
> >> + if (error == QNIOERROR_CHANNEL_HUP) {
> >> + /*
> >> + * Channel failed, spontaneous notification,
> >> + * not in response to I/O
> >> + */
> >> + trace_vxhs_iio_callback_chnfail(error, errno);
> >> + } else {
> >> + trace_vxhs_iio_callback_unknwn(opcode, error);
> >> + }
> >> + break;
> >> + }
> >> +out:
> >> + return;
> >> +}
> >> +
> >> +static void vxhs_complete_aio(VXHSAIOCB *acb, BDRVVXHSState *s)
> >> +{
> >> + BlockCompletionFunc *cb = acb->common.cb;
> >> + void *opaque = acb->common.opaque;
> >> + int ret = 0;
> >> +
> >> + if (acb->err != 0) {
> >> + trace_vxhs_complete_aio(acb, acb->err);
> >> + /*
> >> + * We mask all the IO errors generically as EIO for upper
> >>layers
> >> + * Right now our IO Manager uses non standard error codes.
> >>Instead
> >> + * of confusing upper layers with incorrect interpretation we
> >>are
> >> + * doing this workaround.
> >> + */
> >> + ret = (-EIO);
> >> + }
> >> +
> >> + acb->state = VXHS_IO_COMPLETED;
> >
> >What is this state used for? I see it also set to VXHS_IO_INPROGRESS at
> >the
> >beginning of the request.
>
> This was useful when failover code was implemented. It is no longer used.
> So removed the ³state² filed.
>
> >
> >> + qemu_aio_unref(acb);
> >> + cb(opaque, ret);
> >> +}
> >> +
> >> +/*
> >> + * This is the HyperScale event handler registered to QEMU.
> >> + * It is invoked when any IO gets completed and written on pipe
> >> + * by callback called from QNIO thread context. Then it marks
> >> + * the AIO as completed, and releases HyperScale AIO callbacks.
> >> + */
> >> +static void vxhs_aio_event_reader(void *opaque)
> >> +{
> >> + BDRVVXHSState *s = opaque;
> >> + char *p;
> >> + ssize_t ret;
> >> +
> >> + do {
> >> + p = (char *)&s->qnio_event_acb;
> >> + ret = read(s->fds[VDISK_FD_READ], p + s->event_reader_pos,
> >> + sizeof(s->qnio_event_acb) - s->event_reader_pos);
> >> + if (ret > 0) {
> >> + s->event_reader_pos += ret;
> >> + if (s->event_reader_pos == sizeof(s->qnio_event_acb)) {
> >> + s->event_reader_pos = 0;
> >> + vxhs_complete_aio(s->qnio_event_acb, s);
> >> + }
> >> + }
> >> + } while (ret < 0 && errno == EINTR);
> >
> >I'm bit confused on this event reader loop. If ret is > 0, but
> >s->event_reader_pos != sizeof(s->qnio_event_acb), we'll exit the event
> >reader leaving event_read_pos non-zero. And since we are reading in a
> >pointer for the acb, what does that mean for the next time we come we have
> >an aio reader event?
> On the next reader event, it will read the rest of ³acb² structure(from
> s->event_reader_pos), and calls vxhs_complete_aio().
>
>
> >
> >> +}
> >> +
> >> +/*
> >> + * Call QNIO operation to create channels to do IO on vDisk.
> >> + */
> >> +
> >> +static void *vxhs_setup_qnio(void)
> >> +{
> >> + void *qnio_ctx = NULL;
> >> +
> >> + qnio_ctx = iio_init(vxhs_iio_callback);
> >> + if (qnio_ctx != NULL) {
> >> + trace_vxhs_setup_qnio(qnio_ctx);
> >> + }
> >> + return qnio_ctx;
> >> +}
> >> +
> >> +static QemuOptsList runtime_opts = {
> >> + .name = "vxhs",
> >> + .head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> >> + .desc = {
> >> + {
> >> + .name = VXHS_OPT_FILENAME,
> >> + .type = QEMU_OPT_STRING,
> >> + .help = "URI to the Veritas HyperScale image",
> >> + },
> >> + {
> >> + .name = VXHS_OPT_VDISK_ID,
> >> + .type = QEMU_OPT_STRING,
> >> + .help = "UUID of the VxHS vdisk",
> >> + },
> >> + { /* end of list */ }
> >> + },
> >> +};
> >> +
> >> +static QemuOptsList runtime_tcp_opts = {
> >> + .name = "vxhs_tcp",
> >> + .head = QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head),
> >> + .desc = {
> >> + {
> >> + .name = VXHS_OPT_HOST,
> >> + .type = QEMU_OPT_STRING,
> >> + .help = "host address (ipv4 addresses)",
> >> + },
> >> + {
> >> + .name = VXHS_OPT_PORT,
> >> + .type = QEMU_OPT_NUMBER,
> >> + .help = "port number on which VxHSD is listening (default
> >>9999)",
> >> + .def_value_str = "9999"
> >> + },
> >> + { /* end of list */ }
> >> + },
> >> +};
> >> +
> >> +/*
> >> + * 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.
> >> + */
> >> +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"0.host");
> >> + qdict_put(options, hoststr, qstring_from_str(uri->server));
> >> + g_free(hoststr);
> >> +
> >> + portstr = g_strdup(VXHS_OPT_SERVER"0.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);
> >> +
> >> + if (strstr(uri->path, "vxhs") == NULL) {
> >> + qdict_put(options, "vdisk_id", qstring_from_str(uri->path));
> >> + }
> >> +
> >> + trace_vxhs_parse_uri_hostinfo(1, uri->server, uri->port);
> >> + uri_free(uri);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static void vxhs_parse_filename(const char *filename, QDict *options,
> >> + Error **errp)
> >> +{
> >> + if (qdict_haskey(options, "vdisk_id") || qdict_haskey(options,
> >>"server")) {
> >> + error_setg(errp, "vdisk_id/server and a file name may not be
> >>specified "
> >> + "at the same time");
> >> + return;
> >> + }
> >> +
> >> + if (strstr(filename, "://")) {
> >> + int ret = vxhs_parse_uri(filename, options);
> >> + if (ret < 0) {
> >> + error_setg(errp, "Invalid URI. URI should be of the form "
> >> + " vxhs://<host_ip>:<port>/{<vdisk_id>}");
> >> + }
> >> + }
> >> +}
> >> +
> >> +static int vxhs_qemu_init(QDict *options, BDRVVXHSState *s,
> >> + int *cfd, int *rfd, Error **errp)
> >> +{
> >> + QDict *backing_options = NULL;
> >> + QemuOpts *opts, *tcp_opts;
> >> + const char *vxhs_filename;
> >> + char *of_vsa_addr = NULL;
> >> + Error *local_err = NULL;
> >> + const char *vdisk_id_opt;
> >> + char *file_name = NULL;
> >> + size_t num_servers = 0;
> >> + char *str = NULL;
> >> + int ret = 0;
> >> +
> >> + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
> >> + qemu_opts_absorb_qdict(opts, options, &local_err);
> >> + if (local_err) {
> >> + error_propagate(errp, local_err);
> >
> >You are already propagating this error in 'out'.
> Removed ³error_propagate()² call.
>
> >
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +
> >> + vxhs_filename = qemu_opt_get(opts, VXHS_OPT_FILENAME);
> >> + if (vxhs_filename) {
> >> + trace_vxhs_qemu_init_filename(vxhs_filename);
> >> + }
> >> +
> >> + 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;
> >> + }
> >> + s->vdisk_guid = g_strdup(vdisk_id_opt);
> >> + trace_vxhs_qemu_init_vdisk(vdisk_id_opt);
> >> +
> >> + num_servers = qdict_array_entries(options, VXHS_OPT_SERVER);
> >> + if (num_servers < 1) {
> >> + error_setg(&local_err, QERR_MISSING_PARAMETER, "server");
> >> + ret = -EINVAL;
> >> + goto out;
> >> + } else if (num_servers > 1) {
> >> + error_setg(&local_err, QERR_INVALID_PARAMETER, "server");
> >> + error_append_hint(errp, "Only one server allowed.\n");
> >
> >s/errp/&local_err
>
> Replaced as suggested.
> >
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> + trace_vxhs_qemu_init_numservers(num_servers);
> >> +
> >> + str = g_strdup_printf(VXHS_OPT_SERVER"0.");
> >> + qdict_extract_subqdict(options, &backing_options, str);
> >> +
> >> + /* Create opts info from runtime_tcp_opts list */
> >> + tcp_opts = qemu_opts_create(&runtime_tcp_opts, NULL, 0,
> >>&error_abort);
> >> + qemu_opts_absorb_qdict(tcp_opts, backing_options, &local_err);
> >> + if (local_err) {
> >> + qdict_del(backing_options, str);
> >> + qemu_opts_del(tcp_opts);
> >> + g_free(str);
> >
> >Rather than free str here, you can just do it in the 'out'. (g_free is
> >NULL-safe)
>
> Moved g_free() call to ³out² label.
> >
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> +
> >> + s->vdisk_hostinfo.hostip = g_strdup(qemu_opt_get(tcp_opts,
> >> + VXHS_OPT_HOST));
> >> + s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
> >> +
> >>VXHS_OPT_PORT),
> >> + NULL, 0);
> >> +
> >> + s->vdisk_hostinfo.qnio_cfd = -1;
> >> + s->vdisk_hostinfo.vdisk_rfd = -1;
> >> + trace_vxhs_qemu_init(s->vdisk_hostinfo.hostip,
> >> + s->vdisk_hostinfo.port);
> >> +
> >> + qdict_del(backing_options, str);
> >> + qemu_opts_del(tcp_opts);
> >> + g_free(str);
> >
> >Same here.
>
> Moved g_free() call to ³out² label.
>
> >
> >> +
> >> + file_name = g_strdup_printf("%s%s", vdisk_prefix, s->vdisk_guid);
> >> + of_vsa_addr = g_strdup_printf("of://%s:%d",
> >> + s->vdisk_hostinfo.hostip,
> >> + s->vdisk_hostinfo.port);
> >> +
> >> + /*
> >> + * .bdrv_open() and .bdrv_create() run under the QEMU global mutex.
> >> + */
> >> + if (global_qnio_ctx == NULL) {
> >> + global_qnio_ctx = vxhs_setup_qnio();
> >
> >I assume this would prevent the scenario I described in the beginning
> >(two different
> >drives using two different vxhs servers) from working properly? Is that
> >by
> >design?
> >
> >If so, it needs to fail gracefully; I don't think the current
> >implementation
> >will.
> >
> >
> >But it seems like you should just be able to do
> >s->qnio_ctx = vxhs_setup_qnio() here, right?
>
> As mentioned before ³qnio_ctx² is global context, and gets initialized on
> first disk open.
>
> >
> >
> >> + if (global_qnio_ctx == NULL) {
> >> + error_setg(&local_err, "Failed vxhs_setup_qnio");
> >> + ret = -EINVAL;
> >> + goto out;
> >> + }
> >> + }
> >> +
> >> + ret = vxhs_qnio_iio_open(cfd, of_vsa_addr, rfd, file_name);
> >
> >This overwrites the host / port, and I presume other info, of other drives
> >in the case of another vxhs drive being opened. This seems wrong.
> >
> >> + if (ret) {
> >> + error_setg(&local_err, "Failed qnio_iio_open");
> >> + ret = -EIO;
> >> + }
> >> +
> >> +out:
> >> + g_free(file_name);
> >> + g_free(of_vsa_addr);
> >> + qemu_opts_del(opts);
> >> +
> >> + if (ret < 0) {
> >> + g_free(s->vdisk_hostinfo.hostip);
> >> + g_free(s->vdisk_guid);
> >> + s->vdisk_guid = NULL;
> >> + errno = -ret;
> >> + }
> >> + error_propagate(errp, local_err);
> >
> >This can be moved up into the 'if' block above it.
>
> Moved the code as suggested.
>
> >
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static int vxhs_open(BlockDriverState *bs, QDict *options,
> >> + int bdrv_flags, Error **errp)
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> + AioContext *aio_context;
> >> + int qemu_qnio_cfd = -1;
> >> + int device_opened = 0;
> >
> >A bool makes more sense here.
>
> Modified the type to ³bool² as suggested.
> >
> >> + int qemu_rfd = -1;
> >> + int ret = 0;
> >> +
> >> + ret = vxhs_qemu_init(options, s, &qemu_qnio_cfd, &qemu_rfd, errp);
> >> + if (ret < 0) {
> >> + trace_vxhs_open_fail(ret);
> >> + return ret;
> >> + }
> >> +
> >> + device_opened = 1;
> >> + s->qnio_ctx = global_qnio_ctx;
> >> + s->vdisk_hostinfo.qnio_cfd = qemu_qnio_cfd;
> >> + s->vdisk_hostinfo.vdisk_rfd = qemu_rfd;
> >> + s->vdisk_size = 0;
> >> +
> >> + /*
> >> + * Create a pipe for communicating between two threads in different
> >> + * context. Set handler for read event, which gets triggered when
> >> + * IO completion is done by non-QEMU context.
> >> + */
> >> + ret = qemu_pipe(s->fds);
> >> + if (ret < 0) {
> >> + trace_vxhs_open_epipe(ret);
> >> + ret = -errno;
> >> + goto errout;
> >> + }
> >> + fcntl(s->fds[VDISK_FD_READ], F_SETFL, O_NONBLOCK);
> >> +
> >> + aio_context = bdrv_get_aio_context(bs);
> >> + aio_set_fd_handler(aio_context, s->fds[VDISK_FD_READ],
> >> + false, vxhs_aio_event_reader, NULL, s);
> >> + return 0;
> >> +
> >> +errout:
> >> + /*
> >> + * Close remote vDisk device if it was opened earlier
> >> + */
> >> + if (device_opened) {
> >> + vxhs_qnio_iio_close(s);
> >> + }
> >> + trace_vxhs_open_fail(ret);
> >> + return ret;
> >> +}
> >> +
> >> +static const AIOCBInfo vxhs_aiocb_info = {
> >> + .aiocb_size = sizeof(VXHSAIOCB)
> >> +};
> >> +
> >> +/*
> >> + * This allocates QEMU-VXHS callback for each IO
> >> + * and is passed to QNIO. When QNIO completes the work,
> >> + * it will be passed back through the callback.
> >> + */
> >> +static BlockAIOCB *vxhs_aio_rw(BlockDriverState *bs, int64_t
> >>sector_num,
> >> + QEMUIOVector *qiov, int nb_sectors,
> >> + BlockCompletionFunc *cb, void *opaque,
> >>int iodir)
> >> +{
> >> + VXHSAIOCB *acb = NULL;
> >> + BDRVVXHSState *s = bs->opaque;
> >> + size_t size;
> >> + uint64_t offset;
> >> + int iio_flags = 0;
> >> + int ret = 0;
> >> + void *qnio_ctx = s->qnio_ctx;
> >> + uint32_t rfd = s->vdisk_hostinfo.vdisk_rfd;
> >> +
> >> + offset = sector_num * BDRV_SECTOR_SIZE;
> >> + size = nb_sectors * BDRV_SECTOR_SIZE;
> >> + acb = qemu_aio_get(&vxhs_aiocb_info, bs, cb, opaque);
> >> + /*
> >> + * Setup or initialize VXHSAIOCB.
> >> + * Every single field should be initialized since
> >> + * acb will be picked up from the slab without
> >> + * initializing with zero.
> >> + */
> >> + acb->io_offset = offset;
> >> + acb->size = size;
> >> + acb->err = 0;
> >> + acb->state = VXHS_IO_INPROGRESS;
> >> + acb->qiov = qiov;
> >> + acb->direction = iodir;
> >> +
> >> + iio_flags = (IIO_FLAG_DONE | IIO_FLAG_ASYNC);
> >> +
> >> + switch (iodir) {
> >> + case VDISK_AIO_WRITE:
> >> + ret = iio_writev(qnio_ctx, rfd, qiov->iov, qiov->niov,
> >> + offset, (uint64_t)size, (void *)acb,
> >>iio_flags);
> >> + break;
> >> + case VDISK_AIO_READ:
> >> + ret = iio_readv(qnio_ctx, rfd, qiov->iov, qiov->niov,
> >> + offset, (uint64_t)size, (void *)acb,
> >>iio_flags);
> >> + break;
> >> + default:
> >> + trace_vxhs_aio_rw_invalid(iodir);
> >> + goto errout;
> >> + }
> >> +
> >> + if (ret != 0) {
> >> + trace_vxhs_aio_rw_ioerr(s->vdisk_guid, iodir, size, offset,
> >> + acb, ret, errno);
> >> + goto errout;
> >> + }
> >> + return &acb->common;
> >> +
> >> +errout:
> >> + qemu_aio_unref(acb);
> >> + return NULL;
> >> +}
> >> +
> >> +static BlockAIOCB *vxhs_aio_readv(BlockDriverState *bs,
> >> + int64_t sector_num, QEMUIOVector
> >>*qiov,
> >> + int nb_sectors,
> >> + BlockCompletionFunc *cb, void
> >>*opaque)
> >> +{
> >> + return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors, cb,
> >> + opaque, VDISK_AIO_READ);
> >> +}
> >> +
> >> +static BlockAIOCB *vxhs_aio_writev(BlockDriverState *bs,
> >> + int64_t sector_num, QEMUIOVector
> >>*qiov,
> >> + int nb_sectors,
> >> + BlockCompletionFunc *cb, void
> >>*opaque)
> >> +{
> >> + return vxhs_aio_rw(bs, sector_num, qiov, nb_sectors,
> >> + cb, opaque, VDISK_AIO_WRITE);
> >> +}
> >> +
> >> +static void vxhs_close(BlockDriverState *bs)
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> +
> >> + trace_vxhs_close(s->vdisk_guid);
> >> + close(s->fds[VDISK_FD_READ]);
> >> + close(s->fds[VDISK_FD_WRITE]);
> >> +
> >> + /*
> >> + * Clearing all the event handlers for oflame registered to QEMU
> >> + */
> >> + aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
> >> + false, NULL, NULL, NULL);
> >> + g_free(s->vdisk_guid);
> >> + s->vdisk_guid = NULL;
> >> + vxhs_qnio_iio_close(s);
> >> +
> >> + /*
> >> + * Free the dynamically allocated hostip string
> >> + */
> >> + g_free(s->vdisk_hostinfo.hostip);
> >> + s->vdisk_hostinfo.hostip = NULL;
> >> + s->vdisk_hostinfo.port = 0;
> >> +}
> >> +
> >> +static unsigned long vxhs_get_vdisk_stat(BDRVVXHSState *s)
> >
> >The functions that call this use its value as an int64_t, although this is
> >returning an unsigned long. It should return int64_t.
>
> Modified the return value as suggested.
> >
> >> +{
> >> + int64_t vdisk_size = 0;
> >> + int ret = 0;
> >> + uint32_t rfd = s->vdisk_hostinfo.vdisk_rfd;
> >> +
> >> + ret = iio_ioctl(s->qnio_ctx, rfd, IOR_VDISK_STAT, &vdisk_size,
> >>NULL, 0);
> >> + if (ret < 0) {
> >> + trace_vxhs_get_vdisk_stat_err(s->vdisk_guid, ret, errno);
> >> + return 0;
> >
> >Repeat comment from RFC v7:
> >
> >Why is is this returning 0 here, rather then 'ret'? Does IOR_VDIST_STAT
> >return sane error codes? If not, instead of returning zero, map it to
> >-EIO
> >here.
> Changed the return value to ³-EIO² incase of error.
>
> >
> >> + }
> >> +
> >> + trace_vxhs_get_vdisk_stat(s->vdisk_guid, vdisk_size);
> >> + return vdisk_size;
> >> +}
> >> +
> >> +/*
> >> + * Returns the size of vDisk in bytes. This is required
> >> + * by QEMU block upper block layer so that it is visible
> >> + * to guest.
> >> + */
> >> +static int64_t vxhs_getlength(BlockDriverState *bs)
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> + int64_t vdisk_size = 0;
> >> +
> >> + if (s->vdisk_size > 0) {
> >> + vdisk_size = s->vdisk_size;
> >> + } else {
> >> + /*
> >> + * Fetch the vDisk size using stat ioctl
> >> + */
> >> + vdisk_size = vxhs_get_vdisk_stat(s);
> >> + if (vdisk_size > 0) {
> >> + s->vdisk_size = vdisk_size;
> >> + }
> >> + }
> >> +
> >> + if (vdisk_size > 0) {
> >> + return vdisk_size; /* return size in bytes */
> >> + }
> >> +
> >
> >Repeat comment from RFC v7:
> >
> >If vxhs_get_vdisk_stat() returned error rather than 0, then this check
> >is unnecessary. You can just return vdisk_size.
> >
> >(Also: I don't think you need to return -EIO for a zero-sized image, do
> >you?)
> Modified the code as suggested. Zero size disk will return zero.
>
> >
> >> + return -EIO;
> >> +}
> >> +
> >> +/*
> >> + * Returns actual blocks allocated for the vDisk.
> >
> >s/blocks/bytes
> >
> >> + * This is required by qemu-img utility.
> >> + */
> >> +static int64_t vxhs_get_allocated_blocks(BlockDriverState *bs)
> >
> >s/blocks/bytes
> >
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> + int64_t vdisk_size = 0;
> >> +
> >> + if (s->vdisk_size > 0) {
> >> + vdisk_size = s->vdisk_size;
> >> + } else {
> >> + /*
> >> + * TODO:
> >> + * Once HyperScale storage-virtualizer provides
> >> + * actual physical allocation of blocks then
> >> + * fetch that information and return back to the
> >> + * caller but for now just get the full size.
> >> + */
> >> + vdisk_size = vxhs_get_vdisk_stat(s);
> >> + if (vdisk_size > 0) {
> >> + s->vdisk_size = vdisk_size;
> >> + }
> >> + }
> >> +
> >> + if (vdisk_size > 0) {
> >> + return vdisk_size; /* return size in bytes */
> >> + }
> >> +
> >> + return -EIO;
> >
> >Repeat comment from RFC v7:
> >
> >Since this is just identical to vxhs_getlength(), why not just call it
> >here,
> >and reduce code duplication?
> >
> >Or even better: do not implement this function. It isn't required. If it
> >is not implemented, the block layer above this will just return -ENOTSUP,
> >which in qapi will be interpreted as "unavailable". This function should
> >be
> >dropped, until it is something supported by VXHS.
>
> Removed vxhs_get_allocated_blocks() function.
> >
> >> +}
> >> +
> >> +static void vxhs_detach_aio_context(BlockDriverState *bs)
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> +
> >> + aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
> >> + false, NULL, NULL, NULL);
> >> +
> >> +}
> >> +
> >> +static void vxhs_attach_aio_context(BlockDriverState *bs,
> >> + AioContext *new_context)
> >> +{
> >> + BDRVVXHSState *s = bs->opaque;
> >> +
> >> + aio_set_fd_handler(new_context, s->fds[VDISK_FD_READ],
> >> + false, vxhs_aio_event_reader, NULL, s);
> >> +}
> >> +
> >> +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_get_allocated_file_size = vxhs_get_allocated_blocks,
> >> + .bdrv_aio_readv = vxhs_aio_readv,
> >> + .bdrv_aio_writev = vxhs_aio_writev,
> >> + .bdrv_detach_aio_context = vxhs_detach_aio_context,
> >> + .bdrv_attach_aio_context = vxhs_attach_aio_context,
> >> +};
> >> +
> >> +static void bdrv_vxhs_init(void)
> >> +{
> >> + bdrv_register(&bdrv_vxhs);
> >> +}
> >> +
> >> +block_init(bdrv_vxhs_init);
> >> diff --git a/configure b/configure
> >> index d3dafcb..4413e88 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -321,6 +321,7 @@ numa=""
> >> tcmalloc="no"
> >> jemalloc="no"
> >> replication="yes"
> >> +vxhs=""
> >>
> >> # parse CC options first
> >> for opt do
> >> @@ -1162,6 +1163,11 @@ for opt do
> >> ;;
> >> --enable-replication) replication="yes"
> >> ;;
> >> + --disable-vxhs) vxhs="no"
> >> + ;;
> >> + --enable-vxhs) vxhs="yes"
> >> + ;;
> >> +
> >> *)
> >> echo "ERROR: unknown option $opt"
> >> echo "Try '$0 --help' for more information"
> >> @@ -1391,6 +1397,7 @@ disabled with --disable-FEATURE, default is
> >>enabled if available:
> >> tcmalloc tcmalloc support
> >> jemalloc jemalloc support
> >> replication replication support
> >> + vxhs Veritas HyperScale vDisk backend support
> >>
> >> NOTE: The object files are built at the place where configure is
> >>launched
> >> EOF
> >> @@ -4625,6 +4632,33 @@ if do_cc -nostdlib -Wl,-r -Wl,--no-relax -o
> >>$TMPMO $TMPO; then
> >> fi
> >>
> >> ##########################################
> >> +# Veritas HyperScale block driver VxHS
> >> +# Check if libqnio is installed
> >> +
> >> +if test "$vxhs" != "no" ; then
> >> + cat > $TMPC <<EOF
> >> +#include <stdint.h>
> >> +#include <qnio/qnio_api.h>
> >> +
> >> +void *vxhs_callback;
> >> +
> >> +int main(void) {
> >> + iio_init(vxhs_callback);
> >> + return 0;
> >> +}
> >> +EOF
> >> + vxhs_libs="-lqnio"
> >> + if compile_prog "" "$vxhs_libs" ; then
> >> + vxhs=yes
> >> + else
> >> + if test "$vxhs" = "yes" ; then
> >> + feature_not_found "vxhs block device" "Install libqnio. See
> >>github"
> >> + fi
> >> + vxhs=no
> >> + fi
> >> +fi
> >
> >How is libqnio going to be distributed normally? Should we be checking
> >for
> >a minimum version here to compile against?
>
> Yes, version check is required. We will add the changes in separate patch.
> >
> >> +
> >> +##########################################
> >> # End of CC checks
> >> # After here, no more $cc or $ld runs
> >>
> >> @@ -4990,6 +5024,7 @@ echo "tcmalloc support $tcmalloc"
> >> echo "jemalloc support $jemalloc"
> >> echo "avx2 optimization $avx2_opt"
> >> echo "replication support $replication"
> >> +echo "VxHS block device $vxhs"
> >>
> >> if test "$sdl_too_old" = "yes"; then
> >> echo "-> Your SDL version is too old - please upgrade to have SDL
> >>support"
> >> @@ -5590,6 +5625,12 @@ if test "$pthread_setname_np" = "yes" ; then
> >> echo "CONFIG_PTHREAD_SETNAME_NP=y" >> $config_host_mak
> >> fi
> >>
> >> +if test "$vxhs" = "yes" ; then
> >> + echo "CONFIG_VXHS=y" >> $config_host_mak
> >> + echo "VXHS_CFLAGS=$vxhs_cflags" >> $config_host_mak
> >> + echo "VXHS_LIBS=$vxhs_libs" >> $config_host_mak
> >> +fi
> >> +
> >> if test "$tcg_interpreter" = "yes"; then
> >> QEMU_INCLUDES="-I\$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
> >> elif test "$ARCH" = "sparc64" ; then
> >> --
> >> 2.5.5
> >>
>
>
> >