[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new bloc
From: |
ashish mittal |
Subject: |
Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs" |
Date: |
Tue, 15 Nov 2016 12:49:41 -0800 |
On Mon, Nov 14, 2016 at 7:07 AM, Stefan Hajnoczi <address@hidden> 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 <address@hidden>
>> ---
>> 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 <qnio/qnio_api.h>
>
> 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 <address@hidden> 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.
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", (continued)
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Fam Zheng, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Stefan Hajnoczi, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", ashish mittal, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Stefan Hajnoczi, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Markus Armbruster, 2016/11/16
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Fam Zheng, 2016/11/16
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Stefan Hajnoczi, 2016/11/16
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", ashish mittal, 2016/11/16
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Eric Blake, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Markus Armbruster, 2016/11/15
- Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs",
ashish mittal <=
Re: [Qemu-devel] [PATCH v6 1/2] block/vxhs.c: Add support for a new block device type called "vxhs", Daniel P. Berrange, 2016/11/17
[Qemu-devel] [PATCH v6 2/2] block/vxhs.c: Add qemu-iotests for new block device type "vxhs", Ashish Mittal, 2016/11/07