[QEMU HCI-8.0 PATCH 0/5] vhost-blk change backend setup

[QEMU HCI-8.0 PATCH 0/5] vhost-blk change backend setup

[Andrey Zhadchenko]

This series changes vhost-blk backend setup from block node to
a path to the device.
Getting fd from block node was a hack in the first place. It
looked ugly and broke encapsulation.
Change vhost-blk to have it's own property so it opens and owns
the descriptor.

Patches #4 implement auto-resize by watching netlink. The problem
is that we cannot efficiently limit the scope without proper eBPF
setup which needs compilation, map handling, eBPFpriveleges and
so on.
Patch #5 tries to filter all events so we can get only resizes
but limited by BPF size.
Both patches are more like an RFC.

Andrey Zhadchenko (5):
  blk: factor out validation
  vhost-blk: change backend setup
  vhost-blk: add read-only flag
  vhost-blk: watch the device for resize events
  vhost-blk: filter uevents in the kernel

 hw/block/block.c              |   5 +
 hw/block/vhost-blk.c          | 461 ++++++++++++++++++++++++++++++----
 include/hw/block/block.h      |   1 +
 include/hw/virtio/vhost-blk.h |  11 +-
 4 files changed, 434 insertions(+), 44 deletions(-)

-- 
2.43.5

[QEMU HCI-8.0 PATCH 1/5] blk: factor out validation

[Andrey Zhadchenko]

Preparational patch. This helper will be used in the next patches
to validate vhost-blk block parameters.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 hw/block/block.c         | 5 +++++
 include/hw/block/block.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/hw/block/block.c b/hw/block/block.c
index 47897ca5d3..6f135b7e2f 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -162,6 +162,11 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
         }
     }
 
+    return blkconf_validate_blocksizes(conf, errp);
+}
+
+bool blkconf_validate_blocksizes(BlockConf *conf, Error **errp)
+{
     if (conf->logical_block_size > conf->physical_block_size) {
         error_setg(errp,
                    "logical_block_size > physical_block_size not supported");
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 664d629d37..dfd3ff0f62 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -106,6 +106,7 @@ bool blkconf_geometry(BlockConf *conf, int *trans,
                       unsigned cyls_max, unsigned heads_max, unsigned secs_max,
                       Error **errp);
 bool blkconf_blocksizes(BlockConf *conf, Error **errp);
+bool blkconf_validate_blocksizes(BlockConf *conf, Error **errp);
 bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
                                    bool resizable, Error **errp);
 
-- 
2.43.5

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

[Andrey Zhadchenko]

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>
---
 hw/block/vhost-blk.c          | 144 ++++++++++++++++++++++------------
 include/hw/virtio/vhost-blk.h |   5 +-
 2 files changed, 95 insertions(+), 54 deletions(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index c52851fcf8..a3e0010982 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;
 }
 
-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;
+    }
 
-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;
         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_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),
@@ -470,6 +508,8 @@ static void vhost_blk_instance_init(Object *obj)
 {
     VHostBlk *s = VHOST_BLK(obj);
 
+    s->vhostfd = -1;
+    s->backend_fd = -1;
     device_add_bootindex_property(obj, &s->conf.conf.bootindex,
                                   "bootindex", "/disk@0,0",
                                   DEVICE(obj));
diff --git a/include/hw/virtio/vhost-blk.h b/include/hw/virtio/vhost-blk.h
index 0c7e212595..c194b421d9 100644
--- a/include/hw/virtio/vhost-blk.h
+++ b/include/hw/virtio/vhost-blk.h
@@ -14,7 +14,6 @@
 #include "standard-headers/linux/virtio_blk.h"
 #include "hw/block/block.h"
 #include "hw/virtio/vhost.h"
-#include "system/block-backend.h"
 
 #define TYPE_VHOST_BLK "vhost-blk"
 #define VHOST_BLK(obj) \
@@ -25,6 +24,7 @@
 
 typedef struct VhostBlkConf {
     BlockConf conf;
+    char *devpath;
     uint16_t num_queues;
     uint16_t queue_size;
     uint16_t num_threads;
@@ -37,10 +37,11 @@ typedef struct VHostBlk {
     VMChangeStateEntry *mighand;
     uint64_t host_features;
     uint64_t decided_features;
-    struct virtio_blk_config blkcfg;
     int vhostfd;
+    int backend_fd;
     struct vhost_dev dev;
     bool vhost_started;
+    uint64_t length;
 } VHostBlk;
 
 #endif
-- 
2.43.5

[QEMU HCI-8.0 PATCH 3/5] vhost-blk: add read-only flag

[Andrey Zhadchenko]

and set RO respectively. Also compare BLKROGET with the selected
mode and reject r/w if needed.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 hw/block/vhost-blk.c          | 23 ++++++++++++++++++++++-
 include/hw/virtio/vhost-blk.h |  1 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index a3e0010982..47b6e560d4 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -24,6 +24,7 @@
 #include "system/system.h"
 #include "linux-headers/linux/vhost.h"
 #include <sys/ioctl.h>
+#include <linux/fs.h>
 #include "system/runstate.h"
 
 static int vhost_blk_start(VirtIODevice *vdev)
@@ -233,8 +234,10 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
 {
     BlockConf *conf = &s->conf.conf;
     struct stat st;
+    int readonly;
+    int open_flags = s->conf.readonly ? O_RDONLY : O_RDWR;
 
-    s->backend_fd = qemu_open(s->conf.devpath, O_RDWR, errp);
+    s->backend_fd = qemu_open(s->conf.devpath, open_flags, errp);
     if (s->backend_fd < 0) {
         error_prepend(errp, "vhost-blk: unable to open backend: ");
         return false;
@@ -252,6 +255,19 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
         goto fail;
     }
 
+    if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
+        error_setg_errno(errp, errno,
+                         "vhost-blk: unable to get read-only status of '%s'",
+                         s->conf.devpath);
+        goto fail;
+    }
+
+    if (readonly && !s->conf.readonly) {
+        error_setg(errp, "vhost-blk: '%s' is read-only",
+                   s->conf.devpath);
+        goto fail;
+    }
+
     if (vhost_blk_update_size(s, errp) < 0) {
         goto fail;
     }
@@ -416,6 +432,10 @@ static uint64_t vhost_blk_get_features(VirtIODevice *vdev,
 
     virtio_add_feature(&features, VIRTIO_F_VERSION_1);
 
+    if (s->conf.readonly) {
+        virtio_add_feature(&features, VIRTIO_BLK_F_RO);
+    }
+
     if (s->conf.num_queues > 1) {
         virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
     }
@@ -464,6 +484,7 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 static const Property vhost_blk_properties[] = {
     DEFINE_BLOCK_PROPERTIES_BASE(VHostBlk, conf.conf),
     DEFINE_PROP_STRING("devpath", VHostBlk, conf.devpath),
+    DEFINE_PROP_BOOL("read-only", VHostBlk, conf.readonly, false),
     DEFINE_PROP_UINT16("num-queues", VHostBlk, conf.num_queues,
                        VHOST_BLK_AUTO_NUM_QUEUES),
     DEFINE_PROP_UINT16("queue-size", VHostBlk, conf.queue_size, 256),
diff --git a/include/hw/virtio/vhost-blk.h b/include/hw/virtio/vhost-blk.h
index c194b421d9..c6646f5845 100644
--- a/include/hw/virtio/vhost-blk.h
+++ b/include/hw/virtio/vhost-blk.h
@@ -25,6 +25,7 @@
 typedef struct VhostBlkConf {
     BlockConf conf;
     char *devpath;
+    bool readonly;
     uint16_t num_queues;
     uint16_t queue_size;
     uint16_t num_threads;
-- 
2.43.5

[QEMU HCI-8.0 PATCH 4/5] vhost-blk: watch the device for resize events

[Andrey Zhadchenko]

Resize was tied to block node, which we removed some time ago.
Luckily we can make resize automated: watch netlink for relevant
events and call virtio_notify_config() if we detect capacity
change.
Failed netlink setup during creation leads to failure, but this
is a price we are ready to pay for consistency.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 hw/block/vhost-blk.c          | 193 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost-blk.h |   5 +
 2 files changed, 198 insertions(+)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index 47b6e560d4..f8eca4d58a 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -10,7 +10,9 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/cutils.h"
 #include "qemu/error-report.h"
+#include "qemu/main-loop.h"
 #include "qom/object.h"
 #include "hw/qdev-core.h"
 #include "hw/boards.h"
@@ -25,8 +27,13 @@
 #include "linux-headers/linux/vhost.h"
 #include <sys/ioctl.h>
 #include <linux/fs.h>
+#include <linux/netlink.h>
 #include "system/runstate.h"
 
+static int vhost_blk_uevent_fd = -1;
+static QLIST_HEAD(, VHostBlk) vhost_blk_uevent_watchers =
+    QLIST_HEAD_INITIALIZER(vhost_blk_uevent_watchers);
+
 static int vhost_blk_start(VirtIODevice *vdev)
 {
     VHostBlk *s = VHOST_BLK(vdev);
@@ -230,6 +237,182 @@ static int vhost_blk_update_size(VHostBlk *s, Error **errp)
     return changed;
 }
 
+static void vhost_blk_resize_bh(void *opaque)
+{
+    VHostBlk *s = opaque;
+    Error *local_err = NULL;
+    int ret;
+
+    ret = vhost_blk_update_size(s, &local_err);
+    if (ret < 0) {
+        error_report_err(local_err);
+        return;
+    }
+
+    if (ret) {
+        virtio_notify_config(VIRTIO_DEVICE(s));
+    }
+}
+
+static void vhost_blk_uevent_read(void *opaque)
+{
+    char buffer[64 * 1024 + 1];
+
+    for (;;) {
+        struct sockaddr_nl source;
+        socklen_t source_len = sizeof(source);
+        uint64_t event_major = UINT64_MAX;
+        uint64_t event_minor = UINT64_MAX;
+        bool action_change = false;
+        bool subsystem_block = false;
+        bool resize = false;
+        char *field;
+        char *end;
+        ssize_t len;
+
+        memset(&source, 0, sizeof(source));
+        len = recvfrom(vhost_blk_uevent_fd, buffer, sizeof(buffer) - 1,
+                       MSG_DONTWAIT, (struct sockaddr *)&source, &source_len);
+        if (len < 0) {
+            if (errno == EINTR) {
+                continue;
+            }
+            if (errno == ENOBUFS) {
+                VHostBlk *s;
+
+                /* uevents dropped. Re-check just to be sure */
+                QLIST_FOREACH(s, &vhost_blk_uevent_watchers, uevent_node) {
+                    qemu_bh_schedule(s->resize_bh);
+                }
+                continue;
+            }
+            if (errno != EAGAIN && errno != EWOULDBLOCK) {
+                error_report("vhost-blk: unable to receive uevent: %s",
+                             strerror(errno));
+            }
+            return;
+        }
+
+        if (source.nl_family != AF_NETLINK || source.nl_pid != 0) {
+            continue;
+        }
+
+        buffer[len] = '\0';
+        field = buffer;
+        end = buffer + len;
+        while (field < end) {
+            size_t field_len = strnlen(field, end - field);
+
+            if (!strcmp(field, "ACTION=change")) {
+                action_change = true;
+            } else if (!strcmp(field, "SUBSYSTEM=block")) {
+                subsystem_block = true;
+            } else if (!strcmp(field, "RESIZE=1")) {
+                resize = true;
+            } else if (g_str_has_prefix(field, "MAJOR=")) {
+                uint64_t value;
+
+                if (!qemu_strtou64(field + strlen("MAJOR="), NULL, 10,
+                                   &value)) {
+                    event_major = value;
+                }
+            } else if (g_str_has_prefix(field, "MINOR=")) {
+                uint64_t value;
+
+                if (!qemu_strtou64(field + strlen("MINOR="), NULL, 10,
+                                   &value)) {
+                    event_minor = value;
+                }
+            }
+
+            if (field_len == end - field) {
+                break;
+            }
+            field += field_len + 1;
+        }
+
+        if (action_change && subsystem_block && resize &&
+            event_major <= UINT_MAX && event_minor <= UINT_MAX) {
+            VHostBlk *s;
+
+            QLIST_FOREACH(s, &vhost_blk_uevent_watchers, uevent_node) {
+                dev_t rdev = s->backend_rdev;
+
+                if (major(rdev) == event_major &&
+                    minor(rdev) == event_minor) {
+                    qemu_bh_schedule(s->resize_bh);
+                }
+            }
+        }
+    }
+}
+
+static bool vhost_blk_uevent_init(Error **errp)
+{
+    struct sockaddr_nl address = {
+        .nl_family = AF_NETLINK,
+        .nl_groups = 1,
+    };
+
+    if (vhost_blk_uevent_fd >= 0) {
+        return true;
+    }
+
+    vhost_blk_uevent_fd = socket(AF_NETLINK,
+                                 SOCK_DGRAM | SOCK_NONBLOCK | SOCK_CLOEXEC,
+                                 NETLINK_KOBJECT_UEVENT);
+    if (vhost_blk_uevent_fd < 0) {
+        error_setg_errno(errp, errno,
+                         "vhost-blk: unable to create uevent socket");
+        return false;
+    }
+
+    if (bind(vhost_blk_uevent_fd, (struct sockaddr *)&address,
+             sizeof(address)) < 0) {
+        error_setg_errno(errp, errno,
+                         "vhost-blk: unable to bind uevent socket");
+        qemu_close(vhost_blk_uevent_fd);
+        vhost_blk_uevent_fd = -1;
+        return false;
+    }
+
+    qemu_set_fd_handler(vhost_blk_uevent_fd, vhost_blk_uevent_read,
+                        NULL, NULL);
+    return true;
+}
+
+static void vhost_blk_uevent_cleanup_if_unused(void)
+{
+    if (vhost_blk_uevent_fd < 0 ||
+        !QLIST_EMPTY(&vhost_blk_uevent_watchers)) {
+        return;
+    }
+
+    qemu_set_fd_handler(vhost_blk_uevent_fd, NULL, NULL, NULL);
+    qemu_close(vhost_blk_uevent_fd);
+    vhost_blk_uevent_fd = -1;
+}
+
+static void vhost_blk_uevent_register(VHostBlk *s)
+{
+    s->resize_bh = qemu_bh_new(vhost_blk_resize_bh, s);
+    QLIST_INSERT_HEAD(&vhost_blk_uevent_watchers, s, uevent_node);
+    s->uevent_registered = true;
+}
+
+static void vhost_blk_uevent_unregister(VHostBlk *s)
+{
+    if (!s->uevent_registered) {
+        return;
+    }
+
+    QLIST_REMOVE(s, uevent_node);
+    s->uevent_registered = false;
+    qemu_bh_delete(s->resize_bh);
+    s->resize_bh = NULL;
+    vhost_blk_uevent_cleanup_if_unused();
+}
+
 static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
 {
     BlockConf *conf = &s->conf.conf;
@@ -254,6 +437,7 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
                    s->conf.devpath);
         goto fail;
     }
+    s->backend_rdev = st.st_rdev;
 
     if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
         error_setg_errno(errp, errno,
@@ -332,6 +516,12 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!vhost_blk_uevent_init(errp)) {
+        qemu_close(s->backend_fd);
+        s->backend_fd = -1;
+        return;
+    }
+
     s->dev.nvqs = conf->num_queues;
     s->dev.max_queues = conf->num_queues;
     s->dev.vqs = g_new0(struct vhost_virtqueue, s->dev.nvqs);
@@ -366,6 +556,7 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
         goto cleanup;
     }
 
+    vhost_blk_uevent_register(s);
     return;
 
 cleanup:
@@ -385,6 +576,7 @@ cleanup:
         virtio_del_queue(vdev, i);
     }
     virtio_cleanup(vdev);
+    vhost_blk_uevent_cleanup_if_unused();
     return;
 }
 
@@ -393,6 +585,7 @@ static void vhost_blk_device_unrealize(DeviceState *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostBlk *s = VHOST_BLK(dev);
 
+    vhost_blk_uevent_unregister(s);
     qemu_del_vm_change_state_handler(s->mighand);
     vhost_blk_set_status(vdev, 0);
     vhost_dev_cleanup(&s->dev);
diff --git a/include/hw/virtio/vhost-blk.h b/include/hw/virtio/vhost-blk.h
index c6646f5845..815939419f 100644
--- a/include/hw/virtio/vhost-blk.h
+++ b/include/hw/virtio/vhost-blk.h
@@ -14,6 +14,7 @@
 #include "standard-headers/linux/virtio_blk.h"
 #include "hw/block/block.h"
 #include "hw/virtio/vhost.h"
+#include "qemu/queue.h"
 
 #define TYPE_VHOST_BLK "vhost-blk"
 #define VHOST_BLK(obj) \
@@ -43,6 +44,10 @@ typedef struct VHostBlk {
     struct vhost_dev dev;
     bool vhost_started;
     uint64_t length;
+    uint64_t backend_rdev;
+    QEMUBH *resize_bh;
+    QLIST_ENTRY(VHostBlk) uevent_node;
+    bool uevent_registered;
 } VHostBlk;
 
 #endif
-- 
2.43.5

[QEMU HCI-8.0 PATCH 5/5] vhost-blk: filter uevents in the kernel

[Andrey Zhadchenko]

The uevent socket receives every uevent broadcast on the host:
NETLINK_KOBJECT_UEVENT group 1 has no kernel-side subscription by
subsystem or device. On a dense node every add/remove/change event
of every device (mass container starts creating dm and loop
devices, SCSI rescans, udevadm trigger) wakes up the main loop of
every QEMU with a vhost-blk device just to parse and discard the
message, and a burst can overflow the socket receive buffer.

Attach a classic BPF socket filter which passes only messages
starting with "change@" and containing a "RES"-prefixed property
within the first 512 bytes. Messages longer than the scan window
are passed to userspace instead of being dropped, so the filter
can have false positives but never false negatives:
vhost_blk_uevent_read() remains the authoritative parser. Also
enlarge the receive buffer to 1M: with the filter attached even a
large backlog consists of relevant events only.

Matching the full "RESIZE=1" property or MAJOR=/MINOR= of the
watched devices kernel-side was considered and rejected: the
kernel converts classic BPF to eBPF on attach and the converted
program must fit in BPF_MAXINSNS, which allows only ~1900 classic
instructions of this shape (three per scanned offset). Classic BPF
also cannot loop, so device numbers (variable-length decimal
strings at variable offsets) would need unrolled matching code
regenerated and re-attached on every device plug/unplug. Resize
events are rare; the coarse kernel filter drops all of the heavy
traffic and userspace keeps doing the exact matching.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 hw/block/vhost-blk.c | 123 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index f8eca4d58a..80b0e06b4c 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -28,6 +28,7 @@
 #include <sys/ioctl.h>
 #include <linux/fs.h>
 #include <linux/netlink.h>
+#include <linux/filter.h>
 #include "system/runstate.h"
 
 static int vhost_blk_uevent_fd = -1;
@@ -347,6 +348,126 @@ static void vhost_blk_uevent_read(void *opaque)
     }
 }
 
+/*
+ * The kernel broadcasts uevents of every device on the host to
+ * NETLINK_KOBJECT_UEVENT group 1 and provides no subscription by subsystem
+ * or device. Without a filter each uevent (device hotplug, SCSI rescan,
+ * udevadm trigger, ...) wakes up the main loop of every QEMU with a
+ * vhost-blk device just to parse and discard the message.
+ *
+ * Attach a classic BPF socket filter passing only what we are interested
+ * in: messages which start with "change@" and contain a property beginning
+ * with "RES" ("\0RES" match at every offset) within the first
+ * VHOST_BLK_UEVENT_SCAN_LEN bytes. Messages longer than the scan window
+ * are passed to userspace instead of being dropped. The filter can have
+ * false positives but never false negatives: vhost_blk_uevent_read()
+ * remains the authoritative parser.
+ *
+ * Matching the full "\0RESIZE=1\0" property would be nicer, but the kernel
+ * converts classic BPF to eBPF on attach and every packet load expands to
+ * several eBPF instructions; the converted program must fit in
+ * BPF_MAXINSNS (4096) instructions, which allows roughly 1900 classic
+ * instructions of this shape. Three instructions per scanned offset
+ * (load, match, accept-jump) fit with a good margin, seven do not.
+ *
+ * Filtering by MAJOR=/MINOR= of the watched devices is done in userspace
+ * only. Classic BPF cannot loop, so matching these variable-length decimal
+ * strings at variable offsets would require regenerating and re-attaching
+ * unrolled matching code on every device plug/unplug, and the instruction
+ * budget above does not allow anything close to that. Resize events are
+ * rare, all of the heavy traffic is already dropped by the "change@" and
+ * "\0RES" matches.
+ */
+#define VHOST_BLK_UEVENT_SCAN_LEN     512
+#define VHOST_BLK_UEVENT_FILTER_HEAD  10
+#define VHOST_BLK_UEVENT_FILTER_BLOCK 3
+#define VHOST_BLK_UEVENT_FILTER_INSNS (VHOST_BLK_UEVENT_FILTER_HEAD + \
+                                       VHOST_BLK_UEVENT_FILTER_BLOCK * \
+                                       VHOST_BLK_UEVENT_SCAN_LEN + 2)
+
+static void vhost_blk_uevent_apply_filter(int fd)
+{
+    g_autofree struct sock_filter *insns =
+        g_new0(struct sock_filter, VHOST_BLK_UEVENT_FILTER_INSNS);
+    const uint32_t accept = VHOST_BLK_UEVENT_FILTER_INSNS - 1;
+    struct sock_fprog prog = {
+        .len = VHOST_BLK_UEVENT_FILTER_INSNS,
+        .filter = insns,
+    };
+    int rcvbuf = 1024 * 1024;
+    uint32_t pc = 0;
+    uint32_t i;
+
+    QEMU_BUILD_BUG_ON(VHOST_BLK_UEVENT_FILTER_INSNS > BPF_MAXINSNS);
+
+    /* Drop everything which does not start with "change@" */
+    insns[pc++] = (struct sock_filter)
+        BPF_STMT(BPF_LD | BPF_W | BPF_ABS, 0);
+    insns[pc++] = (struct sock_filter)
+        BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0x6368616e /* "chan" */, 0, 4);
+    insns[pc++] = (struct sock_filter)
+        BPF_STMT(BPF_LD | BPF_H | BPF_ABS, 4);
+    insns[pc++] = (struct sock_filter)
+        BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0x6765 /* "ge" */, 0, 2);
+    insns[pc++] = (struct sock_filter)
+        BPF_STMT(BPF_LD | BPF_B | BPF_ABS, 6);
+    insns[pc++] = (struct sock_filter)
+        BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, '@', 1, 0);
+    insns[pc++] = (struct sock_filter)
+        BPF_STMT(BPF_RET | BPF_K, 0);
+
+    /*
+     * A message longer than the scan window cannot be scanned completely:
+     * pass it to userspace instead of risking a lost resize event.
+     */
+    insns[pc++] = (struct sock_filter)
+        BPF_STMT(BPF_LD | BPF_W | BPF_LEN, 0);
+    insns[pc++] = (struct sock_filter)
+        BPF_JUMP(BPF_JMP | BPF_JGT | BPF_K, VHOST_BLK_UEVENT_SCAN_LEN, 0, 1);
+    insns[pc] = (struct sock_filter)
+        BPF_STMT(BPF_JMP | BPF_JA, accept - pc - 1);
+    pc++;
+
+    /*
+     * Scan for "\0RES" at every offset. A load beyond the end of the
+     * message terminates the program with a drop verdict, which is
+     * correct: had the message contained the pattern, it would have been
+     * matched at an earlier, in-bounds offset.
+     */
+    for (i = 0; i < VHOST_BLK_UEVENT_SCAN_LEN; i++) {
+        insns[pc++] = (struct sock_filter)
+            BPF_STMT(BPF_LD | BPF_W | BPF_ABS, i);
+        insns[pc++] = (struct sock_filter)
+            BPF_JUMP(BPF_JMP | BPF_JEQ | BPF_K, 0x00524553 /* "\0RES" */,
+                     0, 1);
+        insns[pc] = (struct sock_filter)
+            BPF_STMT(BPF_JMP | BPF_JA, accept - pc - 1);
+        pc++;
+    }
+
+    /* Drop */
+    insns[pc++] = (struct sock_filter)BPF_STMT(BPF_RET | BPF_K, 0);
+    /* Accept */
+    insns[pc++] = (struct sock_filter)BPF_STMT(BPF_RET | BPF_K, 0xffffffff);
+    assert(pc == VHOST_BLK_UEVENT_FILTER_INSNS);
+
+    if (setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, &prog, sizeof(prog))) {
+        warn_report("vhost-blk: unable to attach uevent filter: %s",
+                    strerror(errno));
+    }
+
+    /*
+     * Make the socket resilient to main loop stalls. With the filter
+     * attached even a large backlog consists of relevant events only.
+     */
+    if (setsockopt(fd, SOL_SOCKET, SO_RCVBUFFORCE,
+                   &rcvbuf, sizeof(rcvbuf)) &&
+        setsockopt(fd, SOL_SOCKET, SO_RCVBUF, &rcvbuf, sizeof(rcvbuf))) {
+        warn_report("vhost-blk: unable to enlarge uevent socket buffer: %s",
+                    strerror(errno));
+    }
+}
+
 static bool vhost_blk_uevent_init(Error **errp)
 {
     struct sockaddr_nl address = {
@@ -367,6 +488,8 @@ static bool vhost_blk_uevent_init(Error **errp)
         return false;
     }
 
+    vhost_blk_uevent_apply_filter(vhost_blk_uevent_fd);
+
     if (bind(vhost_blk_uevent_fd, (struct sockaddr *)&address,
              sizeof(address)) < 0) {
         error_setg_errno(errp, errno,
-- 
2.43.5

Re: [QEMU HCI-8.0 PATCH 1/5] blk: factor out validation

[Andrey Drobyshev]

Nit: "blk:" is never used as a prefix, better be "block:".

> Preparational patch. This helper will be used in the next patches
> to validate vhost-blk block parameters.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-143437
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/hw/block/block.c b/hw/block/block.c
> index 47897ca5d3c..6f135b7e2f9 100644
> --- a/hw/block/block.c
> +++ b/hw/block/block.c
> @@ -162,6 +162,11 @@ bool blkconf_blocksizes(BlockConf *conf, Error **errp)
>          }
>      }
>  
> +    return blkconf_validate_blocksizes(conf, errp);
> +}
> +
> +bool blkconf_validate_blocksizes(BlockConf *conf, Error **errp)
> +{

When calling blkconf_blocksizes() -> blkconf_validate_blocksizes(), we
have conf->logical_block_size preset.  But since we turn
blkconf_validate_blocksizes() into a separate helper, this precondition
isn't true in general.  And we do further:

    conf->min_io_size / conf->logical_block_size

- potential division by zero.

Andrey

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

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

[Andrey Drobyshev]

> 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>

Re: [QEMU HCI-8.0 PATCH 3/5] vhost-blk: add read-only flag

[Andrey Drobyshev]

> and set RO respectively. Also compare BLKROGET with the selected

Nit: make commit message start with their own sentence.

> mode and reject r/w if needed.
> 
> 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 a3e0010982f..47b6e560d47 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -24,6 +24,7 @@
>  #include "system/system.h"
>  #include "linux-headers/linux/vhost.h"
>  #include <sys/ioctl.h>
> +#include <linux/fs.h>

Previous patch removes the header, now we add it back.  Let's just not
touch it.

>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
> @@ -233,8 +234,10 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>  {
>      BlockConf *conf = &s->conf.conf;
>      struct stat st;
> +    int readonly;
> +    int open_flags = s->conf.readonly ? O_RDONLY : O_RDWR;
>  
> -    s->backend_fd = qemu_open(s->conf.devpath, O_RDWR, errp);
> +    s->backend_fd = qemu_open(s->conf.devpath, open_flags, errp);
>      if (s->backend_fd < 0) {
>          error_prepend(errp, "vhost-blk: unable to open backend: ");
>          return false;
> @@ -252,6 +255,19 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>          goto fail;
>      }
>  
> +    if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "vhost-blk: unable to get read-only status of '%s'",
> +                         s->conf.devpath);
> +        goto fail;
> +    }
> +
> +    if (readonly && !s->conf.readonly) {
> +        error_setg(errp, "vhost-blk: '%s' is read-only",
> +                   s->conf.devpath);
> +        goto fail;
> +    }
> +

How about doing it similarly to file-posix:

    if (!s->conf.readonly) {
        if (ioctl(s->backend_fd, BLKROGET, &readonly) < 0) {
            error_setg_errno(errp, errno,
                             "vhost-blk: unable to get read-only status of "
                             "'%s'", s->conf.devpath);
            goto fail;
         }

         if (readonly) {
             error_setg_errno(errp, EROFS, "The device is not writable");
             goto fail;
         }
    }

In addition: conf.readonly value comes from libvirt.  conf.devpath is
also provided by libvirt.  Shouldn't we check BLKROGET early on and fail
in libvirt instead of waiting till here?  I'd prefer having both checks.
Leave the check here, but fail early on in libvirt.

Andrey

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

Re: [QEMU HCI-8.0 PATCH 4/5] vhost-blk: watch the device for resize events

[Andrey Drobyshev]

The approach introduced by this and subsequent patches, i.e. subscribing
to NETLINK_KOBJECT_UEVENT and filtering them - that looks very kludgy
for production and will unlikely be ever accepted upstream.

So, QEMU must get the knowledge about updated size.  It can only do so
by getting this knowledge from libvirt (as it used to be with
qmp_block_resize) or from the kernel via some kind of notification.

So our options:

1. Another QMP command sent directly from libvirt, smth like
'vhost-blk-config-update id=virtio-disk0 data={...}'

2. Patching the kernel so it knows the size, caches it.  Making ploop
send an ioctl to inform the kernel about new size.  Then establishing
eventfd from vhost-blk backend -> qemu, and processing the events.

I think 1st is cleaner and requires less work.

Andrey

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

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

[Andrey Zhadchenko]

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.

> 
>>   
>> -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?

> 
> Andrey
> 

Re: [QEMU HCI-8.0 PATCH 4/5] vhost-blk: watch the device for resize events

[Andrey Zhadchenko]

On 9/3/26 16:56, Andrey Drobyshev wrote:
> The approach introduced by this and subsequent patches, i.e. subscribing
> to NETLINK_KOBJECT_UEVENT and filtering them - that looks very kludgy
> for production and will unlikely be ever accepted upstream.

I also think qmp command is much cleaner.

> 
> So, QEMU must get the knowledge about updated size.  It can only do so
> by getting this knowledge from libvirt (as it used to be with
> qmp_block_resize) or from the kernel via some kind of notification.

Updated size can be read again with llseek/BLKSZGET. I would avoid 
passing it into QEMU explicitly via QMP (we also don't pass it during 
the start, so it matches)

> 
> So our options:
> 
> 1. Another QMP command sent directly from libvirt, smth like
> 'vhost-blk-config-update id=virtio-disk0 data={...}'
> 
> 2. Patching the kernel so it knows the size, caches it.  Making ploop
> send an ioctl to inform the kernel about new size.  Then establishing
> eventfd from vhost-blk backend -> qemu, and processing the events.
> 
> I think 1st is cleaner and requires less work.
> 
> Andrey
> 

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

[Andrey Drobyshev]

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

Re: [QEMU HCI-8.0 PATCH 4/5] vhost-blk: watch the device for resize events

[Andrey Drobyshev]

On 9/3/26 6:30 PM, Andrey Zhadchenko wrote:
> 
> 
> On 9/3/26 16:56, Andrey Drobyshev wrote:
>> The approach introduced by this and subsequent patches, i.e. subscribing
>> to NETLINK_KOBJECT_UEVENT and filtering them - that looks very kludgy
>> for production and will unlikely be ever accepted upstream.
> 
> I also think qmp command is much cleaner.
> 
>>
>> So, QEMU must get the knowledge about updated size.  It can only do so
>> by getting this knowledge from libvirt (as it used to be with
>> qmp_block_resize) or from the kernel via some kind of notification.
> 
> Updated size can be read again with llseek/BLKSZGET. I would avoid 
> passing it into QEMU explicitly via QMP (we also don't pass it during 
> the start, so it matches)

Currently libvirt calls QMP block-resize which has size as an explicit
argument.  You're saying we should just issue a command with no args,
smth like 'qmp_trigger_size_reread'.  That's generally fine, but what if
for some reason final size which we get by lseek() doesn't match what
was requested from libvirt? When passing an argument we can at least
compare the expectation vs reality and emit an error.

> 
>>
>> So our options:
>>
>> 1. Another QMP command sent directly from libvirt, smth like
>> 'vhost-blk-config-update id=virtio-disk0 data={...}'
>>
>> 2. Patching the kernel so it knows the size, caches it.  Making ploop
>> send an ioctl to inform the kernel about new size.  Then establishing
>> eventfd from vhost-blk backend -> qemu, and processing the events.
>>
>> I think 1st is cleaner and requires less work.
>>
>> Andrey
>>
> 

Re: [QEMU HCI-8.0 PATCH 4/5] vhost-blk: watch the device for resize events

[Andrey Zhadchenko]

On 9/3/26 17:45, Andrey Drobyshev wrote:
> On 9/3/26 6:30 PM, Andrey Zhadchenko wrote:
>>
>>
>> On 9/3/26 16:56, Andrey Drobyshev wrote:
>>> The approach introduced by this and subsequent patches, i.e. subscribing
>>> to NETLINK_KOBJECT_UEVENT and filtering them - that looks very kludgy
>>> for production and will unlikely be ever accepted upstream.
>>
>> I also think qmp command is much cleaner.
>>
>>>
>>> So, QEMU must get the knowledge about updated size.  It can only do so
>>> by getting this knowledge from libvirt (as it used to be with
>>> qmp_block_resize) or from the kernel via some kind of notification.
>>
>> Updated size can be read again with llseek/BLKSZGET. I would avoid
>> passing it into QEMU explicitly via QMP (we also don't pass it during
>> the start, so it matches)
> 
> Currently libvirt calls QMP block-resize which has size as an explicit
> argument.  You're saying we should just issue a command with no args,
> smth like 'qmp_trigger_size_reread'.  That's generally fine, but what if
> for some reason final size which we get by lseek() doesn't match what
> was requested from libvirt? When passing an argument we can at least
> compare the expectation vs reality and emit an error.

I don't think that detecting size mismatch makes much sense. It won't 
protect us against concurrent resize anyway (it can happen after lseek, 
etc.) and just increase the amount of code.
In my opinion it is perfectly fine to infer the size from the device 
instead of doing it + also checking against user input.

> 
>>
>>>
>>> So our options:
>>>
>>> 1. Another QMP command sent directly from libvirt, smth like
>>> 'vhost-blk-config-update id=virtio-disk0 data={...}'
>>>
>>> 2. Patching the kernel so it knows the size, caches it.  Making ploop
>>> send an ioctl to inform the kernel about new size.  Then establishing
>>> eventfd from vhost-blk backend -> qemu, and processing the events.
>>>
>>> I think 1st is cleaner and requires less work.
>>>
>>> Andrey
>>>
>>
>