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

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

> 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 other devices do: on cold boot
> cpr_save_fd() the socket under the "<proxy id>_ueventfd" key, 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.  The restored socket is validated just like
> a cold-boot one.
> 
> Since a unique device ID is mandatory for FD preservation, block
> CPR migration modes when the proxy has no ID, as other devices do.
> 
> 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 a69bdcb551f..fa951da47a6 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -29,6 +29,8 @@
>  #include <sys/ioctl.h>
>  #include <linux/fs.h>
>  #include <linux/netlink.h>
> +#include "migration/blocker.h"
> +#include "migration/cpr.h"
>  #include "system/runstate.h"
>  
>  static int vhost_blk_start(VirtIODevice *vdev)
> @@ -248,12 +250,27 @@ static void vhost_blk_resize_bh(void *opaque)
>      }
>  }
>  
> +static char *vhost_blk_cpr_name(VHostBlk *s)
> +{
> +    DeviceState *proxy = qdev_get_parent_bus(DEVICE(s))->parent;
> +
> +    return proxy->id ? g_strdup_printf("%s_ueventfd", proxy->id) : NULL;
> +}
> +
>  static void vhost_blk_uevent_detach(VHostBlk *s)
>  {
> +    g_autofree char *cpr_name = NULL;
> +
> +    migrate_del_blocker(&s->migration_blocker);
> +
>      if (s->uevent_fd < 0) {
>          return;
>      }
>  
> +    cpr_name = vhost_blk_cpr_name(s);
> +    if (cpr_name) {
> +        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;
> @@ -395,25 +412,71 @@ 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;
>      }

On CPR target, if libvirt (mistakenly) skips ueventfd param in cmdline,
we return 'true' here, socket remains open, and there's no resize
detection.  I'd prefer spitting an error loud and clear in such a case.

>  
> -    s->uevent_fd = monitor_fd_param(monitor_cur(), s->conf.ueventfd, errp);
> -    if (s->uevent_fd < 0) {
> -        error_prepend(errp, "vhost-blk: unable to obtain uevent socket: ");
> -        return false;
> +    cpr_name = vhost_blk_cpr_name(s);
> +
> +    /*
> +     * A unique ID is mandatory for preserving the uevent socket across
> +     * CPR migration, so block CPR modes when it is absent.
> +     */
> +    if (!cpr_name) {
> +        error_setg(&s->migration_blocker,
> +                   "vhost-blk: device ID is required for CPR migration");
> +        if (migrate_add_blocker_modes(&s->migration_blocker, errp,
> +                                      MIG_MODE_CPR_TRANSFER,
> +                                      MIG_MODE_CPR_EXEC, -1) < 0) {
> +            return false;
> +        }
> +    }
> +
> +    if (cpr_is_incoming()) {
> +        if (!cpr_name) {
> +            error_setg(errp,
> +                       "vhost-blk: device ID is required for CPR migration");
> +            goto fail;
> +        }
> +        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");
> +            goto fail;
> +        }
> +    } else {
> +        s->uevent_fd = monitor_fd_param(monitor_cur(), s->conf.ueventfd, errp);
> +        if (s->uevent_fd < 0) {
> +            error_prepend(errp, "vhost-blk: unable to obtain uevent socket: ");
> +            goto fail;
> +        }
>      }
>  
>      if (!vhost_blk_uevent_check(s->uevent_fd, s->conf.ueventfd, errp)) {

Nit: on CPR target, s->conf.ueventfd is likely to be a placeholder string.

> -        qemu_close(s->uevent_fd);
> -        s->uevent_fd = -1;
> -        return false;
> +        goto fail;
> +    }
> +
> +    if (cpr_name && !cpr_is_incoming()) {
> +        /* 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);
>      qemu_set_fd_handler(s->uevent_fd, vhost_blk_uevent_read, NULL, s);
>      return true;
> +
> +fail:
> +    if (s->uevent_fd >= 0) {
> +        if (cpr_name && cpr_is_incoming()) {
> +            cpr_delete_fd(cpr_name, 0);
> +        }
> +        qemu_close(s->uevent_fd);
> +        s->uevent_fd = -1;
> +    }
> +    migrate_del_blocker(&s->migration_blocker);

On cpr_is_incoming() && !cpr_name, we're gonna add blocker, goto fail,
then delete blocker.  Redundant.

> +    return false;
>  }

Consider the following overall shape (designed by Claude, so please
recheck):

  static bool vhost_blk_uevent_attach(VHostBlk *s, Error **errp)
  {
      g_autofree char *cpr_name = vhost_blk_cpr_name(s);
      bool incoming = cpr_is_incoming();
      int saved_fd = -1;

      if (incoming && cpr_name) {
          saved_fd = cpr_find_fd(cpr_name, 0);
      }

      if (!s->conf.ueventfd) {
          if (saved_fd >= 0) {
              error_setg(errp, "vhost-blk: a preserved uevent socket exists "
                         "but the ueventfd property is not set");
              return false;
          }
          return true;
      }

      if (incoming) {
          if (saved_fd < 0) {
              error_setg(errp, "vhost-blk: no preserved uevent socket to "
                         "restore%s", cpr_name ? "" : " (device has no ID)");
              return false;
          }
          s->uevent_fd = saved_fd;
      } else {
          s->uevent_fd = monitor_fd_param(monitor_cur(), s->conf.ueventfd, errp);
          if (s->uevent_fd < 0) {
              error_prepend(errp, "vhost-blk: unable to obtain uevent socket: ");
              return false;
          }
      }

      if (!vhost_blk_uevent_check(s->uevent_fd,
                                  incoming ? cpr_name : s->conf.ueventfd, errp)) {
          goto fail;
      }

      if (!incoming) {
          if (cpr_name) {
              /* Preserve the socket across a future CPR migration. */
              cpr_save_fd(cpr_name, 0, s->uevent_fd);
          } else {
              error_setg(&s->migration_blocker,
                         "vhost-blk: device ID is required for CPR migration");
              if (migrate_add_blocker_modes(&s->migration_blocker, errp,
                                            MIG_MODE_CPR_TRANSFER,
                                            MIG_MODE_CPR_EXEC, -1) < 0) {
                  goto fail;
              }
          }
      }

      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;

  fail:
      if (incoming) {
          cpr_delete_fd(cpr_name, 0);
      }
      qemu_close(s->uevent_fd);
      s->uevent_fd = -1;
      return false;
  }

Andrey

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