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

[ashish mittal]

libqnio source has been updated. qemu changes should now build properly.
https://github.com/MittalAshish/libqnio.git

I will work on the other suggestions and get back.

Regards,
Ashish


On Wed, Sep 21, 2016 at 8:03 AM, Paolo Bonzini <address@hidden> wrote:
>
>
> 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