Re: [QEMU HCI-8.0 PATCH v2 2/5] vhost-blk: change backend setup

[Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>]

> Previously we used very ugly and incapsulation-breaking assignment
> fd = blk_bs(s->conf.conf.blk)->file->bs->opaque;
> It is wrong in a many ways, so let's rework this.
> 
> Patch changes default `drive` to new `devpath` option so device fd
> is managed by vhost-blk itself. Unfortunately this way we need a
> bit more preparational work: finding out disk length, etc. Don't
> be too broad and just do the minimal work. Drop the generic block
> device properties along with the block node: the kernel module
> does all IO in terms of 512 sectors, so simply report 512 byte
> logical/physical block size to the guest.
> Also we lose resize, as this is tied to the block node, which is
> now have no place in the setup. We will add this in the next
> patches as well as RO mode.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-143437
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index 9bd49fef2da..24f4fbe2b68 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -24,23 +24,17 @@
>  #include "system/system.h"
>  #include "linux-headers/linux/vhost.h"
>  #include <sys/ioctl.h>
> -#include <linux/fs.h>

Nit: next patch brings this header back, just keep it here.

> -#include "include/block/block_int-common.h"
>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
>  {
>      VHostBlk *s = VHOST_BLK(vdev);
>      struct vhost_vring_file backend;
> -    int ret, i, nworkers, *fd;
> +    int ret, i, nworkers;
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      char serial[VIRTIO_BLK_ID_BYTES] = {0};
>  
> -    bdrv_graph_rdlock_main_loop();
> -    fd = blk_bs(s->conf.conf.blk)->file->bs->opaque;
> -    bdrv_graph_rdunlock_main_loop();
> -
>      if (!k->set_guest_notifiers) {
>          error_report("vhost-blk: binding does not support guest notifiers");
>          return -ENOSYS;
> @@ -92,7 +86,7 @@ static int vhost_blk_start(VirtIODevice *vdev)
>  
>      memset(&backend, 0, sizeof(backend));
>      backend.index = 0;
> -    backend.fd = *fd;
> +    backend.fd = s->backend_fd;
>      if (ioctl(s->vhostfd, VHOST_BLK_SET_BACKEND, &backend)) {
>          error_report("vhost-blk: unable to set backend");
>          ret = -errno;
> @@ -208,29 +202,69 @@ static void vhost_blk_vm_state(void *opaque, bool running, RunState state)
>      }
>  }
>  
> -static void vhost_blk_resize_cb(void *opaque)
> +static int vhost_blk_update_size(VHostBlk *s, bool *changed, Error **errp)
>  {
> -    VirtIODevice *vdev = opaque;
> +    BlockConf *conf = &s->conf.conf;
> +    uint64_t length;
> +
> +    if (ioctl(s->backend_fd, BLKGETSIZE64, &length) < 0) {
> +        int error = errno;

This is redundant, error_setg_errno() preserves errno value.

> +
> +        error_setg_errno(errp, error,
> +                         "vhost-blk: unable to determine size of '%s'",
> +                         s->conf.devpath);
> +        return -error;

This is wrong error handling.  Let this function return 'bool changed'.
Then callers of vhost_blk_update_size() should check whether Error **errp
was set to smth, and either process the error themselves or propagate it
further.  That's how it's usually done in QEMU codebase.

> +    }
> +
> +    *changed = s->length != length;
> +    s->length = length;
> +    conf->heads = 16;
> +    conf->secs = 63;
> +    conf->cyls = s->length / BDRV_SECTOR_SIZE /
> +                 (conf->heads * conf->secs);
> +    conf->cyls = MIN(MAX(conf->cyls, 2U), 16383U);
>  
> -    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> -    virtio_notify_config(vdev);
> +    return 0;
>  }
>  
> -static void vhost_blk_resize(void *opaque)
> +static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>  {
> -    VirtIODevice *vdev = VIRTIO_DEVICE(opaque);
> +    BlockConf *conf = &s->conf.conf;
> +    struct stat st;
> +    bool changed;
>  
> -    /*
> -     * virtio_notify_config() needs to acquire the global mutex,
> -     * so it can't be called from an iothread. Instead, schedule
> -     * it to be run in the main context BH.
> -     */
> -    aio_bh_schedule_oneshot(qemu_get_aio_context(), vhost_blk_resize_cb, vdev);
> -}
> +    s->backend_fd = qemu_open(s->conf.devpath, O_RDWR, errp);
> +    if (s->backend_fd < 0) {
> +        error_prepend(errp, "vhost-blk: unable to open backend: ");
> +        return false;
> +    }
>  
> -static const BlockDevOps vhost_blk_block_ops = {
> -    .resize_cb     = vhost_blk_resize,
> -};
> +    if (fstat(s->backend_fd, &st) < 0) {
> +        error_setg_errno(errp, errno, "vhost-blk: unable to stat '%s'",
> +                         s->conf.devpath);
> +        goto fail;
> +    }
> +
> +    if (!S_ISBLK(st.st_mode)) {
> +        error_setg(errp, "vhost-blk: '%s' is not a block device",
> +                   s->conf.devpath);
> +        goto fail;
> +    }
> +
> +    if (vhost_blk_update_size(s, &changed, errp) < 0) {
> +        goto fail;
> +    }
> +
> +    conf->logical_block_size = BDRV_SECTOR_SIZE;
> +    conf->physical_block_size = BDRV_SECTOR_SIZE;
> +
> +    return true;
> +
> +fail:
> +    qemu_close(s->backend_fd);
> +    s->backend_fd = -1;
> +    return false;
> +}
>  
>  static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
>  {
> @@ -239,13 +273,8 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
>      VhostBlkConf *conf = &s->conf;
>      int i, ret;
>  
> -    if (!conf->conf.blk) {
> -        error_setg(errp, "vhost-blk: drive property not set");
> -        return;
> -    }
> -
> -    if (!blk_is_inserted(conf->conf.blk)) {
> -        error_setg(errp, "vhost-blk: device needs media, but drive is empty");
> +    if (!conf->devpath) {
> +        error_setg(errp, "vhost-blk: devpath property must be set");
>          return;
>      }
>  
> @@ -273,17 +302,7 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> -    if (!blkconf_apply_backend_options(&conf->conf,
> -                                       !blk_supports_write_perm(conf->conf.blk),
> -                                       true, errp)) {
> -        return;
> -    }
> -
> -    if (!blkconf_geometry(&conf->conf, NULL, 65535, 255, 255, errp)) {
> -        return;
> -    }
> -
> -    if (!blkconf_blocksizes(&conf->conf, errp)) {
> +    if (!vhost_blk_open_backend(s, errp)) {
>          return;
>      }
>  
> @@ -311,8 +330,6 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
>          goto cleanup;
>      }
>  
> -    blk_set_dev_ops(s->conf.conf.blk, &vhost_blk_block_ops, s);
> -
>      ret = vhost_dev_init(&s->dev, (void *)((size_t)s->vhostfd),
>                           VHOST_BACKEND_TYPE_KERNEL, 0, NULL);
>      if (ret < 0) {
> @@ -334,6 +351,10 @@ cleanup:
>          close(s->vhostfd);
>          s->vhostfd = -1;
>      }
> +    if (s->backend_fd >= 0) {
> +        qemu_close(s->backend_fd);
> +        s->backend_fd = -1;
> +    }
>      for (i = 0; i < conf->num_queues; i++) {
>          virtio_del_queue(vdev, i);
>      }
> @@ -349,6 +370,10 @@ static void vhost_blk_device_unrealize(DeviceState *dev)
>      qemu_del_vm_change_state_handler(s->mighand);
>      vhost_blk_set_status(vdev, 0);
>      vhost_dev_cleanup(&s->dev);
> +    if (s->backend_fd >= 0) {
> +        qemu_close(s->backend_fd);
> +        s->backend_fd = -1;
> +    }
>      g_free(s->dev.vqs);
>      virtio_cleanup(vdev);
>  }
> @@ -381,10 +406,6 @@ static uint64_t vhost_blk_get_features(VirtIODevice *vdev,
>  
>      virtio_add_feature(&features, VIRTIO_F_VERSION_1);
>  
> -    if (!blk_is_writable(s->conf.conf.blk)) {
> -        virtio_add_feature(&features, VIRTIO_BLK_F_RO);
> -    }
> -
>      if (s->conf.num_queues > 1) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
>      }
> @@ -403,7 +424,9 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      int64_t length;
>      int blk_size = conf->logical_block_size;
>  
> -    blk_get_geometry(s->conf.conf.blk, &capacity);
> +    length = s->length;
> +    capacity = length / BDRV_SECTOR_SIZE;
> +
>      memset(&blkcfg, 0, sizeof(blkcfg));
>      virtio_stq_p(vdev, &blkcfg.capacity, capacity);
>      virtio_stl_p(vdev, &blkcfg.seg_max, s->conf.queue_size - 2);
> @@ -411,7 +434,6 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      virtio_stl_p(vdev, &blkcfg.blk_size, blk_size);
>      blkcfg.geometry.heads = conf->heads;
>  
> -    length = blk_getlength(s->conf.conf.blk);
>      if (length > 0 && length / conf->heads / conf->secs % blk_size) {
>          unsigned short mask;
>  
> @@ -430,7 +452,7 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  }
>  
>  static const Property vhost_blk_properties[] = {
> -    DEFINE_BLOCK_PROPERTIES(VHostBlk, conf.conf),

Removing these props currently results into:

  error: Failed to start domain
  Property 'vhost-blk-pci.physical_block_size' not found

I understand we also patch libvirt so that it doesn't send those
block_size props, as well as write-cache etc.  Let's mention that in
commit message.

> +    DEFINE_PROP_STRING("devpath", VHostBlk, conf.devpath),
>      DEFINE_PROP_UINT16("num-queues", VHostBlk, conf.num_queues,
>                         VHOST_BLK_AUTO_NUM_QUEUES),
>      DEFINE_PROP_UINT16("queue-size", VHostBlk, conf.queue_size, 256),
> @@ -475,6 +497,8 @@ static void vhost_blk_instance_init(Object *obj)
>  {
>      VHostBlk *s = VHOST_BLK(obj);
>  
> +    s->vhostfd = -1;

Nit: ideally belongs to patch #1.  Not a big deal, but if you do a respin -
put it there.

Andrey

-- 
Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>