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

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

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