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

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

On 9/3/26 6:27 PM, Andrey Zhadchenko wrote:
> 
> 
> On 9/3/26 16:56, Andrey Drobyshev wrote:
>>> 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?
> 
> I would personally avoid this for now. First of all BLKSSZGET (and other 
> things) may be undefined (failed this for BLKROGET btw) and proper ifdef
> decoration are not so important for 'change backend setup' patch. We can
> always add it later.
>

At least in our headers BLKROGET and BLKSSZGET are defined together.
Both conf.readonly and conf.logical_block_size are set by the user (i.e.
libvirt) and might mismatch with the actual device state.  So IMHO
they're symmetrical in this regard.

>>
>>>   
>>> -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.
> 
>  From one point of view yes, from another point of view a lot other make 
> sense for virtio-device. I thought it was better to leave it as is and 
> use it later.
> But maybe remove it altogether (also along with logical/physical block 
> size) and better add it later as separate options if we feel tuning 
> these values brings any impact?


In general I'd just vote for limiting that list to the properties that
we're actually using and that matter to us.  Whether it's done via a new
list or via limiting existing ones is technical details.

Andrey