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

[QEMU HCI-8.0 PATCH v2 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 descripor
comes prepared with some filter from outside.

v2:
patch#1
 - old patch dropped

patch#2
 - moved double close fix to new 'do not double close vhostfd' patch
 - dropped DEFINE_BLOCK_PROPERTIES_BASE exposition to the outside. Let's
just use defaults for now. We can later check if tweaking this values
makes any difference and add them back.
 - change vhost_blk_update_size() to return zero or error and move 'changed'
to a separate argument
 - changed lseek to BLKGETSIZE64 (we already use BLKROGET anyway)

patch#3
 - check BLKROGET only if rw was requested

patch#4
 - rework patch a bit: now we expect opened and filtered fd from
libvirt.

patch#5
 - old one dropped, setup moved to libvirt
 - added new patch to support uevent socket cpr


Andrey Zhadchenko (5):
  vhost-blk: do not double close vhostfd
  vhost-blk: change backend setup
  vhost-blk: add read-only flag
  vhost-blk: watch the device for resize events
  vhost-blk: preserve the uevent socket across cpr-exec

 hw/block/vhost-blk.c          | 343 +++++++++++++++++++++++++++++-----
 include/hw/virtio/vhost-blk.h |  10 +-
 2 files changed, 307 insertions(+), 46 deletions(-)

-- 
2.43.5

[QEMU HCI-8.0 PATCH v2 1/5] vhost-blk: do not double close vhostfd

[Andrey Zhadchenko]

vhost_dev_init() closes the backend fd on failure, so the cleanup
path of vhost_blk_device_realize() closed vhostfd for the second
time. The cleanup path is also reachable with vhostfd never opened,
in which case we called close(-1).

Fixes: 0ff3cb5ff2 ("block: add vhost-blk backend")
https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
 hw/block/vhost-blk.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index c52851fcf8..9bd49fef2d 100644
--- a/hw/block/vhost-blk.c
+++ b/hw/block/vhost-blk.c
@@ -318,6 +318,8 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
     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 +330,10 @@ 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;
+    }
     for (i = 0; i < conf->num_queues; i++) {
         virtio_del_queue(vdev, i);
     }
-- 
2.43.5

Re: [QEMU HCI-8.0 PATCH v2 1/5] vhost-blk: do not double close vhostfd

[Andrey Drobyshev]

> vhost_dev_init() closes the backend fd on failure, so the cleanup
> path of vhost_blk_device_realize() closed vhostfd for the second
> time. The cleanup path is also reachable with vhostfd never opened,
> in which case we called close(-1).
> 
> Fixes: 0ff3cb5ff2 ("block: add vhost-blk backend")
> 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..9bd49fef2da 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -318,6 +318,8 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
>      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 +330,10 @@ cleanup:
>              qemu_del_vm_change_state_handler(s->mighand);
>      }
>      g_free(s->dev.vqs);

Not related, but while we're here:
  vhost_blk_device_realize()
    s->dev.vqs = g_new0()
    vhost_dev_init() fails ->
      vhost_dev_cleanup(s->dev)
        memset(s->dev, 0)
    g_free(s->dev.vqs)

Result: g_free(NULL) does nothing, array allocated with g_new0() leaks.
Worth another patch.  Or if it "Fixes:" the same commit - might fold
into the same patch.

Andrey

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

[QEMU HCI-8.0 PATCH v2 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, etc. Don't
be too broad and just do the minimal work. Drop the generic block
device properties along with the block node: the kernel module
does all IO in terms of 512 sectors, so simply report 512 byte
logical/physical block size to the guest.
Also we lose resize, as this is tied to the block node, which is
now have no place in the setup. We will add this in the next
patches as well as RO mode.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
v2:
 - moved double close fix to  'do not double close vhostfd' patch
 - dropped DEFINE_BLOCK_PROPERTIES_BASE exposition to the outside. Let's
just use defaults for now. We can later check if tweaking this values
makes any difference and add them back.
 - change vhost_blk_update_size() to return zero or error and move 'changed'
to a separate argument
 - changed lseek to BLKGETSIZE64 (we already use BLKROGET anyway)

 hw/block/vhost-blk.c          | 126 ++++++++++++++++++++--------------
 include/hw/virtio/vhost-blk.h |   5 +-
 2 files changed, 78 insertions(+), 53 deletions(-)

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

Re: [QEMU HCI-8.0 PATCH v2 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, etc. Don't
> be too broad and just do the minimal work. Drop the generic block
> device properties along with the block node: the kernel module
> does all IO in terms of 512 sectors, so simply report 512 byte
> logical/physical block size to the guest.
> Also we lose resize, as this is tied to the block node, which is
> now have no place in the setup. We will add this in the next
> patches as well as RO mode.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-143437
> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
>
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index 9bd49fef2da..24f4fbe2b68 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -24,23 +24,17 @@
>  #include "system/system.h"
>  #include "linux-headers/linux/vhost.h"
>  #include <sys/ioctl.h>
> -#include <linux/fs.h>

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

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

This is redundant, error_setg_errno() preserves errno value.

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

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

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

Removing these props currently results into:

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

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

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

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

Andrey

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

[QEMU HCI-8.0 PATCH v2 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>
---
v2:
 - check BLKROGET only if rw was requested like posix_file

 hw/block/vhost-blk.c          | 26 +++++++++++++++++++++++++-
 include/hw/virtio/vhost-blk.h |  1 +
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index 24f4fbe2b6..04a7013e52 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)
@@ -232,8 +233,9 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
     BlockConf *conf = &s->conf.conf;
     struct stat st;
     bool changed;
+    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;
@@ -251,6 +253,23 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
         goto fail;
     }
 
+    if (!s->conf.readonly) {
+        int 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, "vhost-blk: '%s' is not writable",
+                             s->conf.devpath);
+            goto fail;
+        }
+    }
+
     if (vhost_blk_update_size(s, &changed, errp) < 0) {
         goto fail;
     }
@@ -406,6 +425,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);
     }
@@ -453,6 +476,7 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static const Property vhost_blk_properties[] = {
     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

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

[Andrey Drobyshev]

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

Please have commit message start their own sentences.

> 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 24f4fbe2b68..04a7013e521 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>

Header belongs to patch 2.

>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
> @@ -232,8 +233,9 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>      BlockConf *conf = &s->conf.conf;
>      struct stat st;
>      bool changed;
> +    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;
> @@ -251,6 +253,23 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
>          goto fail;
>      }
>  
> +    if (!s->conf.readonly) {
> +        int 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, "vhost-blk: '%s' is not writable",
> +                             s->conf.devpath);
> +            goto fail;
> +        }
> +    }
> +
>      if (vhost_blk_update_size(s, &changed, errp) < 0) {
>          goto fail;
>      }
> @@ -406,6 +425,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);
>      }
> @@ -453,6 +476,7 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>  
>  static const Property vhost_blk_properties[] = {
>      DEFINE_PROP_STRING("devpath", VHostBlk, conf.devpath),
> +    DEFINE_PROP_BOOL("read-only", VHostBlk, conf.readonly, false),

Not for this patch, but related: AFAIU currently "<readonly/>" in domain
XML is applied by libvirt to the block node.  Correct?  If so - we also
need to patch libvirt making sure it applies to the device.

Andrey

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

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

[Andrey Zhadchenko]

Resize was tied to block node, which we removed a few patches ago.
Luckily we can make resize automated: receive an uevent socket
from the management layer via the new "ueventfd" property (e.g. a
/dev/fdset/N path), watch it for relevant netlink messages and
call virtio_notify_config() if we detect a capacity change.

The socket is set up (possibly with filter) by management layer.
We only need to check that it is netlink and do some message
filtering.

When the property is not set, capacity changes are not detected.

https://virtuozzo.atlassian.net/browse/VSTOR-143437
Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>
---
v2:
 - rework patch a bit: now we expect opened and filtered fd from
libvirt.

 hw/block/vhost-blk.c          | 181 ++++++++++++++++++++++++++++++++++
 include/hw/virtio/vhost-blk.h |   4 +
 2 files changed, 185 insertions(+)

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index 04a7013e52..7cb842a859 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,6 +27,7 @@
 #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_start(VirtIODevice *vdev)
@@ -228,6 +231,173 @@ static int vhost_blk_update_size(VHostBlk *s, bool *changed, Error **errp)
     return 0;
 }
 
+static void vhost_blk_resize_bh(void *opaque)
+{
+    VHostBlk *s = opaque;
+    Error *local_err = NULL;
+    bool changed;
+
+    if (vhost_blk_update_size(s, &changed, &local_err) < 0) {
+        error_report_err(local_err);
+        return;
+    }
+
+    if (changed) {
+        virtio_notify_config(VIRTIO_DEVICE(s));
+    }
+}
+
+/*
+ * The uevent socket is created, bound and filtered by the management
+ * layer and passed to us via the "ueventfd" property.
+ */
+static void vhost_blk_uevent_read(void *opaque)
+{
+    VHostBlk *s = 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(s->uevent_fd, buffer, sizeof(buffer) - 1,
+                       MSG_DONTWAIT, (struct sockaddr *)&source, &source_len);
+        if (len < 0) {
+            if (errno == EINTR) {
+                continue;
+            }
+            if (errno == ENOBUFS) {
+                /* Some events may be dropped, just re-check */
+                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 == major(s->backend_rdev) &&
+            event_minor == minor(s->backend_rdev)) {
+            qemu_bh_schedule(s->resize_bh);
+        }
+    }
+}
+
+static bool vhost_blk_uevent_check(int fd, const char *src, Error **errp)
+{
+    socklen_t optlen;
+    int domain;
+    int protocol;
+
+    optlen = sizeof(domain);
+    if (getsockopt(fd, SOL_SOCKET, SO_DOMAIN, &domain, &optlen) < 0) {
+        error_setg_errno(errp, errno, "vhost-blk: '%s' is not a socket", src);
+        return false;
+    }
+
+    optlen = sizeof(protocol);
+    if (getsockopt(fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen) < 0) {
+        error_setg_errno(errp, errno,
+                         "vhost-blk: unable to get protocol of '%s'", src);
+        return false;
+    }
+
+    if (domain != AF_NETLINK || protocol != NETLINK_KOBJECT_UEVENT) {
+        error_setg(errp,
+                   "vhost-blk: '%s' is not a NETLINK_KOBJECT_UEVENT socket",
+                   src);
+        return false;
+    }
+
+    return true;
+}
+
+static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
+{
+    if (!s->conf.ueventfd) {
+        return true;
+    }
+
+    s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);
+    if (s->uevent_fd < 0) {
+        error_prepend(errp, "vhost-blk: unable to open uevent socket: ");
+        return false;
+    }
+
+    if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {
+        qemu_close(s->uevent_fd);
+        s->uevent_fd = -1;
+        return false;
+    }
+
+    s->resize_bh = qemu_bh_new(vhost_blk_resize_bh, s);
+    qemu_set_fd_handler(s->uevent_fd, vhost_blk_uevent_read, NULL, s);
+    return true;
+}
+
+static void vhost_blk_uevent_detach(VHostBlk *s)
+{
+    if (s->uevent_fd < 0) {
+        return;
+    }
+
+    qemu_set_fd_handler(s->uevent_fd, NULL, NULL, NULL);
+    qemu_bh_delete(s->resize_bh);
+    s->resize_bh = NULL;
+    qemu_close(s->uevent_fd);
+    s->uevent_fd = -1;
+}
+
 static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
 {
     BlockConf *conf = &s->conf.conf;
@@ -252,6 +422,7 @@ static bool vhost_blk_open_backend(VHostBlk *s, Error **errp)
                    s->conf.devpath);
         goto fail;
     }
+    s->backend_rdev = st.st_rdev;
 
     if (!s->conf.readonly) {
         int readonly;
@@ -325,6 +496,12 @@ static void vhost_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (!vhost_blk_uevent_attach(s, 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);
@@ -374,6 +551,7 @@ cleanup:
         qemu_close(s->backend_fd);
         s->backend_fd = -1;
     }
+    vhost_blk_uevent_detach(s);
     for (i = 0; i < conf->num_queues; i++) {
         virtio_del_queue(vdev, i);
     }
@@ -386,6 +564,7 @@ static void vhost_blk_device_unrealize(DeviceState *dev)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostBlk *s = VHOST_BLK(dev);
 
+    vhost_blk_uevent_detach(s);
     qemu_del_vm_change_state_handler(s->mighand);
     vhost_blk_set_status(vdev, 0);
     vhost_dev_cleanup(&s->dev);
@@ -476,6 +655,7 @@ static void vhost_blk_update_config(VirtIODevice *vdev, uint8_t *config)
 
 static const Property vhost_blk_properties[] = {
     DEFINE_PROP_STRING("devpath", VHostBlk, conf.devpath),
+    DEFINE_PROP_STRING("ueventfd", VHostBlk, conf.ueventfd),
     DEFINE_PROP_BOOL("read-only", VHostBlk, conf.readonly, false),
     DEFINE_PROP_UINT16("num-queues", VHostBlk, conf.num_queues,
                        VHOST_BLK_AUTO_NUM_QUEUES),
@@ -523,6 +703,7 @@ static void vhost_blk_instance_init(Object *obj)
 
     s->vhostfd = -1;
     s->backend_fd = -1;
+    s->uevent_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 c6646f5845..e3ad92bc6e 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;
+    char *ueventfd;
     bool readonly;
     uint16_t num_queues;
     uint16_t queue_size;
@@ -40,9 +41,12 @@ typedef struct VHostBlk {
     uint64_t decided_features;
     int vhostfd;
     int backend_fd;
+    int uevent_fd;
     struct vhost_dev dev;
     bool vhost_started;
     uint64_t length;
+    uint64_t backend_rdev;
+    QEMUBH *resize_bh;
 } VHostBlk;
 
 #endif
-- 
2.43.5

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

[Andrey Drobyshev]

> Resize was tied to block node, which we removed a few patches ago.
> Luckily we can make resize automated: receive an uevent socket
> from the management layer via the new "ueventfd" property (e.g. a
> /dev/fdset/N path), watch it for relevant netlink messages and
> call virtio_notify_config() if we detect a capacity change.
> 
> The socket is set up (possibly with filter) by management layer.
> We only need to check that it is netlink and do some message
> filtering.
> 
> When the property is not set, capacity changes are not detected.

So libvirt must ALWAYS set it, and we fail in .realize() if it's not
set.  Correct?  Let's mention it.

> 
> 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 04a7013e521..7cb842a859e 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,6 +27,7 @@
>  #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_start(VirtIODevice *vdev)
> @@ -228,6 +231,173 @@ static int vhost_blk_update_size(VHostBlk *s, bool *changed, Error **errp)
>      return 0;
>  }
>  
> +static void vhost_blk_resize_bh(void *opaque)
> +{
> +    VHostBlk *s = opaque;
> +    Error *local_err = NULL;
> +    bool changed;
> +
> +    if (vhost_blk_update_size(s, &changed, &local_err) < 0) {
> +        error_report_err(local_err);
> +        return;
> +    }
> +
> +    if (changed) {
> +        virtio_notify_config(VIRTIO_DEVICE(s));
> +    }
> +}
> +
> +/*
> + * The uevent socket is created, bound and filtered by the management
> + * layer and passed to us via the "ueventfd" property.
> + */
> +static void vhost_blk_uevent_read(void *opaque)
> +{
> +    VHostBlk *s = opaque;
> +    char buffer[64 * 1024 + 1];

include/linux/kobject.h
33:#define UEVENT_BUFFER_SIZE           2048    /* buffer for the variables */

Looks like 64K is overkill, maybe 4K?

> +
> +    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(s->uevent_fd, buffer, sizeof(buffer) - 1,
> +                       MSG_DONTWAIT, (struct sockaddr *)&source, &source_len);
> +        if (len < 0) {
> +            if (errno == EINTR) {
> +                continue;
> +            }
> +            if (errno == ENOBUFS) {
> +                /* Some events may be dropped, just re-check */
> +                qemu_bh_schedule(s->resize_bh);
> +                continue;
> +            }
> +            if (errno != EAGAIN && errno != EWOULDBLOCK) {

Should ENOMEM go here to?  And if it's an error that breaks the socket,
like EBADF or smth else - maybe spit the error and detach the handler?
What's the point of keeping it afterwards?

> +                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 == major(s->backend_rdev) &&
> +            event_minor == minor(s->backend_rdev)) {
> +            qemu_bh_schedule(s->resize_bh);
> +        }
> +    }
> +}
> +
> +static bool vhost_blk_uevent_check(int fd, const char *src, Error **errp)
> +{
> +    socklen_t optlen;
> +    int domain;
> +    int protocol;
> +
> +    optlen = sizeof(domain);
> +    if (getsockopt(fd, SOL_SOCKET, SO_DOMAIN, &domain, &optlen) < 0) {
> +        error_setg_errno(errp, errno, "vhost-blk: '%s' is not a socket", src);
> +        return false;
> +    }
> +
> +    optlen = sizeof(protocol);
> +    if (getsockopt(fd, SOL_SOCKET, SO_PROTOCOL, &protocol, &optlen) < 0) {
> +        error_setg_errno(errp, errno,
> +                         "vhost-blk: unable to get protocol of '%s'", src);
> +        return false;
> +    }
> +
> +    if (domain != AF_NETLINK || protocol != NETLINK_KOBJECT_UEVENT) {
> +        error_setg(errp,
> +                   "vhost-blk: '%s' is not a NETLINK_KOBJECT_UEVENT socket",
> +                   src);
> +        return false;
> +    }

How about also checking nl_groups? Should be 1 for multicast.  I.e.

  if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) < 0) {
      error_setg_errno(...);
      return false;
  }
  if (addr.nl_groups != 1) {
      error_setg(errp, "vhost-blk: '%s' is not subscribed to kernel uevents",
                 src);
      return false;
  }

> +
> +    return true;
> +}
> +
> +static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
> +{
> +    if (!s->conf.ueventfd) {
> +        return true;
> +    }
> +
> +    s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);

For conf values monitor_fd_param() is usually used, so how about

  s->uevent_fd = monitor_fd_param(monitor_cur(), s->conf.ueventfd, errp);

Andrey

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

[QEMU HCI-8.0 PATCH v2 5/5] vhost-blk: preserve the uevent socket across cpr-exec

[Andrey Zhadchenko]

qemu-update uses cpr-exec migration: QEMU re-execs itself in place.
A plain monitor fdset descriptor like our uevent socket does not
survive that (unlike the cpr_save_fd()'d tap/vhost fds), so without
help the resized-device notifications would silently stop until the
next full VM start.

Preserve it the same way the net backends do: on cold boot
cpr_save_fd() the socket under the device's canonical path, and on
the re-exec'd (incoming) side adopt it with cpr_find_fd() instead of
reopening a command line fd that no longer resolves. The management
layer passes a placeholder for the "ueventfd" property in that case.
Drop the saved descriptor on teardown so it does not outlive the
device.

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

diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
index 7cb842a859..eca12356ae 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 "migration/cpr.h"
 #include "system/runstate.h"
 
 static int vhost_blk_start(VirtIODevice *vdev)
@@ -364,20 +365,36 @@ static bool vhost_blk_uevent_check(int fd, const char *src, Error **errp)
 
 static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
 {
+    g_autofree char *cpr_name = NULL;
+
     if (!s->conf.ueventfd) {
         return true;
     }
 
-    s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);
-    if (s->uevent_fd < 0) {
-        error_prepend(errp, "vhost-blk: unable to open uevent socket: ");
-        return false;
-    }
+    cpr_name = object_get_canonical_path(OBJECT(s));
 
-    if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {
-        qemu_close(s->uevent_fd);
-        s->uevent_fd = -1;
-        return false;
+    if (cpr_is_incoming()) {
+        s->uevent_fd = cpr_find_fd(cpr_name, 0);
+        if (s->uevent_fd < 0) {
+            error_setg(errp,
+                       "vhost-blk: no preserved uevent socket to restore");
+            return false;
+        }
+    } else {
+        s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);
+        if (s->uevent_fd < 0) {
+            error_prepend(errp, "vhost-blk: unable to open uevent socket: ");
+            return false;
+        }
+
+        if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {
+            qemu_close(s->uevent_fd);
+            s->uevent_fd = -1;
+            return false;
+        }
+
+        /* Preserve the socket across a future cpr-exec qemu-update. */
+        cpr_save_fd(cpr_name, 0, s->uevent_fd);
     }
 
     s->resize_bh = qemu_bh_new(vhost_blk_resize_bh, s);
@@ -387,10 +404,14 @@ static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
 
 static void vhost_blk_uevent_detach(VHostBlk *s)
 {
+    g_autofree char *cpr_name = NULL;
+
     if (s->uevent_fd < 0) {
         return;
     }
 
+    cpr_name = object_get_canonical_path(OBJECT(s));
+    cpr_delete_fd(cpr_name, 0);
     qemu_set_fd_handler(s->uevent_fd, NULL, NULL, NULL);
     qemu_bh_delete(s->resize_bh);
     s->resize_bh = NULL;
-- 
2.43.5

Re: [QEMU HCI-8.0 PATCH v2 5/5] vhost-blk: preserve the uevent socket across cpr-exec

[Andrey Drobyshev]

> qemu-update uses cpr-exec migration: QEMU re-execs itself in place.
> A plain monitor fdset descriptor like our uevent socket does not
> survive that (unlike the cpr_save_fd()'d tap/vhost fds), so without
> help the resized-device notifications would silently stop until the
> next full VM start.
> 
> Preserve it the same way the net backends do: on cold boot
> cpr_save_fd() the socket under the device's canonical path, and on
> the re-exec'd (incoming) side adopt it with cpr_find_fd() instead of
> reopening a command line fd that no longer resolves. The management
> layer passes a placeholder for the "ueventfd" property in that case.
> Drop the saved descriptor on teardown so it does not outlive the
> device.
> 
> 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 7cb842a859e..eca12356ae5 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 "migration/cpr.h"
>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
> @@ -364,20 +365,36 @@ static bool vhost_blk_uevent_check(int fd, const char *src, Error **errp)
>  
>  static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
>  {
> +    g_autofree char *cpr_name = NULL;
> +
>      if (!s->conf.ueventfd) {
>          return true;
>      }
>  
> -    s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);
> -    if (s->uevent_fd < 0) {
> -        error_prepend(errp, "vhost-blk: unable to open uevent socket: ");
> -        return false;
> -    }
> +    cpr_name = object_get_canonical_path(OBJECT(s));

That name is gonna be "/machine/peripheral/...".  Let's do CPR
consistently with other devices.  E.g. see how it's done in
vhost_vsock_device_realize():

  DeviceState *proxy = qdev_get_parent_bus(DEVICE(vsock))->parent;
  ...
  /* Add migration blockers if proxy->id isn't present */

For vhost-blk proxy->id is likely gonna be "virtio-disk0".

Also, in this case, since it's not a vhostfd or backend FD, but an
ueventfd, I'd prefer adding it as a suffix, as it's done for other
devices.  So CPR key should end up looking like "virtio-disk0_ueventfd".

>  
> -    if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {
> -        qemu_close(s->uevent_fd);
> -        s->uevent_fd = -1;
> -        return false;
> +    if (cpr_is_incoming()) {
> +        s->uevent_fd = cpr_find_fd(cpr_name, 0);
> +        if (s->uevent_fd < 0) {
> +            error_setg(errp,
> +                       "vhost-blk: no preserved uevent socket to restore");
> +            return false;
> +        }
> +    } else {
> +        s->uevent_fd = qemu_open(s->conf.ueventfd, O_RDWR, errp);
> +        if (s->uevent_fd < 0) {
> +            error_prepend(errp, "vhost-blk: unable to open uevent socket: ");
> +            return false;
> +        }
> +
> +        if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {

Don't we want the same validation for cpr_is_incoming() case?

Andrey

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