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

[ashish mittal]

Hi Stefan, others,

Thank you for all the review comments.

On Wed, Sep 28, 2016 at 4:13 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Sep 27, 2016 at 09:09:49PM -0700, Ashish Mittal wrote:
>> +vxhs_bdrv_init(const char c) "Registering VxHS AIO driver%c"
>
> Why do several trace events have a %c format specifier at the end and it
> always takes a '.' value?
>
>> +#define QNIO_CONNECT_TIMOUT_SECS    120
>
> This isn't used and there is a typo (s/TIMOUT/TIMEOUT/).  Can it be
> dropped?
>
>> +static int32_t
>> +vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcode, int64_t 
>> *in,
>> +                    void *ctx, uint32_t flags)
>> +{
>> +    int   ret = 0;
>> +
>> +    switch (opcode) {
>> +    case VDISK_STAT:
>
> It seems unnecessary to abstract the iio_ioctl() constants and then have
> a switch statement to translate to the actual library constants.  It
> makes little sense since the flags argument already uses the library
> constants.  Just use the library's constants.
>
>> +        ret = iio_ioctl(apictx, rfd, IOR_VDISK_STAT,
>> +                                     in, ctx, flags);
>> +        break;
>> +
>> +    case VDISK_AIO_FLUSH:
>> +        ret = iio_ioctl(apictx, rfd, IOR_VDISK_FLUSH,
>> +                                     in, ctx, flags);
>> +        break;
>> +
>> +    case VDISK_CHECK_IO_FAILOVER_READY:
>> +        ret = iio_ioctl(apictx, rfd, IOR_VDISK_CHECK_IO_FAILOVER_READY,
>> +                                     in, ctx, flags);
>> +        break;
>> +
>> +    default:
>> +        ret = -ENOTSUP;
>> +        break;
>> +    }
>> +
>> +    if (ret) {
>> +        *in = 0;
>
> Some callers pass in = NULL so this will crash.
>
> The naming seems wrong: this is an output argument, not an input
> argument.  Please call it "out_val" or similar.
>
>> +    res = vxhs_reopen_vdisk(s, s->vdisk_ask_failover_idx);
>> +    if (res == 0) {
>> +        res = vxhs_qnio_iio_ioctl(s->qnio_ctx,
>> +                  s->vdisk_hostinfo[s->vdisk_ask_failover_idx].vdisk_rfd,
>> +                  VDISK_CHECK_IO_FAILOVER_READY, NULL, s, flags);
>
> Looking at iio_ioctl(), I'm not sure how this can ever work.  The fourth
> argument is NULL and iio_ioctl() will attempt *vdisk_size = 0 so this
> will crash.
>
> Do you have tests that exercise this code path?
>

You are right. This bug crept in to the FAILOVER path when I moved the
qnio shim code to qemu. Earlier code in libqnio did *in = 0 on a
per-case basis and skipped it for VDISK_CHECK_IO_FAILOVER_READY. I
will fix this.

We do thoroughly test these code paths, but the problem is that the
existing tests do not fully work with the new changes I am doing. I do
not yet have test case to test failover with the latest code. I do
frequently test using qemu-io (open a vdisk, read, write and re-read
to check written data) and also try to bring up an existing guest VM
using latest qemu-system-x86_64 binary to make sure I don't regress
main functionality. I did not however run these tests for v7 patch,
therefore some of the v7 changes do break the code. This patch had
some major code reconfiguration over v6, therefore my intention was to
just get a feel for whether the main code structure looks good.

A lot of changes have been proposed. I will discuss these with the
team and get back with inputs. I guess having a test framework is
really important at this time.

Regards,
Ashish

On Fri, Sep 30, 2016 at 1:36 AM, Stefan Hajnoczi <address@hidden> wrote:
> On Tue, Sep 27, 2016 at 09:09:49PM -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
>
> The QEMU block driver should deal with BlockDriver<->libqnio integration
> and libqnio should deal with vxhs logic (network protocol, failover,
> etc).  Right now the vxhs logic is spread between both components.  If
> responsibilities aren't cleanly separated between QEMU and libqnio then
> I see no point in having libqnio.
>
> Failover code should move into libqnio so that programs using libqnio
> avoid duplicating the failover code.
>
> Similarly IIO_IO_BUF_SIZE/segments should be handled internally by
> libqnio so programs using libqnio do not duplicate this code.
>
> libqnio itself can be simplified significantly:
>
> The multi-threading is not necessary and adds complexity.  Right now
> there seem to be two reasons for multi-threading: shared contexts and
> the epoll thread.  Both can be eliminated as follows.
>
> Shared contexts do not make sense in a multi-disk, multi-core
> environment.  Why is it advantages to tie disks to a single context?
> It's simpler and more multi-core friendly to let every disk have its own
> connection.
>
> The epoll thread forces library users to use thread synchronization when
> processing callbacks.  Look at libiscsi for an example of how to
> eliminate it.  Two APIs are defined: int iscsi_get_fd(iscsi) and int
> iscsi_which_events(iscsi) (e.g. POLLIN, POLLOUT).  The program using the
> library can integrate the fd into its own event loop.  The advantage of
> doing this is that no library threads are necessary and all callbacks
> are invoked from the program's event loop.  Therefore no thread
> synchronization is needed.
>
> If you make these changes then all multi-threading in libqnio and the
> QEMU block driver can be dropped.  There will be less code and it will
> be simpler.