From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46889) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6kfp-0001Ix-8H for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:49:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6kfn-0003me-KJ for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:49:45 -0500 Received: from mail-oi0-x241.google.com ([2607:f8b0:4003:c06::241]:34181) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c6kfn-0003m2-8Z for qemu-devel@nongnu.org; Tue, 15 Nov 2016 15:49:43 -0500 Received: by mail-oi0-x241.google.com with SMTP id z62so3988270oiz.1 for ; Tue, 15 Nov 2016 12:49:43 -0800 (PST) MIME-Version: 1.0 In-Reply-To: <20161114150701.GD26198@stefanha-x1.localdomain> References: <1478566785-4002-1-git-send-email-ashish.mittal@veritas.com> <1478566785-4002-2-git-send-email-ashish.mittal@veritas.com> <20161114150701.GD26198@stefanha-x1.localdomain> From: ashish mittal Date: Tue, 15 Nov 2016 12:49:41 -0800 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: qemu-devel , Paolo Bonzini , Kevin Wolf , Markus Armbruster , "Daniel P. Berrange" , Jeff Cody , Fam Zheng , Ashish Mittal , Rakesh Ranjan , Buddhi.Madhav@veritas.com, Ketan Nilangekar , Abhijit Dey , Venkatesha.Mg@veritas.com On Mon, Nov 14, 2016 at 7:07 AM, Stefan Hajnoczi wrote: > On Mon, Nov 07, 2016 at 04:59:44PM -0800, Ashish Mittal wrote: >> Source code for the qnio library that this code loads can be downloaded from: >> https://github.com/MittalAshish/libqnio.git >> >> Sample command line using the JSON syntax: >> ./qemu-system-x86_64 -name instance-00000008 -S -vnc 0.0.0.0:0 -k en-us >> -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5 >> -msg timestamp=on >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410", >> "server":{"host":"172.172.17.4","port":"9999"}}' >> >> Sample command line using the URI syntax: >> qemu-img convert -f raw -O raw -n >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad >> vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0 >> >> Signed-off-by: Ashish Mittal >> --- >> v6 changelog: >> (1) Added qemu-iotests for VxHS as a new patch in the series. >> (2) Replaced release version from 2.8 to 2.9 in block-core.json. >> >> v5 changelog: >> (1) Incorporated v4 review comments. >> >> v4 changelog: >> (1) Incorporated v3 review comments on QAPI changes. >> (2) Added refcounting for device open/close. >> Free library resources on last device close. >> >> v3 changelog: >> (1) Added QAPI schema for the VxHS driver. >> >> v2 changelog: >> (1) Changes done in response to v1 comments. >> >> block/Makefile.objs | 2 + >> block/trace-events | 21 ++ >> block/vxhs.c | 689 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> configure | 41 +++ >> qapi/block-core.json | 21 +- >> 5 files changed, 772 insertions(+), 2 deletions(-) >> create mode 100644 block/vxhs.c >> >> diff --git a/block/Makefile.objs b/block/Makefile.objs >> index 67a036a..58313a2 100644 >> --- a/block/Makefile.objs >> +++ b/block/Makefile.objs >> @@ -18,6 +18,7 @@ block-obj-$(CONFIG_LIBNFS) += nfs.o >> block-obj-$(CONFIG_CURL) += curl.o >> block-obj-$(CONFIG_RBD) += rbd.o >> block-obj-$(CONFIG_GLUSTERFS) += gluster.o >> +block-obj-$(CONFIG_VXHS) += vxhs.o >> block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o >> block-obj-$(CONFIG_LIBSSH2) += ssh.o >> block-obj-y += accounting.o dirty-bitmap.o >> @@ -38,6 +39,7 @@ rbd.o-cflags := $(RBD_CFLAGS) >> rbd.o-libs := $(RBD_LIBS) >> gluster.o-cflags := $(GLUSTERFS_CFLAGS) >> gluster.o-libs := $(GLUSTERFS_LIBS) >> +vxhs.o-libs := $(VXHS_LIBS) >> ssh.o-cflags := $(LIBSSH2_CFLAGS) >> ssh.o-libs := $(LIBSSH2_LIBS) >> archipelago.o-libs := $(ARCHIPELAGO_LIBS) >> diff --git a/block/trace-events b/block/trace-events >> index 882c903..efdd5ef 100644 >> --- a/block/trace-events >> +++ b/block/trace-events >> @@ -112,3 +112,24 @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t offset, size_t len) "s >> qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64 >> qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64 >> qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) "s %p acb %p ret %d offset %"PRIu64" len %zu" >> + >> +# block/vxhs.c >> +vxhs_iio_callback(int error, int reason) "ctx is NULL: error %d, reason %d" >> +vxhs_setup_qnio(void *s) "Context to HyperScale IO manager = %p" >> +vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o %d, %d" >> +vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, errno %d" >> +vxhs_open_fail(int ret) "Could not open the device. Error = %d" >> +vxhs_open_epipe(int ret) "Could not create a pipe for device. Bailing out. Error=%d" >> +vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d" >> +vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off, void *acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d size = %lu offset = %lu ACB = %p. Error = %d, errno = %d" >> +vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl failed, ret = %d, errno = %d" >> +vxhs_get_vdisk_stat(char *vdisk_guid, uint64_t vdisk_size) "vDisk %s stat ioctl returned size %lu" >> +vxhs_qnio_iio_open(const char *ip) "Failed to connect to storage agent on host-ip %s" >> +vxhs_qnio_iio_devopen(const char *fname) "Failed to open vdisk device: %s" >> +vxhs_complete_aio(void *acb, uint64_t ret) "aio failed acb %p ret %ld" >> +vxhs_parse_uri_filename(const char *filename) "URI passed via bdrv_parse_filename %s" >> +vxhs_qemu_init_vdisk(const char *vdisk_id) "vdisk-id from json %s" >> +vxhs_parse_uri_hostinfo(int num, char *host, int port) "Host %d: IP %s, Port %d" >> +vxhs_qemu_init(char *of_vsa_addr, int port) "Adding host %s:%d to BDRVVXHSState" >> +vxhs_qemu_init_filename(const char *filename) "Filename passed as %s" >> +vxhs_close(char *vdisk_guid) "Closing vdisk %s" >> diff --git a/block/vxhs.c b/block/vxhs.c >> new file mode 100644 >> index 0000000..8913e8f >> --- /dev/null >> +++ b/block/vxhs.c >> @@ -0,0 +1,689 @@ >> +/* >> + * 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. >> + * >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "block/block_int.h" >> +#include > > Please move system headers (<>) above user headers (""). This way you > can be sure the system header isn't affected by any macros defined by > user headers. > >> +#include "qapi/qmp/qerror.h" >> +#include "qapi/qmp/qdict.h" >> +#include "qapi/qmp/qstring.h" >> +#include "trace.h" >> +#include "qemu/uri.h" >> +#include "qapi/error.h" >> +#include "qemu/error-report.h" > > Is this header file needed? > >> + >> +#define VDISK_FD_READ 0 >> +#define VDISK_FD_WRITE 1 >> + >> +#define VXHS_OPT_FILENAME "filename" >> +#define VXHS_OPT_VDISK_ID "vdisk-id" >> +#define VXHS_OPT_SERVER "server" >> +#define VXHS_OPT_HOST "host" >> +#define VXHS_OPT_PORT "port" >> + >> +typedef struct QNIOLibState { >> + int refcnt; >> + void *context; >> +} QNIOLibState; >> + >> +typedef enum { >> + VDISK_AIO_READ, >> + VDISK_AIO_WRITE, >> + VDISK_STAT > > This is unused. > >> +} VDISKAIOCmd; > > This typedef is unused but the VDISK_AIO_READ/VDISK_AIO_WRITE enum > constants are used. Please use the typedef name instead of int. That > way it's clear that the only valid values are the VDISK_* enum > constants. > >> + >> +/* >> + * HyperScale AIO callbacks structure >> + */ >> +typedef struct VXHSAIOCB { >> + BlockAIOCB common; >> + int err; >> + int direction; /* IO direction (r/w) */ > > This field is unused. > >> + size_t io_offset; > > This field is unused. > >> + size_t size; > > This field is unused. > >> + QEMUIOVector *qiov; >> +} VXHSAIOCB; >> + >> +typedef struct VXHSvDiskHostsInfo { >> + int qnio_cfd; /* Channel FD */ >> + int vdisk_rfd; /* vDisk remote FD */ > > Please don't call things FDs if they are not FDs. This is confusing > because dup(), close(), etc don't work on them. They are handles. > > Handles are an unnecessary layer of indirection in the first place. > libqnio would be simpler if it returned opaque pointers to structs > instead. This way hash/map lookups can be eliminated. > > Instead of storing strings in the hash/map, define structs with useful > fields. This eliminates some of the string parsing in libqnio. > >> + char *hostip; /* Host's IP addresses */ > > Is this strictly an IP address? If a hostname can be used too then > "host" would be clearer name. > >> + int port; /* Host's port number */ >> +} VXHSvDiskHostsInfo; >> + >> +/* >> + * Structure per vDisk maintained for state >> + */ >> +typedef struct BDRVVXHSState { >> + int fds[2]; >> + int event_reader_pos; > > Why is a pipe still being used? Back in August I mentioned that this > approach isn't the best practice anymore. It's more code and slower > than QEMUBH. > > You didn't respond to my review comment. Feel free to disagree with my > comments but please respond so I know what to expect. Now I'm wondering > whether other comments have been ignored too... > I did respond, also resent the same email today in case it was missed the first time. Please let me know if you did not receive the one I forwarded today and I will check if there's a problem at my end. Here's the text again: /+++++++++++++++++/ On Tue, Aug 23, 2016 at 3:22 PM, ashish mittal wrote: > Thanks Stefan, I will look at block/quorum.c. > > I have sent V4 of the patch with a reworked parsing logic for both > JSON and URI. Both of them are quite compact now. > > URI parsing now follows the suggestion given by Kevin. > /================/ > However, you should use the proper interfaces to implement this, which > is .bdrv_parse_filename(). This is a function that gets a string and > converts it into a QDict, which is then passed to .bdrv_open(). So it > uses exactly the same code path in .bdrv_open() as if used directly with > QAPI. > /================/ > > Additionally, I have fixed all the issues pointed out by you on V1 of > the patch. The only change I haven't done is to replace pipes with > QEMUBH. I am hoping this will not hold up the patch from being > accepted, and I can make this transition later with proper dev and > testing. > > Regards, > Ashish /+++++++++++++++++/ Will work on all the other suggestions in this email and the one you pointed out earlier. Thanks! >> +static int vxhs_qnio_iio_open(int *cfd, const char *of_vsa_addr, >> + int *rfd, const char *file_name) >> +{ >> + int ret = 0; >> + bool qnio_open = false; > > This variable isn't necessary since the vxhs_qnio_open() error uses > return instead of goto. > > > Pausing review at this point because I realized that my previous review > comments weren't addressed.