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

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 9/4/26 17:33, Andrey Drobyshev wrote:
>> 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.

This is an optional field. If it is not set, nothing happens:
vhost_blk_uevent_attach() returns true and realize goes on.

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

ENOMEM is expected only for recvmsg.

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