From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35757) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmj45-00062V-AB for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:04:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bmj3z-00078x-9l for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:04:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39202) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bmj3z-00078A-0F for qemu-devel@nongnu.org; Wed, 21 Sep 2016 11:03:55 -0400 References: <1474420065-110145-1-git-send-email-ashish.mittal@veritas.com> From: Paolo Bonzini Message-ID: Date: Wed, 21 Sep 2016 17:03:48 +0200 MIME-Version: 1.0 In-Reply-To: <1474420065-110145-1-git-send-email-ashish.mittal@veritas.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v6 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ashish Mittal , qemu-devel@nongnu.org, kwolf@redhat.com, armbru@redhat.com, berrange@redhat.com, ashish.mittal@veritas.com, stefanha@gmail.com Cc: Ketan.Nilangekar@veritas.com, Abhijit.Dey@veritas.com On 21/09/2016 03:07, Ashish Mittal wrote: > +int32_t vxhs_qnio_iio_writev(void *qnio_ctx, uint32_t rfd, struct iove= c *iov, > + int iovcnt, uint64_t offse= t, > + void *ctx, uint32_t flags)= ; > +int32_t vxhs_qnio_iio_readv(void *qnio_ctx, uint32_t rfd, struct iovec= *iov, > + int iovcnt, uint64_t offse= t, > + void *ctx, uint32_t flags)= ; > +int32_t vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcod= e, > + 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 =3D s->qnio_ctx) - int idx instead of uint32_t rfd (rfd =3D s->vdisk_hostinfo[idx].vdisk_r= fd) - 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 >=3D 0) { iio_devclose(s->qnio_ctx, 0, s->vdisk_hostinfo[i].vdisk_rfd); s->vdisk_hostinfo[i].vdisk_rfd =3D -1; } if (s->vdisk_hostinfo[i].qnio_cfd >=3D 0) { iio_close(s->qnio_ctx, s->vdisk_hostinfo[i].qnio_cfd); s->vdisk_hostinfo[i].qnio_cfd =3D -1; } } (Likewise, iio_open/iio_devopen always happen in pairs and always build t= he 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 >=3D 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 =3D 0; i < VXHS_MAX_HOSTS; i++) {". (It's also the only place that calls iio_devclose and not iio_close). >=20 > + if (s->vdisk_hostinfo[index].qnio_cfd < 0) { > + s->vdisk_hostinfo[index].qnio_cfd =3D > + 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 consistentl= y. 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 =3D=3D 0) { > + return VXHS_VECTOR_ALIGNED; > + } Unnecessary "if". The loop below never rolls if niov =3D=3D 0, and you s= hould never have "!iov && niov > 0". > + for (i =3D 0; i < niov; i++) { > + if (iov[i].iov_len % sector !=3D 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 =3D NULL; > + size_t size =3D 0; > + > + if (!iov || niov =3D=3D 0) { > + return buf; > + } > + > + size =3D vxhs_calculate_iovec_size(iov, niov); If you have the QEMUIOVector, vxhs_calculate_iovec_size is just qiov->siz= e. > + buf =3D qemu_memalign(sector, size); > + if (!buf) { > + trace_vxhs_convert_iovector_to_buffer(size); > + errno =3D -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 =3D=3D 0 case do= esn't set errno and returns NULL. You should just use qemu_try_memalign(qiov->size, BDRV_SECTOR_SIZE) in th= e caller. However, why is alignment check necessary for read but not for write? >=20 > + if (ret =3D=3D -1) { > + trace_vxhs_qnio_iio_writev_err(i, iov[i].iov_len, = errno); > + /* > + * Need to adjust the AIOCB segment count to preve= nt > + * blocking of AIOCB completion within QEMU block = driver. > + */ > + if (segcount > 0 && (segcount - nsio) > 0) { > + vxhs_dec_acb_segment_count(ctx, segcount - nsi= o); > + } > + 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 +=3D iov[i].iov_len; > + } > + nsio++; [...] >=20 > +unsigned long vxhs_get_vdisk_stat(BDRVVXHSState *s) > +{ > + void *ctx =3D NULL; > + int flags =3D 0; > + int64_t vdisk_size =3D 0; > + int ret =3D 0; > + > + ret =3D 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. >=20 > + int64_t vdisk_size =3D 0; > + > + if (s->vdisk_size > 0) { > + vdisk_size =3D s->vdisk_size; > + } else { > + /* > + * Fetch the vDisk size using stat ioctl > + */ > + vdisk_size =3D vxhs_get_vdisk_stat(s); > + if (vdisk_size > 0) { > + s->vdisk_size =3D vdisk_size; > + } Same here for vdisk_size. >=20 > +static QemuOptsList runtime_tcp_opts =3D { > + .name =3D "vxhs_tcp", > + .head =3D QTAILQ_HEAD_INITIALIZER(runtime_tcp_opts.head), > + .desc =3D { > + { > + .name =3D VXHS_OPT_HOST, > + .type =3D QEMU_OPT_STRING, > + .help =3D "host address (ipv4 addresses)", > + }, > + { > + .name =3D VXHS_OPT_PORT, > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "port number on which VxHSD is listening (defaul= t 9999)", > + .def_value_str =3D "9999" > + }, > + { > + .name =3D "to", > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "max port number, not supported by VxHS", > + }, > + { > + .name =3D "ipv4", > + .type =3D QEMU_OPT_BOOL, > + .help =3D "ipv4 bool value, not supported by VxHS", > + }, > + { > + .name =3D "ipv6", > + .type =3D QEMU_OPT_BOOL, > + .help =3D "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 =3D NULL; > + char *hoststr, *portstr; > + char *vdisk_id =3D NULL; > + char *port; > + int ret =3D 0; > + int i =3D 0; > + > + trace_vxhs_parse_uri_filename(filename); > + target_list =3D g_strsplit(filename, "%7D", 0); > + assert(target_list !=3D NULL && target_list[0] !=3D 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. >=20 > +{ > + 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 sp= ecified " > + "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 =3D VXHS_SPIN_LOCK_ALLOC; > + s->vdisk_acb_lock =3D 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 fi= le. Also please make all functions static in vxhs.c, and order them so that y= ou 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