[Qemu-devel] [PATCH v6 0/2] block/vxhs: Add Veritas HyperScale VxHS block device support

[Qemu-devel] [PATCH v6 0/2] block/vxhs: Add Veritas HyperScale VxHS block device support

[Ashish Mittal]

- Veritas HyperScale block driver in QEMU is designed to provide an accelerated
  IO path from KVM virtual machines to Veritas HyperScale storage service.

- A network IO transfer library that translates block IO from HyperScale block
  driver to a network IO format to send it to Veritas HyperScale storage
  service. This library (libqnio) has been open sourced and is available on
  github here: https://github.com/MittalAshish/libqnio

Ashish Mittal (2):
  block/vxhs.c: Add support for a new block device type called "vxhs"
  block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

 block/Makefile.objs              |   2 +
 block/trace-events               |  21 ++
 block/vxhs.c                     | 689 +++++++++++++++++++++++++++++++++++++++
 configure                        |  41 +++
 qapi/block-core.json             |  21 +-
 tests/qemu-iotests/common        |   6 +
 tests/qemu-iotests/common.config |  13 +
 tests/qemu-iotests/common.filter |   1 +
 tests/qemu-iotests/common.rc     |  19 ++
 9 files changed, 811 insertions(+), 2 deletions(-)
 create mode 100644 block/vxhs.c

-- 
1.8.3.1

[Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Ashish Mittal]

Source code for the qnio library that this code loads can be downloaded from:
https://github.com/MittalAshish/libqnio.git

Sample command line using the 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 the URI syntax:
qemu-img convert -f raw -O raw -n
/var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
---
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   |  21 ++
 block/vxhs.c         | 689 +++++++++++++++++++++++++++++++++++++++++++++++++++
 configure            |  41 +++
 qapi/block-core.json |  21 +-
 5 files changed, 772 insertions(+), 2 deletions(-)
 create mode 100644 block/vxhs.c

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 882c903..efdd5ef 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -112,3 +112,24 @@ 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_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..8913e8f
--- /dev/null
+++ b/block/vxhs.c
@@ -0,0 +1,689 @@
+/*
+ * 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"
+
+typedef struct QNIOLibState {
+    int refcnt;
+    void *context;
+} QNIOLibState;
+
+typedef enum {
+    VDISK_AIO_READ,
+    VDISK_AIO_WRITE,
+    VDISK_STAT
+} VDISKAIOCmd;
+
+/*
+ * HyperScale AIO callbacks structure
+ */
+typedef struct VXHSAIOCB {
+    BlockAIOCB common;
+    int err;
+    int direction; /* IO direction (r/w) */
+    size_t io_offset;
+    size_t size;
+    QEMUIOVector *qiov;
+} 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];
+    int event_reader_pos;
+    VXHSAIOCB *qnio_event_acb;
+    VXHSvDiskHostsInfo vdisk_hostinfo; /* Per host info */
+    char *vdisk_guid;
+} BDRVVXHSState;
+
+/* QNIO Library State */
+static QNIOLibState qniolib;
+
+/* vdisk prefix to pass to qnio */
+static const char vdisk_prefix[] = "/dev/of/vdisk";
+
+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;
+}
+
+/*
+ * Initialize QNIO library on first open.
+ */
+static int vxhs_qnio_open(void)
+{
+    int ret = 0;
+
+    if (qniolib.refcnt != 0) {
+        g_assert(qniolib.context != NULL);
+        qniolib.refcnt++;
+        return 0;
+    }
+    qniolib.context = iio_init(QNIO_VERSION, vxhs_iio_callback);
+    if (qniolib.context == NULL) {
+        ret = -ENODEV;
+    } else {
+        qniolib.refcnt = 1;
+    }
+    return ret;
+}
+
+/*
+ * Cleanup QNIO library on last close.
+ */
+static void vxhs_qnio_close(void)
+{
+    qniolib.refcnt--;
+    if (qniolib.refcnt == 0) {
+        iio_fini(qniolib.context);
+        qniolib.context = NULL;
+    }
+}
+
+static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr,
+                              int *rfd, const char *file_name)
+{
+    int ret = 0;
+    bool qnio_open = false;
+
+    ret = vxhs_qnio_open();
+    if (ret) {
+        return ret;
+    }
+    qnio_open = true;
+
+    /*
+     * Open qnio channel to storage agent if not opened before.
+     */
+    *cfd = iio_open(qniolib.context, of_vsa_addr, 0);
+    if (*cfd < 0) {
+        trace_vxhs_qnio_iio_open(of_vsa_addr);
+        ret = -ENODEV;
+        goto err_out;
+    }
+
+    /*
+     * Open vdisk device
+     */
+    *rfd = iio_devopen(qniolib.context, *cfd, file_name, 0);
+    if (*rfd < 0) {
+        trace_vxhs_qnio_iio_devopen(file_name);
+        ret = -ENODEV;
+        goto err_out;
+    }
+    return 0;
+
+err_out:
+    if (*cfd >= 0) {
+        iio_close(qniolib.context, *cfd);
+    }
+    if (qnio_open) {
+        vxhs_qnio_close();
+    }
+    *cfd = -1;
+    *rfd = -1;
+    return ret;
+}
+
+static void vxhs_qnio_iio_close(BDRVVXHSState *s)
+{
+    /*
+     * Close vDisk device
+     */
+    if (s->vdisk_hostinfo.vdisk_rfd >= 0) {
+        iio_devclose(qniolib.context, 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(qniolib.context, s->vdisk_hostinfo.qnio_cfd);
+        s->vdisk_hostinfo.qnio_cfd = -1;
+    }
+
+    vxhs_qnio_close();
+}
+
+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);
+    }
+
+    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);
+}
+
+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".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);
+
+    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;
+    const char *server_host_opt;
+    char *file_name = NULL;
+    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) {
+        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);
+
+    str = g_strdup_printf(VXHS_OPT_SERVER".");
+    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);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    server_host_opt = qemu_opt_get(tcp_opts, VXHS_OPT_HOST);
+    if (!server_host_opt) {
+        error_setg(&local_err, QERR_MISSING_PARAMETER,
+                   VXHS_OPT_SERVER"."VXHS_OPT_HOST);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    s->vdisk_hostinfo.hostip = g_strdup(server_host_opt);
+
+    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);
+
+    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);
+
+    ret = vxhs_qnio_iio_open(cfd, of_vsa_addr, rfd, file_name);
+    if (ret) {
+        error_setg(&local_err, "Failed qnio_iio_open");
+        ret = -EIO;
+    }
+
+out:
+    g_free(str);
+    g_free(file_name);
+    g_free(of_vsa_addr);
+    qemu_opts_del(opts);
+
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+        g_free(s->vdisk_hostinfo.hostip);
+        g_free(s->vdisk_guid);
+        s->vdisk_guid = NULL;
+        errno = -ret;
+    }
+
+    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 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;
+    }
+
+    s->vdisk_hostinfo.qnio_cfd = qemu_qnio_cfd;
+    s->vdisk_hostinfo.vdisk_rfd = qemu_rfd;
+
+    /*
+     * 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
+     */
+    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;
+    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->qiov = qiov;
+    acb->direction = iodir;
+
+    iio_flags = (IIO_FLAG_DONE | IIO_FLAG_ASYNC);
+
+    switch (iodir) {
+    case VDISK_AIO_WRITE:
+            ret = iio_writev(qniolib.context, rfd, qiov->iov, qiov->niov,
+                             offset, (uint64_t)size, (void *)acb, iio_flags);
+            break;
+    case VDISK_AIO_READ:
+            ret = iio_readv(qniolib.context, 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 int64_t vxhs_get_vdisk_stat(BDRVVXHSState *s)
+{
+    int64_t vdisk_size = -1;
+    int ret = 0;
+    uint32_t rfd = s->vdisk_hostinfo.vdisk_rfd;
+
+    ret = iio_ioctl(qniolib.context, rfd, IOR_VDISK_STAT, &vdisk_size, NULL, 0);
+    if (ret < 0) {
+        trace_vxhs_get_vdisk_stat_err(s->vdisk_guid, ret, errno);
+        return -EIO;
+    }
+
+    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;
+
+    vdisk_size = vxhs_get_vdisk_stat(s);
+    if (vdisk_size < 0) {
+        return -EIO;
+    }
+
+    return vdisk_size;
+}
+
+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_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 fd6f898..9f1e9cd 100755
--- a/configure
+++ b/configure
@@ -322,6 +322,7 @@ numa=""
 tcmalloc="no"
 jemalloc="no"
 replication="yes"
+vxhs=""
 
 # parse CC options first
 for opt do
@@ -1167,6 +1168,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"
@@ -1400,6 +1406,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
@@ -4721,6 +4728,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(QNIO_VERSION, 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
+
+##########################################
 # End of CC checks
 # After here, no more $cc or $ld runs
 
@@ -5087,6 +5121,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"
@@ -5703,6 +5738,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
diff --git a/qapi/block-core.json b/qapi/block-core.json
index bcd3b9e..bd9729c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1715,6 +1715,7 @@
 # @host_device, @host_cdrom: Since 2.1
 # @gluster: Since 2.7
 # @nbd, @nfs, @replication, @ssh: Since 2.8
+# @vxhs: Since 2.9
 #
 # Since: 2.0
 ##
@@ -1724,7 +1725,7 @@
             'host_device', 'http', 'https', 'luks', 'nbd', 'nfs', 'null-aio',
             'null-co', 'parallels', 'qcow', 'qcow2', 'qed', 'quorum', 'raw',
             'replication', 'ssh', 'tftp', 'vdi', 'vhdx', 'vmdk', 'vpc',
-            'vvfat' ] }
+            'vvfat','vxhs' ] }
 
 ##
 # @BlockdevOptionsFile
@@ -2352,6 +2353,21 @@
   'data': { '*offset': 'int', '*size': 'int' } }
 
 ##
+# @BlockdevOptionsVxHS
+#
+# Driver specific block device options for VxHS
+#
+# @vdisk-id:    UUID of VxHS volume
+#
+# @server:      vxhs server IP, port
+#
+# Since: 2.9
+##
+{ 'struct': 'BlockdevOptionsVxHS',
+  'data': { 'vdisk-id': 'str',
+            'server': 'InetSocketAddress' } }
+
+##
 # @BlockdevOptions
 #
 # Options for creating a block device.  Many options are available for all
@@ -2415,7 +2431,8 @@
       'vhdx':       'BlockdevOptionsGenericFormat',
       'vmdk':       'BlockdevOptionsGenericCOWFormat',
       'vpc':        'BlockdevOptionsGenericFormat',
-      'vvfat':      'BlockdevOptionsVVFAT'
+      'vvfat':      'BlockdevOptionsVVFAT',
+      'vxhs':       'BlockdevOptionsVxHS'
   } }
 
 ##
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 8652 bytes --]

On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
> Source code for the qnio library that this code loads can be downloaded from:
> https://github.com/MittalAshish/libqnio.git
> 
> Sample command line using the 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 the URI syntax:
> qemu-img convert -f raw -O raw -n
> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0
> 
> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> ---
> 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   |  21 ++
>  block/vxhs.c         | 689 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure            |  41 +++
>  qapi/block-core.json |  21 +-
>  5 files changed, 772 insertions(+), 2 deletions(-)
>  create mode 100644 block/vxhs.c
> 
> 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 882c903..efdd5ef 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -112,3 +112,24 @@ 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_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..8913e8f
> --- /dev/null
> +++ b/block/vxhs.c
> @@ -0,0 +1,689 @@
> +/*
> + * 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>

Please move system headers (<>) above user headers ("").  This way you
can be sure the system header isn't affected by any macros defined by
user headers.

> +#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"

Is this header file needed?

> +
> +#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"
> +
> +typedef struct QNIOLibState {
> +    int refcnt;
> +    void *context;
> +} QNIOLibState;
> +
> +typedef enum {
> +    VDISK_AIO_READ,
> +    VDISK_AIO_WRITE,
> +    VDISK_STAT

This is unused.

> +} VDISKAIOCmd;

This typedef is unused but the VDISK_AIO_READ/VDISK_AIO_WRITE enum
constants are used.  Please use the typedef name instead of int.  That
way it's clear that the only valid values are the VDISK_* enum
constants.

> +
> +/*
> + * HyperScale AIO callbacks structure
> + */
> +typedef struct VXHSAIOCB {
> +    BlockAIOCB common;
> +    int err;
> +    int direction; /* IO direction (r/w) */

This field is unused.

> +    size_t io_offset;

This field is unused.

> +    size_t size;

This field is unused.

> +    QEMUIOVector *qiov;
> +} VXHSAIOCB;
> +
> +typedef struct VXHSvDiskHostsInfo {
> +    int qnio_cfd; /* Channel FD */
> +    int vdisk_rfd; /* vDisk remote FD */

Please don't call things FDs if they are not FDs.  This is confusing
because dup(), close(), etc don't work on them.  They are handles.

Handles are an unnecessary layer of indirection in the first place.
libqnio would be simpler if it returned opaque pointers to structs
instead.  This way hash/map lookups can be eliminated.

Instead of storing strings in the hash/map, define structs with useful
fields.  This eliminates some of the string parsing in libqnio.

> +    char *hostip; /* Host's IP addresses */

Is this strictly an IP address?  If a hostname can be used too then
"host" would be clearer name.

> +    int port; /* Host's port number */
> +} VXHSvDiskHostsInfo;
> +
> +/*
> + * Structure per vDisk maintained for state
> + */
> +typedef struct BDRVVXHSState {
> +    int fds[2];
> +    int event_reader_pos;

Why is a pipe still being used?  Back in August I mentioned that this
approach isn't the best practice anymore.  It's more code and slower
than QEMUBH.

You didn't respond to my review comment.  Feel free to disagree with my
comments but please respond so I know what to expect.  Now I'm wondering
whether other comments have been ignored too...

> +static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr,
> +                              int *rfd, const char *file_name)
> +{
> +    int ret = 0;
> +    bool qnio_open = false;

This variable isn't necessary since the vxhs_qnio_open() error uses
return instead of goto.


Pausing review at this point because I realized that my previous review
comments weren't addressed.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Mon, 11/14 15:07, Stefan Hajnoczi wrote:
> On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
> > diff --git a/block/vxhs.c b/block/vxhs.c
> > new file mode 100644
> > index 0000000..8913e8f
> > --- /dev/null
> > +++ b/block/vxhs.c
> > @@ -0,0 +1,689 @@
> > +/*
> > + * 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>
> 
> Please move system headers (<>) above user headers ("").  This way you
> can be sure the system header isn't affected by any macros defined by
> user headers.

Yes, but still after "qemu/osdep.h", which prepares necessary bits for any other
headers.

Fam

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1360 bytes --]

On Mon, Nov 14, 2016 at 11:49:06PM +0800, Fam Zheng wrote:
> On Mon, 11/14 15:07, Stefan Hajnoczi wrote:
> > On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
> > > diff --git a/block/vxhs.c b/block/vxhs.c
> > > new file mode 100644
> > > index 0000000..8913e8f
> > > --- /dev/null
> > > +++ b/block/vxhs.c
> > > @@ -0,0 +1,689 @@
> > > +/*
> > > + * 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>
> > 
> > Please move system headers (<>) above user headers ("").  This way you
> > can be sure the system header isn't affected by any macros defined by
> > user headers.
> 
> Yes, but still after "qemu/osdep.h", which prepares necessary bits for any other
> headers.

I disagree.  qnio_api.h is a third-party library that doesn't need QEMU
headers to fix up the environment for it.

By including osdep.h first you mask bugs in qnio_api.h.  Perhaps
qnio_api.h forgot to include a header and we won't notice because
osdep.h happened to bring in those headers first...

Can you explain the rationale for your statement?

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Mon, 11/14 16:50, Stefan Hajnoczi wrote:
> On Mon, Nov 14, 2016 at 11:49:06PM +0800, Fam Zheng wrote:
> > On Mon, 11/14 15:07, Stefan Hajnoczi wrote:
> > > On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
> > > > diff --git a/block/vxhs.c b/block/vxhs.c
> > > > new file mode 100644
> > > > index 0000000..8913e8f
> > > > --- /dev/null
> > > > +++ b/block/vxhs.c
> > > > @@ -0,0 +1,689 @@
> > > > +/*
> > > > + * 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>
> > > 
> > > Please move system headers (<>) above user headers ("").  This way you
> > > can be sure the system header isn't affected by any macros defined by
> > > user headers.
> > 
> > Yes, but still after "qemu/osdep.h", which prepares necessary bits for any other
> > headers.
> 
> I disagree.  qnio_api.h is a third-party library that doesn't need QEMU
> headers to fix up the environment for it.
> 
> By including osdep.h first you mask bugs in qnio_api.h.  Perhaps
> qnio_api.h forgot to include a header and we won't notice because
> osdep.h happened to bring in those headers first...
> 
> Can you explain the rationale for your statement?

I point this out just because I rememebr this effort happened not long ago,
which is to make osdep.h always included first (there is also a
./scripts/clean-includes to reorder the include):

https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01110.html

I think it is mostly for uncommon compilers that should have little to do with
libqnio in particular, but this is a common practice of current QEMU.

Fam

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1894 bytes --]

On 11/14/2016 12:03 PM, Fam Zheng wrote:

>>>> Please move system headers (<>) above user headers ("").  This way you
>>>> can be sure the system header isn't affected by any macros defined by
>>>> user headers.
>>>
>>> Yes, but still after "qemu/osdep.h", which prepares necessary bits for any other
>>> headers.
>>
>> I disagree.  qnio_api.h is a third-party library that doesn't need QEMU
>> headers to fix up the environment for it.
>>
>> By including osdep.h first you mask bugs in qnio_api.h.  Perhaps
>> qnio_api.h forgot to include a header and we won't notice because
>> osdep.h happened to bring in those headers first...
>>
>> Can you explain the rationale for your statement?
> 
> I point this out just because I rememebr this effort happened not long ago,
> which is to make osdep.h always included first (there is also a
> ./scripts/clean-includes to reorder the include):
> 
> https://lists.nongnu.org/archive/html/qemu-devel/2015-12/msg01110.html
> 
> I think it is mostly for uncommon compilers that should have little to do with
> libqnio in particular, but this is a common practice of current QEMU.

If the file is copied in verbatim from a third-party source, then it
should not be including osdep.h, and should be added to the list of
exempt files in scripts/clean-includes - at which point the file SHOULD
be clean because it should already be usable as-is in its third-party
original location.

If we modify the file as part of including it in qemu, then qemu rules
apply and having osdep.h first is good practice.

So I guess you have to determine if libqnio is something that should
compile completely independent from qemu, or whether it is so closely
tied to the rest of qemu that it should follow qemu conventions.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Mon, 11/14 13:06, Eric Blake wrote:
> So I guess you have to determine if libqnio is something that should
> compile completely independent from qemu, or whether it is so closely
> tied to the rest of qemu that it should follow qemu conventions.

The question is on include directives in block/vxhs.c, not libnqio library
header, so qemu conventions apply.

Fam

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Tue, Nov 15, 2016 at 10:04:05AM +0800, Fam Zheng wrote:
> On Mon, 11/14 13:06, Eric Blake wrote:
> > So I guess you have to determine if libqnio is something that should
> > compile completely independent from qemu, or whether it is so closely
> > tied to the rest of qemu that it should follow qemu conventions.
> 
> The question is on include directives in block/vxhs.c, not libnqio library
> header, so qemu conventions apply.

Eric: The libqnio library header is not copied into the QEMU source
tree.  It is an external library dependency like libnfs or libglfs.

Fam, Markus: Unfortunately neither the clean-includes script nor its
patch series cover letter explains *why* osdep.h should be included
before system headers.

The libqnio header is self-contained (i.e. you can #include it and it
has no dependencies) and only used by vxhs.c.  Why is it a good idea to
include qemu/osdep.h first?

Seems like a bad idea to me because it masks missing dependencies in the
libqnio header.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Tue, 11/15 10:18, Stefan Hajnoczi wrote:
> Fam, Markus: Unfortunately neither the clean-includes script nor its
> patch series cover letter explains *why* osdep.h should be included
> before system headers.

I don't know Peter's exact intention either, but AFAICT it is about the few
quirks in osdep.h:


/* Older versions of C++ don't get definitions of various macros from
 * stdlib.h unless we define these macros before first inclusion of
 * that system header.
 */
#ifndef __STDC_CONSTANT_MACROS
#define __STDC_CONSTANT_MACROS
#endif
#ifndef __STDC_LIMIT_MACROS
#define __STDC_LIMIT_MACROS
#endif
#ifndef __STDC_FORMAT_MACROS
#define __STDC_FORMAT_MACROS
#endif

/* The following block of code temporarily renames the daemon() function so the
 * compiler does not see the warning associated with it in stdlib.h on OSX
 */
#ifdef __APPLE__
#define daemon qemu_fake_daemon_function
#include <stdlib.h>
#undef daemon
extern int daemon(int, int);
#endif

<...>


/* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
 * the wrong type. Our replacement isn't usable in preprocessor
 * expressions, but it is sufficient for our needs. */
#if defined(HAVE_BROKEN_SIZE_MAX) && HAVE_BROKEN_SIZE_MAX
#undef SIZE_MAX
#define SIZE_MAX ((size_t)-1)
#endif

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On Tue, Nov 15, 2016 at 08:44:17PM +0800, Fam Zheng wrote:
> On Tue, 11/15 10:18, Stefan Hajnoczi wrote:
> > Fam, Markus: Unfortunately neither the clean-includes script nor its
> > patch series cover letter explains *why* osdep.h should be included
> > before system headers.
> 
> I don't know Peter's exact intention either, but AFAICT it is about the few
> quirks in osdep.h:
> 
> 
> /* Older versions of C++ don't get definitions of various macros from
>  * stdlib.h unless we define these macros before first inclusion of
>  * that system header.
>  */
> #ifndef __STDC_CONSTANT_MACROS
> #define __STDC_CONSTANT_MACROS
> #endif
> #ifndef __STDC_LIMIT_MACROS
> #define __STDC_LIMIT_MACROS
> #endif
> #ifndef __STDC_FORMAT_MACROS
> #define __STDC_FORMAT_MACROS
> #endif
> 
> /* The following block of code temporarily renames the daemon() function so the
>  * compiler does not see the warning associated with it in stdlib.h on OSX
>  */
> #ifdef __APPLE__
> #define daemon qemu_fake_daemon_function
> #include <stdlib.h>
> #undef daemon
> extern int daemon(int, int);
> #endif
> 
> <...>
> 
> 
> /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
>  * the wrong type. Our replacement isn't usable in preprocessor
>  * expressions, but it is sufficient for our needs. */
> #if defined(HAVE_BROKEN_SIZE_MAX) && HAVE_BROKEN_SIZE_MAX
> #undef SIZE_MAX
> #define SIZE_MAX ((size_t)-1)
> #endif

This is exactly the kind of stuff we should not be doing to the libqnio
header file!

Those redefinitions are useful for QEMU code.  They should not be done
to third-party system headers though.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Tue, 11/15 14:45, Stefan Hajnoczi wrote:
> On Tue, Nov 15, 2016 at 08:44:17PM +0800, Fam Zheng wrote:
> > On Tue, 11/15 10:18, Stefan Hajnoczi wrote:
> > > Fam, Markus: Unfortunately neither the clean-includes script nor its
> > > patch series cover letter explains *why* osdep.h should be included
> > > before system headers.
> > 
> > I don't know Peter's exact intention either, but AFAICT it is about the few
> > quirks in osdep.h:
> > 
> > 
> > /* Older versions of C++ don't get definitions of various macros from
> >  * stdlib.h unless we define these macros before first inclusion of
> >  * that system header.
> >  */
> > #ifndef __STDC_CONSTANT_MACROS
> > #define __STDC_CONSTANT_MACROS
> > #endif
> > #ifndef __STDC_LIMIT_MACROS
> > #define __STDC_LIMIT_MACROS
> > #endif
> > #ifndef __STDC_FORMAT_MACROS
> > #define __STDC_FORMAT_MACROS
> > #endif
> > 
> > /* The following block of code temporarily renames the daemon() function so the
> >  * compiler does not see the warning associated with it in stdlib.h on OSX
> >  */
> > #ifdef __APPLE__
> > #define daemon qemu_fake_daemon_function
> > #include <stdlib.h>
> > #undef daemon
> > extern int daemon(int, int);
> > #endif
> > 
> > <...>
> > 
> > 
> > /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
> >  * the wrong type. Our replacement isn't usable in preprocessor
> >  * expressions, but it is sufficient for our needs. */
> > #if defined(HAVE_BROKEN_SIZE_MAX) && HAVE_BROKEN_SIZE_MAX
> > #undef SIZE_MAX
> > #define SIZE_MAX ((size_t)-1)
> > #endif
> 
> This is exactly the kind of stuff we should not be doing to the libqnio
> header file!
> 
> Those redefinitions are useful for QEMU code.  They should not be done
> to third-party system headers though.

But we still want to include osdep.h before libqnio pulls in stdint.h. If we
don't make osdep.h the very first, that cannot be guaranteed.

Fam

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2263 bytes --]

On Tue, Nov 15, 2016 at 11:00:16PM +0800, Fam Zheng wrote:
> On Tue, 11/15 14:45, Stefan Hajnoczi wrote:
> > On Tue, Nov 15, 2016 at 08:44:17PM +0800, Fam Zheng wrote:
> > > On Tue, 11/15 10:18, Stefan Hajnoczi wrote:
> > > > Fam, Markus: Unfortunately neither the clean-includes script nor its
> > > > patch series cover letter explains *why* osdep.h should be included
> > > > before system headers.
> > > 
> > > I don't know Peter's exact intention either, but AFAICT it is about the few
> > > quirks in osdep.h:
> > > 
> > > 
> > > /* Older versions of C++ don't get definitions of various macros from
> > >  * stdlib.h unless we define these macros before first inclusion of
> > >  * that system header.
> > >  */
> > > #ifndef __STDC_CONSTANT_MACROS
> > > #define __STDC_CONSTANT_MACROS
> > > #endif
> > > #ifndef __STDC_LIMIT_MACROS
> > > #define __STDC_LIMIT_MACROS
> > > #endif
> > > #ifndef __STDC_FORMAT_MACROS
> > > #define __STDC_FORMAT_MACROS
> > > #endif
> > > 
> > > /* The following block of code temporarily renames the daemon() function so the
> > >  * compiler does not see the warning associated with it in stdlib.h on OSX
> > >  */
> > > #ifdef __APPLE__
> > > #define daemon qemu_fake_daemon_function
> > > #include <stdlib.h>
> > > #undef daemon
> > > extern int daemon(int, int);
> > > #endif
> > > 
> > > <...>
> > > 
> > > 
> > > /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
> > >  * the wrong type. Our replacement isn't usable in preprocessor
> > >  * expressions, but it is sufficient for our needs. */
> > > #if defined(HAVE_BROKEN_SIZE_MAX) && HAVE_BROKEN_SIZE_MAX
> > > #undef SIZE_MAX
> > > #define SIZE_MAX ((size_t)-1)
> > > #endif
> > 
> > This is exactly the kind of stuff we should not be doing to the libqnio
> > header file!
> > 
> > Those redefinitions are useful for QEMU code.  They should not be done
> > to third-party system headers though.
> 
> But we still want to include osdep.h before libqnio pulls in stdint.h. If we
> don't make osdep.h the very first, that cannot be guaranteed.

Thanks, that makes sense!

I'll add a comment to clean-includes with this explanation of why
osdep.h must go before system headers.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

Thanks for concluding on this.

I will rearrange the qnio_api.h header accordingly as follows:

+#include "qemu/osdep.h"
+#include <qnio/qnio_api.h>   <=== after osdep.h
+#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/error-report.h"  <==== remove


On Tue, Nov 15, 2016 at 11:20 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Nov 15, 2016 at 11:00:16PM +0800, Fam Zheng wrote:
>> On Tue, 11/15 14:45, Stefan Hajnoczi wrote:
>> > On Tue, Nov 15, 2016 at 08:44:17PM +0800, Fam Zheng wrote:
>> > > On Tue, 11/15 10:18, Stefan Hajnoczi wrote:
>> > > > Fam, Markus: Unfortunately neither the clean-includes script nor its
>> > > > patch series cover letter explains *why* osdep.h should be included
>> > > > before system headers.
>> > >
>> > > I don't know Peter's exact intention either, but AFAICT it is about the few
>> > > quirks in osdep.h:
>> > >
>> > >
>> > > /* Older versions of C++ don't get definitions of various macros from
>> > >  * stdlib.h unless we define these macros before first inclusion of
>> > >  * that system header.
>> > >  */
>> > > #ifndef __STDC_CONSTANT_MACROS
>> > > #define __STDC_CONSTANT_MACROS
>> > > #endif
>> > > #ifndef __STDC_LIMIT_MACROS
>> > > #define __STDC_LIMIT_MACROS
>> > > #endif
>> > > #ifndef __STDC_FORMAT_MACROS
>> > > #define __STDC_FORMAT_MACROS
>> > > #endif
>> > >
>> > > /* The following block of code temporarily renames the daemon() function so the
>> > >  * compiler does not see the warning associated with it in stdlib.h on OSX
>> > >  */
>> > > #ifdef __APPLE__
>> > > #define daemon qemu_fake_daemon_function
>> > > #include <stdlib.h>
>> > > #undef daemon
>> > > extern int daemon(int, int);
>> > > #endif
>> > >
>> > > <...>
>> > >
>> > >
>> > > /* Mac OSX has a <stdint.h> bug that incorrectly defines SIZE_MAX with
>> > >  * the wrong type. Our replacement isn't usable in preprocessor
>> > >  * expressions, but it is sufficient for our needs. */
>> > > #if defined(HAVE_BROKEN_SIZE_MAX) && HAVE_BROKEN_SIZE_MAX
>> > > #undef SIZE_MAX
>> > > #define SIZE_MAX ((size_t)-1)
>> > > #endif
>> >
>> > This is exactly the kind of stuff we should not be doing to the libqnio
>> > header file!
>> >
>> > Those redefinitions are useful for QEMU code.  They should not be done
>> > to third-party system headers though.
>>
>> But we still want to include osdep.h before libqnio pulls in stdint.h. If we
>> don't make osdep.h the very first, that cannot be guaranteed.
>
> Thanks, that makes sense!
>
> I'll add a comment to clean-includes with this explanation of why
> osdep.h must go before system headers.
>
> Stefan

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

On Tue, Nov 15, 2016 at 8:39 PM, ashish mittal <ashmit602@gmail.com> wrote:
> Thanks for concluding on this.
>
> I will rearrange the qnio_api.h header accordingly as follows:
>
> +#include "qemu/osdep.h"
> +#include <qnio/qnio_api.h>   <=== after osdep.h
> +#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/error-report.h"  <==== remove

Looks good.

Stefan

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Markus Armbruster]

ashish mittal <ashmit602@gmail.com> writes:

> Thanks for concluding on this.
>
> I will rearrange the qnio_api.h header accordingly as follows:
>
> +#include "qemu/osdep.h"

Headers should not include osdep.h.

> +#include <qnio/qnio_api.h>   <=== after osdep.h
> +#include "block/block_int.h"

Including block_int.h in a header is problematic.  Are you sure you need
it?  Will qnio/qnio_api.h ever be included outside block/?

> +#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"  <==== remove

In general, headers should include what they need, but no more.

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Fam Zheng]

On Wed, 11/16 10:04, Markus Armbruster wrote:
> ashish mittal <ashmit602@gmail.com> writes:
> 
> > Thanks for concluding on this.
> >
> > I will rearrange the qnio_api.h header accordingly as follows:
> >
> > +#include "qemu/osdep.h"
> 
> Headers should not include osdep.h.

This is about including "osdep.h" _and_ "qnio_api.h" in block/vxhs.c, so what
Ashish means looks good to me.

Fam

> 
> > +#include <qnio/qnio_api.h>   <=== after osdep.h
> > +#include "block/block_int.h"
> 
> Including block_int.h in a header is problematic.  Are you sure you need
> it?  Will qnio/qnio_api.h ever be included outside block/?
> 
> > +#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"  <==== remove
> 
> In general, headers should include what they need, but no more.
> 

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

On Wed, Nov 16, 2016 at 9:49 AM, Fam Zheng <famz@redhat.com> wrote:
> On Wed, 11/16 10:04, Markus Armbruster wrote:
>> ashish mittal <ashmit602@gmail.com> writes:
>>
>> > Thanks for concluding on this.
>> >
>> > I will rearrange the qnio_api.h header accordingly as follows:
>> >
>> > +#include "qemu/osdep.h"
>>
>> Headers should not include osdep.h.
>
> This is about including "osdep.h" _and_ "qnio_api.h" in block/vxhs.c, so what
> Ashish means looks good to me.

Yes, I think "will rearrange the qnio_api.h header" was a typo and was
supposed to be block/vxhs.c.

Stefan

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

On Wed, Nov 16, 2016 at 3:27 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Wed, Nov 16, 2016 at 9:49 AM, Fam Zheng <famz@redhat.com> wrote:
>> On Wed, 11/16 10:04, Markus Armbruster wrote:
>>> ashish mittal <ashmit602@gmail.com> writes:
>>>
>>> > Thanks for concluding on this.
>>> >
>>> > I will rearrange the qnio_api.h header accordingly as follows:
>>> >
>>> > +#include "qemu/osdep.h"
>>>
>>> Headers should not include osdep.h.
>>
>> This is about including "osdep.h" _and_ "qnio_api.h" in block/vxhs.c, so what
>> Ashish means looks good to me.
>
> Yes, I think "will rearrange the qnio_api.h header" was a typo and was
> supposed to be block/vxhs.c.
>
> Stefan

Thanks for the correction. Yes, i meant rearrange headers in block/vxhs.c.

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

Hi,

There has been some work going on on the VxHS libvirt patches and we
are making good progress.

A suggestion has been made on the libvirt community to check if we can
change the qemu VxHS device specification syntax as follows.

Replace

(a) +file.server.host=192.168.0.1,file.server.port=9999

with

(b) +file.host=192.168.0.1,file.port=9999

The reasoning being that since we have only one host (true as the
failover is now being handled completely/transparently) within the
libqnio library), the "server" part is redundant.

Excerpt from John Ferlan's email -
/======================================/
#2. Is the desire to ever support more than 1 host? If not, then is the
"server" syntax you've borrowed from the Gluster code necessary? Could
you just go with the single "host" like NBD and SSH. As it relates to
the qemu command line - I'm not quite as clear. From the example I see
in commit id '7b7da9e28', the gluster syntax would have:

+file.server.0.type=tcp,file.server.0.host=example.org,file.server.0.port=6000,\
+file.server.1.type=tcp,file.server.1.host=example.org,file.server.1.port=24007,\
+file.server.2.type=unix,file.server.2.socket=/path/to/sock,format=qcow2,\

whereas, the VxHS syntax is:
 +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\

FWIW: I also note there is no ".type=tcp" in your output - so perhaps
the "default" is tcp unless otherwise specified, but I'm sure of the
qemu syntax requirements in this area. I assume that since there's only
1 server, the ".0, .1, .2" become unnecessary (something added by commit
id 'f1bbc7df4' for multiple gluster hosts).

I haven't closedly followed the qemu syntax discussion, but it would it
would be possible to use:

+file.host=192.168.0.1,file.port=9999

Similar to how NBD (see commit id 'a1674fd9') and SSH (see commit id
'bc225b1b5') are handled.
/======================================/

If this proposal looks OK to the community, then I will make this user
interface change in the next VxHS qemu patch.

Thanks,
Ashish

On Wed, Nov 16, 2016 at 9:05 AM, ashish mittal <ashmit602@gmail.com> wrote:
> On Wed, Nov 16, 2016 at 3:27 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Wed, Nov 16, 2016 at 9:49 AM, Fam Zheng <famz@redhat.com> wrote:
>>> On Wed, 11/16 10:04, Markus Armbruster wrote:
>>>> ashish mittal <ashmit602@gmail.com> writes:
>>>>
>>>> > Thanks for concluding on this.
>>>> >
>>>> > I will rearrange the qnio_api.h header accordingly as follows:
>>>> >
>>>> > +#include "qemu/osdep.h"
>>>>
>>>> Headers should not include osdep.h.
>>>
>>> This is about including "osdep.h" _and_ "qnio_api.h" in block/vxhs.c, so what
>>> Ashish means looks good to me.
>>
>> Yes, I think "will rearrange the qnio_api.h header" was a typo and was
>> supposed to be block/vxhs.c.
>>
>> Stefan
>
> Thanks for the correction. Yes, i meant rearrange headers in block/vxhs.c.

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

On Tue, Jan 31, 2017 at 05:55:47PM -0800, ashish mittal wrote:
> Hi,
> 
> There has been some work going on on the VxHS libvirt patches and we
> are making good progress.
> 
> A suggestion has been made on the libvirt community to check if we can
> change the qemu VxHS device specification syntax as follows.
> 
> Replace
> 
> (a) +file.server.host=192.168.0.1,file.server.port=9999
> 
> with
> 
> (b) +file.host=192.168.0.1,file.port=9999
> 
> The reasoning being that since we have only one host (true as the
> failover is now being handled completely/transparently) within the
> libqnio library), the "server" part is redundant.

Sounds good to me.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

We have retained the original syntax in v7 per the following
suggestion on libvirt thread -

>> That is correct. Above syntax would also work for us. I will pose this
>> suggestion to the qemu community and update with their response.
>>

It's not that important... I was looking for a simplification and
generation of only what's required. You can continue using the server
syntax - perhaps just leave a note/comment in the code indicating the
decision point and move on.


On Thu, Feb 2, 2017 at 2:08 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 31, 2017 at 05:55:47PM -0800, ashish mittal wrote:
>> Hi,
>>
>> There has been some work going on on the VxHS libvirt patches and we
>> are making good progress.
>>
>> A suggestion has been made on the libvirt community to check if we can
>> change the qemu VxHS device specification syntax as follows.
>>
>> Replace
>>
>> (a) +file.server.host=192.168.0.1,file.server.port=9999
>>
>> with
>>
>> (b) +file.host=192.168.0.1,file.port=9999
>>
>> The reasoning being that since we have only one host (true as the
>> failover is now being handled completely/transparently) within the
>> libqnio library), the "server" part is redundant.
>
> Sounds good to me.
>
> Stefan

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On 11/15/2016 04:18 AM, Stefan Hajnoczi wrote:
> On Tue, Nov 15, 2016 at 10:04:05AM +0800, Fam Zheng wrote:
>> On Mon, 11/14 13:06, Eric Blake wrote:
>>> So I guess you have to determine if libqnio is something that should
>>> compile completely independent from qemu, or whether it is so closely
>>> tied to the rest of qemu that it should follow qemu conventions.
>>
>> The question is on include directives in block/vxhs.c, not libnqio library
>> header, so qemu conventions apply.
> 
> Eric: The libqnio library header is not copied into the QEMU source
> tree.  It is an external library dependency like libnfs or libglfs.
> 
> Fam, Markus: Unfortunately neither the clean-includes script nor its
> patch series cover letter explains *why* osdep.h should be included
> before system headers.

For the same reason that <config.h> should be included first before
system headers in an autoconf'd project. Many platforms ship
(semi-)broken system headers, where configure detects AND works around
the problems, but only if the workaround CONSISTENTLY happens before any
system header is included.  It is a LOT harder to track down compilation
or link or even subtle runtime failures if you don't consistently use
your workaround all the time, such that some .c files got the workaround
but others did not.

> The libqnio header is self-contained (i.e. you can #include it and it
> has no dependencies) and only used by vxhs.c.  Why is it a good idea to
> include qemu/osdep.h first?
> 
> Seems like a bad idea to me because it masks missing dependencies in the
> libqnio header.

If the libqnio header is completely independent, then it should not be
part of the qemu source tree (with "" inclusion), but should instead be
externally included (with <> inclusion), just like any other third-party
library we link against.  But even then, we STILL want our osdep.h to
occur before ANY third-party library, so that we have a CONSISTENT
environment, which is why we mandate that osdep.h be included first in
all qemu .c files.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Markus Armbruster]

Fam Zheng <famz@redhat.com> writes:

> On Mon, 11/14 15:07, Stefan Hajnoczi wrote:
>> On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
>> > diff --git a/block/vxhs.c b/block/vxhs.c
>> > new file mode 100644
>> > index 0000000..8913e8f
>> > --- /dev/null
>> > +++ b/block/vxhs.c
>> > @@ -0,0 +1,689 @@
>> > +/*
>> > + * 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>
>> 
>> Please move system headers (<>) above user headers ("").  This way you
>> can be sure the system header isn't affected by any macros defined by
>> user headers.
>
> Yes, but still after "qemu/osdep.h", which prepares necessary bits for any other
> headers.

Yes, osdep.h must come first.  See also scripts/clean-includes.

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

On Mon, Nov 14, 2016 at 7:07 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
>> Source code for the qnio library that this code loads can be downloaded from:
>> https://github.com/MittalAshish/libqnio.git
>>
>> Sample command line using the 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 the URI syntax:
>> qemu-img convert -f raw -O raw -n
>> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>> vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>>
>> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>> ---
>> 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   |  21 ++
>>  block/vxhs.c         | 689 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  configure            |  41 +++
>>  qapi/block-core.json |  21 +-
>>  5 files changed, 772 insertions(+), 2 deletions(-)
>>  create mode 100644 block/vxhs.c
>>
>> 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 882c903..efdd5ef 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -112,3 +112,24 @@ 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_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..8913e8f
>> --- /dev/null
>> +++ b/block/vxhs.c
>> @@ -0,0 +1,689 @@
>> +/*
>> + * 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>
>
> Please move system headers (<>) above user headers ("").  This way you
> can be sure the system header isn't affected by any macros defined by
> user headers.
>
>> +#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"
>
> Is this header file needed?
>
>> +
>> +#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"
>> +
>> +typedef struct QNIOLibState {
>> +    int refcnt;
>> +    void *context;
>> +} QNIOLibState;
>> +
>> +typedef enum {
>> +    VDISK_AIO_READ,
>> +    VDISK_AIO_WRITE,
>> +    VDISK_STAT
>
> This is unused.
>
>> +} VDISKAIOCmd;
>
> This typedef is unused but the VDISK_AIO_READ/VDISK_AIO_WRITE enum
> constants are used.  Please use the typedef name instead of int.  That
> way it's clear that the only valid values are the VDISK_* enum
> constants.
>
>> +
>> +/*
>> + * HyperScale AIO callbacks structure
>> + */
>> +typedef struct VXHSAIOCB {
>> +    BlockAIOCB common;
>> +    int err;
>> +    int direction; /* IO direction (r/w) */
>
> This field is unused.
>
>> +    size_t io_offset;
>
> This field is unused.
>
>> +    size_t size;
>
> This field is unused.
>
>> +    QEMUIOVector *qiov;
>> +} VXHSAIOCB;
>> +
>> +typedef struct VXHSvDiskHostsInfo {
>> +    int qnio_cfd; /* Channel FD */
>> +    int vdisk_rfd; /* vDisk remote FD */
>
> Please don't call things FDs if they are not FDs.  This is confusing
> because dup(), close(), etc don't work on them.  They are handles.
>
> Handles are an unnecessary layer of indirection in the first place.
> libqnio would be simpler if it returned opaque pointers to structs
> instead.  This way hash/map lookups can be eliminated.
>
> Instead of storing strings in the hash/map, define structs with useful
> fields.  This eliminates some of the string parsing in libqnio.
>
>> +    char *hostip; /* Host's IP addresses */
>
> Is this strictly an IP address?  If a hostname can be used too then
> "host" would be clearer name.
>
>> +    int port; /* Host's port number */
>> +} VXHSvDiskHostsInfo;
>> +
>> +/*
>> + * Structure per vDisk maintained for state
>> + */
>> +typedef struct BDRVVXHSState {
>> +    int fds[2];
>> +    int event_reader_pos;
>
> Why is a pipe still being used?  Back in August I mentioned that this
> approach isn't the best practice anymore.  It's more code and slower
> than QEMUBH.
>
> You didn't respond to my review comment.  Feel free to disagree with my
> comments but please respond so I know what to expect.  Now I'm wondering
> whether other comments have been ignored too...
>

I did respond, also resent the same email today in case it was missed
the first time. Please let me know if you did not receive the one I
forwarded today and I will check if there's a problem at my end.
Here's the text again:
/+++++++++++++++++/
On Tue, Aug 23, 2016 at 3:22 PM, ashish mittal <ashmit602@gmail.com> wrote:
> Thanks Stefan, I will look at block/quorum.c.
>
> I have sent V4 of the patch with a reworked parsing logic for both
> JSON and URI. Both of them are quite compact now.
>
> URI parsing now follows the suggestion given by Kevin.
> /================/
> However, you should use the proper interfaces to implement this, which
> is .bdrv_parse_filename(). This is a function that gets a string and
> converts it into a QDict, which is then passed to .bdrv_open(). So it
> uses exactly the same code path in .bdrv_open() as if used directly with
> QAPI.
> /================/
>
> Additionally, I have fixed all the issues pointed out by you on V1 of
> the patch. The only change I haven't done is to replace pipes with
> QEMUBH. I am hoping this will not hold up the patch from being
> accepted, and I can make this transition later with proper dev and
> testing.
>
> Regards,
> Ashish
/+++++++++++++++++/

Will work on all the other suggestions in this email and the one you
pointed out earlier. Thanks!


>> +static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr,
>> +                              int *rfd, const char *file_name)
>> +{
>> +    int ret = 0;
>> +    bool qnio_open = false;
>
> This variable isn't necessary since the vxhs_qnio_open() error uses
> return instead of goto.
>
>
> Pausing review at this point because I realized that my previous review
> comments weren't addressed.

Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Daniel P. Berrange]

On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote:
> Source code for the qnio library that this code loads can be downloaded from:
> https://github.com/MittalAshish/libqnio.git
> 
> Sample command line using the 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 the URI syntax:
> qemu-img convert -f raw -O raw -n
> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0

I'm wondering what the security story is here.  QEMU is connecting to a
remote TCP server but apparently not providing any username or password
or other form of credentials to authenticate itself with.

This seems to imply that the network server accepts connections to access
disks from any client anywhere. Surely this isn't correct, and there must
be some authentication scheme in there, to prevent the server being wide
open to any malicious attacker, whether on the network, or via a compromised
QEMU process accessing volumes it shouldn't ?

If there is authentication, then how is QEMU providing the credentials ?

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

[Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Ashish Mittal]

These changes use a vxhs test server that is a part of the following
repository:
https://github.com/MittalAshish/libqnio.git

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
---
v6 changelog:
(1) Added iotests for VxHS block device.

 tests/qemu-iotests/common        |  6 ++++++
 tests/qemu-iotests/common.config | 13 +++++++++++++
 tests/qemu-iotests/common.filter |  1 +
 tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
 4 files changed, 39 insertions(+)

diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
index d60ea2c..41430d8 100644
--- a/tests/qemu-iotests/common
+++ b/tests/qemu-iotests/common
@@ -158,6 +158,7 @@ check options
     -nfs                test nfs
     -archipelago        test archipelago
     -luks               test luks
+    -vxhs               test vxhs
     -xdiff              graphical mode diff
     -nocache            use O_DIRECT on backing file
     -misalign           misalign memory allocations
@@ -261,6 +262,11 @@ testlist options
             xpand=false
             ;;
 
+        -vxhs)
+            IMGPROTO=vxhs
+            xpand=false
+            ;;
+
         -ssh)
             IMGPROTO=ssh
             xpand=false
diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
index f6384fb..c7a80c0 100644
--- a/tests/qemu-iotests/common.config
+++ b/tests/qemu-iotests/common.config
@@ -105,6 +105,10 @@ if [ -z "$QEMU_NBD_PROG" ]; then
     export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
 fi
 
+if [ -z "$QEMU_VXHS_PROG" ]; then
+    export QEMU_VXHS_PROG="`set_prog_path qnio_server /usr/local/bin`"
+fi
+
 _qemu_wrapper()
 {
     (
@@ -156,10 +160,19 @@ _qemu_nbd_wrapper()
     )
 }
 
+_qemu_vxhs_wrapper()
+{
+    (
+        echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
+        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
+    )
+}
+
 export QEMU=_qemu_wrapper
 export QEMU_IMG=_qemu_img_wrapper
 export QEMU_IO=_qemu_io_wrapper
 export QEMU_NBD=_qemu_nbd_wrapper
+export QEMU_VXHS=_qemu_vxhs_wrapper
 
 QEMU_IMG_EXTRA_ARGS=
 if [ "$IMGOPTSSYNTAX" = "true" ]; then
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index 240ed06..a8a4d0e 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -123,6 +123,7 @@ _filter_img_info()
         -e "s#$TEST_DIR#TEST_DIR#g" \
         -e "s#$IMGFMT#IMGFMT#g" \
         -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
+        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
         -e "/encrypted: yes/d" \
         -e "/cluster_size: [0-9]\\+/d" \
         -e "/table_size: [0-9]\\+/d" \
diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
index 3213765..06a3164 100644
--- a/tests/qemu-iotests/common.rc
+++ b/tests/qemu-iotests/common.rc
@@ -89,6 +89,9 @@ else
         TEST_IMG=$TEST_DIR/t.$IMGFMT
     elif [ "$IMGPROTO" = "archipelago" ]; then
         TEST_IMG="archipelago:at.$IMGFMT"
+    elif [ "$IMGPROTO" = "vxhs" ]; then
+        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
+        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
     else
         TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
     fi
@@ -175,6 +178,12 @@ _make_test_img()
         eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
         sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
     fi
+
+    # Start QNIO server on image directory for vxhs protocol
+    if [ $IMGPROTO = "vxhs" ]; then
+        eval "$QEMU_VXHS -d  $TEST_DIR &"
+        sleep 1 # Wait for server to come up.
+    fi
 }
 
 _rm_test_img()
@@ -201,6 +210,16 @@ _cleanup_test_img()
             fi
             rm -f "$TEST_IMG_FILE"
             ;;
+        vxhs)
+            if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
+                local QEMU_VXHS_PID
+                read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
+                kill ${QEMU_VXHS_PID} >/dev/null 2>&1
+                rm -f "${TEST_DIR}/qemu-vxhs.pid"
+            fi
+            rm -f "$TEST_IMG_FILE"
+            ;;
+
         file)
             _rm_test_img "$TEST_DIR/t.$IMGFMT"
             _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> These changes use a vxhs test server that is a part of the following
> repository:
> https://github.com/MittalAshish/libqnio.git
> 
> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> ---
> v6 changelog:
> (1) Added iotests for VxHS block device.
> 
>  tests/qemu-iotests/common        |  6 ++++++
>  tests/qemu-iotests/common.config | 13 +++++++++++++
>  tests/qemu-iotests/common.filter |  1 +
>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>  4 files changed, 39 insertions(+)
> 
> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> index d60ea2c..41430d8 100644
> --- a/tests/qemu-iotests/common
> +++ b/tests/qemu-iotests/common

When using raw format, I was able to run the test successfully for all
supported test cases (26 of them).

With qcow2, they fail - but not the fault of this patch, I think; but
rather, the fault of the test server.  Can qnio_server be modified so that
it does not work on just raw files?



> @@ -158,6 +158,7 @@ check options
>      -nfs                test nfs
>      -archipelago        test archipelago
>      -luks               test luks
> +    -vxhs               test vxhs
>      -xdiff              graphical mode diff
>      -nocache            use O_DIRECT on backing file
>      -misalign           misalign memory allocations
> @@ -261,6 +262,11 @@ testlist options
>              xpand=false
>              ;;
>  
> +        -vxhs)
> +            IMGPROTO=vxhs
> +            xpand=false
> +            ;;
> +
>          -ssh)
>              IMGPROTO=ssh
>              xpand=false
> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
> index f6384fb..c7a80c0 100644
> --- a/tests/qemu-iotests/common.config
> +++ b/tests/qemu-iotests/common.config
> @@ -105,6 +105,10 @@ if [ -z "$QEMU_NBD_PROG" ]; then
>      export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
>  fi
>  
> +if [ -z "$QEMU_VXHS_PROG" ]; then
> +    export QEMU_VXHS_PROG="`set_prog_path qnio_server /usr/local/bin`"
> +fi
> +
>  _qemu_wrapper()
>  {
>      (
> @@ -156,10 +160,19 @@ _qemu_nbd_wrapper()
>      )
>  }
>  
> +_qemu_vxhs_wrapper()
> +{
> +    (
> +        echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
> +        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
> +    )
> +}
> +
>  export QEMU=_qemu_wrapper
>  export QEMU_IMG=_qemu_img_wrapper
>  export QEMU_IO=_qemu_io_wrapper
>  export QEMU_NBD=_qemu_nbd_wrapper
> +export QEMU_VXHS=_qemu_vxhs_wrapper
>  
>  QEMU_IMG_EXTRA_ARGS=
>  if [ "$IMGOPTSSYNTAX" = "true" ]; then
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 240ed06..a8a4d0e 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -123,6 +123,7 @@ _filter_img_info()
>          -e "s#$TEST_DIR#TEST_DIR#g" \
>          -e "s#$IMGFMT#IMGFMT#g" \
>          -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
> +        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
>          -e "/encrypted: yes/d" \
>          -e "/cluster_size: [0-9]\\+/d" \
>          -e "/table_size: [0-9]\\+/d" \
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 3213765..06a3164 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -89,6 +89,9 @@ else
>          TEST_IMG=$TEST_DIR/t.$IMGFMT
>      elif [ "$IMGPROTO" = "archipelago" ]; then
>          TEST_IMG="archipelago:at.$IMGFMT"
> +    elif [ "$IMGPROTO" = "vxhs" ]; then
> +        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
> +        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
>      else
>          TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
>      fi
> @@ -175,6 +178,12 @@ _make_test_img()
>          eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
>          sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
>      fi
> +
> +    # Start QNIO server on image directory for vxhs protocol
> +    if [ $IMGPROTO = "vxhs" ]; then
> +        eval "$QEMU_VXHS -d  $TEST_DIR &"
> +        sleep 1 # Wait for server to come up.
> +    fi
>  }
>  
>  _rm_test_img()
> @@ -201,6 +210,16 @@ _cleanup_test_img()
>              fi
>              rm -f "$TEST_IMG_FILE"
>              ;;
> +        vxhs)
> +            if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
> +                local QEMU_VXHS_PID
> +                read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
> +                kill ${QEMU_VXHS_PID} >/dev/null 2>&1
> +                rm -f "${TEST_DIR}/qemu-vxhs.pid"
> +            fi
> +            rm -f "$TEST_IMG_FILE"
> +            ;;
> +
>          file)
>              _rm_test_img "$TEST_DIR/t.$IMGFMT"
>              _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[ashish mittal]

I plan to submit v7 soon, therefore going over any unanswered emails
at this time.

On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
> On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
>> These changes use a vxhs test server that is a part of the following
>> repository:
>> https://github.com/MittalAshish/libqnio.git
>>
>> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>> ---
>> v6 changelog:
>> (1) Added iotests for VxHS block device.
>>
>>  tests/qemu-iotests/common        |  6 ++++++
>>  tests/qemu-iotests/common.config | 13 +++++++++++++
>>  tests/qemu-iotests/common.filter |  1 +
>>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>>  4 files changed, 39 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
>> index d60ea2c..41430d8 100644
>> --- a/tests/qemu-iotests/common
>> +++ b/tests/qemu-iotests/common
>
> When using raw format, I was able to run the test successfully for all
> supported test cases (26 of them).
>
> With qcow2, they fail - but not the fault of this patch, I think; but
> rather, the fault of the test server.  Can qnio_server be modified so that
> it does not work on just raw files?
>
>

VxHS supports and uses only the raw format.

>
>> @@ -158,6 +158,7 @@ check options
>>      -nfs                test nfs
>>      -archipelago        test archipelago
>>      -luks               test luks
>> +    -vxhs               test vxhs
>>      -xdiff              graphical mode diff
>>      -nocache            use O_DIRECT on backing file
>>      -misalign           misalign memory allocations
>> @@ -261,6 +262,11 @@ testlist options
>>              xpand=false
>>              ;;
>>
>> +        -vxhs)
>> +            IMGPROTO=vxhs
>> +            xpand=false
>> +            ;;
>> +
>>          -ssh)
>>              IMGPROTO=ssh
>>              xpand=false
>> diff --git a/tests/qemu-iotests/common.config b/tests/qemu-iotests/common.config
>> index f6384fb..c7a80c0 100644
>> --- a/tests/qemu-iotests/common.config
>> +++ b/tests/qemu-iotests/common.config
>> @@ -105,6 +105,10 @@ if [ -z "$QEMU_NBD_PROG" ]; then
>>      export QEMU_NBD_PROG="`set_prog_path qemu-nbd`"
>>  fi
>>
>> +if [ -z "$QEMU_VXHS_PROG" ]; then
>> +    export QEMU_VXHS_PROG="`set_prog_path qnio_server /usr/local/bin`"
>> +fi
>> +
>>  _qemu_wrapper()
>>  {
>>      (
>> @@ -156,10 +160,19 @@ _qemu_nbd_wrapper()
>>      )
>>  }
>>
>> +_qemu_vxhs_wrapper()
>> +{
>> +    (
>> +        echo $BASHPID > "${TEST_DIR}/qemu-vxhs.pid"
>> +        exec "$QEMU_VXHS_PROG" $QEMU_VXHS_OPTIONS "$@"
>> +    )
>> +}
>> +
>>  export QEMU=_qemu_wrapper
>>  export QEMU_IMG=_qemu_img_wrapper
>>  export QEMU_IO=_qemu_io_wrapper
>>  export QEMU_NBD=_qemu_nbd_wrapper
>> +export QEMU_VXHS=_qemu_vxhs_wrapper
>>
>>  QEMU_IMG_EXTRA_ARGS=
>>  if [ "$IMGOPTSSYNTAX" = "true" ]; then
>> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
>> index 240ed06..a8a4d0e 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -123,6 +123,7 @@ _filter_img_info()
>>          -e "s#$TEST_DIR#TEST_DIR#g" \
>>          -e "s#$IMGFMT#IMGFMT#g" \
>>          -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
>> +        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
>>          -e "/encrypted: yes/d" \
>>          -e "/cluster_size: [0-9]\\+/d" \
>>          -e "/table_size: [0-9]\\+/d" \
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 3213765..06a3164 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -89,6 +89,9 @@ else
>>          TEST_IMG=$TEST_DIR/t.$IMGFMT
>>      elif [ "$IMGPROTO" = "archipelago" ]; then
>>          TEST_IMG="archipelago:at.$IMGFMT"
>> +    elif [ "$IMGPROTO" = "vxhs" ]; then
>> +        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
>> +        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
>>      else
>>          TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
>>      fi
>> @@ -175,6 +178,12 @@ _make_test_img()
>>          eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
>>          sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
>>      fi
>> +
>> +    # Start QNIO server on image directory for vxhs protocol
>> +    if [ $IMGPROTO = "vxhs" ]; then
>> +        eval "$QEMU_VXHS -d  $TEST_DIR &"
>> +        sleep 1 # Wait for server to come up.
>> +    fi
>>  }
>>
>>  _rm_test_img()
>> @@ -201,6 +210,16 @@ _cleanup_test_img()
>>              fi
>>              rm -f "$TEST_IMG_FILE"
>>              ;;
>> +        vxhs)
>> +            if [ -f "${TEST_DIR}/qemu-vxhs.pid" ]; then
>> +                local QEMU_VXHS_PID
>> +                read QEMU_VXHS_PID < "${TEST_DIR}/qemu-vxhs.pid"
>> +                kill ${QEMU_VXHS_PID} >/dev/null 2>&1
>> +                rm -f "${TEST_DIR}/qemu-vxhs.pid"
>> +            fi
>> +            rm -f "$TEST_IMG_FILE"
>> +            ;;
>> +
>>          file)
>>              _rm_test_img "$TEST_DIR/t.$IMGFMT"
>>              _rm_test_img "$TEST_DIR/t.$IMGFMT.orig"
>> --
>> 1.8.3.1
>>

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1858 bytes --]

On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
> On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
> > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> >> These changes use a vxhs test server that is a part of the following
> >> repository:
> >> https://github.com/MittalAshish/libqnio.git
> >>
> >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> >> ---
> >> v6 changelog:
> >> (1) Added iotests for VxHS block device.
> >>
> >>  tests/qemu-iotests/common        |  6 ++++++
> >>  tests/qemu-iotests/common.config | 13 +++++++++++++
> >>  tests/qemu-iotests/common.filter |  1 +
> >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
> >>  4 files changed, 39 insertions(+)
> >>
> >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> >> index d60ea2c..41430d8 100644
> >> --- a/tests/qemu-iotests/common
> >> +++ b/tests/qemu-iotests/common
> >
> > When using raw format, I was able to run the test successfully for all
> > supported test cases (26 of them).
> >
> > With qcow2, they fail - but not the fault of this patch, I think; but
> > rather, the fault of the test server.  Can qnio_server be modified so that
> > it does not work on just raw files?
> >
> >
> 
> VxHS supports and uses only the raw format.

That's like saying only ext4 guest file systems are supported on VxHS
and not ZFS.  The VxHS driver should not care what file system is used,
it just does block I/O without interpreting the data.

It must be possible to use any format on top of the VxHS protocol.
After all, the image format drivers just do block I/O.  If there is a
case where qcow2 on VxHS fails then it needs to be investigated.

The VxHS driver can't be merged until we at least understand the cause
of the qcow2 test failures.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
> On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
> > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
> > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> > >> These changes use a vxhs test server that is a part of the following
> > >> repository:
> > >> https://github.com/MittalAshish/libqnio.git
> > >>
> > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> > >> ---
> > >> v6 changelog:
> > >> (1) Added iotests for VxHS block device.
> > >>
> > >>  tests/qemu-iotests/common        |  6 ++++++
> > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
> > >>  tests/qemu-iotests/common.filter |  1 +
> > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
> > >>  4 files changed, 39 insertions(+)
> > >>
> > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> > >> index d60ea2c..41430d8 100644
> > >> --- a/tests/qemu-iotests/common
> > >> +++ b/tests/qemu-iotests/common
> > >
> > > When using raw format, I was able to run the test successfully for all
> > > supported test cases (26 of them).
> > >
> > > With qcow2, they fail - but not the fault of this patch, I think; but
> > > rather, the fault of the test server.  Can qnio_server be modified so that
> > > it does not work on just raw files?
> > >
> > >
> > 
> > VxHS supports and uses only the raw format.
> 
> That's like saying only ext4 guest file systems are supported on VxHS
> and not ZFS.  The VxHS driver should not care what file system is used,
> it just does block I/O without interpreting the data.
> 
> It must be possible to use any format on top of the VxHS protocol.
> After all, the image format drivers just do block I/O.  If there is a
> case where qcow2 on VxHS fails then it needs to be investigated.
> 
> The VxHS driver can't be merged until we at least understand the cause
> of the qcow2 test failures.
>

A quick run with the test server and a QEMU process showed an abort() in the
test server, so I just sent a pull req to libqnio to fix that.  

But playing with it in gdb right now with a test qcow2 file, I see that we
are waiting in aio_poll() forever for the test server to respond to a read
request, when using qcow2 format.  

As Stefan said, this doesn't really make any sense - why would VXHS behave
differently based on the file contents?

Jeff

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Ketan Nilangekar]

On 2/13/17, 8:32 AM, "Jeff Cody" <jcody@redhat.com> wrote:

    On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
    > On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
    > > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
    > > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
    > > >> These changes use a vxhs test server that is a part of the following
    > > >> repository:
    > > >> https://github.com/MittalAshish/libqnio.git
    > > >>
    > > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
    > > >> ---
    > > >> v6 changelog:
    > > >> (1) Added iotests for VxHS block device.
    > > >>
    > > >>  tests/qemu-iotests/common        |  6 ++++++
    > > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
    > > >>  tests/qemu-iotests/common.filter |  1 +
    > > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
    > > >>  4 files changed, 39 insertions(+)
    > > >>
    > > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
    > > >> index d60ea2c..41430d8 100644
    > > >> --- a/tests/qemu-iotests/common
    > > >> +++ b/tests/qemu-iotests/common
    > > >
    > > > When using raw format, I was able to run the test successfully for all
    > > > supported test cases (26 of them).
    > > >
    > > > With qcow2, they fail - but not the fault of this patch, I think; but
    > > > rather, the fault of the test server.  Can qnio_server be modified so that
    > > > it does not work on just raw files?
    > > >
    > > >
    > > 
    > > VxHS supports and uses only the raw format.
    > 
    > That's like saying only ext4 guest file systems are supported on VxHS
    > and not ZFS.  The VxHS driver should not care what file system is used,
    > it just does block I/O without interpreting the data.
    > 
    > It must be possible to use any format on top of the VxHS protocol.
    > After all, the image format drivers just do block I/O.  If there is a
    > case where qcow2 on VxHS fails then it needs to be investigated.
    > 
    > The VxHS driver can't be merged until we at least understand the cause
    > of the qcow2 test failures.
    >
    
    A quick run with the test server and a QEMU process showed an abort() in the
    test server, so I just sent a pull req to libqnio to fix that.  
    
    But playing with it in gdb right now with a test qcow2 file, I see that we
    are waiting in aio_poll() forever for the test server to respond to a read
    request, when using qcow2 format.  
    
    As Stefan said, this doesn't really make any sense - why would VXHS behave
    differently based on the file contents?

[Ketan] 
To read/write a qcow2 backed device VxHS server implementation will need to understand the qcow2 format. This is not just block IO but actually does involve interpreting the qcow2 header and cluster formats. Clearly the test server implementation does not handle it as it was never intended to. VxHS backend won't handle it either because VxHS virtual disks are written as non-sparse files.
There are space savings with the qcow2 format but performance penalties as well because of the metadata overhead. As a block storage provider, VxHS does not support sparse file formats like qcow2 primarily because of performance reasons.
Implementing a qcow2 backend in the test server would be a non-trivial and non-useful exercise since the VxHS server won't support it.

   

 Jeff
    

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Mon, Feb 13, 2017 at 10:36:53PM +0000, Ketan Nilangekar wrote:
> 
> 
> On 2/13/17, 8:32 AM, "Jeff Cody" <jcody@redhat.com> wrote:
> 
>     On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
>     > On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
>     > > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
>     > > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
>     > > >> These changes use a vxhs test server that is a part of the following
>     > > >> repository:
>     > > >> https://github.com/MittalAshish/libqnio.git
>     > > >>
>     > > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>     > > >> ---
>     > > >> v6 changelog:
>     > > >> (1) Added iotests for VxHS block device.
>     > > >>
>     > > >>  tests/qemu-iotests/common        |  6 ++++++
>     > > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
>     > > >>  tests/qemu-iotests/common.filter |  1 +
>     > > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>     > > >>  4 files changed, 39 insertions(+)
>     > > >>
>     > > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
>     > > >> index d60ea2c..41430d8 100644
>     > > >> --- a/tests/qemu-iotests/common
>     > > >> +++ b/tests/qemu-iotests/common
>     > > >
>     > > > When using raw format, I was able to run the test successfully for all
>     > > > supported test cases (26 of them).
>     > > >
>     > > > With qcow2, they fail - but not the fault of this patch, I think; but
>     > > > rather, the fault of the test server.  Can qnio_server be modified so that
>     > > > it does not work on just raw files?
>     > > >
>     > > >
>     > > 
>     > > VxHS supports and uses only the raw format.
>     > 
>     > That's like saying only ext4 guest file systems are supported on VxHS
>     > and not ZFS.  The VxHS driver should not care what file system is used,
>     > it just does block I/O without interpreting the data.
>     > 
>     > It must be possible to use any format on top of the VxHS protocol.
>     > After all, the image format drivers just do block I/O.  If there is a
>     > case where qcow2 on VxHS fails then it needs to be investigated.
>     > 
>     > The VxHS driver can't be merged until we at least understand the cause
>     > of the qcow2 test failures.
>     >
>     
>     A quick run with the test server and a QEMU process showed an abort() in the
>     test server, so I just sent a pull req to libqnio to fix that.  
>     
>     But playing with it in gdb right now with a test qcow2 file, I see that we
>     are waiting in aio_poll() forever for the test server to respond to a read
>     request, when using qcow2 format.  
>     
>     As Stefan said, this doesn't really make any sense - why would VXHS behave
>     differently based on the file contents?
> 
> [Ketan] To read/write a qcow2 backed device VxHS server implementation
> will need to understand the qcow2 format. This is not just block IO but
> actually does involve interpreting the qcow2 header and cluster formats.
> Clearly the test server implementation does not handle it as it was never
> intended to. VxHS backend won't handle it either because VxHS virtual
> disks are written as non-sparse files.  There are space savings with the
> qcow2 format but performance penalties as well because of the metadata
> overhead. As a block storage provider, VxHS does not support sparse file
> formats like qcow2 primarily because of performance reasons.  Implementing
> a qcow2 backend in the test server would be a non-trivial and non-useful
> exercise since the VxHS server won't support it.
>

What?  Why would the backend need to know anything about qcow2 formats; are
you manipulating the guest image data directly yourself?  But regardless,
since the test server is naive and just reads and writes data, the fact that
the test server breaks on qcow2 image formats means that the test server is
broken for raw images, as well [1].

The backend should not need to know anything about the image file format in
order to serve data, as the backend is essentially serving bytes. (I guess
the only thing the backend would need to be aware of is how to invoke
qemu-img to create a qcow2 image file initially).


QEMU is what interprets the qcow2 format (or any image format).  Every read
from QEMU will look like a raw file read from the perspective of libqnio /
vxhs.  I can't see any reason why vxhs would need to know anything about the
contents of the image file itself.

The only thing the test server needs to do, in order to be able to serve
qcow2 files correctly, is to be able to serve raw files correctly.

Looking at the libqnio implementation from the test server with gdb, I think
that the reason why qcow2 format does not work is that the current
libqnio implementation does not handle short reads correctly.  For instance,
if the file is not a even multiple of the read size, and we try to read 512
bytes at the end of the file, it appears libqnio hangs on the short read.  I
am not sure if this is a bug exclusive to the test server, or in the libqnio
implementation.

[1]
This bug isn't limited to qcow2, since as mentioned above, QEMU is just
relying on libqnio to read raw bytes.  So, you can reproduce this bug with a
raw image file, as well:

# dd if=/dev/zero of=test.raw bs=1 count=196616

# qemu-io -c "read 196608 512" vxhs://localhost:9999/test.raw

The above qemu-io process will hang in aio_poll, because libqnio never
processes the callback. This is because process_incoming_messages() thinks
it needs a full 512 bytes before it will call the registered callback, even
though there will only ever be 8 bytes read.

The above scenario is also exactly what is happening when I try to use a
qcow2 format; QEMU issues a read for 8 bytes at offset 196608, and the file
is 196616 bytes long.

(The sparse image thing seems shouldn't be an issue - first, the test server
works just fine with sparse raw images... as matter of fact, the
"create_vdisk.sh" script in the libqnio repository does exactly that.  Also,
if you choose to not use sparse images for whatever reason on the backend,
qcow2 does not require it to be sparse, as image files can be created with
the "-o preallocation=full" option).

-Jeff

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Ketan Nilangekar]

On 2/13/17, 3:23 PM, "Jeff Cody" <jcody@redhat.com> wrote:

    On Mon, Feb 13, 2017 at 10:36:53PM +0000, Ketan Nilangekar wrote:
    > 
    > 
    > On 2/13/17, 8:32 AM, "Jeff Cody" <jcody@redhat.com> wrote:
    > 
    >     On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
    >     > On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
    >     > > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
    >     > > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
    >     > > >> These changes use a vxhs test server that is a part of the following
    >     > > >> repository:
    >     > > >> https://github.com/MittalAshish/libqnio.git
    >     > > >>
    >     > > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
    >     > > >> ---
    >     > > >> v6 changelog:
    >     > > >> (1) Added iotests for VxHS block device.
    >     > > >>
    >     > > >>  tests/qemu-iotests/common        |  6 ++++++
    >     > > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
    >     > > >>  tests/qemu-iotests/common.filter |  1 +
    >     > > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
    >     > > >>  4 files changed, 39 insertions(+)
    >     > > >>
    >     > > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
    >     > > >> index d60ea2c..41430d8 100644
    >     > > >> --- a/tests/qemu-iotests/common
    >     > > >> +++ b/tests/qemu-iotests/common
    >     > > >
    >     > > > When using raw format, I was able to run the test successfully for all
    >     > > > supported test cases (26 of them).
    >     > > >
    >     > > > With qcow2, they fail - but not the fault of this patch, I think; but
    >     > > > rather, the fault of the test server.  Can qnio_server be modified so that
    >     > > > it does not work on just raw files?
    >     > > >
    >     > > >
    >     > > 
    >     > > VxHS supports and uses only the raw format.
    >     > 
    >     > That's like saying only ext4 guest file systems are supported on VxHS
    >     > and not ZFS.  The VxHS driver should not care what file system is used,
    >     > it just does block I/O without interpreting the data.
    >     > 
    >     > It must be possible to use any format on top of the VxHS protocol.
    >     > After all, the image format drivers just do block I/O.  If there is a
    >     > case where qcow2 on VxHS fails then it needs to be investigated.
    >     > 
    >     > The VxHS driver can't be merged until we at least understand the cause
    >     > of the qcow2 test failures.
    >     >
    >     
    >     A quick run with the test server and a QEMU process showed an abort() in the
    >     test server, so I just sent a pull req to libqnio to fix that.  
    >     
    >     But playing with it in gdb right now with a test qcow2 file, I see that we
    >     are waiting in aio_poll() forever for the test server to respond to a read
    >     request, when using qcow2 format.  
    >     
    >     As Stefan said, this doesn't really make any sense - why would VXHS behave
    >     differently based on the file contents?
    > 
    > [Ketan] To read/write a qcow2 backed device VxHS server implementation
    > will need to understand the qcow2 format. This is not just block IO but
    > actually does involve interpreting the qcow2 header and cluster formats.
    > Clearly the test server implementation does not handle it as it was never
    > intended to. VxHS backend won't handle it either because VxHS virtual
    > disks are written as non-sparse files.  There are space savings with the
    > qcow2 format but performance penalties as well because of the metadata
    > overhead. As a block storage provider, VxHS does not support sparse file
    > formats like qcow2 primarily because of performance reasons.  Implementing
    > a qcow2 backend in the test server would be a non-trivial and non-useful
    > exercise since the VxHS server won't support it.
    >
    
    What?  Why would the backend need to know anything about qcow2 formats; are
    you manipulating the guest image data directly yourself?  But regardless,
    since the test server is naive and just reads and writes data, the fact that
    the test server breaks on qcow2 image formats means that the test server is
    broken for raw images, as well [1].
    
    The backend should not need to know anything about the image file format in
    order to serve data, as the backend is essentially serving bytes. (I guess
    the only thing the backend would need to be aware of is how to invoke
    qemu-img to create a qcow2 image file initially).
    
    
    QEMU is what interprets the qcow2 format (or any image format).  Every read
    from QEMU will look like a raw file read from the perspective of libqnio /
    vxhs.  I can't see any reason why vxhs would need to know anything about the
    contents of the image file itself.
    
    The only thing the test server needs to do, in order to be able to serve
    qcow2 files correctly, is to be able to serve raw files correctly.
    
    Looking at the libqnio implementation from the test server with gdb, I think
    that the reason why qcow2 format does not work is that the current
    libqnio implementation does not handle short reads correctly.  For instance,
    if the file is not a even multiple of the read size, and we try to read 512
    bytes at the end of the file, it appears libqnio hangs on the short read.  I
    am not sure if this is a bug exclusive to the test server, or in the libqnio
    implementation.
    
    [1]
    This bug isn't limited to qcow2, since as mentioned above, QEMU is just
    relying on libqnio to read raw bytes.  So, you can reproduce this bug with a
    raw image file, as well:
    
    # dd if=/dev/zero of=test.raw bs=1 count=196616
    
    # qemu-io -c "read 196608 512" vxhs://localhost:9999/test.raw
    
    The above qemu-io process will hang in aio_poll, because libqnio never
    processes the callback. This is because process_incoming_messages() thinks
    it needs a full 512 bytes before it will call the registered callback, even
    though there will only ever be 8 bytes read.
    
    The above scenario is also exactly what is happening when I try to use a
    qcow2 format; QEMU issues a read for 8 bytes at offset 196608, and the file
    is 196616 bytes long.
    
    (The sparse image thing seems shouldn't be an issue - first, the test server
    works just fine with sparse raw images... as matter of fact, the
    "create_vdisk.sh" script in the libqnio repository does exactly that.  Also,
    if you choose to not use sparse images for whatever reason on the backend,
    qcow2 does not require it to be sparse, as image files can be created with
    the "-o preallocation=full" option).
    
[Ketan] You are correct. We have identified the problem with test server and have a patch for it which returns zero filled reads to align the read buffers to requested block size. We could also fix this in libvxhs by not expecting/waiting for requested block size in case the reads are partial.
Ashish will upload the patch for the test server fix. Please let us know how the qcow tests go.

    -Jeff
    

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[ashish mittal]

On Mon, Feb 13, 2017 at 7:02 PM, Ketan Nilangekar
<Ketan.Nilangekar@veritas.com> wrote:
>
>
> On 2/13/17, 3:23 PM, "Jeff Cody" <jcody@redhat.com> wrote:
>
>     On Mon, Feb 13, 2017 at 10:36:53PM +0000, Ketan Nilangekar wrote:
>     >
>     >
>     > On 2/13/17, 8:32 AM, "Jeff Cody" <jcody@redhat.com> wrote:
>     >
>     >     On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
>     >     > On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
>     >     > > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
>     >     > > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
>     >     > > >> These changes use a vxhs test server that is a part of the following
>     >     > > >> repository:
>     >     > > >> https://github.com/MittalAshish/libqnio.git
>     >     > > >>
>     >     > > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>     >     > > >> ---
>     >     > > >> v6 changelog:
>     >     > > >> (1) Added iotests for VxHS block device.
>     >     > > >>
>     >     > > >>  tests/qemu-iotests/common        |  6 ++++++
>     >     > > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
>     >     > > >>  tests/qemu-iotests/common.filter |  1 +
>     >     > > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>     >     > > >>  4 files changed, 39 insertions(+)
>     >     > > >>
>     >     > > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
>     >     > > >> index d60ea2c..41430d8 100644
>     >     > > >> --- a/tests/qemu-iotests/common
>     >     > > >> +++ b/tests/qemu-iotests/common
>     >     > > >
>     >     > > > When using raw format, I was able to run the test successfully for all
>     >     > > > supported test cases (26 of them).
>     >     > > >
>     >     > > > With qcow2, they fail - but not the fault of this patch, I think; but
>     >     > > > rather, the fault of the test server.  Can qnio_server be modified so that
>     >     > > > it does not work on just raw files?
>     >     > > >
>     >     > > >
>     >     > >
>     >     > > VxHS supports and uses only the raw format.
>     >     >
>     >     > That's like saying only ext4 guest file systems are supported on VxHS
>     >     > and not ZFS.  The VxHS driver should not care what file system is used,
>     >     > it just does block I/O without interpreting the data.
>     >     >
>     >     > It must be possible to use any format on top of the VxHS protocol.
>     >     > After all, the image format drivers just do block I/O.  If there is a
>     >     > case where qcow2 on VxHS fails then it needs to be investigated.
>     >     >
>     >     > The VxHS driver can't be merged until we at least understand the cause
>     >     > of the qcow2 test failures.
>     >     >
>     >
>     >     A quick run with the test server and a QEMU process showed an abort() in the
>     >     test server, so I just sent a pull req to libqnio to fix that.
>     >
>     >     But playing with it in gdb right now with a test qcow2 file, I see that we
>     >     are waiting in aio_poll() forever for the test server to respond to a read
>     >     request, when using qcow2 format.
>     >
>     >     As Stefan said, this doesn't really make any sense - why would VXHS behave
>     >     differently based on the file contents?
>     >
>     > [Ketan] To read/write a qcow2 backed device VxHS server implementation
>     > will need to understand the qcow2 format. This is not just block IO but
>     > actually does involve interpreting the qcow2 header and cluster formats.
>     > Clearly the test server implementation does not handle it as it was never
>     > intended to. VxHS backend won't handle it either because VxHS virtual
>     > disks are written as non-sparse files.  There are space savings with the
>     > qcow2 format but performance penalties as well because of the metadata
>     > overhead. As a block storage provider, VxHS does not support sparse file
>     > formats like qcow2 primarily because of performance reasons.  Implementing
>     > a qcow2 backend in the test server would be a non-trivial and non-useful
>     > exercise since the VxHS server won't support it.
>     >
>
>     What?  Why would the backend need to know anything about qcow2 formats; are
>     you manipulating the guest image data directly yourself?  But regardless,
>     since the test server is naive and just reads and writes data, the fact that
>     the test server breaks on qcow2 image formats means that the test server is
>     broken for raw images, as well [1].
>
>     The backend should not need to know anything about the image file format in
>     order to serve data, as the backend is essentially serving bytes. (I guess
>     the only thing the backend would need to be aware of is how to invoke
>     qemu-img to create a qcow2 image file initially).
>
>
>     QEMU is what interprets the qcow2 format (or any image format).  Every read
>     from QEMU will look like a raw file read from the perspective of libqnio /
>     vxhs.  I can't see any reason why vxhs would need to know anything about the
>     contents of the image file itself.
>
>     The only thing the test server needs to do, in order to be able to serve
>     qcow2 files correctly, is to be able to serve raw files correctly.
>
>     Looking at the libqnio implementation from the test server with gdb, I think
>     that the reason why qcow2 format does not work is that the current
>     libqnio implementation does not handle short reads correctly.  For instance,
>     if the file is not a even multiple of the read size, and we try to read 512
>     bytes at the end of the file, it appears libqnio hangs on the short read.  I
>     am not sure if this is a bug exclusive to the test server, or in the libqnio
>     implementation.
>
>     [1]
>     This bug isn't limited to qcow2, since as mentioned above, QEMU is just
>     relying on libqnio to read raw bytes.  So, you can reproduce this bug with a
>     raw image file, as well:
>
>     # dd if=/dev/zero of=test.raw bs=1 count=196616
>
>     # qemu-io -c "read 196608 512" vxhs://localhost:9999/test.raw
>
>     The above qemu-io process will hang in aio_poll, because libqnio never
>     processes the callback. This is because process_incoming_messages() thinks
>     it needs a full 512 bytes before it will call the registered callback, even
>     though there will only ever be 8 bytes read.
>
>     The above scenario is also exactly what is happening when I try to use a
>     qcow2 format; QEMU issues a read for 8 bytes at offset 196608, and the file
>     is 196616 bytes long.
>
>     (The sparse image thing seems shouldn't be an issue - first, the test server
>     works just fine with sparse raw images... as matter of fact, the
>     "create_vdisk.sh" script in the libqnio repository does exactly that.  Also,
>     if you choose to not use sparse images for whatever reason on the backend,
>     qcow2 does not require it to be sparse, as image files can be created with
>     the "-o preallocation=full" option).
>
> [Ketan] You are correct. We have identified the problem with test server and have a patch for it which returns zero filled reads to align the read buffers to requested block size. We could also fix this in libvxhs by not expecting/waiting for requested block size in case the reads are partial.
> Ashish will upload the patch for the test server fix. Please let us know how the qcow tests go.
>
>     -Jeff
>
>

Changes per the above suggestion present in branch ashish_securify. Thanks!

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Mon, Feb 13, 2017 at 11:43:17PM -0800, ashish mittal wrote:
> On Mon, Feb 13, 2017 at 7:02 PM, Ketan Nilangekar
> <Ketan.Nilangekar@veritas.com> wrote:
> >
> >
> > On 2/13/17, 3:23 PM, "Jeff Cody" <jcody@redhat.com> wrote:
> >
> >     On Mon, Feb 13, 2017 at 10:36:53PM +0000, Ketan Nilangekar wrote:
> >     >
> >     >
> >     > On 2/13/17, 8:32 AM, "Jeff Cody" <jcody@redhat.com> wrote:
> >     >
> >     >     On Mon, Feb 13, 2017 at 01:37:25PM +0000, Stefan Hajnoczi wrote:
> >     >     > On Tue, Feb 07, 2017 at 03:12:36PM -0800, ashish mittal wrote:
> >     >     > > On Tue, Nov 8, 2016 at 12:44 PM, Jeff Cody <jcody@redhat.com> wrote:
> >     >     > > > On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> >     >     > > >> These changes use a vxhs test server that is a part of the following
> >     >     > > >> repository:
> >     >     > > >> https://github.com/MittalAshish/libqnio.git
> >     >     > > >>
> >     >     > > >> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> >     >     > > >> ---
> >     >     > > >> v6 changelog:
> >     >     > > >> (1) Added iotests for VxHS block device.
> >     >     > > >>
> >     >     > > >>  tests/qemu-iotests/common        |  6 ++++++
> >     >     > > >>  tests/qemu-iotests/common.config | 13 +++++++++++++
> >     >     > > >>  tests/qemu-iotests/common.filter |  1 +
> >     >     > > >>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
> >     >     > > >>  4 files changed, 39 insertions(+)
> >     >     > > >>
> >     >     > > >> diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common
> >     >     > > >> index d60ea2c..41430d8 100644
> >     >     > > >> --- a/tests/qemu-iotests/common
> >     >     > > >> +++ b/tests/qemu-iotests/common
> >     >     > > >
> >     >     > > > When using raw format, I was able to run the test successfully for all
> >     >     > > > supported test cases (26 of them).
> >     >     > > >
> >     >     > > > With qcow2, they fail - but not the fault of this patch, I think; but
> >     >     > > > rather, the fault of the test server.  Can qnio_server be modified so that
> >     >     > > > it does not work on just raw files?
> >     >     > > >
> >     >     > > >
> >     >     > >
> >     >     > > VxHS supports and uses only the raw format.
> >     >     >
> >     >     > That's like saying only ext4 guest file systems are supported on VxHS
> >     >     > and not ZFS.  The VxHS driver should not care what file system is used,
> >     >     > it just does block I/O without interpreting the data.
> >     >     >
> >     >     > It must be possible to use any format on top of the VxHS protocol.
> >     >     > After all, the image format drivers just do block I/O.  If there is a
> >     >     > case where qcow2 on VxHS fails then it needs to be investigated.
> >     >     >
> >     >     > The VxHS driver can't be merged until we at least understand the cause
> >     >     > of the qcow2 test failures.
> >     >     >
> >     >
> >     >     A quick run with the test server and a QEMU process showed an abort() in the
> >     >     test server, so I just sent a pull req to libqnio to fix that.
> >     >
> >     >     But playing with it in gdb right now with a test qcow2 file, I see that we
> >     >     are waiting in aio_poll() forever for the test server to respond to a read
> >     >     request, when using qcow2 format.
> >     >
> >     >     As Stefan said, this doesn't really make any sense - why would VXHS behave
> >     >     differently based on the file contents?
> >     >
> >     > [Ketan] To read/write a qcow2 backed device VxHS server implementation
> >     > will need to understand the qcow2 format. This is not just block IO but
> >     > actually does involve interpreting the qcow2 header and cluster formats.
> >     > Clearly the test server implementation does not handle it as it was never
> >     > intended to. VxHS backend won't handle it either because VxHS virtual
> >     > disks are written as non-sparse files.  There are space savings with the
> >     > qcow2 format but performance penalties as well because of the metadata
> >     > overhead. As a block storage provider, VxHS does not support sparse file
> >     > formats like qcow2 primarily because of performance reasons.  Implementing
> >     > a qcow2 backend in the test server would be a non-trivial and non-useful
> >     > exercise since the VxHS server won't support it.
> >     >
> >
> >     What?  Why would the backend need to know anything about qcow2 formats; are
> >     you manipulating the guest image data directly yourself?  But regardless,
> >     since the test server is naive and just reads and writes data, the fact that
> >     the test server breaks on qcow2 image formats means that the test server is
> >     broken for raw images, as well [1].
> >
> >     The backend should not need to know anything about the image file format in
> >     order to serve data, as the backend is essentially serving bytes. (I guess
> >     the only thing the backend would need to be aware of is how to invoke
> >     qemu-img to create a qcow2 image file initially).
> >
> >
> >     QEMU is what interprets the qcow2 format (or any image format).  Every read
> >     from QEMU will look like a raw file read from the perspective of libqnio /
> >     vxhs.  I can't see any reason why vxhs would need to know anything about the
> >     contents of the image file itself.
> >
> >     The only thing the test server needs to do, in order to be able to serve
> >     qcow2 files correctly, is to be able to serve raw files correctly.
> >
> >     Looking at the libqnio implementation from the test server with gdb, I think
> >     that the reason why qcow2 format does not work is that the current
> >     libqnio implementation does not handle short reads correctly.  For instance,
> >     if the file is not a even multiple of the read size, and we try to read 512
> >     bytes at the end of the file, it appears libqnio hangs on the short read.  I
> >     am not sure if this is a bug exclusive to the test server, or in the libqnio
> >     implementation.
> >
> >     [1]
> >     This bug isn't limited to qcow2, since as mentioned above, QEMU is just
> >     relying on libqnio to read raw bytes.  So, you can reproduce this bug with a
> >     raw image file, as well:
> >
> >     # dd if=/dev/zero of=test.raw bs=1 count=196616
> >
> >     # qemu-io -c "read 196608 512" vxhs://localhost:9999/test.raw
> >
> >     The above qemu-io process will hang in aio_poll, because libqnio never
> >     processes the callback. This is because process_incoming_messages() thinks
> >     it needs a full 512 bytes before it will call the registered callback, even
> >     though there will only ever be 8 bytes read.
> >
> >     The above scenario is also exactly what is happening when I try to use a
> >     qcow2 format; QEMU issues a read for 8 bytes at offset 196608, and the file
> >     is 196616 bytes long.
> >
> >     (The sparse image thing seems shouldn't be an issue - first, the test server
> >     works just fine with sparse raw images... as matter of fact, the
> >     "create_vdisk.sh" script in the libqnio repository does exactly that.  Also,
> >     if you choose to not use sparse images for whatever reason on the backend,
> >     qcow2 does not require it to be sparse, as image files can be created with
> >     the "-o preallocation=full" option).
> >
> > [Ketan] You are correct. We have identified the problem with test server
> > and have a patch for it which returns zero filled reads to align the
> > read buffers to requested block size. We could also fix this in libvxhs
> > by not expecting/waiting for requested block size in case the reads are
> > partial.  Ashish will upload the patch for the test server fix. Please
> > let us know how the qcow tests go.
> >
> >     -Jeff
> >
> >
> 
> Changes per the above suggestion present in branch ashish_securify.
> Thanks!


All the qemu-iotests that I would expect to pass now, with vxhs, seem to
pass (tests that try to create a qemu image through the protocol obviously
fail; I submitted a patch upstream that should make it easier to exclude
those tests).

Once the patches I've submitted upstream are in master, you should be able
to use them like so (feel free to just pull this into your branch and send
it out with your patches if/when the patches I just sent are in master):


>From 7c135439fd0151860cb0f6ef1a857dfbee6e6317 Mon Sep 17 00:00:00 2001
From: Jeff Cody <jcody@redhat.com>
Date: Tue, 14 Feb 2017 09:51:42 -0500
Subject: [PATCH] qemu-iotests: exclude vxhs from image creation via protocol

The protocol VXHS does not support image creation.  Some tests expect
to be able to create images through the protocol.  Exclude VXHS from
these tests.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 tests/qemu-iotests/017 | 1 +
 tests/qemu-iotests/020 | 1 +
 tests/qemu-iotests/029 | 1 +
 tests/qemu-iotests/073 | 1 +
 tests/qemu-iotests/114 | 1 +
 tests/qemu-iotests/130 | 1 +
 tests/qemu-iotests/134 | 1 +
 tests/qemu-iotests/156 | 1 +
 tests/qemu-iotests/158 | 1 +
 9 files changed, 9 insertions(+)

diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
index e3f9e75..4f9302d 100755
--- a/tests/qemu-iotests/017
+++ b/tests/qemu-iotests/017
@@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting backing files
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
 
diff --git a/tests/qemu-iotests/020 b/tests/qemu-iotests/020
index 9c4a68c..7a11110 100755
--- a/tests/qemu-iotests/020
+++ b/tests/qemu-iotests/020
@@ -43,6 +43,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting backing files
 _supported_fmt qcow qcow2 vmdk qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 _unsupported_imgopts "subformat=monolithicFlat" \
                      "subformat=twoGbMaxExtentFlat" \
diff --git a/tests/qemu-iotests/029 b/tests/qemu-iotests/029
index e639ac0..30bab24 100755
--- a/tests/qemu-iotests/029
+++ b/tests/qemu-iotests/029
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 # Any format supporting intenal snapshots
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 # Internal snapshots are (currently) impossible with refcount_bits=1
 _unsupported_imgopts 'refcount_bits=1[^0-9]'
diff --git a/tests/qemu-iotests/073 b/tests/qemu-iotests/073
index ad37a61..40f85b1 100755
--- a/tests/qemu-iotests/073
+++ b/tests/qemu-iotests/073
@@ -39,6 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 CLUSTER_SIZE=64k
diff --git a/tests/qemu-iotests/114 b/tests/qemu-iotests/114
index f110d4f..5b7dc54 100755
--- a/tests/qemu-iotests/114
+++ b/tests/qemu-iotests/114
@@ -39,6 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
diff --git a/tests/qemu-iotests/130 b/tests/qemu-iotests/130
index ecc8a5b..f941fc9 100755
--- a/tests/qemu-iotests/130
+++ b/tests/qemu-iotests/130
@@ -42,6 +42,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 qemu_comm_method="monitor"
diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
index af618b8..acce946 100755
--- a/tests/qemu-iotests/134
+++ b/tests/qemu-iotests/134
@@ -39,6 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
diff --git a/tests/qemu-iotests/156 b/tests/qemu-iotests/156
index cc95ff1..78deaff 100755
--- a/tests/qemu-iotests/156
+++ b/tests/qemu-iotests/156
@@ -48,6 +48,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2 qed
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 # Create source disk
diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
index a6cdd6d..ef8d70f 100755
--- a/tests/qemu-iotests/158
+++ b/tests/qemu-iotests/158
@@ -39,6 +39,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 
 _supported_fmt qcow2
 _supported_proto generic
+_unsupported_proto vxhs
 _supported_os Linux
 
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Daniel P. Berrange]

On Tue, Feb 14, 2017 at 11:35:17AM -0500, Jeff Cody wrote:
> From 7c135439fd0151860cb0f6ef1a857dfbee6e6317 Mon Sep 17 00:00:00 2001
> From: Jeff Cody <jcody@redhat.com>
> Date: Tue, 14 Feb 2017 09:51:42 -0500
> Subject: [PATCH] qemu-iotests: exclude vxhs from image creation via protocol
> 
> The protocol VXHS does not support image creation.  Some tests expect
> to be able to create images through the protocol.  Exclude VXHS from
> these tests.

[snip]

> diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
> index e3f9e75..4f9302d 100755
> --- a/tests/qemu-iotests/017
> +++ b/tests/qemu-iotests/017
> @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
>  # Any format supporting backing files
>  _supported_fmt qcow qcow2 vmdk qed
>  _supported_proto generic
> +_unsupported_proto vxhs
>  _supported_os Linux
>  _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"

I've no objection to your patch as is, just a thought for future
improvements.

There's quite a few block protos that doen't support image
creation. Rather than listing protocols in _unsupported_proto
it would be more scalable if we could list features. eg if
each test had

  _supported_feature imagecreate

Then, the tests/qemu-iotests/common file could set the list
of features supported by each protocol/format. Thus avoiding
the need to update all iotests when adding new protocols. We
could have features for "encryption", and "snapshots" and
"backing_files", etc too

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Tue, Feb 14, 2017 at 04:51:09PM +0000, Daniel P. Berrange wrote:
> On Tue, Feb 14, 2017 at 11:35:17AM -0500, Jeff Cody wrote:
> > From 7c135439fd0151860cb0f6ef1a857dfbee6e6317 Mon Sep 17 00:00:00 2001
> > From: Jeff Cody <jcody@redhat.com>
> > Date: Tue, 14 Feb 2017 09:51:42 -0500
> > Subject: [PATCH] qemu-iotests: exclude vxhs from image creation via protocol
> > 
> > The protocol VXHS does not support image creation.  Some tests expect
> > to be able to create images through the protocol.  Exclude VXHS from
> > these tests.
> 
> [snip]
> 
> > diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017
> > index e3f9e75..4f9302d 100755
> > --- a/tests/qemu-iotests/017
> > +++ b/tests/qemu-iotests/017
> > @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >  # Any format supporting backing files
> >  _supported_fmt qcow qcow2 vmdk qed
> >  _supported_proto generic
> > +_unsupported_proto vxhs
> >  _supported_os Linux
> >  _unsupported_imgopts "subformat=monolithicFlat" "subformat=twoGbMaxExtentFlat"
> 
> I've no objection to your patch as is, just a thought for future
> improvements.
> 
> There's quite a few block protos that doen't support image
> creation. Rather than listing protocols in _unsupported_proto
> it would be more scalable if we could list features. eg if
> each test had
> 
>   _supported_feature imagecreate
> 
> Then, the tests/qemu-iotests/common file could set the list
> of features supported by each protocol/format. Thus avoiding
> the need to update all iotests when adding new protocols. We
> could have features for "encryption", and "snapshots" and
> "backing_files", etc too
>

I like that approach, good idea.

-Jeff

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[Jeff Cody]

On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
> These changes use a vxhs test server that is a part of the following
> repository:
> https://github.com/MittalAshish/libqnio.git
> 
> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
> ---
> v6 changelog:
> (1) Added iotests for VxHS block device.
> 
>  tests/qemu-iotests/common        |  6 ++++++
>  tests/qemu-iotests/common.config | 13 +++++++++++++
>  tests/qemu-iotests/common.filter |  1 +
>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>  4 files changed, 39 insertions(+)
> 

[...]

> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index 240ed06..a8a4d0e 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -123,6 +123,7 @@ _filter_img_info()
>          -e "s#$TEST_DIR#TEST_DIR#g" \
>          -e "s#$IMGFMT#IMGFMT#g" \
>          -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
> +        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
>          -e "/encrypted: yes/d" \
>          -e "/cluster_size: [0-9]\\+/d" \
>          -e "/table_size: [0-9]\\+/d" \
> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
> index 3213765..06a3164 100644
> --- a/tests/qemu-iotests/common.rc
> +++ b/tests/qemu-iotests/common.rc
> @@ -89,6 +89,9 @@ else
>          TEST_IMG=$TEST_DIR/t.$IMGFMT
>      elif [ "$IMGPROTO" = "archipelago" ]; then
>          TEST_IMG="archipelago:at.$IMGFMT"
> +    elif [ "$IMGPROTO" = "vxhs" ]; then
> +        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
> +        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
>      else
>          TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
>      fi
> @@ -175,6 +178,12 @@ _make_test_img()
>          eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
>          sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
>      fi
> +
> +    # Start QNIO server on image directory for vxhs protocol
> +    if [ $IMGPROTO = "vxhs" ]; then
> +        eval "$QEMU_VXHS -d  $TEST_DIR &"

So I spoke too soon about tests passing, but it is not really your fault :)

After rebasing to master, there is a new test 174 that now hangs (and hangs
for nbd, as well).  This is because the test is piping the results of
_make_test_image to sed to filter it.

This line should redirect stdout to /dev/null, so that the pipe does not
need to wait until process completion:

    eval "$QEMU_VXHS -d  $TEST_DIR > /dev/null &"

 

Re: [Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs"

[ashish mittal]

On Tue, Feb 14, 2017 at 10:12 AM, Jeff Cody <jcody@redhat.com> wrote:
> On Mon, Nov 07, 2016 at 04:59:45PM -0800, Ashish Mittal wrote:
>> These changes use a vxhs test server that is a part of the following
>> repository:
>> https://github.com/MittalAshish/libqnio.git
>>
>> Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com>
>> ---
>> v6 changelog:
>> (1) Added iotests for VxHS block device.
>>
>>  tests/qemu-iotests/common        |  6 ++++++
>>  tests/qemu-iotests/common.config | 13 +++++++++++++
>>  tests/qemu-iotests/common.filter |  1 +
>>  tests/qemu-iotests/common.rc     | 19 +++++++++++++++++++
>>  4 files changed, 39 insertions(+)
>>
>
> [...]
>
>> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
>> index 240ed06..a8a4d0e 100644
>> --- a/tests/qemu-iotests/common.filter
>> +++ b/tests/qemu-iotests/common.filter
>> @@ -123,6 +123,7 @@ _filter_img_info()
>>          -e "s#$TEST_DIR#TEST_DIR#g" \
>>          -e "s#$IMGFMT#IMGFMT#g" \
>>          -e 's#nbd://127.0.0.1:10810$#TEST_DIR/t.IMGFMT#g' \
>> +        -e 's#json.*vdisk-id.*vxhs"}}#TEST_DIR/t.IMGFMT#' \
>>          -e "/encrypted: yes/d" \
>>          -e "/cluster_size: [0-9]\\+/d" \
>>          -e "/table_size: [0-9]\\+/d" \
>> diff --git a/tests/qemu-iotests/common.rc b/tests/qemu-iotests/common.rc
>> index 3213765..06a3164 100644
>> --- a/tests/qemu-iotests/common.rc
>> +++ b/tests/qemu-iotests/common.rc
>> @@ -89,6 +89,9 @@ else
>>          TEST_IMG=$TEST_DIR/t.$IMGFMT
>>      elif [ "$IMGPROTO" = "archipelago" ]; then
>>          TEST_IMG="archipelago:at.$IMGFMT"
>> +    elif [ "$IMGPROTO" = "vxhs" ]; then
>> +        TEST_IMG_FILE=$TEST_DIR/t.$IMGFMT
>> +        TEST_IMG="vxhs://127.0.0.1:9999/t.$IMGFMT"
>>      else
>>          TEST_IMG=$IMGPROTO:$TEST_DIR/t.$IMGFMT
>>      fi
>> @@ -175,6 +178,12 @@ _make_test_img()
>>          eval "$QEMU_NBD -v -t -b 127.0.0.1 -p 10810 -f $IMGFMT  $TEST_IMG_FILE &"
>>          sleep 1 # FIXME: qemu-nbd needs to be listening before we continue
>>      fi
>> +
>> +    # Start QNIO server on image directory for vxhs protocol
>> +    if [ $IMGPROTO = "vxhs" ]; then
>> +        eval "$QEMU_VXHS -d  $TEST_DIR &"
>
> So I spoke too soon about tests passing, but it is not really your fault :)
>
> After rebasing to master, there is a new test 174 that now hangs (and hangs
> for nbd, as well).  This is because the test is piping the results of
> _make_test_image to sed to filter it.
>
> This line should redirect stdout to /dev/null, so that the pipe does not
> need to wait until process completion:
>
>     eval "$QEMU_VXHS -d  $TEST_DIR > /dev/null &"
>
>

Will make this change in the next series. Thanks again!