Re: [QEMU HCI-8.0 PATCH 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, block size,
> etc. Don't be too broad and just do the minimal and set reasonable
> default values. Validate with previously introduced
> blkconf_validate_blocksizes().
> 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 c52851fcf8b..a3e0010982f 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>
> -#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,79 @@ 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, Error **errp)
>  {
> -    VirtIODevice *vdev = opaque;
> +    BlockConf *conf = &s->conf.conf;
> +    off_t length;
> +    bool changed;
> +
> +    length = lseek(s->backend_fd, 0, SEEK_END);
> +    if (length < 0) {
> +        int error = errno;
> +
> +        error_setg_errno(errp, error,
> +                         "vhost-blk: unable to determine size of '%s'",
> +                         s->conf.devpath);
> +        return -error;
> +    }
>  
> -    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
> -    virtio_notify_config(vdev);
> +    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);
> +
> +    return changed;

This function should return int, but here we return bool.  And then
we do 'if (vhost_blk_update_size() < 0) ...', which never fires.

>  }
>  
> -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;
>  
> -    /*
> -     * 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;
> +    }

Suggestion: how about also checking BLKSSZGET value of the device at this
point and comparing it against conf->logical_block_size?

>  
> -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, errp) < 0) {
> +        goto fail;
> +    }
> +
> +    if (!conf->logical_block_size) {
> +        conf->logical_block_size = BDRV_SECTOR_SIZE;
> +    }
> +
> +    if (!conf->physical_block_size) {
> +        conf->physical_block_size = BDRV_SECTOR_SIZE;
> +    }
> +
> +    if (!blkconf_validate_blocksizes(conf, errp)) {
> +        goto fail;
> +    }
> +
> +    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 +283,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 +312,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,13 +340,13 @@ 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) {
>          error_setg(errp, "vhost-blk: vhost initialization failed: %s",
>                  strerror(-ret));
> +        /* vhost_dev_init() closes vhostfd on failure */
> +        s->vhostfd = -1;

Before this patch we were doing double close(vhostfd) after vhost_dev_init()
failure.  I'd make it a separate commit with a "Fixes:" tag.

>          goto cleanup;
>      }
>  
> @@ -328,7 +357,14 @@ cleanup:
>              qemu_del_vm_change_state_handler(s->mighand);
>      }
>      g_free(s->dev.vqs);
> -    close(s->vhostfd);
> +    if (s->vhostfd >= 0) {
> +        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);
>      }
> @@ -344,6 +380,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);
>  }
> @@ -376,10 +416,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);
>      }
> @@ -398,7 +434,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);
> @@ -406,7 +444,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;
>  
> @@ -425,7 +462,8 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  }
>  
>  static const Property vhost_blk_properties[] = {
> -    DEFINE_BLOCK_PROPERTIES(VHostBlk, conf.conf),
> +    DEFINE_BLOCK_PROPERTIES_BASE(VHostBlk, conf.conf),

DEFINE_BLOCK_PROPERTIES_BASE() macro defines lots of properties that
make no sense without BlockBackend.  E.g. backend_defaults, write-cache,
share-rw, account-invalid, account-failed, stats-intervals.  We should
consider limiting the list of config properties to the ones which really
matter to us.  Ideally as a separate commit.

Andrey

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