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
