Re: [Qemu-devel] [PATCH v6 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

[Paolo Bonzini]

On 21/09/2016 03:07, Ashish Mittal wrote:
> +int32_t vxhs_qnio_iio_writev(void *qnio_ctx, uint32_t rfd, struct iovec *iov,
> +                                            int iovcnt, uint64_t offset,
> +                                            void *ctx, uint32_t flags);
> +int32_t vxhs_qnio_iio_readv(void *qnio_ctx, uint32_t rfd, struct iovec *iov,
> +                                            int iovcnt, uint64_t offset,
> +                                            void *ctx, uint32_t flags);
> +int32_t vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcode,
> +                                            int64_t *in, void *ctx,
> +                                            uint32_t flags);

Since you have wrappers for this, please use less verbose arguments, such as

- BDRVVXHSState *s instead of the void * (qnio_ctx = s->qnio_ctx)

- int idx instead of uint32_t rfd (rfd = s->vdisk_hostinfo[idx].vdisk_rfd)

- the QEMUIOVector * instead of the iov/iovcnt pair

Likewise I suggest adding a wrapper

void vxhs_qnio_iio_close(BDRVVXHSState *s, int idx)
{
    if (s->vdisk_hostinfo[idx].vdisk_rfd >= 0) {
        iio_devclose(s->qnio_ctx, 0, s->vdisk_hostinfo[i].vdisk_rfd);
        s->vdisk_hostinfo[i].vdisk_rfd = -1;
    }


    if (s->vdisk_hostinfo[i].qnio_cfd >= 0) {
        iio_close(s->qnio_ctx, s->vdisk_hostinfo[i].qnio_cfd);
        s->vdisk_hostinfo[i].qnio_cfd = -1;
    }
}

(Likewise, iio_open/iio_devopen always happen in pairs and always build the
openflame URI, so that's another candidate for a wrapper function).

Also on the topic of closing:

- there's no loop that initializes vdisk_rfd's and qnio_cfd's to -1.

- here you are closing a vdisk_rfd twice:

+    if (s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd >= 0) {
+        iio_devclose(s->qnio_ctx, 0,
+            s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd);
+    }

because later you have another call to iio_devclose within
"for (i = 0; i < VXHS_MAX_HOSTS; i++) {".  (It's also the only place
that calls iio_devclose and not iio_close).

> 
> +    if (s->vdisk_hostinfo[index].qnio_cfd < 0) {
> +        s->vdisk_hostinfo[index].qnio_cfd =
> +                iio_open(global_qnio_ctx, of_vsa_addr, 0);

s->qnio_ctx seems to be always equal to global_qnio_ctx.  If that's
how the API works that's fine, however please use s->qnio_ctx consistently.
Initialize it early.

> + * Return Value:
> + *      On Success : return VXHS_VECTOR_ALIGNED
> + *      On Failure : return VXHS_VECTOR_NOT_ALIGNED.
> + */
> +int vxhs_is_iovector_read_aligned(struct iovec *iov, int niov, size_t sector)

Pass a QEMUIOVector here too.

> +{
> +    int i;
> +
> +    if (!iov || niov == 0) {
> +        return VXHS_VECTOR_ALIGNED;
> +    }

Unnecessary "if".  The loop below never rolls if niov == 0, and you should
never have "!iov && niov > 0".

> +    for (i = 0; i < niov; i++) {
> +        if (iov[i].iov_len % sector != 0) {
> +            return VXHS_VECTOR_NOT_ALIGNED;
> +        }
> +    }
> +    return VXHS_VECTOR_ALIGNED;
> +}

Please return just true or false.

> +void *vxhs_convert_iovector_to_buffer(struct iovec *iov, int niov,
> +                                      size_t sector)
> +{
> +    void *buf = NULL;
> +    size_t size = 0;
> +
> +    if (!iov || niov == 0) {
> +        return buf;
> +    }
> +
> +    size = vxhs_calculate_iovec_size(iov, niov);

If you have the QEMUIOVector, vxhs_calculate_iovec_size is just qiov->size.

> +    buf = qemu_memalign(sector, size);
> +    if (!buf) {
> +        trace_vxhs_convert_iovector_to_buffer(size);
> +        errno = -ENOMEM;
> +        return NULL;
> +    }
> +    return buf;
> +}
> +

This function should use qemu_try_memalign, not qemu_memalign.  But it is
obviously not very well tested, because the !iov || niov == 0 case doesn't
set errno and returns NULL.

You should just use qemu_try_memalign(qiov->size, BDRV_SECTOR_SIZE) in the
caller.

However, why is alignment check necessary for read but not for write?

> 
> +                if (ret == -1) {
> +                    trace_vxhs_qnio_iio_writev_err(i, iov[i].iov_len, errno);
> +                    /*
> +                     * Need to adjust the AIOCB segment count to prevent
> +                     * blocking of AIOCB completion within QEMU block driver.
> +                     */
> +                    if (segcount > 0 && (segcount - nsio) > 0) {
> +                        vxhs_dec_acb_segment_count(ctx, segcount - nsio);
> +                    }
> +                    return ret;

Here you return, so it is unnecessary to modify cur_offset in an "else"
(especially since nsio++ is outside the else).

There are other cases of this in vxhs_qnio_iio_writev.

> +                } else {
> +                    cur_offset += iov[i].iov_len;
> +                }
> +                nsio++;

[...]


> 
> +unsigned long vxhs_get_vdisk_stat(BDRVVXHSState *s)
> +{
> +    void *ctx = NULL;
> +    int flags = 0;
> +    int64_t vdisk_size = 0;
> +    int ret = 0;
> +
> +    ret = vxhs_qnio_iio_ioctl(s->qnio_ctx,
> +            s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd,
> +            VDISK_STAT, &vdisk_size, ctx, flags);
> +

ctx and flags are constants, please inline them instead of declaring them 
separately.

> 
> +    int64_t vdisk_size = 0;
> +
> +    if (s->vdisk_size > 0) {
> +        vdisk_size = s->vdisk_size;
> +    } else {
> +        /*
> +         * Fetch the vDisk size using stat ioctl
> +         */
> +        vdisk_size = vxhs_get_vdisk_stat(s);
> +        if (vdisk_size > 0) {
> +            s->vdisk_size = vdisk_size;
> +        }

Same here for vdisk_size.

> 
> +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"
> +        },
> +        {
> +            .name = "to",
> +            .type = QEMU_OPT_NUMBER,
> +            .help = "max port number, not supported by VxHS",
> +        },
> +        {
> +            .name = "ipv4",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "ipv4 bool value, not supported by VxHS",
> +        },
> +        {
> +            .name = "ipv6",
> +            .type = QEMU_OPT_BOOL,
> +            .help = "ipv6 bool value, not supported by VxHS",
> +        },
> +        { /* end of list */ }
> +    },
> +};

If they're not supported, do not include them.

> +static int vxhs_parse_uri(const char *filename, QDict *options)
> +{
> +    gchar **target_list;
> +    URI *uri = NULL;
> +    char *hoststr, *portstr;
> +    char *vdisk_id = NULL;
> +    char *port;
> +    int ret = 0;
> +    int i = 0;
> +
> +    trace_vxhs_parse_uri_filename(filename);
> +    target_list = g_strsplit(filename, "%7D", 0);
> +    assert(target_list != NULL && target_list[0] != NULL);
> +

This g_strsplit is not very robust.  You have something like
vxhs://foo/{ABC}vxhs://bar/{ABC}vxhs://foo/{ABC} and expecting the { and }
to be escaped to %7B and %7D.  But this doesn't have to be the case
especially when the filename is passed on the command-line by a user.

I think it's simpler if you limit the filename case to a single URI,
and force the user to use the host/port/vdisk_id to specify multiple
URIs.

> 
> +{
> +    if (qdict_haskey(options, "host")
> +        || qdict_haskey(options, "port")
> +        || qdict_haskey(options, "path"))
> +    {
> +        error_setg(errp, "host/port/path and a file name may not be 
> specified "
> +                         "at the same time");

I think you need to check server and vdisk_id, not host/port/path.

> +     * NOTE:
> +     *      Since spin lock is being allocated
> +     *      dynamically hence moving acb struct
> +     *      specific lock to BDRVVXHSState
> +     *      struct. The reason being,
> +     *      we don't want the overhead of spin
> +     *      lock being dynamically allocated and
> +     *      freed for every AIO.
> +     */
> +    s->vdisk_lock = VXHS_SPIN_LOCK_ALLOC;
> +    s->vdisk_acb_lock = VXHS_SPIN_LOCK_ALLOC;

There is no need to allocate the spinlock dynamically.  But you
need to initialize it with qemu_spin_init, which VXHS_SPIN_LOCK_ALLOC
is not doing.

> diff --git a/block/vxhs.h b/block/vxhs.h
> new file mode 100644
> index 0000000..d3d94bf
> --- /dev/null
> +++ b/block/vxhs.h
> @@ -0,0 +1,221 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#ifndef VXHSD_H
> +#define VXHSD_H


You don't need a header file, since everything is included in a single file.
Also please make all functions static in vxhs.c, and order them so that you
do not need prototypes.

> +#define VXHS_SPIN_LOCK(lock)                  \
> +    (qemu_spin_lock(lock))
> +#define VXHS_SPIN_UNLOCK(lock)                \
> +    (qemu_spin_unlock(lock))

Please inline these instead of using macros.

> +#define VXHS_SPIN_LOCK_DESTROY(lock)          \
> +    (g_free(lock))
> +

Paolo