[Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-08-23 Thread Ashish Mittal
This patch adds support for a new block device type called "vxhs".
Source code for the library that this code loads can be downloaded from:
https://github.com/MittalAshish/libqnio.git

Sample command line using JSON syntax:
./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'

Sample command line using URI syntax:
qemu-img convert -f raw -O raw -n 
/var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D

Signed-off-by: Ashish Mittal 
---
v3 changelog:
=
(1) Reworked QAPI/JSON parsing.
(2) Reworked URI parsing as suggested by Kevin.
(3) Fixes per review comments from Stefan on v1.
(4) Fixes per review comments from Daniel on v3.

 block/Makefile.objs |2 +
 block/trace-events  |   41 ++
 block/vxhs.c| 1304 +++
 block/vxhs.h|  237 ++
 configure   |   50 ++
 5 files changed, 1634 insertions(+)
 create mode 100644 block/vxhs.c
 create mode 100644 block/vxhs.h

diff --git a/block/Makefile.objs b/block/Makefile.objs
index 2593a2f..5f83305 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
 block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
 block-obj-$(CONFIG_LIBSSH2) += ssh.o
+block-obj-$(CONFIG_VXHS) += vxhs.o
 block-obj-y += accounting.o dirty-bitmap.o
 block-obj-y += write-threshold.o
 
@@ -43,3 +44,4 @@ block-obj-m+= dmg.o
 dmg.o-libs := $(BZIP2_LIBS)
 qcow.o-libs:= -lz
 linux-aio.o-libs   := -laio
+vxhs.o-libs:= $(VXHS_LIBS)
diff --git a/block/trace-events b/block/trace-events
index 05fa13c..06c6d8c 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -114,3 +114,44 @@ 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_bdrv_init(const char c) "Registering VxHS AIO driver%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_setup_qnio_nwerror(char c) "Could not initialize the network channel. 
Bailing out%c"
+vxhs_iio_callback_iofail(int err, int reason, void *acb, int seg) "Read/Write 
failed: error %d, reason %d, acb %p, segment %d"
+vxhs_iio_callback_retry(char *guid, void *acb) "vDisk %s, added acb %p to 
retry queue (5)"
+vxhs_iio_callback_chnlfail(int error) "QNIO channel failed, no i/o (%d)"
+vxhs_iio_callback_fail(int r, void *acb, int seg, uint64_t size, int err) " 
ALERT: reason = %d , acb = %p, acb->segments = %d, acb->size = %lu Error = %d"
+vxhs_fail_aio(char * guid, void *acb) "vDisk %s, failing acb %p"
+vxhs_iio_callback_ready(char *vd, int err) "async vxhs_iio_callback: 
IRP_VDISK_CHECK_IO_FAILOVER_READY completed for vdisk %s with error %d"
+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(char c) "Could not create a pipe for device. Bailing out%c"
+vxhs_aio_rw(char *guid, int iodir, uint64_t size, uint64_t offset) "vDisk %s, 
vDisk device is in failed state iodir = %d size = %lu offset = %lu"
+vxhs_aio_rw_retry(char *guid, void *acb, int queue) "vDisk %s, added acb %p to 
retry queue(%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 seg, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d 
size = %lu offset = %lu ACB = %p Segments = %d. Error = %d, errno = %d"
+vxhs_co_flush(char *guid, int ret, int err) "vDisk (%s) Flush ioctl failed ret 
= %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_switch_storage_agent(char *ip, char *guid) "Query host %s for vdisk %s"
+vxhs_switch_storage_agent_failed(char *ip, char *guid, int res, int err) 
"Query to host %s for vdisk %s failed, res = %d, errno = %d"
+vxhs_failover_ioctl_cb(char *ip, char *guid) "Switched to storage server 
host-IP 

Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-08-30 Thread Jeff Cody

First-pass-over-the-code review:

On Mon, Aug 22, 2016 at 11:56:30PM -0700, Ashish Mittal wrote:
> This patch adds support for a new block device type called "vxhs".
> Source code for the library that this code loads can be downloaded from:
> https://github.com/MittalAshish/libqnio.git
> 
> Sample command line using JSON syntax:
> ./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'
> 
> Sample command line using URI syntax:
> qemu-img convert -f raw -O raw -n 
> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
> vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
> 

Is there a reference specification for vxhs available?  If so, the commit
message would be a good place to drop a link.

> Signed-off-by: Ashish Mittal 
> ---
> v3 changelog:
> =
> (1) Reworked QAPI/JSON parsing.
> (2) Reworked URI parsing as suggested by Kevin.
> (3) Fixes per review comments from Stefan on v1.
> (4) Fixes per review comments from Daniel on v3.
> 
>  block/Makefile.objs |2 +
>  block/trace-events  |   41 ++
>  block/vxhs.c| 1304 
> +++
>  block/vxhs.h|  237 ++
>  configure   |   50 ++
>  5 files changed, 1634 insertions(+)
>  create mode 100644 block/vxhs.c
>  create mode 100644 block/vxhs.h
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index 2593a2f..5f83305 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
> +block-obj-$(CONFIG_VXHS) += vxhs.o
>  block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  
> @@ -43,3 +44,4 @@ block-obj-m+= dmg.o
>  dmg.o-libs := $(BZIP2_LIBS)
>  qcow.o-libs:= -lz
>  linux-aio.o-libs   := -laio
> +vxhs.o-libs:= $(VXHS_LIBS)
> diff --git a/block/trace-events b/block/trace-events
> index 05fa13c..06c6d8c 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -114,3 +114,44 @@ 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_bdrv_init(const char c) "Registering VxHS AIO driver%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_setup_qnio_nwerror(char c) "Could not initialize the network channel. 
> Bailing out%c"
> +vxhs_iio_callback_iofail(int err, int reason, void *acb, int seg) 
> "Read/Write failed: error %d, reason %d, acb %p, segment %d"
> +vxhs_iio_callback_retry(char *guid, void *acb) "vDisk %s, added acb %p to 
> retry queue (5)"
> +vxhs_iio_callback_chnlfail(int error) "QNIO channel failed, no i/o (%d)"
> +vxhs_iio_callback_fail(int r, void *acb, int seg, uint64_t size, int err) " 
> ALERT: reason = %d , acb = %p, acb->segments = %d, acb->size = %lu Error = %d"
> +vxhs_fail_aio(char * guid, void *acb) "vDisk %s, failing acb %p"
> +vxhs_iio_callback_ready(char *vd, int err) "async vxhs_iio_callback: 
> IRP_VDISK_CHECK_IO_FAILOVER_READY completed for vdisk %s with error %d"
> +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(char c) "Could not create a pipe for device. Bailing out%c"
> +vxhs_aio_rw(char *guid, int iodir, uint64_t size, uint64_t offset) "vDisk 
> %s, vDisk device is in failed state iodir = %d size = %lu offset = %lu"
> +vxhs_aio_rw_retry(char *guid, void *acb, int queue) "vDisk %s, added acb %p 
> to retry queue(%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 seg, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d 
> size = %lu offset = %lu ACB = %p Segments = %d. Error = %d, errno = %d"
> +vxhs_co_flush(char *guid, int ret, int err) "vDisk (%s) Flush ioctl failed 
> ret = %d errno = %d"
> +vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl 
> failed, re

Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-20 Thread ashish mittal
Hi,

I have submitted a new patch (V6) after fixing the problem of
libqnioshim referencing symbols from inside qemu vxhs.c code.

(1) libqnioshim code has now been brought inside vxhs.c
(2) vxhs.c now needs to link only with libqnio.so.
(3) libqnio.so does not have any unreferenced symbols coming from qemu
code. This can also be witnessed from the code in configure that
checks for the presence of libqnio. It does not have to provide any
empty function definitions (as before) for the test code to compile.

Have also fixed a couple of other points raised during review. Will
reply to pending questions soon.

Regards,
Ashish

On Thu, Sep 8, 2016 at 4:15 PM, ashish mittal  wrote:
>>> Yes, qemu_iio_ioctl(VDISK_AIO_FLUSH) is only a place-holder at present
>>> in case we later want to add some functionality to it. I have now
>>> added a comment to this affect to avoid any confusion.
>>>
>>
>> The problem is you don't know which version of the qnio library any given
>> QEMU binary will be using, since it is a shared library.  Future versions
>> may implement the flush ioctl as expressed above, in which case we may hide
>> a valid error.
>>
>> Am I correct in assuming that this call suppresses errors because an error
>> is returned for an unknown ioctl operation of VDISK_AIO_FLUSH?  If so, and
>> you want a placeholder here for flushing, you should go all the way and stub
>> out the underlying ioctl call to return success.  Then QEMU can at least
>> rely on the error return from the flush operation.
>>
>>
>
> I agree. Will change accordingly. I also completely agree on the
> discussion of libqnio calling a qemu function. Will check on the best
> way to resolve that and get back.
>
> Thanks,
> Ashish
>
>
> On Thu, Sep 8, 2016 at 7:00 AM, Jeff Cody  wrote:
>> On Wed, Sep 07, 2016 at 03:32:47PM -0700, ashish mittal wrote:
>>> Hi Jeff,
>>>
>>> Thank you for the comments and corrections.
>>>
>>> I have submitted a v5 patch after incorporating most of your review
>>> comments. Please find details in-line.
>>>
>>> Thanks,
>>> Ashish
>>>
>>> On Tue, Aug 30, 2016 at 10:35 AM, Jeff Cody  wrote:
>>> >
>>> > First-pass-over-the-code review:
>>> >
>>> > On Mon, Aug 22, 2016 at 11:56:30PM -0700, Ashish Mittal wrote:
>>> >> This patch adds support for a new block device type called "vxhs".
>>> >> Source code for the library that this code loads can be downloaded from:
>>> >> https://github.com/MittalAshish/libqnio.git
>>> >>
>>> >> Sample command line using JSON syntax:
>>> >> ./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'
>>> >>
>>> >> Sample command line using URI syntax:
>>> >> qemu-img convert -f raw -O raw -n 
>>> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
>>> >> vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
>>> >>
>>> >
>>> > Is there a reference specification for vxhs available?  If so, the commit
>>> > message would be a good place to drop a link.
>>> >
>>>
>>> We don't have a reference specification doc yet. Although, I will
>>> shortly be adding a test program to libqnio that can be used to
>>> test/demo basic libqnio IO transfer.
>>>
>>> >> Signed-off-by: Ashish Mittal 
>>> >> ---
>>> >> v3 changelog:
>>> >> =
>>> >> (1) Reworked QAPI/JSON parsing.
>>> >> (2) Reworked URI parsing as suggested by Kevin.
>>> >> (3) Fixes per review comments from Stefan on v1.
>>> >> (4) Fixes per review comments from Daniel on v3.
>>> >>
>>> >>  block/Makefile.objs |2 +
>>> >>  block/trace-events  |   41 ++
>>> >>  block/vxhs.c| 1304 
>>> >> +++
>>> >>  block/vxhs.h|  237 ++
>>> >>  configure   |   50 ++
>>> >>  5 files changed, 1634 insertions(+)
>>> >>  create mode 100644 block/vxhs.c
>>> >>  create mode 100644 block/vxhs.h
>>> >>
>>> >> diff --git a/block/Makefile.objs b/block/Makefile.objs
>>> >> index 2593a2f..5f83305 100644
>>> >> --- a/block/Makefile.objs
>>> >> +++ b/block/Makefile.objs
>>> >> @@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
>>> >>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>>> >>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>>> >>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
>>> >> +block-obj-$(CONFIG_VXHS) += vxhs.o
>>> >>  block-obj-y += accounting.o dirty-bitmap.o
>>> >>  block-obj-y += write-threshold.o
>>> >>
>>> >> @@ -43,3 +44,4 @@ block-obj-m+= dmg.o
>>> >>  dmg.o-libs := $(BZIP2_LIBS)
>>> >>  qcow.o-libs:= -lz
>>> >>  linux-aio.o-libs   := -laio
>>> >> +vxhs.o-libs:= $(VXHS_LIBS)
>>> >> diff --git a/block/trace-events b/block/trace-events
>>> >> index 05fa13c..06c6d8c 100644
>>> >> --- a/block/trace-events
>>> >> +++ b/block/trace

Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-06 Thread Fam Zheng
On Mon, 08/22 23:56, Ashish Mittal wrote:
>  block/Makefile.objs |2 +
>  block/trace-events  |   41 ++
>  block/vxhs.c| 1304 
> +++
>  block/vxhs.h|  237 ++
>  configure   |   50 ++

If vxhs code is contained in one file, no need to add vxhs.h and all forward
declarations can go to vxhs.c. Then local functions can be marked as "static",
and all unused functions can be cleaned up.

Fam



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-07 Thread ashish mittal
Hi Jeff,

Thank you for the comments and corrections.

I have submitted a v5 patch after incorporating most of your review
comments. Please find details in-line.

Thanks,
Ashish

On Tue, Aug 30, 2016 at 10:35 AM, Jeff Cody  wrote:
>
> First-pass-over-the-code review:
>
> On Mon, Aug 22, 2016 at 11:56:30PM -0700, Ashish Mittal wrote:
>> This patch adds support for a new block device type called "vxhs".
>> Source code for the library that this code loads can be downloaded from:
>> https://github.com/MittalAshish/libqnio.git
>>
>> Sample command line using JSON syntax:
>> ./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'
>>
>> Sample command line using URI syntax:
>> qemu-img convert -f raw -O raw -n 
>> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
>> vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
>>
>
> Is there a reference specification for vxhs available?  If so, the commit
> message would be a good place to drop a link.
>

We don't have a reference specification doc yet. Although, I will
shortly be adding a test program to libqnio that can be used to
test/demo basic libqnio IO transfer.

>> Signed-off-by: Ashish Mittal 
>> ---
>> v3 changelog:
>> =
>> (1) Reworked QAPI/JSON parsing.
>> (2) Reworked URI parsing as suggested by Kevin.
>> (3) Fixes per review comments from Stefan on v1.
>> (4) Fixes per review comments from Daniel on v3.
>>
>>  block/Makefile.objs |2 +
>>  block/trace-events  |   41 ++
>>  block/vxhs.c| 1304 
>> +++
>>  block/vxhs.h|  237 ++
>>  configure   |   50 ++
>>  5 files changed, 1634 insertions(+)
>>  create mode 100644 block/vxhs.c
>>  create mode 100644 block/vxhs.h
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index 2593a2f..5f83305 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
>>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
>> +block-obj-$(CONFIG_VXHS) += vxhs.o
>>  block-obj-y += accounting.o dirty-bitmap.o
>>  block-obj-y += write-threshold.o
>>
>> @@ -43,3 +44,4 @@ block-obj-m+= dmg.o
>>  dmg.o-libs := $(BZIP2_LIBS)
>>  qcow.o-libs:= -lz
>>  linux-aio.o-libs   := -laio
>> +vxhs.o-libs:= $(VXHS_LIBS)
>> diff --git a/block/trace-events b/block/trace-events
>> index 05fa13c..06c6d8c 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -114,3 +114,44 @@ 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_bdrv_init(const char c) "Registering VxHS AIO driver%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_setup_qnio_nwerror(char c) "Could not initialize the network channel. 
>> Bailing out%c"
>> +vxhs_iio_callback_iofail(int err, int reason, void *acb, int seg) 
>> "Read/Write failed: error %d, reason %d, acb %p, segment %d"
>> +vxhs_iio_callback_retry(char *guid, void *acb) "vDisk %s, added acb %p to 
>> retry queue (5)"
>> +vxhs_iio_callback_chnlfail(int error) "QNIO channel failed, no i/o (%d)"
>> +vxhs_iio_callback_fail(int r, void *acb, int seg, uint64_t size, int err) " 
>> ALERT: reason = %d , acb = %p, acb->segments = %d, acb->size = %lu Error = 
>> %d"
>> +vxhs_fail_aio(char * guid, void *acb) "vDisk %s, failing acb %p"
>> +vxhs_iio_callback_ready(char *vd, int err) "async vxhs_iio_callback: 
>> IRP_VDISK_CHECK_IO_FAILOVER_READY completed for vdisk %s with error %d"
>> +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(char c) "Could not create a pipe for device. Bailing out%c"
>> +vxhs_aio_rw(char *guid, int iodir, uint64_t size, uint64_t offset) "vDisk 
>> %s, vDisk device is in failed state iodir = %d size = %lu offset = %lu"
>> +vxhs_aio_rw_retry(char *guid, void *acb, int queue) "vDisk %s, added acb %p 
>> to retry qu

Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Daniel P. Berrange
On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > >> +
> > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > >> +{
> > >> +VXHSAIOCB *acb = ptr;
> > >> +
> > >> +acb->buffer = buffer;
> > >> +}
> > >> +
> > >
> > > Unused function?
> > 
> > This is called from within libqnio.
> 
> Wait, you mean the library references a symbol in the qemu binary? This
> sounds completely wrong to me. I wouldn't even do something like this if
> it were an internal qemu library because I think libraries should be
> self-contained, but it's a much larger problem in something that is
> supposed to be an independent library.

I'd be surprised if that actually worked. If an external library wants
to refrence symbols in the QEMU binary, then QEMU would ned to be using
the -export-dynamic / -rdynamic linker flags to export all its symbols,
and AFAIK, we're not doing that.


Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Kevin Wolf
Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> >> +
> >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> >> +{
> >> +VXHSAIOCB *acb = ptr;
> >> +
> >> +acb->buffer = buffer;
> >> +}
> >> +
> >
> > Unused function?
> 
> This is called from within libqnio.

Wait, you mean the library references a symbol in the qemu binary? This
sounds completely wrong to me. I wouldn't even do something like this if
it were an internal qemu library because I think libraries should be
self-contained, but it's a much larger problem in something that is
supposed to be an independent library.

If the library has to call into qemu, it should only do so through
callbacks that qemu registered with the library (and then the function
wouldn't look unused in qemu).

In an earlier version of the series I suggested moving the part directly
related to qemu (at least the functions called qemu_*) from the library
to the block driver. Did you consider this?

Kevin



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Daniel P. Berrange
On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
> Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
> > On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> > > Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > > > >> +
> > > > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > > > >> +{
> > > > >> +VXHSAIOCB *acb = ptr;
> > > > >> +
> > > > >> +acb->buffer = buffer;
> > > > >> +}
> > > > >> +
> > > > >
> > > > > Unused function?
> > > > 
> > > > This is called from within libqnio.
> > > 
> > > Wait, you mean the library references a symbol in the qemu binary? This
> > > sounds completely wrong to me. I wouldn't even do something like this if
> > > it were an internal qemu library because I think libraries should be
> > > self-contained, but it's a much larger problem in something that is
> > > supposed to be an independent library.
> > 
> > I'd be surprised if that actually worked. If an external library wants
> > to refrence symbols in the QEMU binary, then QEMU would ned to be using
> > the -export-dynamic / -rdynamic linker flags to export all its symbols,
> > and AFAIK, we're not doing that.
> 
> Hm, but if the function is really used by the library, how else would it
> be when its name isn't mentioned anywhere in the patch except in its
> declaration? And it appears to be called there directly:
> 
> https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
> 
> Anyway, something is fishy here.

Oh, notice that .c file is actually part of the shim library, not the
main libqnio.so that QEMU would link against.

So, it really is unused and should be deleted from this block/vxhs.c
file.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Daniel P. Berrange
On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
> Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
> > On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> > > Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > > > >> +
> > > > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > > > >> +{
> > > > >> +VXHSAIOCB *acb = ptr;
> > > > >> +
> > > > >> +acb->buffer = buffer;
> > > > >> +}
> > > > >> +
> > > > >
> > > > > Unused function?
> > > > 
> > > > This is called from within libqnio.
> > > 
> > > Wait, you mean the library references a symbol in the qemu binary? This
> > > sounds completely wrong to me. I wouldn't even do something like this if
> > > it were an internal qemu library because I think libraries should be
> > > self-contained, but it's a much larger problem in something that is
> > > supposed to be an independent library.
> > 
> > I'd be surprised if that actually worked. If an external library wants
> > to refrence symbols in the QEMU binary, then QEMU would ned to be using
> > the -export-dynamic / -rdynamic linker flags to export all its symbols,
> > and AFAIK, we're not doing that.
> 
> Hm, but if the function is really used by the library, how else would it
> be when its name isn't mentioned anywhere in the patch except in its
> declaration? And it appears to be called there directly:
> 
> https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
> 
> Anyway, something is fishy here.

Yeah, and as you say, this approach is just plain wrong, and absolutely
can never be merged as is. The external library must never use anything
in QEMU that it wasn't explicitly given via a callback, otherwise that
library is defacto part of QEMU.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Kevin Wolf
Am 08.09.2016 um 11:43 hat Daniel P. Berrange geschrieben:
> On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
> > Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
> > > On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> > > > Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > > > > >> +
> > > > > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > > > > >> +{
> > > > > >> +VXHSAIOCB *acb = ptr;
> > > > > >> +
> > > > > >> +acb->buffer = buffer;
> > > > > >> +}
> > > > > >> +
> > > > > >
> > > > > > Unused function?
> > > > > 
> > > > > This is called from within libqnio.
> > > > 
> > > > Wait, you mean the library references a symbol in the qemu binary? This
> > > > sounds completely wrong to me. I wouldn't even do something like this if
> > > > it were an internal qemu library because I think libraries should be
> > > > self-contained, but it's a much larger problem in something that is
> > > > supposed to be an independent library.
> > > 
> > > I'd be surprised if that actually worked. If an external library wants
> > > to refrence symbols in the QEMU binary, then QEMU would ned to be using
> > > the -export-dynamic / -rdynamic linker flags to export all its symbols,
> > > and AFAIK, we're not doing that.
> > 
> > Hm, but if the function is really used by the library, how else would it
> > be when its name isn't mentioned anywhere in the patch except in its
> > declaration? And it appears to be called there directly:
> > 
> > https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
> > 
> > Anyway, something is fishy here.
> 
> Oh, notice that .c file is actually part of the shim library, not the
> main libqnio.so that QEMU would link against.
> 
> So, it really is unused and should be deleted from this block/vxhs.c
> file.

I haven't fully understood yet what's the deal with this shim library,
but this patch links against both.

Kevin



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Kevin Wolf
Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
> On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> > Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > > >> +
> > > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > > >> +{
> > > >> +VXHSAIOCB *acb = ptr;
> > > >> +
> > > >> +acb->buffer = buffer;
> > > >> +}
> > > >> +
> > > >
> > > > Unused function?
> > > 
> > > This is called from within libqnio.
> > 
> > Wait, you mean the library references a symbol in the qemu binary? This
> > sounds completely wrong to me. I wouldn't even do something like this if
> > it were an internal qemu library because I think libraries should be
> > self-contained, but it's a much larger problem in something that is
> > supposed to be an independent library.
> 
> I'd be surprised if that actually worked. If an external library wants
> to refrence symbols in the QEMU binary, then QEMU would ned to be using
> the -export-dynamic / -rdynamic linker flags to export all its symbols,
> and AFAIK, we're not doing that.

Hm, but if the function is really used by the library, how else would it
be when its name isn't mentioned anywhere in the patch except in its
declaration? And it appears to be called there directly:

https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer

Anyway, something is fishy here.

Kevin



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Paolo Bonzini


On 08/09/2016 12:18, Kevin Wolf wrote:
> Am 08.09.2016 um 11:43 hat Daniel P. Berrange geschrieben:
>> On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
>>> Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
 On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
 +
 +void vxhs_set_acb_buffer(void *ptr, void *buffer)
 +{
 +VXHSAIOCB *acb = ptr;
 +
 +acb->buffer = buffer;
 +}
 +
>>>
>>> Unused function?
>>
>> This is called from within libqnio.
>
> Wait, you mean the library references a symbol in the qemu binary? This
> sounds completely wrong to me. I wouldn't even do something like this if
> it were an internal qemu library because I think libraries should be
> self-contained, but it's a much larger problem in something that is
> supposed to be an independent library.

 I'd be surprised if that actually worked. If an external library wants
 to refrence symbols in the QEMU binary, then QEMU would ned to be using
 the -export-dynamic / -rdynamic linker flags to export all its symbols,
 and AFAIK, we're not doing that.
>>>
>>> Hm, but if the function is really used by the library, how else would it
>>> be when its name isn't mentioned anywhere in the patch except in its
>>> declaration? And it appears to be called there directly:
>>>
>>> https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
>>>
>>> Anyway, something is fishy here.
>>
>> Oh, notice that .c file is actually part of the shim library, not the
>> main libqnio.so that QEMU would link against.
>>
>> So, it really is unused and should be deleted from this block/vxhs.c
>> file.
> 
> I haven't fully understood yet what's the deal with this shim library,
> but this patch links against both.

Indeed, I suspect that the "shim library" should really be part of QEMU.
 It's just 700 lines of code, and some parts (e.g.
qnio_ck_initialize_lock seem unused even).  The main thing to do is
convert it from cJSON to QEMU's internal libraries for the same thing.

Paolo



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Jeff Cody
On Thu, Sep 08, 2016 at 10:34:24AM +0100, Daniel P. Berrange wrote:
> On Thu, Sep 08, 2016 at 11:29:41AM +0200, Kevin Wolf wrote:
> > Am 08.09.2016 um 10:49 hat Daniel P. Berrange geschrieben:
> > > On Thu, Sep 08, 2016 at 10:43:19AM +0200, Kevin Wolf wrote:
> > > > Am 08.09.2016 um 00:32 hat ashish mittal geschrieben:
> > > > > >> +
> > > > > >> +void vxhs_set_acb_buffer(void *ptr, void *buffer)
> > > > > >> +{
> > > > > >> +VXHSAIOCB *acb = ptr;
> > > > > >> +
> > > > > >> +acb->buffer = buffer;
> > > > > >> +}
> > > > > >> +
> > > > > >
> > > > > > Unused function?
> > > > > 
> > > > > This is called from within libqnio.
> > > > 
> > > > Wait, you mean the library references a symbol in the qemu binary? This
> > > > sounds completely wrong to me. I wouldn't even do something like this if
> > > > it were an internal qemu library because I think libraries should be
> > > > self-contained, but it's a much larger problem in something that is
> > > > supposed to be an independent library.
> > > 
> > > I'd be surprised if that actually worked. If an external library wants
> > > to refrence symbols in the QEMU binary, then QEMU would ned to be using
> > > the -export-dynamic / -rdynamic linker flags to export all its symbols,
> > > and AFAIK, we're not doing that.
> > 
> > Hm, but if the function is really used by the library, how else would it
> > be when its name isn't mentioned anywhere in the patch except in its
> > declaration? And it appears to be called there directly:
> > 
> > https://github.com/MittalAshish/libqnio/search?q=vxhs_set_acb_buffer
> > 
> > Anyway, something is fishy here.
> 
> Yeah, and as you say, this approach is just plain wrong, and absolutely
> can never be merged as is. The external library must never use anything
> in QEMU that it wasn't explicitly given via a callback, otherwise that
> library is defacto part of QEMU.
>

Looking at the qnio test server, it does not link against the shim; it looks
like only QEMU links against the shim.

There are at least 3 qemu symbols referenced by the shim library (as of v5):
vxhs_dec_acb_segment_count, vxhs_inc_acb_segment_count, vxhs_set_acb_buffer.

So given that:

1. QEMU is the only user of the shim,
2. The shim calls QEMU functions directly,
3. The shim only exists to bridge QEMU and the QNIO library,
   (From the shim .c file:
   "Network IO library for VxHS QEMU block driver ")
4. The shim makes assumptions about QEMU (e.g. sector size alignment)

I strongly agree with everyone that the shim should just be part of QEMU,
and is the better choice over simply refactoring out QEMU calls from the
shim.


Jeff



Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread Jeff Cody
On Wed, Sep 07, 2016 at 03:32:47PM -0700, ashish mittal wrote:
> Hi Jeff,
> 
> Thank you for the comments and corrections.
> 
> I have submitted a v5 patch after incorporating most of your review
> comments. Please find details in-line.
> 
> Thanks,
> Ashish
> 
> On Tue, Aug 30, 2016 at 10:35 AM, Jeff Cody  wrote:
> >
> > First-pass-over-the-code review:
> >
> > On Mon, Aug 22, 2016 at 11:56:30PM -0700, Ashish Mittal wrote:
> >> This patch adds support for a new block device type called "vxhs".
> >> Source code for the library that this code loads can be downloaded from:
> >> https://github.com/MittalAshish/libqnio.git
> >>
> >> Sample command line using JSON syntax:
> >> ./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'
> >>
> >> Sample command line using URI syntax:
> >> qemu-img convert -f raw -O raw -n 
> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
> >> vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
> >>
> >
> > Is there a reference specification for vxhs available?  If so, the commit
> > message would be a good place to drop a link.
> >
> 
> We don't have a reference specification doc yet. Although, I will
> shortly be adding a test program to libqnio that can be used to
> test/demo basic libqnio IO transfer.
> 
> >> Signed-off-by: Ashish Mittal 
> >> ---
> >> v3 changelog:
> >> =
> >> (1) Reworked QAPI/JSON parsing.
> >> (2) Reworked URI parsing as suggested by Kevin.
> >> (3) Fixes per review comments from Stefan on v1.
> >> (4) Fixes per review comments from Daniel on v3.
> >>
> >>  block/Makefile.objs |2 +
> >>  block/trace-events  |   41 ++
> >>  block/vxhs.c| 1304 
> >> +++
> >>  block/vxhs.h|  237 ++
> >>  configure   |   50 ++
> >>  5 files changed, 1634 insertions(+)
> >>  create mode 100644 block/vxhs.c
> >>  create mode 100644 block/vxhs.h
> >>
> >> diff --git a/block/Makefile.objs b/block/Makefile.objs
> >> index 2593a2f..5f83305 100644
> >> --- a/block/Makefile.objs
> >> +++ b/block/Makefile.objs
> >> @@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
> >>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> >>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
> >>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
> >> +block-obj-$(CONFIG_VXHS) += vxhs.o
> >>  block-obj-y += accounting.o dirty-bitmap.o
> >>  block-obj-y += write-threshold.o
> >>
> >> @@ -43,3 +44,4 @@ block-obj-m+= dmg.o
> >>  dmg.o-libs := $(BZIP2_LIBS)
> >>  qcow.o-libs:= -lz
> >>  linux-aio.o-libs   := -laio
> >> +vxhs.o-libs:= $(VXHS_LIBS)
> >> diff --git a/block/trace-events b/block/trace-events
> >> index 05fa13c..06c6d8c 100644
> >> --- a/block/trace-events
> >> +++ b/block/trace-events
> >> @@ -114,3 +114,44 @@ 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_bdrv_init(const char c) "Registering VxHS AIO driver%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_setup_qnio_nwerror(char c) "Could not initialize the network 
> >> channel. Bailing out%c"
> >> +vxhs_iio_callback_iofail(int err, int reason, void *acb, int seg) 
> >> "Read/Write failed: error %d, reason %d, acb %p, segment %d"
> >> +vxhs_iio_callback_retry(char *guid, void *acb) "vDisk %s, added acb %p to 
> >> retry queue (5)"
> >> +vxhs_iio_callback_chnlfail(int error) "QNIO channel failed, no i/o (%d)"
> >> +vxhs_iio_callback_fail(int r, void *acb, int seg, uint64_t size, int err) 
> >> " ALERT: reason = %d , acb = %p, acb->segments = %d, acb->size = %lu Error 
> >> = %d"
> >> +vxhs_fail_aio(char * guid, void *acb) "vDisk %s, failing acb %p"
> >> +vxhs_iio_callback_ready(char *vd, int err) "async vxhs_iio_callback: 
> >> IRP_VDISK_CHECK_IO_FAILOVER_READY completed for vdisk %s with error %d"
> >> +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(char c) "Could not

Re: [Qemu-devel] [PATCH v4 RFC] block/vxhs: Initial commit to add Veritas HyperScale VxHS block device support

2016-09-08 Thread ashish mittal
>> Yes, qemu_iio_ioctl(VDISK_AIO_FLUSH) is only a place-holder at present
>> in case we later want to add some functionality to it. I have now
>> added a comment to this affect to avoid any confusion.
>>
>
> The problem is you don't know which version of the qnio library any given
> QEMU binary will be using, since it is a shared library.  Future versions
> may implement the flush ioctl as expressed above, in which case we may hide
> a valid error.
>
> Am I correct in assuming that this call suppresses errors because an error
> is returned for an unknown ioctl operation of VDISK_AIO_FLUSH?  If so, and
> you want a placeholder here for flushing, you should go all the way and stub
> out the underlying ioctl call to return success.  Then QEMU can at least
> rely on the error return from the flush operation.
>
>

I agree. Will change accordingly. I also completely agree on the
discussion of libqnio calling a qemu function. Will check on the best
way to resolve that and get back.

Thanks,
Ashish


On Thu, Sep 8, 2016 at 7:00 AM, Jeff Cody  wrote:
> On Wed, Sep 07, 2016 at 03:32:47PM -0700, ashish mittal wrote:
>> Hi Jeff,
>>
>> Thank you for the comments and corrections.
>>
>> I have submitted a v5 patch after incorporating most of your review
>> comments. Please find details in-line.
>>
>> Thanks,
>> Ashish
>>
>> On Tue, Aug 30, 2016 at 10:35 AM, Jeff Cody  wrote:
>> >
>> > First-pass-over-the-code review:
>> >
>> > On Mon, Aug 22, 2016 at 11:56:30PM -0700, Ashish Mittal wrote:
>> >> This patch adds support for a new block device type called "vxhs".
>> >> Source code for the library that this code loads can be downloaded from:
>> >> https://github.com/MittalAshish/libqnio.git
>> >>
>> >> Sample command line using JSON syntax:
>> >> ./qemu-system-x86_64 -name instance-0008 -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":""},{"host":"172.172.17.2","port":""}]}'
>> >>
>> >> Sample command line using URI syntax:
>> >> qemu-img convert -f raw -O raw -n 
>> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad 
>> >> vxhs://192.168.0.1:/%7Bc6718f6b-0401-441d-a8c3-1f0064d75ee0%7D
>> >>
>> >
>> > Is there a reference specification for vxhs available?  If so, the commit
>> > message would be a good place to drop a link.
>> >
>>
>> We don't have a reference specification doc yet. Although, I will
>> shortly be adding a test program to libqnio that can be used to
>> test/demo basic libqnio IO transfer.
>>
>> >> Signed-off-by: Ashish Mittal 
>> >> ---
>> >> v3 changelog:
>> >> =
>> >> (1) Reworked QAPI/JSON parsing.
>> >> (2) Reworked URI parsing as suggested by Kevin.
>> >> (3) Fixes per review comments from Stefan on v1.
>> >> (4) Fixes per review comments from Daniel on v3.
>> >>
>> >>  block/Makefile.objs |2 +
>> >>  block/trace-events  |   41 ++
>> >>  block/vxhs.c| 1304 
>> >> +++
>> >>  block/vxhs.h|  237 ++
>> >>  configure   |   50 ++
>> >>  5 files changed, 1634 insertions(+)
>> >>  create mode 100644 block/vxhs.c
>> >>  create mode 100644 block/vxhs.h
>> >>
>> >> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> >> index 2593a2f..5f83305 100644
>> >> --- a/block/Makefile.objs
>> >> +++ b/block/Makefile.objs
>> >> @@ -20,6 +20,7 @@ block-obj-$(CONFIG_RBD) += rbd.o
>> >>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>> >>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>> >>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
>> >> +block-obj-$(CONFIG_VXHS) += vxhs.o
>> >>  block-obj-y += accounting.o dirty-bitmap.o
>> >>  block-obj-y += write-threshold.o
>> >>
>> >> @@ -43,3 +44,4 @@ block-obj-m+= dmg.o
>> >>  dmg.o-libs := $(BZIP2_LIBS)
>> >>  qcow.o-libs:= -lz
>> >>  linux-aio.o-libs   := -laio
>> >> +vxhs.o-libs:= $(VXHS_LIBS)
>> >> diff --git a/block/trace-events b/block/trace-events
>> >> index 05fa13c..06c6d8c 100644
>> >> --- a/block/trace-events
>> >> +++ b/block/trace-events
>> >> @@ -114,3 +114,44 @@ 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_bdrv_init(const char c) "Registering VxHS AIO driver%c"
>> >> +vxhs_iio_callback(int error, int reason) "ctx is NULL: error %d, reason 
>> >> %d"
>> >> +vxhs_setup_qnio(void *s) "Context