[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 RFC] block/vxhs: Initial commit to add Verita
|
From: |
ashish mittal |
|
Subject: |
Re: [Qemu-devel] [PATCH v6 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support |
|
Date: |
Tue, 27 Sep 2016 22:27:37 -0700 |
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
>
I have hopefully got all of these. Please let me know if any others
need changed.
> 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;
> }
> }
>
Done.
> (Likewise, iio_open/iio_devopen always happen in pairs and always build the
> openflame URI, so that's another candidate for a wrapper function).
>
Done.
> Also on the topic of closing:
>
> - there's no loop that initializes vdisk_rfd's and qnio_cfd's to -1.
>
vxhs_close() calling vxhs_qnio_iio_close() in a loop should initialize
rfd and cfd to -1 for all VXHS_MAX_HOSTS. Please let me know if I
missed this somewhere else.
> - 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).
>
Fixed. Thanks!
>>
>> + 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.
>
Yes, they are the same. I am now using global_qnio_ctx in
vxhs_qnio_iio_open() to avoid passing an additional arg. Let me know
if I should change this to get rid of the global variable.
>> + * 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.
>
Changed in all places to pass QEMUIOVector * instead of iiv/niov pair.
>> +{
>> + 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".
>
Fixed.
>> + 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.
>
Fixed.
>> +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.
>
Fixed. Thanks!
>> + 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.
>
Changed to use qemu_try_memalign(). The error condition should be
caught when we detect NULL as below:
buffer = vxhs_convert_iovector_to_buffer(qiov);
if (buffer == NULL) {
return -ENOMEM;
}
> However, why is alignment check necessary for read but not for write?
>
This is required for the readv as the qnio library does not implement
a iio_readv() at present. It does, however, implement a iio_writev(),
therefore the alignment check and new buffer allocation is not
required for the writev case.
>>
>> + 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++;
>
> [...]
>
Fixed all cases I could find. Thanks!
>
>>
>> +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.
>
Fixed.
>>
>> + 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.
>
Fixed the unnecessary "else".
>>
>> +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.
>
Removed.
>> +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.
>
Changed accordingly. I have yet to test this change completely.
>>
>> +{
>> + 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.
>
Changed. Thanks!
>> + * 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.
>
Fixed.
>> 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.
>
Got rid of the header file and most of the function 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.
>
Done.
>> +#define VXHS_SPIN_LOCK_DESTROY(lock) \
>> + (g_free(lock))
>> +
>
> Paolo
Regards,
Ashish