Re: [PATCH hci-8.1 v3 3/4] vhost-blk: create kernel threads with the mainstream worker ioctls

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

On 10/28/26 2:33 PM, Alexander Ivanov wrote:
> num-threads was passed to our own VHOST_SET_NWORKERS kernel ioctl. The
> kernel drops it with the revert of "drivers/vhost: add ioctl to
> increase the number of workers" in favour of the mainstream worker
> ioctls, which vhost-blk now supports. Create num-threads - 1 workers
> with VHOST_NEW_WORKER and attach them to the virtqueues round-robin
> with VHOST_ATTACH_VRING_WORKER, the layout VHOST_SET_NWORKERS produced.
> 
> Our kernel, unlike mainline, defaults to kthread mode, where
> VHOST_NEW_WORKER fails with -EFAULT rather than -ENOTTY. So switch the
> device to vhost_task mode with VHOST_SET_FORK_FROM_OWNER before
> VHOST_SET_OWNER. With num-threads=1 nothing changes: no worker ioctls,
> kthread mode.
> 
> Released vz10 kernels have VHOST_SET_FORK_FROM_OWNER, but their
> vhost-blk does not handle the worker ioctls and fails VHOST_NEW_WORKER
> with -ENOTTY. On such a kernel, or one without
> VHOST_SET_FORK_FROM_OWNER, warn and run a single thread rather than
> fail to start vhost-blk.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  hw/block/vhost-blk.c | 100 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 86 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index 8a015d54fa7..cc05eb057df 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -33,9 +33,60 @@
>  #include "migration/cpr.h"
>  #include "system/runstate.h"
>  
> +/*
> + * Spread the virtqueues round-robin over nworkers kernel threads. Thread 0 is
> + * the default worker VHOST_SET_OWNER created; vq 0 and every nworkers-th vq
> + * after it stay on it.
> + */
> +static int vhost_blk_set_workers(struct vhost_dev *hdev, int nworkers)
> +{
> +    uint32_t ids[VHOST_BLK_MAX_QUEUES];
> +    int i, ret;
> +
> +    assert(nworkers <= ARRAY_SIZE(ids));
> +
> +    for (i = 1; i < hdev->nvqs; i++) {
> +        int w = i % nworkers;
> +        struct vhost_vring_worker vq_worker = { .index = i };
> +
> +        if (w == 0) {
> +            continue;
> +        }
> +
> +        /* vq i < nworkers is the first user of thread i: create it here */
> +        if (i < nworkers) {
> +            struct vhost_worker_state worker = { };
> +
> +            ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
> +            if (ret == -ENOTTY) {
> +                /* vhost-blk predating worker ioctls, fails on vq 1 only */
> +                warn_report_once("vhost-blk: kernel does not support worker "
> +                                 "ioctls, using a single kernel thread");
> +                return 0;

AFAIU Sashiko is right about failure not always being indicated by
-ENOTTY.  We might get EPERM, ENOBUFS, EINVAL, EFAULT.

Maybe let's just stop keying on exact error:

>       ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev, VHOST_FORK_OWNER_TASK);
>       if (ret < 0) {
>           warn_report_once("vhost-blk: unable to use multiple kernel threads: %s",
>                            strerror(-ret));
>           nworkers = 1;
>       }

And the same for vhost_new_worker() inside the loop.

@Andrey Zhadchenko can you confirm that's the right error-handling
approach here?

> +            } else if (ret < 0) {
> +                error_report("vhost-blk: unable to create kernel thread: %s",
> +                             strerror(-ret));
> +                return ret;
> +            }
> +            ids[w] = worker.worker_id;
> +        }
> +
> +        vq_worker.worker_id = ids[w];
> +        ret = hdev->vhost_ops->vhost_attach_vring_worker(hdev, &vq_worker);
> +        if (ret < 0) {
> +            error_report("vhost-blk: unable to attach kernel thread to "
> +                         "queue %d: %s", i, strerror(-ret));
> +            return ret;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_blk_start(VirtIODevice *vdev)
>  {
>      VHostBlk *s = VHOST_BLK(vdev);
> +    struct vhost_dev *hdev = &s->dev;
>      struct vhost_vring_file backend;
>      int ret, i, nworkers;
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> @@ -51,15 +102,36 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          return 0;
>      }
>  
> +    nworkers = MIN(s->conf.num_threads, hdev->nvqs);
> +    if (nworkers > 1) {
> +        /*
> +         * VHOST_NEW_WORKER needs vhost_task mode; our kernel defaults to
> +         * kthreads and VHOST_RESET_OWNER brings that default back, so ask
> +         * on every start. Must precede VHOST_SET_OWNER.
> +         */
> +        ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
> +                                                         VHOST_FORK_OWNER_TASK);
> +        if (ret == -ENOTTY) {
> +            warn_report_once("vhost-blk: kernel does not support vhost tasks, "
> +                             "using a single kernel thread");
> +            nworkers = 1;
> +        } else if (ret < 0) {
> +            error_report("vhost-blk: unable to set vhost task mode: %s",
> +                         strerror(-ret));
> +            return ret;
> +        }
> +    }
> +
>      if (ioctl(s->vhostfd, VHOST_SET_OWNER, NULL)) {
>          error_report("vhost-blk: unable to set owner");
>          return -ENOSYS;
>      }
>  
> -    nworkers = s->conf.num_threads;
> -    if (nworkers != 1 && ioctl(s->vhostfd, VHOST_SET_NWORKERS, &nworkers)) {
> -        error_report("vhost-blk: unable to set number of kernel threads");
> -        return -EINVAL;
> +    if (nworkers > 1) {
> +        ret = vhost_blk_set_workers(hdev, nworkers);
> +        if (ret < 0) {
> +            return ret;
> +        }
>      }
>  
>      if (s->conf.serial) {
> @@ -70,22 +142,22 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          }
>      }
>  
> -    ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true);
> +    ret = k->set_guest_notifiers(qbus->parent, hdev->nvqs, true);

Nit: these s->dev -> hdev replacements generally don't belong here as
they aren't part of the changing logic.  Split it into another patch
(maybe covering stop()/reset() as well?) and add 'Fixes: ...' tag, so I
can squash it during the next rebase keeping only the meaningful history.

Andrey