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

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

On 10/31/26 5:35 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. Do it for num-threads=1 as well, so the default
> thread is of the same kind whatever the number of workers.
> 
> Fail to start vhost-blk if any of these ioctls fails rather than
> silently run on fewer threads than requested. Released vz10 kernels
> have VHOST_SET_FORK_FROM_OWNER, but their vhost-blk does not handle
> the worker ioctls, so num-threads above 1 now requires a kernel that
> does.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-107537
> 
> Signed-off-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> ---
>  hw/block/vhost-blk.c | 62 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/vhost-blk.c b/hw/block/vhost-blk.c
> index b3eb181ceae..3cde12051fb 100644
> --- a/hw/block/vhost-blk.c
> +++ b/hw/block/vhost-blk.c
> @@ -33,6 +33,50 @@
>  #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;
> +        }
> +
> +        if (i < nworkers) {
> +            struct vhost_worker_state worker = { };
> +
> +            ret = hdev->vhost_ops->vhost_new_worker(hdev, &worker);
> +            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);
> @@ -52,15 +96,25 @@ static int vhost_blk_start(VirtIODevice *vdev)
>          return 0;
>      }
>  
> +    ret = hdev->vhost_ops->vhost_set_fork_from_owner(hdev,
> +                                                     VHOST_FORK_OWNER_TASK);
> +    if (ret < 0) {
> +        error_report("vhost-blk: unable to set vhost_task mode: %s",
> +                     strerror(-ret));
> +        return ret;
> +    }

Note, with this newly introduced strict retval checking:

> long vhost_dev_ioctl()
> {
>     if (ioctl == VHOST_SET_OWNER) { ... }
> 
> #ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
>     if (ioctl == VHOST_SET_FORK_FROM_OWNER) { ... }

That means any vhost-blk disk will fail to start on an (older) kernel
which lacks VHOST_SET_FORK_FROM_OWNER.

This shouldn't be an issue for us, but still worth keeping in mind.
E.g. our vzkernel 6.12.0-55.* didn't have this setting AFAICT.
> +
>      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;
> +    nworkers = MIN(s->conf.num_threads, hdev->nvqs);
> +    if (nworkers > 1) {
> +        ret = vhost_blk_set_workers(hdev, nworkers);
> +        if (ret < 0) {
> +            return ret;

Pre-existing, but each return after VHOST_SET_OWNER above leaves the
device in a stale state.  This might deserve another fix, separately
from this patch.
> +        }
>      }
>  
>      if (s->conf.serial) {

LGTM overall.

Reviewed-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>