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

[Paolo Bonzini]

Thanks, this looks much better!  A few more remarks and code smells, but
much smaller than the previous review.

> +static int32_t
> +vxhs_qnio_iio_ioctl(void *apictx, uint32_t rfd, uint32_t opcode, int64_t *in,
> +                    void *ctx, uint32_t flags)

The "BDRVVXHSState *s, int idx" interface would apply here too.

> +{
> +    int   ret = 0;
> +
> +    switch (opcode) {
> +    case VDISK_STAT:
> +        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;
> +        trace_vxhs_qnio_iio_ioctl(opcode);
> +    }
> +
> +    return ret;
> +}
> +
> +/*
> + * Try to reopen the vDisk on one of the available hosts
> + * If vDisk reopen is successful on any of the host then
> + * check if that node is ready to accept I/O.
> + */
> +static int vxhs_reopen_vdisk(BDRVVXHSState *s, int index)
> +{
> +    VXHSvDiskHostsInfo hostinfo = s->vdisk_hostinfo[index];

This should almost definitely be

    VXHSvDiskHostsInfo *hostinfo = &s->vdisk_hostinfo[index];

and below:

    res = vxhs_qnio_iio_open(&hostinfo->qnio_cfd, of_vsa_addr,
                             &hostinfo->vdisk_rfd, file_name);

How was the failover code tested?



> +/*
> + * This helper function converts an array of iovectors into a flat buffer.
> + */
> +
> +static void *vxhs_convert_iovector_to_buffer(QEMUIOVector *qiov)

Please rename to vxhs_allocate_buffer_for_iovector.

> +static int vxhs_qemu_init(QDict *options, BDRVVXHSState *s,
> +                              int *cfd, int *rfd, Error **errp)

Passing the cfd and rfd as int* is unnecessary, because here:

> +    s->vdisk_cur_host_idx = 0;
> +    file_name = g_strdup_printf("%s%s", vdisk_prefix, s->vdisk_guid);
> +    of_vsa_addr = g_strdup_printf("of://%s:%d",
> +                                
> s->vdisk_hostinfo[s->vdisk_cur_host_idx].hostip,
> +                                
> s->vdisk_hostinfo[s->vdisk_cur_host_idx].port);
> +
> +    /*
> +     * .bdrv_open() and .bdrv_create() run under the QEMU global mutex.
> +     */
> +    if (global_qnio_ctx == NULL) {
> +        global_qnio_ctx = vxhs_setup_qnio();
> +        if (global_qnio_ctx == NULL) {
> +            error_setg(&local_err, "Failed vxhs_setup_qnio");
> +            ret = -EINVAL;
> +            goto out;
> +        }
> +    }
> +
> +    ret = vxhs_qnio_iio_open(cfd, of_vsa_addr, rfd, file_name);
> +    if (!ret) {
> +        error_setg(&local_err, "Failed qnio_iio_open");
> +        ret = -EIO;
> +    }
> +out:
> +    g_free(file_name);
> +    g_free(of_vsa_addr);


... you're basically doing the same as

     /* ... create global_qnio_ctx ... */

     s->qnio_ctx = global_qnio_ctx;
     s->vdisk_cur_host_idx = 0;
     ret = vxhs_reopen_vdisk(s, s->vdisk_cur_host_idx);

I suggest that you use vxhs_reopen_vdisk (rename it if you prefer; in
particular it's not specific to a *re*open). vxhs_qnio_iio_open remains
an internal function to vxhs_reopen_vdisk.

This would have also caught the bug in vxhs_reopen_vdisk! :->


> +errout:
> +    /*
> +     * Close remote vDisk device if it was opened earlier
> +     */
> +    if (device_opened) {
> +        for (i = 0; i < s->vdisk_nhosts; i++) {
> +            vxhs_qnio_iio_close(s, i);
> +        }
> +    }

No need for device_opened, since you have s->vdisk_nhosts = 0 on entry
and qnio_cfd/vdisk_rfd initialized to -1 at the point where
s->vdisk_nhosts become nonzero.

On the other hand, you could also consider inverting the initialization
order.  If you open the pipe first, cleaning it up is much easier than
cleaning up the backends.

> +static void vxhs_close(BlockDriverState *bs)
> +{
> +    BDRVVXHSState *s = bs->opaque;
> +    int i;
> +
> +    trace_vxhs_close(s->vdisk_guid);
> +    close(s->fds[VDISK_FD_READ]);
> +    close(s->fds[VDISK_FD_WRITE]);
> +
> +    /*
> +     * Clearing all the event handlers for oflame registered to QEMU
> +     */
> +    aio_set_fd_handler(bdrv_get_aio_context(bs), s->fds[VDISK_FD_READ],
> +                       false, NULL, NULL, NULL);
> +    g_free(s->vdisk_guid);
> +    s->vdisk_guid = NULL;
> +
> +    for (i = 0; i < VXHS_MAX_HOSTS; i++) {

Only loop up to s->vdisk_nhosts.

> +        vxhs_qnio_iio_close(s, i);
> +        /*
> +         * Free the dynamically allocated hostip string
> +         */
> +        g_free(s->vdisk_hostinfo[i].hostip);
> +        s->vdisk_hostinfo[i].hostip = NULL;
> +        s->vdisk_hostinfo[i].port = 0;
> +    }
> +}
> +
> +/*
> + * Returns the size of vDisk in bytes. This is required
> + * by QEMU block upper block layer so that it is visible
> + * to guest.
> + */
> +static int64_t vxhs_getlength(BlockDriverState *bs)
> +{
> +    BDRVVXHSState *s = bs->opaque;
> +    int64_t vdisk_size = 0;
> +
> +    if (s->vdisk_size > 0) {
> +        vdisk_size = s->vdisk_size;
> +    } else {
> +        /*
> +         * Fetch the vDisk size using stat ioctl
> +         */
> +        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;

Simpler:

    BDRVVXHSState *s = bs->opaque;

    if (s->vdisk_size == 0) {
        int64_t vdisk_size = vxhs_get_vdisk_stat(s);
        if (vdisk_size == 0) {
            return -EIO;
        }
        s->vdisk_size = vdisk_size;
    }

    return s->vdisk_size;

> +}
> +
> +/*
> + * Returns actual blocks allocated for the vDisk.
> + * This is required by qemu-img utility.
> + */
> +static int64_t vxhs_get_allocated_blocks(BlockDriverState *bs)
> +{
> +    BDRVVXHSState *s = bs->opaque;
> +    int64_t vdisk_size = 0;

Just return vxhs_getlength(bs).

Paolo