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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4749 bytes --]

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?

> +/*
> + * This is called by QEMU when a flush gets triggered from within
> + * a guest at the block layer, either for IDE or SCSI disks.
> + */
> +static int vxhs_co_flush(BlockDriverState *bs)

This is called from coroutine context, please add the coroutine_fn
function attribute to document this.

> +{
> +    BDRVVXHSState *s = bs->opaque;
> +    int64_t size = 0;
> +    int ret = 0;
> +
> +    /*
> +     * VDISK_AIO_FLUSH ioctl is a no-op at present and will
> +     * always return success. This could change in the future.
> +     */
> +    ret = vxhs_qnio_iio_ioctl(s->qnio_ctx,
> +            s->vdisk_hostinfo[s->vdisk_cur_host_idx].vdisk_rfd,
> +            VDISK_AIO_FLUSH, &size, NULL, IIO_FLAG_SYNC);

This function is not allowed to block.  It cannot do a synchronous
flush.  This line is misleading because the constant is called
VDISK_AIO_FLUSH, but looking at the library code I see it's actually a
synchronous call that ends up in a loop that sleeps (!) waiting for the
response.

Please do an async flush and qemu_coroutine_yield() to return
control to QEMU's event loop.  When the flush completes you can
qemu_coroutine_enter() again to return from this function.

> +
> +    if (ret < 0) {
> +        trace_vxhs_co_flush(s->vdisk_guid, ret, errno);
> +        vxhs_close(bs);

This looks unsafe.  Won't it cause double close() calls for s->fds[]
when bdrv_close() is called later?

> +    }
> +
> +    return ret;
> +}
> +
> +static unsigned long vxhs_get_vdisk_stat(BDRVVXHSState *s)

sizeof(unsigned long) = 4 on some machines, please change it to int64_t.

I also suggest changing the function name to vxhs_get_vdisk_size() since
it only provides the size.

> +static int64_t vxhs_get_allocated_blocks(BlockDriverState *bs)
> +{
> +    BDRVVXHSState *s = bs->opaque;
> +    int64_t vdisk_size = 0;
> +
> +    if (s->vdisk_size > 0) {
> +        vdisk_size = s->vdisk_size;
> +    } else {
> +        /*
> +         * TODO:
> +         * Once HyperScale storage-virtualizer provides
> +         * actual physical allocation of blocks then
> +         * fetch that information and return back to the
> +         * caller but for now just get the full size.
> +         */
> +        vdisk_size = vxhs_get_vdisk_stat(s);
> +        if (vdisk_size > 0) {
> +            s->vdisk_size = vdisk_size;
> +        }
> +    }
> +
> +    if (vdisk_size > 0) {
> +        return vdisk_size; /* return size in bytes */
> +    }
> +
> +    return -EIO;
> +}

Why are you implementing this function if vxhs doesn't support querying
the allocated file size?  Don't return a bogus number.  Just don't
implement it like other block drivers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]