Re: [QEMU HCI-8.0 PATCH v3 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.
> Also drop 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.
> Existing vhost-blk setups will break, but it is expected.
> 
> We also 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 20c2f416916..d6681235e5d 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -25,22 +25,17 @@
>  #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 +87,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 +203,69 @@ static void vhost_blk_vm_state(void *opaque, bool running, RunState state)
>      }
>  }
>  
> -static void vhost_blk_resize_cb(void *opaque)
> +static bool vhost_blk_update_size(VHostBlk *s, Error **errp)
>  {
> -    VirtIODevice *vdev = opaque;
> +    BlockConf *conf = &s->conf.conf;
> +    uint64_t length;
> +    bool changed;
> +
> +    if (ioctl(s->backend_fd, BLKGETSIZE64, &length) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "vhost-blk: unable to determine size of '%s'",
> +                         s->conf.devpath);
> +        return false;
> +    }
>  
> -    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);

This hunk haven't changed since v2, but I've only noticed now.  Where do
these constants come from?  This code is extremely obscure as-is, no
explanation, no comment.  Are we applying the same logic here as in
hw/block/hd-geometry.c ? If so, let's just factor out some helper from
there and reuse it.  If it's inconvenient - at least comment where do
these values come from.

Another thing: these geometry values don't change over time, why we set
them on every call to update_size()?  Maybe just put them into
open_backend(), together with setting logical/physical block sizes.

Andrey

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